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.

Sign up for GitHub

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

Jump to bottom

fix: continue processing events in new block after unknown events #1198

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: continue processing events in new block after unknown events #1198

wants to merge 1 commit into from

Conversation

matteojug
Copy link
Collaborator

Description

Closes: #1182

Changes

Instead of returning after the first invalid event (skipping the rest of the events), we continue.

Testing Information

Add a test for this.

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

cylewitruk
cylewitruk approved these changes Jan 10, 2025
View reviewed changes
Copy link
Member

@cylewitruk cylewitruk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm -- you could probably simplify test_invalid_event to simply call new_block_handler() with the json body and skip the api stuff, but up to you

signer/src/api/new_block.rs
@@ -452,6 +453,9 @@ mod tests {

const ROTATE_KEYS_WEBHOOK: &str = include_str!("../../tests/fixtures/rotate-keys-event.json");

const ROTATE_KEYS_AND_INVALID_WEBHOOK: &str =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AND_INVALID_EVENT maybe?

djordon
djordon approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@djordon djordon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks good. ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cylewitruk cylewitruk cylewitruk approved these changes

@djordon djordon djordon approved these changes

@Jiloc Jiloc Awaiting requested review from Jiloc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Avoid skipping the rest of the block if we err when processing an event in new block handler
3 participants
@matteojug @djordon @cylewitruk