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

fix: error handling with retries when waiting for receipt #9650

Merged
merged 3 commits into from
Jan 13, 2025

Conversation

grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented Jan 8, 2025

Motivation

closes #9636

Solution

  • use Retry utility. add Continue variant to preserve number of retries (without decrementing)
  • use Continue on watcher timeout err if tx is still known to the node (won't count as a retry). For all other errs use Retry
  • tested with 100 txes on base sepolia

@grandizzy grandizzy changed the title fix: err with retries fix: error handling with retries when waiting for receipt Jan 8, 2025
@grandizzy
Copy link
Collaborator Author

@klkvr do you think this makes sense in context of #9636 (comment) ?

@klkvr
Copy link
Member

klkvr commented Jan 9, 2025

I don't think this will address the issue as we're hitting error in different arm #9636 (comment)

we could consider retrying several fatal errors instead of immidiately returning here

// treat other errors as fatal
Err(e) => return Err(e.into()),

@grandizzy
Copy link
Collaborator Author

I don't think this will address the issue as we're hitting error in different arm #9636 (comment)

we could consider retrying several fatal errors instead of immidiately returning here

// treat other errors as fatal
Err(e) => return Err(e.into()),

yeah, that's what I wanted with https://github.com/foundry-rs/foundry/pull/9650/files#diff-40b142bf1cc0e034c56533fad30c71161b2cc0b4dd4d6a72468e0649e3c7002dR61-R66 what do I miss? thank you!

@klkvr
Copy link
Member

klkvr commented Jan 9, 2025

ah missed that second branch has changed too

lgtm but I'd keep provider.get_transaction_by_hash below so it runs for both cases

@grandizzy grandizzy marked this pull request as ready for review January 9, 2025 11:03
@grandizzy
Copy link
Collaborator Author

ah missed that second branch has changed too

lgtm but I'd keep provider.get_transaction_by_hash below so it runs for both cases

please recheck

@grandizzy grandizzy added T-bug Type: bug C-forge Command: forge labels Jan 9, 2025
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.

seems appropriate to retry this a few times

crates/script/src/receipts.rs Outdated Show resolved Hide resolved
crates/script/src/receipts.rs Outdated Show resolved Hide resolved
@grandizzy grandizzy requested a review from DaniPopes January 13, 2025 08:54
@grandizzy grandizzy merged commit b6c094c into foundry-rs:master Jan 13, 2025
22 checks passed
@grandizzy grandizzy deleted the issue-9636 branch January 13, 2025 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-bug Type: bug
Projects
Status: No status
4 participants