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] #1845: Non-mintable assets can be minted once only. #2044

Merged
merged 3 commits into from Apr 13, 2022

Conversation

appetrosyan
Copy link
Contributor

@appetrosyan appetrosyan commented Mar 31, 2022

Signed-off-by: Aleksandr Petrosyan a-p-petrosyan@yandex.ru

Description of the Change

Non-mintable assets are now registered as Mintable::Once and can be minted precisely once.

Issue

Closes #1845

Benefits

Non-mintable assets are now functional.

Possible Drawbacks

None

Usage examples/tests

client/integration/non_mintable.rs

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Mar 31, 2022
@appetrosyan appetrosyan marked this pull request as draft March 31, 2022 06:33
@codecov
Copy link

codecov bot commented Mar 31, 2022

Codecov Report

Merging #2044 (eae4993) into iroha2-dev (2a94dba) will decrease coverage by 77.27%.
The diff coverage is n/a.

❗ Current head eae4993 differs from pull request most recent head a264c90. Consider uploading reports for the commit a264c90 to get more accurate results

@@              Coverage Diff               @@
##           iroha2-dev   #2044       +/-   ##
==============================================
- Coverage       77.27%       0   -77.28%     
==============================================
  Files             176       0      -176     
  Lines           24216       0    -24216     
==============================================
- Hits            18713       0    -18713     
+ Misses           5503       0     -5503     
Impacted Files Coverage Δ
cli/src/torii/routing.rs
cli/src/torii/tests.rs
client/examples/million_accounts_genesis.rs
client/src/client.rs
client/tests/integration/add_asset.rs
client/tests/integration/asset_propagation.rs
client/tests/integration/events/data.rs
client/tests/integration/events/pipeline.rs
...lient/tests/integration/multiple_blocks_created.rs
client/tests/integration/multisignature_account.rs
... and 166 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9889c65...a264c90. Read the comment docs.

@appetrosyan appetrosyan force-pushed the i2-schema-endpoint branch 3 times, most recently from 0e989da to cf03236 Compare March 31, 2022 08:51
@appetrosyan appetrosyan marked this pull request as ready for review April 1, 2022 11:23
@appetrosyan appetrosyan force-pushed the i2-schema-endpoint branch 3 times, most recently from f70a616 to 4441696 Compare April 4, 2022 15:10
client/tests/integration/non_mintable.rs Outdated Show resolved Hide resolved
client/tests/integration/roles.rs Outdated Show resolved Hide resolved
client/tests/integration/roles.rs Outdated Show resolved Hide resolved
data_model/src/asset.rs Outdated Show resolved Hide resolved
data_model/src/asset.rs Outdated Show resolved Hide resolved
rust-toolchain.toml Show resolved Hide resolved
data_model/src/asset.rs Outdated Show resolved Hide resolved
data_model/src/asset.rs Outdated Show resolved Hide resolved
data_model/src/asset.rs Outdated Show resolved Hide resolved
data_model/src/events/data/events.rs Show resolved Hide resolved
core/test_network/src/lib.rs Outdated Show resolved Hide resolved
client/tests/integration/non_mintable.rs Outdated Show resolved Hide resolved
client/tests/integration/roles.rs Outdated Show resolved Hide resolved
@appetrosyan appetrosyan force-pushed the i2-schema-endpoint branch 2 times, most recently from fd5fcc7 to fe773b6 Compare April 6, 2022 07:16
core/src/smartcontracts/isi/query.rs Outdated Show resolved Hide resolved
data_model/src/asset.rs Show resolved Hide resolved
@appetrosyan appetrosyan requested a review from mversic April 6, 2022 08:05
@appetrosyan appetrosyan force-pushed the i2-schema-endpoint branch 2 times, most recently from 6c2f30b to e4de7b1 Compare April 7, 2022 07:24
mversic
mversic previously approved these changes Apr 7, 2022
mversic
mversic previously approved these changes Apr 7, 2022
s8sato
s8sato previously approved these changes Apr 8, 2022
Comment on lines +49 to +57
// However, this will fail
assert!(test_client
.poll_request(client::asset::by_account_id(account_id), |result| {
result.iter().any(|asset| {
asset.id().definition_id == asset_definition_id
&& *asset.value() == AssetValue::Quantity(400_u32)
})
})
.is_err());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion,
one of more explicit ways than waiting timeout would be to submit_blocking and query the rejected transaction and assert the rejection reason

data_model/src/asset.rs Show resolved Hide resolved
Arjentix
Arjentix previously approved these changes Apr 11, 2022
}
}

impl From<(<AssetDefinition as Identifiable>::Id, AssetValueType, bool)> for AssetDefinition {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This thing looks a little bit strange for me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove it as well. I think the given constructors are sufficient and this one would be unexpected to use. But I wouldn't be against implementing the following destructuring conversion:
impl From<AssetDefinition> for (<AssetDefinition as Identifiable>::Id, AssetValueType, bool)

data_model/src/asset.rs Outdated Show resolved Hide resolved
s8sato
s8sato previously approved these changes Apr 13, 2022
}
}

impl From<(<AssetDefinition as Identifiable>::Id, AssetValueType, bool)> for AssetDefinition {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove it as well. I think the given constructors are sufficient and this one would be unexpected to use. But I wouldn't be against implementing the following destructuring conversion:
impl From<AssetDefinition> for (<AssetDefinition as Identifiable>::Id, AssetValueType, bool)

data_model/src/asset.rs Outdated Show resolved Hide resolved
mversic
mversic previously approved these changes Apr 13, 2022
Arjentix
Arjentix previously approved these changes Apr 13, 2022
mversic
mversic previously approved these changes Apr 13, 2022
Signed-off-by: Aleksandr Petrosyan <a-p-petrosyan@yandex.ru>
Signed-off-by: Aleksandr Petrosyan <a-p-petrosyan@yandex.ru>
Signed-off-by: Aleksandr Petrosyan <a-p-petrosyan@yandex.ru>
@appetrosyan appetrosyan merged commit 6781520 into hyperledger:iroha2-dev Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants