Metamask-extension: batch transactions appear in wrong order and fail my transactions

[ashachaf]

Describe the bug
when trying to sign a batched transaction, it actually appear in the wrong order on metamask and force nonce that sends the transactions in the wrong order to the blockchain, resulting in a fail transaction due to that.

To Reproduce
Steps to reproduce the behavior:

1. Go to 'https://www.bancor.network/tokens'
2. Click on 'convert'
3. select to execute the transaction using metamask
4. select any pair of tokens to convert (select erc20, not ETH or BNT)
   

1. view the transaction on metamask
   

Expected behavior
expected: 1st transaction is "approve", 2nd is the quickconvert
actual: 1st transaction is quickconvert (should be 2nd), 2nd is "approve" (should be 1st)

Screenshots
metamask interface display the transactions in the wrong order (notice order and nonce)

Browser details (please complete the following information):

• OS: Windows
• Browser chrome
• MetaMask Version 5.0.3

Additional context
old UI handle the transactions incorrectly as well, where it orders them correctly but let the user sign the 2nd transaction first.
this is the popup you will see

[ashachaf]

@whymarrh @PaddyMc,
please let me know if you require additional information on this bug.
thanks,

[ashachaf]

@whymarrh @danfinlay,
bug #3425 was opened to fix an existing issue. the work was done and published yet it fixed nothing.
how can we reopen this topic and make sure it is sorted?

thanks,

[or2008]

any updates?

[mitar]

For me this is a problem with simple ERC-20 token, where I first call approve on it and then transferFrom (wrapped inside a call to my contract's method) to transfer tokens. If I batch them together, they are not shown in the correct order, so transferFrom is called before approve. Which of course does not work.

[danfinlay]

If someone can provide a sample dapp demonstrating how batching is not working correctly, I'll reopen this issue.

[ashachaf]

Hi @danfinlay,

If you try to convert any ERC20 token to another token using bancor.network, you will be able to see how batch transactions are not handled correctly on metamask.

I can provide more details yet it is all available both online and in this thread.

Best,

[0v1se]

Hi everyone.
I get the same bug. Sometimes order is wrong when I use batch.
But this behavior is rather unstable. It occurs about 25%.

This is shown when order is incorrect (although caption says 1 of 2 )

And this is shown when the order is correct (approve transaction added first to the batch)

[0v1se]

Also, more information found: When I batch.execute(), new chromium window is opened.
My colleague has slightly different behaviour: window is not opened automatically. If he presses metamask button on the toolbar, 1 of 2 transactions is always shown

[adamsoffer]

Running into this issue as well! batch.execute() is confirming transactions in an incorrect order. Not an issue with other web3 providers I've tested with. Seems there are a few open issues related to this. Any ideas @danfinlay @kumavis ?

[danfinlay]

Can anyone provide a low-level example? batch.execute() is not a provider method, so it seems likely to me that this is the web3.js convenience library misbehaving. Some sample code would really be _the_ thing to allow easy fixing of this. Sorry it's just not high enough of a priority for me to go digging into Bancor's code, since the workaround (delay serial transactions) is so simple.

[adamsoffer]

@danfinlay Here ya go: https://codesandbox.io/s/metamask-batchrequest-bug-demo-emnph?fontsize=14

Instructions:

1. Connect to Rinkeby on MetaMask
2. Click "Activate MetaMask"
3. Click "Send Batch Request"

Clicking "Send Batch Request" will attempt to batch submit 3 transactions to a contract I deployed to rinkeby with a method called setNumber which will revert if you try and set a number lower than the currently set number. Notice in this example I set up that the third transaction fails because metamask always submits what should be the 2nd transaction last (ie 1,3,2 instead of 1,2,3).

Works fine with any of the other providers.

[adamsoffer]

Let me know if there's anything else you need to help reproduce and debug.

[adamsoffer]

@danfinlay Trying to debug. I think the issue might be happening somewhere here? Or maybe earlier before that function is invoked. Any suspicions?

[adamsoffer]

Also, until this is fixed, do you mind sharing that simple workaround you mentioned (delaying serial transactions)? Maybe in a forked version of that codesandbox I sent?

[danfinlay]

A fix is starting to roll out tomorrow. I recommend waiting.

[adamsoffer]

Hey @danfinlay - looks like transactions are being ordered correctly now. Awesome! Not sure if y'all are still working on it, but noticing a few issues introduced:

1. It no longer prompts you to confirm the next transaction in the batch after you click confirm which is confusing.
2. It no longer displays the total number of transactions in the extension, it always displays 1 even if there are 3 pending transactions in the batch to be confirmed.
3. The ability to navigate between each transaction is gone (not really an issue but noticed it was gone).

To illustrate: here's what a batch request of 3 transactions looks like for me now.

[danfinlay]

Thanks for the quick catches, @adamsoffer. We'll address these sooner than the batch bug. @rekmarks ^

[ashachaf]

A M A Z I N G !
@danfinlay thanks for finally sorting this out!
@adamsoffer thanks for picking up a long unresolved conversation and bringing it to resolution!

[rekmarks]

@adamsoffer just wanted to note that those UX issues you described should not be an issue as of 7.7.2