-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
dev: use U64 for transaction_index
#4261
dev: use U64 for transaction_index
#4261
Conversation
1d41342
to
43b5664
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
index should always be set in receipt and I believe it is always set by reth.
apparently, some clients include to: null, some skip it, I'd like to keep null
/// Address of the receiver. None when it's a contract creation transaction. | ||
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub to: Option<Address>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be null for creations, but it could be that some clients differ
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub transaction_index: Option<U256>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should actually not be an Option at all.
so we should change this to U64 probably
Gotcha, thanks for the review! I've updated the pr to use u64 and remove the skip |
None
transaction_index
@@ -9,7 +9,7 @@ pub struct TransactionReceipt { | |||
/// Transaction Hash. | |||
pub transaction_hash: Option<H256>, | |||
/// Index within the block. | |||
pub transaction_index: Option<U256>, | |||
pub transaction_index: u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be U64 because should be hex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha makes sense, thanks! Just changed it to U64 🙂
2697737
to
011dd8d
Compare
transaction_index
transaction_index
Codecov Report
... and 12 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Hello, this smol pr does the following
add skipping of serialization fortransaction_index
andto
transaction_index
TransactionReceipt
fieldtransaction_index
is set toNone
#4260