fix: Nonce form #2120
fix: Nonce form #2120
Conversation
Branch preview✅ Deploy successful! |
| const validateInput = (value: string) => { | ||
| if (!Number.isInteger(value)) { | ||
| return false | ||
| } else if (Number(value) < safeNonce) { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Previously we displayed error messages but with the new input field there is no room for it.
ESLint Summary View Full Report
Report generated by eslint-plus-action |
| const queuedTxs = useMemo(() => { | ||
| if (!page || page.results.length === 0) { | ||
| return [] | ||
| } | ||
|
|
||
| const txs = page.results.filter(isTransactionListItem).map((item) => item.transaction) | ||
|
|
||
| return uniqBy(txs, (tx) => { | ||
| return isMultisigExecutionInfo(tx.executionInfo) ? tx.executionInfo.nonce : '' | ||
| }) | ||
| }, [page]) | ||
|
|
||
| const options = useMemo(() => { | ||
| return queuedTxs | ||
| .map((tx) => (isMultisigExecutionInfo(tx.executionInfo) ? tx.executionInfo.nonce : undefined)) | ||
| .filter((nonce) => nonce !== undefined) as number[] | ||
| }, [queuedTxs]) |
There was a problem hiding this comment.
Could you move this into a hook? Like usePreviousNonces?
| const { nonce, setNonce, safeTx } = useContext(SafeTxContext) | ||
| const { page } = useTxQueue() | ||
| const { safe } = useSafeInfo() | ||
| const safeNonce = safe.nonce || 0 |
There was a problem hiding this comment.
You can use the hook useIsValidNonce instead (don't rememeber the exact name).
There was a problem hiding this comment.
I think we have useValidateNonce but it returns a boolean. Not sure why we have this fallback here since safe.nonce can't be undefined but is -1 by default. Since we only use the value for validation I suggest we remove this fallback and use safe.nonce directly.
| const readonly = !isEditable | ||
|
|
||
| const queuedTxs = useMemo(() => { | ||
| if (!page || page.results.length === 0) { |
There was a problem hiding this comment.
I think we can remove page.results.length === 0 here as that is an empty array which also return in this case.
There was a problem hiding this comment.
Nice catch! I also wrote some unit tests to validate this.
| setNonce(newNonce) | ||
| const handleChange = useCallback( | ||
| (e: SyntheticEvent, value: string | AutocompleteValue<unknown, false, false, false>) => { | ||
| const nonce = Number(value) |
There was a problem hiding this comment.
To me personally it always feels a bit weird if an Input puts in "0" when I clear it.
But Number("") is 0. It still works fine but feels weird if my nonce is "9" and I want to put in "10" for instance.
There was a problem hiding this comment.
Agreed, I think having the cursor on the right of the 0 is also confusing even if the 0 is replaced once I start typing another number. Previously it was possible to have the input empty. I will look into restoring that behaviour
There was a problem hiding this comment.
I looked into this and it seems we are storing the nonce as a number now and use undefined as the loading state so the current context doesn't give us the option to set the nonce to something other than a number. The input field is controlled by the context value so we would have to adjust the context i.e. store the nonce as a string but I am not sure if we want this. Wdyt @katspaugh
There was a problem hiding this comment.
The input can operate on strings and the context can operate on numbers.
You can probably do something like:
<Input
value={String(nonce || '')}
onChange={(e) => {
setNonce(e.target.valueAsNumber)
}}
/>
There was a problem hiding this comment.
Problem is that nonce will always be defined so String(nonce || '') would never result in an empty string. We can either change how we store nonce in the Context or make a copy of it for the nonce form and operate on that. The latter option was how we had it before where we copied the nonce value into RHF but it adds more complexity to the form since we have to keep it up to date depending on the queue.
There was a problem hiding this comment.
I've added an additional isEmpty flag in the form now to achieve this.
What it solves
Resolves #2110
How this PR fixes it
How to test it
Screenshots
Checklist