add alloy_chains #5952

1010adigupta · 2024-01-05T12:03:32Z

resolves #5429

1010adigupta · 2024-01-05T12:07:24Z

Hey @mattsse I have just started the work of switching to alloy_chains, just added few lines now.
Actually I was getting the errors related to some traits which are used for Chain and NamedChain in reth earlier are not implemented for alloy_chains:

so should I simultaneously make a pr for this on alloy_chains repo as well?? implementing what is necessary there?
or is there any other solution?

mattsse

the missing impls are hidden behind features

mattsse · 2024-01-05T16:12:50Z

crates/net/eth-wire/src/errors/eth.rs

@@ -101,7 +102,7 @@ pub enum EthHandshakeError {
    MismatchedProtocolVersion(GotExpected<u8>),
    #[error("mismatched chain in status message: {0}")]
    /// Mismatch in chain details in status messages.
-    MismatchedChain(GotExpected<Chain>),
+    MismatchedChain(GotExpected<ChainKind>),


this should remain Chain

mattsse · 2024-01-05T16:13:03Z

crates/net/eth-wire/src/types/status.rs

@@ -24,7 +25,7 @@ pub struct Status {

    /// The chain id, as introduced in
    /// [EIP155](https://eips.ethereum.org/EIPS/eip-155#list-of-chain-ids).
-    pub chain: Chain,
+    pub chain: ChainKind,


should remain Chain

mattsse · 2024-01-05T16:17:03Z

Cargo.toml

@@ -150,6 +150,7 @@ alloy-primitives = "0.5"
 alloy-dyn-abi = "0.5"
 alloy-sol-types = "0.5"
 alloy-rlp = "0.3"
+alloy-chains = "0.1.7"


this needs features:

https://github.com/alloy-rs/chains/blob/a36d918234a40e62f932507448c8c3e1e040be16/src/chain.rs#L110-L175

okay got it

bin/reth/src/args/network_args.rs

1010adigupta · 2024-01-06T17:30:22Z

bin/reth/src/dirs.rs

@@ -12,8 +12,8 @@ use std::{
 /// Constructs a string to be used as a path for configuration and db paths.
 pub fn config_path_prefix(chain: Chain) -> String {
    match chain {


here whether I use Named of from_named, I am getting the same errror, need some help here?

I think this can be used

pub fn config_path_prefix(chain: Chain) -> String { if let Some(named) = chain.named() { named.to_string() } else { chain.id().to_string() } }

should we move this to a Display / ToString impl?

seems like Chain already implements Display (which gets us ToString)

Rjected

needs rebase and some changes to pass tests and satisfy clippy.

for parse test errors this should work: .parse::<alloy_chains::Chain>().unwrap()

crates/net/eth-wire/src/types/status.rs

crates/trie/src/proof.rs

bin/reth/src/init.rs

mattsse

great, one more nit

bin/reth/src/dirs.rs

mattsse

there are lots of unrelated changes (semicolon), please discard those.

and we don't want to add the alloy chains dep everywhere instead we should continue to re-export Chain from the primitives crate

bin/reth/src/args/network_args.rs

bin/reth/Cargo.toml

mattsse · 2024-01-10T10:23:08Z

bin/reth/src/cli/db_type.rs

@@ -101,7 +101,7 @@ impl DatabaseInstance {
 #[cfg(test)]
 mod tests {
    use super::*;
-    use reth_primitives::Chain;
+    use alloy_chains::Chain;


this should still use from pritimitives

ig but the db functions are using alloy_chains::Chain, if I use reth_primitives::Chain it will give a mismatch type error

it should be the same exact type.

bin/reth/src/commands/debug_cmd/merkle.rs

bin/reth/src/commands/import.rs

bin/reth/src/commands/node/mod.rs

1010adigupta · 2024-01-10T11:37:05Z

there are lots of unrelated changes (semicolon), please discard those.

and we don't want to add the alloy chains dep everywhere instead we should continue to re-export Chain from the primitives crate

I have removed the semicolon issue, can you give more details on the second one?

mattsse

sorry I overlooked that this PR didn't actually remove the all the reth_primitive::Chain types:

https://github.com/paradigmxyz/reth/blob/main/crates/primitives/src/chain/mod.rs#L99-L99

which was the actual goal of this PR

mattsse · 2024-01-10T13:55:37Z

bin/reth/src/cli/db_type.rs

@@ -101,7 +101,7 @@ impl DatabaseInstance {
 #[cfg(test)]
 mod tests {
    use super::*;
-    use reth_primitives::Chain;
+    use alloy_chains::Chain;


it should be the same exact type.

bin/reth/Cargo.toml

mattsse

great!

I simply removed the reth-primitive chain def and replaced it with the reexport.

now this is complete, ty!

completed

gakonst · 2024-01-12T22:16:29Z

bin/reth/src/args/utils.rs

-pub const SUPPORTED_CHAINS: &[&str] = &["base", "base_goerli", "base_sepolia"];
+pub const SUPPORTED_CHAINS: &[&str] = &["base", "base-goerli", "base-sepolia"];


this is a breaking change in the CLI right, we should communicate that in the release notes

yeah but this now mirrors what op-geth accepts so this is a good change

Trial solution

3d0687a

1010adigupta changed the title ~~Trial solution~~ add alloy_chains Jan 5, 2024

Trial solution

e738624

mattsse requested changes Jan 5, 2024

View reviewed changes

mattsse added the C-debt Refactor of code section that is hard to understand or maintain label Jan 5, 2024

1010adigupta added 2 commits January 6, 2024 22:53

Chain and NamedChain replaced

71a940b

Error correction

124d388

1010adigupta commented Jan 6, 2024

View reviewed changes

bin/reth/src/args/network_args.rs Outdated Show resolved Hide resolved

1010adigupta commented Jan 6, 2024

View reviewed changes

1010adigupta requested a review from mattsse January 6, 2024 17:30

DaniPopes self-requested a review January 8, 2024 16:54

mattsse marked this pull request as ready for review January 9, 2024 14:36

mattsse requested review from rkrasiuk, Rjected, onbjerg and gakonst as code owners January 9, 2024 14:36

Final corrections added

d540b39

Rjected previously requested changes Jan 9, 2024

View reviewed changes

crates/net/eth-wire/src/types/status.rs Show resolved Hide resolved

crates/trie/src/proof.rs Show resolved Hide resolved

bin/reth/src/init.rs Outdated Show resolved Hide resolved

mattsse requested changes Jan 10, 2024

View reviewed changes

bin/reth/src/dirs.rs Outdated Show resolved Hide resolved

1010adigupta requested review from rakita, joshieDo and shekhirin as code owners January 10, 2024 10:18

1010adigupta requested review from mattsse and Rjected January 10, 2024 10:19

mattsse requested changes Jan 10, 2024

View reviewed changes

1010adigupta closed this Jan 10, 2024

1010adigupta reopened this Jan 10, 2024

some issues resolved

9b66089

1010adigupta requested a review from mattsse January 10, 2024 11:41

mattsse requested changes Jan 10, 2024

View reviewed changes

mattsse added 5 commits January 12, 2024 14:18

Merge branch 'main' into add-alloy_chains

ea191c2

remove reth native chain type

c37e37c

some cleanup

bdd64eb

bootnode docs

0ce8ab1

rm unused

962969a

mattsse approved these changes Jan 12, 2024

View reviewed changes

mattsse enabled auto-merge January 12, 2024 13:40

mattsse added 2 commits January 12, 2024 14:51

fix tests

1344d07

chore: use kebab case for default chains

e23f0a0

mattsse added this pull request to the merge queue Jan 12, 2024

Merged via the queue into paradigmxyz:main with commit 3ce9fcf Jan 12, 2024
28 checks passed

gakonst reviewed Jan 12, 2024

View reviewed changes

gakonst mentioned this pull request Jan 12, 2024

feat: add activation_block method for Ethereum hardforks #5723

Merged

1010adigupta deleted the add-alloy_chains branch January 16, 2024 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add alloy_chains #5952

add alloy_chains #5952

1010adigupta commented Jan 5, 2024 •

edited

Loading

1010adigupta commented Jan 5, 2024 •

edited

Loading

mattsse left a comment

mattsse Jan 5, 2024

mattsse Jan 5, 2024

mattsse Jan 5, 2024

1010adigupta Jan 6, 2024

1010adigupta Jan 6, 2024

supernovahs Jan 7, 2024

gakonst Jan 9, 2024

Rjected Jan 9, 2024

Rjected left a comment

mattsse left a comment

mattsse left a comment

mattsse Jan 10, 2024

1010adigupta Jan 10, 2024

mattsse Jan 10, 2024

1010adigupta commented Jan 10, 2024

mattsse left a comment

mattsse Jan 10, 2024

mattsse left a comment

gakonst Jan 12, 2024

mattsse Jan 12, 2024 •

edited

Loading

		pub const SUPPORTED_CHAINS: &[&str] = &["base", "base_goerli", "base_sepolia"];
		pub const SUPPORTED_CHAINS: &[&str] = &["base", "base-goerli", "base-sepolia"];

add alloy_chains #5952

add alloy_chains #5952

Conversation

1010adigupta commented Jan 5, 2024 • edited Loading

1010adigupta commented Jan 5, 2024 • edited Loading

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rjected left a comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1010adigupta commented Jan 10, 2024

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

1010adigupta commented Jan 5, 2024 •

edited

Loading

1010adigupta commented Jan 5, 2024 •

edited

Loading

mattsse Jan 12, 2024 •

edited

Loading