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

feat!: Separate fungible and non-fungible assets #5308

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dima74
Copy link
Contributor

@dima74 dima74 commented Feb 7, 2025

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.

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.

@dima74 dima74 added the api-changes Changes in the API for client libraries label Feb 7, 2025
@dima74 dima74 self-assigned this Feb 7, 2025
@github-actions github-actions bot added the config-changes Changes in configuration and start up of the Iroha label Feb 7, 2025
Copy link

github-actions bot commented Feb 7, 2025

@BAStos525

crates/iroha_data_model/src/nft.rs Outdated Show resolved Hide resolved
crates/iroha_core/src/smartcontracts/isi/nft.rs Outdated Show resolved Hide resolved
@s8sato s8sato self-assigned this Feb 10, 2025
crates/iroha_data_model/src/nft.rs Show resolved Hide resolved
)+};
}

impl_froms_and_validate_grant_revoke!(CanUnregisterNft, CanTransferNft, CanModifyNftMetadata);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think NFT contents are meant to be modified by owners after registration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we should have the following rules, what do you think?

  • Owner of nft.domain can unregister, transfer and modify NFT
  • Owner of NFT can only transfer NFT

crates/iroha_cli/src/main.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries config-changes Changes in configuration and start up of the Iroha
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[suggestion] Eliminate AssetType inconsistencies
2 participants