-
Notifications
You must be signed in to change notification settings - Fork 407
feat(clerk-js,types,clerk-react): Support Coinbase Wallet web3 provider and authentication strategy #4082
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
feat(clerk-js,types,clerk-react): Support Coinbase Wallet web3 provider and authentication strategy #4082
Conversation
🦋 Changeset detectedLatest commit: 44de48c The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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 |
| "@clerk/localizations": "2.8.0", | ||
| "@clerk/shared": "2.6.1", | ||
| "@clerk/types": "4.18.0", | ||
| "@coinbase/wallet-sdk": "4.0.4", |
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.
Note to reviewers: For now this is lazy loaded with the help of @panteliselef
octoper
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.
🔥
|
!snapshot |
c106fbc to
fb4e005
Compare
fb4e005 to
e74b609
Compare
panteliselef
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.
Only minor feedback and I added some info about the other reviews
| const generateSignature = | ||
| provider === 'metamask' | ||
| ? generateSignatureWithMetamask | ||
| : provider === 'coinbase' | ||
| ? generateSignatureWithCoinbase | ||
| : generateSignatureWithCoinbaseWallet; |
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.
In a follow up PR this will become simpler as we are dropping the generateSignatureWithCoinbase ?
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.
That's correct, on the next PR we are dropping the Coinbase provider in favor of the new Coinbase Wallet
| id: 'coinbase', | ||
| name: 'Coinbase Wallet', | ||
| }, | ||
| coinbase_wallet: { |
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.
🙃 SW = smart wallet
| coinbase_wallet: { | |
| coinbase_sw: { |
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.
Coinbase prefer to use the term Coinbase Wallet rather than Coinbase Smart Wallet
| cacheGroups: { | ||
| zxcvbnTSCoreVendor: { | ||
| test: /[\\/]node_modules[\\/](@zxcvbn-ts)[\\/](core)[\\/]/, | ||
| test: /[\\/]node_modules[\\/](@zxcvbn-ts\/core|fastest-levenshtein)[\\/]/, |
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.
This is unrelated, but it is minor so it's ok
| chunks: 'all', | ||
| }, | ||
| coinbaseWalletSDKVendor: { | ||
| test: /[\\/]node_modules[\\/](@coinbase\/wallet-sdk|ieee754|preact|keccak|buffer|string_decoder|sha\.js|base64-js|safe-buffer|util-deprecate|inherits)[\\/]/, |
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.
We are manually grouping together all deps that are only used from the coinbase wallet sdk in order to avoid puting them in the vendors chunk
packages/types/src/clerk.ts
Outdated
| authenticateWithCoinbase: (params?: AuthenticateWithCoinbaseParams) => Promise<unknown>; | ||
|
|
||
| /** | ||
| * Authenticates user using their Coinbase Wallet Smart Wallet and browser extension |
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.
Should this be
| * Authenticates user using their Coinbase Wallet Smart Wallet and browser extension | |
| * Authenticates user using their Coinbase Smart Wallet and Wallet browser extension |
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.
Oops nice catch!
e74b609 to
c0da1a2
Compare
…vider and authentication strategy Coinbase Wallet is a new generic web3 provider and strategy that supports both Smart Wallets and Coinbase Wallet browser extension. Users can use it to authenticate themselves using their own web3 address as an identifier
c0da1a2 to
44de48c
Compare
Description
Coinbase Wallet is a new generic web3 provider and strategy that supports both Smart Wallets and Coinbase Wallet browser extension. Users can use it to authenticate themselves using their own web3 address as an identifier
In order to take advantage of the Coinbase Wallet we are lazy loading
@coinbase/wallet-sdkand all its direct dependencies. Resulting in a new chunk ~58KB which will only be loading when the end user selects that provider.Checklist
npm testruns as expected.npm run buildruns as expected.Type of change