- 
                Notifications
    
You must be signed in to change notification settings  - Fork 36
 
feat(checkout-widgets): Add allowlistWalletRdns option to Checkout Widgets #2715
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(checkout-widgets): Add allowlistWalletRdns option to Checkout Widgets #2715
Conversation
| 
           View your CI Pipeline Execution ↗ for commit cba1ac8 
 ☁️ Nx Cloud last updated this comment at   | 
    
| targetChainId: ChainId; | ||
| walletProviderName?: WalletProviderName; | ||
| browserProvider?: WrappedBrowserProvider; | ||
| walletProviderName: WalletProviderName | undefined; | 
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.
IMO preferable in this case because it makes it explicit that we are not passing a value through (for example in packages/checkout/widgets-lib/src/widgets/immutable-commerce/functions/getConnectLoaderParams.ts)
| allowedChains={allowedChains ?? [targetChain]} | ||
| blocklistWalletRdns={blocklistWalletRdns} | ||
| allowlistWalletRdns={allowlistWalletRdns} | ||
| blocklistWalletRdns={blocklistWalletRdns ?? []} | 
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.
Default to [] here so that we reduce complexity by not having to deal with undefined lower in the stack
| browserProvider: this.browserProvider, | ||
| checkout: this.checkout, | ||
| allowedChains: [this.checkout.config.l1ChainId, this.checkout.config.l2ChainId], | ||
| allowlistWalletRdns: undefined, | 
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 feature is not supported on legacy widgets (except for on the ConnectWidget)
| checkout: this.checkout, | ||
| allowedChains: [this.checkout.config.l2ChainId], | ||
| allowlistWalletRdns: undefined, | ||
| walletProviderName: undefined, | 
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.
Interesting that we're not passing along this.parameters.walletProviderName, like in the other widget roots, not sure what the implication of that is...
Hi👋, please ensure the PR title follows the below standards:
type(scope): message. For example:feat(passport): my new feature!after thetype(scope), for examplefeat(passport)!: my new breaking featureSummary
Allows the Commerce widgets to be restricted to Passport-only.
Detail and impact of the change
Example usage:
Added
Changed
Deprecated
Removed
Fixed
Security
Anything else worth calling out?