Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CU-37t8q47] Added RFC 0013 - Depracate Currency Factory #2489

Closed
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .config/dictionaries/terminology.dic
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ Currency
unignore
modularly
admined
mintable

coeff # coefficient
recvs # recieves
Expand Down
93 changes: 93 additions & 0 deletions rfcs/0013-deprecate-currency-factory.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
# Depreciation of Currency Factory - Assessment RFC

This assessment aims to gather all relevant details and consequences of
depreciating Currency Factory and its replacements.

## Reasoning

There are several reasons we stand to benefit from replacing Currency Factory.
These reasons are detailed below:

### Currency Factory Ranges are Confusing
PoisonPhang marked this conversation as resolved.
Show resolved Hide resolved

As detailed in [this](https://composablefinance.slack.com/archives/C031G5NT0CA/p1667492928188269)
PoisonPhang marked this conversation as resolved.
Show resolved Hide resolved
thread, the Currency Factory ranges are a confusing abstraction that we don't
stand to gain from. We really only have two types of currencies native
(mintable) and non-native (external). However, we have many more ranges that
PoisonPhang marked this conversation as resolved.
Show resolved Hide resolved
enforce artificial limitations on various abstractions of these two currency
types. This complicates the currency creation process and leads to confusion
while providing no benefit to us.

### Currency Factory is Already Losing Functionality

Recently, we moved away from storing ED in Currency Factory. This leaves two
uses for currency factory: reserving asset IDs and storing asset metadata.

As we have already discussed, currency factory over complicates the asset ID
PoisonPhang marked this conversation as resolved.
Show resolved Hide resolved
reservation system. As for asset metadata, we don't have standards for
formatting this nor do we utilize it.

Assuming we replace the reservation system, Currency Factory will be left
without value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we missing any migration requirement from the existing solution to any new solution?

## Solution Proposals
PoisonPhang marked this conversation as resolved.
Show resolved Hide resolved

There are multiple ways we could go about replacing Currency Factory. These
solutions are detailed below:

### 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. we are 4 month old orml-tokens. what state they have now? may be they did some stuff for non payable assets and substrate assets compatibility?
  2. could we move functionally to our assets pallet? can we make our assets pallet to be as compatible as possible with substrate assets?

so in theory we can ditch currency factory and move all to our assets in a way compable on binary level with substrate assets.

also we can throw away our governance-registry imho

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are 4 month old orml-tokens. what state they have now? may be they did some stuff for non payable assets and substrate assets compatibility?

We'll either need to request another back-port or upgrade our runtime to enjoy their newer additions

could we move functionally to our assets pallet? can we make our assets pallet to be as compatible as possible with substrate assets?

Like you concluded later, I think maintaining a fork would be the easiest way to get all the benefits of Parity's pallet-assets

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is our old substrate version holding us back from using newer features? And if so, what is keeping us on an older substrate version?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. Creating and tracking assets available (assets-registry, currency-factory)
  2. 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.

reservation system, this will be a minimal change.
PoisonPhang marked this conversation as resolved.
Show resolved Hide resolved

Instead of reserving asset IDs via our current range system, we could simply
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be done withing each name space as proposed above :)

have a nonce for reserving new asset IDs while ensuring a lack of collisions.
This nonce can start at an arbitrary but high number so that our hard-coded
asset IDs are still safe (i.e `u32::MAX`). This would reduce the complexity of
PoisonPhang marked this conversation as resolved.
Show resolved Hide resolved
our current reservation system while still avoiding collision.

### Use an Externally Maintained Asset ID Reservation System
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


Neither option presented seems to handle the reserving of asset IDs. Therefore,
neither of these will fully replace currency factory and would more correctly
replace our own pallet-assets. A replacement for pallet-assets may be a good
idea but would require much more substantial changes A replacement for
pallet-assets may be a good idea but would require much more substantial
changes.

* **Parity's pallet-assets**

#### Pros
PoisonPhang marked this conversation as resolved.
Show resolved Hide resolved

* Very nice UI compatibility with Polkadot Dashboard

* Maintained by Parity
Copy link
Contributor

@dzmitry-lahoda dzmitry-lahoda Nov 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very important - very nice and clear and good documents are here. writing pallet is ony of work.


* Simple Interface

#### Cons

* No asset ID reservation system, only prevents duplicates
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so really we do not care lack of reservation system

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
I've included this in the solution section


* Hold functionality is not present (We depend on this in several cases)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can use assets-registry here. it can do next

  1. implement orml traits
  2. implement substrate traits
  3. implement free/reserved/hold/lock in consistent/property tested/fuzzied way
  4. route calls to assets

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implement free/reserved/hold/lock in consistent/property tested/fuzzied way

Is there any particular reason that hold and lock/freeze are implemented differently?


* **Moonbeam's pallet-asset-manager**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree for not using Moonbeam stuff but we can learn from their impl :)


#### Pros

* Maintained by moonbeam

#### Cons

* Partially specialized

* No asset ID reservation system, only prevents duplicates

## Questions

* > As for asset metadata, we don't have standards for formatting this nor do we
utilize it.

Is this true? A brief investigation revealed that outside of `scripts/polkadot-launch/initialization/src/interfaces/basilisk/definitions.ts`
there is no mention of the Asset Metadata structure.