Skip to content
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

Merged
merged 23 commits into from
Oct 25, 2023

Conversation

ikem-legend
Copy link
Member

@ikem-legend ikem-legend commented Oct 13, 2023

What was the problem?

This PR resolves #5317

How was it solved?

  • Create account nonce sync hook
  • Implement future transaction warning

How was it tested?

  • Creating multiple transactions and the nonce should increase for subsequent transactions provided the previous ones haven't been signed. Nonce warning should be displayed when transaction nonce is greater than on-chain nonce
  • Unit tests

@ikem-legend ikem-legend self-assigned this Oct 13, 2023
@ManuGowda ManuGowda marked this pull request as ready for review October 19, 2023 06:38
Comment on lines 26 to 29
const setNonceByAccount = useCallback(
(address, nonce, transactionHex) => dispatch(setAccountNonce(address, nonce, transactionHex)),
[]
);
Copy link
Contributor

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.

Copy link
Contributor

@eniolam1000752 eniolam1000752 Oct 25, 2023

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.

Copy link
Member Author

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

Comment on lines +67 to +73
return {
...state,
[address]: {
...state[address],
[transactionHex]: nonce,
},
};
Copy link
Contributor

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.

Comment on lines +33 to +44
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]);
Copy link
Contributor

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

Suggested change
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, ])

@ikem-legend ikem-legend merged commit 2d292c5 into release/3.0.0 Oct 25, 2023
3 checks passed
@ikem-legend ikem-legend deleted the 5317-enable-creating-future-transactions branch October 25, 2023 17:18
@ManuGowda ManuGowda removed their request for review October 26, 2023 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants