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

Fix approval + swaps #6259

Merged
merged 6 commits into from
Nov 12, 2024
Merged

Fix approval + swaps #6259

merged 6 commits into from
Nov 12, 2024

Conversation

brunobar79
Copy link
Member

@brunobar79 brunobar79 commented Nov 8, 2024

Fixes APP-2023
Fixes APP-2017

What changed (plus any additional context for devs)

Two separate issues:

  • Supporting permit in a few tokens was conflicting with gas estimation causing different issues, resulting in incorrect gas estimations and onchain failures running out of gas (see ticket 2017)
  • We have a check that waits for node ack before continuing on to the next rap action; any failure to find the tx (ex. private mempool) would make the rap fail and subsequent actions would never execute (see ticket 2023). Historical context for this node ack in general: raps failures fix browser-extension#740

Changes:

  • Disable permit support for now as there are pros and cons to this; we can revisit this separately
  • Simplifies node ack check to being before any rap action past the first one needs it (means we no longer need to check if there is more than 1 action after the first action; it also means we don't unnecessarily do this check after the last action)
  • Simplifies the node ack check to only be for non-mainnet transactions
  • Caps the number of retries for node ack to 10

Screen recordings / screenshots

Screen.Recording.2024-11-08.at.5.50.47.PM.mov

What to test

All the combinations of swaps

src/raps/execute.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@walmat walmat left a comment

Choose a reason for hiding this comment

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

nice work, left a small suggestion but it's a nitpick

src/raps/execute.ts Outdated Show resolved Hide resolved
src/raps/execute.ts Outdated Show resolved Hide resolved
* Skip node ack check if txn params are for mainnet

* Cap retries to 10 retries for node ack
Copy link
Member

@jinchung jinchung left a comment

Choose a reason for hiding this comment

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

🌮

@jinchung jinchung removed the request for review from BrodyHughes November 12, 2024 18:30
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

@jinchung jinchung merged commit 910a789 into develop Nov 12, 2024
7 of 8 checks passed
@jinchung jinchung deleted the @bruno/remove-permit-on-mainnet branch November 12, 2024 18:34
@BrodyHughes BrodyHughes mentioned this pull request Nov 19, 2024
Copy link

sentry-io bot commented Nov 21, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Error: [raps/swap]: error executeSwap <global>(src/logger/index) View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants