[stable2506] Backport #9179

polkadot/xcm/xcm-executor/src/assets.rs

@@ -132,34 +132,21 @@ impl AssetsInHolding {
 
 	/// Mutate `self` to contain all given `assets`, saturating if necessary.
 	///
-	/// NOTE: [`AssetsInHolding`] are always sorted, allowing us to optimize this function from
-	/// `O(n^2)` to `O(n)`.
+	/// NOTE: [`AssetsInHolding`] are always sorted
 	pub fn subsume_assets(&mut self, mut assets: AssetsInHolding) {
-		let mut f_iter = assets.fungible.iter_mut();
-		let mut g_iter = self.fungible.iter_mut();
-		if let (Some(mut f), Some(mut g)) = (f_iter.next(), g_iter.next()) {
-			loop {
-				if f.0 == g.0 {
-					// keys are equal. in this case, we add `self`'s balance for the asset onto
-					// `assets`, balance, knowing that the `append` operation which follows will
-					// clobber `self`'s value and only use `assets`'s.
-					(*f.1).saturating_accrue(*g.1);
-				}
-				if f.0 <= g.0 {
-					f = match f_iter.next() {
-						Some(x) => x,
-						None => break,
-					};
-				}
-				if f.0 >= g.0 {
-					g = match g_iter.next() {
-						Some(x) => x,
-						None => break,
-					};
-				}
-			}
+		// for fungibles, find matching fungibles and sum their amounts so we end-up having just
+		// single such fungible but with increased amount inside
+		for (asset_id, asset_amount) in assets.fungible {
+			self.fungible
+				.entry(asset_id)
+				.and_modify(|current_asset_amount| {
+					current_asset_amount.saturating_accrue(asset_amount)
+				})
+				.or_insert(asset_amount);
 		}
-		self.fungible.append(&mut assets.fungible);
+		// for non-fungibles, every entry is unique so there is no notion of amount to sum-up
+		// together if there is the same non-fungible in both holdings (same instance_id) these
+		// will be collapsed into just single one
 		self.non_fungible.append(&mut assets.non_fungible);
 	}
 
@@ -530,6 +517,21 @@ mod tests {
 		(Here, amount).into()
 	}
 	#[allow(non_snake_case)]
+	/// Concrete fungible constructor with index for GeneralIndex
+	fn CFG(index: u128, amount: u128) -> Asset {
+		(GeneralIndex(index), amount).into()
+	}
+	#[allow(non_snake_case)]
+	/// Concrete fungible constructor (parent=1)
+	fn CFP(amount: u128) -> Asset {
+		(Parent, amount).into()
+	}
+	#[allow(non_snake_case)]
+	/// Concrete fungible constructor (parent=2)
+	fn CFPP(amount: u128) -> Asset {
+		((Parent, Parent), amount).into()
+	}
+	#[allow(non_snake_case)]
 	/// Concrete non-fungible constructor
 	fn CNF(instance_id: u8) -> Asset {
 		(Here, [instance_id; 4]).into()
@@ -543,18 +545,156 @@ mod tests {
 	}
 
 	#[test]
-	fn subsume_assets_works() {
-		let t1 = test_assets();
+	fn assets_in_holding_order_works() {
+		// populate assets in non-ordered fashion
+		let mut assets = AssetsInHolding::new();
+		assets.subsume(CFPP(300));
+		assets.subsume(CFP(200));
+		assets.subsume(CNF(2));
+		assets.subsume(CF(100));
+		assets.subsume(CNF(1));
+		assets.subsume(CFG(10, 400));
+		assets.subsume(CFG(15, 500));
+
+		// following is the order we expect from AssetsInHolding
+		// - fungibles before non-fungibles
+		// - for fungibles, sort by parent first, if parents match, then by other components like
+		//   general index
+		// - for non-fungibles, sort by instance_id
+		let mut iter = assets.clone().into_assets_iter();
+		// fungible, order by parent, parent=0
+		assert_eq!(Some(CF(100)), iter.next());
+		// fungible, order by parent then by general index, parent=0, general index=10
+		assert_eq!(Some(CFG(10, 400)), iter.next());
+		// fungible, order by parent then by general index, parent=0, general index=15
+		assert_eq!(Some(CFG(15, 500)), iter.next());
+		// fungible, order by parent, parent=1
+		assert_eq!(Some(CFP(200)), iter.next());
+		// fungible, order by parent, parent=2
+		assert_eq!(Some(CFPP(300)), iter.next());
+		// non-fungible, after fungibles, order by instance id, id=1
+		assert_eq!(Some(CNF(1)), iter.next());
+		// non-fungible, after fungibles, order by instance id, id=2
+		assert_eq!(Some(CNF(2)), iter.next());
+		// nothing else in the assets
+		assert_eq!(None, iter.next());
+
+		// lets add copy of the assets to the assets itself, just to check if order stays the same
+		// we also expect 2x amount for every fungible and collapsed non-fungibles
+		let assets_same = assets.clone();
+		assets.subsume_assets(assets_same);
+
+		let mut iter = assets.into_assets_iter();
+		assert_eq!(Some(CF(200)), iter.next());
+		assert_eq!(Some(CFG(10, 800)), iter.next());
+		assert_eq!(Some(CFG(15, 1000)), iter.next());
+		assert_eq!(Some(CFP(400)), iter.next());
+		assert_eq!(Some(CFPP(600)), iter.next());
+		assert_eq!(Some(CNF(1)), iter.next());
+		assert_eq!(Some(CNF(2)), iter.next());
+		assert_eq!(None, iter.next());
+	}
+
+	#[test]
+	fn subsume_assets_equal_length_holdings() {
+		let mut t1 = test_assets();
 		let mut t2 = AssetsInHolding::new();
 		t2.subsume(CF(300));
 		t2.subsume(CNF(50));
-		let mut r1 = t1.clone();
-		r1.subsume_assets(t2.clone());
-		let mut r2 = t1.clone();
-		for a in t2.assets_iter() {
-			r2.subsume(a)
-		}
-		assert_eq!(r1, r2);
+
+		let t1_clone = t1.clone();
+		let mut t2_clone = t2.clone();
+
+		// ensure values for same fungibles are summed up together
+		// and order is also ok (see assets_in_holding_order_works())
+		t1.subsume_assets(t2.clone());
+		let mut iter = t1.into_assets_iter();
+		assert_eq!(Some(CF(600)), iter.next());
+		assert_eq!(Some(CNF(40)), iter.next());
+		assert_eq!(Some(CNF(50)), iter.next());
+		assert_eq!(None, iter.next());
+
+		// try the same initial holdings but other way around
+		// expecting same exact result as above
+		t2_clone.subsume_assets(t1_clone.clone());
+		let mut iter = t2_clone.into_assets_iter();
+		assert_eq!(Some(CF(600)), iter.next());
+		assert_eq!(Some(CNF(40)), iter.next());
+		assert_eq!(Some(CNF(50)), iter.next());
+		assert_eq!(None, iter.next());
+	}
+
+	#[test]
+	fn subsume_assets_different_length_holdings() {
+		let mut t1 = AssetsInHolding::new();
+		t1.subsume(CFP(400));
+		t1.subsume(CFPP(100));
+
+		let mut t2 = AssetsInHolding::new();
+		t2.subsume(CF(100));
+		t2.subsume(CNF(50));
+		t2.subsume(CNF(40));
+		t2.subsume(CFP(100));
+		t2.subsume(CFPP(100));
+
+		let t1_clone = t1.clone();
+		let mut t2_clone = t2.clone();
+
+		// ensure values for same fungibles are summed up together
+		// and order is also ok (see assets_in_holding_order_works())
+		t1.subsume_assets(t2);
+		let mut iter = t1.into_assets_iter();
+		assert_eq!(Some(CF(100)), iter.next());
+		assert_eq!(Some(CFP(500)), iter.next());
+		assert_eq!(Some(CFPP(200)), iter.next());
+		assert_eq!(Some(CNF(40)), iter.next());
+		assert_eq!(Some(CNF(50)), iter.next());
+		assert_eq!(None, iter.next());
+
+		// try the same initial holdings but other way around
+		// expecting same exact result as above
+		t2_clone.subsume_assets(t1_clone);
+		let mut iter = t2_clone.into_assets_iter();
+		assert_eq!(Some(CF(100)), iter.next());
+		assert_eq!(Some(CFP(500)), iter.next());
+		assert_eq!(Some(CFPP(200)), iter.next());
+		assert_eq!(Some(CNF(40)), iter.next());
+		assert_eq!(Some(CNF(50)), iter.next());
+		assert_eq!(None, iter.next());
+	}
+
+	#[test]
+	fn subsume_assets_empty_holding() {
+		let mut t1 = AssetsInHolding::new();
+		let t2 = AssetsInHolding::new();
+		t1.subsume_assets(t2.clone());
+		let mut iter = t1.clone().into_assets_iter();
+		assert_eq!(None, iter.next());
+
+		t1.subsume(CFP(400));
+		t1.subsume(CNF(40));
+		t1.subsume(CFPP(100));
+
+		let t1_clone = t1.clone();
+		let mut t2_clone = t2.clone();
+
+		// ensure values for same fungibles are summed up together
+		// and order is also ok (see assets_in_holding_order_works())
+		t1.subsume_assets(t2.clone());
+		let mut iter = t1.into_assets_iter();
+		assert_eq!(Some(CFP(400)), iter.next());
+		assert_eq!(Some(CFPP(100)), iter.next());
+		assert_eq!(Some(CNF(40)), iter.next());
+		assert_eq!(None, iter.next());
+
+		// try the same initial holdings but other way around
+		// expecting same exact result as above
+		t2_clone.subsume_assets(t1_clone.clone());
+		let mut iter = t2_clone.into_assets_iter();
+		assert_eq!(Some(CFP(400)), iter.next());
+		assert_eq!(Some(CFPP(100)), iter.next());
+		assert_eq!(Some(CNF(40)), iter.next());
+		assert_eq!(None, iter.next());
 	}
 
 	#[test]

prdoc/pr_9179.prdoc

@@ -0,0 +1,10 @@
+title: 'Fix subsume_assets incorrectly merging two AssetsInHolding'
+doc:
+- audience: Runtime Dev
+  description: |
+    Fix subsume_assets incorrectly merging two AssetsInHolding instances under certain conditions,
+    which caused asset values to be overridden rather than summed.
+
+crates:
+- name: staging-xcm-executor
+  bump: patch