-
Notifications
You must be signed in to change notification settings - Fork 15
Post events at the execution level and horde them in the inspector #79
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
core/src/engine/inspector.rs
Outdated
| ft_withdraw: &FtWithdraw, | ||
| intent_hash: CryptoHash, | ||
| ) -> Result<()>; | ||
| fn emit_event(&mut self, event: DefuseEvent<'_>); |
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.
It's up to implementation whether to emit event, store it somewhere internally or handle it in some another way:
| fn emit_event(&mut self, event: DefuseEvent<'_>); | |
| fn on_event(&mut self, event: DefuseEvent<'_>); |
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 way it will only be possible to emit dip4 events, but not nep245 and others...
Can you make it accept all possible events that we might have in some structured way? E.g. by accepting Event enum as parameter:
#[serde(untagged)]
pub enum Event<'a> {
Dip4(DefuseEvent<'a>),
Nep245(MtEvent<'a>),
// ...
}
impl Event<'_> {
pub fn emit(&self) {
match self {
Self::Dip4(event) => event.emit(),
Self::Nep245(event) => event.emit(),
}
}
}The complex part will be to re-write Contract::deposit() and Contract::withdraw methods, so that they become Inspector-aware, or, in general, intent-aware... Or maybe you have other suggestions?
| pub ft_withdrawals: Option<Vec<FtWithdraw>>, | ||
| pub nft_withdrawals: Option<Vec<NftWithdraw>>, | ||
| pub mt_withdrawals: Option<Vec<MtWithdraw>>, | ||
| pub events_emitted: Vec<String>, |
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.
Make it more convenient for callers to avoid parsing strings as JSON:
| pub events_emitted: Vec<String>, | |
| pub events: Vec<serde_json::Value>, |
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.
Or it would be even better if we don't bind to JSON specifically, but use strongly-typed Event struct/enum...
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.
Push event objects presents a challenge because the values are mostly references. In fact, I changed some Cow::Borrow to Owned to make this work. If we get rid of references, then this is easily doable.
| pub nft_withdrawals: Option<Vec<NftWithdraw>>, | ||
| pub mt_withdrawals: Option<Vec<MtWithdraw>>, | ||
| pub events_emitted: Vec<String>, | ||
| pub events_emitted: Vec<serde_json::Value>, |
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.
| pub events_emitted: Vec<serde_json::Value>, | |
| pub events: Vec<serde_json::Value>, |
|
Closed in favor of #91 |
No description provided.