-
Notifications
You must be signed in to change notification settings - Fork 245
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
network: Add status
method to ReceiptResponse
trait
#846
Conversation
directionally approve. can you copy the doc warnings from #848 ?
|
cc @mattsse should |
ah, right this way we could enforce embedding the receipt type in rpc, so this makes sense, then we could also add |
this is more work than belongs in this PR. captured in #854 |
@prestwich Added the doc warnings ✅ |
crates/network/src/ethereum/mod.rs
Outdated
/// ## Note | ||
/// | ||
/// Caution must be taken when using this method for deep-historical | ||
/// receipts, as it may not accurately reflect the status of the | ||
/// transaction. The transaction status is not knowable from the receipt | ||
/// for transactions before [EIP-658]. | ||
/// | ||
/// This can be handled using [`TxReceipt::status_or_post_state`]. | ||
/// | ||
/// [EIP-658]: https://eips.ethereum.org/EIPS/eip-658 | ||
/// [`TxReceipt::status_or_post_state`]: alloy_consensus::TxReceipt::status_or_post_state |
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.
duplicating the docs is not necessary imo
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'd like the docs to be on every place the user may encounter this (potentially confusing) behavior
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.
trait impls never need docstrings tho :)
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.
yes, they are inherited from the trait definition, pls remove
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 got it, removed the docs from the trait impls: b42dce5
* network: Add status func to ReceiptResponse trait * Add doc warnings * Fix doc test * Fix doc test * Remove unnecessary docs
Motivation
from: #845
It would be helpful to know the tx status from the receipt gotten by
get_receipt
method inPendingTransactionBuilder
or byget_transaction_receipt
inProvider
.Solution
Define
status
method inReceiptResponse
trait and implement it inTransactionReceipt
for each networkPR Checklist