-
Notifications
You must be signed in to change notification settings - Fork 0
Alert whenever an important account does anything, and de-duplicate some alerts #49
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
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.
Comment @cursor review
or bugbot run
to trigger another review on this PR
} else { | ||
who.into_iter().next().map(Account::Signer) | ||
} | ||
} |
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.
Bug: Event Handling Fails on Multiple Initiators
The EventInfo::initiator_account
method uses unreachable!()
if an event contains multiple 'who' accounts. This assumes a strict event structure that external blockchain data may not always uphold, potentially causing the alerter to panic and crash. This approach also goes against the codebase's pattern of gracefully handling unexpected blockchain data.
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 is technically true, if the event is being deserialized from an array of tuples. But events are almost always structs, so duplicate names are unlikely to happen in practice:
https://docs.rs/scale-value/0.18.0/scale_value/enum.Composite.html
But just in case I'll fix this over the next few days.
Not a blocker for merging.
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.
LGTM
695ca30
to
d5825f5
Compare
Updated with the "multiple |
This PR adds alerts whenever an important account signs an extrinsic or initiates an event.
It also de-duplicates alerts, using this priority order:
Other alert kinds don't need de-duplication.
This de-duplication covers most of ticket #18, but we still need to combine alerts between an extrinsic and its events. (Currently, extrinsics and events don't participate in alert de-duplication, because the logic is more complicated than a priority order.)
How to review
Commit ca21716 is pure code movement, and should be reviewed separately.