From 46ff16803850c177036c2e275976fd57a44aefe2 Mon Sep 17 00:00:00 2001 From: Valentin Fernandez Date: Fri, 9 Jun 2023 09:18:53 -0300 Subject: [PATCH 1/6] Validate location for register_reserve_asset --- pallets/asset-registry/src/lib.rs | 42 ++++-- pallets/asset-registry/src/tests.rs | 219 +++++++++++++++++++--------- 2 files changed, 181 insertions(+), 80 deletions(-) diff --git a/pallets/asset-registry/src/lib.rs b/pallets/asset-registry/src/lib.rs index 6865b71f..14378212 100644 --- a/pallets/asset-registry/src/lib.rs +++ b/pallets/asset-registry/src/lib.rs @@ -39,8 +39,8 @@ pub mod pallet { use frame_system::pallet_prelude::*; use xcm::latest::{ - Junction::{GeneralIndex, PalletInstance, Parachain}, - Junctions, MultiLocation, + Junction::{AccountId32, AccountKey20, GeneralIndex, PalletInstance}, + MultiLocation, }; #[pallet::pallet] @@ -113,16 +113,7 @@ pub mod pallet { ); // verify MultiLocation is valid - ensure!( - matches!( - asset_multi_location, - MultiLocation { - parents: 1, - interior: Junctions::X3(Parachain(_), PalletInstance(_), GeneralIndex(_)) - } - ), - Error::::WrongMultiLocation - ); + ensure!(Self::valid_location(&asset_multi_location), Error::::WrongMultiLocation); // register asset_id => asset_multi_location AssetIdMultiLocation::::insert(asset_id, &asset_multi_location); @@ -153,6 +144,33 @@ pub mod pallet { } } + impl Pallet { + fn valid_location(location: &MultiLocation) -> bool { + let (split_multilocation, last_junction) = location.clone().split_last_interior(); + + if let Some(junction) = last_junction { + match junction { + AccountId32 { .. } | AccountKey20 { .. } => true, + GeneralIndex(_) => { + let penultimate = split_multilocation.last(); + if let Some(junction) = penultimate { + match junction { + PalletInstance(_) => true, + _ => false, + } + } else { + false + } + }, + PalletInstance(_) => true, + _ => false, + } + } else { + false + } + } + } + impl xcm_primitives::AssetMultiLocationGetter> for Pallet { fn get_asset_multi_location(asset_id: AssetIdOf) -> Option { AssetIdMultiLocation::::get(asset_id) diff --git a/pallets/asset-registry/src/tests.rs b/pallets/asset-registry/src/tests.rs index 63bfac51..dd370bd5 100644 --- a/pallets/asset-registry/src/tests.rs +++ b/pallets/asset-registry/src/tests.rs @@ -29,84 +29,167 @@ const STATEMINE_ASSET_MULTI_LOCATION: MultiLocation = MultiLocation { ), }; -#[test] -fn register_reserve_asset_works() { - new_test_ext().execute_with(|| { - assert_ok!(AssetRegistry::register_reserve_asset( - RuntimeOrigin::root(), - LOCAL_ASSET_ID, - STATEMINE_ASSET_MULTI_LOCATION, - )); - - assert_eq!( - AssetIdMultiLocation::::get(LOCAL_ASSET_ID), - Some(STATEMINE_ASSET_MULTI_LOCATION) - ); - assert_eq!( - AssetMultiLocationId::::get(STATEMINE_ASSET_MULTI_LOCATION), - Some(LOCAL_ASSET_ID) - ); - }); -} - -#[test] -fn cannot_register_unexisting_asset() { - new_test_ext().execute_with(|| { - let unexisting_asset_id = 9999; +mod register_reserve_assest { + use super::*; - assert_noop!( - AssetRegistry::register_reserve_asset( + #[test] + fn register_reserve_asset_works() { + new_test_ext().execute_with(|| { + assert_ok!(AssetRegistry::register_reserve_asset( RuntimeOrigin::root(), - unexisting_asset_id, + LOCAL_ASSET_ID, STATEMINE_ASSET_MULTI_LOCATION, - ), - Error::::AssetDoesNotExist - ); - }); -} + )); + + assert_eq!( + AssetIdMultiLocation::::get(LOCAL_ASSET_ID), + Some(STATEMINE_ASSET_MULTI_LOCATION) + ); + assert_eq!( + AssetMultiLocationId::::get(STATEMINE_ASSET_MULTI_LOCATION), + Some(LOCAL_ASSET_ID) + ); + }); + } + + #[test] + fn cannot_register_unexisting_asset() { + new_test_ext().execute_with(|| { + let unexisting_asset_id = 9999; + + assert_noop!( + AssetRegistry::register_reserve_asset( + RuntimeOrigin::root(), + unexisting_asset_id, + STATEMINE_ASSET_MULTI_LOCATION, + ), + Error::::AssetDoesNotExist + ); + }); + } -#[test] -fn cannot_double_register() { - new_test_ext().execute_with(|| { - assert_ok!(AssetRegistry::register_reserve_asset( - RuntimeOrigin::root(), - LOCAL_ASSET_ID, - STATEMINE_ASSET_MULTI_LOCATION, - )); - - assert_noop!( - AssetRegistry::register_reserve_asset( + #[test] + fn cannot_double_register() { + new_test_ext().execute_with(|| { + assert_ok!(AssetRegistry::register_reserve_asset( RuntimeOrigin::root(), LOCAL_ASSET_ID, STATEMINE_ASSET_MULTI_LOCATION, - ), - Error::::AssetAlreadyRegistered - ); - }); -} + )); + + assert_noop!( + AssetRegistry::register_reserve_asset( + RuntimeOrigin::root(), + LOCAL_ASSET_ID, + STATEMINE_ASSET_MULTI_LOCATION, + ), + Error::::AssetAlreadyRegistered + ); + }); + } + + #[test] + fn valid_locations_succced() { + let native_frame_based_currency = MultiLocation{parents: 1, interior: X2(Parachain(1000), PalletInstance(1))}; + let multiasset_pallet_instance = MultiLocation{parents: 1, interior: X3(Parachain(1000), PalletInstance(1), GeneralIndex(2))}; + let erc20_frame_sm_asset = MultiLocation{parents: 1, interior: X3(Parachain(1000), PalletInstance(2), AccountId32 { network: Any, id: [0;32] })}; + let erc20_ethereum_sm_asset = MultiLocation {parents: 1, interior: X2(Parachain(2000), AccountKey20 { network: Any, key: [0;20] })}; + new_test_ext().execute_with(|| { + assert_ok!(AssetRegistry::register_reserve_asset( + RuntimeOrigin::root(), + LOCAL_ASSET_ID, + native_frame_based_currency, + )); + }); + new_test_ext().execute_with(|| { + assert_ok!(AssetRegistry::register_reserve_asset( + RuntimeOrigin::root(), + LOCAL_ASSET_ID, + multiasset_pallet_instance, + )); + }); + new_test_ext().execute_with(|| { + assert_ok!(AssetRegistry::register_reserve_asset( + RuntimeOrigin::root(), + LOCAL_ASSET_ID, + erc20_frame_sm_asset, + )); + }); + new_test_ext().execute_with(|| { + assert_ok!(AssetRegistry::register_reserve_asset( + RuntimeOrigin::root(), + LOCAL_ASSET_ID, + erc20_ethereum_sm_asset, + )); + }); + } -#[test] -fn unregister_reserve_asset_works() { - new_test_ext().execute_with(|| { - assert_ok!(AssetRegistry::register_reserve_asset( - RuntimeOrigin::root(), - LOCAL_ASSET_ID, - STATEMINE_ASSET_MULTI_LOCATION, - )); + #[test] + fn invalid_locations_fail() { + let trappist_location = MultiLocation{parents: 0, interior: Here}; + let governance_location = MultiLocation {parents: 1, interior: X2(Parachain(1000), Plurality { id: BodyId::Executive, part: BodyPart::Voice })}; + let invalid_general_index = MultiLocation {parents: 1, interior: X2(Parachain(1000), GeneralIndex(1u128))}; + + new_test_ext().execute_with(|| { + assert_noop!( + AssetRegistry::register_reserve_asset( + RuntimeOrigin::root(), + LOCAL_ASSET_ID, + trappist_location, + ), + Error::::WrongMultiLocation + ); - assert_ok!(AssetRegistry::unregister_reserve_asset(RuntimeOrigin::root(), LOCAL_ASSET_ID)); + assert_noop!( + AssetRegistry::register_reserve_asset( + RuntimeOrigin::root(), + LOCAL_ASSET_ID, + governance_location, + ), + Error::::WrongMultiLocation + ); - assert!(AssetIdMultiLocation::::get(LOCAL_ASSET_ID).is_none()); - assert!(AssetMultiLocationId::::get(STATEMINE_ASSET_MULTI_LOCATION).is_none()); - }); + assert_noop!( + AssetRegistry::register_reserve_asset( + RuntimeOrigin::root(), + LOCAL_ASSET_ID, + invalid_general_index, + ), + Error::::WrongMultiLocation + ); + }) + } } -#[test] -fn cannot_register_unregistered_asset() { - new_test_ext().execute_with(|| { - assert_noop!( - AssetRegistry::unregister_reserve_asset(RuntimeOrigin::root(), LOCAL_ASSET_ID), - Error::::AssetIsNotRegistered - ); - }); +mod unregister_reserve_asset { + use super::*; + + #[test] + fn unregister_reserve_asset_works() { + new_test_ext().execute_with(|| { + assert_ok!(AssetRegistry::register_reserve_asset( + RuntimeOrigin::root(), + LOCAL_ASSET_ID, + STATEMINE_ASSET_MULTI_LOCATION, + )); + + assert_ok!(AssetRegistry::unregister_reserve_asset( + RuntimeOrigin::root(), + LOCAL_ASSET_ID + )); + + assert!(AssetIdMultiLocation::::get(LOCAL_ASSET_ID).is_none()); + assert!(AssetMultiLocationId::::get(STATEMINE_ASSET_MULTI_LOCATION).is_none()); + }); + } + + #[test] + fn cannot_register_unregistered_asset() { + new_test_ext().execute_with(|| { + assert_noop!( + AssetRegistry::unregister_reserve_asset(RuntimeOrigin::root(), LOCAL_ASSET_ID), + Error::::AssetIsNotRegistered + ); + }); + } } From a9eccf9ad50777f3377ff3f3d4d5e38d8ceee6cd Mon Sep 17 00:00:00 2001 From: Valentin Fernandez Date: Fri, 9 Jun 2023 09:29:45 -0300 Subject: [PATCH 2/6] fmt --- pallets/asset-registry/src/lib.rs | 9 +++++-- pallets/asset-registry/src/tests.rs | 37 ++++++++++++++++++++++------- 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/pallets/asset-registry/src/lib.rs b/pallets/asset-registry/src/lib.rs index 14378212..378423b1 100644 --- a/pallets/asset-registry/src/lib.rs +++ b/pallets/asset-registry/src/lib.rs @@ -113,7 +113,10 @@ pub mod pallet { ); // verify MultiLocation is valid - ensure!(Self::valid_location(&asset_multi_location), Error::::WrongMultiLocation); + ensure!( + Self::valid_asset_location(&asset_multi_location), + Error::::WrongMultiLocation + ); // register asset_id => asset_multi_location AssetIdMultiLocation::::insert(asset_id, &asset_multi_location); @@ -145,7 +148,9 @@ pub mod pallet { } impl Pallet { - fn valid_location(location: &MultiLocation) -> bool { + //Validates that the location points to an asset (Native, Frame based, Erc20) as described + // in the xcm-format: https://github.com/paritytech/xcm-format#concrete-identifiers + fn valid_asset_location(location: &MultiLocation) -> bool { let (split_multilocation, last_junction) = location.clone().split_last_interior(); if let Some(junction) = last_junction { diff --git a/pallets/asset-registry/src/tests.rs b/pallets/asset-registry/src/tests.rs index dd370bd5..9d623d31 100644 --- a/pallets/asset-registry/src/tests.rs +++ b/pallets/asset-registry/src/tests.rs @@ -90,10 +90,24 @@ mod register_reserve_assest { #[test] fn valid_locations_succced() { - let native_frame_based_currency = MultiLocation{parents: 1, interior: X2(Parachain(1000), PalletInstance(1))}; - let multiasset_pallet_instance = MultiLocation{parents: 1, interior: X3(Parachain(1000), PalletInstance(1), GeneralIndex(2))}; - let erc20_frame_sm_asset = MultiLocation{parents: 1, interior: X3(Parachain(1000), PalletInstance(2), AccountId32 { network: Any, id: [0;32] })}; - let erc20_ethereum_sm_asset = MultiLocation {parents: 1, interior: X2(Parachain(2000), AccountKey20 { network: Any, key: [0;20] })}; + let native_frame_based_currency = + MultiLocation { parents: 1, interior: X2(Parachain(1000), PalletInstance(1)) }; + let multiasset_pallet_instance = MultiLocation { + parents: 1, + interior: X3(Parachain(1000), PalletInstance(1), GeneralIndex(2)), + }; + let erc20_frame_sm_asset = MultiLocation { + parents: 1, + interior: X3( + Parachain(1000), + PalletInstance(2), + AccountId32 { network: Any, id: [0; 32] }, + ), + }; + let erc20_ethereum_sm_asset = MultiLocation { + parents: 1, + interior: X2(Parachain(2000), AccountKey20 { network: Any, key: [0; 20] }), + }; new_test_ext().execute_with(|| { assert_ok!(AssetRegistry::register_reserve_asset( RuntimeOrigin::root(), @@ -126,10 +140,17 @@ mod register_reserve_assest { #[test] fn invalid_locations_fail() { - let trappist_location = MultiLocation{parents: 0, interior: Here}; - let governance_location = MultiLocation {parents: 1, interior: X2(Parachain(1000), Plurality { id: BodyId::Executive, part: BodyPart::Voice })}; - let invalid_general_index = MultiLocation {parents: 1, interior: X2(Parachain(1000), GeneralIndex(1u128))}; - + let trappist_location = MultiLocation { parents: 0, interior: Here }; + let governance_location = MultiLocation { + parents: 1, + interior: X2( + Parachain(1000), + Plurality { id: BodyId::Executive, part: BodyPart::Voice }, + ), + }; + let invalid_general_index = + MultiLocation { parents: 1, interior: X2(Parachain(1000), GeneralIndex(1u128)) }; + new_test_ext().execute_with(|| { assert_noop!( AssetRegistry::register_reserve_asset( From 8ccaf7940e5d2b3eeea954c2aad3e5ab91fd2bdd Mon Sep 17 00:00:00 2001 From: Valentin Fernandez Date: Fri, 9 Jun 2023 09:31:16 -0300 Subject: [PATCH 3/6] fmt --- pallets/asset-registry/src/tests.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/pallets/asset-registry/src/tests.rs b/pallets/asset-registry/src/tests.rs index 9d623d31..1e35ba2e 100644 --- a/pallets/asset-registry/src/tests.rs +++ b/pallets/asset-registry/src/tests.rs @@ -108,6 +108,7 @@ mod register_reserve_assest { parents: 1, interior: X2(Parachain(2000), AccountKey20 { network: Any, key: [0; 20] }), }; + new_test_ext().execute_with(|| { assert_ok!(AssetRegistry::register_reserve_asset( RuntimeOrigin::root(), From 63318bab7499a15c230c70a402efde86cf7b05d6 Mon Sep 17 00:00:00 2001 From: Valentin Fernandez Date: Fri, 9 Jun 2023 09:37:51 -0300 Subject: [PATCH 4/6] fmt --- pallets/asset-registry/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pallets/asset-registry/src/tests.rs b/pallets/asset-registry/src/tests.rs index 1e35ba2e..d86135c2 100644 --- a/pallets/asset-registry/src/tests.rs +++ b/pallets/asset-registry/src/tests.rs @@ -108,7 +108,7 @@ mod register_reserve_assest { parents: 1, interior: X2(Parachain(2000), AccountKey20 { network: Any, key: [0; 20] }), }; - + new_test_ext().execute_with(|| { assert_ok!(AssetRegistry::register_reserve_asset( RuntimeOrigin::root(), From cda8efedb6f4aebe11f1c65be396bd9d2a2fb2cc Mon Sep 17 00:00:00 2001 From: Valentin Fernandez Date: Fri, 9 Jun 2023 10:56:31 -0300 Subject: [PATCH 5/6] validator refactor --- pallets/asset-registry/src/lib.rs | 36 ++++++++++++++--------------- pallets/asset-registry/src/tests.rs | 18 +++++++-------- 2 files changed, 25 insertions(+), 29 deletions(-) diff --git a/pallets/asset-registry/src/lib.rs b/pallets/asset-registry/src/lib.rs index 378423b1..20af4f2d 100644 --- a/pallets/asset-registry/src/lib.rs +++ b/pallets/asset-registry/src/lib.rs @@ -38,9 +38,11 @@ pub mod pallet { use frame_support::{pallet_prelude::*, traits::tokens::fungibles::Inspect}; use frame_system::pallet_prelude::*; - use xcm::latest::{ - Junction::{AccountId32, AccountKey20, GeneralIndex, PalletInstance}, - MultiLocation, + use xcm::{ + latest::{ + Junction::{AccountId32, AccountKey20, GeneralIndex, PalletInstance}, + MultiLocation, + }, }; #[pallet::pallet] @@ -153,26 +155,22 @@ pub mod pallet { fn valid_asset_location(location: &MultiLocation) -> bool { let (split_multilocation, last_junction) = location.clone().split_last_interior(); - if let Some(junction) = last_junction { - match junction { - AccountId32 { .. } | AccountKey20 { .. } => true, - GeneralIndex(_) => { + let check = matches!( + last_junction, + Some(AccountId32 { .. }) | + Some(AccountKey20 { .. }) | + Some(PalletInstance(_)) | + None + ); + + check | + match last_junction { + Some(GeneralIndex(_)) => { let penultimate = split_multilocation.last(); - if let Some(junction) = penultimate { - match junction { - PalletInstance(_) => true, - _ => false, - } - } else { - false - } + matches!(penultimate, Some(PalletInstance(_))) }, - PalletInstance(_) => true, _ => false, } - } else { - false - } } } diff --git a/pallets/asset-registry/src/tests.rs b/pallets/asset-registry/src/tests.rs index d86135c2..f3d10abb 100644 --- a/pallets/asset-registry/src/tests.rs +++ b/pallets/asset-registry/src/tests.rs @@ -96,6 +96,7 @@ mod register_reserve_assest { parents: 1, interior: X3(Parachain(1000), PalletInstance(1), GeneralIndex(2)), }; + let relay_native_currency = MultiLocation { parents: 1, interior: Junctions::Here }; let erc20_frame_sm_asset = MultiLocation { parents: 1, interior: X3( @@ -123,6 +124,13 @@ mod register_reserve_assest { multiasset_pallet_instance, )); }); + new_test_ext().execute_with(|| { + assert_ok!(AssetRegistry::register_reserve_asset( + RuntimeOrigin::root(), + LOCAL_ASSET_ID, + relay_native_currency, + )); + }); new_test_ext().execute_with(|| { assert_ok!(AssetRegistry::register_reserve_asset( RuntimeOrigin::root(), @@ -141,7 +149,6 @@ mod register_reserve_assest { #[test] fn invalid_locations_fail() { - let trappist_location = MultiLocation { parents: 0, interior: Here }; let governance_location = MultiLocation { parents: 1, interior: X2( @@ -153,15 +160,6 @@ mod register_reserve_assest { MultiLocation { parents: 1, interior: X2(Parachain(1000), GeneralIndex(1u128)) }; new_test_ext().execute_with(|| { - assert_noop!( - AssetRegistry::register_reserve_asset( - RuntimeOrigin::root(), - LOCAL_ASSET_ID, - trappist_location, - ), - Error::::WrongMultiLocation - ); - assert_noop!( AssetRegistry::register_reserve_asset( RuntimeOrigin::root(), From 9da09ef76cf57cb15b1070575eea2348c56c3757 Mon Sep 17 00:00:00 2001 From: Valentin Fernandez Date: Fri, 9 Jun 2023 10:57:52 -0300 Subject: [PATCH 6/6] fmt --- pallets/asset-registry/src/lib.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/pallets/asset-registry/src/lib.rs b/pallets/asset-registry/src/lib.rs index 20af4f2d..fc820270 100644 --- a/pallets/asset-registry/src/lib.rs +++ b/pallets/asset-registry/src/lib.rs @@ -38,11 +38,9 @@ pub mod pallet { use frame_support::{pallet_prelude::*, traits::tokens::fungibles::Inspect}; use frame_system::pallet_prelude::*; - use xcm::{ - latest::{ - Junction::{AccountId32, AccountKey20, GeneralIndex, PalletInstance}, - MultiLocation, - }, + use xcm::latest::{ + Junction::{AccountId32, AccountKey20, GeneralIndex, PalletInstance}, + MultiLocation, }; #[pallet::pallet]