Metamask-extension: Better support for batch transactions

[or2008]

Bounty: improve MetaMask's support for transactions sent with the batch web3 method, and improve support for standard transactions proposed in quick sequence. Specifically, MetaMask should:

• Stack transactions in a single popup in the order they're proposed
• Support batch transactions: stack transactions in a single popup in the order they're added to the batch
• Display N of M UI element in new UI to communicate when the user has multiple transactions waiting for confirmation (shown in this comment)
• Make sure transactions are queued in the same order in the standalone popup & in the extension view (when opening MetaMask from the top-right extension icon)

For questions or clarifications, ping @bdresser

--

see image attached, this is the first view I see when sending 2 transactions to metamask, looks like the order is reversed, (the 2nd transaction displays first)

Edit: I am using Promise.all to batch the transactions together.
Edit2: just to be clear, it was working as excepted few days ago, looks like a recent version caused that issue

[Zanibas]

I believe if I'm remembering correctly, this is actually an anti-phishing measure to prevent a dapp from "surprising" their users by asking for another tx last second and causing the user to submit that one instead of the original one.

We are open to discussion on this topic though, if you feel that isn't the correct design decision.

[yudilevi]

Thanks @Zanibas

Many times it's required because the ERC20 allowance mechanism so we need users to also sign an approve transaction before executing the actual transaction.

So how do you suggest we handle multiple transactions in this case?

[Zanibas]

This is a legitimate concern and I will relay this at our next sprint. Apologies if we get a bit slow--keep pinging us and we'll get to it!

[danfinlay]

Raising priority on this because it is breaking the flow for Bancor. We managed to not discuss it during the previous sprint planning (we need to get better at using that discuss at next sprint tag), so this may be as late as the April 10th release.

@yudilevi To keep your app functional for now, I would recommend simply approving one transaction after another.

This may also be fixed by a fix to #1666 (#3513).

[trigun0x2]

@danfinlay I haven't looked into this. And my PR in it's current state will not fix this.

[yudilevi]

Any idea if/when this issue is going to be resolved?

[danfinlay]

If: Yes.
when: By April 22nd.

[yudilevi]

@danfinlay Great, thank you! :)

[rstormsf]

@danfinlay quick heads up:
here how I "hacked" it in order to be displayed in order: 1,2,3
https://github.com/rstormsf/Dapptester/blob/master/src/App.js#L117
Add second first, then third then first as last :-)
You can test here by calling: Test Batch send button
https://rstormsf.github.io/Dapptester/

What I'd like to ask is how to get txHash when using batch send for each tx?

Also even though all tx are mined in order 1,2,3, the callbacks are executed in the order of batch.add, not in the order when tx was mined

[or2008]

@rstormsf did you hack it in the new beta version? because this solution doesn't work with the old metamask UI

[didil]

also facing this issue

[rstormsf]

@or2008 I did in new Beta UI and old UI and the hack works

[or2008]

@rstormsf Thanks, but it doesnt work for me, this is what I see with the old UI:

[rstormsf]

first tx should have 1.01 gwei which is correct.
second tx should have 2.02 gwei gasPrice
third tx should have 3.03 gwei gasPrice

FYI - please don't waste money on mainnet, use Kovan, Rinkeby, or Sokol POA

about list of tx - not sure why you are not seeing list of tx, I do have them

Also this Dapptester contracts has not been deployed to Mainnet, that's probably why you are not seeing batch working on Mainnet

[yudilevi]

Hey guys, any update on this one?

[rstormsf]

@yudilevi just use a hack as workaround :-)

[didil]

I ended up using a 2s timer in between calls, that made the popups display in the right order

[or2008]

@didil can you please share the code you used?

[rstormsf]

@or2008 I think he just used setTimeout to add delay

[didil]

@or2008 yes I just used setTimeout

[wbt]

@Zanibas
Per your March 5 comment, it seems to me that putting the last transaction on top of whatever the user might be looking at just before clicking Submit makes that kind of last-second-new-transaction attack _easier_, not harder. To thwart that, new transactions should join the end of the queue so a user can finish approving/rejecting the first transactions first.

[or2008]

any update when are you going to fix this? @danfinlay

[danfinlay]

@or2008 No update. We will get to this when we don't have more important issues. Please use a timeout in the meanwhile.

[ashachaf]

batch transactions is a web3 standard for sending multiple transactions that design to get executed at a pre defined order https://github.com/ethereum/wiki/wiki/JavaScript-API#batch-requests.

this is the best practice way to send "approve" transaction ahead of another and make sure they are executed in the correct order.

few existing issues on metamast:

1. ignore nonce - we send transactions with pre defined nonce that indicate their execution order. this is ignored by metamask and override upon signing. resulting with users signing in the wrong order and sending transaction to get executed in an invalid order.

1. display batched transactions incorrectly (current metamast version) - when sending 2 transactions, the users sign window indicate the last transaction as the first to sign. this along with the nonce override issue cause bad execution and failed transactions

