From 6adf1b1d970ad25b49edb8d13816047ee2f45677 Mon Sep 17 00:00:00 2001 From: Daanvdplas Date: Fri, 16 Aug 2024 15:56:50 +0200 Subject: [PATCH 1/3] refactor: fungibles pallet --- pallets/api/Cargo.toml | 2 +- pallets/api/src/fungibles/mod.rs | 227 +++++++++++++++-------------- pallets/api/src/fungibles/tests.rs | 202 +++++++++++++------------ 3 files changed, 224 insertions(+), 207 deletions(-) diff --git a/pallets/api/Cargo.toml b/pallets/api/Cargo.toml index a813a09c..f9a807a3 100644 --- a/pallets/api/Cargo.toml +++ b/pallets/api/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "pallet-api" authors.workspace = true -description = "Api pallet, enabling smart(er) contracts with the power of Polkadot" +description = "API pallet, enabling smart(er) contracts with the power of Polkadot" edition.workspace = true license.workspace = true version = "0.1.0" diff --git a/pallets/api/src/fungibles/mod.rs b/pallets/api/src/fungibles/mod.rs index 7aa9ac00..feb34a21 100644 --- a/pallets/api/src/fungibles/mod.rs +++ b/pallets/api/src/fungibles/mod.rs @@ -1,6 +1,11 @@ -/// The fungibles pallet serves as a wrapper around the pallet_assets, offering a streamlined -/// interface for interacting with fungible assets. The goal is to provide a simplified, consistent -/// API that adheres to standards in the smart contract space. +//! The fungibles pallet offers a streamlined interface for interacting with fungible assets. The +//! goal is to provide a simplified, consistent API that adheres to standards in the smart contract +//! space. + +use frame_support::traits::fungibles::{metadata::Inspect as MetadataInspect, Inspect}; +pub use pallet::*; +use pallet_assets::WeightInfo as AssetsWeightInfoTrait; +use weights::WeightInfo; #[cfg(feature = "runtime-benchmarks")] mod benchmarking; @@ -8,11 +13,6 @@ mod benchmarking; mod tests; pub mod weights; -use frame_support::traits::fungibles::{metadata::Inspect as MetadataInspect, Inspect}; -pub use pallet::*; -use pallet_assets::WeightInfo as AssetsWeightInfoTrait; -use weights::WeightInfo; - type AccountIdOf = ::AccountId; type AssetIdOf = > as Inspect< ::AccountId, @@ -41,42 +41,42 @@ pub mod pallet { }; use sp_std::vec::Vec; - /// State reads for the fungibles api with required input. + /// State reads for the fungibles API with required input. #[derive(Encode, Decode, Debug, MaxEncodedLen)] #[repr(u8)] #[allow(clippy::unnecessary_cast)] pub enum Read { - /// Total token supply for a given asset ID. + /// Total token supply for a specified asset. #[codec(index = 0)] TotalSupply(AssetIdOf), - /// Account balance for a given asset ID. + /// Account balance for a specified `asset` and `owner`. #[codec(index = 1)] BalanceOf { - /// The asset ID. - id: AssetIdOf, - /// The account ID of the owner. + /// The asset. + asset: AssetIdOf, + /// The owner of the asset. owner: AccountIdOf, }, - /// Allowance for a spender approved by an owner, for a given asset ID. + /// Allowance for a `spender` approved by an `owner`, for a specified `asset`. #[codec(index = 2)] Allowance { - /// The asset ID. - id: AssetIdOf, - /// The account ID of the owner. + /// The asset. + asset: AssetIdOf, + /// The owner of the asset. owner: AccountIdOf, - /// The account ID of the spender. + /// The spender with an allowance. spender: AccountIdOf, }, - /// Token name for a given asset ID. + /// Name of the specified asset. #[codec(index = 8)] TokenName(AssetIdOf), - /// Token symbol for a given asset ID. + /// Symbol for the specified asset. #[codec(index = 9)] TokenSymbol(AssetIdOf), - /// Token decimals for a given asset ID. + /// Decimals for the specified asset. #[codec(index = 10)] TokenDecimals(AssetIdOf), - /// Check if token with a given asset ID exists. + /// Check if a specified asset exists. #[codec(index = 18)] AssetExists(AssetIdOf), } @@ -101,62 +101,61 @@ pub mod pallet { pub enum Event { /// Event emitted when allowance by `owner` to `spender` changes. Approval { - /// The ID of the asset. - id: AssetIdOf, - /// Account providing allowance. + /// The asset. + asset: AssetIdOf, + /// The owner providing the allowance. owner: AccountIdOf, - /// Allowance beneficiary. + /// The beneficiary of the allowance. spender: AccountIdOf, - /// New allowance amount. + /// The new allowance amount. value: BalanceOf, }, - /// Event emitted when transfer of tokens occurs. + /// Event emitted when an asset transfer occurs. Transfer { - /// The ID of the asset. - id: AssetIdOf, - /// Transfer sender. `None` in case of minting new tokens. + /// The asset. + asset: AssetIdOf, + /// The source of the transfer. `None` when minting. from: Option>, - /// Transfer recipient. `None` in case of burning tokens. + /// The recipient of the transfer. `None` when burning. to: Option>, - /// Amount of tokens transferred (or minted/burned). + /// The amount transferred (or minted/burned). value: BalanceOf, }, - /// Event emitted when a token class is created. + /// Event emitted when an asset is created. Create { - /// The ID of the asset. + /// The asset identifier. id: AssetIdOf, - /// Creator of the asset. + /// The creator of the asset. creator: AccountIdOf, - /// Admin of the asset. + /// The administrator of the asset. admin: AccountIdOf, }, } #[pallet::call] impl Pallet { - /// Transfers `value` amount of tokens from the caller's account to account `to`, with additional - /// `data` in unspecified format. + /// Transfers `value` amount of tokens from the caller's account to account `to`. /// /// # Parameters - /// - `id` - The ID of the asset. + /// - `asset` - The asset to transfer. /// - `to` - The recipient account. /// - `value` - The number of tokens to transfer. #[pallet::call_index(3)] #[pallet::weight(AssetsWeightInfoOf::::transfer_keep_alive())] pub fn transfer( origin: OriginFor, - id: AssetIdOf, + asset: AssetIdOf, to: AccountIdOf, value: BalanceOf, ) -> DispatchResult { + let from = ensure_signed(origin.clone())?; AssetsOf::::transfer_keep_alive( - origin.clone(), - id.clone().into(), + origin, + asset.clone().into(), T::Lookup::unlookup(to.clone()), value, )?; - let from = ensure_signed(origin)?; - Self::deposit_event(Event::Transfer { id, from: Some(from), to: Some(to), value }); + Self::deposit_event(Event::Transfer { asset, from: Some(from), to: Some(to), value }); Ok(()) } @@ -164,7 +163,7 @@ pub mod pallet { /// in unspecified format. /// /// # Parameters - /// - `id` - The ID of the asset. + /// - `asset` - The asset to transfer. /// - `from` - The account from which the asset balance will be withdrawn. /// - `to` - The recipient account. /// - `value` - The number of tokens to transfer. @@ -172,39 +171,39 @@ pub mod pallet { #[pallet::weight(AssetsWeightInfoOf::::transfer_approved())] pub fn transfer_from( origin: OriginFor, - id: AssetIdOf, + asset: AssetIdOf, from: AccountIdOf, to: AccountIdOf, value: BalanceOf, ) -> DispatchResult { AssetsOf::::transfer_approved( origin, - id.clone().into(), + asset.clone().into(), T::Lookup::unlookup(from.clone()), T::Lookup::unlookup(to.clone()), value, )?; - Self::deposit_event(Event::Transfer { id, from: Some(from), to: Some(to), value }); + Self::deposit_event(Event::Transfer { asset, from: Some(from), to: Some(to), value }); Ok(()) } /// Approves an account to spend a specified number of tokens on behalf of the caller. /// /// # Parameters - /// - `id` - The ID of the asset. + /// - `asset` - The asset to approve. /// - `spender` - The account that is allowed to spend the tokens. /// - `value` - The number of tokens to approve. #[pallet::call_index(5)] #[pallet::weight(::WeightInfo::approve(1, 1))] pub fn approve( origin: OriginFor, - id: AssetIdOf, + asset: AssetIdOf, spender: AccountIdOf, value: BalanceOf, ) -> DispatchResultWithPostInfo { let owner = ensure_signed(origin.clone()) .map_err(|e| e.with_weight(Self::weight_approve(0, 0)))?; - let current_allowance = AssetsOf::::allowance(id.clone(), &owner, &spender); + let current_allowance = AssetsOf::::allowance(asset.clone(), &owner, &spender); let weight = match value.cmp(¤t_allowance) { // If the new value is equal to the current allowance, do nothing. @@ -214,7 +213,7 @@ pub mod pallet { Greater => { AssetsOf::::approve_transfer( origin, - id.clone().into(), + asset.clone().into(), T::Lookup::unlookup(spender.clone()), value.saturating_sub(current_allowance), ) @@ -224,37 +223,42 @@ pub mod pallet { // If the new value is less than the current allowance, cancel the approval and // set the new value. Less => { - let id_param: AssetIdParameterOf = id.clone().into(); + let asset_param: AssetIdParameterOf = asset.clone().into(); let spender_source = T::Lookup::unlookup(spender.clone()); AssetsOf::::cancel_approval( origin.clone(), - id_param.clone(), + asset_param.clone(), spender_source.clone(), ) .map_err(|e| e.with_weight(Self::weight_approve(0, 1)))?; if value.is_zero() { Self::weight_approve(0, 1) } else { - AssetsOf::::approve_transfer(origin, id_param, spender_source, value)?; + AssetsOf::::approve_transfer( + origin, + asset_param, + spender_source, + value, + )?; Self::weight_approve(1, 1) } }, }; - Self::deposit_event(Event::Approval { id, owner, spender, value }); + Self::deposit_event(Event::Approval { asset, owner, spender, value }); Ok(Some(weight).into()) } - /// Increases the allowance of a spender. + /// Increases the allowance of a spender and asset. /// /// # Parameters - /// - `id` - The ID of the asset. + /// - `asset` - The asset to have an allowance increased. /// - `spender` - The account that is allowed to spend the tokens. /// - `value` - The number of tokens to increase the allowance by. #[pallet::call_index(6)] #[pallet::weight(::WeightInfo::approve(1, 0))] pub fn increase_allowance( origin: OriginFor, - id: AssetIdOf, + asset: AssetIdOf, spender: AccountIdOf, value: BalanceOf, ) -> DispatchResultWithPostInfo { @@ -262,27 +266,27 @@ pub mod pallet { .map_err(|e| e.with_weight(Self::weight_approve(0, 0)))?; AssetsOf::::approve_transfer( origin, - id.clone().into(), + asset.clone().into(), T::Lookup::unlookup(spender.clone()), value, ) .map_err(|e| e.with_weight(AssetsWeightInfoOf::::approve_transfer()))?; - let value = AssetsOf::::allowance(id.clone(), &owner, &spender); - Self::deposit_event(Event::Approval { id, owner, spender, value }); + let value = AssetsOf::::allowance(asset.clone(), &owner, &spender); + Self::deposit_event(Event::Approval { asset, owner, spender, value }); Ok(().into()) } - /// Decreases the allowance of a spender. + /// Decreases the allowance of a spender and asset. /// /// # Parameters - /// - `id` - The ID of the asset. + /// - `asset` - The asset to have an allowance decreased. /// - `spender` - The account that is allowed to spend the tokens. /// - `value` - The number of tokens to decrease the allowance by. #[pallet::call_index(7)] #[pallet::weight(::WeightInfo::approve(1, 1))] pub fn decrease_allowance( origin: OriginFor, - id: AssetIdOf, + asset: AssetIdOf, spender: AccountIdOf, value: BalanceOf, ) -> DispatchResultWithPostInfo { @@ -291,14 +295,14 @@ pub mod pallet { if value.is_zero() { return Ok(Some(Self::weight_approve(0, 0)).into()); } - let current_allowance = AssetsOf::::allowance(id.clone(), &owner, &spender); + let current_allowance = AssetsOf::::allowance(asset.clone(), &owner, &spender); let spender_source = T::Lookup::unlookup(spender.clone()); - let id_param: AssetIdParameterOf = id.clone().into(); + let asset_param: AssetIdParameterOf = asset.clone().into(); // Cancel the approval and set the new value if `new_allowance` is more than zero. AssetsOf::::cancel_approval( origin.clone(), - id_param.clone(), + asset_param.clone(), spender_source.clone(), ) .map_err(|e| e.with_weight(Self::weight_approve(0, 1)))?; @@ -306,17 +310,22 @@ pub mod pallet { let weight = if new_allowance.is_zero() { Self::weight_approve(0, 1) } else { - AssetsOf::::approve_transfer(origin, id_param, spender_source, new_allowance)?; + AssetsOf::::approve_transfer( + origin, + asset_param, + spender_source, + new_allowance, + )?; Self::weight_approve(1, 1) }; - Self::deposit_event(Event::Approval { id, owner, spender, value: new_allowance }); + Self::deposit_event(Event::Approval { asset, owner, spender, value: new_allowance }); Ok(Some(weight).into()) } - /// Create a new token with a given asset ID. + /// Create a new token with a given identifier. /// /// # Parameters - /// - `id` - The ID of the asset. + /// - `id` - The identifier of the asset. /// - `admin` - The account that will administer the asset. /// - `min_balance` - The minimum balance required for accounts holding this asset. #[pallet::call_index(11)] @@ -338,92 +347,90 @@ pub mod pallet { Ok(()) } - /// Start the process of destroying a token with a given asset ID. + /// Start the process of destroying a token. /// /// # Parameters - /// - `id` - The ID of the asset. + /// - `asset` - The asset to be destroyed. #[pallet::call_index(12)] #[pallet::weight(AssetsWeightInfoOf::::start_destroy())] - pub fn start_destroy(origin: OriginFor, id: AssetIdOf) -> DispatchResult { - AssetsOf::::start_destroy(origin, id.into()) + pub fn start_destroy(origin: OriginFor, asset: AssetIdOf) -> DispatchResult { + AssetsOf::::start_destroy(origin, asset.into()) } - /// Set the metadata for a token with a given asset ID. + /// Set the metadata for a token. /// /// # Parameters - /// - `id`: The identifier of the asset to update. - /// - `name`: The user friendly name of this asset. Limited in length by - /// `pallet_assets::Config::StringLimit`. - /// - `symbol`: The exchange symbol for this asset. Limited in length by - /// `pallet_assets::Config::StringLimit`. + /// - `asset`: The asset to update. + /// - `name`: The user friendly name of this asset. + /// - `symbol`: The exchange symbol for this asset. /// - `decimals`: The number of decimals this asset uses to represent one unit. #[pallet::call_index(16)] #[pallet::weight(AssetsWeightInfoOf::::set_metadata(name.len() as u32, symbol.len() as u32))] pub fn set_metadata( origin: OriginFor, - id: AssetIdOf, + asset: AssetIdOf, name: Vec, symbol: Vec, decimals: u8, ) -> DispatchResult { - AssetsOf::::set_metadata(origin, id.into(), name, symbol, decimals) + AssetsOf::::set_metadata(origin, asset.into(), name, symbol, decimals) } - /// Clear the metadata for a token with a given asset ID. + /// Clear the metadata for a token. /// /// # Parameters - /// - `id` - The ID of the asset. + /// - `asset` - The asset to update. #[pallet::call_index(17)] #[pallet::weight(AssetsWeightInfoOf::::clear_metadata())] - pub fn clear_metadata(origin: OriginFor, id: AssetIdOf) -> DispatchResult { - AssetsOf::::clear_metadata(origin, id.into()) + pub fn clear_metadata(origin: OriginFor, asset: AssetIdOf) -> DispatchResult { + AssetsOf::::clear_metadata(origin, asset.into()) } /// Creates `value` amount of tokens and assigns them to `account`, increasing the total supply. /// /// # Parameters - /// - `id` - The ID of the asset. + /// - `asset` - The asset to mint. /// - `account` - The account to be credited with the created tokens. /// - `value` - The number of tokens to mint. #[pallet::call_index(19)] #[pallet::weight(AssetsWeightInfoOf::::mint())] pub fn mint( origin: OriginFor, - id: AssetIdOf, + asset: AssetIdOf, account: AccountIdOf, value: BalanceOf, ) -> DispatchResult { AssetsOf::::mint( origin, - id.clone().into(), + asset.clone().into(), T::Lookup::unlookup(account.clone()), value, )?; - Self::deposit_event(Event::Transfer { id, from: None, to: Some(account), value }); + Self::deposit_event(Event::Transfer { asset, from: None, to: Some(account), value }); Ok(()) } /// Destroys `value` amount of tokens from `account`, reducing the total supply. /// /// # Parameters - /// - `id` - The ID of the asset. + /// - `asset` - the asset to burn. /// - `account` - The account from which the tokens will be destroyed. /// - `value` - The number of tokens to destroy. #[pallet::call_index(20)] #[pallet::weight(AssetsWeightInfoOf::::burn())] pub fn burn( origin: OriginFor, - id: AssetIdOf, + asset: AssetIdOf, account: AccountIdOf, value: BalanceOf, ) -> DispatchResult { AssetsOf::::burn( origin, - id.clone().into(), + asset.clone().into(), T::Lookup::unlookup(account.clone()), value, )?; - Self::deposit_event(Event::Transfer { id, from: Some(account), to: None, value }); + Self::deposit_event(Event::Transfer { asset, from: Some(account), to: None, value }); Ok(()) } } @@ -441,25 +448,25 @@ pub mod pallet { use Read::*; match value { - TotalSupply(id) => AssetsOf::::total_supply(id).encode(), - BalanceOf { id, owner } => AssetsOf::::balance(id, owner).encode(), - Allowance { id, owner, spender } => { - AssetsOf::::allowance(id, &owner, &spender).encode() + TotalSupply(asset) => AssetsOf::::total_supply(asset).encode(), + BalanceOf { asset, owner } => AssetsOf::::balance(asset, owner).encode(), + Allowance { asset, owner, spender } => { + AssetsOf::::allowance(asset, &owner, &spender).encode() }, - TokenName(id) => { - as MetadataInspect>>::name(id).encode() + TokenName(asset) => { + as MetadataInspect>>::name(asset).encode() }, - TokenSymbol(id) => { - as MetadataInspect>>::symbol(id).encode() + TokenSymbol(asset) => { + as MetadataInspect>>::symbol(asset).encode() }, - TokenDecimals(id) => { - as MetadataInspect>>::decimals(id).encode() + TokenDecimals(asset) => { + as MetadataInspect>>::decimals(asset).encode() }, - AssetExists(id) => AssetsOf::::asset_exists(id).encode(), + AssetExists(asset) => AssetsOf::::asset_exists(asset).encode(), } } - pub fn weight_approve(approve: u32, cancel: u32) -> Weight { + fn weight_approve(approve: u32, cancel: u32) -> Weight { ::WeightInfo::approve(cancel, approve) } } diff --git a/pallets/api/src/fungibles/tests.rs b/pallets/api/src/fungibles/tests.rs index ca2a85c9..9610139e 100644 --- a/pallets/api/src/fungibles/tests.rs +++ b/pallets/api/src/fungibles/tests.rs @@ -16,16 +16,16 @@ type Event = crate::fungibles::Event; fn transfer_works() { new_test_ext().execute_with(|| { let value: Balance = 100 * UNIT; - let id = ASSET; + let asset = ASSET; let from = Some(ALICE); let to = Some(BOB); - create_asset_and_mint_to(ALICE, id, ALICE, value * 2); - let balance_before_transfer = Assets::balance(id, &BOB); - assert_ok!(Fungibles::transfer(signed(ALICE), id, BOB, value)); - let balance_after_transfer = Assets::balance(id, &BOB); + create_asset_and_mint_to(ALICE, asset, ALICE, value * 2); + let balance_before_transfer = Assets::balance(asset, &BOB); + assert_ok!(Fungibles::transfer(signed(ALICE), asset, BOB, value)); + let balance_after_transfer = Assets::balance(asset, &BOB); assert_eq!(balance_after_transfer, balance_before_transfer + value); - System::assert_last_event(Event::Transfer { id, from, to, value }.into()); + System::assert_last_event(Event::Transfer { asset, from, to, value }.into()); }); } @@ -33,22 +33,22 @@ fn transfer_works() { fn transfer_from_works() { new_test_ext().execute_with(|| { let value: Balance = 100 * UNIT; - let id = ASSET; + let asset = ASSET; let from = Some(ALICE); let to = Some(BOB); // Approve CHARLIE to transfer up to `value` to BOB. - create_asset_mint_and_approve(ALICE, id, ALICE, value * 2, CHARLIE, value); + create_asset_mint_and_approve(ALICE, asset, ALICE, value * 2, CHARLIE, value); // Successfully call transfer from. - let alice_balance_before_transfer = Assets::balance(id, &ALICE); - let bob_balance_before_transfer = Assets::balance(id, &BOB); - assert_ok!(Fungibles::transfer_from(signed(CHARLIE), id, ALICE, BOB, value)); - let alice_balance_after_transfer = Assets::balance(id, &ALICE); - let bob_balance_after_transfer = Assets::balance(id, &BOB); + let alice_balance_before_transfer = Assets::balance(asset, &ALICE); + let bob_balance_before_transfer = Assets::balance(asset, &BOB); + assert_ok!(Fungibles::transfer_from(signed(CHARLIE), asset, ALICE, BOB, value)); + let alice_balance_after_transfer = Assets::balance(asset, &ALICE); + let bob_balance_after_transfer = Assets::balance(asset, &BOB); // Check that BOB receives the `value` and ALICE `amount` is spent successfully by CHARLIE. assert_eq!(bob_balance_after_transfer, bob_balance_before_transfer + value); assert_eq!(alice_balance_after_transfer, alice_balance_before_transfer - value); - System::assert_last_event(Event::Transfer { id, from, to, value }.into()); + System::assert_last_event(Event::Transfer { asset, from, to, value }.into()); }); } @@ -57,31 +57,37 @@ fn transfer_from_works() { fn approve_works() { new_test_ext().execute_with(|| { let value: Balance = 100 * UNIT; - let id = ASSET; + let asset = ASSET; let owner = ALICE; let spender = BOB; - create_asset_and_mint_to(ALICE, id, ALICE, value); - assert_eq!(0, Assets::allowance(id, &ALICE, &BOB)); - assert_ok!(Fungibles::approve(signed(ALICE), id, BOB, value)); - assert_eq!(Assets::allowance(id, &ALICE, &BOB), value); - System::assert_last_event(Event::Approval { id, owner, spender, value }.into()); + create_asset_and_mint_to(ALICE, asset, ALICE, value); + assert_eq!(0, Assets::allowance(asset, &ALICE, &BOB)); + assert_ok!(Fungibles::approve(signed(ALICE), asset, BOB, value)); + assert_eq!(Assets::allowance(asset, &ALICE, &BOB), value); + System::assert_last_event(Event::Approval { asset, owner, spender, value }.into()); // Approves an value to spend that is lower than the current allowance. - assert_ok!(Fungibles::approve(signed(ALICE), id, BOB, value / 2)); - assert_eq!(Assets::allowance(id, &ALICE, &BOB), value / 2); - System::assert_last_event(Event::Approval { id, owner, spender, value: value / 2 }.into()); + assert_ok!(Fungibles::approve(signed(ALICE), asset, BOB, value / 2)); + assert_eq!(Assets::allowance(asset, &ALICE, &BOB), value / 2); + System::assert_last_event( + Event::Approval { asset, owner, spender, value: value / 2 }.into(), + ); // Approves an value to spend that is higher than the current allowance. - assert_ok!(Fungibles::approve(signed(ALICE), id, BOB, value * 2)); - assert_eq!(Assets::allowance(id, &ALICE, &BOB), value * 2); - System::assert_last_event(Event::Approval { id, owner, spender, value: value * 2 }.into()); + assert_ok!(Fungibles::approve(signed(ALICE), asset, BOB, value * 2)); + assert_eq!(Assets::allowance(asset, &ALICE, &BOB), value * 2); + System::assert_last_event( + Event::Approval { asset, owner, spender, value: value * 2 }.into(), + ); // Approves an value to spend that is equal to the current allowance. - assert_ok!(Fungibles::approve(signed(ALICE), id, BOB, value * 2)); - assert_eq!(Assets::allowance(id, &ALICE, &BOB), value * 2); - System::assert_last_event(Event::Approval { id, owner, spender, value: value * 2 }.into()); + assert_ok!(Fungibles::approve(signed(ALICE), asset, BOB, value * 2)); + assert_eq!(Assets::allowance(asset, &ALICE, &BOB), value * 2); + System::assert_last_event( + Event::Approval { asset, owner, spender, value: value * 2 }.into(), + ); // Sets allowance to zero. - assert_ok!(Fungibles::approve(signed(ALICE), id, BOB, 0)); - assert_eq!(Assets::allowance(id, &ALICE, &BOB), 0); - System::assert_last_event(Event::Approval { id, owner, spender, value: 0 }.into()); + assert_ok!(Fungibles::approve(signed(ALICE), asset, BOB, 0)); + assert_eq!(Assets::allowance(asset, &ALICE, &BOB), 0); + System::assert_last_event(Event::Approval { asset, owner, spender, value: 0 }.into()); }); } @@ -89,19 +95,21 @@ fn approve_works() { fn increase_allowance_works() { new_test_ext().execute_with(|| { let value: Balance = 100 * UNIT; - let id = ASSET; + let asset = ASSET; let owner = ALICE; let spender = BOB; - create_asset_and_mint_to(ALICE, id, ALICE, value); - assert_eq!(0, Assets::allowance(id, &ALICE, &BOB)); - assert_ok!(Fungibles::increase_allowance(signed(ALICE), id, BOB, value)); - assert_eq!(Assets::allowance(id, &ALICE, &BOB), value); - System::assert_last_event(Event::Approval { id, owner, spender, value }.into()); + create_asset_and_mint_to(ALICE, asset, ALICE, value); + assert_eq!(0, Assets::allowance(asset, &ALICE, &BOB)); + assert_ok!(Fungibles::increase_allowance(signed(ALICE), asset, BOB, value)); + assert_eq!(Assets::allowance(asset, &ALICE, &BOB), value); + System::assert_last_event(Event::Approval { asset, owner, spender, value }.into()); // Additive. - assert_ok!(Fungibles::increase_allowance(signed(ALICE), id, BOB, value)); - assert_eq!(Assets::allowance(id, &ALICE, &BOB), value * 2); - System::assert_last_event(Event::Approval { id, owner, spender, value: value * 2 }.into()); + assert_ok!(Fungibles::increase_allowance(signed(ALICE), asset, BOB, value)); + assert_eq!(Assets::allowance(asset, &ALICE, &BOB), value * 2); + System::assert_last_event( + Event::Approval { asset, owner, spender, value: value * 2 }.into(), + ); }); } @@ -109,23 +117,25 @@ fn increase_allowance_works() { fn decrease_allowance_works() { new_test_ext().execute_with(|| { let value: Balance = 100 * UNIT; - let id = ASSET; + let asset = ASSET; let owner = ALICE; let spender = BOB; - create_asset_mint_and_approve(ALICE, id, ALICE, value, BOB, value); - assert_eq!(Assets::allowance(id, &ALICE, &BOB), value); + create_asset_mint_and_approve(ALICE, asset, ALICE, value, BOB, value); + assert_eq!(Assets::allowance(asset, &ALICE, &BOB), value); // Owner balance is not changed if decreased by zero. - assert_ok!(Fungibles::decrease_allowance(signed(ALICE), id, BOB, 0)); - assert_eq!(Assets::allowance(id, &ALICE, &BOB), value); + assert_ok!(Fungibles::decrease_allowance(signed(ALICE), asset, BOB, 0)); + assert_eq!(Assets::allowance(asset, &ALICE, &BOB), value); // Decrease allowance successfully. - assert_ok!(Fungibles::decrease_allowance(signed(ALICE), id, BOB, value / 2)); - assert_eq!(Assets::allowance(id, &ALICE, &BOB), value / 2); - System::assert_last_event(Event::Approval { id, owner, spender, value: value / 2 }.into()); + assert_ok!(Fungibles::decrease_allowance(signed(ALICE), asset, BOB, value / 2)); + assert_eq!(Assets::allowance(asset, &ALICE, &BOB), value / 2); + System::assert_last_event( + Event::Approval { asset, owner, spender, value: value / 2 }.into(), + ); // Saturating if current allowance is decreased more than the owner balance. - assert_ok!(Fungibles::decrease_allowance(signed(ALICE), id, BOB, value)); - assert_eq!(Assets::allowance(id, &ALICE, &BOB), 0); - System::assert_last_event(Event::Approval { id, owner, spender, value: 0 }.into()); + assert_ok!(Fungibles::decrease_allowance(signed(ALICE), asset, BOB, value)); + assert_eq!(Assets::allowance(asset, &ALICE, &BOB), 0); + System::assert_last_event(Event::Approval { asset, owner, spender, value: 0 }.into()); }); } @@ -146,45 +156,45 @@ fn create_works() { #[test] fn start_destroy_works() { new_test_ext().execute_with(|| { - let id = ASSET; + let asset = ASSET; - create_asset(ALICE, id); - assert_ok!(Fungibles::start_destroy(signed(ALICE), id)); + create_asset(ALICE, asset); + assert_ok!(Fungibles::start_destroy(signed(ALICE), asset)); }); } #[test] fn set_metadata_works() { new_test_ext().execute_with(|| { - let id = ASSET; + let asset = ASSET; let name = vec![42]; let symbol = vec![42]; let decimals = 42; - create_asset(ALICE, id); + create_asset(ALICE, asset); assert_ok!(Fungibles::set_metadata( signed(ALICE), - id, + asset, name.clone(), symbol.clone(), decimals )); - assert_eq!(Assets::name(id), name); - assert_eq!(Assets::symbol(id), symbol); - assert_eq!(Assets::decimals(id), decimals); + assert_eq!(Assets::name(asset), name); + assert_eq!(Assets::symbol(asset), symbol); + assert_eq!(Assets::decimals(asset), decimals); }); } #[test] fn clear_metadata_works() { new_test_ext().execute_with(|| { - let id = ASSET; + let asset = ASSET; - create_asset_and_set_metadata(ALICE, id, vec![42], vec![42], 42); - assert_ok!(Fungibles::clear_metadata(signed(ALICE), id)); - assert!(Assets::name(id).is_empty()); - assert!(Assets::symbol(id).is_empty()); - assert!(Assets::decimals(id).is_zero()); + create_asset_and_set_metadata(ALICE, asset, vec![42], vec![42], 42); + assert_ok!(Fungibles::clear_metadata(signed(ALICE), asset)); + assert!(Assets::name(asset).is_empty()); + assert!(Assets::symbol(asset).is_empty()); + assert!(Assets::decimals(asset).is_zero()); }); } @@ -192,16 +202,16 @@ fn clear_metadata_works() { fn mint_works() { new_test_ext().execute_with(|| { let value: Balance = 100 * UNIT; - let id = ASSET; + let asset = ASSET; let from = None; let to = Some(BOB); - create_asset(ALICE, id); - let balance_before_mint = Assets::balance(id, &BOB); - assert_ok!(Fungibles::mint(signed(ALICE), id, BOB, value)); - let balance_after_mint = Assets::balance(id, &BOB); + create_asset(ALICE, asset); + let balance_before_mint = Assets::balance(asset, &BOB); + assert_ok!(Fungibles::mint(signed(ALICE), asset, BOB, value)); + let balance_after_mint = Assets::balance(asset, &BOB); assert_eq!(balance_after_mint, balance_before_mint + value); - System::assert_last_event(Event::Transfer { id, from, to, value }.into()); + System::assert_last_event(Event::Transfer { asset, from, to, value }.into()); }); } @@ -209,16 +219,16 @@ fn mint_works() { fn burn_works() { new_test_ext().execute_with(|| { let value: Balance = 100 * UNIT; - let id = ASSET; + let asset = ASSET; let from = Some(BOB); let to = None; - create_asset_and_mint_to(ALICE, id, BOB, value); - let balance_before_burn = Assets::balance(id, &BOB); - assert_ok!(Fungibles::burn(signed(ALICE), id, BOB, value)); - let balance_after_burn = Assets::balance(id, &BOB); + create_asset_and_mint_to(ALICE, asset, BOB, value); + let balance_before_burn = Assets::balance(asset, &BOB); + assert_ok!(Fungibles::burn(signed(ALICE), asset, BOB, value)); + let balance_after_burn = Assets::balance(asset, &BOB); assert_eq!(balance_after_burn, balance_before_burn - value); - System::assert_last_event(Event::Transfer { id, from, to, value }.into()); + System::assert_last_event(Event::Transfer { asset, from, to, value }.into()); }); } @@ -236,7 +246,7 @@ fn balance_of_works() { create_asset_and_mint_to(ALICE, ASSET, ALICE, 100); assert_eq!( Assets::balance(ASSET, ALICE).encode(), - Fungibles::read_state(BalanceOf { id: ASSET, owner: ALICE }) + Fungibles::read_state(BalanceOf { asset: ASSET, owner: ALICE }) ); }); } @@ -247,7 +257,7 @@ fn allowance_works() { create_asset_mint_and_approve(ALICE, ASSET, BOB, 100, ALICE, 50); assert_eq!( Assets::allowance(ASSET, &ALICE, &BOB).encode(), - Fungibles::read_state(Allowance { id: ASSET, owner: ALICE, spender: BOB }) + Fungibles::read_state(Allowance { asset: ASSET, owner: ALICE, spender: BOB }) ); }); } @@ -277,48 +287,48 @@ fn signed(account: AccountId) -> RuntimeOrigin { RuntimeOrigin::signed(account) } -fn create_asset(owner: AccountId, asset_id: AssetId) { - assert_ok!(Assets::create(signed(owner), asset_id, owner, 1)); +fn create_asset(owner: AccountId, asset_asset: AssetId) { + assert_ok!(Assets::create(signed(owner), asset_asset, owner, 1)); } -fn mint_asset(owner: AccountId, asset_id: AssetId, to: AccountId, value: Balance) { - assert_ok!(Assets::mint(signed(owner), asset_id, to, value)); +fn mint_asset(owner: AccountId, asset_asset: AssetId, to: AccountId, value: Balance) { + assert_ok!(Assets::mint(signed(owner), asset_asset, to, value)); } -fn create_asset_and_mint_to(owner: AccountId, asset_id: AssetId, to: AccountId, value: Balance) { - create_asset(owner, asset_id); - mint_asset(owner, asset_id, to, value) +fn create_asset_and_mint_to(owner: AccountId, asset_asset: AssetId, to: AccountId, value: Balance) { + create_asset(owner, asset_asset); + mint_asset(owner, asset_asset, to, value) } fn create_asset_mint_and_approve( owner: AccountId, - asset_id: AssetId, + asset_asset: AssetId, to: AccountId, mint: Balance, spender: AccountId, approve: Balance, ) { - create_asset_and_mint_to(owner, asset_id, to, mint); - assert_ok!(Assets::approve_transfer(signed(to), asset_id, spender, approve,)); + create_asset_and_mint_to(owner, asset_asset, to, mint); + assert_ok!(Assets::approve_transfer(signed(to), asset_asset, spender, approve,)); } fn create_asset_and_set_metadata( owner: AccountId, - asset_id: AssetId, + asset_asset: AssetId, name: Vec, symbol: Vec, decimals: u8, ) { - assert_ok!(Assets::create(signed(owner), asset_id, owner, 100)); - set_metadata_asset(owner, asset_id, name, symbol, decimals); + assert_ok!(Assets::create(signed(owner), asset_asset, owner, 100)); + set_metadata_asset(owner, asset_asset, name, symbol, decimals); } fn set_metadata_asset( owner: AccountId, - asset_id: AssetId, + asset_asset: AssetId, name: Vec, symbol: Vec, decimals: u8, ) { - assert_ok!(Assets::set_metadata(signed(owner), asset_id, name, symbol, decimals)); + assert_ok!(Assets::set_metadata(signed(owner), asset_asset, name, symbol, decimals)); } From 38c35e5e51333a158f9a5ef5e2885c67cf720079 Mon Sep 17 00:00:00 2001 From: Daanvdplas Date: Sat, 17 Aug 2024 16:45:46 +0200 Subject: [PATCH 2/3] tests: improve test cases --- extension/src/lib.rs | 42 ++-- extension/src/tests.rs | 137 +++++++++++-- extension/src/v0.rs | 430 ++++++++++++++++++----------------------- 3 files changed, 321 insertions(+), 288 deletions(-) diff --git a/extension/src/lib.rs b/extension/src/lib.rs index 650f73f8..3fb46b37 100644 --- a/extension/src/lib.rs +++ b/extension/src/lib.rs @@ -18,16 +18,12 @@ use sp_std::vec::Vec; mod tests; mod v0; -/// Logging target for categorizing messages from the Pop API extension module. +// Logging target for categorizing messages from the Pop API extension module. const LOG_TARGET: &str = "pop-api::extension"; const DECODING_FAILED_ERROR: DispatchError = DispatchError::Other("DecodingFailed"); -// TODO: issue #93, we can also encode the `pop_primitives::Error::UnknownCall` which means we do use -// `Error` in the runtime and it should stay in primitives. Perhaps issue #91 will also influence -// here. Should be looked at together. const DECODING_FAILED_ERROR_ENCODED: [u8; 4] = [255u8, 0, 0, 0]; const UNKNOWN_CALL_ERROR: DispatchError = DispatchError::Other("UnknownCall"); -// TODO: see above. const UNKNOWN_CALL_ERROR_ENCODED: [u8; 4] = [254u8, 0, 0, 0]; type ContractSchedule = ::Schedule; @@ -248,7 +244,7 @@ enum VersionedDispatch { /// The `FuncId` specifies the available functions that can be called through the Pop API. Each /// variant corresponds to a specific functionality provided by the API, facilitating the /// interaction between smart contracts and the runtime. -#[derive(Debug)] +#[derive(Debug, PartialEq, Eq)] pub enum FuncId { /// Represents a function call to dispatch a runtime call. Dispatch, @@ -287,8 +283,17 @@ impl TryFrom for FuncId { /// /// - `error`: The `DispatchError` encountered during contract execution. /// - `version`: The version of the chain extension, used to determine the known errors. -pub(crate) fn convert_to_status_code(error: DispatchError, version: u8) -> u32 { - let mut encoded_error: [u8; 4] = match error { +fn convert_to_status_code(error: DispatchError, version: u8) -> u32 { + let mut encoded_error: [u8; 4] = encode_error(error); + match version { + 0 => v0::handle_unknown_error(&mut encoded_error), + _ => encoded_error = UNKNOWN_CALL_ERROR_ENCODED, + } + u32::from_le_bytes(encoded_error) +} + +fn encode_error(error: DispatchError) -> [u8; 4] { + match error { // "UnknownCall" and "DecodingFailed" are mapped to specific errors in the API and will // never change. UNKNOWN_CALL_ERROR => UNKNOWN_CALL_ERROR_ENCODED, @@ -299,26 +304,5 @@ pub(crate) fn convert_to_status_code(error: DispatchError, version: u8) -> u32 { encoded_error.resize(4, 0); encoded_error.try_into().expect("qed, resized to 4 bytes line above") }, - }; - match version { - // If an unknown variant of the `DispatchError` is detected the error needs to be converted - // into the encoded value of `Error::Other`. This conversion is performed by shifting the bytes one - // position forward (discarding the last byte as it is not used) and setting the first byte to the - // encoded value of `Other` (0u8). This ensures the error is correctly categorized as an `Other` - // variant which provides all the necessary information to debug which error occurred in the runtime. - // - // Byte layout explanation: - // - Byte 0: index of the variant within `Error` - // - Byte 1: - // - Must be zero for `UNIT_ERRORS`. - // - Represents the nested error in `SINGLE_NESTED_ERRORS`. - // - Represents the first level of nesting in `DOUBLE_NESTED_ERRORS`. - // - Byte 2: - // - Represents the second level of nesting in `DOUBLE_NESTED_ERRORS`. - // - Byte 3: - // - Unused or represents further nested information. - 0 => v0::handle_unknown_error(&mut encoded_error), - _ => encoded_error = UNKNOWN_CALL_ERROR_ENCODED, } - u32::from_le_bytes(encoded_error) } diff --git a/extension/src/tests.rs b/extension/src/tests.rs index a47feef9..ef10e1fa 100644 --- a/extension/src/tests.rs +++ b/extension/src/tests.rs @@ -1,7 +1,15 @@ +use super::{ + encode_error, DispatchError::*, FuncId, DECODING_FAILED_ERROR, DECODING_FAILED_ERROR_ENCODED, + UNKNOWN_CALL_ERROR, UNKNOWN_CALL_ERROR_ENCODED, +}; use codec::{Decode, Encode}; +use sp_runtime::{ + ArithmeticError::*, DispatchError, ModuleError, TokenError::*, TransactionalError::*, +}; -// Test ensuring `func_id()` and `ext_id()` work as expected, i.e. extracting the first two -// bytes and the last two bytes, respectively, from a 4 byte array. +// TODO: #110 +// Test ensuring `func_id()` and `ext_id()` work as expected. I.e. extracting the first two +// bytes and the last two bytes from a 4 byte array, respectively. #[test] fn test_byte_extraction() { use rand::Rng; @@ -42,6 +50,112 @@ fn test_byte_extraction() { } } +// Assert encoded `DispatchError` with expected encoding. +pub(crate) fn assert_encoding_matches(dispatch_error: DispatchError, expected_encoding: [u8; 4]) { + let encoding = encode_error(dispatch_error); + assert_eq!(encoding, expected_encoding); +} + +// Assert all unit error possibilities with expected encoding. +#[test] +fn encode_error_unit_variants_works() { + let test_cases = vec![ + (Other(""), [0, 0, 0, 0]), + (CannotLookup, [1, 0, 0, 0]), + (BadOrigin, [2, 0, 0, 0]), + (ConsumerRemaining, [4, 0, 0, 0]), + (NoProviders, [5, 0, 0, 0]), + (TooManyConsumers, [6, 0, 0, 0]), + (Exhausted, [10, 0, 0, 0]), + (Corruption, [11, 0, 0, 0]), + (Unavailable, [12, 0, 0, 0]), + (RootNotAllowed, [13, 0, 0, 0]), + (UNKNOWN_CALL_ERROR, UNKNOWN_CALL_ERROR_ENCODED), + (DECODING_FAILED_ERROR, DECODING_FAILED_ERROR_ENCODED), + ]; + for (dispatch_error, expected_encoding) in test_cases { + assert_encoding_matches(dispatch_error, expected_encoding); + } +} + +// Assert all single nested error possibilities with expected encoding. +#[test] +fn encode_error_single_nested_variants_works() { + // TokenError. + let test_cases = vec![ + (FundsUnavailable, 0), + (OnlyProvider, 1), + (BelowMinimum, 2), + (CannotCreate, 3), + (UnknownAsset, 4), + (Frozen, 5), + (Unsupported, 6), + (CannotCreateHold, 7), + (NotExpendable, 8), + (Blocked, 9), + ]; + for (error, index) in test_cases { + assert_encoding_matches(Token(error), [7, index, 0, 0]); + } + + // ArithmeticError. + let test_cases = vec![(Underflow, 0), (Overflow, 1), (DivisionByZero, 2)]; + for (error, index) in test_cases { + assert_encoding_matches(Arithmetic(error), [8, index, 0, 0]); + } + + // TransactionalError. + let test_cases = vec![(LimitReached, 0), (NoLayer, 1)]; + for (error, index) in test_cases { + assert_encoding_matches(Transactional(error), [9, index, 0, 0]); + } +} + +// Assert all module error possibilities with expected encoding. +#[test] +fn encode_error_module_error_works() { + let test_cases = vec![ + ( + Module(ModuleError { index: 1, error: [2, 0, 0, 0], message: Some("hallo") }), + [3, 1, 2, 0], + ), + ( + Module(ModuleError { index: 1, error: [2, 3, 0, 0], message: Some("hallo") }), + [3, 1, 2, 3], + ), + ( + Module(ModuleError { index: 1, error: [2, 3, 4, 0], message: Some("hallo") }), + [3, 1, 2, 3], + ), + ( + Module(ModuleError { index: 1, error: [2, 3, 4, 5], message: Some("hallo") }), + [3, 1, 2, 3], + ), + (Module(ModuleError { index: 1, error: [2, 3, 4, 5], message: None }), [3, 1, 2, 3]), + ]; + for (dispatch_error, expected_encoding) in test_cases { + let encoding = encode_error(dispatch_error); + assert_eq!(encoding, expected_encoding); + } +} + +#[test] +fn func_id_try_from_works() { + let test_cases = [ + (0u8, Ok(FuncId::Dispatch)), + (1, Ok(FuncId::ReadState)), + (2, Err(UNKNOWN_CALL_ERROR)), + (3, Err(UNKNOWN_CALL_ERROR)), + (100, Err(UNKNOWN_CALL_ERROR)), + (u8::MAX, Err(UNKNOWN_CALL_ERROR)), + ]; + + for (input_value, expected_result) in test_cases { + let actual_result: Result = input_value.try_into(); + assert_eq!(actual_result, expected_result, "Failed on input: {}", input_value); + } +} + // Test showing all the different type of variants and its encoding. #[test] fn encoding_of_enum() { @@ -103,31 +217,26 @@ fn encoding_of_enum() { #[test] fn encoding_decoding_dispatch_error() { - use sp_runtime::{ArithmeticError, DispatchError, ModuleError, TokenError}; - - let error = DispatchError::Module(ModuleError { - index: 255, - error: [2, 0, 0, 0], - message: Some("error message"), - }); + let error = + Module(ModuleError { index: 255, error: [2, 0, 0, 0], message: Some("error message") }); let encoded = error.encode(); let decoded = DispatchError::decode(&mut &encoded[..]).unwrap(); assert_eq!(encoded, vec![3, 255, 2, 0, 0, 0]); assert_eq!( decoded, // `message` is skipped for encoding. - DispatchError::Module(ModuleError { index: 255, error: [2, 0, 0, 0], message: None }) + Module(ModuleError { index: 255, error: [2, 0, 0, 0], message: None }) ); - // Example DispatchError::Token - let error = DispatchError::Token(TokenError::UnknownAsset); + // Example Token + let error = Token(UnknownAsset); let encoded = error.encode(); let decoded = DispatchError::decode(&mut &encoded[..]).unwrap(); assert_eq!(encoded, vec![7, 4]); assert_eq!(decoded, error); - // Example DispatchError::Arithmetic - let error = DispatchError::Arithmetic(ArithmeticError::Overflow); + // Example Arithmetic + let error = Arithmetic(Overflow); let encoded = error.encode(); let decoded = DispatchError::decode(&mut &encoded[..]).unwrap(); assert_eq!(encoded, vec![8, 1]); diff --git a/extension/src/v0.rs b/extension/src/v0.rs index 4c3536a4..8fedb763 100644 --- a/extension/src/v0.rs +++ b/extension/src/v0.rs @@ -1,28 +1,6 @@ -#[cfg(test)] -use crate::convert_to_status_code; +//! TODO -pub(crate) fn handle_unknown_error(encoded_error: &mut [u8; 4]) { - let unknown = match encoded_error[0] { - code if UNIT_ERRORS.contains(&code) => nested_errors(&encoded_error[1..], None), - // Single nested errors with a limit in their nesting. - // - // `TokenError`: has ten variants - translated to a limit of nine. - 7 => nested_errors(&encoded_error[1..], Some(9)), - // `ArithmeticError`: has 3 variants - translated to a limit of two. - 8 => nested_errors(&encoded_error[1..], Some(2)), - // `TransactionalError`: has 2 variants - translated to a limit of one. - 9 => nested_errors(&encoded_error[1..], Some(1)), - code if DOUBLE_NESTED_ERRORS.contains(&code) => nested_errors(&encoded_error[3..], None), - _ => true, - }; - if unknown { - encoded_error[..].rotate_right(1); - encoded_error[0] = 0u8; - } -} - -// Unit `Error` variants. -// (variant: index): +// Unit errors. // - CannotLookup: 1, // - BadOrigin: 2, // - ConsumerRemaining: 4, @@ -32,27 +10,63 @@ pub(crate) fn handle_unknown_error(encoded_error: &mut [u8; 4]) { // - Corruption: 11, // - Unavailable: 12, // - RootNotAllowed: 13, -// - UnknownFunctionId: 254, +// - UnknownCall: 254, // - DecodingFailed: 255, const UNIT_ERRORS: [u8; 11] = [1, 2, 4, 5, 6, 10, 11, 12, 13, 254, 255]; -#[cfg(test)] -const SINGLE_NESTED_ERRORS: [u8; 3] = [7, 8, 9]; +// Single nested errors. +const TOKEN_ERROR: u8 = 7; +const ARITHMETIC_ERROR: u8 = 8; +const TRANSACTIONAL_ERROR: u8 = 9; +// Starting indices at which specific single nested error variants are considered invalid. These +// constants define the boundary for valid error variant indices within certain `DispatchError` enums. +const INVALID_TOKEN_INDEX: u8 = 10; +const INVALID_ARITHMETIC_INDEX: u8 = 3; +const INVALID_TRANSACTIONAL_INDEX: u8 = 2; -// Double nested `Error` variants -// (variant: index): -// - Module: 3, -const DOUBLE_NESTED_ERRORS: [u8; 1] = [3]; +// Double nested error. +const MODULE_ERROR: u8 = 3; -// Checks for unknown nested errors within the `DispatchError`. -// - For single nested errors with a limit, it verifies if the nested value exceeds the limit. -// - For other nested errors, it checks if any subsequent bytes are non-zero. +// If an unknown error of the `DispatchError` is detected, the error needs to be converted +// into the encoded value of `pop_api::Error::Other`. This conversion is performed by shifting the bytes one +// position forward (discarding the last byte as it is not used) and setting the first byte to the +// encoded value of `Other` (0u8). This ensures the error is correctly categorized as an `Other` +// variant which provides all the necessary information to debug which error occurred in the runtime. // -// `nested_error` - The slice of bytes representing the nested error. -// `limit` - An optional limit for single nested errors. -fn nested_errors(nested_error: &[u8], limit: Option) -> bool { - match limit { - Some(l) => nested_error[0] > l || nested_error[1..].iter().any(|&x| x != 0u8), +// Byte layout explanation (prior to conversion by `handle_unknown_error`): +// - Byte 0: index of the error within `pop_api::Error` +// - Byte 1: +// - Must be zero for `UNIT_ERRORS`. +// - Represents the nested error in `SINGLE_NESTED_ERRORS`. +// - Represents the first level of nesting in `DOUBLE_NESTED_ERRORS`. +// - Byte 2: +// - Represents the second level of nesting in `DOUBLE_NESTED_ERRORS`. +// - Byte 3: +// - Unused or represents further nested information. +pub(crate) fn handle_unknown_error(encoded_error: &mut [u8; 4]) { + let is_unknown_error = match encoded_error[0] { + code if UNIT_ERRORS.contains(&code) => nested_errors(&encoded_error[1..], None), + TOKEN_ERROR => nested_errors(&encoded_error[1..], Some(INVALID_TOKEN_INDEX)), + ARITHMETIC_ERROR => nested_errors(&encoded_error[1..], Some(INVALID_ARITHMETIC_INDEX)), + TRANSACTIONAL_ERROR => { + nested_errors(&encoded_error[1..], Some(INVALID_TRANSACTIONAL_INDEX)) + }, + MODULE_ERROR => nested_errors(&encoded_error[3..], None), + _ => true, + }; + if is_unknown_error { + encoded_error[..].rotate_right(1); + encoded_error[0] = 0u8; + } +} + +// Checks for unknown or invalid nested errors within the `DispatchError`. +fn nested_errors(nested_error: &[u8], invalid_index_start: Option) -> bool { + match invalid_index_start { + // Checks if the first byte is equal or exceeds `invalid_index_start` or if any subsequent + // byte is non-zero. + Some(i) => nested_error[0] >= i || nested_error[1..].iter().any(|&x| x != 0u8), + // If no limit is provided, check if any byte is non-zero. None => nested_error.iter().any(|&x| x != 0u8), } } @@ -60,239 +74,165 @@ fn nested_errors(nested_error: &[u8], limit: Option) -> bool { #[cfg(test)] mod tests { use super::*; - use pop_primitives::error::{ - ArithmeticError::*, - Error::{self, *}, - TokenError::*, - TransactionalError::*, - }; - use sp_runtime::DispatchError; - - // Compare all the different `DispatchError` variants with the expected `Error`. - #[test] - fn dispatch_error_to_error() { - let test_cases = vec![ - ( - DispatchError::Other(""), - (Other { dispatch_error_index: 0, error_index: 0, error: 0 }), - ), - (DispatchError::Other("UnknownCall"), UnknownCall), - (DispatchError::Other("DecodingFailed"), DecodingFailed), - (DispatchError::CannotLookup, CannotLookup), - (DispatchError::BadOrigin, BadOrigin), - ( - DispatchError::Module(sp_runtime::ModuleError { - index: 1, - error: [2, 0, 0, 0], - message: Some("hallo"), - }), - Module { index: 1, error: 2 }, - ), - (DispatchError::ConsumerRemaining, ConsumerRemaining), - (DispatchError::NoProviders, NoProviders), - (DispatchError::TooManyConsumers, TooManyConsumers), - (DispatchError::Token(sp_runtime::TokenError::BelowMinimum), Token(BelowMinimum)), - ( - DispatchError::Arithmetic(sp_runtime::ArithmeticError::Overflow), - Arithmetic(Overflow), - ), - ( - DispatchError::Transactional(sp_runtime::TransactionalError::LimitReached), - Transactional(LimitReached), - ), - (DispatchError::Exhausted, Exhausted), - (DispatchError::Corruption, Corruption), - (DispatchError::Unavailable, Unavailable), - (DispatchError::RootNotAllowed, RootNotAllowed), - ]; - for (dispatch_error, expected) in test_cases { - let status_code = crate::convert_to_status_code(dispatch_error, 0); - let error: Error = status_code.into(); - assert_eq!(error, expected); - } - } + use crate::tests::assert_encoding_matches; + use sp_runtime::{ArithmeticError::*, DispatchError::*, TokenError::*, TransactionalError::*}; - // Compare all the different `DispatchError::Other` possibilities with the expected `Error`. - #[test] - fn other_error() { - let test_cases = vec![ - ( - DispatchError::Other("Random"), - (Other { dispatch_error_index: 0, error_index: 0, error: 0 }), - ), - (DispatchError::Other("UnknownCall"), UnknownCall), - (DispatchError::Other("DecodingFailed"), DecodingFailed), - ]; - for (dispatch_error, expected) in test_cases { - let status_code = convert_to_status_code(dispatch_error, 0); - let error: Error = status_code.into(); + // Assert encoding after `handle_unknown_error` with expected encoding. + fn assert_error_cases(test_cases: Vec<([u8; 4], [u8; 4])>) { + for (mut error, expected) in test_cases { + handle_unknown_error(&mut error); assert_eq!(error, expected); } } - // Compare all the different `DispatchError::Module` nesting possibilities, which can not be - // handled, with the expected `Error`. See `double_nested_error_variants` fourth byte nesting. - #[test] - fn test_module_error() { - let test_cases = vec![ - DispatchError::Module(sp_runtime::ModuleError { - index: 1, - error: [2, 2, 0, 0], - message: Some("Random"), - }), - DispatchError::Module(sp_runtime::ModuleError { - index: 1, - error: [2, 2, 2, 0], - message: Some("Random"), - }), - DispatchError::Module(sp_runtime::ModuleError { - index: 1, - error: [2, 2, 2, 2], - message: Some("Random"), - }), - ]; - for dispatch_error in test_cases { - let status_code = convert_to_status_code(dispatch_error, 0); - let error: Error = status_code.into(); - assert_eq!(error, Other { dispatch_error_index: 3, error_index: 1, error: 2 }); - } - } + // Tests for `handle_unknown_error`: + // 1. Unit errors. + // 2. Single nested errors. + // 3. Double nested errors. - // Converts 4 bytes into `Error` and handles unknown errors (used in `convert_to_status_code`). - fn into_error(mut error_bytes: [u8; 4]) -> Error { - handle_unknown_error(&mut error_bytes); - u32::from_le_bytes(error_bytes).into() - } - - // Tests the `handle_unknown_error` for `Error`, version 0. + // 1. Unit errors. // - // Unit variants: - // If the encoded value indicates a nested `Error` which is known by V0 as a - // unit variant, the encoded value is converted into `Error::Other`. + // If the encoded value indicates a nested enum, which is known by V0 as a + // unit error, the encoded value is converted. // // Example: the error `BadOrigin` (encoded: `[2, 0, 0, 0]`) with a non-zero value for one - // of the bytes [1..4]: `[2, 0, 1, 0]` is converted into `[0, 2, 0, 1]` (shifting the bits + // of the bytes [1..4] - `[2, 0, 1, 0]` - is converted into `[0, 2, 0, 1]` (shifting the bits // one forward). This is decoded to `Error::Other { dispatch_error: 2, index: 0, error: 1 }`. - #[test] - fn unit_error_variants() { - let errors = vec![ - CannotLookup, - BadOrigin, - ConsumerRemaining, - NoProviders, - TooManyConsumers, - Exhausted, - Corruption, - Unavailable, - RootNotAllowed, - UnknownCall, - DecodingFailed, - ]; - // Compare an `Error`, which is converted from an encoded value, with the expected `Error`. - for (i, &error_code) in UNIT_ERRORS.iter().enumerate() { - // No nesting and unit variant correctly returned. - assert_eq!(into_error([error_code, 0, 0, 0]), errors[i]); - // Unexpected second byte nested. - assert_eq!( - into_error([error_code, 1, 0, 0]), - (Other { dispatch_error_index: error_code, error_index: 1, error: 0 }), - ); - // Unexpected third byte nested. - assert_eq!( - into_error([error_code, 1, 1, 0]), - (Other { dispatch_error_index: error_code, error_index: 1, error: 1 }), - ); - // Unexpected fourth byte nested. - assert_eq!( - into_error([error_code, 1, 1, 1]), - (Other { dispatch_error_index: error_code, error_index: 1, error: 1 }), - ); - } + + macro_rules! unit_error_test { + ($name:ident, $error_variant:expr, $encoded_value:expr) => { + #[test] + fn $name() { + assert_encoding_matches($error_variant, $encoded_value); + let index = $encoded_value[0]; + let test_cases = vec![ + ($encoded_value, $encoded_value), + ([index, 1, 0, 0], [0, index, 1, 0]), + ([index, 1, 1, 0], [0, index, 1, 1]), + ([index, 1, 1, 1], [0, index, 1, 1]), + ([index, 1, 0, 1], [0, index, 1, 0]), + ([index, 0, 1, 1], [0, index, 0, 1]), + ([index, 0, 0, 1], [0, index, 0, 0]), + ]; + assert_error_cases(test_cases); + } + }; } - // Single nested variants: - // If the encoded value indicates a double nested `Error` which is known by V0 - // as a single nested variant, the encoded value is converted into `Error::Other`. + unit_error_test!(cannot_lookup_works, CannotLookup, [1u8, 0, 0, 0]); + unit_error_test!(bad_origin_works, BadOrigin, [2u8, 0, 0, 0]); + unit_error_test!(consumer_remaining_works, ConsumerRemaining, [4u8, 0, 0, 0]); + unit_error_test!(no_providers_works, NoProviders, [5u8, 0, 0, 0]); + unit_error_test!(too_many_consumers_works, TooManyConsumers, [6u8, 0, 0, 0]); + unit_error_test!(exhausted_works, Exhausted, [10u8, 0, 0, 0]); + unit_error_test!(corruption_works, Corruption, [11u8, 0, 0, 0]); + unit_error_test!(unavailable_works, Unavailable, [12u8, 0, 0, 0]); + unit_error_test!(root_not_allowed_works, RootNotAllowed, [13u8, 0, 0, 0]); + + // 2. Single nested errors. + // + // If the encoded value indicates a double nested error which is known by V0 + // as a single nested error, the encoded value is converted. // // Example: the error `Arithmetic(Overflow)` (encoded: `[8, 1, 0, 0]`) with a non-zero // value for one of the bytes [2..4]: `[8, 1, 1, 0]` is converted into `[0, 8, 1, 1]`. This is // decoded to `Error::Other { dispatch_error: 8, index: 1, error: 1 }`. - #[test] - fn single_nested_error_variants() { - let errors = vec![ - [Token(FundsUnavailable), Token(OnlyProvider)], - [Arithmetic(Underflow), Arithmetic(Overflow)], - [Transactional(LimitReached), Transactional(NoLayer)], - ]; - // Compare an `Error`, which is converted from an encoded value, with the expected `Error`. - for (i, &error_code) in SINGLE_NESTED_ERRORS.iter().enumerate() { - // No nested and single nested variant correctly returned. - assert_eq!(into_error([error_code, 0, 0, 0]), errors[i][0]); - assert_eq!(into_error([error_code, 1, 0, 0]), errors[i][1]); - // Unexpected third byte nested. - assert_eq!( - into_error([error_code, 1, 1, 0]), - (Other { dispatch_error_index: error_code, error_index: 1, error: 1 }), - ); - // Unexpected fourth byte nested. - assert_eq!( - into_error([error_code, 1, 1, 1]), - Other { dispatch_error_index: error_code, error_index: 1, error: 1 }, - ); - } - } - #[test] - fn single_nested_unknown_variants() { - // Unknown `TokenError` variant. - assert_eq!( - into_error([7, 10, 0, 0]), - Other { dispatch_error_index: 7, error_index: 10, error: 0 } - ); - // Unknown `Arithmetic` variant. - assert_eq!( - into_error([8, 3, 0, 0]), - Other { dispatch_error_index: 8, error_index: 3, error: 0 } - ); - // Unknown `Transactional` variant. - assert_eq!( - into_error([9, 2, 0, 0]), - Other { dispatch_error_index: 9, error_index: 2, error: 0 } - ); + macro_rules! single_nested_error_test { + ($name:ident, $error_enum:path, $base_error_code:expr, $invalid_index:expr, $errors:expr) => { + #[test] + fn $name() { + for (error, index) in $errors { + let valid_variant_expected_encoding = [$base_error_code, index, 0, 0]; + assert_encoding_matches($error_enum(error), valid_variant_expected_encoding); + // Test cases with valid single nested values. + let test_cases = vec![ + ([$base_error_code, index, 0, 0], valid_variant_expected_encoding), + ([$base_error_code, index, 1, 0], [0, $base_error_code, index, 1]), + ([$base_error_code, index, 0, 1], [0, $base_error_code, index, 0]), + ([$base_error_code, index, 1, 1], [0, $base_error_code, index, 1]), + ]; + assert_error_cases(test_cases); + } + + // Test cases with invalid single nested values. + for x in [$invalid_index, 100, u8::MAX] { + let test_cases = vec![ + ([$base_error_code, x, 0, 0], [0, $base_error_code, x, 0]), + ([$base_error_code, x, 1, 0], [0, $base_error_code, x, 1]), + ([$base_error_code, x, 0, 1], [0, $base_error_code, x, 0]), + ([$base_error_code, x, 1, 1], [0, $base_error_code, x, 1]), + ]; + assert_error_cases(test_cases); + } + } + }; } - // Double nested variants: - // If the encoded value indicates a triple nested `Error` which is known by V0 - // as a double nested variant, the encoded value is converted into `Error::Other`. + single_nested_error_test!( + token_error_works, + Token, + TOKEN_ERROR, + INVALID_TOKEN_INDEX, + vec![ + (FundsUnavailable, 0), + (OnlyProvider, 1), + (BelowMinimum, 2), + (CannotCreate, 3), + (UnknownAsset, 4), + (Frozen, 5), + (Unsupported, 6), + (CannotCreateHold, 7), + (NotExpendable, 8), + (Blocked, 9), + ] + ); + + single_nested_error_test!( + arithmetic_error_works, + Arithmetic, + ARITHMETIC_ERROR, + INVALID_ARITHMETIC_INDEX, + vec![(Underflow, 0), (Overflow, 1), (DivisionByZero, 2)] + ); + + single_nested_error_test!( + transactional_error_works, + Transactional, + TRANSACTIONAL_ERROR, + INVALID_TRANSACTIONAL_INDEX, + vec![(LimitReached, 0), (NoLayer, 1)] + ); + + // 3. Double nested error. + // + // If the encoded value indicates a triple nested error which is known by V0 + // as a double nested error, the encoded value is converted. // // Example: the error `Module { index: 10, error 5 }` (encoded: `[3, 10, 5, 0]`) with a non-zero // value for the last byte: `[3, 10, 5, 3]` is converted into `[0, 3, 10, 5]`. This is - // decoded to `Error::Other { dispatch_error: 3, index: 10, error: 5 }`. + // decoded to `Error::Other { dispatch_error: 3, index: 10, error: 5 }`. + #[test] fn double_nested_error_variants() { - // Compare an `Error`, which is converted from an encoded value, with the expected `Error`. - // No nesting and unit variant correctly returned. - assert_eq!(into_error([3, 0, 0, 0]), Module { index: 0, error: 0 }); - // Allowed single nesting and variant correctly returned. - assert_eq!(into_error([3, 1, 0, 0]), Module { index: 1, error: 0 }); - // Allowed double nesting and variant correctly returned. - assert_eq!(into_error([3, 1, 1, 0]), Module { index: 1, error: 1 }); - // Unexpected fourth byte nested. - assert_eq!( - into_error([3, 1, 1, 1]), - Other { dispatch_error_index: 3, error_index: 1, error: 1 }, - ); + for x in [1, 100, u8::MAX] { + let test_cases = vec![ + ([MODULE_ERROR, x, 0, 0], [MODULE_ERROR, x, 0, 0]), + ([MODULE_ERROR, x, x, 0], [MODULE_ERROR, x, x, 0]), + ([MODULE_ERROR, x, x, x], [0, MODULE_ERROR, x, x]), + ([MODULE_ERROR, 0, x, x], [0, MODULE_ERROR, 0, x]), + ([MODULE_ERROR, 0, 0, x], [0, MODULE_ERROR, 0, 0]), + ]; + assert_error_cases(test_cases); + } } #[test] fn test_random_encoded_values() { - assert_eq!( - into_error([100, 100, 100, 100]), - Other { dispatch_error_index: 100, error_index: 100, error: 100 } - ); - assert_eq!( - into_error([200, 200, 200, 200]), - Other { dispatch_error_index: 200, error_index: 200, error: 200 } - ); + let test_cases = vec![ + ([100, 100, 100, 100], [0, 100, 100, 100]), + ([u8::MAX, u8::MAX, u8::MAX, u8::MAX], [0, u8::MAX, u8::MAX, u8::MAX]), + ]; + assert_error_cases(test_cases); } } From bdfdc963d7865679b7399c90562e3ad5c406311d Mon Sep 17 00:00:00 2001 From: Daanvdplas Date: Sun, 18 Aug 2024 18:40:41 +0200 Subject: [PATCH 3/3] docs: v0 extension --- extension/src/v0.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/extension/src/v0.rs b/extension/src/v0.rs index 8fedb763..da1c028d 100644 --- a/extension/src/v0.rs +++ b/extension/src/v0.rs @@ -1,4 +1,9 @@ -//! TODO +//! This module is responsible for handling and converting unknown errors returned to contracts that use the first version of the Pop API. +//! +//! The errors are categorized into unit errors, single nested errors, and double nested errors, +//! each of which is encoded into a 4-byte array. The module provides functionality to convert +//! unknown errors into the general "pop_api::Error::Other" error type, ensuring consistent error categorization +//! across the runtime for contracts using the API. // Unit errors. // - CannotLookup: 1, @@ -43,7 +48,7 @@ const MODULE_ERROR: u8 = 3; // - Represents the second level of nesting in `DOUBLE_NESTED_ERRORS`. // - Byte 3: // - Unused or represents further nested information. -pub(crate) fn handle_unknown_error(encoded_error: &mut [u8; 4]) { +pub(super) fn handle_unknown_error(encoded_error: &mut [u8; 4]) { let is_unknown_error = match encoded_error[0] { code if UNIT_ERRORS.contains(&code) => nested_errors(&encoded_error[1..], None), TOKEN_ERROR => nested_errors(&encoded_error[1..], Some(INVALID_TOKEN_INDEX)), @@ -96,7 +101,7 @@ mod tests { // unit error, the encoded value is converted. // // Example: the error `BadOrigin` (encoded: `[2, 0, 0, 0]`) with a non-zero value for one - // of the bytes [1..4] - `[2, 0, 1, 0]` - is converted into `[0, 2, 0, 1]` (shifting the bits + // of the bytes [1..4]: `[2, 0, 1, 0]` is converted into `[0, 2, 0, 1]` (shifting the bits // one forward). This is decoded to `Error::Other { dispatch_error: 2, index: 0, error: 1 }`. macro_rules! unit_error_test {