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

feat(partial-approvals): partial approve v2 #5269

Merged
merged 18 commits into from
Jan 8, 2025

Conversation

alfetopito
Copy link
Collaborator

@alfetopito alfetopito commented Jan 7, 2025

Update 1: 2025-01-08

  • Moved the banner to the top
  • Display it always
  • Shortened text

image

Summary

Builds on top of #5256

Changes from base PR:

  • Rename Partial Approve to Minimal Approvals and update the tooltip. Text still TBD.
    image

  • Add a banner when an approval is needed (both permit and regular). Text still TBD.
    image

To Test

  1. Pick as sell a permittable token for which you have no approvals
  • Banner should be displayed
  1. Repeat with a non-permittable token for which you have no approvals
  • Banner should be displayed
  1. Pick as sell any token for which you have enough approvals
  • Banner should NOT be displayed
  1. For all cases where a permit is needed:
  • Do an approval without the flag on: should approve/permit only the minimum necessary
  • Do an approval with the flag off: should approve/permit the infinite amount

@alfetopito alfetopito self-assigned this Jan 7, 2025
@alfetopito alfetopito requested review from a team January 7, 2025 14:22
Copy link

vercel bot commented Jan 7, 2025

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

Name Status Preview Updated (UTC)
cowfi ✅ Ready (Inspect) Visit Preview Jan 8, 2025 4:11pm
explorer-dev ✅ Ready (Inspect) Visit Preview Jan 8, 2025 4:11pm
swap-dev ✅ Ready (Inspect) Visit Preview Jan 8, 2025 4:11pm
3 Skipped Deployments
Name Status Preview Updated (UTC)
cosmos ⬜️ Ignored (Inspect) Visit Preview Jan 8, 2025 4:11pm
sdk-tools ⬜️ Ignored (Inspect) Visit Preview Jan 8, 2025 4:11pm
widget-configurator ⬜️ Ignored (Inspect) Visit Preview Jan 8, 2025 4:11pm

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Hey @alfetopito , great job!
Just 3 questions:

  1. WDYT about moving the message closer to the 'Approve' button?
    closer to approve button
  2. The message is displayed for a ETH-flow (when no approvals are needed) when connected to EOA wallet. Should it?
    image

But I can see 2 cases when it should be displayed:

  • Bundled ETH-flow inside the Safe app
  • A user picks a classic ETH-flow with wrapping eth to WETH. Maybe here?
    image
  1. (asked in this thread)
    I'm confused with this message in the tooltip: “To change previous approvals, you must first revoke the existing approval and then do a new approval on CoW Swap.”
    If it is related to an unlimited previous approval, might it be possible to clarify it and add 'unlimited' to this sentence? Like:
    “To change previous **unlimited** approvals, you must first revoke the existing approval and then do a new approval on CoW Swap.”

@alfetopito
Copy link
Collaborator Author

Hey @alfetopito , great job! Just 3 questions:

1. WDYT about moving the message closer to the 'Approve' button?
   ![closer to approve button](https://private-user-images.githubusercontent.com/70885163/401147881-1dae4662-3aa8-4267-8c4f-c0e82828d740.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzYzNDYyMzksIm5iZiI6MTczNjM0NTkzOSwicGF0aCI6Ii83MDg4NTE2My80MDExNDc4ODEtMWRhZTQ2NjItM2FhOC00MjY3LThjNGYtYzBlODI4MjhkNzQwLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAxMDglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMTA4VDE0MTg1OVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWZjYjdjMDk4NjliZWNhOGUwZGE0YmY5ZGIxOTI3YjZmYjJhMmIxNGE3MmFhY2EwNjg1YzJjMjQ5N2ZlZjVmZmImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.01wM10IwT4N-BQIxqCI5s-RQgGQ9QAMiMgeDQK8p0qk)

Moved it after TWAP banner.
But not trivial to move it after Smart slippage banner.
Anyway, if Mindy decides to show it always, it'll be easier to show there.

2. The message is displayed for a ETH-flow (when no approvals are needed) when connected to EOA wallet. Should it?
   ![image](https://private-user-images.githubusercontent.com/70885163/401148670-7111f3aa-88d5-4cd3-b9d5-e8652051b13a.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzYzNDYyMzksIm5iZiI6MTczNjM0NTkzOSwicGF0aCI6Ii83MDg4NTE2My80MDExNDg2NzAtNzExMWYzYWEtODhkNS00Y2QzLWI5ZDUtZTg2NTIwNTFiMTNhLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAxMDglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMTA4VDE0MTg1OVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTliYTY3NjI4NzdhMTkyY2YxNTcyMGMyZTE3MzZkYjhkYjgxZDRlYjY1NmIzODhiYjM1MTRmMTBmNTg1YThlOTAmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.yNlZBM--pFitZzAv4YTA6UPd_9zIh0ni7jdLAK9bPp0)

Done

But I can see 2 cases when it should be displayed:

* Bundled ETH-flow inside the Safe app

For those cases, the user already has the option to edit the amounts in the Safe UI when creating the tx, so I don't think it's needed.

* A user picks a classic ETH-flow with wrapping eth to WETH. Maybe here?
  ![image](https://private-user-images.githubusercontent.com/70885163/401148438-144b8287-0828-414c-aef9-694a1a533949.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzYzNDYyMzksIm5iZiI6MTczNjM0NTkzOSwicGF0aCI6Ii83MDg4NTE2My80MDExNDg0MzgtMTQ0YjgyODctMDgyOC00MTRjLWFlZjktNjk0YTFhNTMzOTQ5LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAxMDglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMTA4VDE0MTg1OVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTc0ZWM5NzZkM2VjZTVmODUxZDRhMzAzM2MwNTM3NjMzYzAxYzI3Zjg5YzFhZjYwYmU0MTMwMTIyZWFiNjQ3MDMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.ir0zLUJHrFcErOp4wOHTCHFX-J8Q5g8FTOOP8dmQkEI)

Not worth it IMO

3. (asked in this [thread](https://cowservices.slack.com/archives/C03S445V822/p1736338903803929?thread_ts=1736181732.165549&cid=C03S445V822))
   I'm confused with this message in the tooltip: `“To change previous approvals, you must first revoke the existing approval and then do a new approval on CoW Swap.”`
   If it is related to an unlimited previous approval, might it be possible to clarify it and add 'unlimited' to this sentence? Like:
   ` “To change previous **unlimited** approvals, you must first revoke the existing approval and then do a new approval on CoW Swap.”`

According to Mindy, it can remain as is.

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Looks good now, thank you!

@alfetopito alfetopito merged commit 05a723f into feat/partial-approve Jan 8, 2025
13 checks passed
@alfetopito alfetopito deleted the feat/partial-approve_v2 branch January 8, 2025 17:28
@github-actions github-actions bot locked and limited conversation to collaborators Jan 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants