-
Notifications
You must be signed in to change notification settings - Fork 2k
fix: handle empty receipt when transactions are discarded by RPCs #10457
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: master
Are you sure you want to change the base?
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.
ty,
conceptionally this makes sense, but I wonder if we can sleep + retry a few times here instead?
crates/script/src/receipts.rs
Outdated
// Check if the receipt has valid block information | ||
if receipt.block_number.is_none() || receipt.block_hash.is_none() || receipt.transaction_index.is_none() { |
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.
receipt usually should only return mined transactions, but I can see that some networks might behave that way.
since these are mostly faster L2s, I wonder if the better approach here would be, just retry a few times 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.
a few more style nits
crates/script/src/progress.rs
Outdated
error_msg += "\n\n Add `--resume` to your command to try and continue broadcasting "; | ||
error_msg += "the transactions. This will attempt to resend transactions that were discarded by the RPC."; |
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 prefer a single raw string here r#""#
crates/script/src/receipts.rs
Outdated
} else { | ||
// Receipt has valid block information, proceed normally | ||
Ok(receipt.into()) |
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 to lift this up, like
let is_pending = receipt.block_number.is_none() || receipt.block_hash.is_none() || receipt.transaction_index.is_none();
if !is_pnding {return Ok(receipt.into())}
crates/script/src/progress.rs
Outdated
|
||
seq_progress.inner.write().finish_tx_spinner(tx_hash); | ||
// Check if this is a retry error for empty receipts | ||
if err.to_string().contains("Received an empty receipt") { |
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 isn't quite empty, but pending, can we update the wording here?
Motivation
Fixes issue #10453 where some RPCs discard transactions but return empty receipts (with missing block information), causing the --resume flag to be ineffective when trying to retry these transactions.
Solution
PR Checklist