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(ramp): estimate gasLimit for ERC20 transfers (sell feature) #12467

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

AxelGes
Copy link
Contributor

@AxelGes AxelGes commented Nov 27, 2024

Previously, we only estimated the gas limit for native token transactions. Now, we’ve added functionality to be able to estimate the gas limit for ERC20 token transfers during the sell operation.

This limit is then provided to the hook that estimates the total gas price.

We still use the default native token gas limit as a fallback (21,000 units).

Description

This is necessary to enable the sell feature with tokens other than the native chain token on mobile.

Related issues

https://consensyssoftware.atlassian.net/browse/RAMPS-2042?atlOrigin=eyJpIjoiNWUyZmVmNDY3MzcyNGE1NGJkZDVjM2RjNTM4MzgxNDkiLCJwIjoiaiJ9

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

NA

Before

NA

After

NA

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot metamaskbot added the team-ramp issues related to Ramp features label Nov 27, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 72.41379% with 8 lines in your changes missing coverage. Please review.

Project coverage is 56.96%. Comparing base (22a4989) to head (5c6743e).
Report is 60 commits behind head on main.

Files with missing lines Patch % Lines
...onents/UI/Ramp/hooks/useERC20GasLimitEstimation.ts 76.47% 3 Missing and 1 partial ⚠️
app/components/UI/Ramp/index.tsx 0.00% 2 Missing ⚠️
app/components/UI/Swaps/SwapsLiveness.ts 0.00% 1 Missing ⚠️
app/components/hooks/useInterval.ts 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12467      +/-   ##
==========================================
+ Coverage   56.41%   56.96%   +0.55%     
==========================================
  Files        1797     1816      +19     
  Lines       40586    40899     +313     
  Branches     5097     5172      +75     
==========================================
+ Hits        22896    23298     +402     
+ Misses      16134    16016     -118     
- Partials     1556     1585      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AxelGes AxelGes changed the title feat: ramps estimate gasLimit for erc20 transfers feat(ramp): estimate gasLimit for erc20 transfers Nov 28, 2024
@AxelGes AxelGes changed the title feat(ramp): estimate gasLimit for erc20 transfers feat(ramp): estimate gasLimit for ERC20 transfers (sell feature) Nov 28, 2024
@AxelGes AxelGes added the Run Smoke E2E Triggers smoke e2e on Bitrise label Nov 28, 2024
@AxelGes AxelGes marked this pull request as ready for review November 28, 2024 20:09
@AxelGes AxelGes requested review from a team as code owners November 28, 2024 20:09
Copy link
Contributor

github-actions bot commented Nov 28, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 28f67a9
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/254345b3-5a47-4437-bcac-829959fb7a63

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@metamaskbot metamaskbot added INVALID-PR-TEMPLATE PR's body doesn't match template and removed INVALID-PR-TEMPLATE PR's body doesn't match template labels Nov 28, 2024
Copy link
Member

@wachunei wachunei left a comment

Choose a reason for hiding this comment

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

LGTM

@wachunei wachunei removed the Run Smoke E2E Triggers smoke e2e on Bitrise label Dec 3, 2024
@wachunei wachunei added the Run Smoke E2E Triggers smoke e2e on Bitrise label Dec 3, 2024
Copy link
Contributor

github-actions bot commented Dec 3, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 39b6782
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4763e7e1-4334-49e2-92cb-9404a0ba1e2a

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link

sonarcloud bot commented Dec 3, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Run Smoke E2E Triggers smoke e2e on Bitrise team-ramp issues related to Ramp features
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

4 participants