-
Notifications
You must be signed in to change notification settings - Fork 746
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in bfc90ba. This case is easy to miss... Perhaps the parameters should not be mutable in the first place?
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.
@@ -71,9 +71,13 @@ impl<UniversalLocation, EthereumNetwork, OutboundQueue, AgentHashedDescription, | |||
let dest = destination.take().ok_or(SendError::MissingArgument)?; |
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.
How about just replace take
here with clone
without other reset operations?
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.
Good point, that is much better. :) 4fe4085
let (local_net, local_sub) = universal_source | ||
.take() |
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.
Same as #5789 (comment)
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.
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.
+1
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🙏🏻 |
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.
+1
Description
The EthereumBlobExporter consumes the
dest
parameter when the destination is notHere
. Subsequent exporters will receive aNone
value 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_destination
andexporter_validate_with_invalid_universal_source_does_not_alter_universal_source
fail without the fix in the exporter.