Remove Pallet ethereum-chain-id in favor of frontier evm-chain-id #2379
Conversation
|
Coverage generated "Wed Sep 20 15:03:34 UTC 2023": Master coverage: 87.39% |
There was a problem hiding this comment.
This looks good so far. We may need a migration in order to make sure the new pallet has the same value as the previous one.
Edit (more context): A migration is a FRAME-specific concept where values that are in on-chain storage are modified at the very beginning of a runtime upgrade. In this case, I believe it's necessary because the new pallet's storage item for the chain_id will not be initialized, so it should be copied from the old one.
Feel free to follow up in private for more details :)
| is_pallet_index::<moonriver_runtime::EthereumChainId>(50); | ||
| is_pallet_index::<moonriver_runtime::EvmChainId>(50); |
There was a problem hiding this comment.
We usually try not to reuse pallet indices like this, but it might be ok if the pallets are identical. I think the safer choice is to use a fresh index (53?) though.
There was a problem hiding this comment.
Used 54 as 53 was already used by on old pallet.
There was a problem hiding this comment.
Yes always use a different id for a different pallet, the main reason is to avoid incompatibility, mostly due to the fact that the extrinsics use the index to specify the pallet call.
There was a problem hiding this comment.
I really think we should use the same pallet index (50). The pallet is identical, so I dont think there is a reason not to re-use it. It also has no extrinsics whatsoever, so there is not really a risk I think..
There was a problem hiding this comment.
There's disagreement in the comments here, so I unresolved.
@girazoki do you see any harm in using a fresh pallet index? I agree with your assessment, but also don't see any harm in using a new one.
There was a problem hiding this comment.
not really, using the same one or different one only affects things like XCM (where you rely on extrinsic indices to perform transactions). So I dont see a risk neither using the same nor using a different one
ahmadkaouk
added
the
B7-runtimenoteworthy
ahmadkaouk
added
D9-needsaudit👮
notlesh
added
D1-audited👍
|
@ahmadkaouk @notlesh is this ready to be merged? |
runtime/moonbase/src/lib.rs
Outdated
| @@ -1359,6 +1359,7 @@ construct_runtime! { | |||
| RootTesting: pallet_root_testing::{Pallet, Call, Storage} = 47, | |||
| Erc20XcmBridge: pallet_erc20_xcm_bridge::{Pallet} = 48, | |||
| Multisig: pallet_multisig::{Pallet, Call, Storage, Event<T>} = 49, | |||
| EvmChainId: pallet_evm_chain_id::{Pallet, Storage, Config} = 50, | |||
There was a problem hiding this comment.
A migration is not needed as long as the pallet is named the same and the storage uses the same hashers and same name. This right now requires a migration, but if we keep the previous naming, I dont think we would need one.
CC @notlesh to confirm
There was a problem hiding this comment.
This works for me just as long as we test it
There was a problem hiding this comment.
I would still maintain the name just in case someone is using the pallet (no real reason to change it and there are benefits to not change it)
There was a problem hiding this comment.
I agree, this is essentially a "no-op" and we should probably keep it that way.
What does it do?
Remove Pallet Ethereum Chain Id in favor of Frontier Evm Chain Id
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?