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

feature: embed primitives Log in rpc Log and consensus Receipt in rpc Receipt #396

Merged
merged 11 commits into from
Mar 29, 2024

Conversation

prestwich
Copy link
Member

@prestwich prestwich commented Mar 26, 2024

Supersedes #395, closes #399

Motivation

Prevent accidental divergence of consensus and rpc types by embedding the consensus type in the RPC type

Solution

  • Embed alloy_primitives::Log into alloy_rpc_types::Log
  • Embed alloy_consensus::ReceiptEnvelope into alloy_rpc_types::Receipt

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@prestwich prestwich added the debt Tech debt which needs to be addressed label Mar 26, 2024
@prestwich prestwich self-assigned this Mar 26, 2024
@prestwich prestwich force-pushed the prestwich/embed-log branch 2 times, most recently from 8d1fd19 to 16a39bf Compare March 26, 2024 23:04
@prestwich prestwich linked an issue Mar 26, 2024 that may be closed by this pull request
@prestwich prestwich changed the title feature: embed primitives Log in rpc Log feature: embed primitives Log in rpc Log and consensus Receipt in rpc Receipt Mar 27, 2024
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.

could we add some sanity serde checks to ensure this works for eth_getTransactionReceipt

random tx cast rpc eth_getTransactionReceipt "0x21f6554c28453a01e7276c1db2fc1695bb512b170818bfa98fa8136433100616"

{"transactionHash":"0x21f6554c28453a01e7276c1db2fc1695bb512b170818bfa98fa8136433100616","blockHash":"0x4acbdefb861ef4adedb135ca52865f6743451bfbfa35db78076f881a40401a5e","blockNumber":"0x129f4b9","logsBloom":"0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000200000000000000000040000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000400000800000000000000000000000000000000004000000000000000000800000000100000020000000000000000000080000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000010000000000000000000000000000","gasUsed":"0xbde1","contractAddress":null,"cumulativeGasUsed":"0xa42aec","transactionIndex":"0x7f","from":"0x9a53bfba35269414f3b2d20b52ca01b15932c7b2","to":"0xdac17f958d2ee523a2206206994597c13d831ec7","type":"0x2","effectiveGasPrice":"0xfb0f6e8c9","logs":[{"blockHash":"0x4acbdefb861ef4adedb135ca52865f6743451bfbfa35db78076f881a40401a5e","address":"0xdac17f958d2ee523a2206206994597c13d831ec7","logIndex":"0x118","data":"0x00000000000000000000000000000000000000000052b7d2dcc80cd2e4000000","removed":false,"topics":["0x8c5be1e5ebec7d5bd14f71427d1e84f3dd0314c0f7b2291e5b200ac8c7c3b925","0x0000000000000000000000009a53bfba35269414f3b2d20b52ca01b15932c7b2","0x00000000000000000000000039e5dbb9d2fead31234d7c647d6ce77d85826f76"],"blockNumber":"0x129f4b9","transactionIndex":"0x7f","transactionHash":"0x21f6554c28453a01e7276c1db2fc1695bb512b170818bfa98fa8136433100616"}],"status":"0x1"}

Comment on lines 31 to 32
/// Reserialize the data.
pub fn reserialize(&self) -> Log<alloy_primitives::LogData> {
Copy link
Member

Choose a reason for hiding this comment

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

I see, this can be used to convert anything other than LogData in the default Log

could add this as context to "deserialize the data into the default Log type"

if we have &self, we can also add an Into<Log> either by impl Into trait (perhaps not possible) or into_reserialize

crates/rpc-types/src/eth/log.rs Show resolved Hide resolved
crates/rpc-types/src/eth/transaction/receipt.rs Outdated Show resolved Hide resolved
crates/consensus/src/receipt/receipts.rs Outdated Show resolved Hide resolved
crates/rpc-types/src/eth/transaction/receipt.rs Outdated Show resolved Hide resolved
@prestwich
Copy link
Member Author

could we add some sanity serde checks to ensure this works for eth_getTransactionReceipt

yeah, there are tests further up the stack (in contracts I think). but i'll add at least one here

@prestwich prestwich marked this pull request as draft March 27, 2024 17:03
@prestwich
Copy link
Member Author

converting to draft as I need to resolve a type structure issue that results in the deserialized logs not including the RPC log metadata. need to put a generic in ReceiptEnvelope and push it down to ReceiptWithBloom and Receipt

@prestwich prestwich marked this pull request as ready for review March 27, 2024 17:20
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.

this should make things a bit easier,

one thing to note is that this changes the order of the json keys.
there's no spec for this and it doesn't matter really.

I'd like one more check to ensure we're also serializing properly

@prestwich
Copy link
Member Author

rebasing now and adding one last generic

@prestwich prestwich merged commit eaa357c into main Mar 29, 2024
17 checks passed
@prestwich prestwich deleted the prestwich/embed-log branch March 29, 2024 23:56
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.

[Feature] Embed consensus types in rpc types wherever possible
2 participants