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

Stops the unintended 'User Rejected...' error caused by buggy submitt… #1578

Merged
merged 5 commits into from
Jun 4, 2020

Conversation

danjm
Copy link
Contributor

@danjm danjm commented May 14, 2020

Fixes #1420


  1. Create a new wallet
  2. Switch to the ropsten network
  3. Go to the metamask faucet and get 1 eth for your Account 1
  4. Create a new account, Account 2, so that your wallet has two accounts
  5. Go to the metamask faucet and get 1 eth for your Account 2
  6. Once the transaction is confirmed, go to your Transaction History
  7. Now open the wallet sidebar and click "Send"
  8. Send a transaction (this will be a transaction sent from account 2).
  9. Wait for the transaction to confirm.
  10. Switch to Account 1
  11. go to your Transaction History
  12. Now open the wallet sidebar and click "Send"
  13. When you attempt to send this transaction, you will see the "User rejected this transaction" error, but when you cancel you will see that the transaction was submitted.

From this state, you can continuously recreate the error by doing the following:
14. Wait for the last transaction to confirm
15. Switch to Account 2
16. Repeat steps 7-13


Explanation of why this is happening from the perspective of the code.

  1. When the "Transaction History" is open, the TransactionView component mounts
  2. On every update of the TransactionView component, its normalizeTransactions method is called
  3. When in the send flow after having been on the transaction history page, the TransactionView component stays mounted. So when in this flow and confirming a transaction by clicking "Send", the normalizeTransactions method of TransactionView will be called.
  4. normalizeTransactions compares the nonces of all submitted transactions to all confirmed transactions, if any of the submitted tx nonces are the same as the confirmedTx nonces, TransactionController.cancelTransaction is called on that submitted transaction.
  5. Important: the confirmed transactions here includes all confirmed transactions from all accounts. If the just submitted transaction has a nonce equal to the nonce of an already confirmed transaction from another account then the cancelTransaction method mentioned in 4 will be called.
  6. cancelTransaction causes an error to be emitted that causes the modal "User rejected..." message to be shown
  7. However, step 5 and 6 happens after the TransactionController.approveTransaction method has been called and sent the transaction to the blockchain. Which is why the transaction still goes through

const submittedNonces = [];
submittedTxs = submittedTxs.filter(transaction => {
const alreadyConfirmed = confirmedTxs.find(
const alreadyConfirmed = currentAccountConfirmedTxs.find(
Copy link
Member

@andrepimenta andrepimenta May 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here couldn't we do:
tx => tx.transaction.nonce === transaction.transaction.nonce && tx.transaction.from === this.props.selectedAddress.toLowerCase()?

In this way we can remove the previous filter and we would do only 1 cycle. This would be noticeable in terms of performance if confirmedTxs possibly gets large.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have addressed this in the latest commit

Copy link
Member

@andrepimenta andrepimenta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a small comment about the filter, to see what you think.

But apart from that, all the logic and solution looks great! 👍

@ibrahimtaveras00 ibrahimtaveras00 added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label May 29, 2020
@ibrahimtaveras00 ibrahimtaveras00 requested a review from a team as a code owner May 29, 2020 01:12
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix looks good, QA Passed 👍

@ibrahimtaveras00 ibrahimtaveras00 added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels May 29, 2020
@danjm danjm merged commit 6d254ff into develop Jun 4, 2020
@danjm danjm deleted the fix-nonce-comparison-in-normalizeTx-methods branch June 4, 2020 16:06
rickycodes pushed a commit that referenced this pull request Jan 31, 2022
#1578)

* Stops the unintended 'User Rejected...' error caused by buggy submitted<>confirmed nonce checks

* Improve filtering logic

Co-authored-by: Ibrahim Taveras <ibrahimtaveras00@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release QA Passed A successful QA run through has been done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When sending a transaction, "Transaction error" alert pops up but transaction still goes through
4 participants