refactor: remove index Assets in AssetsMap by AssetDefinitionId

AssetsMap stores assets for a particular account, so indexing them via AssetId (which is AccountId + AssetDefinitionId) is redundant Also remove some unnecessary AccoundId clones Signed-off-by: ⭐️NINIKA⭐️ <[email protected]>
hyperledger-iroha · Jun 6, 2024 · ca6ed1f · ca6ed1f
1 parent c8b8d7b
commit ca6ed1f
Show file tree

Hide file tree

Showing 7 changed files with 40 additions and 43 deletions.
diff --git a/core/src/smartcontracts/isi/account.rs b/core/src/smartcontracts/isi/account.rs
@@ -95,14 +95,13 @@ pub mod isi {
             state_transaction: &mut StateTransaction<'_, '_>,
         ) -> Result<(), Error> {
             let asset_id = self.object_id;
-            let account_id = asset_id.account_id.clone();
 
             let asset = state_transaction
                 .world
-                .account_mut(&account_id)
+                .account_mut(&asset_id.account_id)
                 .and_then(|account| {
                     account
-                        .remove_asset(&asset_id)
+                        .remove_asset(&asset_id.definition_id)
                         .ok_or_else(|| FindError::Asset(asset_id))
                 })?;
 
@@ -613,9 +612,7 @@ pub mod query {
                     .world()
                     .map_domain(&asset_definition_id.domain_id.clone(), move |domain| {
                         domain.accounts.values().filter(move |account| {
-                            let asset_id =
-                                AssetId::new(asset_definition_id.clone(), account.id().clone());
-                            account.assets.contains_key(&asset_id)
+                            account.assets.contains_key(&asset_definition_id)
                         })
                     })?
                     .cloned(),

diff --git a/core/src/smartcontracts/isi/asset.rs b/core/src/smartcontracts/isi/asset.rs
@@ -143,14 +143,13 @@ pub mod isi {
                 state_transaction,
                 expected_asset_value_type_store,
             )?;
-            let account_id = asset_id.account_id.clone();
 
             let asset = state_transaction
                 .world
-                .account_mut(&account_id)
+                .account_mut(&asset_id.account_id)
                 .and_then(|account| {
                     account
-                        .remove_asset(&asset_id)
+                        .remove_asset(&asset_id.definition_id)
                         .ok_or_else(|| FindError::Asset(asset_id.clone()))
                 })?;
 
@@ -239,7 +238,7 @@ pub mod isi {
             let account = state_transaction.world.account_mut(&asset_id.account_id)?;
             let asset = account
                 .assets
-                .get_mut(&asset_id)
+                .get_mut(&asset_id.definition_id)
                 .ok_or_else(|| FindError::Asset(asset_id.clone()))?;
             let AssetValue::Numeric(quantity) = &mut asset.value else {
                 return Err(Error::Conversion("Expected numeric asset type".to_owned()));
@@ -249,7 +248,7 @@ pub mod isi {
                 .ok_or(MathError::NotEnoughQuantity)?;
 
             if asset.value.is_zero_value() {
-                assert!(account.remove_asset(&asset_id).is_some());
+                assert!(account.remove_asset(&asset_id.definition_id).is_some());
             }
 
             #[allow(clippy::float_arithmetic)]
@@ -295,7 +294,7 @@ pub mod isi {
                 let account = state_transaction.world.account_mut(&source_id.account_id)?;
                 let asset = account
                     .assets
-                    .get_mut(&source_id)
+                    .get_mut(&source_id.definition_id)
                     .ok_or_else(|| FindError::Asset(source_id.clone()))?;
                 let AssetValue::Numeric(quantity) = &mut asset.value else {
                     return Err(Error::Conversion("Expected numeric asset type".to_owned()));
@@ -304,7 +303,7 @@ pub mod isi {
                     .checked_sub(self.object)
                     .ok_or(MathError::NotEnoughQuantity)?;
                 if asset.value.is_zero_value() {
-                    assert!(account.remove_asset(&source_id).is_some());
+                    assert!(account.remove_asset(&source_id.definition_id).is_some());
                 }
             }
 

diff --git a/core/src/smartcontracts/isi/domain.rs b/core/src/smartcontracts/isi/domain.rs
@@ -188,11 +188,10 @@ pub mod isi {
 
             let mut events = Vec::with_capacity(assets_to_remove.len() + 1);
             for asset_id in assets_to_remove {
-                let account_id = asset_id.account_id.clone();
                 if state_transaction
                     .world
-                    .account_mut(&account_id)?
-                    .remove_asset(&asset_id)
+                    .account_mut(&asset_id.account_id)?
+                    .remove_asset(&asset_id.definition_id)
                     .is_none()
                 {
                     error!(%asset_id, "asset not found. This is a bug");

diff --git a/core/src/state.rs b/core/src/state.rs
@@ -410,7 +410,8 @@ pub trait WorldReadOnly {
     fn account_assets(
         &self,
         id: &AccountId,
-    ) -> Result<std::collections::btree_map::Values<'_, AssetId, Asset>, QueryExecutionFail> {
+    ) -> Result<std::collections::btree_map::Values<'_, AssetDefinitionId, Asset>, QueryExecutionFail>
+    {
         self.map_account(id, |account| account.assets.values())
     }
 
@@ -493,7 +494,7 @@ pub trait WorldReadOnly {
             |account| -> Result<Asset, QueryExecutionFail> {
                 account
                     .assets
-                    .get(id)
+                    .get(&id.definition_id)
                     .ok_or_else(|| QueryExecutionFail::from(FindError::Asset(id.clone())))
                     .cloned()
             },
@@ -708,7 +709,7 @@ impl WorldTransaction<'_, '_> {
         self.account_mut(&id.account_id).and_then(move |account| {
             account
                 .assets
-                .get_mut(id)
+                .get_mut(&id.definition_id)
                 .ok_or_else(|| FindError::Asset(id.clone()))
         })
     }
@@ -747,15 +748,18 @@ impl WorldTransaction<'_, '_> {
             .get_mut(account_id)
             .ok_or(FindError::Account(account_id.clone()))?;
 
-        Ok(account.assets.entry(asset_id.clone()).or_insert_with(|| {
-            let asset = Asset::new(asset_id, default_asset_value.into());
-            Self::emit_events_impl(
-                &mut self.triggers,
-                &mut self.events_buffer,
-                Some(AccountEvent::Asset(AssetEvent::Created(asset.clone()))),
-            );
-            asset
-        }))
+        Ok(account
+            .assets
+            .entry(asset_id.definition_id.clone())
+            .or_insert_with(|| {
+                let asset = Asset::new(asset_id, default_asset_value.into());
+                Self::emit_events_impl(
+                    &mut self.triggers,
+                    &mut self.events_buffer,
+                    Some(AccountEvent::Asset(AssetEvent::Created(asset.clone()))),
+                );
+                asset
+            }))
     }
 
     /// Get mutable reference to [`AssetDefinition`]

diff --git a/data_model/src/account.rs b/data_model/src/account.rs
@@ -15,10 +15,7 @@ use serde_with::{DeserializeFromStr, SerializeDisplay};
 
 pub use self::model::*;
 use crate::{
-    asset::{
-        prelude::{Asset, AssetId},
-        AssetsMap,
-    },
+    asset::{Asset, AssetDefinitionId, AssetsMap},
     domain::prelude::*,
     metadata::Metadata,
     HasMetadata, Identifiable, ParseError, PublicKey, Registered,
@@ -137,7 +134,7 @@ impl Account {
 
     /// Return a reference to the [`Asset`] corresponding to the asset id.
     #[inline]
-    pub fn asset(&self, asset_id: &AssetId) -> Option<&Asset> {
+    pub fn asset(&self, asset_id: &AssetDefinitionId) -> Option<&Asset> {
         self.assets.get(asset_id)
     }
 
@@ -153,12 +150,13 @@ impl Account {
     /// Add [`Asset`] into the [`Account`] returning previous asset stored under the same id
     #[inline]
     pub fn add_asset(&mut self, asset: Asset) -> Option<Asset> {
-        self.assets.insert(asset.id().clone(), asset)
+        assert_eq!(self.id, asset.id.account_id);
+        self.assets.insert(asset.id.definition_id.clone(), asset)
     }
 
     /// Remove asset from the [`Account`] and return it
     #[inline]
-    pub fn remove_asset(&mut self, asset_id: &AssetId) -> Option<Asset> {
+    pub fn remove_asset(&mut self, asset_id: &AssetDefinitionId) -> Option<Asset> {
         self.assets.remove(asset_id)
     }
 }

diff --git a/data_model/src/asset.rs b/data_model/src/asset.rs
@@ -22,7 +22,7 @@ use crate::{
 };
 
 /// API to work with collections of [`Id`] : [`Asset`] mappings.
-pub type AssetsMap = btree_map::BTreeMap<AssetId, Asset>;
+pub type AssetsMap = btree_map::BTreeMap<AssetDefinitionId, Asset>;
 
 /// [`AssetDefinitionsMap`] provides an API to work with collection of key([`AssetDefinitionId`])-value([`AssetDefinition`])
 /// pairs.

diff --git a/docs/source/references/schema.json b/docs/source/references/schema.json
@@ -7,7 +7,7 @@
       },
       {
         "name": "assets",
-        "type": "SortedMap<AssetId, Asset>"
+        "type": "SortedMap<AssetDefinitionId, Asset>"
       },
       {
         "name": "metadata",
@@ -3703,6 +3703,12 @@
       "value": "Account"
     }
   },
+  "SortedMap<AssetDefinitionId, Asset>": {
+    "Map": {
+      "key": "AssetDefinitionId",
+      "value": "Asset"
+    }
+  },
   "SortedMap<AssetDefinitionId, AssetDefinition>": {
     "Map": {
       "key": "AssetDefinitionId",
@@ -3715,12 +3721,6 @@
       "value": "Numeric"
     }
   },
-  "SortedMap<AssetId, Asset>": {
-    "Map": {
-      "key": "AssetId",
-      "value": "Asset"
-    }
-  },
   "SortedMap<Name, MetadataValueBox>": {
     "Map": {
       "key": "Name",