Refactor: reactive recommended nonce #2050
Conversation
Branch preview✅ Deploy successful! https://recommended_nonce--walletweb.review-wallet-web.5afe.dev |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
katspaugh
force-pushed
the
recommended-nonce
branch
2 times, most recently
from
6644c32 to
c8f882b
Compare
katspaugh
force-pushed
the
recommended-nonce
branch
from
c8f882b to
c76ff83
Compare
Reduce re-renders Nonce -1
katspaugh
force-pushed
the
recommended-nonce
branch
from
a815695 to
35c095a
Compare
katspaugh
force-pushed
the
recommended-nonce
branch
from
b5c1e9b to
94cc648
Compare
| import { type SafeTransaction } from '@safe-global/safe-core-sdk-types' | ||
|
|
||
| const useWalletCanRelay = (tx: SafeTransaction | undefined) => { | ||
| const { safe } = useSafeInfo() | ||
| const wallet = useWallet() | ||
| const hasEnoughSignatures = tx && tx.signatures.size >= safe.threshold |
There was a problem hiding this comment.
I think we could even move this into the useAsync call since it already has a dependency on safe.threshold
There was a problem hiding this comment.
Tx changes whenever the nonce changes, so that would be bad. That's the whole point why I moved it out here and in gas estimation.
There was a problem hiding this comment.
Its probably more of a nit but the hook depends on hasEnoughSignatures which updates every time tx updates so it would still run when the nonce changes I assume. Just noticed that we have safe.threshold in that dependency array even though it is not being used accessed anymore.
There was a problem hiding this comment.
No, it will update only if the number of signatures change.
Good catch with the safe.threshold, removed.
| | { | ||
| nonce: number | ||
| safeTxGas: number | ||
| } |
There was a problem hiding this comment.
nit: We could extract this into a type that can be reused by getRecommendedTxParams as well.
| [safeTxData?.to, safeTxData?.value, safeTxData?.data, safeTxData?.operation], | ||
| ) | ||
|
|
||
| const [recommendedParams] = useAsync(() => { |
There was a problem hiding this comment.
I would suggest also exporting the loading flag here since there is a small window when creating transactions now where it will show the execute option in any case until the nonce is estimated.
katspaugh
force-pushed
the
recommended-nonce
branch
from
63319fa to
2337f6b
Compare
katspaugh
force-pushed
the
recommended-nonce
branch
from
2337f6b to
c4598b2
Compare
| <DialogContent> | ||
| {props.children} | ||
|
|
||
| <TxSimulation |
There was a problem hiding this comment.
This is now in the wrong order. I think it should be below the ExecuteCheckbox and Simulation block
|
|
||
| return ( | ||
| <form onSubmit={handleSubmit}> |
There was a problem hiding this comment.
By removing the form element here we lost some padding. We could add a div instead or adjust the css for DialogContent
There was a problem hiding this comment.
Why does it matter? There'll be a new design for all of these things.
|
|
||
| // If the nonce is not provided, we get the recommended one | ||
| if (nonce === undefined) { | ||
| const recParams = (await getRecommendedTxParams(txParams)) || {} | ||
| txParams = { ...txParams, ...recParams } | ||
| } else { | ||
| // Otherwise, we use the provided one | ||
| txParams = { ...txParams, nonce } | ||
| } | ||
|
|
There was a problem hiding this comment.
I just realized that we don't estimate the nonce anymore for the initial tx that is passed to SignOrExecuteForm. Thats probably the reason why the execute checkbox is showing initially. I suggest we restore this block.
There was a problem hiding this comment.
It also applies to the other create calls. We might need the withRecommendedNonce helper after all.
What it solves
Resolves #2034
What this PR does
The "recommended" nonce was previously fetched and set only once, when generating a SafeTransaction.
If the queue updates (and thus the recommended nonce) while the user is still looking at the Sign/Execute modal, neither the Advanced Params form, not the SafeTransaction would pick up an updated recommended nonce.
With this PR, the recommended nonce is fetched every time the queue is updated.
It still doesn't guarantee that the user doesn't submit the modal with an outdated nonce, but it makes it much more robust.