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

EIP-2831 Transaction Replacement Message Type #2831

Merged
merged 7 commits into from
Jul 29, 2020

Conversation

GregTheGreek
Copy link
Contributor

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

@GregTheGreek GregTheGreek changed the title Greg/tx replacement EIP-2831 Transaction Replacement Message Type Jul 26, 2020
EIPS/eip-2831.md Outdated Show resolved Hide resolved
EIPS/eip-2831.md Outdated Show resolved Hide resolved
EIPS/eip-2831.md Outdated Show resolved Hide resolved
EIPS/eip-2831.md Outdated Show resolved Hide resolved
EIPS/eip-2831.md Outdated Show resolved Hide resolved
EIPS/eip-2831.md Outdated Show resolved Hide resolved
EIPS/eip-2831.md Outdated Show resolved Hide resolved
EIPS/eip-2831.md Outdated Show resolved Hide resolved
Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

Left a bunch of miscellaneous feedback for the author, but there are a few that need to be addressed before merging as draft:

  1. discussion-to link should follow normal linking format.
  2. ## Motivation section is required
  3. ## Security Considerations section is required.

They can just contain placeholder text for now, but they do need to be present.

GregTheGreek and others added 3 commits July 27, 2020 11:41
Co-authored-by: Micah Zoltu <micah@zoltu.net>
Co-authored-by: Micah Zoltu <micah@zoltu.net>
Co-authored-by: Micah Zoltu <micah@zoltu.net>
EIPS/eip-2831.md Outdated Show resolved Hide resolved
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
@JakobP
Copy link

JakobP commented Jul 28, 2020

I have spent a fair amount of time think about replacement transactions from an user experience prespective as well as how they might be dealt with from a technical perspective. I were unable to come up with a satisfactory technical solution.

I can't speak on the exact implementation suggested in the EIP, but from a frontend development perspective this functionality is very much something I would like to see.

@moodysalem
Copy link
Contributor

With meta transactions where multiple relayers are able to submit a transaction (and potentially decentralized infrastructure where any account or contract can submit the transaction), is this change the most generally useful?

