[TX flow] Adds nonce validation #2175

usame-algan · 2023-06-26T15:08:44Z

What it solves

Resolves #2110

How this PR fixes it

Displays an error message in a tooltip around the nonce form
Always displays the reset button but in a disabled state to prevent the input from jumping

How to test it

Open the Safe
Create a new transaction
Choose a nonce lower than the current safe nonce
Observe an error inside a tooltip

Screenshots

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) 🧑‍💻

github-actions · 2023-06-26T15:08:59Z

Branch preview

✅ Deploy successful!

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

usame-algan · 2023-06-26T15:10:17Z

We should also think about disabling the "Next" and "Submit" button if the nonce is invalid. Previously this was covered since the nonce was edited in a separate modal and the user couldn't leave without entering a valid input there. I suggest we add another state nonceError to the context but it would have to be added to every component that has Next or Submit buttons.

github-actions · 2023-06-26T15:11:18Z

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}

usame-algan · 2023-06-26T15:11:24Z

Should we also limit the maximum number the user can enter or additionally show another warning/error if the input is much higher than the current nonce? Currently, if the user keeps typing the number changes to "Infinity" at some point which is handled as a valid input.

usame-algan · 2023-06-26T15:14:16Z

src/components/tx-flow/flows/TokenTransfer/CreateTokenTransfer.tsx

@@ -224,7 +224,7 @@ const CreateTokenTransfer = ({
          </FormControl>

          {isDisabled && (
-            <Box mt={1} display="flex" alignItems="center">
+            <Box display="flex" alignItems="center" mt={-2} mb={3}>


This fixes the issue that the safe error is too close to the divider

katspaugh · 2023-06-26T16:27:06Z

We should also think about disabling the "Next" and "Submit" button if the nonce is invalid. Previously this was covered since the nonce was edited in a separate modal and the user couldn't leave without entering a valid input there. I suggest we add another state nonceError to the context but it would have to be added to every component that has Next or Submit buttons.

How about just not letting the user choose an incorrect nonce? I assumed this validation would do that.

usame-algan · 2023-06-27T08:27:19Z

After our discussion:

We don't set the nonce if there is a validation error. Once the user blurs the input it will reset back to the previous value that was valid
Add a max number input validation so it doesn't flip to "Infinity"

iamacook

I'm not sure about preventing the deletion of the value. I was initially confused why I couldn't backspace. (Here I'm trying to delete the value:)

iamacook · 2023-06-27T09:49:44Z

src/components/tx-flow/common/TxNonce/index.tsx

+      }
+
+      if (nonce >= Number.MAX_SAFE_INTEGER) {
+        return 'Nonce is too big'


Suggested change

return 'Nonce is too big'

return 'Nonce is too high'

iamacook · 2023-06-27T09:50:20Z

src/components/tx-flow/common/TxNonce/index.tsx

+      const isNotValid = validateInput(value)
+
+      if (isNotValid) {
+        setError(validateInput(value))


Suggested change

const isNotValid = validateInput(value)

if (isNotValid) {

setError(validateInput(value))

const error = validateInput(value)

if (error) {

setError(error)

iamacook · 2023-06-27T09:51:13Z

src/components/tx-flow/common/TxNonce/index.tsx

  )

+  const handleBlur = useCallback(() => {
+    setError(undefined)


What if the current nonce is erroneous?

Edit: this ties in with my review summary.

Since we don't save the nonce if its incorrect the current nonce must be correct so we can safely remove the error.

iamacook · 2023-06-27T10:19:15Z

For visibility (as per our discussion) two thoughts:

A Tooltip inside a Tooltip seemed initially wrong. However, MUI mounts a Popper outside of the children so that's ok.
When replacing a transaction, we should disable nonce editing:

usame-algan · 2023-06-28T12:36:34Z

I'm not sure about preventing the deletion of the value. I was initially confused why I couldn't backspace. (Here I'm trying to delete the value:)

I agree its not ideal but since we use the same value for controlling the input we can't easily let the user remove and not update it.

When replacing a transaction, we should disable nonce editing:

Thanks for catching this. I updated both the replace and reject flow to work now. For replacing a transaction we need to call setTxNonce to overwrite the state with the nonce that is passed. For simple rejections we can just disable the nonce field. I've added a helper function to detect reject txs so we don't have to pass a flag anymore like we did in the old tx flow.

iamacook · 2023-06-28T12:47:19Z

I agree its not ideal but since we use the same value for controlling the input we can't easily let the user remove and not update it.

We could save the new nonce if validation fails and not return early or have a local state that saves the nonce onBlur, for example.

# Conflicts: # src/components/tx-flow/common/TxNonce/index.tsx # src/components/tx-flow/flows/TokenTransfer/CreateTokenTransfer.tsx

Adds nonce validation

5a0bcdc

usame-algan requested review from katspaugh and iamacook June 26, 2023 15:11

usame-algan commented Jun 26, 2023

View reviewed changes

katspaugh approved these changes Jun 26, 2023

View reviewed changes

Adjust nonce validation

740f4f4

usame-algan requested a review from katspaugh June 27, 2023 09:45

iamacook reviewed Jun 27, 2023

View reviewed changes

usame-algan added 3 commits June 28, 2023 12:30

Merge remote-tracking branch 'origin/epic-tx-flow' into nonce-validation

90503af

Disable nonce for reject tx, use correct nonce when replacing a tx

99614aa

Adjust wording

41102a5

usame-algan requested a review from iamacook June 28, 2023 12:38

iamacook and others added 3 commits June 29, 2023 14:21

fix: migrate input to RHF

466800c

Limit nonce input width

bf1e945

Merge remote-tracking branch 'origin/epic-tx-flow' into nonce-validation

d6a86f2

# Conflicts: # src/components/tx-flow/common/TxNonce/index.tsx # src/components/tx-flow/flows/TokenTransfer/CreateTokenTransfer.tsx

iamacook linked an issue Jun 29, 2023 that may be closed by this pull request

[TX flow] Nonce form #2110

Closed

iamacook approved these changes Jun 29, 2023

View reviewed changes

Fix e2e tests

d1cf727

usame-algan merged commit 75f142d into epic-tx-flow Jun 29, 2023
6 of 7 checks passed

usame-algan deleted the nonce-validation branch June 29, 2023 13:53

github-actions bot locked and limited conversation to collaborators Jun 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TX flow] Adds nonce validation #2175

[TX flow] Adds nonce validation #2175

usame-algan commented Jun 26, 2023 •

edited

Loading

github-actions bot commented Jun 26, 2023 •

edited

Loading

usame-algan commented Jun 26, 2023

github-actions bot commented Jun 26, 2023 •

edited

Loading

usame-algan commented Jun 26, 2023

usame-algan Jun 26, 2023

katspaugh commented Jun 26, 2023 •

edited

Loading

usame-algan commented Jun 27, 2023

iamacook left a comment

iamacook Jun 27, 2023

iamacook Jun 27, 2023

iamacook Jun 27, 2023 •

edited

Loading

usame-algan Jun 28, 2023

iamacook commented Jun 27, 2023

usame-algan commented Jun 28, 2023 •

edited

Loading

iamacook commented Jun 28, 2023 •

edited

Loading

[TX flow] Adds nonce validation #2175

[TX flow] Adds nonce validation #2175

Conversation

usame-algan commented Jun 26, 2023 • edited Loading

What it solves

How this PR fixes it

How to test it

Screenshots

Checklist

github-actions bot commented Jun 26, 2023 • edited Loading

Branch preview

usame-algan commented Jun 26, 2023

github-actions bot commented Jun 26, 2023 • edited Loading

ESLint Summary View Full Report

usame-algan commented Jun 26, 2023

usame-algan Jun 26, 2023

Choose a reason for hiding this comment

katspaugh commented Jun 26, 2023 • edited Loading

usame-algan commented Jun 27, 2023

iamacook left a comment

Choose a reason for hiding this comment

iamacook Jun 27, 2023

Choose a reason for hiding this comment

iamacook Jun 27, 2023

Choose a reason for hiding this comment

iamacook Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

usame-algan Jun 28, 2023

Choose a reason for hiding this comment

iamacook commented Jun 27, 2023

usame-algan commented Jun 28, 2023 • edited Loading

iamacook commented Jun 28, 2023 • edited Loading

usame-algan commented Jun 26, 2023 •

edited

Loading

github-actions bot commented Jun 26, 2023 •

edited

Loading

github-actions bot commented Jun 26, 2023 •

edited

Loading

katspaugh commented Jun 26, 2023 •

edited

Loading

iamacook Jun 27, 2023 •

edited

Loading

usame-algan commented Jun 28, 2023 •

edited

Loading

iamacook commented Jun 28, 2023 •

edited

Loading