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: show nonce warning when above current nonce #2625

Merged
merged 4 commits into from
Oct 16, 2023
Merged

Fix: show nonce warning when above current nonce #2625

merged 4 commits into from
Oct 16, 2023

Conversation

katspaugh
Copy link
Member

What it solves

Resolves #2560

How this PR fixes it

Shows a warning if the nonce is above the recommended nonce, or if the nonce is more than 100 higher than the safe nonce.

How to test it

Enter a nonce that's higher than the recommended one.

Screenshots

Screenshot 2023-10-12 at 17 48 59

@github-actions
Copy link

github-actions bot commented Oct 12, 2023

Branch preview

✅ Deploy successful!

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

Comment on lines -88 to -95
const ADORNMENT_PADDING = '24px'

const clamped = `clamp(calc(${MIN_CHARS}ch + 6px), calc(${Math.max(MIN_CHARS, value.length)}ch + 6px), ${MAX_WIDTH})`

if (showRecommendedNonceButton) {
return `calc(${clamped} + ${ADORNMENT_PADDING})`
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this to avoid the jumping when the reset button appears, it was really annoying. I made it wider by default.

@github-actions
Copy link

github-actions bot commented Oct 12, 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

NONCE_MUST_BE_NUMBER = 'Nonce must be a number',
NONCE_TOO_LOW = "Nonce can't be lower than %%nonce%%",
NONCE_TOO_HIGH = 'Nonce is too high',
NONCE_TOO_FAR = 'Nonce is very far from the current nonce',
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like "Nonce is much higher than the current nonce"? "very far" sounds ambiguous imo

Copy link
Member Author

@katspaugh katspaugh Oct 13, 2023

Choose a reason for hiding this comment

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

Maybe! What does BritGPT say, @iamacook?

Copy link
Member

Choose a reason for hiding this comment

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

The suggestion sounds good to me.

Comment on lines +132 to +144
useEffect(() => {
let message = ''
// Warnings
if (Number(nonce) >= safe.nonce + MAX_NONCE_DIFFERENCE) {
message = ErrorMessages.NONCE_TOO_FAR
}

if (Number(nonce) > Number(recommendedNonce)) {
message = ErrorMessages.NONCE_GT_RECOMMENDED
}

setWarning(message)
}, [nonce, recommendedNonce, safe.nonce])
Copy link
Member

Choose a reason for hiding this comment

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

Why can't this be part of the validate call where we also have the rest of the messages?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because in some cases this might be legit what the user wants to do. At least this was like this before the tx flow redesign, IIRC.

@liliya-soroka
Copy link
Member

liliya-soroka commented Oct 16, 2023

Verified.
Checked cases:

  1. new nonce<the current safe nonce
  2. new nonce< the recommended nonce
  3. new nonce> the recommended nonce
  4. new nonce= the recommended nonce
  5. reject tx +nonce updating
  6. safe apps txs +nonce updating
  7. batch txs ( create batch)
  8. there is tx with the incorrect high nonce
  9. 1 of 1 safe=> tx execution, tx creation without execution and next txs
  10. nonce=0
  11. N of M safe

@katspaugh katspaugh merged commit a9c36ee into dev Oct 16, 2023
9 checks passed
@katspaugh katspaugh deleted the nonce branch October 16, 2023 10:50
@github-actions github-actions bot locked and limited conversation to collaborators Oct 16, 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.

Show a warning/prompt when nonce is above the latest nonce
4 participants