feat: add dynamic frontend wallet support #120
Conversation
✅ Deploy Preview for verbs-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
tremarkley
force-pushed
the
harry/frontend_dynamic
branch
from
38fe047 to
8cb2301
Compare
tremarkley
force-pushed
the
harry/frontend_dynamic
branch
from
8cb2301 to
7c29958
Compare
tremarkley
force-pushed
the
harry/frontend_dynamic
branch
15 times, most recently
from
0c775ca to
af5850d
Compare
tremarkley
changed the title
| signTypedData: walletClient.signTypedData, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🤔 With lend, I have all the Morpho related files together in: sdk/src/lend/providers/morpho/
I wonder if it makes sense to do something similar with wallet providers?
There was a problem hiding this comment.
yeah right now everything is under sdk/src/wallet/ and sdk/src/wallet/providers. Maybe it makes sense to do sdk/src/wallet/dynamic and sdk/src/wallet/providers/dynamic?
| ? fallback(rpcUrls.map((rpcUrl) => http(rpcUrl))) | ||
| : http(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🤔 It looks like this function doesn't have anything specific to Dynamic, can it be pulled into the parent Wallet and called from there?
There was a problem hiding this comment.
this is specifically for Wallet that are non smart wallets. I dont think we would want this for a smart wallet so I am hesitant to pull it into the Wallet class. We could consider having a non-smart wallet parent class in the future though. if we did that, we could probably pull this into it and maybe even the performInitialization implementation. But currently, it's such a small amount of code, I don't think it's worth creating that abstraction just yet
| dynamicWallet: DynamicHostedWalletToVerbsWalletOptions['wallet'], | ||
| ) { | ||
| super(chainManager) | ||
| this.dynamicWallet = dynamicWallet |
There was a problem hiding this comment.
🤔 If we renamed this something general like this.hostedWallet, can it be abstracted into the parent? If so, would it reduce the amount of boilerplate for this and and PrivyWallet?
There was a problem hiding this comment.
the issue is that the type this has is going to be different for each hosted wallet, so if we were to abstract it to the parent we would have to give it a generic type and ultimately, it wouldn't really end up saving us any code except maybe this one line. We would still need to add code to every implementation class to specify how to convert the wallet to a LocalAccount
| protected async performInitialization() { | ||
| this.signer = await this.createAccount() | ||
| this.address = this.signer.address | ||
| } |
| chainManager: ChainManager | ||
| }): Promise<DynamicWallet> { | ||
| const wallet = new DynamicWallet(params.chainManager, params.dynamicWallet) | ||
| await wallet.initialize() |
There was a problem hiding this comment.
🤔 I don't know what the fix is, but It's a little confusing that this class (and I assume PrivyWallet) have:
constructor()create()createAccount()performInitialization()initialize()from the parentWallet
There was a problem hiding this comment.
we can try to think of better ways to share code. Things just get a little gnarly when you have async calls that need to occur in order to fully initialize a class.
| */ | ||
| export class WalletNamespace< | ||
| H extends HostedWalletProvider = HostedWalletProvider, | ||
| H extends HostedWalletProvider<any> = HostedWalletProvider<any>, |
There was a problem hiding this comment.
❓ Can this be more strict?
| */ | ||
| export class WalletProvider< | ||
| H extends HostedWalletProvider, | ||
| H extends HostedWalletProvider<any>, |
tremarkley
force-pushed
the
harry/frontend_dynamic
branch
3 times, most recently
from
d9d9ad3 to
e57d44b
Compare
tremarkley
force-pushed
the
harry/frontend_dynamic
branch
from
e57d44b to
584b9fd
Compare
Closes #86
Demo
Description
Adds support for frontend wallets hosted by Dynamic. If you want to check out a fully working demo with dynamic wallets build and run this PR and also reach out to me for the Dynamic environment variables.
Changes
DynamicWalletclassVerbsinstances that leverages the hosted wallet registry in order to register the correct wallet infra depending on whetherVerbsis being used in Node or React