Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: add package for client sdk #45

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ node_modules
.env*
dist
.DS_Store
*.pem
*.pem
yarn-error.log
1 change: 1 addition & 0 deletions packages/relay-sdk/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
index.js
5 changes: 5 additions & 0 deletions packages/relay-sdk/.npmignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Ignore all files and directories starting with a dot
.*

# Build related stuff
node_modules/
44 changes: 44 additions & 0 deletions packages/relay-sdk/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# Relay SDK

## Description

The Lit Network Relay SDK is a lightweight JavaScript library that can be used for setting up payment delegations using the Lit Network Relay Server. The SDK provides a simple interface to connect to the Relay server, register a new payer, and add payees.

## Features

- Connect to the Relay server
- Register a new payer
- Add payees to the payer's delegation

## Installation

```bash
npm install @lit-protocol/relay-sdk

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't this package live in the js-sdk repository? It has dependencies on many packages there -- we're going to need to keep it up-to-date with the SDK, and our release process for publishing NPM packages is already defined there and for publishing the relay server to our hosting is already defined here but no client -- this is even listed as an sdk package, and we even have a relayer object defined in the js sdk in the lit-auth-client package.

```

or

```bash
yarn add @lit-protocol/relay-sdk
```

## Usage

### Registering a new payer:

```javascript
import { LitRelayClient } from '@lit-protocol/relay-sdk';

const client = await LitRelayClient.register('habanero', 'you-api-key');
const secret = client.secret;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property was renamed since this was written, I think?

```

### Adding a payee:

```javascript
import { LitRelayClient } from '@lit-protocol/relay-sdk';

const client = await LitRelayClient.connect('habanero', 'you-api-key', 'your-payer-secret');
await client.addPayee('user-wallet-address');
```

16 changes: 16 additions & 0 deletions packages/relay-sdk/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"name": "@litnetwork/relay-sdk",
"version": "0.0.1",
"description": "Lightweight client for interacting with Lit Network relay server.",
"main": "index.js",
"scripts": {
"build": "esbuild src/index.ts --bundle --platform=node --target=node12 --outfile=index.js"
},
"license": "ISC",
"devDependencies": {
"esbuild": "^0.21.4"
},
"dependencies": {
"@lit-protocol/constants": "^6.0.1"
}
}
132 changes: 132 additions & 0 deletions packages/relay-sdk/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@

import { RELAY_URL_HABANERO, RELAY_URL_MANZANO } from '@lit-protocol/constants';
import { assertNotNull, assertStatus, assertString, getProp } from './util';

type SupportedNetworks = 'habanero' | 'manzano';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use LIT_NETWORKS for this instead of our own string values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some way to filter the networks by supported features? I looked into this, but I would have ended up needing to hard code a union anyways since I need to filter out the other networks.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need to define networks w/ specific behaviours, it'd be great to implement those maps in our sdk's constants similarly to how we define some things in lit-core here:
https://github.com/LIT-Protocol/js-sdk/blob/d8c7dc55febb262154061aaac01daad4aae1b239/packages/core/src/lib/lit-core.ts#L105-L117

This is related to my above question -- why not have this package live in the js-sdk? Then we could have single PRs that include updates to constants or other packages and this code as well. A good example of this in action is that we just refactored how we select relayer URLs in the SDK; this code would've been updated as a matter-of-course if it was in the same repo. See PR


function getRelayURLByNetwork(network: SupportedNetworks): string {
switch (network) {
case 'habanero':
return RELAY_URL_HABANERO;
case 'manzano':
return RELAY_URL_MANZANO;
}
}

export class LitRelayClient {

private readonly baseUrl: string;
private readonly apiKey: string;

private payerSecret: string | undefined = undefined;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:nit: can just be ? optional :)

Suggested change
private payerSecret: string | undefined = undefined;
private payerSecret?: string


/**
* Create a new LitRelayClient instance. Requires that the payer is already registered.
* and the the payer secret is known.
*
* ```typescript
* const client = new LitRelayClient('https://habanero.lit.dev', 'my-api-key', 'my-payer-secret');
* ```
*
* @param baseUrl
* @param apiKey
* @param payerSecret
*/
private constructor(baseUrl: string, apiKey: string, payerSecret: string) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wild -- a constructor can actually be private?! I learned something new today. if this is actually true though, why make it private? Why not allow someone to construct an instance of the object?

this.baseUrl = baseUrl;
this.apiKey = apiKey;

if (payerSecret) {
this.payerSecret = payerSecret;
}
}

/**
* Adds a new payee to the payer's delegation list.
*
* ```typescript
* const client = await LitRelayClient.connect('habanero', 'my-api-key', 'my-payer-secret');
* const result = await client.addPayee('payee-wallet-address');
* ```
* @param payeeAddress
*
* @returns Promise<{ tokenId: string } | Error>
*/
public addPayee(payeeAddress: string): Promise<{ tokenId: string }> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change this function to be addPayees given that the backend API supports that? Otherwise we will inevitably end up with 2 methods, and this method will end up either calling the addPayees method or duplicating the code inside of that method.

