refactor: Move total asset quantity to asset definition (#5029)

* refactor: integrate the `asset_total_quantities` map into the asset definition itself Signed-off-by: ⭐️NINIKA⭐️ <[email protected]> * refactor(queries): remove FindTotalAssetQuantityByAssetDefinitionId and replace its usages Signed-off-by: ⭐️NINIKA⭐️ <[email protected]> --------- Signed-off-by: ⭐️NINIKA⭐️ <[email protected]>
hyperledger-iroha · Sep 3, 2024 · 6613210 · 6613210
1 parent 2fb551f
commit 6613210
Show file tree

Hide file tree

Showing 14 changed files with 71 additions and 162 deletions.
diff --git a/client/tests/integration/queries/asset.rs b/client/tests/integration/queries/asset.rs
@@ -1,13 +1,9 @@
 use eyre::Result;
 use iroha::{
     client::{Client, QueryError},
-    data_model::{
-        asset::AssetValue,
-        isi::Instruction,
-        prelude::*,
-        query::{asset::FindTotalAssetQuantityByAssetDefinitionId, error::QueryExecutionFail},
-    },
+    data_model::{asset::AssetValue, isi::Instruction, prelude::*},
 };
+use iroha_data_model::query::builder::SingleQueryError;
 use test_network::*;
 use test_samples::{gen_account_in, ALICE_ID};
 
@@ -76,10 +72,17 @@ fn find_asset_total_quantity() -> Result<()> {
         .map(|account_id| AssetId::new(definition_id.clone(), account_id))
         .collect::<Vec<_>>();
 
+    let get_quantity = || -> Result<Numeric, SingleQueryError<QueryError>> {
+        Ok(test_client
+            .query(FindAssetsDefinitions::new())
+            .filter_with(|asset_definition| asset_definition.id.eq(definition_id.clone()))
+            .execute_single()?
+            .total_quantity)
+    };
+
     // Assert that initial total quantity before any registrations and unregistrations is zero
-    let initial_total_asset_quantity = test_client.query_single(
-        FindTotalAssetQuantityByAssetDefinitionId::new(definition_id.clone()),
-    )?;
+    let initial_total_asset_quantity = get_quantity()?;
+
     assert!(initial_total_asset_quantity.is_zero());
 
     let register_assets = asset_ids
@@ -91,9 +94,7 @@ fn find_asset_total_quantity() -> Result<()> {
     test_client.submit_all_blocking(register_assets)?;
 
     // Assert that total quantity is equal to number of registrations
-    let result = test_client.query_single(FindTotalAssetQuantityByAssetDefinitionId::new(
-        definition_id.clone(),
-    ))?;
+    let result = get_quantity()?;
     assert_eq!(numeric!(5), result);
 
     let unregister_assets = asset_ids
@@ -104,24 +105,15 @@ fn find_asset_total_quantity() -> Result<()> {
     test_client.submit_all_blocking(unregister_assets)?;
 
     // Assert that total asset quantity is zero after unregistering asset from all accounts
-    let total_asset_quantity = test_client.query_single(
-        FindTotalAssetQuantityByAssetDefinitionId::new(definition_id.clone()),
-    )?;
+    let total_asset_quantity = get_quantity()?;
     assert!(total_asset_quantity.is_zero());
 
     // Unregister asset definition
     test_client.submit_blocking(Unregister::asset_definition(definition_id.clone()))?;
 
     // Assert that total asset quantity cleared with unregistering of asset definition
-    let result = test_client.query_single(FindTotalAssetQuantityByAssetDefinitionId::new(
-        definition_id,
-    ));
-    assert!(matches!(
-        result,
-        Err(QueryError::Validation(ValidationFail::QueryFailed(
-            QueryExecutionFail::Find(_)
-        )))
-    ));
+    let result = get_quantity();
+    assert!(matches!(result, Err(SingleQueryError::ExpectedOneGotNone)));
 
     Ok(())
 }
@@ -156,10 +148,16 @@ where
         .map(|account_id| AssetId::new(definition_id.clone(), account_id))
         .collect::<Vec<_>>();
 
+    let get_quantity = || -> Result<Numeric, SingleQueryError<QueryError>> {
+        Ok(test_client
+            .query(FindAssetsDefinitions::new())
+            .filter_with(|asset_definition| asset_definition.id.eq(definition_id.clone()))
+            .execute_single()?
+            .total_quantity)
+    };
+
     // Assert that initial total quantity before any burns and mints is zero
-    let initial_total_asset_quantity = test_client.query_single(
-        FindTotalAssetQuantityByAssetDefinitionId::new(definition_id.clone()),
-    )?;
+    let initial_total_asset_quantity = get_quantity()?;
     assert!(initial_total_asset_quantity.is_zero());
 
     let register_assets = asset_ids
@@ -184,9 +182,7 @@ where
     test_client.submit_all_blocking(burn_assets)?;
 
     // Assert that total asset quantity is equal to: `n_accounts * (initial_value + to_mint - to_burn)`
-    let total_asset_quantity = test_client.query_single(
-        FindTotalAssetQuantityByAssetDefinitionId::new(definition_id.clone()),
-    )?;
+    let total_asset_quantity = get_quantity()?;
     assert_eq!(expected_total_asset_quantity, total_asset_quantity);
 
     let unregister_assets = asset_ids
@@ -197,24 +193,15 @@ where
     test_client.submit_all_blocking(unregister_assets)?;
 
     // Assert that total asset quantity is zero after unregistering asset from all accounts
-    let total_asset_quantity = test_client.query_single(
-        FindTotalAssetQuantityByAssetDefinitionId::new(definition_id.clone()),
-    )?;
+    let total_asset_quantity = get_quantity()?;
     assert!(total_asset_quantity.is_zero());
 
     // Unregister asset definition
     test_client.submit_blocking(Unregister::asset_definition(definition_id.clone()))?;
 
     // Assert that total asset quantity cleared with unregistering of asset definition
-    let result = test_client.query_single(FindTotalAssetQuantityByAssetDefinitionId::new(
-        definition_id,
-    ));
-    assert!(matches!(
-        result,
-        Err(QueryError::Validation(ValidationFail::QueryFailed(
-            QueryExecutionFail::Find(_)
-        )))
-    ));
+    let result = get_quantity();
+    assert!(matches!(result, Err(SingleQueryError::ExpectedOneGotNone)));
 
     Ok(())
 }
diff --git a/core/src/smartcontracts/isi/asset.rs b/core/src/smartcontracts/isi/asset.rs
@@ -23,6 +23,7 @@ impl Registrable for NewAssetDefinition {
             logo: self.logo,
             metadata: self.metadata,
             owned_by: authority.clone(),
+            total_quantity: Numeric::ZERO,
         }
     }
 }
