feat: add asset conversion pallet #309

Moliholy · 2023-11-07T13:59:19Z

Closes #281

Moliholy · 2023-11-28T17:28:25Z

@stiiifff in order to migrate to 1.3.0 pallet-dex is causing problems, since we'd also have to migrate that pallet. We could save us some time in this regard once this pallet is out of scope when merging this PR.

stiiifff · 2023-12-05T11:38:46Z

runtime/stout/src/lib.rs

@@ -546,6 +538,127 @@ impl pallet_asset_registry::Config for Runtime {
 	type BenchmarkHelper = AssetRegistryBenchmarkHelper;
 }

+parameter_types! {
+	pub const AssetDeposit: Balance = UNITS / 10; // 1 / 10 UNITS deposit to create asset


Same remark as in the Trappist runtime.

stiiifff · 2023-12-05T11:44:08Z

runtime/trappist/src/lib.rs

@@ -562,23 +568,124 @@ impl pallet_democracy::Config for Runtime {
 }

 parameter_types! {
-	pub const DexPalletId: PalletId = PalletId(*b"trap/dex");
+	pub const AssetDeposit: Balance = UNITS / 10; // 1 / 10 UNITS deposit to create asset


I would align this on the local Assets instance (1 UNITS) as it doesn't make sense that creating a foreign / pool asset is 10x less expensive than a local asset. Wdyt ?

I checked several integrations of this pallet in polkadot-sdk, and in most asset-hub projects they use the value I provided, both for local and foreign asset creation. Also please note that the foreign asset deposit is the same:

pub const ForeignAssetsAssetDeposit: Balance = AssetDeposit::get();

Still, it's fine for me to change it 👍

stiiifff · 2023-12-05T12:00:09Z

runtime/trappist/src/xcm_config.rs

@Moliholy

Some thoughts:

We most probably want to have a ForeignFungiblesTransactor and PoolFungiblesTransactor (see the AH Rococo's XCM config).

With a separate instance of the FRAME Assets pallet for foreign assets, do we still need the AssetRegistry pallet in the runtime ? (as far as I understand, we don't need it anymore .. and the runtime & XCM configs must be amended accordingly).

Don't we need to adapt the Trader configuration (see AH Rococo's XCM Trader config for inspiration).

Should we update the SafeCallFilter ? (for now, we allow Everything ... which doesn't seem very safe).

wdyt @hbulgarini @metricaez ?

We most probably want to have a ForeignFungiblesTransactor and PoolFungiblesTransactor (see the AH Rococo's XCM config).

Agreed 👍

With a separate instance of the FRAME Assets pallet for foreign assets, do we still need the AssetRegistry pallet in the runtime ? (as far as I understand, we don't need it anymore .. and the runtime & XCM configs must be amended accordingly).

True. I simply didn't know wether it'd be reasonable to simply remove the whole pallet. Should we maybe remove it and place it somewhere else and mark the repository as staged? For now I'll simply remove it from the runtime and afterwards we can decide what to do with it.

Don't we need to adapt the Trader configuration (see AH Rococo's XCM Trader config for inspiration).

No idea about this one. @metricaez ?

Should we update the SafeCallFilter ? (for now, we allow Everything ... which doesn't seem very safe).

We had a similar discussion in the past about another parachain, and we agreed it's OK to leave it like that, as the logic to filter calls is very cumbersome and hard to maintain, while the attack surface is supposed to be very limited. Still, no problem changing it if needed be.

stiiifff · 2023-12-05T12:05:00Z

@stiiifff in order to migrate to 1.3.0 pallet-dex is causing problems, since we'd also have to migrate that pallet. We could save us some time in this regard once this pallet is out of scope when merging this PR.

Given my comments above, I think we might need to upgrade the DEX pallet to v1.3.0 to proceed with the v1.3.0 upgrade so that we can take more time to think about all the impacts of the integration of the Asset Conversion pallet, and bring all the necessary changes in a proper way. Let's not rush it.

Moliholy · 2023-12-05T12:08:15Z

@stiiifff in order to migrate to 1.3.0 pallet-dex is causing problems, since we'd also have to migrate that pallet. We could save us some time in this regard once this pallet is out of scope when merging this PR.

Given my comments above, I think we might need to upgrade the DEX pallet to v1.3.0 to proceed with the v1.3.0 upgrade so that we can take more time to think about all the impacts of the integration of the Asset Conversion pallet, and bring all the necessary changes in a proper way. Let's not rush it.

I already upgraded the dex pallet to 1.3.0. Still, it was causing compilation problems that were very hard to debug.

metricaez · 2023-12-05T17:40:50Z

hi! I'd like to jump in the discussion with some thoughts.

TL:DR I would implement asset conversion without adding the second pallet-assets instance for foreign assets and delete pallet-dex.

I fully understand the reason behind moving into a more standard way of managing assets MultiLocation like foreign assets approach that store this type as part of the AssetId, however I do consider that asset registry does a good job doing so in pretty straightforward way.

I think that implementing a second instance of pallet-assets (and its associated difficulties like Traders) like foreign's makes sense if further logic will be applied to these type of assets, for example in the case of AH only Sibling parachains can create them, different fee's and so. The main reason I see behind foreign assets is to become a reserve but this is just a personal opinion.

Since I think that is not Trappist purpose, I would promote a protectionist approach to assets and implement asset conversion only to local assets, and for outsiders tokens applications keep on limiting ourselves to accept tokens from other chains only as reserve asset transfers with a local derivative created and registered through asset registry that requires sudo, therefore we would be managing only tokens that were willingly approved by sudo/governance.

Overall I think is best to push as much asset related functionality to AH to lower the chances of scams (with scam versions of the same asset) and the creation of multiple exchange rates (one on Trappist, one on AH) that eventually would be arbitraged but that is not always a good experiences for users.

To wrap up, I see the use case for asset management for Trappist to be as follow:

Asset conversion is implemented for converting local assets in/out HOP. For example, we create different games that rewards users with different tokens for each game, to cash them out they should convert them to HOP. That is a case in where asset conversion makes sense.
HOP as our gate to the world via AH, both teleports into the reserve and reserve transfer to sibling chains. This is good for local exchange fees and AH (or any other parachain with exchange functionalities ) wouldn't need to worry to manage and register all the assets inside Trappist ecosystem.
If you want to do business with use, use AH and HOP. This minimize Trader and asset rates issues.
However, on certain particular cases that we want to manage foreign currencies, we do so by creating their derivative in our pallet-asset and registering them on asset-registry, we would only accept reserve transfer of these assets.

Wdyt ?

stiiifff · 2023-12-06T22:36:42Z

@metricaez Great feedback, very insightful. I agree with the approach, and this also has the benefit of showcasing the integration of the FRAME asset conversion pallet (i.e. a DEX based on the Uniswap v2 protocol) in a different way than on the Asset Hub (which is heavily focused on the use case of foreign assets for which the AH is acting as a reserve, as you rightly mentioned).
So, we keep the asset registry, with its simple sudo / governance -based approach of allow-listing (local) derivative assets, instead of an additional instance of the Assets pallet for managing foreign assets.
And the asset conversion pallet is to be configured to allow the creation of liquidity pool for swaps between local assets on Trappist (whether arbitrary fungible assets, or derivative assets).
/cc @Moliholy

Moliholy · 2023-12-07T14:41:39Z

@stiiifff @metricaez I just pushed 4346a71 that removes the foreign asset instance and only allows pallet-asset-conversion to trade locally.

…onversion-pallet

Moliholy · 2023-12-11T12:59:08Z

Updated to latest main branch.

Moliholy self-assigned this Nov 7, 2023

Moliholy force-pushed the feat/Moliholy/asset-conversion-pallet branch 6 times, most recently from 9dfce32 to 8982d63 Compare November 8, 2023 16:15

feat: add asset conversion pallet

eab6896

Moliholy force-pushed the feat/Moliholy/asset-conversion-pallet branch from 8982d63 to eab6896 Compare November 8, 2023 16:26

Moliholy requested review from hbulgarini, stiiifff and metricaez November 8, 2023 16:30

Moliholy marked this pull request as ready for review November 8, 2023 16:31

Merge branch 'main' into feat/Moliholy/asset-conversion-pallet

053556d

Steve Degosserie added 2 commits December 4, 2023 17:06

Merge branch 'main' into feat/Moliholy/asset-conversion-pallet

e54cfe7

Cargo fmt

fdbd320

stiiifff suggested changes Dec 5, 2023

View reviewed changes

Moliholy mentioned this pull request Dec 5, 2023

feat: upgrade trappist to polkadot-v1.3.0 #315

Merged

feat: use only local assets in pallet-asset-coversion

4346a71

Merge remote-tracking branch 'origin/main' into feat/Moliholy/asset-c…

fb3dc89

…onversion-pallet

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add asset conversion pallet #309

feat: add asset conversion pallet #309

Moliholy commented Nov 7, 2023 •

edited

Loading

Moliholy commented Nov 28, 2023

stiiifff Dec 5, 2023

Moliholy Dec 11, 2023

stiiifff Dec 5, 2023

Moliholy Dec 5, 2023

Moliholy Dec 11, 2023

stiiifff Dec 5, 2023 •

edited

Loading

Moliholy Dec 5, 2023 •

edited

Loading

stiiifff commented Dec 5, 2023

Moliholy commented Dec 5, 2023

metricaez commented Dec 5, 2023

stiiifff commented Dec 6, 2023 •

edited

Loading

Moliholy commented Dec 7, 2023 •

edited

Loading

Moliholy commented Dec 11, 2023

feat: add asset conversion pallet #309

Are you sure you want to change the base?

feat: add asset conversion pallet #309

Conversation

Moliholy commented Nov 7, 2023 • edited Loading

Moliholy commented Nov 28, 2023

stiiifff Dec 5, 2023

Choose a reason for hiding this comment

Moliholy Dec 11, 2023

Choose a reason for hiding this comment

stiiifff Dec 5, 2023

Choose a reason for hiding this comment

Moliholy Dec 5, 2023

Choose a reason for hiding this comment

Moliholy Dec 11, 2023

Choose a reason for hiding this comment

stiiifff Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Moliholy Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

stiiifff commented Dec 5, 2023

Moliholy commented Dec 5, 2023

metricaez commented Dec 5, 2023

stiiifff commented Dec 6, 2023 • edited Loading

Moliholy commented Dec 7, 2023 • edited Loading

Moliholy commented Dec 11, 2023

Moliholy commented Nov 7, 2023 •

edited

Loading

stiiifff Dec 5, 2023 •

edited

Loading

Moliholy Dec 5, 2023 •

edited

Loading

stiiifff commented Dec 6, 2023 •

edited

Loading

Moliholy commented Dec 7, 2023 •

edited

Loading