Metamask-extension: Batched eth_sendTransaction opens multiple popups

[danfinlay]

As reported on this stackexchange post, I've confirmed that using the batch api to send multiple transaction signing requests opens many metamask windows.

Should be a simple fix.

[danfinlay]

Reproduction code:

var a = web3.eth.sendTransaction.request({ from: web3.eth.accounts[0], to: web3.eth.accounts[0], value: 0 })
var a = web3.eth.sendTransaction.request({ from: web3.eth.accounts[0], to: web3.eth.accounts[0], value: 0 })
var batch = web3.createBatch()
batch.add(a)
batch.add(b)
batch.execute()

[glowkeeper]

Oooo - I'm the person who opened the question on StackExchange. My question was, essentially - how should I form a sendAsync call so I can send multiple transactions, but have them signed only once? For instance, say I had these three transactions:

myContractObject.doSomething(name,type)
myContractObject.doSomethingElse(hash)
myContractObject.doSomethingElseEntirely(key)

What would my call to web3.currentProvider.sendAsync look like?

...however, batch.add would be way cleaner than sendAsync! Thanks for opening the issue....

[danfinlay]

Oh, that's an interesting distinction. That could be another feature to look at eventually, but we're still working on making a single transaction approval more comprehensible, so multiple at once is probably a little complexity beyond what we're ready for at the moment.

Feel free to open that as a separate issue here, as a feature request.

[glowkeeper]

