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

dApp cannot know about txs if MetaMask "speed up" button used #3585

Closed
miohtama opened this issue Jun 14, 2020 · 8 comments
Closed

dApp cannot know about txs if MetaMask "speed up" button used #3585

miohtama opened this issue Jun 14, 2020 · 8 comments
Labels
1.x 1.0 related issues Discussion Feature Request Stale Has not received enough activity

Comments

@miohtama
Copy link
Contributor

I am filing this issue as the discussion opener, it needs a wider community brainstorming to be solved.

Wallets, like MetaMask offer a "speed up transaction" feature where the past transaction is rewritten with new one to get rid of stuck transactions of too low gas price.

image

Expected behavior

dApp will find out about the transaction after "Speed up" has been used.

Actual behavior

Any sped up transactions are opaque to dApp.

Steps to reproduce the behavior

  1. Create a tranasction with any dApp with MetaMask
  2. Speed it up
  3. Now the new transaction has a new hash that dApp is not aware off

My suggestion with have a PromiEvent.on("replace") that emits (oldHash, newHash, newTxData) event for "replacement transaction". It will make the dApp logic tricky because often transactions are referred by their hashes and now dApps would need to track all transactions with their internally generated ids.

@miohtama
Copy link
Contributor Author

For reference, EtherScan can now show replacement transactions

image

@GregTheGreek
Copy link
Contributor

This is really interesting, definitely a big problem. I think this needs to be addressed at a much wider audience, I'm thinking eth magicians, because this would require a fundamental change in how developers write dapps. Would be happy to help out with the discussion if its moved there.

Thought:
"Speed Up" really just sends a new transaction to the mem pool with a % increase in the gas (I believe minimum 10%) and the same nonce, so Etherscan does some fancy work under the hood to recognise that this condition has been met, I want to believe their db does a check on nonces for a given account before updating the ui.

I think your suggestion is right on. Does metamask give any indication that a new transaction has been sent to the mem pool? If so, we can probably probably watch for that, and compare it against a previously known nonce? if not, we could track the currently selected account and poll for changes in the mempool, although i think this might slow things down a bit having to poll and filter through the mempool.

@lcdupree
Copy link

lcdupree commented Jul 25, 2020

We've had this issue in our issue backlog for Burner Machine since launching it last October. The promise semantics in web3js (and other web3 libraries) are pretty badly broken since a transaction includes 'gasPrice' in the calculation of the transaction hash and the replace-By-Fee mechanism means changing 'gasPrice' and, as a result, changing the transaction hash.

A standard pattern for calling a smart contract method from a dApp using the web3js library is to create a new Contract object parameterized by the contract's ABI. When you call one of the generated methods on this object, it returns an event emitter, and the next most natural thing to do is to wait on a 'transactionHash' event, so that you can then use this transaction hash to await a transaction receipt / confirmation. If your code is waiting on a transaction receipt event for a pending transaction, and the user invokes a replace-by-fee operation to up the gas price on the transaction, your code is going to end up waiting forever, and your user is probably going to have a pretty horrific experience...

Because of other eventing edge-cases like this one, we've already had to drop the usage and convenience of the event emitter pattern when waiting for a transaction receipt, and have instead had to resort to a more traditional polling mechanism (see code below #1). We're then forced to make an unhappy choice between polling more often and overburdening our Ethereum provider, or polling less often and having a dApp that's less responsive. We also have to write more boilerplate code to replicate the same functionality. If we want to make sure our dApp supports handling the edge-case when a user does a replace-by-fee, it means again throwing away the usage of simpler event emitter code (see code below #2) and writing much more complicated polling code that needs to continually look for new transactions sent from the user's address and to parse these transactions to see if they correspond to a smart contract call our dApp attempted to make. It's not only a lot of code everyone is going to end up having to write themselves, but it's also a much more difficult case to implement correctly and will lead to an entirely new class of bugs in and of itself.

code example 1

async function retrieveTransactionReceipt(web3, transactionHash) {
  let pollInterval = 5000;
  let elapsedTime = 0;

  while (true) {
    let receipt;
    try {
      receipt = await web3.eth.getTransactionReceipt(transactionHash);
    } catch (err) {
      console.log('error retrieving transaction receipt: ' + err)
    }

    if(receipt) {
      return receipt;
    } else if (elapsedTime % (pollInterval) === 0) {
      console.log("still waiting for transaction receipt after " + elapsedTime/1000 + " seconds");
    }

    await Util.sleep(pollInterval);
    elapsedTime += pollInterval;
  }
} 

code example 2

      // submit transaction
      console.log('submitting transaction request');
      let eventEmitter = this.resourceBroker.methods.burnerMachine(rawKey, encodedParams, this.requestParams.currency.address).send(params);

      // await transaction hash
      transactionHash = (await Util.eventToPromise(eventEmitter, 'transactionHash'))[0];
      await this.notify(RequestState.APPROVED, {transactionHash: transactionHash});
      console.log('user approved transaction, awaiting confirmation ', transactionHash);

@gorgos
Copy link

gorgos commented Jul 25, 2020

See also the related issues in MetaMask:

First issue is two years old. Considering this probably breaks a lot of Dapps, time to fix it now I suggest.

@GregTheGreek
Copy link
Contributor

GregTheGreek commented Jul 27, 2020

I've created EIP-2831 in attempt to address this (still making edits)

@miohtama
Copy link
Contributor Author

miohtama commented Aug 8, 2020

Excellent work @GregTheGreek - left some comments

@github-actions
Copy link

github-actions bot commented Oct 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

@github-actions github-actions bot added the Stale Has not received enough activity label Oct 8, 2020
@GregTheGreek
Copy link
Contributor

Closing in favour #3723

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Discussion Feature Request Stale Has not received enough activity
Projects
None yet
Development

No branches or pull requests

5 participants