-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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(ramp): add sell quick amounts with gas estimations #8080
Conversation
Updated dependencies detected. Learn more about Socket for GitHub ↗︎
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #8080 +/- ##
==========================================
+ Coverage 39.65% 40.21% +0.56%
==========================================
Files 1233 1234 +1
Lines 29820 29883 +63
Branches 2840 2862 +22
==========================================
+ Hits 11824 12017 +193
+ Misses 17307 17175 -132
- Partials 689 691 +2 ☔ View full report in Codecov by Sentry. |
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/1046cfe8-9727-43e2-ac4c-66ebdfd9f683 |
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
@SocketSecurity ignore @consensys/on-ramp-sdk@1.25.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice feature, I really missed it and did not realise it could be added this way. Good job.
I added comments as I understood the overall code but still find it difficult to exactly get what cases we actually take into account, specifically related to gaslimit, because of the use of numbers for which I have to guess the origin instead of clearly being told what they are for.
I suggest you add some code comments to explain why and/or use explicit constant names to prevent the magic numbers that could not be obvious for other devs. Try to imagine asking yourself what these numbers would mean if you just landed on this code without any knowledge of the project or crypto.
Thanks!
app/components/UI/Ramp/common/hooks/useGasPriceEstimation.test.ts
Outdated
Show resolved
Hide resolved
app/components/UI/Ramp/common/hooks/useGasPriceEstimation.test.ts
Outdated
Show resolved
Hide resolved
@NicolasMassart Thank you for your review. I have addressed some of the comments already in the latest commit. |
9c885a7
to
4fb0842
Compare
@wachunei LGTM I finished verifying this change ✅ |
Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve this changes for merge.
However I believe we should carefully refactor the test to make sure it's very clear what all the values are and what the test case is.
Not doing it doesn't break the code but doing it would ,make the code even more easy to understand.
Please consider this suggestion before merging.
Done in cf91073, thank you very much for your review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Thank you so much, I love reading clear code like this!
|
Description
This PR adds quick amounts buttons as percentages to the Sell Build Quote Keyboard. The percentages and max button will calculate a new amount Amount and always be set to the minimum of (Amount, Balance - Gas Fees) if the selected asset is the native asset, enforcing the Amount never to be over the required gas fees to be paid.
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-12-12.at.14.25.46.mp4
Pre-merge author checklist
Pre-merge reviewer checklist