[pallet-revive] Add chain ID to config an runtime API#5807
Conversation
Signed-off-by: xermicus <cyrill@parity.io>
|
bot fmt |
|
@xermicus https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7408786 was started for your command Comment |
|
@xermicus Command |
| /// This is a unique identifier assigned to each blockchain network, | ||
| /// preventing replay attacks. | ||
| #[pallet::constant] | ||
| type ChainId: Get<u64>; |
There was a problem hiding this comment.
Why not U256 if that is the native format of the ID?
There was a problem hiding this comment.
People seem to generally pick them within the bounds of 64bits. I guess they want to have easily readable for humans? Also there is no ConstU256 yet. But it this be fine, I hope humanity never needs more than 2**64 blockchains :P
There was a problem hiding this comment.
Ah I have the same patch in my branch I used u32, there..., both are fine I guess
There was a problem hiding this comment.
Ok let's stick with u64. As long as we don't need to convert back to u64 it's fine.
There was a problem hiding this comment.
On the RPC side I'm not sure but for the runtime I don't see where we would need to parse this ever.
There was a problem hiding this comment.
However even if so, I feel like this would need to be a fallible conversion anyways (you can parse any U256 but if you want it to match against the configured chain ID, if the U256 is > u64::MAX you know that the chain ID would not match anyways).
| type Xcm = (); | ||
| type RuntimeMemory = ConstU32<{ 128 * 1024 * 1024 }>; | ||
| type PVFMemory = ConstU32<{ 512 * 1024 * 1024 }>; | ||
| type ChainId = ConstU64<{ 0 }>; |
There was a problem hiding this comment.
You mind setting it to 42 here, I need to reconfigure my Metamask extension otherwise 🙃
There was a problem hiding this comment.
What do you mean by extension? Shouldn't Metamask just use an RPC to query to chain id?
There was a problem hiding this comment.
Also this ChainId is not really used. It is just the default derive that can be used for tests. But it is overriden in tests.
There was a problem hiding this comment.
if you configure a local network with metamask, you need to specify the chain_id, you can update it later on, but from what I experienced there is a bug and it still use the old value, so you delete your custom network and recreate it.
There was a problem hiding this comment.
42 is already taken and I thought this should either be zero or something that's not taken yet? (Something that's not taken yet could however get taken later on). WDYT?
There was a problem hiding this comment.
sounds good then, I just need to remember to change this once I rebase on top of this
| /// Returns the chain ID. | ||
| /// See [`pallet_revive_uapi::HostFn::chain_id`]. | ||
| #[api_version(0)] | ||
| fn chain_id(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { | ||
| Ok(self.write_fixed_sandbox_output( |
There was a problem hiding this comment.
Doesn't charge any gas except the syscall overhead. Needs a benchmark.
There was a problem hiding this comment.
Ah yes good catch. Since this doesn't really do anything except writing a constant to contract memory and the syscall overhead is apparently already charged, would the RuntimeCosts::CopyToContract(len) do it instead of a dedicated benchmark?
There was a problem hiding this comment.
Yes this should be fine. However, we need to remember to make our seal_input benchmark better (where this weight is based in). Right now it just does a memory copy. Which is probably fair with in-runtime PolkaVM but with a JIT there will be some constant overhead for a copy.
There are probably also many other "getters" which don't access storage that could be refactored this way.
There was a problem hiding this comment.
Yeah right I see. Just added the gas token for now, thanks!
Signed-off-by: xermicus <cyrill@parity.io>
|
bot fmt |
|
@xermicus https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7411720 was started for your command Comment |
|
@xermicus Command |
This PR adds the EVM chain ID to Config as well as a corresponding runtime API so contracts can query it.
Related issue: paritytech/revive#44