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

refactor: remove retry validation #4617

Merged
merged 11 commits into from
Aug 21, 2024
Merged

Conversation

matthewwalsh0
Copy link
Member

@matthewwalsh0 matthewwalsh0 commented Aug 16, 2024

Explanation

When calling speedUpTransaction or stopTransaction, the existing gas fee values are increased by 10% unless specific gasValues are provided.

If provided, these are validated to at least be an increase of 10% over the existing values in the original transaction.

This validation has resulted in multiple errors due to scenarios such as:

  • Only one of maxFeePerGas or maxPriorityFeePerGas was too low so it alone is increased via the UI, causing these validations to fail since the other value has not increased by at least 10%.
  • A transaction ultimately had sufficient gas fees but was below the market estimate so the validations also fail if this difference is less than 10%.
  • The clients do not currently gracefully handle this error meaning the user is unaware of the problem and why their cancel or speed up attempt failed.

This PR ultimately removes these validations so the controller can still automatically increase the previous gas fees if no new values are provided, however it is now the responsibility of the client to provide suitable values, and these are always respected.

In addition, to simplify the implementation and future maintenance of the fix, existing duplication between the speedUpTransaction and stopTransaction methods has been removed via a new #retryTransaction and getTransactionParamsWithIncreasedGasFee methods.

References

Relates to #25337 #2444

Changelog

@metamask/transaction-controller

  • CHANGED: No longer validate increase of gasValues passed to speedUpTransaction and stopTransaction methods.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@matthewwalsh0
Copy link
Member Author

@metamaskbot publish-preview

@matthewwalsh0
Copy link
Member Author

@metamaskbot publish-preview

@matthewwalsh0
Copy link
Member Author

@metamaskbot publish-preview

@matthewwalsh0 matthewwalsh0 marked this pull request as ready for review August 19, 2024 09:30
@matthewwalsh0 matthewwalsh0 requested a review from a team as a code owner August 19, 2024 09:30
Copy link
Contributor

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.
{
  "@metamask-previews/accounts-controller": "18.0.0-preview-e8f9512d",
  "@metamask-previews/address-book-controller": "5.0.0-preview-e8f9512d",
  "@metamask-previews/announcement-controller": "7.0.0-preview-e8f9512d",
  "@metamask-previews/approval-controller": "7.0.2-preview-e8f9512d",
  "@metamask-previews/assets-controllers": "37.0.0-preview-e8f9512d",
  "@metamask-previews/base-controller": "6.0.2-preview-e8f9512d",
  "@metamask-previews/build-utils": "3.0.0-preview-e8f9512d",
  "@metamask-previews/chain-controller": "0.1.1-preview-e8f9512d",
  "@metamask-previews/composable-controller": "7.0.0-preview-e8f9512d",
  "@metamask-previews/controller-utils": "11.0.2-preview-e8f9512d",
  "@metamask-previews/ens-controller": "13.0.1-preview-e8f9512d",
  "@metamask-previews/eth-json-rpc-provider": "4.1.3-preview-e8f9512d",
  "@metamask-previews/gas-fee-controller": "19.0.1-preview-e8f9512d",
  "@metamask-previews/json-rpc-engine": "9.0.2-preview-e8f9512d",
  "@metamask-previews/json-rpc-middleware-stream": "8.0.2-preview-e8f9512d",
  "@metamask-previews/keyring-controller": "17.1.2-preview-e8f9512d",
  "@metamask-previews/logging-controller": "5.0.0-preview-e8f9512d",
  "@metamask-previews/message-manager": "10.0.2-preview-e8f9512d",
  "@metamask-previews/name-controller": "8.0.0-preview-e8f9512d",
  "@metamask-previews/network-controller": "20.2.0-preview-e8f9512d",
  "@metamask-previews/notification-controller": "6.0.0-preview-e8f9512d",
  "@metamask-previews/notification-services-controller": "0.2.1-preview-e8f9512d",
  "@metamask-previews/permission-controller": "11.0.0-preview-e8f9512d",
  "@metamask-previews/permission-log-controller": "3.0.0-preview-e8f9512d",
  "@metamask-previews/phishing-controller": "10.1.1-preview-e8f9512d",
  "@metamask-previews/polling-controller": "9.0.1-preview-e8f9512d",
  "@metamask-previews/preferences-controller": "13.0.1-preview-e8f9512d",
  "@metamask-previews/profile-sync-controller": "0.2.1-preview-e8f9512d",
  "@metamask-previews/queued-request-controller": "4.0.0-preview-e8f9512d",
  "@metamask-previews/rate-limit-controller": "6.0.0-preview-e8f9512d",
  "@metamask-previews/selected-network-controller": "17.0.0-preview-e8f9512d",
  "@metamask-previews/signature-controller": "18.0.1-preview-e8f9512d",
  "@metamask-previews/transaction-controller": "35.1.1-preview-e8f9512d",
  "@metamask-previews/user-operation-controller": "14.0.1-preview-e8f9512d"
}

@jpuri
Copy link
Contributor

jpuri commented Aug 19, 2024

Hey @matthewwalsh0 : PR is changing and at same time moving the code which makes hard to review.

@matthewwalsh0
Copy link
Member Author

Hey @matthewwalsh0 : PR is changing and at same time moving the code which makes hard to review.

The vast majority of changes are to remove the duplication, meaning the resulting logic should be identical except that validateMinimumIncrease is simply no longer called.

@matthewwalsh0 matthewwalsh0 merged commit 72425c3 into main Aug 21, 2024
116 checks passed
@matthewwalsh0 matthewwalsh0 deleted the refactor/remove-retry-validation branch August 21, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants