feat!: separate fungible and non-fungible assets#5097
feat!: separate fungible and non-fungible assets#5097nxsaken wants to merge 1 commit intohyperledger-iroha:mainfrom nxsaken:feat/separate-assets
Conversation
| resolver = "2" | ||
| members = [ | ||
| "default_executor", | ||
| "create_nft_for_every_user_trigger", |
There was a problem hiding this comment.
this is temporarily removed, or?
There was a problem hiding this comment.
Yes, this is to be re-added
| 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() |
There was a problem hiding this comment.
| 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?
There was a problem hiding this comment.
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 beRegister, it doesn't matter but we can shrink the API surfaceTransfer::fungible_spec(id)Burn::fungible_spec(id)
FungibleAmount– an abstract fungible amount (100.00 ofusd#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 toTransfer, that's why I want to introduce this type
- this is currently
FungibleHandle– an owned fungible asset ID (usd#money#alice@wland), (alt. namesBalanceId,WalletId)FungibleAsset–(FungibleHandle, Numeric), the full state of someone's asset, (alt. namesBalance,Wallet)NftId– a non-fungible asset ID (mona_lisa$louvre)Nft–(NftId, (AccountId, Metadata)), the full state of the NFTMint::nft(id, initial_metadata)Transfer::nft(id, source_account, target_account)Burn::nft(id)Mint::key_value(KeyValue, NftId)Burn::key_value(Key, NftId)
There was a problem hiding this comment.
LGTM in general, with a few notes:
- Makes sense to me for
FungibleSpecto includeid: FungibleIdas an inseparable part of it. Therefore,Mint::fungible_spec(spec), but stillTransfer::fungible_spec(id)and same forBurn::fungible_spec(id). FungibleAmountdoesn't seem to make sense withoutid: FungibleIdeither. So, I imagine it asid: FungibleId, mantissa: int, scale: int. Doesn't seem to contradict you.
There was a problem hiding this comment.
@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)
There was a problem hiding this comment.
Mantissa/scale seem to belong to
NumericSpec, which belongs toFungibleSpec.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
Signed-off-by: Nurzhan Sakén <nurzhan.sakenov@gmail.com>
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
superseeded by #5308 |
Context
AssetTypeinconsistencies #4087, related to Refine store asset transfers #4782Solution
Migration Guide (optional)
mainbranch, provide an instruction on how affected parties might need to adapt to the change.Review notes (optional)
Checklist