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

Re-enable the transaction.other field for optimism feature #2622

Merged
merged 2 commits into from
Nov 16, 2023

Conversation

mdehoog
Copy link
Contributor

@mdehoog mdehoog commented Oct 3, 2023

Motivation

I'm in the process of adding op-stack deposit tx support to anvil. This requires enabling the optimism feature.

Anvil uses the other field to pass back additional information: https://github.com/foundry-rs/foundry/blob/49007938138ae26379e7a19bf3b2ec2ba6822017/crates/anvil/src/eth/api.rs#L1801-L1812

Solution

Remove the optimism feature opt-out from the other field. This also makes it consistent with the Block struct:

/// Captures unknown fields such as additional fields used by L2s
#[cfg(not(feature = "celo"))]
#[serde(flatten)]
pub other: crate::types::OtherFields,

and the TransactionReceipt struct:
/// Captures unknown fields such as additional fields used by L2s
#[cfg(not(feature = "celo"))]
#[serde(flatten)]
pub other: crate::types::OtherFields,

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@mdehoog mdehoog marked this pull request as draft October 3, 2023 18:02
@mdehoog mdehoog force-pushed the michael/optimism-other branch 2 times, most recently from 6493841 to cc01eb1 Compare October 3, 2023 18:08
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

why is opting out required here required for optimism?

@mdehoog
Copy link
Contributor Author

mdehoog commented Oct 3, 2023

My understanding is that Anvil uses the other field to pass back additional information: https://github.com/foundry-rs/foundry/blob/49007938138ae26379e7a19bf3b2ec2ba6822017/crates/anvil/src/eth/api.rs#L1801-L1812

If we enable the optimism feature for the Anvil build, we lose this field.

@mdehoog mdehoog marked this pull request as ready for review October 3, 2023 19:41
@mdehoog
Copy link
Contributor Author

mdehoog commented Oct 4, 2023

for example, in my anvil branch that adds op deposit tx support, i've had to disable usage of the other field: foundry-rs/foundry@master...mdehoog:foundry:op-deposit#diff-a76a82dc700c0415fcd07ec10cde2ad56234e41af0ea3c07f8d1a8383b8b0913L1803-R1812

Comment on lines +135 to 137
#[cfg(not(any(feature = "celo")))]
#[serde(flatten)]
pub other: crate::types::OtherFields,
Copy link
Collaborator

Choose a reason for hiding this comment

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

okay, I think this is reasonable to always enable this because this is also useful for other chains that include additional fields

@mattsse
Copy link
Collaborator

mattsse commented Oct 17, 2023

I'm holding off on merging until we have a foundry companion PR

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

as discussed, this is reasonable to capture anything else, regardless of the optimism feature,
does not impact foundry but makes it possible to capture OP fields

@mattsse mattsse merged commit b4c366c into gakonst:master Nov 16, 2023
17 of 18 checks passed
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.

2 participants