if (!this.payerSecret) {
return Promise.reject('Payer secret key is missing');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See other comment -- async this method and always throw an Error instance please 💖

}

return fetch(`${this.baseUrl}/add-users`, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
'api-key': this.apiKey,
'payer-secret-key': this.payerSecret,
},
body: JSON.stringify([payeeAddress]),
})
.then(assertStatus(200, 'Failed to add payee: request failed'))
.then(res => res.json())
.then(json => assertString(json.tokenId, 'Failed to add payee: missing token ID'))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should explicitly type these endpoints in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure what you mean, what 'endpoints' need explicit typing?

.then(tokenId => ({ tokenId }));
}

/**
* Registers a new payer with the Lit Relay server using the provided API key. Returns
* a new LitRelayClient instance with the payer secret key.
*
* ```typescript
* const client = await LitRelayClient.register('habanero', 'my-api-key');
* ```
*
* @param network
* @param apiKey
*
* @returns LitRelayClient
*/
public static register(network: SupportedNetworks, apiKey: string): Promise<LitRelayClient {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple things here:

  1. Rather than imperatively return a settled promise that has been resolved or rejected, this function should just be an async function and use return or throw as necessary. Otherwise we risk the function not returning a Promise if something unexpected happens. For example, the way this is written, if getRelayUrlByNetwork() was to throw for any reason, this function would actually throw an error instead of returning a rejected Promise, and that would mean that anyone using Promise chaining methods on the return value of the function would get an error 'could not call .catch'; or 'could not call then' on {} (whatever was thrown). I recognize that getRelayUrlByNetwork() won't currently throw, but we want to make our code robust and it would surprise someone that throw'ing from that method would make register() no longer return a promise -- an easy avenue for bugs to creep into the codebase when things change later on.
  2. Throwing a Promise.reject() with a string argument is the equivalent of throw'ing a string, which is an antipattern -- we always want to throw Error objects, so we have a stack trace when we catch() it later. By making this function async, you don't need to use Promise.reject() or Promise.resolve() anywhere in the function body; all sync returns and throw's are converted into a Promise with the appropriate state.
  3. Is this function signature missing a > after LitRelayClient? Curious that this would even compile 🤣

if (network !== 'habanero' && network !== 'manzano') {
throw Promise.reject('Invalid network');
}

const baseUrl = getRelayURLByNetwork(network);

return fetch(`${baseUrl}/register-payer`, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
'api-key': apiKey,
},
})
.then(assertStatus(200, 'Failed to register payer: request failed'))
.then(res => res.json())
.then(json => getProp(json, 'payerSecretKey'))
.then(json => assertString(json.payerSecretKey, 'Failed to register payer: missing secret key'))
.then(payerSecretKey => new LitRelayClient(baseUrl, apiKey, payerSecretKey));
}

/**
* Connects to the Relay server for the specified network using the provided API key and payer secret
* and returns a new LitRelayClient instance.
*
* ```typescript
* const client = await LitRelayClient.connect('habanero', 'my-api-key', 'my-payer-secret');
* ```
*
* @param network
* @param apiKey
* @param payerSecret
*
* @returns LitRelayClient
*/
public static connect(network: SupportedNetworks, apiKey: string, payerSecret: string) {
if (network !== 'habanero' && network !== 'manzano') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we run this check in all methods, it'd be nice if it was standardized to a single assert function

throw new Error('Invalid network');
}

const baseUrl = getRelayURLByNetwork(network);

return new LitRelayClient(baseUrl, apiKey, payerSecret);
}
}
25 changes: 25 additions & 0 deletions packages/relay-sdk/src/util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
export function assertStatus(code: number, error: string) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update assertStatus so that rather than ignoring the response entirely in cases where its code doesn't match what we expected, we instead inspect it and include the actual reason why the request fails if the response included it? It would be preferable to inspect the response and only provide a generic string back to the caller if there was nothing returned by the backend service to pass along to the user. There's an example of this pattern here

return (response: Response) => {
if (response.status !== code) {
throw new Error(error);
}

return response;
};
}

export function assertNotNull(value: any, error: string) {
if (value === null || value === undefined) {
throw new Error(error);
}

return value;
}

export function assertString(value: any, error: string) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:nit: This could be a TS assertion function instead of using as on the return

Suggested change
export function assertString(value: any, error: string) {
export function assertString(value: any, error: string) asserts value is string {

Ref: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#:~:text=The%20other%20type%20of%20assertion,property%20has%20a%20different%20type.&text=Here%20asserts%20val%20is%20string,known%20to%20be%20a%20string%20.&text=assertIsString(str)%3B

if (typeof value !== "string") {
throw new Error(error);
}

return value as string;
}
Loading