[fix] #2227: Implement Register and Unregister for Asset #2333

QuentinI · 2022-06-08T07:53:25Z

Description of the Change

Implemented Register and Unregister instructions for Asset
Changed minting implementation to require registering the asset first
Adjusted public validators accordingly

Issue

Resolves #2227

Benefits

Consistency, no more "unsupported" Register/Unregister instructions

Possible Drawbacks

Register asset and Mint should now be considered together when implementing permission systems.

codecov · 2022-06-08T08:15:43Z

Codecov Report

Merging #2333 (2172a1e) into iroha2-dev (6973f71) will increase coverage by 4.46%.
The diff coverage is 76.26%.

❗ Current head 2172a1e differs from pull request most recent head 07c2a6f. Consider uploading reports for the commit 07c2a6f to get more accurate results

@@              Coverage Diff               @@
##           iroha2-dev    #2333      +/-   ##
==============================================
+ Coverage       65.50%   69.96%   +4.46%     
==============================================
  Files             133      140       +7     
  Lines           24697    27803    +3106     
==============================================
+ Hits            16177    19452    +3275     
+ Misses           8520     8351     -169

Impacted Files	Coverage Δ
cli/derive/src/lib.rs	`74.72% <ø> (ø)`
cli/src/torii/mod.rs	`28.88% <ø> (ø)`
client/src/http.rs	`47.82% <ø> (ø)`
config/src/lib.rs	`15.38% <0.00%> (+1.09%)`	⬆️
core/src/lib.rs	`100.00% <ø> (ø)`
core/src/smartcontracts/isi/account.rs	`10.19% <0.00%> (-2.12%)`	⬇️
core/src/smartcontracts/isi/block.rs	`100.00% <ø> (ø)`
core/src/smartcontracts/isi/domain.rs	`39.82% <ø> (ø)`
core/src/smartcontracts/isi/triggers.rs	`0.00% <0.00%> (ø)`
core/src/smartcontracts/isi/tx.rs	`25.00% <ø> (ø)`
... and 107 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 43be45f...07c2a6f. Read the comment docs.

cli/src/torii/tests.rs

client/tests/integration/register_asset.rs

configs/peer/genesis.json

tools/kagami/src/main.rs

core/src/smartcontracts/isi/asset.rs

permissions_validators/src/public_blockchain/burn.rs

permissions_validators/src/public_blockchain/mint.rs

core/src/wsv.rs

permissions_validators/src/public_blockchain/burn.rs

appetrosyan

This change is important for SDK developers. Need to update with #api_changes.

applied

client/tests/integration/unstable_network.rs

appetrosyan · 2022-06-08T11:05:38Z

core/src/smartcontracts/isi/asset.rs

@@ -355,7 +403,6 @@ pub mod isi {
                AssetValueType::BigQuantity,
            )?;

-            wsv.asset_or_insert(&self.destination_id, 0_u128)?;


@Erigara Be advised that this PR will conflict with your work.

client/tests/integration/asset.rs

core/src/smartcontracts/isi/account.rs

core/src/smartcontracts/isi/mod.rs

Arjentix · 2022-06-10T16:48:05Z

I'll approve when all tests will pass

QuentinI · 2022-06-10T16:56:06Z

@Arjentix I don't think it's ready yet. Apart from there not being any consensus on #2339 , this PR as it is now introduces a loophole to mint non-mintable assets twice.

Arjentix · 2022-06-10T17:42:52Z

Oh, yes, haven't think about it...

core/src/smartcontracts/isi/asset.rs

appetrosyan · 2022-06-20T05:16:42Z

I think disallowing minting altogether and leaving registration as the only way to add non-mintable assets to accounts results in more straightforward API. Is there a particular reason you would want to support minting in this case?

The APIs are comparably simple, but in different ways.

If you retained the Mintable::Once variant, you can create an asset and can defer the amount that you wanted to mint. Why is this good? Consider that we made Mint variadic: it accepts multiple (AccountId, Asset, Quantity) tuples, and so if you want to create a non-mintable asset that is split evenly between e.g. shareholders of a company, you can do that in two instructions. If we don't allow it, we increase the load on the network, because to achieve the same result the person doing it has to split the single Mint ISI into a Mint and several transfers.

Moreover, we will need to synchronise the behaviour of Mint with the implementation in Execute for Register<AssetDefinition>, so nominally you have fewer instructions but each of them is more complex.

Both approaches are valid: in the CPU world what you propose is called CISC, while my stance is closer to RISC with SIMD. I personally would prefer that we kept the API changes in this PR to a minimum, but will approve given a good justification.

QuentinI · 2022-06-20T07:51:19Z

Right, my stance on this was the way it was mostly because I confused AssetDefinition and Asset in regards to Mintable.
I reverted to having Mintable::Once and implemented your proposed solution for double-mint loophole:

So if Register has value 0, creates a Mintable::Once asset, while a register with any other non-zero value necessarily a Mintable::Not variant

I have one question in regards to that - in this implementation one can register Mintable::Once asset to any number of accounts as long as Register has value 0. Are we okay with that?

core/src/smartcontracts/isi/asset.rs

SamHSmith

LGTM. But you should probably address marin's comment.

pesterev

LGTM

Signed-off-by: Artemii Gerasimovich <gerasimovich@soramitsu.co.jp>

…count for (un)registering an asset Signed-off-by: Artemii Gerasimovich <gerasimovich@soramitsu.co.jp>

Signed-off-by: Artemii Gerasimovich <gerasimovich@soramitsu.co.jp>

…yperledger#2333) Signed-off-by: Artemii Gerasimovich <gerasimovich@soramitsu.co.jp> Signed-off-by: BAStos525 <jungle.vas@yandex.ru>

QuentinI requested review from s8sato, appetrosyan, mversic, Arjentix, SamHSmith, outoftardis and ilchu as code owners June 8, 2022 07:53

github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Jun 8, 2022

mversic previously requested changes Jun 8, 2022

View reviewed changes

mversic mentioned this pull request Jun 8, 2022

[fix] #2227: implement asset registration #2330

Closed

QuentinI force-pushed the 2227-unsupported-register branch from cdbc8ff to f786f2c Compare June 8, 2022 08:21

outoftardis reviewed Jun 8, 2022

View reviewed changes

appetrosyan mentioned this pull request Jun 8, 2022

Coherence in Asset registration #2339

Closed

appetrosyan reviewed Jun 8, 2022

View reviewed changes

appetrosyan mentioned this pull request Jun 8, 2022

[Release]: Pre-rc.5 #2343

Merged

12 tasks

mversic self-assigned this Jun 8, 2022

QuentinI force-pushed the 2227-unsupported-register branch from f786f2c to 381dfd8 Compare June 9, 2022 17:16

QuentinI requested review from pesterev and Erigara as code owners June 9, 2022 17:16

QuentinI force-pushed the 2227-unsupported-register branch from 381dfd8 to af34818 Compare June 9, 2022 17:59

appetrosyan reviewed Jun 10, 2022

View reviewed changes

Arjentix self-assigned this Jun 10, 2022

Arjentix reviewed Jun 10, 2022

View reviewed changes

QuentinI force-pushed the 2227-unsupported-register branch from af34818 to 5e26f12 Compare June 10, 2022 16:47

QuentinI force-pushed the 2227-unsupported-register branch from 5e26f12 to 00b23ad Compare June 10, 2022 17:40

pesterev requested changes Jun 19, 2022

View reviewed changes

core/src/smartcontracts/isi/asset.rs Outdated Show resolved Hide resolved

pesterev self-assigned this Jun 19, 2022

QuentinI force-pushed the 2227-unsupported-register branch 2 times, most recently from 3afc1a6 to f851ef5 Compare June 20, 2022 08:16

mversic reviewed Jun 20, 2022

View reviewed changes

core/src/smartcontracts/isi/asset.rs Outdated Show resolved Hide resolved

SamHSmith previously approved these changes Jun 20, 2022

View reviewed changes

QuentinI dismissed SamHSmith’s stale review via ff69b62 June 20, 2022 15:41

QuentinI force-pushed the 2227-unsupported-register branch from c974c83 to 2172a1e Compare June 21, 2022 12:33

appetrosyan approved these changes Jun 22, 2022

View reviewed changes

pesterev approved these changes Jun 22, 2022

View reviewed changes

appetrosyan requested review from appetrosyan, SamHSmith, mversic, Arjentix and outoftardis June 22, 2022 09:28

safinsaf deleted the branch hyperledger:iroha2-dev June 22, 2022 11:56

safinsaf closed this Jun 22, 2022

safinsaf reopened this Jun 22, 2022

appetrosyan approved these changes Jun 22, 2022

View reviewed changes

Artemii Gerasimovich added 6 commits June 22, 2022 16:44

[feature] hyperledger#2227: implement register and unregister for asset

267a7d8

Signed-off-by: Artemii Gerasimovich <gerasimovich@soramitsu.co.jp>

[feature] hyperledger#2227: adjust public blockchain validators to ac…

17cf056

…count for (un)registering an asset Signed-off-by: Artemii Gerasimovich <gerasimovich@soramitsu.co.jp>

[feature] hyperledger#2227: check mintability when registering asset

6fd75ee

Signed-off-by: Artemii Gerasimovich <gerasimovich@soramitsu.co.jp>

Applied suggestions

cc8a225

Signed-off-by: Artemii Gerasimovich <gerasimovich@soramitsu.co.jp>

move asset registration to account

6fb79db

Signed-off-by: Artemii Gerasimovich <gerasimovich@soramitsu.co.jp>

re-apply lost suggestions

07c2a6f

Signed-off-by: Artemii Gerasimovich <gerasimovich@soramitsu.co.jp>

appetrosyan force-pushed the 2227-unsupported-register branch from 2172a1e to 07c2a6f Compare June 22, 2022 12:44

appetrosyan merged commit 77e6429 into hyperledger:iroha2-dev Jun 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] #2227: Implement Register and Unregister for Asset #2333

[fix] #2227: Implement Register and Unregister for Asset #2333

QuentinI commented Jun 8, 2022

codecov bot commented Jun 8, 2022 •

edited

appetrosyan left a comment

appetrosyan Jun 8, 2022

Arjentix commented Jun 10, 2022 •

edited

QuentinI commented Jun 10, 2022

Arjentix commented Jun 10, 2022

appetrosyan commented Jun 20, 2022

QuentinI commented Jun 20, 2022

SamHSmith left a comment

pesterev left a comment

[fix] #2227: Implement Register and Unregister for Asset #2333

[fix] #2227: Implement Register and Unregister for Asset #2333

Conversation

QuentinI commented Jun 8, 2022

Description of the Change

Issue

Benefits

Possible Drawbacks

codecov bot commented Jun 8, 2022 • edited

Codecov Report

appetrosyan left a comment

Choose a reason for hiding this comment

appetrosyan Jun 8, 2022

Choose a reason for hiding this comment

Arjentix commented Jun 10, 2022 • edited

QuentinI commented Jun 10, 2022

Arjentix commented Jun 10, 2022

appetrosyan commented Jun 20, 2022

QuentinI commented Jun 20, 2022

SamHSmith left a comment

Choose a reason for hiding this comment

pesterev left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 8, 2022 •

edited

Arjentix commented Jun 10, 2022 •

edited