From 9820e7fc0c8568cf1efad813b7464d09c13e676f Mon Sep 17 00:00:00 2001 From: Daan van der Plas <93204684+Daanvdplas@users.noreply.github.com> Date: Sun, 28 Jul 2024 12:07:55 +0200 Subject: [PATCH] refactor: remove read state encoding (#122) Co-authored-by: Frank Bell --- pallets/api/src/fungibles/mod.rs | 78 ++++++++----------------- pallets/api/src/fungibles/tests.rs | 20 ++++--- pop-api/src/lib.rs | 4 +- pop-api/src/v0/assets/fungibles.rs | 20 ++----- pop-api/src/v0/assets/mod.rs | 1 - runtime/devnet/src/config/api.rs | 35 ++++++++---- runtime/devnet/src/extensions/mod.rs | 85 ++++++++++++---------------- runtime/devnet/src/extensions/v0.rs | 4 +- 8 files changed, 106 insertions(+), 141 deletions(-) diff --git a/pallets/api/src/fungibles/mod.rs b/pallets/api/src/fungibles/mod.rs index cd34664ad..32f9af678 100644 --- a/pallets/api/src/fungibles/mod.rs +++ b/pallets/api/src/fungibles/mod.rs @@ -180,61 +180,33 @@ pub mod pallet { } impl Pallet { - /// Returns the total token supply for a given asset ID. + /// Reads fungible asset state based on the provided value. /// - /// # Parameters - /// * `id` - The ID of the asset. - pub fn total_supply(id: AssetIdOf) -> BalanceOf { - AssetsOf::::total_supply(id) - } - - /// Returns the account balance for the specified `owner` for a given asset ID. Returns `0` if - /// the account is non-existent. + /// This function matches the value to determine the type of state query and returns the + /// encoded result. /// - /// # Parameters - /// * `id` - The ID of the asset. - /// * `owner` - The account whose balance is being queried. - pub fn balance_of(id: AssetIdOf, owner: &AccountIdOf) -> BalanceOf { - AssetsOf::::balance(id, owner) - } - - /// Returns the amount which `spender` is still allowed to withdraw from `owner` for a given - /// asset ID. Returns `0` if no allowance has been set. - /// - /// # Parameters - /// * `id` - The ID of the asset. - /// * `owner` - The account that owns the tokens. - /// * `spender` - The account that is allowed to spend the tokens. - pub fn allowance( - id: AssetIdOf, - owner: &AccountIdOf, - spender: &AccountIdOf, - ) -> BalanceOf { - AssetsOf::::allowance(id, owner, spender) - } - - /// Returns the token name for a given asset ID. - /// - /// # Parameters - /// * `id` - The ID of the asset. - pub fn token_name(id: AssetIdOf) -> Vec { - as MetadataInspect>>::name(id) - } - - /// Returns the token symbol for a given asset ID. - /// - /// # Parameters - /// * `id` - The ID of the asset. - pub fn token_symbol(id: AssetIdOf) -> Vec { - as MetadataInspect>>::symbol(id) - } - - /// Returns the token decimals for a given asset ID. - /// - /// # Parameters - /// * `id` - The ID of the asset. - pub fn token_decimals(id: AssetIdOf) -> u8 { - as MetadataInspect>>::decimals(id) + /// # Parameter + /// * `value` - An instance of `Read`, which specifies the type of state query and + /// the associated parameters. + pub fn read_state(value: Read) -> Vec { + 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() + }, + TokenName(id) => { + as MetadataInspect>>::name(id).encode() + }, + TokenSymbol(id) => { + as MetadataInspect>>::symbol(id).encode() + }, + TokenDecimals(id) => { + as MetadataInspect>>::decimals(id).encode() + }, + } } } } diff --git a/pallets/api/src/fungibles/tests.rs b/pallets/api/src/fungibles/tests.rs index dbfa0b34b..c8de10205 100644 --- a/pallets/api/src/fungibles/tests.rs +++ b/pallets/api/src/fungibles/tests.rs @@ -1,4 +1,5 @@ -use crate::mock::*; +use crate::{fungibles::Read::*, mock::*}; +use codec::Encode; use frame_support::{ assert_ok, traits::fungibles::{approvals::Inspect, metadata::Inspect as MetadataInspect}, @@ -60,7 +61,7 @@ fn increase_allowance_works() { fn total_supply_works() { new_test_ext().execute_with(|| { create_asset_and_mint_to(ALICE, ASSET, ALICE, 100); - assert_eq!(Assets::total_supply(ASSET), Fungibles::total_supply(ASSET)); + assert_eq!(Assets::total_supply(ASSET).encode(), Fungibles::read_state(TotalSupply(ASSET))); }); } @@ -68,7 +69,10 @@ fn total_supply_works() { fn balance_of_works() { new_test_ext().execute_with(|| { create_asset_and_mint_to(ALICE, ASSET, ALICE, 100); - assert_eq!(Assets::balance(ASSET, ALICE), Fungibles::balance_of(ASSET, &ALICE)); + assert_eq!( + Assets::balance(ASSET, ALICE).encode(), + Fungibles::read_state(BalanceOf { id: ASSET, owner: ALICE }) + ); }); } @@ -77,8 +81,8 @@ fn allowance_works() { new_test_ext().execute_with(|| { create_asset_mint_and_approve(ALICE, ASSET, BOB, 100, ALICE, 50); assert_eq!( - Assets::allowance(ASSET, &ALICE, &BOB), - Fungibles::allowance(ASSET, &ALICE, &BOB) + Assets::allowance(ASSET, &ALICE, &BOB).encode(), + Fungibles::read_state(Allowance { id: ASSET, owner: ALICE, spender: BOB }) ); }); } @@ -90,9 +94,9 @@ fn token_metadata_works() { let symbol: Vec = vec![21, 22, 23]; let decimals: u8 = 69; create_asset_and_set_metadata(ALICE, ASSET, name.clone(), symbol.clone(), decimals); - assert_eq!(Assets::name(ASSET), Fungibles::token_name(ASSET)); - assert_eq!(Assets::symbol(ASSET), Fungibles::token_symbol(ASSET)); - assert_eq!(Assets::decimals(ASSET), Fungibles::token_decimals(ASSET)); + assert_eq!(Assets::name(ASSET).encode(), Fungibles::read_state(TokenName(ASSET))); + assert_eq!(Assets::symbol(ASSET).encode(), Fungibles::read_state(TokenSymbol(ASSET))); + assert_eq!(Assets::decimals(ASSET).encode(), Fungibles::read_state(TokenDecimals(ASSET))); }); } diff --git a/pop-api/src/lib.rs b/pop-api/src/lib.rs index efa7ecb2c..7fcfc85b2 100644 --- a/pop-api/src/lib.rs +++ b/pop-api/src/lib.rs @@ -1,9 +1,7 @@ #![cfg_attr(not(feature = "std"), no_std, no_main)] -use ink::env::chain_extension::ChainExtensionMethod; - use constants::DECODING_FAILED; -use ink::env::chain_extension::FromStatusCode; +use ink::env::chain_extension::{ChainExtensionMethod, FromStatusCode}; #[cfg(feature = "assets")] pub use v0::assets; diff --git a/pop-api/src/v0/assets/fungibles.rs b/pop-api/src/v0/assets/fungibles.rs index 374b6e88c..0712a80b4 100644 --- a/pop-api/src/v0/assets/fungibles.rs +++ b/pop-api/src/v0/assets/fungibles.rs @@ -1,12 +1,10 @@ -use ink::{env::chain_extension::ChainExtensionMethod, prelude::vec::Vec, scale::Decode}; - use crate::{ - constants::{ASSETS, BALANCES, DECODING_FAILED, FUNGIBLES}, + constants::{ASSETS, BALANCES, FUNGIBLES}, primitives::{AccountId, AssetId, Balance}, - v0::V0, Result, StatusCode, }; use constants::*; +use ink::{env::chain_extension::ChainExtensionMethod, prelude::vec::Vec}; pub use metadata::*; /// Helper method to build a dispatch call `ChainExtensionMethod` for fungibles `v0` @@ -78,10 +76,9 @@ mod constants { pub fn total_supply(id: AssetId) -> Result { build_read_state(TOTAL_SUPPLY) .input::() - .output::>, true>() + .output::, true>() .handle_error_code::() .call(&(id)) - .and_then(|v| Balance::decode(&mut &v[..]).map_err(|_e| StatusCode(DECODING_FAILED))) } /// Returns the account balance for the specified `owner` for a given asset ID. Returns `0` if @@ -97,10 +94,9 @@ pub fn total_supply(id: AssetId) -> Result { pub fn balance_of(id: AssetId, owner: AccountId) -> Result { build_read_state(BALANCE_OF) .input::<(AssetId, AccountId)>() - .output::>, true>() + .output::, true>() .handle_error_code::() .call(&(id, owner)) - .and_then(|v| Balance::decode(&mut &v[..]).map_err(|_e| StatusCode(DECODING_FAILED))) } /// Returns the amount which `spender` is still allowed to withdraw from `owner` for a given @@ -117,10 +113,9 @@ pub fn balance_of(id: AssetId, owner: AccountId) -> Result { pub fn allowance(id: AssetId, owner: AccountId, spender: AccountId) -> Result { build_read_state(ALLOWANCE) .input::<(AssetId, AccountId, AccountId)>() - .output::>, true>() + .output::, true>() .handle_error_code::() .call(&(id, owner, spender)) - .and_then(|v| Balance::decode(&mut &v[..]).map_err(|_e| StatusCode(DECODING_FAILED))) } /// Transfers `value` amount of tokens from the caller's account to account `to`, with additional @@ -233,7 +228,6 @@ pub mod metadata { .output::>, true>() .handle_error_code::() .call(&(id)) - .and_then(|v| >::decode(&mut &v[..]).map_err(|_e| StatusCode(DECODING_FAILED))) } /// Returns the token symbol for a given asset ID. @@ -250,7 +244,6 @@ pub mod metadata { .output::>, true>() .handle_error_code::() .call(&(id)) - .and_then(|v| >::decode(&mut &v[..]).map_err(|_e| StatusCode(DECODING_FAILED))) } /// Returns the token decimals for a given asset ID. @@ -264,10 +257,9 @@ pub mod metadata { pub fn token_decimals(id: AssetId) -> Result { build_read_state(TOKEN_DECIMALS) .input::() - .output::>, true>() + .output::, true>() .handle_error_code::() .call(&(id)) - .and_then(|v| ::decode(&mut &v[..]).map_err(|_e| StatusCode(DECODING_FAILED))) } } diff --git a/pop-api/src/v0/assets/mod.rs b/pop-api/src/v0/assets/mod.rs index c68c0181c..197db7107 100644 --- a/pop-api/src/v0/assets/mod.rs +++ b/pop-api/src/v0/assets/mod.rs @@ -1,3 +1,2 @@ #[cfg(feature = "fungibles")] pub mod fungibles; - diff --git a/runtime/devnet/src/config/api.rs b/runtime/devnet/src/config/api.rs index ae179e4a3..884128f2d 100644 --- a/runtime/devnet/src/config/api.rs +++ b/runtime/devnet/src/config/api.rs @@ -11,24 +11,37 @@ pub enum RuntimeRead { Fungibles(fungibles::Read), } -impl fungibles::Config for Runtime { - type AssetsInstance = TrustBackedAssetsInstance; - type WeightInfo = fungibles::weights::SubstrateWeight; -} - -/// A type to identify allowed calls to the Runtime from contracts. Used by Pop API +/// A type to identify allowed calls to the Runtime from the API. pub struct AllowedApiCalls; impl Contains for AllowedApiCalls { + /// Allowed runtime calls from the API. fn contains(c: &RuntimeCall) -> bool { - use fungibles::Call as FungiblesCall; + use fungibles::Call::*; + matches!( + c, + RuntimeCall::Fungibles(transfer { .. } | approve { .. } | increase_allowance { .. }) + ) + } +} + +impl Contains> for AllowedApiCalls { + /// Allowed state queries from the API. + fn contains(c: &RuntimeRead) -> bool { + use fungibles::Read::*; matches!( c, - RuntimeCall::Fungibles( - FungiblesCall::transfer { .. } - | FungiblesCall::approve { .. } - | FungiblesCall::increase_allowance { .. } + RuntimeRead::Fungibles( + TotalSupply(..) + | BalanceOf { .. } | Allowance { .. } + | TokenName(..) | TokenSymbol(..) + | TokenDecimals(..) ) ) } } + +impl fungibles::Config for Runtime { + type AssetsInstance = TrustBackedAssetsInstance; + type WeightInfo = fungibles::weights::SubstrateWeight; +} diff --git a/runtime/devnet/src/extensions/mod.rs b/runtime/devnet/src/extensions/mod.rs index 3aed89dfa..d3bbdd0b9 100644 --- a/runtime/devnet/src/extensions/mod.rs +++ b/runtime/devnet/src/extensions/mod.rs @@ -5,10 +5,7 @@ use crate::{ api::{AllowedApiCalls, RuntimeRead}, assets::TrustBackedAssetsInstance, }, - fungibles::{ - self, - Read::{self, *}, - }, + fungibles::{self}, AccountId, RuntimeCall, RuntimeOrigin, }; use codec::{Decode, Encode}; @@ -26,6 +23,14 @@ use sp_runtime::{traits::Dispatchable, DispatchError}; use sp_std::vec::Vec; 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; @@ -113,8 +118,7 @@ where params.insert(0, version); params.insert(1, pallet_index); params.insert(2, call_index); - let call = ::decode(&mut ¶ms[..]) - .map_err(|_| DispatchError::Other("DecodingFailed"))?; + let call = ::decode(&mut ¶ms[..]).map_err(|_| DECODING_FAILED_ERROR)?; // Contract is the origin by default. let origin: RuntimeOrigin = RawOrigin::Signed(env.ext().address().clone()).into(); @@ -169,19 +173,24 @@ where { const LOG_PREFIX: &str = " read_state |"; - // Prefix params with version, pallet, index to simplify decoding. + // Prefix params with version, pallet, index to simplify decoding, and decode parameters for + // reading state. params.insert(0, version); params.insert(1, pallet_index); params.insert(2, call_index); - let key = >::decode(&mut ¶ms[..]) - .map_err(|_| DispatchError::Other("DecodingFailed"))?; + let read = + >::decode(&mut ¶ms[..]).map_err(|_| DECODING_FAILED_ERROR)?; - let result = match key { - VersionedStateRead::V0(key) => match key { - RuntimeRead::Fungibles(key) => read_fungibles_state::(key, env), + // Charge weight for doing one storage read. + env.charge_weight(T::DbWeight::get().reads(1_u64))?; + let result = match read { + VersionedStateRead::V0(read) => { + ensure!(AllowedApiCalls::contains(&read), UNKNOWN_CALL_ERROR); + match read { + RuntimeRead::Fungibles(key) => fungibles::Pallet::::read_state(key), + } }, - }? - .encode(); + }; log::trace!( target:LOG_TARGET, "{} result: {:?}.", LOG_PREFIX, result @@ -218,16 +227,18 @@ enum VersionedDispatch { // - `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 { - // "UnknownFunctionId" and "DecodingFailed" are mapped to specific errors in the API and will - // never change. - let mut encoded_error = match error { - DispatchError::Other("UnknownFunctionId") => Vec::from([254u8, 0, 0, 0]), - DispatchError::Other("DecodingFailed") => Vec::from([255u8, 0, 0, 0]), - _ => error.encode(), + let mut encoded_error: [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, + DECODING_FAILED_ERROR => DECODING_FAILED_ERROR_ENCODED, + _ => { + let mut encoded_error = error.encode(); + // Resize the encoded value to 4 bytes in order to decode the value in a u32 (4 bytes). + encoded_error.resize(4, 0); + encoded_error.try_into().expect("qed, resized to 4 bytes line above") + }, }; - // Resize the encoded value to 4 bytes in order to decode the value in a u32 (4 bytes). - encoded_error.resize(4, 0); - let mut encoded_error = 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 @@ -246,7 +257,7 @@ pub(crate) fn convert_to_status_code(error: DispatchError, version: u8) -> u32 { // - Byte 3: // - Unused or represents further nested information. 0 => v0::handle_unknown_error(&mut encoded_error), - _ => encoded_error = [254, 0, 0, 0], + _ => encoded_error = UNKNOWN_CALL_ERROR_ENCODED, } u32::from_le_bytes(encoded_error) } @@ -278,37 +289,13 @@ impl TryFrom for FuncId { 0 => Self::Dispatch, 1 => Self::ReadState, _ => { - return Err(DispatchError::Other("UnknownFuncId")); + return Err(UNKNOWN_CALL_ERROR); }, }; Ok(id) } } -fn read_fungibles_state( - key: Read, - env: &mut Environment, -) -> Result, DispatchError> -where - T: pallet_contracts::Config - + pallet_assets::Config - + fungibles::Config, - E: Ext, - T: frame_system::Config, -{ - env.charge_weight(T::DbWeight::get().reads(1_u64))?; - match key { - TotalSupply(id) => Ok(fungibles::Pallet::::total_supply(id).encode()), - BalanceOf { id, owner } => Ok(fungibles::Pallet::::balance_of(id, &owner).encode()), - Allowance { id, owner, spender } => { - Ok(fungibles::Pallet::::allowance(id, &owner, &spender).encode()) - }, - TokenName(id) => Ok(fungibles::Pallet::::token_name(id).encode()), - TokenSymbol(id) => Ok(fungibles::Pallet::::token_symbol(id).encode()), - TokenDecimals(id) => Ok(fungibles::Pallet::::token_decimals(id).encode()), - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/runtime/devnet/src/extensions/v0.rs b/runtime/devnet/src/extensions/v0.rs index b26668f78..727603236 100644 --- a/runtime/devnet/src/extensions/v0.rs +++ b/runtime/devnet/src/extensions/v0.rs @@ -76,7 +76,7 @@ mod tests { DispatchError::Other(""), (Other { dispatch_error_index: 0, error_index: 0, error: 0 }), ), - (DispatchError::Other("UnknownFunctionId"), UnknownCall), + (DispatchError::Other("UnknownCall"), UnknownCall), (DispatchError::Other("DecodingFailed"), DecodingFailed), (DispatchError::CannotLookup, CannotLookup), (DispatchError::BadOrigin, BadOrigin), @@ -120,7 +120,7 @@ mod tests { DispatchError::Other("Random"), (Other { dispatch_error_index: 0, error_index: 0, error: 0 }), ), - (DispatchError::Other("UnknownFunctionId"), UnknownCall), + (DispatchError::Other("UnknownCall"), UnknownCall), (DispatchError::Other("DecodingFailed"), DecodingFailed), ]; for (dispatch_error, expected) in test_cases {