-
Notifications
You must be signed in to change notification settings - Fork 152
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: EIP-5792 in OnchainKitProvider
#1181
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/useCapabilitiesSafe.ts
Outdated
|
||
// Metamask doesn't support wallet_getCapabilities | ||
const isMetamaskWallet = connector?.id === 'io.metamask'; | ||
const enabled = isConnected && !isMetamaskWallet; |
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.
optionally we can also just check for error
returned from useCapabilities
but there will still be an error displayed in the console for unsupported RPCs - IMO this is a more scalable solution than manually excluding individual connector IDs
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.
cc @alissacrane-cb on Tanstack query best practices
src/useCapabilitiesSafe.ts
Outdated
import type { WalletCapabilities } from './types'; | ||
|
||
export function useCapabilitiesSafe({ | ||
chain, |
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.
Going forward we are moving away from using chain
, instead we will have directly chainId
.
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.
changed in latest diff
paymaster: boolean; // If the wallet supports ERC-4337 Paymasters for gas sponsorship | ||
batching: boolean; // If the wallet supports atomic batching of transactions | ||
funding: boolean; // If the wallet supports auxiliary funding of accounts (e.g. Magic Spend) | ||
}; |
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.
mmmm if those are boolean, probably we should use the has
prefix
export type WalletCapabilities = {
hasPaymasterSupport: boolean;
hasBatchingSupport: boolean;
hasFundingSupport: boolean;
};
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 continue this discussion on Wilson's comment below
export function useCapabilitiesSafe({ | ||
chainId, | ||
}: UseCapabilitiesSafeParams): WalletCapabilities { | ||
const { isConnected } = useAccount(); |
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.
Btw as follow up PR, check with @abcrane123 if this will cause a SSR issue.
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 this will be an issue
src/useCapabilitiesSafe.ts
Outdated
return { | ||
hasPaymasterService: capabilities[chainId]?.paymasterService?.supported, | ||
hasAtomicBatch: capabilities[chainId]?.atomicBatch?.supported, | ||
hasAuxiliaryFunds: capabilities[chainId]?.auxiliaryFunds?.supported, |
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.
if these can be undefined, should they be cast to a boolean?
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 be fine per this test?
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 just tests if !capabilities though right? is it possible for useCapabilities to return some of the capabilities, but not all?
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.
good catch :) fixed in the latest diff!
What changed? Why?
useCapabilitiesSafe
hookOnchainKitProvider
for consumption in child componentsNotes to reviewers
How has it been tested?
unit tests and console logging the capabilities on Playground