-
Notifications
You must be signed in to change notification settings - Fork 158
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: integrate Coinbase Onramp for funding EOA wallets #1285
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@steveviselli-cb is attempting to deploy a commit to the Coinbase Team on Vercel. A member of the Team first needs to authorize it. |
@@ -0,0 +1,82 @@ | |||
import { useCallback, useMemo } from 'react'; |
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 whole component was just copy/pasted out of the original fund link component. I just added the popup size override props.
@@ -0,0 +1,21 @@ | |||
import { version } from '../../version'; |
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 code was just extracted out of the original wallet dropdown fund link and moved into a util.
* | ||
* Note: exported as public Type | ||
*/ | ||
export type GetOnrampUrlWithProjectIdParams = { |
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 we rename Onramp
to Fund
wherever possible to avoid confusion?
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.
@cpcramer what I'm trying to make clear here is that it's specific to Coinbase Onramp the product. There could be multiple funding providers, actually there are, there's the keys.coinbase.com one and the Onramp one. Each need different params. That's why I think the distinction is important here. "Fund" is the name of the component that is abstracting away the details of what funding provider you are using. Onramp is one of those providers. Does that make sense?
// If the connected wallet's active chain is not included in the Wagmi config, accountChain will be undefined. If this | ||
// is the case, fall back to the default chain specified in the OnchainKit config. |
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.
Love all the comments in general 😄
What changed? Why?
The current
WalletDropdownFundLink
always takes users tohttps://keys.coinbase.com/fund
regardless of wether the connected wallet is actually a Coinbase Smart Wallet or not. This PR updates theWalletDropdownFundLink
to only take users tohttps://keys.coinbase.com/fund
if the connected wallet is a Coinbase Smart Wallet. Otherwise it will send users to Coinbase Onramp i.e.https://pay.coinbase.com/buy
.Notes to reviewers
I refactored
WalletDropdownFundLink
into a few smaller components to reduce component complexity and make testing much easier. But I did not modify the original code for rendering a Coinbase Smart Wallet fund link.I'm using the combination of Coinbase Wallet Wagmi connector ID (defined here) and atomicBatch capability support to determine if the connected wallet is a Coinbase Smart Wallet.
For users in the US, the Coinbase Onramp UI will give users the option to continue as a guest if they do not have a Coinbase account. You can't see it in the below video because I'm in Canada.
How has it been tested?
Tested in playground.
Screen.Recording.2024-09-22.at.10.59.42.AM.mov