-
Notifications
You must be signed in to change notification settings - Fork 12
[BA-2211] Update the SDK constructor to accept the new originVerification property #35
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
Conversation
- domain verification key generation script for well known file - yarn generate-key-script - readme doc on how to use it
packages/client/src/MWPClient.ts
Outdated
| * Request a nonce for origin verification | ||
| * @returns Promise<string> - The nonce provided by the wallet | ||
| */ | ||
| async getNonce(): Promise<string> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm we probably don't need this func if we have MWPClient.request({ method: 'wallet_getNonce' })?
packages/client/src/MWPClient.ts
Outdated
| type MWPClientOptions = { | ||
| metadata: AppMetadata; | ||
| wallet: Wallet; | ||
| originVerification?: OriginVerification; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use the name domainVerification instead. It's more consistent with how we're talking about this feature in other places
packages/client/src/MWPClient.ts
Outdated
| case 'wallet_getNonce': | ||
| // For now, generate a UUID locally instead of calling the wallet API | ||
| return crypto.randomUUID(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we actually need to define it here. A developer won't need to wallet_getNonce directly. Rather our SDK will call our own nonce endpoint and pass it into the generateDomainSignature function that the developer supplies
fan-zhang-sv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
| console.log(` • ${jwksPath} - JWKS file for your domain`); | ||
| console.log(` • ${privateKeyPath} - Private key (keep this secure!)\n`); | ||
|
|
||
| console.log('🌐 Next steps:'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it makes sense to have these log in the script tho, maybe we can disable this rule for /scripts/ folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to remove them because they kept failing the CI
Summary
https://linear.app/coinbase/issue/BA-2211/wsdk-update-the-sdk-constructor-to-accept-the-new-originverification
Added the optional originVerification property to the MWPClient sdk, as well as the EIP1193Provider, and the wagmi constructor. Made a dummy getNonce function (which we will replace to call the actual API) that returns a uuid. Wrote some testing code for it in the MWPClient.test.ts file.
How did you test your changes?
Added testing code for it in the MWPClient.test.ts and ran yarn test to ensure everything passed.