@@ -492,16 +493,6 @@ pub mod query {
         }
     }
 
-    impl ValidSingularQuery for FindTotalAssetQuantityByAssetDefinitionId {
-        #[metrics(+"find_total_asset_quantity_by_asset_definition_id")]
-        fn execute(&self, state_ro: &impl StateReadOnly) -> Result<Numeric, Error> {
-            let id = &self.id;
-            iroha_logger::trace!(%id);
-            let asset_value = state_ro.world().asset_total_amount(id)?;
-            Ok(asset_value)
-        }
-    }
-
     impl ValidSingularQuery for FindAssetMetadata {
         #[metrics(+"find_asset_key_value_by_id_and_key")]
         fn execute(&self, state_ro: &impl StateReadOnly) -> Result<JsonString, Error> {

diff --git a/core/src/smartcontracts/isi/domain.rs b/core/src/smartcontracts/isi/domain.rs
@@ -154,11 +154,6 @@ pub mod isi {
                 .world
                 .domain(&asset_definition_id.domain)?;
 
-            state_transaction
-                .world
-                .asset_total_quantities
-                .insert(asset_definition_id.clone(), Numeric::ZERO);
-
             state_transaction
                 .world
                 .asset_definitions
@@ -220,11 +215,6 @@ pub mod isi {
                 .world
                 .domain(&asset_definition_id.domain)?;
 
-            state_transaction
-                .world
-                .asset_total_quantities
-                .remove(asset_definition_id.clone());
-
             events.push(DataEvent::from(AssetDefinitionEvent::Deleted(
                 asset_definition_id,
             )));

diff --git a/core/src/smartcontracts/isi/query.rs b/core/src/smartcontracts/isi/query.rs
@@ -245,9 +245,6 @@ impl ValidQueryRequest {
                     SingularQueryBox::FindParameters(q) => {
                         SingularQueryOutputBox::from(q.execute(state)?)
                     }
-                    SingularQueryBox::FindTotalAssetQuantityByAssetDefinitionId(q) => {
-                        SingularQueryOutputBox::from(q.execute(state)?)
-                    }
                     SingularQueryBox::FindTriggerById(q) => {
                         SingularQueryOutputBox::from(q.execute(state)?)
                     }

diff --git a/core/src/smartcontracts/isi/world.rs b/core/src/smartcontracts/isi/world.rs
@@ -169,10 +169,6 @@ pub mod isi {
                     .world
                     .asset_definitions
                     .remove(asset_definition_id.clone());
-                state_transaction
-                    .world
-                    .asset_total_quantities
-                    .remove(asset_definition_id);
             }
 
             if state_transaction

diff --git a/core/src/state.rs b/core/src/state.rs
@@ -73,8 +73,6 @@ pub struct World {
     pub(crate) accounts: Storage<AccountId, Account>,
     /// Registered asset definitions.
     pub(crate) asset_definitions: Storage<AssetDefinitionId, AssetDefinition>,
-    /// Registered asset definition total amounts.
-    pub(crate) asset_total_quantities: Storage<AssetDefinitionId, Numeric>,
     /// Registered assets.
     pub(crate) assets: Storage<AssetId, Asset>,
     /// Roles. [`Role`] pairs.
@@ -103,8 +101,6 @@ pub struct WorldBlock<'world> {
     pub(crate) accounts: StorageBlock<'world, AccountId, Account>,
     /// Registered asset definitions.
     pub(crate) asset_definitions: StorageBlock<'world, AssetDefinitionId, AssetDefinition>,
-    /// Registered asset definition total amounts.
-    pub(crate) asset_total_quantities: StorageBlock<'world, AssetDefinitionId, Numeric>,
     /// Registered assets.
     pub(crate) assets: StorageBlock<'world, AssetId, Asset>,
     /// Roles. [`Role`] pairs.
@@ -138,9 +134,6 @@ pub struct WorldTransaction<'block, 'world> {
     /// Registered asset definitions.
     pub(crate) asset_definitions:
         StorageTransaction<'block, 'world, AssetDefinitionId, AssetDefinition>,
-    /// Registered asset definition total amounts.
-    pub(crate) asset_total_quantities:
-        StorageTransaction<'block, 'world, AssetDefinitionId, Numeric>,
     /// Registered assets.
     pub(crate) assets: StorageTransaction<'block, 'world, AssetId, Asset>,
     /// Roles. [`Role`] pairs.
@@ -181,8 +174,6 @@ pub struct WorldView<'world> {
     pub(crate) accounts: StorageView<'world, AccountId, Account>,
     /// Registered asset definitions.
     pub(crate) asset_definitions: StorageView<'world, AssetDefinitionId, AssetDefinition>,
-    /// Registered asset definition total amounts.
-    pub(crate) asset_total_quantities: StorageView<'world, AssetDefinitionId, Numeric>,
     /// Registered assets.
     pub(crate) assets: StorageView<'world, AssetId, Asset>,
     /// Roles. [`Role`] pairs.
@@ -367,7 +358,6 @@ impl World {
             domains: self.domains.block(),
             accounts: self.accounts.block(),
             asset_definitions: self.asset_definitions.block(),
-            asset_total_quantities: self.asset_total_quantities.block(),
             assets: self.assets.block(),
             roles: self.roles.block(),
             account_permissions: self.account_permissions.block(),
@@ -389,7 +379,6 @@ impl World {
             domains: self.domains.block_and_revert(),
             accounts: self.accounts.block_and_revert(),
             asset_definitions: self.asset_definitions.block_and_revert(),
-            asset_total_quantities: self.asset_total_quantities.block_and_revert(),
             assets: self.assets.block_and_revert(),
             roles: self.roles.block_and_revert(),
             account_permissions: self.account_permissions.block_and_revert(),
@@ -411,7 +400,6 @@ impl World {
             domains: self.domains.view(),
             accounts: self.accounts.view(),
             asset_definitions: self.asset_definitions.view(),
-            asset_total_quantities: self.asset_total_quantities.view(),
             assets: self.assets.view(),
             roles: self.roles.view(),
             account_permissions: self.account_permissions.view(),
@@ -433,7 +421,6 @@ pub trait WorldReadOnly {
     fn domains(&self) -> &impl StorageReadOnly<DomainId, Domain>;
     fn accounts(&self) -> &impl StorageReadOnly<AccountId, Account>;
     fn asset_definitions(&self) -> &impl StorageReadOnly<AssetDefinitionId, AssetDefinition>;
-    fn asset_total_quantities(&self) -> &impl StorageReadOnly<AssetDefinitionId, Numeric>;
     fn assets(&self) -> &impl StorageReadOnly<AssetId, Asset>;
     fn roles(&self) -> &impl StorageReadOnly<RoleId, Role>;
     fn account_permissions(&self) -> &impl StorageReadOnly<AccountId, Permissions>;
@@ -666,10 +653,7 @@ pub trait WorldReadOnly {
     /// # Errors
     /// - Asset definition not found
     fn asset_total_amount(&self, definition_id: &AssetDefinitionId) -> Result<Numeric, FindError> {
-        self.asset_total_quantities()
-            .get(definition_id)
-            .ok_or_else(|| FindError::AssetDefinition(definition_id.clone()))
-            .copied()
+        Ok(self.asset_definition(definition_id)?.total_quantity)
     }
 
     /// Get an immutable iterator over the [`PeerId`]s.
@@ -702,9 +686,6 @@ macro_rules! impl_world_ro {
             fn asset_definitions(&self) -> &impl StorageReadOnly<AssetDefinitionId, AssetDefinition> {
                 &self.asset_definitions
             }
-            fn asset_total_quantities(&self) -> &impl StorageReadOnly<AssetDefinitionId, Numeric> {
-                &self.asset_total_quantities
-            }
             fn assets(&self) -> &impl StorageReadOnly<AssetId, Asset> {
                 &self.assets
             }
@@ -749,7 +730,6 @@ impl<'world> WorldBlock<'world> {
             domains: self.domains.transaction(),
             accounts: self.accounts.transaction(),
             asset_definitions: self.asset_definitions.transaction(),
-            asset_total_quantities: self.asset_total_quantities.transaction(),
             assets: self.assets.transaction(),
             roles: self.roles.transaction(),
             account_permissions: self.account_permissions.transaction(),
@@ -773,7 +753,6 @@ impl<'world> WorldBlock<'world> {
             domains,
             accounts,
             asset_definitions,
-            asset_total_quantities,
             assets,
             roles,
             account_permissions,
@@ -792,7 +771,6 @@ impl<'world> WorldBlock<'world> {
         account_permissions.commit();
         roles.commit();
         assets.commit();
-        asset_total_quantities.commit();
         asset_definitions.commit();
         accounts.commit();
         domains.commit();
@@ -811,7 +789,6 @@ impl WorldTransaction<'_, '_> {
             domains,
             accounts,
             asset_definitions,
-            asset_total_quantities,
             assets,
             roles,
             account_permissions,
@@ -829,7 +806,6 @@ impl WorldTransaction<'_, '_> {
         account_permissions.apply();
         roles.apply();
         assets.apply();
-        asset_total_quantities.apply();
         asset_definitions.apply();
         accounts.apply();
         domains.apply();
@@ -967,10 +943,8 @@ impl WorldTransaction<'_, '_> {
         increment: Numeric,
     ) -> Result<(), Error> {
         let asset_total_amount: &mut Numeric =
-            self.asset_total_quantities.get_mut(definition_id).expect(
-                "INTERNAL BUG: Asset total amount not found. \
-                Insert initial total amount on `Register<AssetDefinition>`",
-            );
+            &mut self.asset_definition_mut(definition_id)?.total_quantity;
+
         *asset_total_amount = asset_total_amount
             .checked_add(increment)
             .ok_or(MathError::Overflow)?;
@@ -999,10 +973,8 @@ impl WorldTransaction<'_, '_> {
         decrement: Numeric,
     ) -> Result<(), Error> {
         let asset_total_amount: &mut Numeric =
-            self.asset_total_quantities.get_mut(definition_id).expect(
-                "INTERNAL BUG: Asset total amount not found. \
-                Insert initial total amount on `Register<AssetDefinition>`",
-            );
+            &mut self.asset_definition_mut(definition_id)?.total_quantity;
+
         *asset_total_amount = asset_total_amount
             .checked_sub(decrement)
             .ok_or(MathError::NotEnoughQuantity)?;
@@ -1967,7 +1939,6 @@ pub(crate) mod deserialize {
                     let mut domains = None;
                     let mut accounts = None;
                     let mut asset_definitions = None;
-                    let mut asset_total_quantities = None;
                     let mut assets = None;
                     let mut roles = None;
                     let mut account_permissions = None;
@@ -1993,9 +1964,6 @@ pub(crate) mod deserialize {
                             "asset_definitions" => {
                                 asset_definitions = Some(map.next_value()?);
                             }
-                            "asset_total_quantities" => {
-                                asset_total_quantities = Some(map.next_value()?);
-                            }
                             "assets" => {
                                 assets = Some(map.next_value()?);
                             }
@@ -2036,9 +2004,6 @@ pub(crate) mod deserialize {
                             .ok_or_else(|| serde::de::Error::missing_field("accounts"))?,
                         asset_definitions: asset_definitions
                             .ok_or_else(|| serde::de::Error::missing_field("asset_definitions"))?,
-                        asset_total_quantities: asset_total_quantities.ok_or_else(|| {
-                            serde::de::Error::missing_field("asset_total_quantities")
-                        })?,
                         assets: assets.ok_or_else(|| serde::de::Error::missing_field("assets"))?,
                         roles: roles.ok_or_else(|| serde::de::Error::missing_field("roles"))?,
                         account_permissions: account_permissions.ok_or_else(|| {