Conversation
| The downside is that it's a worse user experience (the user might not be familiar with ERC20 approvals, | ||
| and their wallet might not explain it to them), and worse security (rather than a bounded recurring limit, | ||
| the user needs to grant a single fixed amount up-front—renewing that amount requires a new approval). | ||
| */ |
There was a problem hiding this comment.
I'm not tracking with this - does this mean that the user is just up-front transfering to the ephemeral wallet, rather than granting the ephemeral wallet permission to spend from the main wallet? Or does it mean this is still "permission to spend from the base wallet", but it won't auto-renew once the initial allowance is used up?
| const existingData = this.loadSavedWalletAndPermission(storage, storageKey); | ||
| if (existingData) { | ||
| const ephemeralSmartWallet = await toEphemeralSmartWallet(existingData.privateKey, config.apiKey); | ||
| const ephemeralSmartWallet = await toEphemeralSmartWallet(existingData.privateKey); |
There was a problem hiding this comment.
Non-blocking ...
Any reason not to still use the config for the apiKey and just have the default when creating the config be the env var?
There was a problem hiding this comment.
Mainly that developers using this method will look at the parameters and assume they need to plug in an API key even if the field is optional. I don't want developers bringing their own API key.
| token: string; | ||
| chainId: number; | ||
| allowance: bigint; | ||
| periodInDays: number; // this is ignored |
There was a problem hiding this comment.
If it is ignored, can we just delete it. Seems annoying / confusing to have otherwise.
There was a problem hiding this comment.
The idea is that we could remove this shim if/when Coinbase gets their shit together with spend permissions in the Base app—so keeping a consistent interface seems useful from that perspective 🤷
|
|
||
| // Use the client | ||
| const hash = await client.sendTransaction({ | ||
| account: params.account as `0x${string}`, |
There was a problem hiding this comment.
FWIW viem has a Hex type we used in other places.
| spender: params.spender, | ||
| token: params.token, | ||
| allowance: params.allowance.toString(), | ||
| period: params.periodInDays * 24 * 60 * 60, |
There was a problem hiding this comment.
Is this correct, or is it misleading too? If the period is no longer required, we should change the type to not require it and handle that?
There was a problem hiding this comment.
Yeah this is a bit of impedance mismatch against spend permissions. See my other comment—maybe it's not realistic to try preserving a common interface, but that was my thought process.
|
Closing this PR as the code was added in #64 |
3 participants
No description provided.