-
Notifications
You must be signed in to change notification settings - Fork 332
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
Handle duplicate packet events and perform full clearing even in the presence of errors #3361
Conversation
…d only retain the event from the highest one with fully matches the criteria
…d timeout packets failed
if let Some(event) = packet_from_tx_search_response(chain_id, request, *seq, tx)? { | ||
// We found the event | ||
result.push(event); | ||
break 'inner; |
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.
Instead of breaking, could we get them all, if more than 1 extracted print a warning with some tx hashes or height info? And then pick the first? In 99.99% of cases there will be only one so there is no perf impact. But it would give very valuable feedback to the user
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.
Looks good. I tried it on the testnet and output looks as good as it can get. I don't have wallets so the actual transactions are not sent so not sure about the fix for |
let (first_event, _, _) = tx_events.remove(0); | ||
result.push(first_event); |
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.
can we pull these out of the if
as we do it for both branches
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.
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.
Looks great! Packets are cleared on the testnet! Thanks @romac !!
Thank you for diagnosing the issue and coming up with a way to fix it 🙏 |
Closes: #3359
Closes: #3358
PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.