Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

71 changes: 71 additions & 0 deletions client/tests/integration/asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}
1 change: 1 addition & 0 deletions data_model/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down
36 changes: 31 additions & 5 deletions data_model/src/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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`
Expand Down Expand Up @@ -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`].
Expand Down
66 changes: 64 additions & 2 deletions data_model/src/asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
mversic marked this conversation as resolved.
Show resolved Hide resolved
}

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.
Expand Down
2 changes: 1 addition & 1 deletion data_model/src/pagination.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,6 @@ mod tests {
.paginate(Pagination::new(Some(1), Some(1)))
.collect::<Vec<_>>(),
vec![2_i32]
)
);
}
}
Binary file modified tools/parity_scale_decoder/samples/account.bin
Binary file not shown.
1 change: 1 addition & 0 deletions tools/parity_scale_decoder/samples/account.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"name": "wonderland"
}
},
"assets_keys": {},
"assets": {},
"signatories": [],
"permission_tokens": [],
Expand Down