[CU-37t8q47] Added RFC 0013 - Depracate Currency Factory #2489
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
| * > As for asset metadata, we don't have standards for formatting this nor do we | ||
| utilize it. | ||
| Is this true? A breif investigation revealed that outside of `scripts/polkadot-launch/initialization/src/interfaces/basilisk/definitions.ts` | ||
| there is no mention of the Asset Metadata structure. |
There was a problem hiding this comment.
As for asset metadata, we don't have standards for formatting this nor do we
utilize it.
Is this true? A breif investigation revealed that outside of scripts/polkadot-launch/initialization/src/interfaces/basilisk/definitions.ts there is no mention of the Asset Metadata structure.
Nix commands for this PRNOTE: You can also run our Nix commands in Docker. See the bottom of this comment. Make sure you have setup the Composable community cache:(you only need to run it once on your machine) nix-shell -p cachix --command "cachix use composable-community"Show all possible apps, shells and packages:Run the Composable node alone:nix run "github:ComposableFi/composable/7511ace292892b0f826c5ef4df3c89019d4b8099#composable-node" -LSpin up a local devnet:nix run "github:ComposableFi/composable/7511ace292892b0f826c5ef4df3c89019d4b8099#devnet" -L --option sandbox relaxed --show-traceSpin up a local XCVM devnet:nix run "github:ComposableFi/composable/7511ace292892b0f826c5ef4df3c89019d4b8099#devnet-xcvm" -LView the docs:nix run ".#docs-server"Run this without Nix in Docker.docker run --rm -v /var/run/docker.sock:/var/run/docker.sock -v nix:/nix -it nixos/nix bash -c "nix-env -iA nixpkgs.cachix && cachix use composable-community && nix run github:ComposableFi/Composable/7511ace292892b0f826c5ef4df3c89019d4b8099#devnet-dali -L --extra-experimental-features nix-command --extra-experimental-features flakes"NOTE: You can swap For more info on how to use Nix, check out our Nix docs Note that the initial build may take about one hour if it has not been cached by our CI yet. Once it is cached, builds should take about one minute. We currently do not provide build caches for ARM machines such as M1 Macs, but building on ARM is supported. |
PoisonPhang
force-pushed
the
connor/CU-37t8q47_deprecate-currency-factory
branch
from
db2890c
to
40ffd87
Compare
PoisonPhang
requested review from
dzmitry-lahoda,
KaiserKarel and
vimukthi-git
PoisonPhang
force-pushed
the
connor/CU-37t8q47_deprecate-currency-factory
branch
from
6a0c4ef
to
3e0b229
Compare
|
|
||
| * Hold functionality is not present (We depend on this in several cases) | ||
|
|
||
| * **Moonbeam's pallet-asset-manager** |
There was a problem hiding this comment.
we should not use assets-manager imho. i used moonbeam just as example of good use of pallet-assets (it is proves assets are good for ETH bridges )
There was a problem hiding this comment.
pallet-asset-manager - idea we will have our own assets-registy which will be tune for our needs.
it is important to check that pallet-assets can be wrapped
There was a problem hiding this comment.
Agree for not using Moonbeam stuff but we can learn from their impl :)
|
|
||
| #### Cons | ||
|
|
||
| * No asset ID reservation system, only prevents duplicates |
There was a problem hiding this comment.
we do not need. we just runtime config assets to be premissioned. when we will have permisionless assets they may already have reservation (any way making fork with reservation is easy later)
There was a problem hiding this comment.
so really we do not care lack of reservation system
There was a problem hiding this comment.
+1
I've included this in the solution section
|
|
||
| * No asset ID reservation system, only prevents duplicates | ||
|
|
||
| * Hold functionality is not present (We depend on this in several cases) |
There was a problem hiding this comment.
we can use assets-registry here. it can do next
- implement orml traits
- implement substrate traits
- implement free/reserved/hold/lock in consistent/property tested/fuzzied way
- route calls to assets
There was a problem hiding this comment.
implement free/reserved/hold/lock in consistent/property tested/fuzzied way
Is there any particular reason that hold and lock/freeze are implemented differently?
|
please document that we need data migrations if using assets from orml tokens. also idea of using assets is that it is forward compatible. it will get reservations/locks/holds/etc/XCM/ERC/etc support as time goes. we will just throw away and migrate. please also review governance. Specifically how assets governance and gov-v2 fit together. |
|
|
||
| * Very nice UI compatibility with Polkadot Dashboard | ||
|
|
||
| * Maintained by Parity |
There was a problem hiding this comment.
very important - very nice and clear and good documents are here. writing pallet is ony of work.
|
|
||
| 2. Configure instance for external assets | ||
|
|
||
| 3. Route calls between the two instances |
There was a problem hiding this comment.
Looking for comments on this.
I was considering using Assets Registry as a routing layer, but that may be out of scope for Assets Registry.
| ### Simplify & Move the Asset ID Reservation System | ||
|
|
||
| One way to remove the need for pallet-currency-factory is to move its | ||
| functionality to pallet-asset-registry. If we also simplify the asset ID |
There was a problem hiding this comment.
Is our old substrate version holding us back from using newer features? And if so, what is keeping us on an older substrate version?
| * If we already have assets in various ranges, the runtime migration process | ||
| will become more complicated as we translate them to existing in one range. | ||
|
|
||
| ### Remove Asset ID Reservation |
There was a problem hiding this comment.
What is the purpose of asset id reservation? (this is a gap in my knowledge, but a short explanation/ refresher would be nice to have in this document regardless)
There was a problem hiding this comment.
Ensuring new asset IDs don't collide with existing ones
There was a problem hiding this comment.
that would be solved by using a single nonce right?
There was a problem hiding this comment.
It could be, yes
There was a problem hiding this comment.
This is related to https://github.com/ComposableFi/composable/pull/2489/files#r1037535250, I think that section could be updated with this
There was a problem hiding this comment.
Maybe not in that section specifically, but somewhere (lol)
There was a problem hiding this comment.
We need to support this as most asset ID creations in our system would be non-permissioned(no hard coded or permissioned extrinsic based creation) eventually
| After analyzing possible solutions, the best course of action to deprecate | ||
| Currency Factory is to maintain a fork of Parity's pallet-assets, embrace the | ||
| design Moonbeam uses to maintain two instances of their pallet, and to remove | ||
| our asset ID reservation system (for release two). |
There was a problem hiding this comment.
what benefits does forking bring over simply wrapping the pallet?
There was a problem hiding this comment.
Could we upstream the changes we need? This may be a more long-term task, so possibly out of scope for what we want to do with this RFC; perhaps a "future possibilities" section would be good?
There was a problem hiding this comment.
Up-streaming is an option, probably not one for release two tho.
There was a problem hiding this comment.
makes sense, i think that should be mentioned as explicitly out of scope then
Co-authored-by: benluelo <57334811+benluelo@users.noreply.github.com> Signed-off-by: Connor Davis <17688291+PoisonPhang@users.noreply.github.com>
|
|
||
| ### No Permissionless Asset Creation | ||
|
|
||
| > We do not have permissionless asset (non-payable/no sufficient) - it is |
There was a problem hiding this comment.
I don't really get this statement, why do we need to create new assets for governance and airdrops?
And what is meant by "no marketable assets"?
There was a problem hiding this comment.
If @dzmitry-lahoda could elaborate on this, that would be appreciated
| Additionally, Currency Factory failed to meet some requirements that we should | ||
| enforce in a future solution. | ||
|
|
||
| * Permissionless asset creation can be done without collision in the asset ID |
There was a problem hiding this comment.
The requirement is not clear here. Why do we need permissionless asset creation? and through what interface? through an extrinsic or pallet interface trait?
|
|
||
| * Permissioned asset creation can be done without collision in the asset ID | ||
|
|
||
| * Asset metadata is available |
There was a problem hiding this comment.
We need to be more specific here what type of meta data are we talking about?
| utilize it. Assuming we replace the reservation system, Currency Factory will be | ||
| left without value. | ||
|
|
||
| ## Requirements |
There was a problem hiding this comment.
I recommend following MosCow prioritization to understand if a requirement is actually a requirement while also understanding priority.
| enforce in a future solution. | ||
|
|
||
| * Permissionless asset creation can be done without collision in the asset ID | ||
|
|
There was a problem hiding this comment.
Are we missing any migration requirement from the existing solution to any new solution?
|
|
||
| 1. Asset IDs & Balances | ||
|
|
There was a problem hiding this comment.
Instead of the complexity of the enum, routing can be handled internally by the pallet by confirm if some storage items exist or not - at the cost of storage access
| * Local | ||
| * Foreign | ||
|
|
||
| Moonbeam maintains separate instances of Parity's pallet-assets to track each of |
There was a problem hiding this comment.
This approach sounds neat for our asset-registry to eventually evolve in to
|
|
||
| * Asset metadata is available | ||
|
|
||
| * Can registrar both local and foreign asset types |
There was a problem hiding this comment.
Can we try to define what assets are considered foreign(assets created by other sovereign chains than the current executing chain) vs local(assets created by the chain that executes the logic). This will be helpful for anyone trying to understand how we handle assets.
There was a problem hiding this comment.
did not looked, but likely moonbeam uses Call filters to prevent any calls to extrinsic on foreign assets registry to prevent any minting or burning even via sudo.
|
one thing, all assets in Statemine have an implicit location. As it has only locally issued assets and only one pallet. It just harcoded translation layer for any new asset to to (0, 50, asset_id). So they do not store default values. |
|
|
||
| * Can registrar both local and foreign asset types | ||
|
|
||
| * Support XCM Multi Locations for foreign assets |
There was a problem hiding this comment.
I think this one is possibly a sub-requirement from the one before.
| ### Simplify & Move the Asset ID Reservation System | ||
|
|
||
| One way to remove the need for pallet-currency-factory is to move its | ||
| functionality to pallet-asset-registry. If we also simplify the asset ID |
There was a problem hiding this comment.
In order to get stuff done without mixing all topics together we need to constrain each topic to solving key concerns. There are two key concerns about general asset management AFAIK in our code base.
- Creating and tracking assets available (assets-registry, currency-factory)
- Minting/transferring/holding ... already known/registered assets (pallet-assets -> [pallet-balances, orml-tokens])
So to constrain and get stuff done, 2 above is not the subject of this RFC. That is a separate concern.
I also see that frame/pallet-assets does handle both but it does not matter as we can easily plug that in to place as long as we separate concerns.
PS: I also think we need to upgrade substrate ASAP, longer we delay the more complicated it becomes. So possible things to consider for release 3.
| change. | ||
|
|
||
| This retains the functionality of Currency Factory, and does not require a new | ||
| pallet. However, this does not enable permissionless asset creation without the |
There was a problem hiding this comment.
This solution(let's number them please) has the deficiency of making another centralized place to create asset Ids, which we don't need with a name spacing system like MultiLocation(as discussed on slack). So the only thing we need is to allow use of MultiLocation and creation of assetIds for each name space by the owner(eg: pallet-pablo for LP tokens + PBLO) of that name space.
|
|
||
| #### Technical Implementation | ||
|
|
||
| Instead of reserving asset IDs via our current range system, we could simply |
There was a problem hiding this comment.
This can be done withing each name space as proposed above :)
|
|
||
| #### Consequences | ||
|
|
||
| * If we already have assets in various ranges, the runtime migration process |
There was a problem hiding this comment.
No need with the above suggested solution
|
|
||
| #### Consequences | ||
|
|
||
| * Minor runtime migration will be needed for deleting Currency Factory's |
There was a problem hiding this comment.
Yes though we do not need this in release 3 if we prevent Pablo from creating LP tokens in release 2.
| * Minor runtime migration will be needed for deleting Currency Factory's | ||
| storage. | ||
|
|
||
| ### Use an Externally Maintained Asset ID Reservation System |
There was a problem hiding this comment.
This is a separate concern as proposed before, we can focus on concern one for release 2. But not a problem to document the solution. I agree in concept though we need to assess how well it supports our requirements such as asset locking at the time of implementation(release 3 or after)
|
|
||
| * Hold functionality is not present (We depend on this in several cases) | ||
|
|
||
| * **Moonbeam's pallet-asset-manager** |
There was a problem hiding this comment.
Agree for not using Moonbeam stuff but we can learn from their impl :)
|
|
||
| #### Consequences | ||
|
|
||
| * Any of these options will require a difficult runtime migration as we move |
There was a problem hiding this comment.
I don't think runtime migration is difficult if we design and plan the steps correctly.
| ## Conclusion | ||
|
|
||
| After analyzing possible solutions, the best course of action to deprecate | ||
| Currency Factory is to maintain a fork of Parity's pallet-assets, embrace the |
There was a problem hiding this comment.
Note that pallet-assets is still a work in progress https://github.com/paritytech/substrate/issues/8453 , we need fungibles::*Hold traits for staking in release 3, which pallet-assets does not support. Any solution suggesting adoption of that pallet need to take into account orml-tokens features that we need for release 3. I would rather deal with a migration into pallet-assets(it supports that for genesis https://github.com/paritytech/substrate/blob/master/frame/assets/src/lib.rs#L362) later than delay our feature roadmap. Note that the number of tokens(LPT * 3, PBLO, pool shares * 3) would not be that large even after release 3 and staking. We can even hard code a migration for that.
I propose the following steps to impl in brief
As finalized in discussion we propose,
Release 2:
Make sure that we don't use currency-factory and create and track(1) the assets in assets-registry with.MultiLocationeven for local assetsAllow Pablo to create assets(LPT) through assets-registry (avoid use of currency-factory)- Hard code assets in the runtime for initial LPT,(KSM/USDT & PICA/USDT),Make sure one can not use local assets to pay gas fees in XCM transfers (Which seems to be the case AFAIK)- For ED for LPT: Asset ratios gets defined in a suitable way at the time of creation for LP (hard code so that we can have the smallest possible LP tokens for an account in the runtime)
Release 3:
- Introduce staking with pool share tokens (Just for context, not related to RFC), need a migration to create the initial staking pools + share tokens anyway .
- Remove currency-factory all together (Just to minimize the changes for release 2)
- Possibly upgrade substrate to latest production version (assess later).
Release 4 or later
- Assess/Introduce frame/pallet-assets with a migration for existing set of assets if
fungibles::*Holdtrait impls are available. Need to take into account the existing asset IDs. - Decide if we can name space token IDs so that there is no global nonce.
- Remove use of our own pallet-assets together with orml-tokens use.
|
Closing in favor of paritytech/substrate#2877 |
Issue
[CU-37t8q47]
Additional Changes
This is an RFC, please comment on any concerns.