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

Girazoki local assets #1279

Merged
merged 226 commits into from
Apr 4, 2022
Merged

Girazoki local assets #1279

merged 226 commits into from
Apr 4, 2022

Conversation

girazoki
Copy link
Collaborator

@girazoki girazoki commented Feb 11, 2022

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 in pallet-asset-manager is only root-callable

We 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 to pallet-asset-manager

`

What important points reviewers should know?

Changes in AssetManager Types:

  • AssetType changes to ForeignAssetType
  • A new associated type LocalAssetModifierOrigin, that configures the origin enabled to grant permission to create local assets
  • A new associated type LocalAssetIdCreator, that configures the way of creating an assetId from a caller
  • A new associated type AssetDestroyWitness, which indicates the type of the witness to be passed for the asset destruction
  • A new associated type Currency, which indicates the currency in which we will reserve the local asset deposit
  • A new associated type LocalAssetDeposit, which indicates the required amount to deposit for local assets
  • New functions in the AssetRegistrar trait: create_local_asset, destroy_foreign_asset, destroy_local_asset and destroy_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 to register_foreign_asset
  • a new extrinsic register_local_asset, which creates the local asset and reserve LocalAssetDeposit from the required account
  • a new extrinsic destroy_foreign_asset, which is able to destroy everything related to a foreign asset
  • a new extrinsic destroy_local_asset, which is able to destroy everything related to a local asset, unreserving the previously deposited amount

Changes in AssetManager Storage:

  • A new storage LocalAssetCounter, that counts the number of assets already created
  • A new storage LocalAssetDeposit, that holds the deposit amounts of each of the asset creators

Changes 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 account
  • burn: Allows the owner of the asset to burn tokens from an account
  • freeze: Allows the owner of the asset to freeze an account
  • thar: Allows the owner of the asset to unfreeze an account
  • freeze_asset: Allows the owner of the asset to freeze the entire asset
  • thaw_asset: Allows the owner of the asset to unfreeze the entire asset
  • transfer_ownership: Allows the owner of the asset to transfer the ownership of the asset
  • set_team: Allows the owner of the asset to set issuer, freezer of an asset
  • set_metadata: Allows the owner of the asset to set the metadata associated to the asset
  • clear_metadata: Allows the owner of the asset to clear the metadata associated to the asset

The AccountIdAssetIdConversion trait has been modified to return and receive the prefix associated with the precompile

Changes 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 and LocalFungiblesTransactorOldReanchor. These allow to transfer and receive local assets through XCM witht he following multilocation:

Pre 0.9.16:

MultiLocation {
		parents:1,
		interior: Junctions::X3(
			Parachain(ParachainInfo::parachain_id().into()),
			PalletInstance(<LocalAssets as PalletInfoAccess>::index() as u8)
                        GeneralIndex(asset_id)
		)
}

Post 0.9.16:

MultiLocation {
		parents:0,
		interior: Junctions::X2(
			PalletInstance(<LocalAssets as PalletInfoAccess>::index() as u8)
                        GeneralIndex(asset_id)
		)
}

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

impl pallet_assets::Config<ForeignAssetInstance> for Runtime {
	type Event = Event;
	type Balance = Balance;
	type AssetId = AssetId;
	type Currency = Balances;
	type ForceOrigin = EnsureRoot<AccountId>;
	type AssetDeposit = AssetDeposit;
	type MetadataDepositBase = MetadataDepositBase;
	type MetadataDepositPerByte = MetadataDepositPerByte;
	type ApprovalDeposit = ApprovalDeposit;
	type StringLimit = AssetsStringLimit;
	type Freezer = ();
	type Extra = ();
	type AssetAccountDeposit = AssetAccountDeposit;
	type AssetDestroyWitness = pallet_assets::DestroyWitness;
	type WeightInfo = pallet_assets::weights::SubstrateWeight<Runtime>;
}

impl pallet_assets::Config<LocalAssetInstance> for Runtime {
	type Event = Event;
	type Balance = Balance;
	type AssetId = AssetId;
	type Currency = Balances;
	type ForceOrigin = EnsureRoot<AccountId>;
	type AssetDeposit = AssetDeposit;
	type MetadataDepositBase = MetadataDepositBase;
	type MetadataDepositPerByte = MetadataDepositPerByte;
	type ApprovalDeposit = ApprovalDeposit;
	type StringLimit = AssetsStringLimit;
	type Freezer = ();
	type Extra = ();
	type AssetAccountDeposit = AssetAccountDeposit;
	type AssetDestroyWitness = pallet_assets::DestroyWitness;
	type WeightInfo = pallet_assets::weights::SubstrateWeight<Runtime>;
} 

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)

parameter_types! {
	pub const AssetDeposit: Balance = 100 * currency::UNIT * currency::SUPPLY_FACTOR;
	pub const ApprovalDeposit: Balance = 0;
	pub const AssetsStringLimit: u32 = 50;
	pub const MetadataDepositBase: Balance = currency::deposit(1,68);
	pub const MetadataDepositPerByte: Balance = currency::deposit(0, 1);
}

CurrencyId adds a new enum variant called LocalAssetReserve, and the previous OtherReserve has been renamed to ForeignAsset

pub enum CurrencyId {
	SelfReserve,
	ForeignAsset(AssetId),
	LocalAssetReserve(AssetId),
}
pub enum CurrencyId {
	SelfReserve,
	OtherReserve(AssetId),
}

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?

…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
@girazoki girazoki changed the base branch from master to notlesh-v0.9.18-deps-update April 1, 2022 16:32
Copy link
Contributor

@notlesh notlesh left a 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.

pallets/asset-manager/src/lib.rs Show resolved Hide resolved
pallets/asset-manager/src/lib.rs Show resolved Hide resolved
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) {
Copy link
Contributor

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

Copy link
Collaborator Author

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

pallets/asset-manager/src/migrations.rs Show resolved Hide resolved
pallets/asset-manager/src/weights.rs Show resolved Hide resolved
precompiles/assets-erc20/src/lib.rs Show resolved Hide resolved
Copy link
Contributor

@notlesh notlesh left a 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...

pallets/asset-manager/src/weights.rs Show resolved Hide resolved
pallets/asset-manager/src/lib.rs Show resolved Hide resolved
Base automatically changed from notlesh-v0.9.18-deps-update to master April 1, 2022 18:27
/// 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<
Copy link
Contributor

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...

Copy link
Collaborator Author

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

Copy link
Contributor

@notlesh notlesh left a 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..!

@girazoki girazoki merged commit 01b01b1 into master Apr 4, 2022
@girazoki girazoki deleted the girazoki-local-assets branch April 4, 2022 09:37
@girazoki girazoki restored the girazoki-local-assets branch April 4, 2022 17:31
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes C3-medium Elevates a release containing this PR to "medium priority". D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants