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

Add runtime for eth relay domain #1203

Merged
merged 5 commits into from
Mar 24, 2023
Merged

Conversation

ParthDesai
Copy link
Contributor

@ParthDesai ParthDesai commented Feb 28, 2023

Description

This PR adds runtime for ethereum relay domain.

The runtime is not integrated in the node in this PR. I will be opening separate PR for that.

Closes #1204

Code contributor checklist:

@ParthDesai ParthDesai closed this Mar 9, 2023
@ParthDesai ParthDesai deleted the add-runtime-for-eth-domain branch March 9, 2023 12:51
@ParthDesai ParthDesai restored the add-runtime-for-eth-domain branch March 16, 2023 09:55
@ParthDesai ParthDesai reopened this Mar 16, 2023
@ParthDesai ParthDesai force-pushed the add-runtime-for-eth-domain branch 3 times, most recently from 23fbfda to 7faba17 Compare March 16, 2023 19:05
@ParthDesai ParthDesai force-pushed the add-runtime-for-eth-domain branch from 7faba17 to 9e78000 Compare March 22, 2023 13:07
@ParthDesai ParthDesai force-pushed the add-runtime-for-eth-domain branch from 9e78000 to 72effe8 Compare March 22, 2023 13:09
@ParthDesai ParthDesai marked this pull request as ready for review March 22, 2023 14:07
Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Looks fine overall.
I'd like to get feedback from @liuchengxu though.
Just some minor nits here.


impl pallet_ethereum_beacon_client::Config for Runtime {
type RuntimeEvent = RuntimeEvent;
// TODO: Replace this with pallet_timestamp once core domains support inherent extrinsics
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure they ever will. Execution is deterministic, timestamp must come from the consensus chain block then.

Comment on lines 369 to 370
Messenger: pallet_messenger = 6,
Transporter: pallet_transporter = 7,
Copy link
Member

Choose a reason for hiding this comment

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

@vedhavyas looks like we'll want this indices to be constants, otherwise feels fragile. Maybe even some integration test if we have infra for that.

Copy link
Member

@vedhavyas vedhavyas Mar 23, 2023

Choose a reason for hiding this comment

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

I dont like it as well. We were already bitten by it. I have a plan to have a safer approach soon that should remove this dependency.

But then yes, maybe constant should be deterrent until then

Copy link
Member

@vedhavyas vedhavyas left a comment

Choose a reason for hiding this comment

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

Make sense.
I wish rename from eth-relay to cor-eth-relay was part of the first commit only.

use subspace_runtime_primitives::{SHANNON, SSC};

#[cfg(any(feature = "std", test))]
pub use sp_runtime::BuildStorage;
Copy link
Member

Choose a reason for hiding this comment

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

not sure if we are using this anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was copied from core-payments runtime.

Upon investigation, It is indeed not in use both here as well as in core-payments. Maybe it is worth removing from core-payments. I have removed it from here.

Copy link
Contributor

@liuchengxu liuchengxu left a comment

Choose a reason for hiding this comment

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

Make sense overall!

Regarding the timestamp, I'm unsure about the entire context, we can discuss how to support it if you can share more background and requirement somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add runtime for ethereum relay core domain
4 participants