From c1189fc5c3c15629b4c27bf3266e88d3ad843c45 Mon Sep 17 00:00:00 2001 From: Fahrrader Date: Thu, 02 Jun 2022 12:50:37 +0000 Subject: [PATCH] refactor(rmrk-proxy): better decoding --- --- a/pallets/proxy-rmrk-core/src/lib.rs +++ b/pallets/proxy-rmrk-core/src/lib.rs @@ -111,7 +111,7 @@ CorruptedCollectionType, NftTypeEncodeError, RmrkPropertyKeyIsTooLong, - RmrkPropertyValueIsTooLong, // todo utilize that in RPCs + RmrkPropertyValueIsTooLong, /* RMRK compatible events */ CollectionNotEmpty, @@ -518,7 +518,7 @@ Ok(scoped_key) } - pub fn rmrk_property( + pub fn rmrk_property( // todo think about renaming this rmrk_key: RmrkProperty, value: &E, ) -> Result { @@ -533,6 +533,21 @@ Ok(property) } + + pub fn decode_property( + vec: PropertyValue, + ) -> Result { + vec.decode().map_err(|_| >::RmrkPropertyValueIsTooLong.into()) + } + + pub fn rebind( + vec: &BoundedVec, + ) -> Result, DispatchError> + where + BoundedVec: TryFrom> + { + vec.rebind().map_err(|_| >::RmrkPropertyValueIsTooLong.into()) + } fn init_collection( sender: T::CrossAccountId, @@ -619,8 +634,7 @@ )?; let resource_collection_id: CollectionId = - Self::get_nft_property(collection_id, token_id, ResourceCollection)? - .decode_or_default(); + Self::get_nft_property_decoded(collection_id, token_id, ResourceCollection)?; let resource_collection = Self::get_typed_nft_collection(resource_collection_id, misc::CollectionType::Resource)?; @@ -709,14 +723,19 @@ Ok(collection_property) } + pub fn get_collection_property_decoded( + collection_id: CollectionId, + key: RmrkProperty, + ) -> Result { + Self::decode_property( + Self::get_collection_property(collection_id, key)? + ) + } + pub fn get_collection_type( collection_id: CollectionId, ) -> Result { - let value = Self::get_collection_property(collection_id, CollectionType)?; - - let mut value = value.as_slice(); - - misc::CollectionType::decode(&mut value) + Self::get_collection_property_decoded(collection_id, CollectionType) .map_err(|_| >::CorruptedCollectionType.into()) } @@ -749,12 +768,22 @@ ) -> Result { let nft_property = >::token_properties((collection_id, nft_id)) .get(&Self::rmrk_property_key(key)?) - .ok_or(>::NoAvailableNftId)? // todo replace with better error + .ok_or(>::NoAvailableNftId)? // todo replace with better error? .clone(); Ok(nft_property) } + pub fn get_nft_property_decoded( + collection_id: CollectionId, + nft_id: TokenId, + key: RmrkProperty, + ) -> Result { + Self::decode_property( + Self::get_nft_property(collection_id, nft_id, key)? + ) + } + pub fn nft_exists(collection_id: CollectionId, nft_id: TokenId) -> bool { >::contains_key((collection_id, nft_id)) } @@ -763,9 +792,8 @@ collection_id: CollectionId, token_id: TokenId, ) -> Result { - Ok(Self::get_nft_property(collection_id, token_id, TokenType)?.decode_or_default()) - // todo throw error - // NftTypeEncodeError? + Self::get_nft_property_decoded(collection_id, token_id, TokenType) + .map_err(|_| >::NftTypeEncodeError.into()) } pub fn ensure_nft_type( @@ -814,18 +842,17 @@ let key: Key = key.try_into().ok()?; let value = match token_id { - Some(token_id) => Self::get_nft_property( + Some(token_id) => Self::get_nft_property_decoded( collection_id, token_id, UserProperty(key.as_ref()), ), - None => Self::get_collection_property( + None => Self::get_collection_property_decoded( collection_id, UserProperty(key.as_ref()), ), } - .ok()? - .decode_or_default(); + .ok()?; Some(mapper(key, value)) }) @@ -862,7 +889,7 @@ let key = key.as_slice().strip_prefix(key_prefix.as_slice())?; let key: Key = key.to_vec().try_into().ok()?; - let value: Value = value.decode_or_default(); + let value: Value = value.decode().ok()?; Some(mapper(key, value)) }); --- a/pallets/proxy-rmrk-core/src/misc.rs +++ b/pallets/proxy-rmrk-core/src/misc.rs @@ -1,6 +1,5 @@ use super::*; -use codec::{Encode, Decode}; -use derivative::Derivative; +use codec::{Encode, Decode, Error}; #[macro_export] macro_rules! map_common_err_to_proxy { @@ -15,30 +14,30 @@ }; } -pub trait RmrkDecode { - fn decode_or_default(&self) -> T; +// Utilize the RmrkCore pallet for access to Runtime errors. +pub trait RmrkDecode { + fn decode(&self) -> Result; } -// todo fail if unwrap doesn't work -impl RmrkDecode for BoundedVec { - fn decode_or_default(&self) -> T { +impl RmrkDecode for BoundedVec { + fn decode(&self) -> Result { let mut value = self.as_slice(); - T::decode(&mut value).unwrap_or_default() + T::decode(&mut value) } } +// Utilize the RmrkCore pallet for access to Runtime errors. pub trait RmrkRebind { - fn rebind(&self) -> BoundedVec; + fn rebind(&self) -> Result, Error>; } -// todo fail if unwrap doesn't work impl RmrkRebind for BoundedVec where BoundedVec: TryFrom>, { - fn rebind(&self) -> BoundedVec { - BoundedVec::::try_from(self.clone().into_inner()).unwrap_or_default() + fn rebind(&self) -> Result, Error> { + BoundedVec::::try_from(self.clone().into_inner()).map_err(|_| "BoundedVec exceeds its limit".into()) } } @@ -49,11 +48,8 @@ Base, } -// todo remove default? -#[derive(Encode, Decode, PartialEq, Eq, Derivative)] -#[derivative(Default(bound = ""))] +#[derive(Encode, Decode, PartialEq, Eq)] pub enum NftType { - #[derivative(Default)] Regular, Resource, FixedPart, @@ -61,11 +57,8 @@ Theme, } -// todo remove default? -#[derive(Encode, Decode, PartialEq, Eq, Derivative)] -#[derivative(Default(bound = ""))] +#[derive(Encode, Decode, PartialEq, Eq)] pub enum ResourceType { - #[derivative(Default)] Basic, Composable, Slot, --- a/runtime/common/src/runtime_apis.rs +++ b/runtime/common/src/runtime_apis.rs @@ -144,7 +144,7 @@ } fn collection_by_id(collection_id: RmrkCollectionId) -> Result>, DispatchError> { - use pallet_proxy_rmrk_core::{RmrkProperty, misc::{CollectionType, RmrkDecode, RmrkRebind}}; + use pallet_proxy_rmrk_core::{RmrkProperty, misc::{CollectionType}}; let collection_id = RmrkCore::unique_collection_id(collection_id)?; let collection = match RmrkCore::get_typed_nft_collection(collection_id, CollectionType::Regular) { @@ -157,9 +157,9 @@ Ok(Some(RmrkCollectionInfo { issuer: collection.owner.clone(), - metadata: RmrkCore::get_collection_property(collection_id, RmrkProperty::Metadata)?.decode_or_default(), + metadata: RmrkCore::get_collection_property_decoded(collection_id, RmrkProperty::Metadata)?, max: collection.limits.token_limit, - symbol: collection.token_prefix.rebind(), + symbol: RmrkCore::rebind(&collection.token_prefix)?, nfts_count })) } @@ -185,9 +185,9 @@ Ok(Some(RmrkInstanceInfo { owner: owner, - royalty: RmrkCore::get_nft_property(collection_id, nft_id, RmrkProperty::RoyaltyInfo)?.decode_or_default(), - metadata: RmrkCore::get_nft_property(collection_id, nft_id, RmrkProperty::Metadata)?.decode_or_default(), - equipped: RmrkCore::get_nft_property(collection_id, nft_id, RmrkProperty::Equipped)?.decode_or_default(), + royalty: RmrkCore::get_nft_property_decoded(collection_id, nft_id, RmrkProperty::RoyaltyInfo)?, + metadata: RmrkCore::get_nft_property_decoded(collection_id, nft_id, RmrkProperty::Metadata)?, + equipped: RmrkCore::get_nft_property_decoded(collection_id, nft_id, RmrkProperty::Equipped)?, pending: allowance.is_some(), })) } @@ -281,8 +281,7 @@ let nft_id = TokenId(nft_id); if RmrkCore::ensure_nft_type(collection_id, nft_id, NftType::Regular).is_err() { return Ok(Vec::new()); } - let res_collection_id: CollectionId = RmrkCore::get_nft_property(collection_id, nft_id, RmrkProperty::ResourceCollection)? - .decode_or_default(); + let res_collection_id: CollectionId = RmrkCore::get_nft_property_decoded(collection_id, nft_id, RmrkProperty::ResourceCollection)?; let resource_collection = RmrkCore::get_typed_nft_collection(res_collection_id, CollectionType::Resource)?; let resources = resource_collection @@ -290,32 +289,31 @@ .iter() .filter_map(|(res_id)| Some(RmrkResourceInfo { id: res_id.0, - pending: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::PendingResourceAccept).unwrap().decode_or_default(), - pending_removal: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::PendingResourceRemoval).unwrap().decode_or_default(), - resource: match RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::ResourceType).unwrap().decode_or_default() { + pending: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::PendingResourceAccept).unwrap(), + pending_removal: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::PendingResourceRemoval).unwrap(), + resource: match RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::ResourceType).unwrap() { ResourceType::Basic => RmrkResourceTypes::Basic(RmrkBasicResource { - src: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::Src).unwrap().decode_or_default(), - metadata: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::Metadata).unwrap().decode_or_default(), - license: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::License).unwrap().decode_or_default(), - thumb: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::Thumb).unwrap().decode_or_default(), + src: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::Src).unwrap(), + metadata: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::Metadata).unwrap(), + license: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::License).unwrap(), + thumb: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::Thumb).unwrap(), }), ResourceType::Composable => RmrkResourceTypes::Composable(RmrkComposableResource { - parts: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::Parts).unwrap().decode_or_default(), - base: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::Base).unwrap().decode_or_default(), - src: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::Src).unwrap().decode_or_default(), - metadata: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::Metadata).unwrap().decode_or_default(), - license: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::License).unwrap().decode_or_default(), - thumb: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::Thumb).unwrap().decode_or_default(), + parts: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::Parts).unwrap(), + base: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::Base).unwrap(), + src: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::Src).unwrap(), + metadata: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::Metadata).unwrap(), + license: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::License).unwrap(), + thumb: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::Thumb).unwrap(), }), ResourceType::Slot => RmrkResourceTypes::Slot(RmrkSlotResource { - base: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::Base).unwrap().decode_or_default(), - src: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::Src).unwrap().decode_or_default(), - metadata: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::Metadata).unwrap().decode_or_default(), - slot: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::Slot).unwrap().decode_or_default(), - license: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::License).unwrap().decode_or_default(), - thumb: RmrkCore::get_nft_property(res_collection_id, *res_id, RmrkProperty::Thumb).unwrap().decode_or_default(), + base: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::Base).unwrap(), + src: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::Src).unwrap(), + metadata: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::Metadata).unwrap(), + slot: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::Slot).unwrap(), + license: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::License).unwrap(), + thumb: RmrkCore::get_nft_property_decoded(res_collection_id, *res_id, RmrkProperty::Thumb).unwrap(), }), - // todo refactor :| }, })) .collect(); @@ -332,28 +330,26 @@ let nft_id = TokenId(nft_id); if RmrkCore::ensure_nft_type(collection_id, nft_id, NftType::Regular).is_err() { return Ok(Vec::new()); } - /*let resource_collection_id: CollectionId = RmrkCore::get_nft_property(collection_id, nft_id, RmrkProperty::ResourceCollection) - .unwrap() - .decode_or_default(); + /*let resource_collection_id: CollectionId = RmrkCore::get_nft_property_decoded(collection_id, nft_id, RmrkProperty::ResourceCollection) + .unwrap(); if RmrkCore::ensure_collection_type(collection_id, CollectionType::Resource).is_err() { return Ok(Vec::new()); } let resources = pallet_nonfungible::TokenProperties::::iter_prefix((resource_collection_id,)) .filter_map(|(resource_id, properties)| Some(( resource_id, // ResourceId property - RmrkCore::get_nft_property(resource_collection_id, resource_id, RmrkProperty::Priority).unwrap().decode_or_default(), + RmrkCore::get_nft_property_decoded(resource_collection_id, resource_id, RmrkProperty::Priority).unwrap(), ))) .collect() .sort_by_key(|(_, index)| *index) .into_iter().map(|(resource_id, _)| resource_id)*/ - let priorities = RmrkCore::get_nft_property(collection_id, nft_id, RmrkProperty::ResourcePriorities)?.decode_or_default(); - // todo let it simply be default here after removing default from decode + let priorities = RmrkCore::get_nft_property_decoded(collection_id, nft_id, RmrkProperty::ResourcePriorities)?; Ok(priorities) } fn base(base_id: RmrkBaseId) -> Result>, DispatchError> { use pallet_proxy_rmrk_core::{ - RmrkProperty, misc::{CollectionType, RmrkDecode, RmrkRebind}, + RmrkProperty, misc::{CollectionType}, }; let collection_id = RmrkCore::unique_collection_id(base_id)?; @@ -364,8 +360,8 @@ Ok(Some(RmrkBaseInfo { issuer: collection.owner.clone(), - base_type: RmrkCore::get_collection_property(collection_id, RmrkProperty::BaseType)?.decode_or_default(), - symbol: collection.token_prefix.rebind(), + base_type: RmrkCore::get_collection_property_decoded(collection_id, RmrkProperty::BaseType)?, + symbol: RmrkCore::rebind(&collection.token_prefix)?, })) } @@ -382,15 +378,15 @@ match nft_type { NftType::FixedPart => Some(RmrkPartType::FixedPart(RmrkFixedPart { - id: RmrkCore::get_nft_property(collection_id, token_id, RmrkProperty::ExternalPartId).ok()?.decode_or_default(), - src: RmrkCore::get_nft_property(collection_id, token_id, RmrkProperty::Src).ok()?.decode_or_default(), - z: RmrkCore::get_nft_property(collection_id, token_id, RmrkProperty::ZIndex).ok()?.decode_or_default(), + id: RmrkCore::get_nft_property_decoded(collection_id, token_id, RmrkProperty::ExternalPartId).ok()?, + src: RmrkCore::get_nft_property_decoded(collection_id, token_id, RmrkProperty::Src).ok()?, + z: RmrkCore::get_nft_property_decoded(collection_id, token_id, RmrkProperty::ZIndex).ok()?, })), NftType::SlotPart => Some(RmrkPartType::SlotPart(RmrkSlotPart { - id: RmrkCore::get_nft_property(collection_id, token_id, RmrkProperty::ExternalPartId).ok()?.decode_or_default(), - src: RmrkCore::get_nft_property(collection_id, token_id, RmrkProperty::Src).ok()?.decode_or_default(), - z: RmrkCore::get_nft_property(collection_id, token_id, RmrkProperty::ZIndex).ok()?.decode_or_default(), - equippable: RmrkCore::get_nft_property(collection_id, token_id, RmrkProperty::EquippableList).ok()?.decode_or_default(), + id: RmrkCore::get_nft_property_decoded(collection_id, token_id, RmrkProperty::ExternalPartId).ok()?, + src: RmrkCore::get_nft_property_decoded(collection_id, token_id, RmrkProperty::Src).ok()?, + z: RmrkCore::get_nft_property_decoded(collection_id, token_id, RmrkProperty::ZIndex).ok()?, + equippable: RmrkCore::get_nft_property_decoded(collection_id, token_id, RmrkProperty::EquippableList).ok()?, })), _ => None } @@ -415,7 +411,7 @@ match nft_type { Theme => Some( - RmrkCore::get_nft_property(collection_id, *token_id, RmrkProperty::ThemeName).unwrap().decode_or_default() + RmrkCore::get_nft_property_decoded(collection_id, *token_id, RmrkProperty::ThemeName).unwrap() ), _ => None } @@ -441,9 +437,9 @@ .find_map(|token_id| { RmrkCore::ensure_nft_type(collection_id, token_id, NftType::Theme).ok()?; - let name: RmrkString = RmrkCore::get_nft_property( + let name: RmrkString = RmrkCore::get_nft_property_decoded( collection_id, token_id, RmrkProperty::ThemeName - ).ok()?.decode_or_default(); + ).ok()?; if name == theme_name { Some((name, token_id)) @@ -467,11 +463,11 @@ } )?; - let inherit = RmrkCore::get_nft_property( + let inherit = RmrkCore::get_nft_property_decoded( collection_id, theme_id, RmrkProperty::ThemeInherit - )?.decode_or_default(); + )?; let theme = RmrkTheme { name, -- gitstuff