Conversation
🦋 Changeset detectedLatest commit: 1ec7267 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| /** @deprecated Use `client` instead. */ | ||
| walletAddressUrl?: string | ||
| /** The client identifier for grant requests. Provide either a JWK or a wallet address URL. */ | ||
| client?: ClientIdentifier |
There was a problem hiding this comment.
I also considered not deprecating walletAddressUrl and instead making it optional alongside a new optional jwk.
My thinking with new optional client and making walletAddressUrl optional is that it will enforce mutual exclusivity at the type level in the future. After we give time to migrate we can remove walletAddressUrl and require client. Although I dont hate optional walletAddressUrl and jwk with run-time checks tbh. Thoughts?
Also I chose the name client here for consistency with our internals but I also considered identifier, "clientIdentifier" etc. And I think from the client user's perspective they wont necessarily know anything about the internal representation as client (the open api spec, auth db field name, etc.) so maybe there is a case to be made for one of those names?
There was a problem hiding this comment.
Fields like client and identifier are derived from their meaning in GNAP, so I would suppose that changing this field name would mean we don't need the OP SDK to pass through GNAP concepts directly.
There was a problem hiding this comment.
In principal I dont necessarily think the OP SDK needs to expose the GNAP concepts. I guess to this point we already didn't exposethis GNAP concept directly - walletAddressUrl as opposed to client. But in practice the client does mirror GNAP concepts in a lot of ways and probably unavoidable (the whole concept of grant requests, access items, etc.). And I do want to avoid using the same names to mean something entirely different. Perhaps identifier would do that. It looks like client does encapsulate both the key itself or some reference to it. Maybe we leave as-is.
| body: { | ||
| ...args, | ||
| client | ||
| client: |
There was a problem hiding this comment.
Should this also handle if someone is using client: { walletAddressUrl } as well instead of in the backwards-compatible way?
There was a problem hiding this comment.
I believe it handles all cases as-is but I'm not sure if I'm completely following. So correct me if I have something wrong.
createAuthenticatedClient supports passing in either the walletAddressUrl: string or a client like:
type ClientIdentifier =
| { jwk: JWK; walletAddressUrl?: never }
| { walletAddressUrl: string; jwk?: never }If the client is initialized in the old way with a walletAddressUrl string, it will get converted to the ClientIdentifier form before this point (line 370 in index.ts). Then it's passed into createGrantRoutes and injected into the post body here - always as the object.
So we arent using the full range of options the api spec allows for the actual request - just the object, not the string. Functionally its the same and it saves migration away from passing the string later. But in terms of initializing the client, any form can be used.
| /** @deprecated Use `client` instead. */ | ||
| walletAddressUrl?: string | ||
| /** The client identifier for grant requests. Provide either a JWK or a wallet address URL. */ | ||
| client?: ClientIdentifier |
There was a problem hiding this comment.
Fields like client and identifier are derived from their meaning in GNAP, so I would suppose that changing this field name would mean we don't need the OP SDK to pass through GNAP concepts directly.
2 participants
Changes proposed in this pull request
Context
fixes #21