feat!: Separate fungible and non-fungible assets

[dima74]

Context

Currently, there are two asset types: Numeric and Store. Because they are quite different entities, it is proposed to separate them. See #4087 for details.

• Fixes [suggestion] Eliminate AssetType inconsistencies #4087
• Closes feat!: separate fungible and non-fungible assets #5097
• cascade or restrict on delete entities? #4672 (comment)

Solution

This PR removes Store asset type and instead introduces new Nft entity. Nft is similar to AssetDefinition (from implementation point of view), see #4672 (comment) for types visualization.

Naming:

• NFT (added):
  • nft$domain
• Numeric assets (no change):
  • asset#domain#account@domain or asset##account@domain

ISI changes:

• Before:
  • Both numeric and store assets:
    • Register<Asset> / Unregister<Asset>
  • Only for numeric assets:
    • Mint<Numeric, Asset> / Burn<Numeric, Asset>
    • Transfer<Asset, Numeric, Account>
  • Only for store assets:
    • Transfer<Asset, Metadata, Account>
      • Transfer asset from one account to another
      • Very strange instruction, first of all Metadata argument is ignored. Also looks like we have bug in implementation, and asset is not moved, but copied
    • SetKeyValue<Asset>
      • Creates asset implicitly if it is not registered
    • RemoveKeyValue<ASset>
• After:
  • Numeric assets or just assets:
    • No register/unregister instructions. Use Mint<Numeric, Asset> instead of register, and Burn<Numeric, Asset> to zero value instead of unregister.
    • Mint<Numeric, Asset> / Burn<Numeric, Asset>
    • Transfer<Asset, Numeric, Account>
  • Nft:
    • Register<Nft> / Unregister<Nft>
    • Transfer<Account, NftId, Account>
      • Transfer NFT to another account
    • SetKeyValue<Nft>
      • Error if NFT is not registered
    • RemoveKeyValue<Nft>

Queries changes:

• Added query FindNfts

Permission changes:

• Removed permissions:
  • CanRegisterAsset / CanUnregisterAsset
  • CanModifyAssetMetadata
• Added permissions:
  • CanRegisterNft / CanUnregisterNft
  • CanTransferNft
  • CanModifyNftMetadata

Migration Guide

• Removed AssetDefinition::store, AssetType::Store, AssetValue::Store. Use Nft instead
• Removed Register<Asset> / Unregister<Asset> ISI for numeric assets. Use Mint<Numeric, Asset> instead of register, and Burn<Numeric, Asset> to zero value instead of unregister.
• Detailed description of changes is in the Solution section

---

Review notes

Files with important changes:

• crates/iroha_data_model/src/nft.rs - New structs Nft and NftId
• crates/iroha_data_model/src/asset.rs - Changes to Asset and AssetDefinition
• crates/iroha_core/src/smartcontracts/isi/nft.rs - ISI and Queries implementation for NFT
• crates/iroha_executor/src/default/mod.rs - default executor visit_* methods for NFT
• crates/iroha_cli/src/main.rs - CLI changes

Todo:

• Fix python tests (e.g. test_register_asset_definitions.py)

Checklist

• I've read CONTRIBUTING.md.
• (optional) I've written unit tests for the code changes.
• All review comments have been resolved.
• All CI checks pass.

[github-actions]

@BAStos525

[mversic]

IMO this looks great. @0x009922 what do you think?

[0x009922]

IMO looks great too!

Finally, this separation makes sense, and it's easier to understand how to use both types.

[0x009922]

Shouldn't it be FindError::Domain?

[dima74]

There are two options. If domain check is after NFT is found and removed, it should be .expect("INTERNAL BUG ..."). If before, it should be FindError::Domain (#5308 (comment)). Changed to have domain check after.

[0x009922]

This seemingly doesn't make sense. spec must be { "scale": 0 } or something, right?

[dima74]

Currently we have this behaviour for formatting NumericSpec:

• scale = 0 is formatted as Numeric(0)
• scale = None is formatted as just Numeric

iroha/crates/iroha_numeric/src/lib.rs

Lines 380 to 385 in 591dd5e

	impl core::fmt::Display for NumericSpec {
	fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
	write!(f, "Numeric")?;
	if let Some(scale) = self.scale {
	write!(f, "({scale})")?;
	}

---

One option I see here is to change spec: NumericSpec field of AssetDefinition to just scale: Option<u32>

[mversic]

This seemingly doesn't make sense. spec must be { "scale": 0 } or something, right?

No, it's not necessary that spec is defined. When scale is undefined the number is a variable precision as opposed to fixed precision when it is defined