-
Notifications
You must be signed in to change notification settings - Fork 358
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
Girazoki local assets #1279
Girazoki local assets #1279
Conversation
…post-0.9.16-anchoring-logics-for-native-token' into girazoki-enable-native-asset-cross-chain-transfer-in-moonriver
…post-0.9.16-anchoring-logics-for-native-token' into girazoki-enable-native-asset-cross-chain-transfer-in-moonriver
…one from calling non-local methods for foreign assets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far. This is the first half[ish] of my review.
Excellent documentation, btw.
AssetTypeUnitsPerSecond::<T>::remove(&asset_type); | ||
|
||
// Only if the old asset is supported we need to remove it | ||
if let Ok(index) = supported_assets.binary_search(&asset_type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More of a note-to-self: make sure this vec is sorted in all cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we always insert them retrieving their potential index with binary_search. So I think this should be enough to guarantee they are sorted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More incremental review. I don't understand why GH sometimes lets met do one-off comments and sometimes makes me do them through a review...
/// Means for transacting local assets that are not the native currency | ||
/// This transactor uses the old reanchor logic | ||
/// Remove once we import https://github.com/open-web3-stack/open-runtime-module-library/pull/708 | ||
pub type LocalFungiblesTransactorOldReanchor = FungiblesAdapter< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Old" and "New" make sense now, but they will have lost their context in the future :) Not a big deal if they are temporary...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am planning to remove them as soon as we release 1400 so I think that's fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I left quite a bit of feedback, but nothing that I would want to hold this up from being merged.
I also haven't made it through the tests thoroughly -- there's a lot of coverage there..!
…moonbeam into girazoki-local-assets
What does it do?
It adds support for local asset creation in Moonbase and Moonriver. It does so by adding a new instance of the assets pallet, which can be configured independently to the one for foreign xcm assets. For now I kept the previous instance with the name Assets, while the new one is called LocalAssets. Changing the previous Assets name would imply a migration so for now I wanted to avoid that.
The new precompile range for the local assets is the 0xFFFFFFFE range, while the precompiles address is composed as 0xFFFFFFFE + assetId.
The assetId cannot be chosen arbitrarely by the user, as it might intentionally be willing to cause a collision between our precompile address and a contract he intentionally deploys. Therefore the assetId is created as Hash(LocalAssetCounter), a new incremental storage value in pallet-asset-manager.
Since we want to restric the local asset creation at the beginning, the new
register_local_asset
extrinsic inpallet-asset-manager
is only root-callableWe need to handle both pre and post 0.9.16 anchoring versions of the asset, and thus we support handling the asset with both prefixes.
Local assets can be created as sufficients and non-sufficients. Since sufficients can only be created through force_create, we are forced to used this
pallet-assets
method to create local assets. This method does not reserve any deposit, hence the reason for us to move the currency reservation topallet-asset-manager
`
What important points reviewers should know?
Changes in AssetManager Types:
AssetType
changes toForeignAssetType
LocalAssetModifierOrigin
, that configures the origin enabled to grant permission to create local assetsLocalAssetIdCreator
, that configures the way of creating an assetId from a callerAssetDestroyWitness
, which indicates the type of the witness to be passed for the asset destructionCurrency
, which indicates the currency in which we will reserve the local asset depositLocalAssetDeposit
, which indicates the required amount to deposit for local assetscreate_local_asset
,destroy_foreign_asset
,destroy_local_asset
anddestroy_asset_weight_info
. The reason for this is that creating and destroying assets involve changes in more than one pallet, and more specifically in pallet-assets and pallet-evm to remove the revert code. Therefore we need a way to configure this from our runtime. The weight info information is necessary to be passed since we cannot benchmark properly the asset destruction because the witness type has private fields.Changes in AssetManager Extrinsics:
register_asset
extrinsic changes toregister_foreign_asset
register_local_asset
, which creates the local asset and reserve LocalAssetDeposit from the required accountdestroy_foreign_asset
, which is able to destroy everything related to a foreign assetdestroy_local_asset
, which is able to destroy everything related to a local asset, unreserving the previously deposited amountChanges in AssetManager Storage:
LocalAssetCounter
, that counts the number of assets already createdLocalAssetDeposit
, that holds the deposit amounts of each of the asset creatorsChanges in Assets ERC20 precompiles:
The following functions are added only for local assets:
mint
: Allows the owner of the asset to mint tokens to an accountburn
: Allows the owner of the asset to burn tokens from an accountfreeze
: Allows the owner of the asset to freeze an accountthar
: Allows the owner of the asset to unfreeze an accountfreeze_asset
: Allows the owner of the asset to freeze the entire assetthaw_asset
: Allows the owner of the asset to unfreeze the entire assettransfer_ownership
: Allows the owner of the asset to transfer the ownership of the assetset_team
: Allows the owner of the asset to set issuer, freezer of an assetset_metadata
: Allows the owner of the asset to set the metadata associated to the assetclear_metadata
: Allows the owner of the asset to clear the metadata associated to the assetThe
AccountIdAssetIdConversion
trait has been modified to return and receive the prefix associated with the precompileChanges in the Moonbase and Moonriver runtimes:
First, the asset configuration has been splitted from the runtime file to allow for easier readibility of the code. We now have an
asset_config.rs
file in each of the runtimes, holding the asset configuration.New transactors added
LocalFungiblesTransactorNewReanchor
andLocalFungiblesTransactorOldReanchor
. These allow to transfer and receive local assets through XCM witht he following multilocation:Pre 0.9.16:
Post 0.9.16:
Two pallet_assets instances, one representing the previous foreign assets and one representing the new local assets with deposits set based on https://github.com/paritytech/cumulus/blob/2bcd7b535a904b646a8055cfff0b2fe6ac37c1e1/polkadot-parachains/statemine/src/lib.rs#L226. For now both instances are configured identically, but this might change in the future
The approval deposit has been set to 100 UNIT in the case of Moonbase, 100 MOVR in the case of Moonriver, and 1000 GLMR for Moonbeam (although this is not used for now)
CurrencyId adds a new enum variant called
LocalAssetReserve
, and the previousOtherReserve
has been renamed toForeignAsset
Is there something left for follow-up PRs?
What alternative implementations were considered?
Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
What value does it bring to the blockchain users?