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

Enable Xcm->Evm and Erc20Xcm on Moonriver & Moonbeam #2318

Merged
merged 77 commits into from
Jun 6, 2023

Conversation

librelois
Copy link
Collaborator

@librelois librelois commented May 30, 2023

What does it do?

Enables the following features in Moonriver & Moonbeam runtimes:

  • Installs pallet-ethereum-xcm, so that XCM to EVM calls are possible.
  • Installs a new XCM barrier accepting DescendOrigin + WithdrawAsset + BuyExecution messages.
  • Installs a new way of deriving xcm accounts by hashing (same as in Moonbase).
  • Installs pallet-erc20-xcm-bridge, so that ERC20 transfers through XCM are possible.
  • Adds new xcm weights to Moonriver & Moonbeam
  • Updates tests, both typescript and rust. In typescript, I added a new describeMoonbeamDevAllRuntimes, to make sure I configured everything correctly for all runtimes

What important points reviewers should know?

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?

@librelois librelois added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. breaking Needs to be mentioned in breaking changes labels May 30, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 30, 2023

Coverage generated "Tue May 30 10:49:08 UTC 2023":
https://s3.amazonaws.com/moonbeam-coverage/pulls/2318/html/index.html

Master coverage: 70.87%
Pull coverage: 71.15%

@librelois librelois mentioned this pull request May 30, 2023
24 tasks
@librelois librelois changed the title Enable Xcm->Evm and Erc20Xcm on Moonriver Enable Xcm->Evm and Erc20Xcm on Moonriver & Moonbeam May 30, 2023
@librelois librelois marked this pull request as ready for review May 30, 2023 10:39
@@ -324,3 +324,14 @@ export function describeDevMoonbeamAllEthTxTypes(
describeDevMoonbeam(title + " (EIP1559)", cb, "EIP1559", "moonbase", wasm);
describeDevMoonbeam(title + " (EIP2930)", cb, "EIP2930", "moonbase", wasm);
}

export function describeDevMoonbeamAllRuntimes(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this being used anywhere? Some uncommitted file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I had started working locally on extending some XCM ts tests to moonriver and moonbeam, but most xcm tests need sudo

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is the democracy helper for that but it will get removed once we move to Referenda.
Do you think it would make sense to restore sudo under a feature flag in the runtimes for testing purposes ?
Or to allow the client to override storage for testing (not sure the impact on the test however, specially the tree checks)

Copy link
Collaborator

@girazoki girazoki Jun 1, 2023

Choose a reason for hiding this comment

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

I'd be interested on the feature flag, the last time I tried I could not make it work since I could not make it under the construct_runtime macro..

Copy link
Collaborator Author

@librelois librelois Jun 1, 2023

Choose a reason for hiding this comment

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

I think it's much simpler to add an RPC method allowing the client to modify the storage, I think we should avoid having to compile a special runtime just for testing.

Weight::from_parts(
T::Erc20TransferGasLimit::get().saturating_mul(T::WeightPerGas::get().ref_time()),
0,
gas_limit / 4, // TODO: apply gas/proof_size ratio
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not related to this PR but could erc20-xcm-bridge not be a pallet? it has no events, no storage and no extrinsic AFAICT. What is the purpose of making it a pallet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@girazoki The point of making it a pallet is to identify the multilocation namespace by a PalletIndex. It allows remote chains to query which pallet index is being used to send us erc20s.

Copy link
Collaborator

@girazoki girazoki Jun 1, 2023

Choose a reason for hiding this comment

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

Oh ok, this could have been done with some other junction though, maybe worth giving it a thought. Although it might be already late. Not very important though

@librelois librelois merged commit a294944 into master Jun 6, 2023
30 checks passed
@librelois librelois deleted the elois-xcm-evm-moonriver branch June 6, 2023 09:32
@librelois librelois added not-breaking Does not need to be mentioned in breaking changes and removed breaking Needs to be mentioned in breaking changes labels Jun 6, 2023
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jul 5, 2023
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 D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants