[fix] #2501: Fixes the order of assets in queries with pagination

Signed-off-by: Vladimir Pesterev <pesterev@pm.me>

client/tests/integration/asset.rs

@@ -235,3 +235,74 @@ fn client_add_asset_with_name_length_more_than_limit_should_not_commit_transacti
 
     Ok(())
 }
+
+#[test]
+fn correct_pagination_assets_after_creating_new_one() -> Result<()> {
+    let (_rt, _peer, test_client) = <PeerBuilder>::new().start_with_runtime();
+
+    let account_id = AccountId::from_str("alice@wonderland")?;
+
+    let mut assets = vec![];
+    let mut instructions: Vec<Instruction> = vec![];
+
+    for i in 0..10 {
+        let asset_definition_id = AssetDefinitionId::from_str(&format!("xor{}#wonderland", i))?;
+        let asset_definition = AssetDefinition::quantity(asset_definition_id.clone());
+        let asset = Asset::new(
+            AssetId::new(asset_definition_id, account_id.clone()),
+            AssetValue::Quantity(0),
+        );
+
+        assets.push(asset.clone());
+
+        let create_asset_definition = RegisterBox::new(asset_definition);
+        let create_asset = RegisterBox::new(asset);
+
+        instructions.push(create_asset_definition.into());
+        instructions.push(create_asset.into());
+    }
+
+    test_client.submit_all_blocking(instructions)?;
+
+    let res = test_client.request_with_pagination(
+        client::asset::by_account_id(account_id.clone()),
+        Pagination::new(Some(1), Some(5)),
+    )?;
+
+    assert_eq!(
+        res.output
+            .iter()
+            .map(|asset| asset.id().definition_id.name.clone())
+            .collect::<Vec<_>>(),
+        assets
+            .iter()
+            .take(5)
+            .map(|asset| asset.id().definition_id.name.clone())
+            .collect::<Vec<_>>()
+    );
+
+    let new_asset_definition_id = AssetDefinitionId::from_str("xor10#wonderland")?;
+    let new_asset_definition = AssetDefinition::quantity(new_asset_definition_id.clone());
+    let new_asset = Asset::new(
+        AssetId::new(new_asset_definition_id, account_id.clone()),
+        AssetValue::Quantity(0),
+    );
+
+    let create_asset_definition = RegisterBox::new(new_asset_definition);
+    let create_asset = RegisterBox::new(new_asset.clone());
+
+    test_client.submit_all_blocking(vec![create_asset_definition.into(), create_asset.into()])?;
+
+    let res = test_client.request_with_pagination(
+        client::asset::by_account_id(account_id),
+        Pagination::new(Some(6), None),
+    )?;
+
+    let mut right = assets.into_iter().skip(5).take(5).collect::<Vec<_>>();
+
+    right.push(new_asset);
+
+    assert_eq!(res.output.into_iter().collect::<Vec<_>>(), right);
+
+    Ok(())
+}

data_model/Cargo.toml

@@ -47,6 +47,7 @@ warp = { version = "0.3", default-features = false, optional = true }
 thiserror = { version = "1.0.28", optional = true }
 getset = "0.1.2"
 strum = { version = "0.24.0", default-features = false, features = ["derive"] }
+chrono = { version = "0.4.19", default-features = false, features = ["alloc"] }
 
 [dev-dependencies]
 iroha_core = { path = "../core", version = "=2.0.0-pre-rc.5" }

data_model/src/account.rs

@@ -11,6 +11,7 @@ use core::str::FromStr;
 #[cfg(feature = "std")]
 use std::collections::{btree_map, btree_set};
 
+use chrono::Utc;
 use derive_more::Display;
 use getset::{Getters, MutGetters, Setters};
 use iroha_data_model_derive::IdOrdEqHash;
@@ -23,7 +24,7 @@ use serde::{Deserialize, Serialize};
 #[cfg(feature = "mutable_api")]
 use crate::Registrable;
 use crate::{
-    asset::{prelude::AssetId, AssetsMap},
+    asset::{prelude::AssetId, AssetsMap, AssetsMapKey, AssetsMapKeyMap},
     domain::prelude::*,
     expression::{ContainsAny, ContextValue, EvaluatesTo},
     metadata::Metadata,
@@ -153,6 +154,7 @@ impl Registrable for NewAccount {
             id: self.id,
             signatories: self.signatories,
             assets: AssetsMap::default(),
+            assets_keys: AssetsMapKeyMap::default(),
             permission_tokens: Permissions::default(),
             signature_check_condition: SignatureCheckCondition::default(),
             metadata: self.metadata,
@@ -183,6 +185,22 @@ impl NewAccount {
     pub(crate) fn id(&self) -> &<Account as Identifiable>::Id {
         &self.id
     }
+
+    /// Construct [`Account`]
+    #[must_use]
+    #[cfg(feature = "mutable_api")]
+    pub fn build(self) -> Account {
+        Account {
+            id: self.id,
+            signatories: self.signatories,
+            assets_keys: AssetsMapKeyMap::default(),
+            assets: AssetsMap::default(),
+            permission_tokens: Permissions::default(),
+            signature_check_condition: SignatureCheckCondition::default(),
+            metadata: self.metadata,
+            roles: RoleIds::default(),
+        }
+    }
 }
 
 #[cfg_attr(feature = "ffi", ffi_export)]
@@ -217,6 +235,8 @@ impl NewAccount {
 pub struct Account {
     /// An Identification of the [`Account`].
     id: <Self as Identifiable>::Id,
+    /// [`AssetsMap`] keys.
+    assets_keys: AssetsMapKeyMap,
     /// Asset's in this [`Account`].
     assets: AssetsMap,
     /// [`Account`]'s signatories.
@@ -264,7 +284,8 @@ impl Account {
     /// Return a reference to the [`Asset`] corresponding to the asset id.
     #[inline]
     pub fn asset(&self, asset_id: &AssetId) -> Option<&Asset> {
-        self.assets.get(asset_id)
+        let assets_map_key = self.assets_keys.get(asset_id)?;
+        self.assets.get(assets_map_key)
     }
 
     /// Get an iterator over [`Asset`]s of the `Account`
@@ -324,19 +345,24 @@ impl Account {
     /// Return a mutable reference to the [`Asset`] corresponding to the asset id
     #[inline]
     pub fn asset_mut(&mut self, asset_id: &AssetId) -> Option<&mut Asset> {
-        self.assets.get_mut(asset_id)
+        let key = self.assets_keys.get(asset_id)?;
+        self.assets.get_mut(key)
     }
 
     /// 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)
+        let datetime = Utc::now();
+        let key = AssetsMapKey::new(asset.id().clone(), datetime.timestamp_millis());
+        self.assets_keys.insert(asset.id().clone(), key.clone());
+        self.assets.insert(key, asset)
     }
 
     /// Remove asset from the [`Account`] and return it
     #[inline]
     pub fn remove_asset(&mut self, asset_id: &AssetId) -> Option<Asset> {
-        self.assets.remove(asset_id)
+        let key = self.assets_keys.remove(asset_id)?;
+        self.assets.remove(&key)
     }
 
     /// Add [`permission`](PermissionToken) into the [`Account`].

data_model/src/asset.rs

@@ -24,9 +24,71 @@ use crate::{
     ParseError, Registered, TryAsMut, TryAsRef, Value,
 };
 
-/// [`AssetsMap`] provides an API to work with collection of key ([`Id`]) - value
+#[derive(Debug, Clone, Decode, Encode, Deserialize, Serialize, IntoSchema)]
+/// Key of [`AssetsMap`]
+pub struct AssetsMapKey {
+    /// [`Id`] of inserted [`Asset`]
+    pub asset_id: <Asset as Identifiable>::Id,
+    /// Insertion timestamp
+    pub timestamp: i64,
+}
+
+impl AssetsMapKey {
+    /// Creates new [`AssetsMapKey`] from [`Asset`]
+    #[inline]
+    pub const fn new(asset_id: <Asset as Identifiable>::Id, timestamp: i64) -> Self {
+        Self {
+            asset_id,
+            timestamp,
+        }
+    }
+}
+
+impl PartialEq for AssetsMapKey {
+    #[inline]
+    fn eq(&self, other: &Self) -> bool {
+        self.asset_id.eq(&other.asset_id)
+    }
+}
+
+impl Eq for AssetsMapKey {}
+
+impl PartialOrd for AssetsMapKey {
+    #[inline]
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        match (
+            self.asset_id.cmp(&other.asset_id),
+            self.timestamp.cmp(&other.timestamp),
+        ) {
+            (Ordering::Equal, _) => Some(Ordering::Equal),
+            (ord, Ordering::Equal) => Some(ord),
+            (_, Ordering::Less) => Some(Ordering::Less),
+            (_, Ordering::Greater) => Some(Ordering::Greater),
+        }
+    }
+}
+
+impl Ord for AssetsMapKey {
+    #[inline]
+    fn cmp(&self, other: &Self) -> Ordering {
+        match (
+            self.asset_id.cmp(&other.asset_id),
+            self.timestamp.cmp(&other.timestamp),
+        ) {
+            (Ordering::Equal, _) => Ordering::Equal,
+            (ord, Ordering::Equal) => ord,
+            (_, Ordering::Less) => Ordering::Less,
+            (_, Ordering::Greater) => Ordering::Greater,
+        }
+    }
+}
+
+/// [`AssetsMapKeyMap`] stores pairs of [`Id`] and [`AssetsMapKey`].
+pub type AssetsMapKeyMap = btree_map::BTreeMap<<Asset as Identifiable>::Id, AssetsMapKey>;
+
+/// [`AssetsMap`] provides an API to work with collection of key ([`AssetsMapKey`]) - value
 /// ([`Asset`]) pairs.
-pub type AssetsMap = btree_map::BTreeMap<<Asset as Identifiable>::Id, Asset>;
+pub type AssetsMap = btree_map::BTreeMap<AssetsMapKey, Asset>;
 
 /// [`AssetDefinitionsMap`] provides an API to work with collection of key ([`DefinitionId`]) - value
 /// (`AssetDefinition`) pairs.

data_model/src/pagination.rs

@@ -247,6 +247,6 @@ mod tests {
                 .paginate(Pagination::new(Some(1), Some(1)))
                 .collect::<Vec<_>>(),
             vec![2_i32]
-        )
+        );
     }
 }

tools/parity_scale_decoder/samples/account.json

@@ -5,6 +5,7 @@
             "name": "wonderland"
         }
     },
+    "assets_keys": {},
     "assets": {},
     "signatories": [],
     "permission_tokens": [],