paritytech / trappist

Apache License 2.0
81 stars 17 forks source link

feat: add asset conversion pallet #309

Open Moliholy opened 8 months ago

Moliholy commented 8 months ago

Closes #281

Moliholy commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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:

Wdyt ?

stiiifff commented 7 months ago

@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 commented 7 months ago

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

Moliholy commented 6 months ago

Updated to latest main branch.