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

feat: added Optimism deposited transaction support #2390

Merged
merged 22 commits into from
May 26, 2023

Conversation

merklefruit
Copy link
Contributor

@merklefruit merklefruit commented Apr 30, 2023

Motivation

Optimism has an extra transaction type: the Deposited transaction (type 0x7E), with 3 new transaction fields as described in the spec here. The RLP encoding order is also different.

Solution

This PR introduces a new feature "optimism" much like the existing "celo" one, which enables the 3 new optional transaction fields.

PR Checklist

  • Added Tests
  • Added Documentation

@merklefruit merklefruit changed the title feat: added Optimism Support feat: added Optimism deposited transaction support Apr 30, 2023
@merklefruit merklefruit marked this pull request as ready for review April 30, 2023 21:25
@prestwich
Copy link
Collaborator

I'm supportive of adding first-class optimism objects. However, I am concerned that this change might be too large for our current release cycle. We're trying to put this version into maintenance so that we can focus on ethers@3.

In addition, our opinion generally is that the Celo feature was a bad approach to network-specific behavior in the first place, and this ought to be solved by abstraction of the argument and return types in the Middleware trait

@gakonst do you think it is worth it to unblock optimism folks?

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

I would like to add this because it also unblocks op-reth work for client side. I am open to merging this if CI passes

@prestwich
Copy link
Collaborator

Tests need to be fixed

Copy link
Collaborator

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

Need to update CI to run with the optimsim feature enabled.
Need to document that Optimism and Celo are mutually exclusive

ethers-core/src/types/transaction/response.rs Outdated Show resolved Hide resolved
ethers-core/src/types/transaction/response.rs Outdated Show resolved Hide resolved
ethers-core/src/types/transaction/response.rs Outdated Show resolved Hide resolved
ethers-core/src/types/transaction/response.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@merklefruit
Copy link
Contributor Author

Ok, I've split the addition of a new TypedTransaction variant to a secondary PR to make the diffs cleaner.
Here: merklefruit#1

I've also added a couple tests for RLP encoding/decoding of this new tx type. Let me know what other kinds of tests I should eventually add!

@gakonst gakonst merged commit a8d5f5f into gakonst:master May 26, 2023
13 of 15 checks passed
@merklefruit
Copy link
Contributor Author

@gakonst I believe the PR above (merklefruit#1) was not merged into this branch yet. I've closed it now, so I need to reopen a PR from this branch

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

5 participants