feat: add transaction builder functions for interacting with Morpho vaults #1852
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| export function useEarnContext() { | ||
| const context = useContext(EarnContext); | ||
| if (!context) { | ||
| throw new Error('useEarnContext must be used within an EarnProvider'); | ||
| } | ||
| return context; | ||
| } |
There was a problem hiding this comment.
our pattern has not been to throw if useComponentContexts have been used outside of their providers. Perhaps that's bad practice, but it's useful in certain cases (eg. if you want to attempt to use a context, and if you get back values use them, and if not source them from elsewhere).
There was a problem hiding this comment.
If we're not throwing these errors in our context hooks, that seems like a code smell. It's the standard way of doing it for pretty much all major libraries.
We also do this in:
useNFTLifecycleContextuseThemeuseBuyContextuseCheckoutContextuseTransactionContext
There was a problem hiding this comment.
i think we should include this in all providers
There was a problem hiding this comment.
as far as I know, wallet is the exception so it will silently fail on an unconnected wallet button
There was a problem hiding this comment.
the providers that do not currently do this are:
OnchainKitProvider
IdentityProvider
WalletProvider
can y'all think of a reason we wouldn't want to throw in these cases? if not I can add
There was a problem hiding this comment.
I think it's a big issue that we don't throw in useOnchainKit especially
src/earn/utils/deposit.ts
Outdated
| tokenAddress: Address; | ||
| amount: bigint; | ||
| receiverAddress: Address; | ||
| }; |
There was a problem hiding this comment.
can you add descriptions for each of theses plz?
| export function useEarnContext() { | ||
| const context = useContext(EarnContext); | ||
| if (!context) { | ||
| throw new Error('useEarnContext must be used within an EarnProvider'); | ||
| } | ||
| return context; | ||
| } |
There was a problem hiding this comment.
the providers that do not currently do this are:
OnchainKitProvider
IdentityProvider
WalletProvider
can y'all think of a reason we wouldn't want to throw in these cases? if not I can add
|
looks like the identity components are set up so that the provider is optional, same as wallet |
What changed? Why?
Notes to reviewers
How has it been tested?