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

Add trait methods cumulative_gas_used and state_root to ReceiptResponse #1275

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

emhane
Copy link
Contributor

@emhane emhane commented Sep 11, 2024

Motivation

Use generic ReceiptResponse type in place of alloy_rpc_types_eth::TransactionReceipt in reth RPC

Solution

Adds trait methods ReceiptResponse::cumulative_gas_used and ReceiptResponse::state_root

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Comment on lines 58 to 61
/// The post-transaction state root (pre Byzantium)
///
/// EIP98 makes this field optional.
fn state_root(&self) -> Option<&B256>;
Copy link
Member

Choose a reason for hiding this comment

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

we can ingore this because no longer useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when converting from ReceiptResponse to TransactionReceipt<T>, would it not be a bug to assume the field to be None for historic blocks?

Copy link
Member

Choose a reason for hiding this comment

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

when converting from ReceiptResponse to TransactionReceipt
this isn't the purpose of this trait, this trait just serves the purpose of providing expected values

but I guess it doesn't really hurt to add this function, even if this is useless.

Comment on lines +55 to +56
/// Returns the cumulative gas used at this receipt.
fn cumulative_gas_used(&self) -> u128;
Copy link
Member

Choose a reason for hiding this comment

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

this makes sense

@emhane emhane requested a review from mattsse September 11, 2024 12:42
Copy link
Member

@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.

nit

/// The post-transaction state root (pre Byzantium)
///
/// EIP98 makes this field optional.
fn state_root(&self) -> Option<&B256>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn state_root(&self) -> Option<&B256>;
fn state_root(&self) -> Option<B256>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not always a need to clone all the 32 bytes, the caller can decide

Copy link
Member

Choose a reason for hiding this comment

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

this is at odds with the other trait functions. I'd like to unify this and return a copy

@emhane emhane requested a review from mattsse September 11, 2024 14:22
@mattsse mattsse merged commit 1d9e194 into main Sep 12, 2024
26 checks passed
@mattsse mattsse deleted the emhane/receipt-resp branch September 12, 2024 05:30
lwedge99 pushed a commit to sentioxyz/alloy that referenced this pull request Oct 8, 2024
…esponse` (alloy-rs#1275)

* Add trait methods cumulative_gas_used and state_root to ReceiptResponse

* touchup

---------

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Tech debt which needs to be addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants