Introduce Symphony chains replacing Eth chain structures from `ethers` #26

dereksione · 2023-06-29T14:05:55Z

Closes #7

This PR naively just substitutes all instances of goerli with the symphony devnet chain and seppolia with testnet.

This does not address any of the work highlighted in #13, #14, #17, #19, #24, #25

Those are going to be future improvements based off of this PR

…o 8-new-primitive-crate

dereksione · 2023-06-29T15:31:51Z

Meta: I realized that the massive PR approach that I was taking was just not working out, so I'm splitting those up into more atomic PRs, and even though the interim code is a little ugly sometimes to make it compile, it will be more manageable in terms of requesting reviews and making consistent and transparent progress

joroshiba · 2023-06-29T19:13:00Z

crates/symphony-primitives/src/chains.rs

+
+        write!(f, "symphony-{chain_name}") 
+    }
+}


nit: new line

joroshiba · 2023-06-29T19:13:08Z

crates/symphony-primitives/src/constants.rs

+
+pub const MAINNET_ID: u64 = 70047;
+pub const DEVNET_ID: u64 = 70048;
+pub const TESTNET_ID: u64 = 70049;


nit: new line

joroshiba · 2023-06-29T19:13:17Z

crates/symphony-primitives/src/lib.rs

+pub mod constants;
+pub mod chains;
+
+pub use chains::{SymphonyChains, SymphonyChainError};


nit: new line

joroshiba · 2023-06-29T19:23:39Z

crates/primitives/src/chain/mod.rs

 pub enum Chain {
    /// Contains a known chain
-    Named(ethers_core::types::Chain),
+    Named(SymphonyChains),


Could we fork ethers potentially instead and update as others have. This might solve the test issues above. We could also then potentially reuse goerli and sepolia names but point at OUR named in ethers devnet and testnet chains. Would likely negate most of the changes here?

We could, but I have a suspicion that they are going to replace this with an alloy implementation soon, so maybe we can hold off on that? Open to suggestions

It looks like there will "soon" be an alloy-rs/chains repo.

It seems either way there is likely a refactor of this work, if alloy-rs/chains is very different we need to redo, and it might make more sense to push our chain definition into there than here.

@joroshiba Do you think it's a bad idea to temporarily park it here and then upstream it when they move to alloy?

My first thought when looking at the primitives and the uncommented changes was also: "this will be annoying to rebase on top of reth changes".

If ethers/alloy have done something akin to introducing new chains, maybe we can follow in their footsteps? Even if they will soon replace ethers will alloy this means that we can profit from their well worn path.

crates/primitives/src/chain/mod.rs

SuperFluffy

On first glance the changes look reasonable.

If however this symphony fork is happening at the wrong level (re @joroshiba's comments further down) I would ask to consider introducing our fork in a similar way that ethers/alloy are doing.

Forking is a big commitment if we want to stay up-to-date with upstream and forking the wrong thing can lead to a lot of work down the road.

Cargo.toml

bin/reth/Cargo.toml

SuperFluffy · 2023-07-03T10:15:39Z

crates/primitives/Cargo.toml


 [dependencies]
+#symphony
+symphony-primitives = { workspace = true }


I realize I have never thought about this too much - but why is this a workspace dependency rather than a path dependency? I.e.:

symphony-primitives = { path = "../symphony-primitives" }

SuperFluffy · 2023-07-03T10:22:30Z

crates/symphony-primitives/src/chains.rs

+}
+
+#[derive(Debug)]
+pub enum SymphonyChainError {


IMO the errors need not be grouped but can be 2 separate error types (I made them wrap () so they cannot be constructed outside this crate):

pub struct UnrecognizedChainId(()); // Clearer name for the second error pub struct UnrecognizedChainName(());

Also, please add doc comments on the errors, but especially implement std::error::Error.

SuperFluffy · 2023-07-03T10:27:35Z

crates/symphony-primitives/src/constants.rs

@@ -0,0 +1,7 @@
+pub const MAINNET_NAME: &str = "MAINNET";


Could they be lower-cased by default if they are only displated as to_lowercase() in their display impl?

SuperFluffy · 2023-07-03T10:32:28Z

crates/primitives/src/chain/mod.rs

-
-    type Strategy = BoxedStrategy<Chain>;
-}
+// FIXME: what does this do?


This stuff is relevant to set up the proptest further down (which you probably know 😄; but the FIXME was here, so I am answering the question here ^^ ).

SuperFluffy · 2023-07-03T10:33:45Z

crates/primitives/src/chain/mod.rs

    }

