Prevents EthereumBlobExporter from consuming dest when returning NotApplicable #5789
Conversation
| @@ -71,6 +71,8 @@ impl<UniversalLocation, EthereumNetwork, OutboundQueue, AgentHashedDescription, | |||
| let dest = destination.take().ok_or(SendError::MissingArgument)?; | |||
| if dest != Here { | |||
| log::trace!(target: "xcm::ethereum_blob_exporter", "skipped due to unmatched remote destination {dest:?}."); | |||
| // We need to make sure that destination is not consumed in case of `NotApplicable`. | |||
| *destination = Some(dest); | |||
There was a problem hiding this comment.
@claravanstaden
bellow there is a another usage:
if Ok(local_net) != universal_location.global_consensus() {
log::trace!(target: "xcm::ethereum_blob_exporter", "skipped due to unmatched relay network {local_net:?}.");
return Err(SendError::NotApplicable)
}
There was a problem hiding this comment.
I wondered about that one, because the comment only mentions dest and msg. But basically no mutable parameters should be altered when the error is NotApplicable?
There was a problem hiding this comment.
Fixed in bfc90ba. This case is easy to miss... Perhaps the parameters should not be mutable in the first place?
| @@ -71,9 +71,13 @@ impl<UniversalLocation, EthereumNetwork, OutboundQueue, AgentHashedDescription, | |||
| let dest = destination.take().ok_or(SendError::MissingArgument)?; | |||
There was a problem hiding this comment.
How about just replace take here with clone without other reset operations?
There was a problem hiding this comment.
Good point, that is much better. :) 4fe4085
| let (local_net, local_sub) = universal_source | ||
| .take() |
| if dest != Here { | ||
| log::trace!(target: "xcm::ethereum_blob_exporter", "skipped due to unmatched remote destination {dest:?}."); | ||
| return Err(SendError::NotApplicable) | ||
| } | ||
|
|
||
| let (local_net, local_sub) = universal_source | ||
| // Cloning universal_source to avoid modifying the value so subsequent exporters can use it. | ||
| let (local_net, local_sub) = universal_source.clone() |
There was a problem hiding this comment.
I would maybe also change this:
log::error!(target: "xcm::ethereum_blob_exporter", "could not get global consensus from universal source '{universal_source:?}'.");
SendError::Unroutable
to:
log::error!(target: "xcm::ethereum_blob_exporter", "could not get global consensus from universal source '{universal_source:?}'.");
SendError::NotApplicable
to give a chance other routers in the tuple.
There was a problem hiding this comment.
and other suggestion change MissingArgument->NotApplicable:
let para_id = match local_sub.as_slice() {
[Parachain(para_id)] => *para_id,
_ => {
log::error!(target: "xcm::ethereum_blob_exporter", "could not get parachain id from universal source '{local_sub:?}'.");
// TODO: why not `NotApplicable`?
return Err(SendError::MissingArgument)
},
};
/// A needed argument is `None` when it should be `Some`.
MissingArgument,
There was a problem hiding this comment.
and one more, I would move message.take() as a last step after all the validations pass.
And also Unroutable could be NotApplicable to give other router a chance.
let message = message.take().ok_or_else(|| {
log::error!(target: "xcm::ethereum_blob_exporter", "xcm message not provided.");
SendError::MissingArgument
})?;
let source_location = Location::new(1, local_sub.clone());
let agent_id = match AgentHashedDescription::convert_location(&source_location) {
Some(id) => id,
None => {
log::error!(target: "xcm::ethereum_blob_exporter", "unroutable due to not being able to create agent id. '{source_location:?}'");
return Err(SendError::Unroutable)
},
};
|
needs a prdoc |
|
@alistair-singh please double check this looks okay. 🙏🏻 |
Description
The EthereumBlobExporter consumes the
destparameter when the destination is notHere. Subsequent exporters will receive aNonevalue for the destination instead of the original destination value, which is incorrect.Closes #5788
Integration
Minor fix related to the exporter behaviour.
Review Notes
Verified that tests
exporter_validate_with_invalid_dest_does_not_alter_destinationandexporter_validate_with_invalid_universal_source_does_not_alter_universal_sourcefail without the fix in the exporter.