Routed Pablo LP Token Creation Through Assets Registry [No-CU] #2606
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
PoisonPhang
requested review from
easteregg,
seunlanlege,
obsessed-cake,
benluelo,
dzmitry-lahoda and
vimukthi-git
|
|
||
| let lp_token = T::CurrencyFactory::create(RangeId::LP_TOKENS)?; | ||
| let lp_token = T::AssetsRegistry::register_asset( | ||
| XcmAssetLocation::LOCAL_NATIVE, |
There was a problem hiding this comment.
This would not work AFAIK as we need to use a unique ID here
There was a problem hiding this comment.
Tracking an ever incrementing next LP token ID in Pablo would be needed
There was a problem hiding this comment.
It calls into currency factory
There was a problem hiding this comment.
No I mean in asset-registry this value is used as a key for ForeignToLocal so this would not work after the first LP token if it's not unique
There was a problem hiding this comment.
so this should perhaps be called something like "create_and_register"?
There was a problem hiding this comment.
Tracking an ever incrementing next LP token ID in Pablo would be needed
Two+ counters/nonce for one type of token seems like a terrible misuse of storage
There was a problem hiding this comment.
hmm not really that bad when you think about the way Multilocation supposed to work https://github.com/paritytech/xcm-format#71-description. So Pablo is at the protocol/smart contract layer. Eventually we can migrate the assets-registry to remove tracking of another local identifier all together and just use Multilocation for mappings. This way we decentralize all asset "unique" ID tracking to each protocol and get rid of the currency factory at the same time. Anyway more stuff to add to your RFC later. For now just generating and storing a nonce is ok for Pablo IMO.
There was a problem hiding this comment.
This asset should also never be considered foreign. We'll eventually want AR (or something else) to be able to route between foreign and local assets. Maybe it's worth taking the time to at least prepare the interface of AR for that. Even if we don't go all the way with a full asset revamp just yet.
This would allow us to dynamically create local assets and, with a Multi Location, create foreign assets without unnecessary complexity and keeping storage abuse at a minimum.
I think something like Moonbeam's Asset Registrar can be implemented here but with more elegance.
There was a problem hiding this comment.
This asset should also never be considered foreign.
Yes, as we agreed it's a temp solution. We can use the multi location to understand if the given asset is foreign or not later.
We'll eventually want AR (or something else) to be able to route between foreign and local assets
yes for BYOG I guess.
This would allow us to dynamically create local assets and, with a Multi Location, create foreign assets without unnecessary complexity and keeping storage abuse at a minimum.
Yes!
I think something like Moonbeam's Asset Registrar can be implemented here but with more elegance.
That would be cool to evolve into :)
There was a problem hiding this comment.
I made updates to #2489 with my thoughts on this system
|
Closing as this will instead be handled by #2624 |
Issue
LP Tokens were getting created without a ratio, leading to an ED of
Balance::MAX