-    /// Returns bootnodes for the given chain.
+    /// Returns bootnodes for the given chain. 


redundant space at the end

SuperFluffy · 2023-07-03T10:33:59Z

crates/primitives/src/chain/mod.rs

-            Sepolia => Some(sepolia_nodes()),
-            _ => None,
-        }
+        todo!()


probably remove the todo! before merge?

Need to keep the todo because this function is used in other places

SuperFluffy · 2023-07-03T10:34:15Z

crates/primitives/src/chain/mod.rs

    pub fn public_dns_network_protocol(self) -> Option<String> {
-        use ethers_core::types::Chain::*;
-        const DNS_PREFIX: &str = "enrtree://AKA3AM6LPBYEUDMVNU3BSVQJ5AD45Y7YPOHJLEF6W26QOE4VTUDPE@";
+        todo!()


remove todo! before merge?

SuperFluffy · 2023-07-03T10:42:47Z

crates/primitives/src/chain/mod.rs

 pub enum Chain {
    /// Contains a known chain
-    Named(ethers_core::types::Chain),
+    Named(SymphonyChains),


My first thought when looking at the primitives and the uncommented changes was also: "this will be annoying to rebase on top of reth changes".

If ethers/alloy have done something akin to introducing new chains, maybe we can follow in their footsteps? Even if they will soon replace ethers will alloy this means that we can profit from their well worn path.

Co-authored-by: Richard Janis Goldschmidt <[email protected]>

dereksione and others added 5 commits June 29, 2023 06:17

feat(primitives): add new crate symphony-primitives

df182aa

wip: adding new chains ds

6955cd6

feat(primitives): naive chain metadata replacements

1d7577d

Merge branch 'symphony-dev' into 8-new-primitive-crate

e7d2e44

Merge branch '8-new-primitive-crate' of github.com:astriaorg/reth int…

1d8a0e3

…o 8-new-primitive-crate

dereksione changed the title ~~8 new primitive crate~~ Introduce Symphony chains replacing Eth chain structures from ethers Jun 29, 2023

dereksione self-assigned this Jun 29, 2023

dereksione marked this pull request as ready for review June 29, 2023 14:38

dereksione requested review from SuperFluffy, joroshiba and sambukowski June 29, 2023 15:28

joroshiba reviewed Jun 29, 2023

View reviewed changes

sambukowski reviewed Jun 29, 2023

View reviewed changes

crates/primitives/src/chain/mod.rs Show resolved Hide resolved

sambukowski reviewed Jun 29, 2023

View reviewed changes

crates/primitives/src/chain/mod.rs Show resolved Hide resolved

dereksione and others added 3 commits June 29, 2023 21:23

fix: addressing comments

694b7bb

fix: rustfmt

2875309

Merge branch 'symphony-dev' into 8-new-primitive-crate

107d6a0

SuperFluffy reviewed Jul 3, 2023

View reviewed changes

Apply suggestions from code review

11944a8

Co-authored-by: Richard Janis Goldschmidt <[email protected]>

Introduce Symphony chains replacing Eth chain structures from ethers #26

Are you sure you want to change the base?

Introduce Symphony chains replacing Eth chain structures from ethers #26

Uh oh!

Conversation

dereksione commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dereksione commented Jun 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SuperFluffy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Introduce Symphony chains replacing Eth chain structures from `ethers` #26

Introduce Symphony chains replacing Eth chain structures from `ethers` #26

dereksione commented Jun 29, 2023 •

edited

Loading