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-2718: Typed Transaction Envelope + EIP-2930 #21502

Merged
merged 83 commits into from
Feb 25, 2021

Conversation

lightclient
Copy link
Member

@lightclient lightclient commented Aug 29, 2020

Hi all, I'm working on implementing EIP-2718: Typed Transaction Envelope. As of ACD 92 it is considered EFI. It is a dependency for the following:

Given the amount of code that this PR will eventually update, it would help me a lot to get some feedback earlier on to make sure this is headed in a good direction. Please let me know if there is a better way to request feedback.

@lightclient lightclient marked this pull request as draft August 30, 2020 00:22
@lightclient lightclient changed the title Implement EIP-2718 EIP-2718: Typed Transaction Envelope Sep 3, 2020
@lightclient
Copy link
Member Author

lightclient commented Sep 4, 2020

I'm planning to update this to use the original Transaction struct + interface as txdata (had trouble encoding the interface version).

@holiman
Copy link
Contributor

holiman commented Sep 7, 2020

Thanks for starting to work on this. It's a bit hard to judge if the interfaces are 'right' without more concrete examples, so I was wondering if you'd also be interested in implementing the 2930 transaction type within the same PR?
Note, you wouldn't need to implement the actual handling of access lists, but use the concrete new transaction type to see if the abstraction model you have chosen is sufficient to accommodate it?

I was thinking of picking it up myself, but it seems like a bad idea for me to implement something that you've already gotten a good start on... ?

@holiman
Copy link
Contributor

holiman commented Sep 7, 2020

Regarding your concrete questions, I don't really have any good answers now, I think it will be easier to give feedback with more concrete types implemented.

@lightclient
Copy link
Member Author

lightclient commented Sep 7, 2020

I was wondering if you'd also be interested in implementing the 2930 transaction type within the same PR?

Yep, I would be happy to.

core/types/legacy_tx.go Outdated Show resolved Hide resolved
@lightclient lightclient force-pushed the impl-eip-2718 branch 2 times, most recently from 7666d19 to 1de464f Compare September 10, 2020 15:30
@lightclient
Copy link
Member Author

@holiman @fjl, do you prefer the tests for this PR to be written natively in geth, as part of retesteth, or both?

@holiman
Copy link
Contributor

holiman commented Sep 15, 2020

do you prefer the tests for this PR to be written natively in geth, as part of retesteth, or both?

We always want 'native' tests in the codebase. If there are other more high-level tests that's good too, but we don't want to rely on those only

core/state_processor.go Outdated Show resolved Hide resolved
@lightclient lightclient force-pushed the impl-eip-2718 branch 5 times, most recently from aaea762 to 3651b77 Compare September 27, 2020 01:55
@lightclient
Copy link
Member Author

Hi @holiman, I implemented EIP-2930 using the EIP-2718 envelope. This PR is rebased against your EIP-2929 PR. For a simpler diff, I have a PR on my own repo against your branch.

@holiman
Copy link
Contributor

holiman commented Oct 19, 2020

I rebased this on top of access_lists.
I think it'll be easier to review this once that one is merged

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

Successfully merging this pull request may close these issues.