Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Optimize multilocation reanchoring logic in XCM #6301

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open
Prev Previous commit
Next Next commit
workaround to prevent mutating self in the unlikely case of an overflow
  • Loading branch information
tonyalaribe committed Nov 17, 2022
commit e4d6359c7b872a523000572936edec4a87f21405
27 changes: 22 additions & 5 deletions xcm/src/v1/multilocation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -330,17 +330,22 @@ impl MultiLocation {
/// Does not modify `self` in case of overflow.
pub fn reanchor(&mut self, target: &MultiLocation, ancestry: &MultiLocation) -> Result<(), ()> {
let mut target = target.clone();
let mut id = self.clone();

self.prepend_with(ancestry.clone()).map_err(|_| ())?;
id.prepend_with(ancestry.clone()).map_err(|_| ())?;
target.prepend_with(ancestry.clone()).map_err(|_| ())?;

// Remove common parents between now normalized self and target
while target.interior().first() == self.interior().first() {
// Remove common parents between now normalized id and target
while target.interior().first() == id.interior().first() {
target.take_first_interior();
self.take_first_interior();
id.take_first_interior();
}

self.parents = self.parent_count() + target.parent_count() + target.interior().len() as u8;
id.parents = id
.parent_count()
.saturating_add(target.parent_count().saturating_add(target.interior().len() as u8));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
id.parents = id
.parent_count()
.saturating_add(target.parent_count().saturating_add(target.interior().len() as u8));
let final_parents = (id.parent_count() as usize).saturating_add(
(target.parent_count() as usize).saturating_add(target.interior().len()),
);
if final_parents > 255 {
return Err(())
}
id.parents = final_parents as u8;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. The idea is to return an error if there's an overflow and not just saturate it to fail silently? In that case I've switched to using a checked_add instead of saturating_add. That should give us the behaviour we want.


*self = id;
Ok(())
}

Expand Down Expand Up @@ -986,6 +991,18 @@ mod tests {
assert_eq!(id, expected);
}

#[test]
fn reanchor_overflow_handled() {
let mut id = MultiLocation::new(255, X1(Parachain(255)));
tonyalaribe marked this conversation as resolved.
Show resolved Hide resolved
let original_id = id.clone();
let ancestry = (Parachain(500)).into();
let target = (Parachain(500), Parachain(501)).into();
// Does not overflow
id.reanchor(&target, &ancestry).unwrap();
tonyalaribe marked this conversation as resolved.
Show resolved Hide resolved
// id is not mutated since reanchor failed.
assert_eq!(id, original_id);
}

#[test]
fn encode_and_decode_works() {
let m = MultiLocation {
Expand Down