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: reset nonce if outdated #2383

Merged
merged 8 commits into from
Sep 18, 2023
Merged

fix: reset nonce if outdated #2383

merged 8 commits into from
Sep 18, 2023

Conversation

schmanu
Copy link
Member

@schmanu schmanu commented Aug 10, 2023

What it solves

If the recommended nonce and safe nonce updates while the transaction modal is open, the current nonce is not updated and transactions with too low nonces are created which lead to validation errors when proposed.

How this PR fixes it

Resets nonce to recommended nonce if the currently selected nonce is lower than the Safe nonce.

How to test it

  1. Open a 1/n Safe with nothing in the queue
  2. Initiate any transaction, note the used nonce and immediately execute it (without signing it first)
  3. While the transaction is executing, initiate another transaction
  4. Observe that this new transaction can initially not be executed and the nonce being the same as the pending transaction
  5. Do nothing and wait in the transaction modal until the first transaction got executed and indexed.
  6. Observe that the new transaction's nonce gets updated to the next nonce and that the execute button is now available for this transaction.

Checklist

  • I've tested the branch on mobile 📱
  • I've documented how it affects the analytics (if at all) 📊
  • I've written a unit/e2e test for it (if applicable) 🧑‍💻

resets nonce to recommended nonce if the currently selected nonce is lower than the Safe nonce.
@schmanu schmanu requested a review from usame-algan August 10, 2023 17:17
@github-actions
Copy link

github-actions bot commented Aug 10, 2023

Branch preview

✅ Deploy successful!

https://fix_refresh_recommended_nonce--walletweb.review-wallet-web.5afe.dev

@github-actions
Copy link

github-actions bot commented Aug 10, 2023

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@schmanu schmanu self-assigned this Aug 14, 2023
@katspaugh
Copy link
Member

@schmanu are you planning to finalize this PR?

@github-actions
Copy link

github-actions bot commented Aug 25, 2023

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@schmanu
Copy link
Member Author

schmanu commented Aug 25, 2023

@katspaugh
I had to adjust it a little to get rid of the SafeTxProvider adjustment.
The mode onTouch makes it work. Then it setNonce only gets called once the user once touched / blurred the field and not initially.

@schmanu schmanu requested a review from katspaugh August 25, 2023 15:09
@schmanu schmanu requested a review from katspaugh August 28, 2023 08:39
@francovenica
Copy link
Contributor

Issue:
I created a tx with nonce 285 in my safe, executed and right away I went to execute a nother tx (both new, none of them in queue)

During the 2nd tx creation the nonce was still 285. My expectation was, that after the first tx is processed and indexed the nonce would change to 286 on its own.

The result is that, the input "understood" that the nonce had to be updated (the little round arrow shows up, indicating that you can update to the suggested nonce) but the nonce itself never changed, so I'm able to sign the tx with the nonce 285 again:

nonce update

@francovenica
Copy link
Contributor

UPDATE:

It seems that is working fine, it just that sometimes it takes just a few seconds to updates and other 15 secs or so, but it updates fine so I'm ok with it
nonce update 2

@katspaugh
Copy link
Member

The result is that, the input "understood" that the nonce had to be updated (the little round arrow shows up, indicating that you can update to the suggested nonce) but the nonce itself never changed

Sounds like an unwanted effect. The field probably got “updated” to the same value and became “dirty” which triggered the reset button.

@francovenica
Copy link
Contributor

The best solution for the user would be that, as soon as you propose a tx the nonce in the safe updates right away, so the next tx already has blocked the nonce of the tx being processed, and if for some reason the tx fails the nonce is kicked back to the value it was before, but I'd assume that is not a feasible solution

@katspaugh
Copy link
Member

I returned it back to todo to check why the reset button appears.

@schmanu
Copy link
Member Author

schmanu commented Sep 7, 2023

I returned it back to todo to check why the reset button appears.

It appears because franco clicked inside the field.
Currently we trigger validation "onTouch". So as soon as a user touches the nonce field it counts as changed by the user.

If we do not want this we have to revert to the additional previous fix in the SafeTxProvider.
OR we could also fetch the nonce on-chain additionally when the modal is open.
Here is another user with this issue. This time with batched transactions and a slow indexer:
5afe/safe-support#280

@katspaugh
Copy link
Member

Ah, OK. Hauptsache it shouldn't show a reset button if the field was updated programmatically.

@katspaugh katspaugh merged commit c95fe4d into dev Sep 18, 2023
@katspaugh katspaugh deleted the fix-refresh-recommended-nonce branch September 18, 2023 10:20
@github-actions github-actions bot locked and limited conversation to collaborators Sep 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants