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

RFC: New instruction for the reservation of asset classes/IDs #35

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

vstam1
Copy link
Contributor

@vstam1 vstam1 commented Jun 9, 2023

The proposed change is to introduce a new Instruction called InitializeAsset.
The instruction allows foreign chains to initialize fully-backed derivatives.
Currently, these derivatives are initialized using the Transact instruction, or directly on the sovereign chain.

@@ -28,10 +28,23 @@ The instruction would have the following specification:
CreateAsset {asset: MultiAsset, owner: MultiLocation}
```

The `CreateAsset` instruction allows for the creation of a single asset. This can be a fungible or non fungible asset.
The `CreateAsset` instruction allows for the creation of a single asset. This can be a fungible or non-fungible asset based on the specification of the `asset` field. The MultiAsset can describe a single asset. The `id` field is either an asset identity for a fungible asset or a class for a non-fungible asset. The fun field represents either the amount in the case of a fungible asset or the instance Id for a non-fungible asset.
Copy link
Contributor

Choose a reason for hiding this comment

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

either the amount in the case of a fungible asset

@vstam1
what amount? e.g. for pallet_assets::create we need min_balance, is this it?

just thinking, if it would be better like this, not sure:

CreateAsset {
    id: Concrete(MultiLocation {parents: 1, interior: X1(GeneralIndex(1))})
    fun: NonFungible(?),
    owner: MultiLocation {parents: 1, interior: 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.

These instructions are not connected to FRAME-based systems, so this instruction should also be usable to create an ERC20 token, for example, where the amount can represent the total_supply.

Like you said, the amount could represent the min_balance for the assets_pallet.

I do not directly see the difference between the CreateAsset with the assets field or with the id and fun fields split as the MultiAsset has both of these fields. However, if we split the fields, we could set the fun to Option<Fungibility>.

Copy link
Contributor

Choose a reason for hiding this comment

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

thx, I know they are not connected and xcm should be disconnected from frame as much possible,

ideally, fields should be self explanatory and also to avoid confusion or misuse,
so e.g. MultiAsset.fun.amount if user thinks about this amount as "total_supply" and on target system, we use this with several "AssetTransactors" where one can interpret this as "total_supply" and other as "min_balance", it could lead to problems
so for this case, fields split, as you said with Option could work better:

CreateAsset {
    id: Concrete(MultiLocation {parents: 1, interior: X1(GeneralIndex(1))})
    total_supply: Option<Funbility>,
    min_balance: Option<Funbility>,
    owner: MultiLocation {parents: 1, interior: Here},
}

we can go also with some properties wrapper and so on :)

AssetProperties {
    total_supply: Option<Funbility>,
    min_balance: Option<Funbility>,
}

yeah, I understand that this should be pretty generic to cover all cases,
so I am interesting where this will go :)

Copy link
Contributor Author

@vstam1 vstam1 Jun 13, 2023

Choose a reason for hiding this comment

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

Yeah you are definitely right that it could cause serious problems (imagining an asset that has its min balance set to value that should be total_supply haha). I would like to see more feedback on this approach. Especially the AssetProperties approach as it might also solve the Metadata related question.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of an AssetProperties, but I think we can't reuse Fungibility for those fields.
I think a possible approach would be to make the fungibility separation in the properties themselves.

enum AssetProperties {
  Fungible {
    total_supply: u128,
    min_balance: u128,
    // probably some other fields
  },
  NonFungible {
    collection_id: u64,
    // probably more fields
  },
}

If we add the largest amount of properties pertaining to that fungibility as possible, if some implementations don't handle some of them, like collections for example, they can just ignore that field.

Copy link
Contributor

@gavofyork gavofyork Jun 18, 2023

Choose a reason for hiding this comment

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

We could reasonably have an AssetProperties field in there I guess. It'll be a bit weird through since we'll need to make all field Optional otherwise we'll force the sender to introduce garbage info for fields which don't make sense for their asset and hope that the receiver ignores them. But then not all receive will support all fields. I guess we can have a convention of strictness, whereby receivers require some fields to be Some (if their implementation really needs them) or None (if their implementation doesn't support them and they're actually important). Other fields it might be flexible on.

enum Instruction {
  /* snip */
  InitializeAsset {
    /// The (concrete) asset ID, relative to the receiver/executor. This is what can
    /// be used as an asset ID when minting or teleporting assets into the chain.
    asset_id: MultiLocation,
    /// The owner location, relative to the receiver/executor. Some implementations
    /// might support all kinds of locations, others might only support
    /// `{ parents: 0, interior: X1(AccountId32 { .. }) }` patterns.
    owner: MultiLocation,
    /// Relevant properties of the asset class/ID to be initialized. The
    /// implementation may require some fields to be `Some` and others `None`.
    properties: AssetProperties,
  }
}
enum AssetProperties {
  Fungible {
    total_supply: Option<u128>,
    min_balance: Option<u128>,
  },
  NonFungible,
}

What would the proposed collection_id be? We have a named owner already for the asset class/ID who can actually mint the (NF)Tokens. If collection_id is passed anywhere, I'd have expected it to be passed when minting, not initializing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of calling the instruction InitializeAsset, as well as adding the AssetProperties field. Is there already a standard for NFT metadata?

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/xcm-rfc-process/2447/4

Copy link
Contributor

@xlc xlc left a comment

Choose a reason for hiding this comment

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

I read the whole thing and still don't really know exactly what this instruction does.
Does it register an asset? The parameters are not enough for such usage.
Does it mint a new derivative asset? What exactly is a derivative asset in this case?

Can you add an example section with more examples (fungible and non fungible), including the expected outcome.

The spec section should also list out error conditions. What's should be supported. What must fail. What can be implementation defined.

As previously discussed, it is possible to create assets using the `Transact` instruction.

## Questions and open Discussions (optional)
- How do we represent the `fun` field in the MultiAsset struct? In most of the FRAME-based systems we currently have, the creation of assets does not require an amount in the case of fungible assets or an instance id in the case of non-fungible assets.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid it altogether by not using a MultiAsset but instead just the id portion of it.

This would require exposing the AssetId, which is fine. But I would also propose that XCMv4 remove the AssetId::Abstract variant and thus make the id just a straight MultiLocation. This would make the instruction contain the fields { asset_id: InteriorMultiLocation, owner: MultiLocation }, where asset_id is an interior location of Origin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I think there's not actually any need to require that asset_id be an InteriorMultiLocation. Some chains might privilege certain origins to be able to create assets which are not interior to them. So this becomes: { asset_id: MultiLocation, owner: MultiLocation }.

The instruction would have the following specification:

```rust
CreateAsset {asset: MultiAsset, owner: MultiLocation}
Copy link
Contributor

@gavofyork gavofyork Jun 16, 2023

Choose a reason for hiding this comment

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

I would instead call this AcquireAssetId, since:

a) you're not actually creating any assets; and
b) you're appointing a controller for the given asset class/ID.

Alternatives are:

  • OccupyAssetId
  • SeizeAssetId
  • TakeAssetId
  • AppointAssetIdAdmin
  • ProcureAssetId
  • ObtainAssetId

My favourites are ReserveAssetId and HoldAssetId, however these clash with pre-existing terminology.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about CreateAssetId or CreateAssetType or CreateAssetClass?

Copy link
Contributor

@gavofyork gavofyork Jun 18, 2023

Choose a reason for hiding this comment

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

I don't think you're really creating anything so much as priming some namespace which is already yours to do so by virtue of the origin.

Perhaps InitializeAsset or SetupAsset..

@gavofyork gavofyork changed the title create asset instruction draft RFC: New instruction for the reservation of asset classes/IDs Jun 16, 2023
```

## Security considerations
The XCVM implementation has to check if the origin is allowed to create this asset.
Copy link
Contributor

Choose a reason for hiding this comment

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

In which case is the origin allowed to create the asset?

Copy link
Contributor

@gavofyork gavofyork Jun 18, 2023

Choose a reason for hiding this comment

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

I suppose this depends entirely on the implementation. For what it's worth, the initial impl for AssetHub will allow any location to create its own asset and assets at any sublocations, which is a perfectly reasonable convention.

@@ -28,10 +28,23 @@ The instruction would have the following specification:
CreateAsset {asset: MultiAsset, owner: MultiLocation}
```

The `CreateAsset` instruction allows for the creation of a single asset. This can be a fungible or non fungible asset.
The `CreateAsset` instruction allows for the creation of a single asset. This can be a fungible or non-fungible asset based on the specification of the `asset` field. The MultiAsset can describe a single asset. The `id` field is either an asset identity for a fungible asset or a class for a non-fungible asset. The fun field represents either the amount in the case of a fungible asset or the instance Id for a non-fungible asset.
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of an AssetProperties, but I think we can't reuse Fungibility for those fields.
I think a possible approach would be to make the fungibility separation in the properties themselves.

enum AssetProperties {
  Fungible {
    total_supply: u128,
    min_balance: u128,
    // probably some other fields
  },
  NonFungible {
    collection_id: u64,
    // probably more fields
  },
}

If we add the largest amount of properties pertaining to that fungibility as possible, if some implementations don't handle some of them, like collections for example, they can just ignore that field.

@vstam1 vstam1 mentioned this pull request Jun 20, 2023
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants