-
Notifications
You must be signed in to change notification settings - Fork 456
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: Update tx flow form when selecting a replacement nonce #2520
Conversation
Branch preview✅ Deploy successful! https://fix_nonce_update--walletweb.review-wallet-web.5afe.dev |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 Nice one!
I've reverted part of #2383 to have the previous validation UX (input is checked without having to blur) and instead added an effect to update the form value when the recommended nonce changes and IF the user has not touched the field yet. I've also added back the loading state. @schmanu @katspaugh can you check if this is the desired behaviour? |
@@ -42,11 +42,12 @@ const SafeTxProvider = ({ children }: { children: ReactNode }): ReactElement => | |||
const isSigned = safeTx && safeTx.signatures.size > 0 | |||
|
|||
// Recommended nonce and safeTxGas | |||
const recommendedNonce = Math.max(safe.nonce, useRecommendedNonce() ?? 0) | |||
const recommendedNonce = useRecommendedNonce() | |||
const safeRecommendedNonce = recommendedNonce ? Math.max(safe.nonce, recommendedNonce) : undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO this should be part of useRecommendedNonce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I've moved it into the hook.
useEffect(() => { | ||
if (formMethods.formState.touchedFields[TxNonceFormFieldNames.NONCE]) return | ||
|
||
formMethods.setValue(TxNonceFormFieldNames.NONCE, recommendedNonce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? Can’t the input just be a controlled input? SafeTxContext returns a finalNonce which already contains the logic that differentiates user input and background updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I think the issue is that the validate
calls setNonce
automatically without user input so that the controlled input value nonce
will never update to the recommended nonce. I've added an early return inside the validate
function to skip validation if the input value is the same as nonce
to avoid initial validation. This fixes the issue too and since we know that nonce
is always valid it is ok to skip validation if the input value is the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it out and it works fine.
83daec9
to
7316ee3
Compare
Looks good to me. |
What it solves
Part of #2383
The form mode for the nonce form changed to
onTouched
with #2383 but selecting a nonce now doesn't update the form unless the user blurs the nonce input. This change callsonBlur()
whenever the user selects something from the nonce replacement dropdown.How to test it
Checklist