-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: add subscription stream function for transaction receipts #19069
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
base: main
Are you sure you want to change the base?
feat: add subscription stream function for transaction receipts #19069
Conversation
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.
cool, a few suggestions
@mattsse re point 4: should I wire this into the match statement now even though SubscriptionKind::TransactionReceipts doesn't exist yet in alloy? Or is the current approach with separate impl block fine until alloy 2974 lands? |
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.
few more nits and q for @klkvr
) -> Vec<ConvertReceiptInput<'a, N>> | ||
where | ||
N::SignedTx: TxHashRef, | ||
{ |
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.
@klkvr thoughts on this
whould we perhaps make this a native fn of RecoveredBlock ?
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.
yep sgtm, thoughConvertReceiptInput::from_block_and_receipts
might be eaiser here because ConvertReceiptInput
is RPC-only type
also I would prefer this to be re-used in
reth/crates/rpc/rpc-eth-api/src/helpers/block.rs
Lines 126 to 157 in 447394f
let inputs = block | |
.transactions_recovered() | |
.zip(Arc::unwrap_or_clone(receipts)) | |
.enumerate() | |
.map(|(idx, (tx, receipt))| { | |
let meta = TransactionMeta { | |
tx_hash: *tx.tx_hash(), | |
index: idx as u64, | |
block_hash, | |
block_number, | |
base_fee, | |
excess_blob_gas, | |
timestamp, | |
}; | |
let cumulative_gas_used = receipt.cumulative_gas_used(); | |
let logs_len = receipt.logs().len(); | |
let input = ConvertReceiptInput { | |
tx, | |
gas_used: cumulative_gas_used - gas_used, | |
next_log_index, | |
meta, | |
receipt, | |
}; | |
gas_used = cumulative_gas_used; | |
next_log_index += logs_len; | |
input | |
}) | |
.collect::<Vec<_>>(); |
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.
@mattsse separate PR for the refactor or include here?
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.
@futreall let's do a separate one for this
Implements transaction_receipts_stream method in EthPubSub similar to new_headers_stream.
Adds build_convert_receipt_inputs helper function in rpc-convert for reusability.
Prep work for alloy-rs/alloy#2974.
Closes #19055