2. not supporting batched transactions (new metamask beta version) - new beta only display 1 of the batched transactions.

as this is best practice and we will see more systems supporting multiple transactions that require specific order of execution, we encourage you to fix these issues and align with best practice.

[yudilevi]

guys, should we assume this issue is low priority and thus we shouldn't expect a fix soon?

[danfinlay]

guys, should we assume this issue is low priority and thus we shouldn't expect a fix soon?

Yes, sorry. Will post a bounty soon.

[bdresser]

I'd like to get a bounty up for this by the end of the week. To make sure we're on the same page - expected behavior is:

• Stack transactions in a single popup in the order they're proposed
• Support batch transactions: stack transactions in a single popup in the order they're added to the batch
• Display N of M UI element in new UI to communicate when the user has multiple transactions waiting for confirmation (shown below)

we send transactions with pre defined nonce that indicate their execution order. this is ignored by metamask and override upon signing.

@ashachaf check out https://github.com/MetaMask/metamask-extension/issues/1410 for a lengthy discussion on why we don't allow dapps to specify the nonce.

Anything else to add? (cc @danfinlay)

[ashachaf]

@bdresser @danfinlay
yes, batch transaction means:

1. Stack transactions in a single popup in the order they're proposed
2. Support batch transactions: stack transactions in a single popup in the order they're added to the batch
3. Display N of M UI element in new UI to communicate when the user has multiple transactions waiting for confirmation (shown below)

if the above is handled correctly, there will be no need to keep the nonce as we submit it simply because the sign order will be aligned with the valid order of execution.

[wbt]

Would it be reasonable to combine this with #4050 in the bounty post?

[bdresser]

@wbt it would, but #4050 is going to require new designs to make sure users can see & understand all the transactions they're submitting at a glance.

It's more likely that gets combined with #4022, which would share a similar view.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

__This issue now has a funding of 0.44 ETH (86.24 USD @ $196.01/ETH) attached to it.__

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
  
  
  
  • Want to chip in? Add your own contribution here.
  
  
  • Questions? Checkout Gitcoin Help or the Gitcoin Slack
  
  
  • $23,025.20 more funded OSS Work available on the Gitcoin Issue Explorer
  
  

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

__Work has been started__.

These users each claimed they can complete the work by 6 months, 1 week from now.
Please review their action plans below:

1) paddymc has started work.

• Reverse the order transactions are displayed
• When multiple transactions are displayed, ensure they are stacked properly, with regards to popup & chrome extension button
• Use promises.All() functions to batch transactions
• Update UI to display 'N of M' transactions

I'll need a day or two to build metamask come up with a non-pseudo code solution, but I believe if I complete the tasks listed above the bounty will be completed, feel free to offer any suggestions or alterations to the task list as well.

Learn more on the Gitcoin Issue Details page.

[PaddyMc]

Just a quick update,

• I've build a sample DApp to test the batch transaction and I can reproduce the issues.

• For reversing the order of transactions I'm looking into the tx-state-manager::getUnapprovedTrxList && tx-state-manager::addTx functions.

• I'm also working on "stacking transactions in a single popup in the order they're proposed"

• After these tasks are complete I'll update the new UI to display 'N of M'

Progress has been pretty slow as the application is quite complex but it's starting to speed up now the setup is completed.

Feel free to offer any suggestions as well! :)

[gitcoinbot]

@PaddyMc Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] warning (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[PaddyMc]

To update,

• I've fixed the order the transactions are displayed => edited this file old-ui/lib/tx-helper.js

• I'm currently working on stacking transactions in a single popup

• I've investigated where to add the code to display 'N of M' in the new UI and I'm confident I will be able to complete this task

[bdresser]

sounds good @PaddyMc!

feel free to ping @alextsg if you have any questions about the confirmation screen UI

[PaddyMc]

Updates,

• Order transactions are displayed is completed

• The transactions are stacked in a single popup

• 'N of M' has been added to the new UI with the ability to switch between transactions

Currently working on,

• Refactoring code

• Testing

Should be able to create a PR in a few days!

[gitcoinbot]

@PaddyMc Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] warning (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[PaddyMc]

Updates,

• Added ability to navigate to first and last transaction

• All other task have been completed

Currently working on,

• Refactoring

• Tests

It'll be another few days before creating a PR, just to make sure everything is running correctly!

[PaddyMc]

Hey @bdresser,

I think I've found a bug with how the "send token" transactions are displayed when they're unapproved and are listed after they're confirmed.

I think it's something to do with this code => HERE

Step to reproduce:

• Open UI fullscreen
• Add token to wallet
• Send token from Account 1 => Account 2
• Check URL + UI

The URL should be "/send-token" but it's "/send-ether", the same problem occurs when displaying the completed transactions.

I will fix this for unapproved transactions with my PR, but I believe this still needs to be investigated specifically with regards to the list of confirmed token transactions. They display as "Sent Ether".

With regards to better support for batch transactions,

• I edited the ConfirmTransactionSwitch component to route correctly (this is where I found the bug)
• I'm unhappy with the way I re-ordered the batch transactions, so I've left out that code, what I wrote to fix the display order ended up making the popup less intuitive, I'd be more that happy to re-do this part, but this task has dragged on a bit, so I think I'll submit a PR for review then come up with a more elegant solution after the other work has been reviewed
• I'll create a PR for review today

[PaddyMc]

Hey @bdresser,

I don't think re-ordering the batch transactions is necessary, I've been testing for the past few days and the order seems OK to me, feel free to refute this if you feel it does need to be changed and I'll re-investigate it!

Therefore I'm gonna submit the work on this bounty, but I'll be able to make any alterations when the PR is reviewed! :)

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

