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

Pallet assets precompiles #827

Merged
merged 149 commits into from
Sep 30, 2021
Merged

Pallet assets precompiles #827

merged 149 commits into from
Sep 30, 2021

Conversation

girazoki
Copy link
Collaborator

@girazoki girazoki commented Sep 17, 2021

What does it do?

Currently pallet-assets storage is non-public nor it has traits to check, e.g., whether a specific approval has been issued. This means we cannot use the approval storage from pallet-assets, and thats the reason why the allowance getter is for now disabled. This paritytech/substrate#9757 against substrate should solve this issue

The way we assign addresses to AssetIds, being this u128 type, is by making the first 4 bytes 0XFF and then attach the assetId 16 bytes. So 0XFFFFFFFF + Bytes(AssetId). Whenever we receive a call with such an address format, we decode the assetId part of it and check its existence in our runtime. However, since the storage is not public in pallet-assets, what we do for now is check whether the total_supply is zero. The total supply of a non-existent asset returns 0, so I think its good enough to only route through precompiles assets with non-zero supply

The approval in pallet-assets is incremental, so in order to match the ERC20 interface we had to clean the previous approval and instantiate a new one

Is there something left for follow-up PRs?

Yes, increase and decrease approval precompiles plus enable allowance once substrate allows it

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?

@JoshOrndorff
Copy link
Contributor

We recently learned that adding a new precompile requires a migration that writes the bogus revert code to evm storage at the right address. We'll have to think about how/when to do that for this one that may have many different precompile addresses.

@girazoki
Copy link
Collaborator Author

I can add that to the asset registration process. That should be easy

@girazoki
Copy link
Collaborator Author

Or add an root extrinsic for when we want to set this code in the evm storage in asset-manager.

@JoshOrndorff
Copy link
Contributor

JoshOrndorff commented Sep 23, 2021

I can add that to the asset registration process. That should be easy

That makes sense. I recommend introducing a trait like OnAssetRegistered. And then adding a type that implements that trait in your precompiles.

I think that would be better than directly updating the bytecode from your asset manager, because doing it directly would require the T: pallet_evm::Config bound which otherwise makes no sense for the asset manager.

@girazoki
Copy link
Collaborator Author

girazoki commented Sep 27, 2021

I incuded it in the AssetRegistrar trait that we already have for asset-manager. This trait is implemented in the runtime and basically instructs how to register an asset for the asset manager, which now includes:

  • Asset registration in pallet-assets
  • Asset metadata setting in pallet-assets
  • precompile address revert code insertion

@girazoki
Copy link
Collaborator Author

As per our feedback, I commented it back the revert code insertion until we feel ready

Copy link
Contributor

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

I've left some nitpicks. I'm fine addressing them in follow-ups if you want to get this in quickly.

precompiles/assets-erc20/src/mock.rs Outdated Show resolved Hide resolved
precompiles/assets-erc20/src/mock.rs Outdated Show resolved Hide resolved
runtime/moonbase/src/precompiles.rs Show resolved Hide resolved
runtime/moonbase/tests/common/mod.rs Outdated Show resolved Hide resolved
@girazoki girazoki merged commit 5d80f8b into master Sep 30, 2021
@girazoki girazoki deleted the pallet-assets-precompiles branch September 30, 2021 12:02
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants