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 #5097

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nxsaken
Copy link
Contributor

@nxsaken nxsaken commented Sep 24, 2024

Context

Solution

  • Describe the approach taken to achieve the objective / resolve the issue.

Migration Guide (optional)

  • If this PR contains a breaking change relative to the main branch, provide an instruction on how affected parties might need to adapt to the change.

Review notes (optional)

  • For complex PRs, try to provide some information on how to approach the review more effectively.
  • For example, is there a natural order in which the affected files should be reviewed?

Checklist

  • Remove asset registering and unregistering
  • Remove Store assets
  • Reintroduce Store assets as proper non-fungible tokens
  • Make the overall API easier to use & more explicit
  • (optional) I've written unit tests for the code changes.
  • All review comments have been resolved.
  • All CI checks pass.

@github-actions github-actions bot added the config-changes Changes in configuration and start up of the Iroha label Sep 24, 2024
Copy link

@BAStos525

@@ -10,7 +10,6 @@ license = "Apache-2.0"
resolver = "2"
members = [
"default_executor",
"create_nft_for_every_user_trigger",
Copy link
Contributor

Choose a reason for hiding this comment

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

this is temporarily removed, or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is to be re-added

@@ -281,7 +281,7 @@ mod tests {

fn get_test_instruction() -> InstructionBox {
let new_asset_id: AssetId = "tulip##ed0120CE7FA46C9DCE7EA4B125E2E36BDB63EA33073E7590AC92816AE1E861B7048B03@wonderland".parse().unwrap();
Register::asset(Asset::new(new_asset_id, 1_u32)).into()
Mint::asset_numeric(1_u32, new_asset_id).into()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Mint::asset_numeric(1_u32, new_asset_id).into()
Mint::asset_numeric(1_u32, new_asset_id).into()

are we keeping the name asset_numeric or could it be just asset or what's the plan here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Asset type naming turned out to be a pain, and I've spent too much time bikeshedding on this. I want specific, clear names to avoid confusion (like asset definitions being confused with assets). For now, I'm settling on these, but I don't really like the Fungible* names. We could call fungible assets Assets and non-fungible NFTs. We could call fungible assets Numerics (renaming Numeric to Decimal), and non-fungible assets Storages.

  • FungibleId – an abstract fungible asset ID (usd#money)
  • FungibleSpec – a fungible asset specification (decimal spec, mintability)
    • Mint::fungible_spec(id, spec) – could be Register, it doesn't matter but we can shrink the API surface
    • Transfer::fungible_spec(id)
    • Burn::fungible_spec(id)
  • FungibleAmount – an abstract fungible amount (100.00 of usd#money)
    • Mint::fungible_amount(amount, account)
    • Burn::fungible_amount(amount, account)
    • Transfer::fungible_amount(amount, source_account, target_account)
      • this is currently Transfer::asset_numeric(FungibleHandle, Numeric, Account) which is confusing and adds an unnecessary type parameter to Transfer, that's why I want to introduce this type
  • FungibleHandle – an owned fungible asset ID (usd#money#alice@wland), (alt. names BalanceId, WalletId)
  • FungibleAsset(FungibleHandle, Numeric), the full state of someone's asset, (alt. names Balance, Wallet)
  • NftId – a non-fungible asset ID (mona_lisa$louvre)
  • Nft – (NftId, (AccountId, Metadata)), the full state of the NFT
    • Mint::nft(id, initial_metadata)
    • Transfer::nft(id, source_account, target_account)
    • Burn::nft(id)
    • Mint::key_value(KeyValue, NftId)
    • Burn::key_value(Key, NftId)

@0x009922 @s8sato I'd appreciate your thoughts on the APIs.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM in general, with a few notes:

  • Makes sense to me for FungibleSpec to include id: FungibleId as an inseparable part of it. Therefore, Mint::fungible_spec(spec), but still Transfer::fungible_spec(id) and same for Burn::fungible_spec(id).
  • FungibleAmount doesn't seem to make sense without id: FungibleId either. So, I imagine it as id: FungibleId, mantissa: int, scale: int. Doesn't seem to contradict you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0x009922 FungibleAmount is (FungibleId, Numeric). Mantissa/scale seem to belong to NumericSpec, which belongs to FungibleSpec.

On including IDs in the entities themselves, for now I agree that they should be bundled – but in general we have a redundancy, since we store the ID twice in the key and the value (#5034)

Copy link
Contributor

Choose a reason for hiding this comment

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

Mantissa/scale seem to belong to NumericSpec, which belongs to FungibleSpec.

AFAIU, scale describes the decimal precision, while mantissa is the actual value. Thus, NumericSpec doesn't include the mantissa, but only the scale.

I don't like the idea of removing scale from FungibleAmount just for the sake of reducing redundancy - this way it will only have mantissa and working with it might be confusing. So, I am for FungibleAmount to have both scale and mantissa as a complete decimal amount.

However, I don't think it is always necessary to have scale in FungibleAmount be equal to the scale in the asset spec. It is an error if it is larger, but it's ok if it is less or equal to it. For example, if USD fungible spec specifies scale: 2, it is okay to have the amount to be mantissa: 30, scale: 0 ($30) or mantissa: 1234, scale: 2 ($12.34), but not mantissa: 1, scale: 10 (invalid tx).

but in general we have a redundancy, since we store the ID twice in the key and the value (#5034)

That sounds to me like an implementation detail, which shouldn't affect the API design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I was thinking of the number of mantissa digits. I still think scale shouldn't be part of FungibleAmount, since it wasn't part of Asset (AssetId + Numeric). We still validate the Numeric against the NumericSpec during execution; I don't see any benefit from scale being in the FungibleAmount (unless we make numerics more type-safe with something like const generics)

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding namings, I think you can suggest by just modifying the data model. Consistency is not an issue, since this is a draft for now

@0x009922 0x009922 self-assigned this Oct 18, 2024
@nxsaken nxsaken force-pushed the feat/separate-assets branch 2 times, most recently from 7ef2c61 to 0d7acd7 Compare October 18, 2024 14:23
Signed-off-by: Nurzhan Sakén <nurzhan.sakenov@gmail.com>
@s8sato s8sato self-assigned this Nov 5, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering impact to internal representation of entities, it might be inevitable for some tests to be temporally removed. What do you think about keeping references to the deletions? For example, we could mark #[cfg(disabled_5097)] to exclude from compiling. Later we can determine if each removal can/should be rewritten or not

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm anticipating we might introduce the concept of soul-bound (cannot be transferred) or not, as an orthogonal concept with fungible or not. I don't know what implementation would it have, but this might be worth keeping in mind here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
4 participants