feature: add package for client sdk #45

jtary · 2024-06-07T19:10:57Z

Adds a lightweight library people can use for interacting with the new delegation features.

packages/relay-sdk/src/index.ts

joshLong145 · 2024-06-12T12:34:05Z

packages/relay-sdk/package.json

+    "esbuild": "^0.21.4"
+  },
+  "dependencies": {
+    "@lit-protocol/constants": "^6.0.0"


why are we using 6.0.0 and not 6.0.1

Hmm, not sure why it decided to go with that. Functionally, they should be the same, since it will grab the most recent compatible version, but I can bump the patch version just to be explicit.

joshLong145 · 2024-06-12T12:34:49Z

packages/relay-sdk/src/index.ts

+        })
+            .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'))


We should explicitly type these endpoints in the future.

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

joshLong145 · 2024-06-12T12:35:41Z

packages/relay-sdk/src/index.ts

+import { RELAY_URL_HABANERO, RELAY_URL_MANZANO } from '@lit-protocol/constants';
+import { assertNotNull, assertStatus, assertString, getProp } from './util';
+
+type SupportedNetworks = 'habanero' | 'manzano';


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

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.

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

MaximusHaximus

Sorry for the long delay in reviewing this -- I've added feedback/comments in-line :)

MaximusHaximus · 2024-06-11T19:05:05Z

packages/relay-sdk/src/index.ts

+            throw new Error('Failed to register payer: missing secret key');
+        }
+
+        return new LitRelayClient(baseUrl, apiKey, data.payerSecretKey);


payerSecret is private (so shouldn't be referenced), and there is no getter to expose it... but consumers of this client will need that secret later on in order to add more payees -- and likely they will need to do so at some point in the future when this instance has gone out of scope. My understanding is that they will need to call register() somewhere and then stash that secret somewhere for future use.

If that's true, rather than create a new instance with the secret injected, can we just return the secret to the caller so that they can either store it for later use or use it to create a new instance? Even better, they could call setPayerSecretKey(payerSecretKey) on the instance that they already have rather than having 2 instances in scope -- one with only the apiKey and another with both the apiKey and the payerSecretKey.

MaximusHaximus · 2024-06-11T19:06:56Z

packages/relay-sdk/README.md

+import { LitRelayClient } from '@lit-protocol/relay-sdk';
+
+const client = await LitRelayClient.register('habanero', 'you-api-key');
+const secret = client.secret;


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

MaximusHaximus · 2024-06-11T19:08:27Z

packages/relay-sdk/README.md

+## Installation
+
+```bash
+npm install @lit-protocol/relay-sdk


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.

MaximusHaximus · 2024-06-11T19:09:41Z

packages/relay-sdk/src/index.ts

+    private readonly baseUrl: string;
+    private readonly apiKey: string;
+
+    private payerSecret: string | undefined = undefined;


:nit: can just be ? optional :)

Suggested change

private payerSecret: string | undefined = undefined;

private payerSecret?: string

MaximusHaximus · 2024-06-11T19:17:01Z

packages/relay-sdk/src/index.ts

+     * @param apiKey
+     * @param payerSecret
+     */
+    private constructor(baseUrl: string, apiKey: string, payerSecret: string) {


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?

MaximusHaximus · 2024-07-11T20:18:58Z

packages/relay-sdk/src/index.ts

+     */
+    public addPayee(payeeAddress: string): Promise<{ tokenId: string }> {
+        if (!this.payerSecret) {
+            return Promise.reject('Payer secret key is missing');


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

MaximusHaximus · 2024-07-11T20:19:58Z

packages/relay-sdk/src/index.ts

+     * 
+     * @returns Promise<{ tokenId: string } | Error>
+     */
+    public addPayee(payeeAddress: string): Promise<{ tokenId: string }> {


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.

MaximusHaximus · 2024-07-11T20:24:48Z

packages/relay-sdk/src/util.ts

@@ -0,0 +1,25 @@
+export function assertStatus(code: number, error: string) {


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

MaximusHaximus · 2024-07-11T20:26:16Z

packages/relay-sdk/src/util.ts

+    return value;
+}
+
+export function assertString(value: any, error: string) {


: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

MaximusHaximus · 2024-07-11T20:28:18Z

packages/relay-sdk/src/index.ts

+     * @returns LitRelayClient
+     */
+    public static connect(network: SupportedNetworks, apiKey: string, payerSecret: string) {
+        if (network !== 'habanero' && network !== 'manzano') {


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

MaximusHaximus · 2024-07-11T20:33:02Z

packages/relay-sdk/src/index.ts

+
+        const data = await res.json();
+
+        if (!data.payerSecretKey) {


If the API is going to return payerSecretKey, can we name it the same thing on this object? It'd be nice for the same magic strings to be used in both the client and server contexts for easier discoverability

jtary added 7 commits June 7, 2024 12:05

feature: add package for client sdk

055e58a

feat: add basic readme for relay-sdk

e7e5328

fix: remove build file

c581334

fix: relocate readme file

fca7920

fix: add build file to gitingore

4f295aa

fix: invalid use of the addPayee method

7a54b2a

fix: issues with endpoint address

bf3264d

jtary requested review from Ansonhkg, joshLong145 and MaximusHaximus June 7, 2024 20:31

joshLong145 reviewed Jun 11, 2024

View reviewed changes

packages/relay-sdk/src/index.ts Outdated Show resolved Hide resolved

packages/relay-sdk/src/index.ts Outdated Show resolved Hide resolved

packages/relay-sdk/src/index.ts Outdated Show resolved Hide resolved

joshLong145 reviewed Jun 11, 2024

View reviewed changes

packages/relay-sdk/src/index.ts Outdated Show resolved Hide resolved

jtary added 2 commits June 11, 2024 10:07

fix: use constants package instead of hard-code urls

d180053

feat: remove exceptions from async funcs

27b8e27

joshLong145 reviewed Jun 12, 2024

View reviewed changes

jtary added 2 commits June 12, 2024 08:07

chore: bump constants packages version

6bb37ce

fix: add missing util file

c69510c

MaximusHaximus requested changes Jul 11, 2024

View reviewed changes

MaximusHaximus reviewed Jul 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add package for client sdk #45

feature: add package for client sdk #45

jtary commented Jun 7, 2024

joshLong145 Jun 12, 2024

jtary Jun 12, 2024

joshLong145 Jun 12, 2024

jtary Jun 12, 2024

joshLong145 Jun 12, 2024

jtary Jun 12, 2024

MaximusHaximus Jul 11, 2024

MaximusHaximus left a comment

MaximusHaximus Jun 11, 2024

MaximusHaximus Jun 11, 2024

MaximusHaximus Jun 11, 2024

MaximusHaximus Jun 11, 2024

MaximusHaximus Jun 11, 2024

MaximusHaximus Jul 11, 2024

MaximusHaximus Jul 11, 2024

MaximusHaximus Jul 11, 2024

MaximusHaximus Jul 11, 2024

MaximusHaximus Jul 11, 2024

MaximusHaximus Jul 11, 2024

	private payerSecret: string \| undefined = undefined;
	private payerSecret?: string

		@@ -0,0 +1,25 @@
		export function assertStatus(code: number, error: string) {

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

feature: add package for client sdk #45

Are you sure you want to change the base?

feature: add package for client sdk #45

Conversation

jtary commented Jun 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaximusHaximus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment