Add EventEmitter to XCM executor. #7234
Conversation
| @@ -427,9 +428,20 @@ impl<Config: config::Config> XcmExecutor<Config> { | |||
| reason = ?reason, | |||
| "Sending msg", | |||
| ); | |||
| let (ticket, fee) = validate_send::<Config::XcmSender>(dest, msg)?; | |||
| let (ticket, fee) = validate_send::<Config::XcmSender>(dest.clone(), msg.clone())?; | |||
There was a problem hiding this comment.
Just thinking out loud, do we really want to log/use msg.clone here?
My concern is that some Config::XcmSender / SendXcm implementations might modify the msg, such as WithUniqueTopic. This could lead to confusion, as the msg in the event might differ from the one actually sent (or received on the other side).
There was a problem hiding this comment.
definitely don't include full message in event, see my comment in the trait definition
|
|
||
| use xcm::latest::prelude::*; | ||
|
|
||
| pub trait EventEmitter { |
There was a problem hiding this comment.
please add some documentation here at least
| use xcm::latest::prelude::*; | ||
|
|
||
| pub trait EventEmitter { | ||
| fn emit_sent_event( |
There was a problem hiding this comment.
The emit_sent_event function, with Self::deposit_event(Event::Sent, makes it a single-purpose solution.
I would prefer a more general approach. For example, there’s a very relevant issue where we want/need to emit a failure event from ProcessXcmMessage.
If we could address this issue using an EventEmitter, that would be fantastic.
There was a problem hiding this comment.
one option is this trait to have multiple functions, each specific to xcm-executor events
fn emit_sent_event is one
fn emit_process_event is another
etc
| use xcm::latest::prelude::*; | ||
|
|
||
| pub trait EventEmitter { | ||
| fn emit_sent_event( |
There was a problem hiding this comment.
one option is this trait to have multiple functions, each specific to xcm-executor events
fn emit_sent_event is one
fn emit_process_event is another
etc
| fn emit_sent_event( | ||
| origin: Location, | ||
| destination: Location, | ||
| message: Xcm<()>, |
There was a problem hiding this comment.
| message: Xcm<()>, |
definitely cannot include full message in Events, on chain storage should be kept to a minimum
providing the message_id: XcmHash should be enough
| @@ -46,6 +46,7 @@ mod assets; | |||
| pub use assets::AssetsInHolding; | |||
| mod config; | |||
| pub use config::Config; | |||
| use crate::traits::EventEmitter; | |||
There was a problem hiding this comment.
move this to the use traits::{... above
| @@ -427,9 +428,20 @@ impl<Config: config::Config> XcmExecutor<Config> { | |||
| reason = ?reason, | |||
| "Sending msg", | |||
| ); | |||
| let (ticket, fee) = validate_send::<Config::XcmSender>(dest, msg)?; | |||
| let (ticket, fee) = validate_send::<Config::XcmSender>(dest.clone(), msg.clone())?; | |||
There was a problem hiding this comment.
definitely don't include full message in event, see my comment in the trait definition
| Config::XcmSender::deliver(ticket).map_err(Into::into) | ||
| let message_id = match Config::XcmSender::deliver(ticket) { | ||
| Ok(message_id) => message_id, | ||
| Err(e) => return Err(e.into()), |
There was a problem hiding this comment.
should emit event for error too
| message: Xcm<()>, | ||
| message_id: XcmHash, | ||
| ) { | ||
| Self::deposit_event(Event::Sent { origin, destination, message, message_id }); |
There was a problem hiding this comment.
would not reuse Event::Sent which is used by pallet_xcm::send()
| Self::deposit_event(Event::Sent { origin, destination, message, message_id }); | |
| Self::deposit_event(Event::XcmExecutorSent { origin, destination, message_id }); |
There was a problem hiding this comment.
Why would you not reuse it? Are they really that different?
If they're just one, apps can just subscribe to one event and know all cross-chain messages from that.
There was a problem hiding this comment.
true, on second thought we could reuse, I am still not sure about including the full message in the event though, some messages could be quite big
|
@jpserrat do you plan on finishing this PR or should we take over? |
|
@acatangiu Sorry for the delay on this. I'm going to finish. |
|
If #7200 is merged before your PR, you may need add the new |
Closes #6851
@franciscoaguirre I added the
EventEmittertrait to the config. I have two questions on this:EventEmittertrait I've created a sent event function instead a generic one that emits any event, do you prefer a generic one?