feat: asset registry #728

sander2 · 2022-04-21T18:48:36Z

This is meant to address #721.

This PR will need cleanup, tests, etc, but I'm opening this as a draft now to get some feedback regarding the direction. Some things to note about the implementation:

In the issue I raised two possible implementations: either we can provide some of the metadata fields by default, or we leave it fully customizable. I went with the latter approach in this PR.
The only dispatchable function is one to register (or update) an asset.
The metadata is fully customizable - the only requirement being that there is a way to extract a MultiAsset from it. This is needed to be able to map xcm assets to assets in this pallet
The assigned asset id is customizable. The last assigned asset id is stored, so you can either derive a sequential id or base it purely on the metadata
Some traits and trait implementations are provided to allow for easy WeightTrader implementations. Unfortunately because WeightTrader::refund_weight allows only a single asset to be refunded, the default implementation does not support paying with different assets. That is, it can pay with any asset in the registry, but only one asset per xcm message. I opened WeightTrader refund limitations paritytech/polkadot-sdk#836 to see if refund_weight can refund multiple assets in the future.

An example of usage:

// the metadata definition...
#[derive(scale_info::TypeInfo, Encode, Decode, Clone, Eq, PartialEq, Debug)]
pub struct AssetMetadata {
    pub location: MultiLocation,
    pub decimals: u32,
    pub fee_per_second: u128,
    pub name: Vec<u8>,
    pub symbol: Vec<u8>,
}
impl Into<MultiLocation> for AssetMetadata {
    fn into(self) -> MultiLocation {
        self.location
    }
}

// the config...
impl orml_asset_registry::Config for Runtime {
    type Event = Event;
    type AssetMetadata = AssetMetadata;
    type AssetId = u32;
    type AuthorityOrigin = EnsureRoot<AccountId>;
    type AssignAsset = SequentialId<u32>;
}

// for paying xcm fees..
pub struct MyFixedConversionRateProvider;
impl FixedConversionRateProvider for MyFixedConversionRateProvider {
    fn get_fee_per_second(location: &MultiLocation) -> Option<u128> {
        let metadata = AssetRegistry::fetch_metadata_by_location(location)?;
        Some(metadata.fee_per_second)
    }
}
pub type Trader = (
    (...)
    AssetRegistryTrader<FixedRateAssetRegistryTrader<MyFixedConversionRateProvider>, ToTreasury>,
);

I would love to get some feedback on this. I'm out of office until tuesday though so might not be able to address any comments until then.

asset-registry/src/lib.rs

xlc · 2022-04-24T11:19:18Z

I think we should still require some basic fields like name, symbol, decimals. otherwise the js sdk still need specialized code to handle it for different parachains.

codecov-commenter · 2022-04-24T11:58:14Z

Codecov Report

Merging #728 (ac2ca60) into master (39057da) will decrease coverage by 0.00%.
The diff coverage is 87.42%.

@@            Coverage Diff             @@
##           master     paritytech/polkadot#728      +/-   ##
==========================================
- Coverage   77.68%   77.67%   -0.01%     
==========================================
  Files          92      100       +8     
  Lines        9069     9598     +529     
==========================================
+ Hits         7045     7455     +410     
- Misses       2024     2143     +119

Impacted Files	Coverage Δ
asset-registry/src/weights.rs	`0.00% <0.00%> (ø)`
traits/src/lib.rs	`0.00% <ø> (ø)`
asset-registry/src/mock/relay.rs	`25.00% <25.00%> (ø)`
asset-registry/src/impls.rs	`66.07% <66.07%> (ø)`
asset-registry/src/mock/para.rs	`66.66% <66.66%> (ø)`
asset-registry/src/mock/mod.rs	`82.53% <82.53%> (ø)`
asset-registry/src/lib.rs	`92.59% <92.59%> (ø)`
asset-registry/src/tests.rs	`96.95% <96.95%> (ø)`
traits/src/asset_registry.rs	`100.00% <100.00%> (ø)`
traits/src/currency.rs	`34.88% <0.00%> (-25.12%)`	⬇️
... and 1 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

enthusiastmartin · 2022-04-25T13:15:11Z

I agree with xlc - that some basic information can be already part of the pallet ( name,symbol, decimals).

personally, i would not require Into<MultiLocation> for asset metadata, and i would not map the location to asset id when asset is registered.
I'd add ability to set/update/remove location explicitly after asset is already registered.

sander2 · 2022-04-26T15:33:56Z

I think we should still require some basic fields like name, symbol, decimals. otherwise the js sdk still need specialized code to handle it for different parachains.

Sure, I added name, symbol, decimals and multilocation.

I agree with xlc - that some basic information can be already part of the pallet ( name,symbol, decimals).

personally, i would not require Into<MultiLocation> for asset metadata, and i would not map the location to asset id when asset is registered. I'd add ability to set/update/remove location explicitly after asset is already registered.

When I added default fields, I also added the (multi)location. Are you opposed to this? Do you think it should be an optional item? Or should it be removed as a default field altogether?

enthusiastmartin · 2022-04-26T20:36:38Z

I think we should still require some basic fields like name, symbol, decimals. otherwise the js sdk still need specialized code to handle it for different parachains.

Sure, I added name, symbol, decimals and multilocation.

I agree with xlc - that some basic information can be already part of the pallet ( name,symbol, decimals).
personally, i would not require Into<MultiLocation> for asset metadata, and i would not map the location to asset id when asset is registered. I'd add ability to set/update/remove location explicitly after asset is already registered.

When I added default fields, I also added the (multi)location. Are you opposed to this? Do you think it should be an optional item? Or should it be removed as a default field altogether?

I might be wrong here - but i think, if you set a location for asset and then xcm config is set to map asset and locations - that will make the asset automatically transferable.

which is probably fine, that's why you add foreign assets in first place.

but not all assets need location, and not all asset are transferable. For example - in HydraDX's xyk pool - we use asset registry to register 'share asset' for each subpool as another asset in the system. However, this asset does not need location nor it can be transferred somewhere else.

sander2 · 2022-04-27T08:07:11Z

I might be wrong here - but i think, if you set a location for asset and then xcm config is set to map asset and locations - that will make the asset automatically transferable.

which is probably fine, that's why you add foreign assets in first place.

but not all assets need location, and not all asset are transferable. For example - in HydraDX's xyk pool - we use asset registry to register 'share asset' for each subpool as another asset in the system. However, this asset does not need location nor it can be transferred somewhere else.

Thank you for the clarification, that makes sense indeed. I changed the location to be a Option<MultiLocation>, and also added a set_asset_location function. In your usecase you'd set the location in the metadata to None, while xcm transferable assets can still be registered in a single step using only register_asset. The reason why I think it's good to be able to register xcm-transferrable assets in a single call, is that making multiple calls is problematic if you have to go through governance. You could also batch register_asset and set_asset_location, but the problem there is that the asset id might not be known in advance in all cases

enthusiastmartin · 2022-04-27T08:22:33Z

What about existential deposit / min balance ?

I believe this could be one of the required fields too.

i think that it would be useful if you could also plug asset registry to eg. orml_tokens's config ExistentialDeposits.
that would mean, it needs GetByKey implementation.

Eg. in our asset-registry here.

asset-registry/src/lib.rs

sander2 · 2022-04-27T12:50:43Z

What about existential deposit / min balance ?

I believe this could be one of the required fields too.

i think that it would be useful if you could also plug asset registry to eg. orml_tokens's config ExistentialDeposits. that would mean, it needs GetByKey implementation.

Eg. in our asset-registry here.

Agreed, added.

asset-registry/src/lib.rs

sander2 · 2022-05-04T08:29:21Z

I cleaned the code up a bit and added some tests, including some that show that it can be used in xcm transfers. From my PoV, this PR is ready to be reviewed.

There is one thing that I still need address: the default weights. For the other pallets, the default weights are generated with acala/target/release/acala benchmark. Since this pallet is not included in that repo, should I add benchmark code in the crate itself?

One more note on the ProcessAsset trait. By request of @xlc it's returning (AssetId, Metadata), meaning that it can modify the supplied metadata. I'm not certain what the use case of this is, but running the same function again when modifying metadata is problematic, so I only call it in the register_asset function and not in update_asset.

asset-registry/src/lib.rs

xlc

not 100% sure if we want all the XCM weight related code in this pallet. I guess we don't really want another crate just for that so it is probably fine.

asset-registry/src/lib.rs

xlc · 2022-05-06T08:00:33Z

asset-registry/src/lib.rs

+
+	#[pallet::pallet]
+	#[pallet::generate_store(pub(super) trait Store)]
+	#[pallet::without_storage_info]


should use bonded types, but that can happen in later PR

asset-registry/src/lib.rs

asset-registry/src/impls.rs

asset-registry/src/lib.rs

xlc · 2022-05-07T01:11:49Z

other than nits it is good for a V1

asset-registry/src/lib.rs

traits/src/asset_registry.rs

asset-registry/src/lib.rs

zqhxuyuan · 2022-05-12T03:09:18Z

asset-registry/src/mock/para.rs

+pub struct MyFixedConversionRateProvider;
+impl FixedConversionRateProvider for MyFixedConversionRateProvider {
+	fn get_fee_per_second(location: &MultiLocation) -> Option<u128> {
+		let metadata = AssetRegistry::fetch_metadata_by_location(location)?;
+		Some(metadata.additional.fee_per_second)
+	}
+}


Can this move to impl.rs, I think this is a common implementation.

We can't - the fee_per_second is part of the custom metadata rather than the default metadata.

Edit: unless we make the impl generic over some callback Fn(Metadata) -> Option<Balance> - should we?

asset-registry/src/impls.rs

shaunxw · 2022-05-17T03:47:00Z

asset-registry/src/impls.rs

+				let new_weight = existing_weight.saturating_add(weight);
+
+				if let Some(amount) = W::convert_weight_to_fee(location, new_weight) {
+					let fee_increase = amount.saturating_sub(existing_fee);


I might miss something, but not sure why adding weight to existing to get new_weight first, then convert to fee and subtract. Simply converting weight to fee should have the same result no?

And the embedded if/if let checks could be flattened for readability, such as:

if let Some(amount) = W::convert_weight_to_fee(location, new_weight) {

-->

let amount = W::convert_weight_to_fee(location, new_weight).ok_or(XcmError::TooExpensive)?; // ...

Also

if fee_increase == 0 { return Ok(payment); } else if let Ok(unused) = payment.clone().checked_sub((asset.clone(), fee_increase).into()) {

-->

if fee_increase == 0 { return Ok(payment); } let unused = payment.clone().checked_sub((asset.clone(), fee_increase).into()).ok_or(XcmError::TooExpensive)?; // ...

I might miss something, but not sure why adding weight to existing to get new_weight first, then convert to fee and subtract. Simply converting weight to fee should have the same result no?

It's equivalent only when convert_weight_to_fee is a linear function, which it is not guaranteed to be. Though maybe I'm overcomplicating things - I don't expect to see non-linear functions being used in practice..

And the embedded if/if let checks could be flattened for readability, such as:
if let Some(amount) = W::convert_weight_to_fee(location, new_weight) {
-->
let amount = W::convert_weight_to_fee(location, new_weight).ok_or(XcmError::TooExpensive)?;

That would stop the iteration over the assets in payment.fungible. The same applies for the last refactoring you suggest

I don't see non-linear functions being used either. AFAIK ppl are all using fixed rate conversions, specifically the fixed rate weight traders implemented in Polkadot repo.

IMO we can simply aim for the same impl to provide a fixed rate trader. It's easier to use and maintain.

asset-registry/src/impls.rs

asset-registry/src/lib.rs

asset-registry/src/tests.rs

shaunxw

I suggest to change update_asset dispatchable call signature to

pub fn update_asset(
			origin: OriginFor<T>,
			asset_id: T::AssetId,
			decimals: Option<u32>,
			name: Option<Vec<u8>>,
			// ... rest fields wrapped in `Option`
		) -> DispatchResult

The impl checks every field and update it if Some. Note location needs to be Option<Option<VersionedMultiLocation>>.

The rationale behind it is that in most cases, only one field will be updated, but the current call requires to copy all existing unchanged ones, which is unnecessary and prone to mistakes. Additionally, set_asset_location call can be merged into this one.

sander2 · 2022-05-18T18:17:20Z

I think I've addressed all comments - PR should be ready for final review pass & merging

shaunxw

LGTM. A few more trivial issues and I think we are good to go.

shaunxw · 2022-05-18T23:22:43Z

asset-registry/src/impls.rs

+				let new_weight = existing_weight.saturating_add(weight);
+
+				if let Some(amount) = W::convert_weight_to_fee(location, new_weight) {
+					let fee_increase = amount.saturating_sub(existing_fee);


I don't see non-linear functions being used either. AFAIK ppl are all using fixed rate conversions, specifically the fixed rate weight traders implemented in Polkadot repo.

IMO we can simply aim for the same impl to provide a fixed rate trader. It's easier to use and maintain.

asset-registry/README.md

asset-registry/Cargo.toml

sander2 · 2022-05-19T14:47:49Z

Fixed the last comments and rebased (and squashed) the branch.

asset-registry/src/lib.rs

asset-registry/src/impls.rs

asset-registry/src/lib.rs

sander2 marked this pull request as draft April 21, 2022 18:48

sander2 mentioned this pull request Apr 21, 2022

Asset registry pallet #721

Closed

xlc requested review from xlc and zqhxuyuan April 24, 2022 11:07

xlc reviewed Apr 24, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

zqhxuyuan reviewed Apr 27, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

sander2 force-pushed the asset-registry branch from 5236c19 to 4a9f247 Compare April 27, 2022 16:19

xlc reviewed Apr 27, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

sander2 marked this pull request as ready for review May 4, 2022 08:15

sander2 requested review from xlc and zqhxuyuan May 4, 2022 08:15

xlc reviewed May 6, 2022

View reviewed changes

asset-registry/src/lib.rs Show resolved Hide resolved

xlc reviewed May 6, 2022

View reviewed changes

sander2 force-pushed the asset-registry branch from 5c30215 to 04aebd2 Compare May 6, 2022 14:02

xlc reviewed May 7, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

xlc reviewed May 7, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

zqhxuyuan reviewed May 7, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

zqhxuyuan reviewed May 7, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

zqhxuyuan reviewed May 8, 2022

View reviewed changes

asset-registry/src/lib.rs Show resolved Hide resolved

zqhxuyuan reviewed May 9, 2022

View reviewed changes

traits/src/asset_registry.rs Outdated Show resolved Hide resolved

zqhxuyuan reviewed May 9, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

zqhxuyuan reviewed May 9, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

sander2 force-pushed the asset-registry branch from a8fe770 to 2f4ea46 Compare May 9, 2022 15:56

zqhxuyuan reviewed May 9, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

zqhxuyuan reviewed May 12, 2022

View reviewed changes

zqhxuyuan requested a review from shaunxw May 17, 2022 02:40

shaunxw reviewed May 17, 2022

View reviewed changes

shaunxw reviewed May 18, 2022

View reviewed changes

feat: added asset registry

51b4617

sander2 force-pushed the asset-registry branch from d9120cf to 51b4617 Compare May 19, 2022 14:46

shaunxw approved these changes May 20, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

fix: deposit event when updating asset

d96864b

zqhxuyuan reviewed May 23, 2022

View reviewed changes

asset-registry/src/impls.rs Outdated Show resolved Hide resolved

zqhxuyuan reviewed May 23, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

zqhxuyuan reviewed May 23, 2022

View reviewed changes

asset-registry/src/lib.rs Show resolved Hide resolved

chore: comments & small refactoring

ac2ca60

zqhxuyuan reviewed May 23, 2022

View reviewed changes

asset-registry/src/lib.rs Show resolved Hide resolved

zqhxuyuan reviewed May 23, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

zqhxuyuan reviewed May 23, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

zqhxuyuan reviewed May 23, 2022

View reviewed changes

asset-registry/src/lib.rs Outdated Show resolved Hide resolved

refactor: extract do_update_location and cleanup do_update_asset

f6b16a4

zqhxuyuan merged commit 78f2d45 into open-web3-stack:master May 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: asset registry #728

feat: asset registry #728

sander2 commented Apr 21, 2022

xlc commented Apr 24, 2022

codecov-commenter commented Apr 24, 2022 •

edited

Loading

enthusiastmartin commented Apr 25, 2022

sander2 commented Apr 26, 2022

enthusiastmartin commented Apr 26, 2022

sander2 commented Apr 27, 2022

enthusiastmartin commented Apr 27, 2022

sander2 commented Apr 27, 2022

sander2 commented May 4, 2022

xlc left a comment

xlc May 6, 2022

xlc commented May 7, 2022

zqhxuyuan May 12, 2022

sander2 May 16, 2022 •

edited

Loading

shaunxw May 17, 2022

sander2 May 18, 2022

shaunxw May 18, 2022

shaunxw left a comment •

edited

Loading

sander2 commented May 18, 2022

shaunxw left a comment

shaunxw May 18, 2022

sander2 commented May 19, 2022

feat: asset registry #728

feat: asset registry #728

Conversation

sander2 commented Apr 21, 2022

xlc commented Apr 24, 2022

codecov-commenter commented Apr 24, 2022 • edited Loading

Codecov Report

enthusiastmartin commented Apr 25, 2022

sander2 commented Apr 26, 2022

enthusiastmartin commented Apr 26, 2022

sander2 commented Apr 27, 2022

enthusiastmartin commented Apr 27, 2022

sander2 commented Apr 27, 2022

sander2 commented May 4, 2022

xlc left a comment

Choose a reason for hiding this comment

xlc May 6, 2022

Choose a reason for hiding this comment

xlc commented May 7, 2022

zqhxuyuan May 12, 2022

Choose a reason for hiding this comment

sander2 May 16, 2022 • edited Loading

Choose a reason for hiding this comment

shaunxw May 17, 2022

Choose a reason for hiding this comment

sander2 May 18, 2022

Choose a reason for hiding this comment

shaunxw May 18, 2022

Choose a reason for hiding this comment

shaunxw left a comment • edited Loading

Choose a reason for hiding this comment

sander2 commented May 18, 2022

shaunxw left a comment

Choose a reason for hiding this comment

shaunxw May 18, 2022

Choose a reason for hiding this comment

sander2 commented May 19, 2022

codecov-commenter commented Apr 24, 2022 •

edited

Loading

sander2 May 16, 2022 •

edited

Loading

shaunxw left a comment •

edited

Loading