-
Notifications
You must be signed in to change notification settings - Fork 96
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
Implement creating future transactions #5390
Implement creating future transactions #5390
Conversation
…317-enable-creating-future-transactions
src/modules/transaction/components/TxSignatureCollector/TxSignatureCollector.js
Outdated
Show resolved
Hide resolved
src/modules/transaction/components/TxSignatureCollector/TxSignatureCollector.js
Outdated
Show resolved
Hide resolved
…317-enable-creating-future-transactions
const setNonceByAccount = useCallback( | ||
(address, nonce, transactionHex) => dispatch(setAccountNonce(address, nonce, transactionHex)), | ||
[] | ||
); |
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.
there would be no need for a call back here since it might impact on memory.
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.
The present implementation of the nonce caching is scary in the sense that there is no history clean up strategy.
Since for every multi sig transaction a user creates, we are always keeping a record of it at a point a user would have like eg 200 transactions possibly from different accounts on that device and since there is no way a transaction done at index 0 would have a nonce greater than the transaction done at index 100.
I think there should be a sanitisation of this states so as not to fill up local storage space (remember it has a max size of 10mb) since transactions HEX can be quite very lengthy.
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.
The fact that a transaction might most likely be completed on a different device than it was initiated on plays a role in why this can be tricky to solve. However, the fix for clearing all stored nonce can play a role in this
return { | ||
...state, | ||
[address]: { | ||
...state[address], | ||
[transactionHex]: nonce, | ||
}, | ||
}; |
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.
As a follow up to my above statement this create a history of transactions created, though there is not clean up strategy.
const handleLocalNonce = (currentNonce) => { | ||
const storedNonce = BigInt(currentAccountNonce || 0); | ||
const localNonce = storedNonce < currentNonce ? currentNonce : storedNonce; | ||
const localNonceStr = localNonce.toString(); | ||
setNonceByAccount(currentAccountAddress, localNonceStr, 'defaultNonce'); | ||
|
||
setAccountNonce(localNonceStr); | ||
}; | ||
|
||
useEffect(() => { | ||
handleLocalNonce(onChainNonce); | ||
}, [onChainNonce]); |
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.
This can be done in a memo
eg
const handleLocalNonce = (currentNonce) => { | |
const storedNonce = BigInt(currentAccountNonce || 0); | |
const localNonce = storedNonce < currentNonce ? currentNonce : storedNonce; | |
const localNonceStr = localNonce.toString(); | |
setNonceByAccount(currentAccountAddress, localNonceStr, 'defaultNonce'); | |
setAccountNonce(localNonceStr); | |
}; | |
useEffect(() => { | |
handleLocalNonce(onChainNonce); | |
}, [onChainNonce]); | |
const accountNonce = useMemo(() => { | |
const storedNonce = BigInt(currentAccountNonce || 0); | |
const localNonce = storedNonce < currentNonce ? currentNonce : storedNonce; | |
const localNonceStr = localNonce.toString(); | |
setNonceByAccount(currentAccountAddress, localNonceStr, 'defaultNonce'); | |
return localNonceStr | |
}, [currentAccountNonce, currentAccountAddress, ]) |
What was the problem?
This PR resolves #5317
How was it solved?
How was it tested?