Skip to content
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

Use an EOA for our spender, simplify tutorial #92

Merged
merged 11 commits into from
Nov 8, 2024
Merged

Conversation

amiecorso
Copy link
Contributor

Significantly speeds up the quickstart to use an EOA for our spender.

Copy link

vercel bot commented Nov 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
smart-wallet-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 8, 2024 8:06pm

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Nov 7, 2024

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@amiecorso amiecorso changed the base branch from main to amie/realtime-buildbash-feedback November 7, 2024 23:50
docs/pages/guides/spend-permissions/quick-start.mdx Outdated Show resolved Hide resolved
docs/pages/guides/spend-permissions/quick-start.mdx Outdated Show resolved Hide resolved
Comment on lines 586 to 589
You can create a new Smart Wallet via the popup. Note that you'll need a little ETH in this wallet to fund the
deployment of your account. If you don't have any testnet ETH, try this [Coinbase faucet](https://portal.cdp.coinbase.com/products/faucet).

Once your wallet is created and funded, return to the app and click "Subscribe".
Sign the prompts, which will deploy your new Smart Wallet (if undeployed), and then prompt you to sign over the spend permission.
Before you continue, note that you'll also need a little testnet ETH in your spender wallet, which will need to fund the gas for the transactions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
You can create a new Smart Wallet via the popup. Note that you'll need a little ETH in this wallet to fund the
deployment of your account. If you don't have any testnet ETH, try this [Coinbase faucet](https://portal.cdp.coinbase.com/products/faucet).
Once your wallet is created and funded, return to the app and click "Subscribe".
Sign the prompts, which will deploy your new Smart Wallet (if undeployed), and then prompt you to sign over the spend permission.
Before you continue, note that you'll also need a little testnet ETH in your spender wallet, which will need to fund the gas for the transactions.
Note that we'll need a little bit of base sepolia ETH in both wallet addresses (the "user" wallet and the "app" wallet). In a more involved implementation you would use a paymaster to eliminate this requirement. For now, If you don't have any base sepolia ETH, try this [Coinbase faucet](https://portal.cdp.coinbase.com/products/faucet).

Do we actually require funds in the user wallet? Wouldn't the dapp's wallet be deploying the user's account during execution?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while I don't ever see the wallet prompt me to deploy it, I do see a warning screen if I have 0 ETH in the wallet with some buttons including the option to "Fund wallet". So while the user may not require ETH in a functional sense, something in our system is enforcing that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ultimately they will require at least a little ETH if the dev wants to see the app work end-to-end because there needs to be a little for the spender to spend)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. If the dapp requests a base asset (i.e. ETH) then the user will be prompted to fund before granting (so the dapp has something to spend). If the dapp requests an erc20 in an ideal world we'd probably ask the user to fund there as well, but in the current world we just let the user grant the permission without funding.

Before you continue, note that you'll also need a little testnet ETH in your spender wallet, which will need to fund the gas for the transactions.

Once your wallet is created and both wallets are funded, return to the app and click "Subscribe".
Sign the prompts, which will deploy your new Smart Wallet (if undeployed), and then prompt you to sign the spend permission requested by the app.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we prompt to deploy here - and if we did for base sepolia the default paymaster would handle funds for us

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it -- yeah as I mentioned in my comment above, I don't see this when I just tried it fresh so will remove

@amiecorso amiecorso changed the base branch from amie/realtime-buildbash-feedback to main November 8, 2024 03:04
spencerstock
spencerstock previously approved these changes Nov 8, 2024
Copy link
Contributor

@spencerstock spencerstock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ilikesymmetry
ilikesymmetry previously approved these changes Nov 8, 2024
Copy link
Contributor

@ilikesymmetry ilikesymmetry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still think we need to shift away from subscribe mental model, but these are all good edits

docs/pages/guides/spend-permissions/quick-start.mdx Outdated Show resolved Hide resolved
docs/pages/guides/spend-permissions/quick-start.mdx Outdated Show resolved Hide resolved
@ilikesymmetry ilikesymmetry mentioned this pull request Nov 8, 2024
@cb-heimdall cb-heimdall dismissed stale reviews from spencerstock and ilikesymmetry November 8, 2024 19:56

Approved review 2424707245 from spencerstock is now dismissed due to new commit. Re-request for approval.

Copy link
Contributor

@ilikesymmetry ilikesymmetry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

going to approve and merge for now so we can ship and blast

@cb-heimdall
Copy link
Collaborator

Review Error for ilikesymmetry @ 2024-11-08 20:05:00 UTC
User cannot review their own commit

@ilikesymmetry ilikesymmetry merged commit 2d0b6ea into main Nov 8, 2024
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants