make set topic mandatory

Snowfork · Nov 7, 2023 · 4011aa5 · 4011aa5
1 parent 7353005
commit 4011aa5
Showing 1 changed file with 22 additions and 59 deletions.
diff --git a/parachain/primitives/router/src/outbound/mod.rs b/parachain/primitives/router/src/outbound/mod.rs
@@ -145,6 +145,7 @@ enum XcmConverterError {
 	BeneficiaryResolutionFailed,
 	AssetResolutionFailed,
 	InvalidFeeAsset,
+	SetTopicExpected,
 }
 
 macro_rules! match_expression {
@@ -248,9 +249,8 @@ impl<'a, Call> XcmConverter<'a, Call> {
 		ensure!(amount > 0, ZeroAssetTransfer);
 
 		// Check if there is a SetTopic and skip over it if found.
-		if match_expression!(self.peek(), Ok(SetTopic(id)), id).is_some() {
-			let _ = self.next();
-		}
+		let _topic_id =
+			match_expression!(self.next()?, SetTopic(id), id).ok_or(SetTopicExpected)?;
 
 		Ok(AgentExecuteCommand::TransferToken { token, recipient, amount })
 	}
@@ -687,7 +687,7 @@ mod tests {
 	}
 
 	#[test]
-	fn xcm_converter_convert_without_clear_origin_yields_success() {
+	fn xcm_converter_convert_without_buy_execution_yields_success() {
 		let network = BridgedNetwork::get();
 
 		let token_address: [u8; 20] = hex!("1000000000000000000000000000000000000000");
@@ -702,11 +702,11 @@ mod tests {
 
 		let message: Xcm<()> = vec![
 			WithdrawAsset(assets.clone()),
-			BuyExecution { fees: assets.get(0).unwrap().clone(), weight_limit: Unlimited },
 			DepositAsset {
 				assets: filter,
 				beneficiary: X1(AccountKey20 { network: None, key: beneficiary_address }).into(),
 			},
+			SetTopic([0; 32]),
 		]
 		.into();
 		let mut converter = XcmConverter::new(&message, &network);
@@ -720,7 +720,7 @@ mod tests {
 	}
 
 	#[test]
-	fn xcm_converter_convert_without_set_topic_yields_success() {
+	fn xcm_converter_convert_with_wildcard_all_asset_filter_succeeds() {
 		let network = BridgedNetwork::get();
 
 		let token_address: [u8; 20] = hex!("1000000000000000000000000000000000000000");
@@ -731,7 +731,7 @@ mod tests {
 			fun: Fungible(1000),
 		}]
 		.into();
-		let filter: MultiAssetFilter = assets.clone().into();
+		let filter: MultiAssetFilter = Wild(All);
 
 		let message: Xcm<()> = vec![
 			WithdrawAsset(assets.clone()),
@@ -741,6 +741,7 @@ mod tests {
 				assets: filter,
 				beneficiary: X1(AccountKey20 { network: None, key: beneficiary_address }).into(),
 			},
+			SetTopic([0; 32]),
 		]
 		.into();
 		let mut converter = XcmConverter::new(&message, &network);
@@ -754,25 +755,29 @@ mod tests {
 	}
 
 	#[test]
-	fn xcm_converter_convert_without_buy_execution_yields_success() {
+	fn xcm_converter_convert_with_fees_less_than_reserve_yields_success() {
 		let network = BridgedNetwork::get();
 
 		let token_address: [u8; 20] = hex!("1000000000000000000000000000000000000000");
 		let beneficiary_address: [u8; 20] = hex!("2000000000000000000000000000000000000000");
 
-		let assets: MultiAssets = vec![MultiAsset {
-			id: Concrete(X1(AccountKey20 { network: None, key: token_address }).into()),
-			fun: Fungible(1000),
-		}]
-		.into();
+		let asset_location = X1(AccountKey20 { network: None, key: token_address }).into();
+		let fee_asset = MultiAsset { id: Concrete(asset_location), fun: Fungible(500) };
+
+		let assets: MultiAssets =
+			vec![MultiAsset { id: Concrete(asset_location), fun: Fungible(1000) }].into();
+
 		let filter: MultiAssetFilter = assets.clone().into();
 
 		let message: Xcm<()> = vec![
 			WithdrawAsset(assets.clone()),
+			ClearOrigin,
+			BuyExecution { fees: fee_asset, weight_limit: Unlimited },
 			DepositAsset {
 				assets: filter,
 				beneficiary: X1(AccountKey20 { network: None, key: beneficiary_address }).into(),
 			},
+			SetTopic([0; 32]),
 		]
 		.into();
 		let mut converter = XcmConverter::new(&message, &network);
@@ -786,7 +791,7 @@ mod tests {
 	}
 
 	#[test]
-	fn xcm_converter_convert_with_wildcard_all_asset_filter_succeeds() {
+	fn xcm_converter_convert_without_clear_origin_yields_set_topic_expected() {
 		let network = BridgedNetwork::get();
 
 		let token_address: [u8; 20] = hex!("1000000000000000000000000000000000000000");
@@ -797,63 +802,21 @@ mod tests {
 			fun: Fungible(1000),
 		}]
 		.into();
-		let filter: MultiAssetFilter = Wild(All);
-
-		let message: Xcm<()> = vec![
-			WithdrawAsset(assets.clone()),
-			ClearOrigin,
-			BuyExecution { fees: assets.get(0).unwrap().clone(), weight_limit: Unlimited },
-			DepositAsset {
-				assets: filter,
-				beneficiary: X1(AccountKey20 { network: None, key: beneficiary_address }).into(),
-			},
-			SetTopic([0; 32]),
-		]
-		.into();
-		let mut converter = XcmConverter::new(&message, &network);
-		let expected_payload = AgentExecuteCommand::TransferToken {
-			token: token_address.into(),
-			recipient: beneficiary_address.into(),
-			amount: 1000,
-		};
-		let result = converter.convert();
-		assert_eq!(result, Ok(expected_payload));
-	}
-
-	#[test]
-	fn xcm_converter_convert_with_fees_less_than_reserve_yields_success() {
-		let network = BridgedNetwork::get();
-
-		let token_address: [u8; 20] = hex!("1000000000000000000000000000000000000000");
-		let beneficiary_address: [u8; 20] = hex!("2000000000000000000000000000000000000000");
-
-		let asset_location = X1(AccountKey20 { network: None, key: token_address }).into();
-		let fee_asset = MultiAsset { id: Concrete(asset_location), fun: Fungible(500) };
-
-		let assets: MultiAssets =
-			vec![MultiAsset { id: Concrete(asset_location), fun: Fungible(1000) }].into();
-
 		let filter: MultiAssetFilter = assets.clone().into();
 
 		let message: Xcm<()> = vec![
 			WithdrawAsset(assets.clone()),
-			ClearOrigin,
-			BuyExecution { fees: fee_asset, weight_limit: Unlimited },
+			BuyExecution { fees: assets.get(0).unwrap().clone(), weight_limit: Unlimited },
 			DepositAsset {
 				assets: filter,
 				beneficiary: X1(AccountKey20 { network: None, key: beneficiary_address }).into(),
 			},
-			SetTopic([0; 32]),
+			ClearTopic,
 		]
 		.into();
 		let mut converter = XcmConverter::new(&message, &network);
-		let expected_payload = AgentExecuteCommand::TransferToken {
-			token: token_address.into(),
-			recipient: beneficiary_address.into(),
-			amount: 1000,
-		};
 		let result = converter.convert();
-		assert_eq!(result, Ok(expected_payload));
+		assert_eq!(result.err(), Some(XcmConverterError::SetTopicExpected));
 	}
 
 	#[test]