Update review comments from Adrian

Update cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/xcm_config.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/bridge_hub_config.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/bridge_hub_config.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/xcm_config.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/bridge_hub_config.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/assets/common/src/matching.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/assets/common/src/matching.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/assets/common/src/matching.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/assets/common/src/matching.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/assets/common/src/matching.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/assets/common/src/matching.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/assets/common/src/matching.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/assets/asset-hub-kusama/src/xcm_config.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/assets/asset-hub-kusama/src/xcm_config.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/xcm_config.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/xcm_config.rs Co-authored-by: Adrian Catangiu <[email protected]> Update cumulus/parachains/runtimes/assets/common/src/matching.rs Co-authored-by: Adrian Catangiu <[email protected]> Fmt + one any to all Co-authored-by: Adrian Catangiu <[email protected]>
paritytech · Sep 5, 2023 · fd705a8 · fd705a8
1 parent 21037dc
commit fd705a8
Show file tree

Hide file tree

Showing 8 changed files with 33 additions and 34 deletions.
diff --git a/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/xcm_config.rs b/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/xcm_config.rs
@@ -608,8 +608,7 @@ impl pallet_xcm::Config for Runtime {
 	type XcmTeleportFilter = Everything;
 	// Allow reserve based transfer to everywhere except for bridging, here we strictly check what
 	// assets are allowed.
-	type XcmReserveTransferFilter =
-		EverythingBut<bridging::IsNotAllowedExplicitlyForReserveTransfer>;
+	type XcmReserveTransferFilter = EverythingBut<bridging::DisallowForReserveTransfer>;
 	type Weigher = WeightInfoBounds<
 		crate::weights::xcm::AssetHubKusamaXcmWeight<RuntimeCall>,
 		RuntimeCall,
@@ -775,11 +774,12 @@ pub mod bridging {
 		>;
 
 	/// Filter out those assets which are not allowed for bridged reserve based transfer.
-	/// (asset, location) filter for `pallet_xcm::Config::XcmReserveTransferFilter`.
-	pub type IsNotAllowedExplicitlyForReserveTransfer = ExcludeOnlyForRemoteDestination<
+	/// Filter is made of (asset, location) and used by
+	/// `pallet_xcm::Config::XcmReserveTransferFilter`.
+	pub type DisallowForReserveTransfer = ExcludeOnlyForRemoteDestination<
 		UniversalLocation,
 		FilteredNetworkExportTable,
-		IsNotAllowedConcreteAssetBy<AllowedReserveTransferAssetsLocations>,
+		DisallowConcreteAssetUnless<AllowedReserveTransferAssetsLocations>,
 	>;
 
 	impl Contains<RuntimeCall> for ToPolkadotXcmRouter {

diff --git a/cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/xcm_config.rs b/cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/xcm_config.rs
@@ -528,8 +528,7 @@ impl pallet_xcm::Config for Runtime {
 	type XcmTeleportFilter = Everything;
 	// Allow reserve based transfer to everywhere except for bridging, here we strictly check what
 	// assets are allowed.
-	type XcmReserveTransferFilter =
-		EverythingBut<bridging::IsNotAllowedExplicitlyForReserveTransfer>;
+	type XcmReserveTransferFilter = EverythingBut<bridging::DisallowForReserveTransfer>;
 	type Weigher = WeightInfoBounds<
 		crate::weights::xcm::AssetHubPolkadotXcmWeight<RuntimeCall>,
 		RuntimeCall,
@@ -682,11 +681,12 @@ pub mod bridging {
 		>;
 
 	/// Filter out those assets which are not allowed for bridged reserve based transfer.
-	/// (asset, location) filter for `pallet_xcm::Config::XcmReserveTransferFilter`.
-	pub type IsNotAllowedExplicitlyForReserveTransfer = ExcludeOnlyForRemoteDestination<
+	/// Filter is made of (asset, location) and used by
+	/// `pallet_xcm::Config::XcmReserveTransferFilter`.
+	pub type DisallowForReserveTransfer = ExcludeOnlyForRemoteDestination<
 		UniversalLocation,
 		FilteredNetworkExportTable,
-		IsNotAllowedConcreteAssetBy<AllowedReserveTransferAssetsLocations>,
+		DisallowConcreteAssetUnless<AllowedReserveTransferAssetsLocations>,
 	>;
 
 	impl Contains<RuntimeCall> for ToKusamaXcmRouter {

diff --git a/cumulus/parachains/runtimes/assets/common/src/matching.rs b/cumulus/parachains/runtimes/assets/common/src/matching.rs
@@ -126,31 +126,31 @@ impl<
 
 		// check asset according to the configured reserve locations
 		for (reserve_location, asset_filter) in Reserves::get() {
-			if origin.eq(&reserve_location) {
-				if asset_filter.matches(asset_location) {
-					return true
-				}
+			if origin.eq(&reserve_location) && asset_filter.matches(asset_location) {
+				return true
 			}
 		}
 
 		false
 	}
 }
 
-/// Filter assets that are explicitly not allowed for destination.
+/// Disallow all assets the are either not `Concrete`, or not explicitly allowed by
+/// `LocationAssetFilters`, iff `dest` matches any location in `LocationAssetFilters`.
 ///
-/// Returns true if asset is not `Concrete` or is explicitly not allowed by `LocationAssetFilters`.
-/// Returns false if `dest` does not match any location in `LocationAssetFilters`.
-pub struct IsNotAllowedConcreteAssetBy<LocationAssetFilters>(
+/// Returns `false` regardless of `assets`, if `dest` does not match any location in
+/// `LocationAssetFilters`. Otherwise, returns `true` if asset is either not `Concrete` or is not
+/// explicitly allowed by `LocationAssetFilters`, otherwise returns `false`.
+pub struct DisallowConcreteAssetUnless<LocationAssetFilters>(
 	sp_std::marker::PhantomData<LocationAssetFilters>,
 );
 impl<LocationAssetFilters: Get<sp_std::vec::Vec<FilteredLocation>>>
 	Contains<(MultiLocation, sp_std::vec::Vec<MultiAsset>)>
-	for IsNotAllowedConcreteAssetBy<LocationAssetFilters>
+	for DisallowConcreteAssetUnless<LocationAssetFilters>
 {
 	fn contains((dest, assets): &(MultiLocation, sp_std::vec::Vec<MultiAsset>)) -> bool {
 		for (allowed_dest, asset_filter) in LocationAssetFilters::get().iter() {
-			// we care only about explicitly configured destinations
+			// we only disallow `assets` on explicitly configured destinations
 			if !allowed_dest.eq(dest) {
 				continue
 			}
@@ -162,22 +162,21 @@ impl<LocationAssetFilters: Get<sp_std::vec::Vec<FilteredLocation>>>
 					_ => return true,
 				};
 
-				// filter have to match for all assets
 				if !asset_filter.matches(asset_location) {
-					// if asset does not match filter, we found explicitly not allowed asset
+					// if asset does not match filter, disallow it
 					return true
 				}
 			}
 		}
 
-		// by default, everything is allowed
+		// if we got here, allow it
 		false
 	}
 }
 
-/// Adapter for `Contains<(MultiLocation, sp_std::vec::Vec<MultiAsset>)>` which checks
-/// if `Exporters` contains exporter for **remote** `MultiLocation` and iff so, then checks
-/// `Filter`, anyway return false.
+/// Adapter for `Contains<(MultiLocation, sp_std::vec::Vec<MultiAsset>)>` which returns `true`
+/// iff `Exporters` contains exporter for **remote** `MultiLocation` _and_
+///`assets` also pass`Filter`, otherwise returns `false`.
 ///
 /// Note: Assumes that `Exporters` do not depend on `XCM program` and works for `Xcm::default()`.
 pub struct ExcludeOnlyForRemoteDestination<UniversalLocation, Exporters, Exclude>(
@@ -204,6 +203,7 @@ where
 				if Exporters::exporter_for(&remote_network, &remote_destination, &Xcm::default())
 					.is_some()
 				{
+					// destination is remote, and has configured exporter, now check filter
 					Exclude::contains(dest_and_assets)
 				} else {
 					log::trace!(
@@ -221,7 +221,7 @@ where
 					"CheckOnlyForRemoteDestination dest: {:?} is not remote to the universal_source: {:?}",
 					dest_and_assets.0, universal_source
 				);
-				//
+				// not a remote destination, do not exclude
 				false
 			},
 		}

diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/bridge_hub_config.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/bridge_hub_config.rs
@@ -14,7 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Cumulus.  If not, see <http://www.gnu.org/licenses/>.
 
-//! Bridge definitions.
+//! Kusama BridgeHub definitions.
 
 use crate::{
 	BridgeParachainPolkadotInstance, BridgePolkadotMessages, Runtime,

diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/xcm_config.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/xcm_config.rs
@@ -134,7 +134,7 @@ impl Contains<RuntimeCall> for SafeCallFilter {
 		// Allow to change dedicated storage items (called by governance-like)
 		match call {
 			RuntimeCall::System(frame_system::Call::set_storage { items })
-				if items.iter().any(|(k, _)| {
+				if items.iter().all(|(k, _)| {
 					k.eq(&DeliveryRewardInBalance::key()) |
 						k.eq(&RequiredStakeForStakeAndSlash::key())
 				}) =>

diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/bridge_hub_config.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/bridge_hub_config.rs
@@ -14,7 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Cumulus.  If not, see <http://www.gnu.org/licenses/>.
 
-//! Bridge definitions.
+//! Polkadot BridgeHub definitions.
 
 use crate::{
 	BridgeKusamaMessages, BridgeParachainKusamaInstance, Runtime,
@@ -168,8 +168,7 @@ pub type BridgeRefundBridgeHubKusamaMessages = RefundBridgedParachainMessages<
 bp_runtime::generate_static_str_provider!(BridgeRefundBridgeHubKusamaMessages);
 
 // TODO: rework once dynamic lanes are supported (https://github.com/paritytech/parity-bridges-common/issues/1760)
-//       now we support only StatemineToStatemint
-/// Lanes setup
+//       now we support only AHP<>AHK Lanes setup
 pub const ASSET_HUB_POLKADOT_TO_ASSET_HUB_KUSAMA_LANE_ID: LaneId = LaneId([0, 0, 0, 0]);
 parameter_types! {
 	pub ActiveOutboundLanesToBridgeHubKusama: &'static [bp_messages::LaneId] = &[ASSET_HUB_POLKADOT_TO_ASSET_HUB_KUSAMA_LANE_ID];

diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/xcm_config.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/xcm_config.rs
@@ -137,7 +137,7 @@ impl Contains<RuntimeCall> for SafeCallFilter {
 		// Allow to change dedicated storage items (called by governance-like)
 		match call {
 			RuntimeCall::System(frame_system::Call::set_storage { items })
-				if items.iter().any(|(k, _)| {
+				if items.iter().all(|(k, _)| {
 					k.eq(&DeliveryRewardInBalance::key()) |
 						k.eq(&RequiredStakeForStakeAndSlash::key())
 				}) =>

diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs
@@ -154,7 +154,7 @@ impl Contains<RuntimeCall> for SafeCallFilter {
 		// Allow to change dedicated storage items (called by governance-like)
 		match call {
 			RuntimeCall::System(frame_system::Call::set_storage { items })
-				if items.iter().any(|(k, _)| {
+				if items.iter().all(|(k, _)| {
 					k.eq(&DeliveryRewardInBalance::key()) |
 						k.eq(&RequiredStakeForStakeAndSlash::key())
 				}) =>