__Work for 0.44 ETH (100.41 USD @ $228.19/ETH) has been submitted by__:

1. @PaddyMc

@bdresser please take a look at the submitted work:

• PR by @PaddyMc

---

• Learn more on the Gitcoin Issue Details page
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $41,627.98 more funded OSS Work available on the Gitcoin Issue Explorer

[PaddyMc]

@bdresser, Would ye like me to re-order the transactions to have the oldest transaction display first in both the new and old UI? It would be trivial enough to do, and I should be able to do it by the end of the day! It would be a case of editing this file for the new ui, and this file for the old ui.

[rmshea]

Hey @bdresser, Ryan from Gitcoin just checking in here. Looks like @PaddyMc submitted a PR for you to take a look at! Let me know if you have any questions :)

[kumavis]

@PaddyMc most important behavior to preserve is that a dapp cant change what tx confirmation you're looking at, while you're looking at it. If we set it to show latest, it should not switch to one submitted by a dapp while the ui is open.

[PaddyMc]

@kumavis thanks for your input, I believe it behaves that way now. If a new transaction is added and the popup is open, the current transaction will not change. And to ensure the correct behavior I'll add that test case to the PR.

[wbt]

Related to @kumavis latest comment: see discussion above, e.g. this comment from April.

[gitcoinbot]

⚡️ A tip worth 0.06600 ETH (14.0 USD @ $212.11/ETH) has been granted to @PaddyMc for this issue from @. ⚡️

Nice work @PaddyMc! To redeem your tip, login to Gitcoin at https://gitcoin.co/explorer and select 'Claim Tip' from dropdown menu in the top right, or check your email for a link to the tip redemption page.

• $47953.43 in Funded OSS Work Available at: https://gitcoin.co/explorer
• Incentivize contributions to your repo: Send a Tip or Fund a PR
• No Email? Get help on the Gitcoin Slack

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

__The funding of 0.44 ETH (93.33 USD @ $212.11/ETH) attached to this issue has been approved & issued to @PaddyMc.__

• Learn more on the Gitcoin Issue Details page
  
  
  
  • Questions? Checkout Gitcoin Help or the Gitcoin Slack
  
  
  • $47,860.10 more funded OSS Work available on the Gitcoin Issue Explorer
  
  

[ashachaf]

hi,

i have tested the new support for batch transaction to find a strange behavior.

for reference, i am using metamask version 5.0.2

when triggering a batched transaction that contains 2 transactions, the following happens:

• i receive a message to sign transaction #2
• i receive a message to sign transaction #1
• i receive a message to sign transaction #2 (again, with the same nonce as the one above)
  if i sign them in the displayed order, metamask force the nonce to the transaction by order of signing, not keeping the nonce indicated in the transactions that are prompt to metamask.

1st popup message (actually #2)

2nd popup message (supposed to be #1)

3rd popup message (supposed to be in 2nd place)

as expected, the transaction fails eventually as i sign it in the wrong order and order has a meaning.

[PaddyMc]

Hey @ashachaf ,

Thanks for your input but the code has not been released yet, it just got merged into dev yesterday. Would you mind re-testing when the code is published?

I tested your issue on Metamask v5.0.2.

[ashachaf]

sure thing!
let me know once this is published.

[ashachaf]

hi @PaddyMc,

please let me know when this code is released and i will test it.
thanks,

[PaddyMc]

hi @ashachaf this is now released in v5.0.3.

[ashachaf]

hi @PaddyMc,

i had a look at version 5.0.3 and notice the following:

1. you no longer display the 3rd popup which was a duplicated
2. you still reverse the order of transactions when handling batch transactions.

example:
when sending 2 transactions as a batch (approve + quickconvert) this is what i see:
first item to sign is actually the 2nd transaction (notice the indicated nonce as well)

next item to sign is actually the 1st transaction

since metamask is forcing nonce and overriding the defined nonce in the transaction, you must reverse the order of transactions if you want them to execute as intended.

[whymarrh]

Hey @ashachaf, please open up a new issue if you have feedback on what's already been merged

[ashachaf]

hi @whymarrh,

this issue was created to support batch transactions which is still not the case as you can see.
should i create a new issue that is identical to this one or should we keep the same thread?

thanks,

[ashachaf]

hi @whymarrh ,

please find the new issue created to fix the wrong order of screens:
https://github.com/MetaMask/metamask-extension/issues/5852

best,