Was that comment aimed at me? If so, I'm not certain what interesting distinction I've made :(

[danfinlay]

The API batch submission vs the user confirmation batch approval.

This issue is for the bug where API batch submission opens many pop ups.

[danfinlay]

Very interesting @glowkeeper, looks like you found an older version of this bug that we closed. Must have regressed.

https://github.com/MetaMask/metamask-plugin/issues/1000#issuecomment-310945715

[glowkeeper]

Oops :(

[glowkeeper]

So I've just added 14 transactions to a batch and executed. While it kinda works (it writes the required records to testrpc), I get this:

TypeError: message is undefined[Learn More] bundle.js%20line%203478%20%3E%20eval:80:9
Jsonrpc.toBatchPayload/< http://127.0.0.1:8081/bundle.js%20line%203478%20%3E%20eval:80:9
map self-hosted:276:17
Jsonrpc.toBatchPayload http://127.0.0.1:8081/bundle.js%20line%203478%20%3E%20eval:79:12
RequestManager.prototype.sendBatch http://127.0.0.1:8081/bundle.js%20line%208658%20%3E%20eval:105:19
Batch.prototype.execute http://127.0.0.1:8081/bundle.js%20line%208511%20%3E%20eval:48:5
executeBatch

And after (annoyingly) clicking through 14 'Accepts' in the batch transaction (I only want to click one!), this happens:

...and that spinning blank window remains for evermore. Furthermore, it seems to fire up a lot of those spinning blank windows:

If I kill those, they leave annoying opaque squares:

I can only get rid of by rebooting! Hmmmm....

[wjagodfrey]

This is a pain. Currently processing several hundred transactions at once. Any fix planned in the near future?

[danfinlay]

@wjagodfrey If you're processing several hundred transactions at once, you might want to consider signing programmatically, not with MetaMask.

[wjagodfrey]

We have other node transactions being signed programmatically. The benefit of MetaMask is that you don't have to. Do you mean sign them and sendRawTransaction via MetaMask, or not use MetaMask at all?

Edit: "other node" as in nodes other than MetaMask.

[danfinlay]

You could do either. I'm just saying using MetaMask accounts is not ideal for signing hundreds of transactions.

[wjagodfrey]

Ah right, thanks for the warning. We've had some trouble with MetaMask (e.g. https://github.com/MetaMask/metamask-extension/issues/1000 and others), so we'll make it very clear to the client to proceed with caution. Thankfully it's not the only wallet we support. Thanks for your help.

[danfinlay]

We escalated the priority on this feature days before you asked, it’s not that we will never improve this, it’s just even then, it’s a strange use of MM. Maybe you’d like to describe your use case so we can sympathize with it.

[wjagodfrey]

I understand. Usecase is https://paycheckapp.io/. MetaMask is an option we support.

[trigun0x2]

Any updates on this? I'm still getting one popup per batch transaction added.

[danfinlay]

Sorry, this hasn't made top priority in a while, we've been a bit over-busy. I'm marking as bounty worthy so we might add a bounty to encourage an external contribution.

[trigun0x2]

Thanks for the response @danfinlay I will take a look at the code.

[trigun0x2]

Sorry, I've been busy for the paste few weeks, but I'm making some good progress on this issue.

Seems like NotificationManager._getPopupIn() checks the width/height to confirm that a popup window is already open. This fails in some Linux window managers (which opens all windows in max height) and possibly when some extensions affect the window size.

Is there any particular reason the height/width is being checked? I see that the NotificationManager can only create one type of popup anyways. Removing this constraint fixes the issue for me.

@danfinlay please advise.

[danfinlay]

This height check was added by @kumavis, and if it isn't working well, I think we're definitely open to a better check. I suspect it was just a cheap way of checking if a popup was open.

[trigun0x2]

@danfinlay thanks for the heads up. I've tried the extension without the width/height check and it works fine on Linux. Seems like if you guys are planning on adding different types of popup windows, it might be valuable. Though in that situation it might be worth assign the windows some sort of id.

@kumavis please let me know the purpose and I'll brainstorm an alternative.

[kumavis]

@trigun0x2 feel free to remove it, sounds like some code i haevn't touched in more than a year

[kumavis]

here's the commit where it was added, its very ancient (Sep 13, 2016)
https://github.com/MetaMask/metamask-extension/commit/11363b4f2a3191806995dbb3d1c01fe9ab403f0d

feel free to remove if it fixes the issue

[trigun0x2]

I'm going to package the build without the width/height check and test it on a few browsers before making the PR. If you recall why you added the checks @kumavis that'd be helpful (I don't want to break anything I don't know).

[gitcoinbot]

__This issue now has a funding of 0.15 ETH (112.56 USD @ $750.43/ETH) attached to it.__

• If you would like to work on this issue you can claim it here.
• If you've completed this issue and want to claim the bounty you can do so here
• Questions? Get help on the Gitcoin Slack
• $6686.73 more Funded OSS Work Available at: https://gitcoin.co/explorer

[vs77bb]

@trigun0x2 Mind clicking 'Start Work' on Gitcoin to claim the issue? Instructions above^ 🙂

[gitcoinbot]

__Work has been started on the 0.15 ETH (112.56 USD @ $750.43/ETH) funding by__:

1. @trigun0x2

__Please work together__ and coordinate delivery of the issue scope. Gitcoin doesn't know enough about everyones skillsets / free time to say who should work on what, but we trust that the community is smart and well-intentioned enough to work together. As a general rule; if you start work first, youll be at the top of the above list ^^, and should have 'dibs' as long as you follow through.

On the above list? Please leave a comment to let the funder (@vs77bb) and the other parties involved what you're working, with respect to this issue and your plans to resolve it. If you don't leave a comment, the funder may expire your submission at their discretion.

• Learn more on the gitcoin issue page
• Questions? Get help on the Gitcoin Slack
• $6686.73 more Funded OSS Work Available at: https://gitcoin.co/explorer

[trigun0x2]

Thanks @vs77bb appreciate the bounty! Are you also on Linux? I'm in the process of testing right now but won't be able to do Microsoft Edge.

[trigun0x2]

There seems to be a bit more at work here. Please refer to https://github.com/MetaMask/metamask-extension/pull/3513
@danfinlay

[glowkeeper]

Closed? Does this mean it's fixed? Didn't appear so yesterday!

On 21 March 2018 at 19:42, kumavis [e-mail] wrote:

Closed #1666 https://github.com/MetaMask/metamask-extension/issues/1666
via #3656 https://github.com/MetaMask/metamask-extension/pull/3656.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/MetaMask/metamask-extension/issues/1666#event-1534089169,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AI1fIfyl0HhKNFKQOCKwjHYWy9XZc_joks5tgq0UgaJpZM4OEVcO
.

[trigun0x2]

@glowkeeper it's been recently merged. Can build locally and test? Or I can give you a zip file to test.

[glowkeeper]

Sure - I'll go grab the repository....

On 21 March 2018 at 22:43, Jeffrey Tong [e-mail] wrote:

@glowkeeper https://github.com/glowkeeper it's been recently merged.
Can build locally and test? Or I can give you a zip file to test.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/MetaMask/metamask-extension/issues/1666#issuecomment-375120021,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AI1fIRTUNQQsm4zG7SqYaEJizei6D3Zfks5tgteDgaJpZM4OEVcO
.

[glowkeeper]

I just cloned and built, then uploaded that to Chrome - my MetaMask there
said I was running version 4.3.0. Right? Anyway, did a batch.add of 17
transactions to Rinkeby. Was hoping to get a single prompt to sign, but
MetaMask still asked me to sign all 17 transactions. Boo. Did I miss
something?

Steve

On 22 March 2018 at 14:36, Steven Huckle [e-mail] wrote:

Sure - I'll go grab the repository....

On 21 March 2018 at 22:43, Jeffrey Tong [e-mail] wrote:

@glowkeeper https://github.com/glowkeeper it's been recently merged.
Can build locally and test? Or I can give you a zip file to test.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/MetaMask/metamask-extension/issues/1666#issuecomment-375120021,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AI1fIRTUNQQsm4zG7SqYaEJizei6D3Zfks5tgteDgaJpZM4OEVcO
.

[trigun0x2]

@glowkeeper damn. You might be experiencing a different issue than I. Really hard for me to test without getting sending a debug build. I sort of patched the issue because fixing the issue from ground up would cause a change in their entire naming convention. Maybe we can talk off github and I can help you debug?

[glowkeeper]

Sure - where do you suggest?

On 22 March 2018 at 17:22, Jeffrey Tong [e-mail] wrote:

@glowkeeper https://github.com/glowkeeper damn. You might be
experiencing a different issue than I. Really hard for me to test without
getting sending a debug build. I sort of patched the issue because fixing
the issue from ground up would cause a change in their entire naming
convention. Maybe we can talk off github and I can help you debug?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/MetaMask/metamask-extension/issues/1666#issuecomment-375388966,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AI1fIWA4wwSB9iTGRbp3IMLO-Lto5yhtks5tg93jgaJpZM4OEVcO
.

[gitcoinbot]

__The funding of 0.15 ETH (58.71 USD @ $391.42/ETH) attached to this issue has been killed by the bounty submitter__

• Questions? Get help on the Gitcoin Slack
• $2729.73 more Funded OSS Work Available at: https://gitcoin.co/explorer

[trigun0x2]

@vs77bb this issue was fixed in my PR. I thought it'd update on the gitcoin automatically.

[vs77bb]

@trigun0x2 Apologies for the confusion. A tip was just sent your way for the 0.15 ETH! Let me know if you have any issues retrieving.