I understand this is useful for EOAs but I would prefer prioritizing efforts towards separating the identifier of a transaction from its hash, and making replacement less likely (e.g. deadlines as in #1681 and escalator gas prices as in #2593)

That said I do think this would improve the UX of the Uniswap Interface for many users if implemented by all providers.

@lcdupree
Copy link

I arrived at this EIP via web3/web3.js#3585. In my comment there, I describe how this has been a known issue for us since launching Burner Machine last October, and I describe some of the gymnastics where polling code was written in places where the promise semantics offered by the library are basically broken.

I appreciate this EIP, but agree with @moodysalem, in terms of looking at a more fundamental solution. Coming from the big-tech/enterprise software space, there are a lot of flaws found at design-time or during development that will stop a product from being shipped (show-stoppers), even if they are edge-cases where no one can articulate when, or if, the flaw will actually become a problem in production. Replace-by-fee presents edge-cases one could have reasoned about all the way back to the launch of mainnet. MetaMask shipped its speed-up feature and no one stopped to make sure that there'd be a solution to dApp breakage if it was used. It's only now that gas prices are 10-20x higher than they were a year ago due to DeFi, that people want to look at trying to fix this. And since the methodology in the blockchain space tends to be more about fixing something once it becomes a known problem, fixes tend to look more like bandaids, than design and architecture changes that would ameliorate the problem so it wouldn't have existed in the first place.

I don't know what to advocate here, but I do know that there are a lot of shipped dApps that are relying on convenience aync/promise semantics in web3.js/ethers.js today, and with this EIP, they will still be broken. I know how to write the polling code to work around this problem, but then that means throwing away the convenience features of these libraries anyway. And I also know that the more of this code that I write, the closer I get to manually parsing transactions myself, running them in a local implementation of the EVM, or even pulling them out of the mempool to inspect them directly. That all sounds pretty far gone from where we started; namely having some example javascript code on a page that shows anyone how to build their own Ethereum dApp using a web3 library by writing only a few lines of code...

@GregTheGreek
Copy link
Contributor Author

With meta transactions where multiple relayers are able to submit a transaction (and potentially decentralized infrastructure where any account or contract can submit the transaction), is this change the most generally useful?

I understand this is useful for EOAs but I would prefer prioritizing efforts towards separating the identifier of a transaction from its hash, and making replacement less likely (e.g. deadlines as in #1681 and escalator gas prices as in #2593)

That said I do think this would improve the UX of the Uniswap Interface for many users if implemented by all providers.

Right, so as you mentioned this is more useful EOA, and I would argue that #1681 is a great starting point, I'm going to look at creating a protocol level EIP that could compliment this one. Ultimately I believe this issue is still that we have a fair amount of systems using this, workflow - and this could provide immediate benefits.

Thank you for the feedback.

@GregTheGreek
Copy link
Contributor Author

@lcdupree

I don't know what to advocate here, but I do know that there are a lot of shipped dApps that are relying on convenience aync/promise semantics in web3.js/ethers.js today, and with this EIP, they will still be broken. I know how to write the polling code to work around this problem, but then that means throwing away the convenience features of these libraries anyway.

Do you mind expanding on this? What is broken? I dont see how this EIP would require a dapp to developer to do any polling, its a subscription from a provider.

Much like in the example I provided, all a dapp would need to do is subscribe to an additional message before requesting a tx. I'm also open to alternative solutions.

@moodysalem
Copy link
Contributor

moodysalem commented Jul 29, 2020

Right, so as you mentioned this is more useful EOA, and I would argue that #1681 is a great starting point, I'm going to look at creating a protocol level EIP that could compliment this one. Ultimately I believe this issue is still that we have a fair amount of systems using this, workflow - and this could provide immediate benefits.

The other point I would like to elaborate is that the ID of a transaction does not need to be the hash, and probably shouldn't be the hash given the hash can change for the same DAPP initiated action (e.g. due to speed up, cancel)

I would recommend exploring an option where the DAPP specifies the ID of the transaction sent to the ethereum provider, and additional methods for querying the state of the transaction by ID (e.g. latest hash, cancelled, confirmed, gas price) + the ability to request the provider cancels the transaction, for cases where it will always fail eg due to price limits or deadlines encoded in the transaction data

This also is compatible with smart contract wallets as with EOAs

@GregTheGreek
Copy link
Contributor Author

The other point I would like to elaborate is that the ID of a transaction does not need to be the hash, and probably shouldn't be the hash given the hash can change for the same DAPP initiated action (e.g. due to speed up, cancel)

Right so, i think this case is covered via the specification hence why there is an old/new tx. Ultimately a provider would need to implement a queue of some sorts to ensure that the transactions are being tracked.

I would recommend exploring an option where the DAPP specifies the ID of the transaction sent to the ethereum provider, and additional methods for querying the state of the transaction by ID (e.g. latest hash, cancelled, confirmed, gas price) + the ability to request the provider cancels the transaction, for cases where it will always fail eg due to price limits or deadlines encoded in the transaction data

This could be an option for sure. I have a few ideas how this could work.

@GregTheGreek
Copy link
Contributor Author

@MicahZoltu I believe i've correctly updated this eip

@MicahZoltu
Copy link
Contributor

@GregTheGreek It is still missing a ## Motivation and ## Security Considerations section: #2831 (review)

@GregTheGreek
Copy link
Contributor Author

GregTheGreek commented Jul 29, 2020

@GregTheGreek It is still missing a ## Motivation and ## Security Considerations section: #2831 (review)

Forgot to push ...

@MicahZoltu MicahZoltu merged commit ad25054 into ethereum:master Jul 29, 2020
@lcdupree
Copy link

I want to respond to @GregTheGreek, but is the conversation over here now that the PR received two approvals and was merged? Do I need to move (copy/paste) my comments over to the discussion thread on eth-magicians to have them considered?

@rstormsf, why did you approve without participating in the dialogue?

@GregTheGreek
Copy link
Contributor Author

I want to respond to @GregTheGreek, but is the conversation over here now that the PR received two approvals and was merged? Do I need to move (copy/paste) my comments over to the discussion thread on eth-magicians to have them considered?

@rstormsf, why did you approve without participating in the dialogue?

Please move the conversation to the Eth magicians thread. A merged EIP does not mean that it is being enforced, it simply means that the EIP is formatted correctly. This is still in a DRAFT state.

@miohtama
Copy link

miohtama commented Aug 8, 2020

I have built a dApp where the transaction tracking was a problem as well. While tracking transaction is a larger problem and probably will need a complete revisit in the future, I feel that this spec would address some shortcomings in dApp UX in the next 2-3 years.

Have a hook when a wallet uses to speed up or cancel transaction feature: The current spec look sufficient enough for this minor feature.

What can still break or is missing

  • The replacement transaction is a different smart contract call what dApp originally intended (possible, though not sure why anyone would do this, so maybe not even worthy of discussion)

  • Tracking transactions with wallet specific ids instead of transaction hashes: e.g. the user interface can associate the transaction in the notification menu easily as the same transaction regardless of how many times speed up feature is used with ever changing transaction hash

  • Requesting transaction speed up (gas top up) from the dApp user interface itself

  • Having a developer friendly events transactioncomplete (for some specified statistical criteria) and transactionfailed (completed, but was rejected by revert) - this would especially boost productivity of newcomer developers of Ethereum world

@MicahZoltu
Copy link
Contributor

@miohtama Please keep discussion in the discussion-to link: https://ethereum-magicians.org/t/eip-2831-transaction-replacement-message-type/4448

tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
* eip-2831

* Update EIPS/eip-2831.md

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Update EIPS/eip-2831.md

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Update EIPS/eip-2831.md

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Update EIPS/eip-2831.md

Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>

* update eip based on comments

* add abstract
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
* eip-2831

* Update EIPS/eip-2831.md

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Update EIPS/eip-2831.md

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Update EIPS/eip-2831.md

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Update EIPS/eip-2831.md

Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>

* update eip based on comments

* add abstract
@huahuayu
Copy link

@ Gregthegreek hey, how's everything going? Currently, what's the best way to detect replaced tx (cancel / speed up/ replaced)?

@htadashi
Copy link
Contributor

ethers.js has implemented EIP- 2831 Signer transaction.wait() style rejections since v5.2.0: ethers-io/ethers.js#1477

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants