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

fix(rpc): Add l1 block info to OpTransactionReceipt #62

Merged
merged 5 commits into from
Aug 30, 2024

Conversation

emhane
Copy link
Collaborator

@emhane emhane commented Aug 30, 2024

Motivation

Closes #54

Solution

  • Moves L1 block info fields out of type OptimismTransactionReceiptFields into new type L1BlockInfo.
  • Adds L1BlockInfo to OpTransactionReceipt

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@emhane emhane requested a review from mattsse as a code owner August 30, 2024 15:13
@emhane emhane added the A-rpc-types Area: op alloy rpc types label Aug 30, 2024
Copy link
Collaborator

@refcell refcell left a comment

Choose a reason for hiding this comment

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

This makes sense - would like to see a PR down the road that adds doc comments about why these are separate types and how they're used. If you're looking at op-geth, these fields are just part of the receipt and not also a separate OptimismTransactionReceiptFields.

@emhane
Copy link
Collaborator Author

emhane commented Aug 30, 2024

This makes sense - would like to see a PR down the road that adds doc comments about why these are separate types and how they're used. If you're looking at op-geth, these fields are just part of the receipt and not also a separate OptimismTransactionReceiptFields.

the deposit envelope has the deposit nonce and receipt version fields. so adding the OptimismTransactionReceiptFeilds type to OpTransactionReceipt directly would have caused duplicates for these keys.

I think down the road, this will be replaced with some new envelope types, as #53, to help set the additional op fields. right now it's really quite hard to debug anything related to this since I have to memorise which fields have to be set for which fork, from reading docs on the OptimismTransactionReceiptFeilds (now L1BlockInfo) type.

the type OptimismTransactionReceiptFields will probably be depreciated when we can stop using the AnyTransactionReceipt type in reth paradigmxyz/reth#10632, which is possible after next op-alloy release to include this pr. when the linked issue is solved, we can just call a builder for OpTransactionReceipt directly, no need for intermediary type OptimismTransactionReceiptFields to convert to OtherFields.

@emhane emhane merged commit 9bb08e9 into main Aug 30, 2024
18 checks passed
@emhane emhane deleted the emhane/fix-op-network branch August 30, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc-types Area: op alloy rpc types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] OP fields missing from op_alloy_rpc_types::OpTransactionReceipt
2 participants