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

[Bug]: Passed transaction nonce is ignored #9118

Open
compojoom opened this issue Apr 3, 2024 · 0 comments
Open

[Bug]: Passed transaction nonce is ignored #9118

compojoom opened this issue Apr 3, 2024 · 0 comments
Labels
external-contributor feature-request Feature requested by the MetaMask community team-transactions Transactions team

Comments

@compojoom
Copy link

Describe the bug

At the safe wallet, we are building a speed up TX function: safe-global/safe-wallet-web#3425
We are polling the RPC for the submitted TX and if we don't find it withing X seconds we offer the option to speed that transaction up. A transaction that needs to be sped up consist of mainly of {to, data, nonce} fields. Nonce is extremely important as it has to be the same as the TX that has been already submitted.
to speed up the transaction we call:
result = await signer.sendTransaction({ to, data, ...txOptions })

This works fine with the Metamask Browser Extension, but is buggy when used with Metamask Mobile.
Metamask mobile appears to ignore the provided nonce and just increase the nonce. I've looked around the MM-Mobile code and found where the problem comes from.
There are 2 cases.

Case 1:
The user has not enabled "customize transaction nonce" in the settings.
So here despite the fact that we've passed a nonce, the transaction.nonce is being reset to undefined:


because showCustomNonce is not true.

Please note that the Browser extension actually works fine. If we send a nonce there, it is set as the nonce of the tx that the user is executing. From our perspective the MM-Mobile behaviour is not expected

Case 2:
"customize transaction nonce" is set to true. If this option is on, the TransactionReview component renders the TransactionReviewInformation component. On DidMount this component sets the TX nonce to the next network nonce:


this now changes the provided transaction nonce, so we effectively end up again ignoring the nonce that the Dapp has provided.

This behaviour is also diverging from the MM Browser extension, where the nonce provided by the dapp is treated as the nonce for the "custom nonce" component.

We think that this behaviour is wrong and MM Mobile should behave as the MM Browser extension

Expected behavior

Provided dapp nonce should be used for submitting the TX.

Screenshots/Recordings

No response

Steps to reproduce

  1. Go to: https://feat_pending_txs--walletweb.review-wallet-web.5afe.dev/home?safe=eth:0xA77DE01e157f9f57C7c4A326eeE9C4874D0598b6
  2. Connect your MM Account
  3. Switch to sepolia and deploy a test safe (you can use the sponsored tx)
  4. Now try to submit a Transaction
  5. Wait for 15 sec you should see a speed up button
    grafik
  6. Click the button and speed up the TX
    grafik
    Note the wallet nonce
  7. Notice that on the Review screen in MM the nonce is different:
    grafik

Error messages or log output

No response

Version

7.19.0(1292)

Build type

None

Device

Iphone and Android

Operating system

Other (please elaborate in the "Additional Context" section)

Additional context

Ios and Android Problem. Happens both on the current stable versions and with a build directly from the repo.

Severity

No response

@compojoom compojoom added the type-bug Something isn't working label Apr 3, 2024
@gauthierpetetin gauthierpetetin added team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead team-confirmations-system DEPRECATED: please use "team-confirmations" label instead team-confirmations-planning (only for internal use within Confirmations team) Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking labels Apr 3, 2024
@bschorchit bschorchit added feature-request Feature requested by the MetaMask community and removed team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels Apr 11, 2024
@cryptotavares cryptotavares removed type-bug Something isn't working Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking labels Apr 12, 2024
@cryptotavares cryptotavares added team-confirmations Push issues to confirmations team team-transactions Transactions team and removed team-confirmations-system DEPRECATED: please use "team-confirmations" label instead team-confirmations Push issues to confirmations team team-confirmations-planning (only for internal use within Confirmations team) labels Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor feature-request Feature requested by the MetaMask community team-transactions Transactions team
Projects
Status: To be fixed
Status: To be fixed
Development

No branches or pull requests

5 participants