Skip to content

Conversation

@clearloop
Copy link
Contributor

@clearloop clearloop commented Jun 10, 2021

After thinking about it over again, I don't think refactoring AssetId to enum is a better idea, the following is what AssetId is in my current understanding

  • AssetId is unique for PINT assets
    • But one asset can have different AssetIds since Assets could be submitted by different accounts.
    • So if we use enum, the enum have to wrap something which can make Assets different from others, and the easiest and the most practical type is number
  • The assets PINT served are dynamic
    • Once an asset has been removed, the match AssetId should not be used anymore
    • Whatever any types we set for Assetid, it's hard to avoid using number

Changes

  • AssetId: + AtLeast32BitUnsigned

Tests

CI

Issues

Closes #102
Closes #115

@clearloop clearloop added the question Further information is requested label Jun 10, 2021
@mattsse
Copy link
Contributor

mattsse commented Jun 10, 2021

AssetId is unique for PINT assets

  • But one asset can have different AssetIds since Assets could be submitted by different accounts.

This is one not clear for me, Assets can be submitted by different accounts, but are always unique.

The assets PINT served are dynamic

  • Once an asset has been removed, the match AssetId should not be used anymore
  • Whatever any types we set for Assetid, it's hard to avoid using number

This is a very good point.

Yes unique int ids make the most sense, I guess the assets pallet does the same, did you have a look?

What we certainly need is some kind of additional metadata struct for every asset, something like

struct AssetInfo<Id> {
    name : Vec<u8>,
    symbol: Vec<u8>,
    id: Id
}

similar to chainlink feed info

I don't think we have a proper storage map for this, we have the MultiAssetRegistry trait that is implemented for the asset_index, that should be extended

@clearloop clearloop added needs review PR needs reviewing and removed question Further information is requested labels Jun 15, 2021
@clearloop clearloop marked this pull request as ready for review June 15, 2021 14:17
Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I think keeping Metadata separate will be more convenient

@mattsse mattsse added needs changes PR needs changes and removed needs review PR needs reviewing labels Jun 16, 2021
@clearloop clearloop changed the title feat(asset-index): add trait AtLeast32BitUnsigned to AssetId feat(asset-index): add AssetMetadata Jun 16, 2021
@clearloop clearloop added needs review PR needs reviewing and removed needs changes PR needs changes labels Jun 16, 2021
@clearloop clearloop requested a review from mattsse June 16, 2021 15:12
Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

LGTM

@mattsse mattsse added approved PR approved to merge and removed needs review PR needs reviewing labels Jun 16, 2021
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
@clearloop clearloop merged commit 3568855 into main Jun 16, 2021
@clearloop clearloop deleted the cl/asset-id branch June 16, 2021 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved PR approved to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor AssetId Refactor AssetId in pallet-asset-index as enumeration

2 participants