-
Notifications
You must be signed in to change notification settings - Fork 2.1k
refactor: extract Chainspec type from primitives #8055
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
refactor: extract Chainspec type from primitives #8055
Conversation
mattsse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, a few nits and suggestions
crates/storage/chainspec/Cargo.toml
Outdated
| license.workspace = true | ||
| repository.workspace = true | ||
| rust-version.workspace = true | ||
| description = "Commonly used types in reth." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some extra imports, will remove them when i get back electricity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs regular lib.rs header, see other crates for ref
crates/primitives/Cargo.toml
Outdated
| revm.workspace = true | ||
| revm-primitives = { workspace = true, features = ["serde"] } | ||
|
|
||
| reth_chainspec.workspace = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is your error
| reth_chainspec.workspace = true | |
| reth-chainspec.workspace = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a cycle dependency error:-
error: cyclic package dependency: package `reth-chainspec v0.2.0-beta.6 (/home/rahul/reth/crates/chainspec)` depends on itself. Cycle:
package `reth-chainspec v0.2.0-beta.6 (/home/rahul/reth/crates/chainspec)`
... which satisfies path dependency `reth-chainspec` (locked to 0.2.0-beta.6) of package `reth-primitives v0.2.0-beta.6 (/home/rahul/reth/crates/primitives)`
... which satisfies path dependency `reth-primitives` (locked to 0.2.0-beta.6) of package `reth-chainspec v0.2.0-beta.6 (/home/rahul/reth/crates/chainspec)`
... which satisfies path dependency `reth-chainspec` (locked to 0.2.0-beta.6) of package `reth-engine-primitives v0.2.0-beta.6 (/home/rahul/reth/crates/engine-primitives)`
... which satisfies path dependency `reth-engine-primitives` (locked to 0.2.0-beta.6) of package `reth-basic-payload-builder v0.2.0-beta.6 (/home/rahul/reth/crates/payload/basic)`
... which satisfies path dependency `reth-basic-payload-builder` (locked to 0.2.0-beta.6) of package `reth v0.2.0-beta.6 (/home/rahul/reth/bin/reth)`
... which satisfies path dependency `reth` (locked to 0.2.0-beta.6) of package `beacon-api-sse v0.0.0 (/home/rahul/reth/examples/beacon-api-sse)`
Should removing reth-chainspec from crates/primitives solve it?
mattsse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also need to delete chainspec from primitives crate
|
hey @mattsse , just a quick clarification , when i am removing |
|
these functions we should also move |
|
|
all chainspec usage needs to be removed from reth-primitives if these are functions then we can move them to the new crate |
|
hey @mattsse , any feedback on the above commits? |
mattsse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, need to take a closer look at all the deps, but in essence this is exactly what we want
mattsse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I made some progress here on figuring out how things should be structured.
what we need to do is keep a dependency on reth-primitives in the chainspec crate, so chainspec depends on reth-primitives
this should do it
|
Regarding the failed tests, i am struggling to understand why i am getting errors in |
|
just updated |
Rjected
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you remove the old chainspec now that it's in a separate crate, along with everything else that was copied to this new crate? we don't want to keep any duplicate files around
|
I also want merge this after we have merged all changes in #8214 |
hey when I am trying to import the new chainspec in place of the old one , I am getting cyclic dependency errors and @mattsse previously mentioned we cannot import chainspec in reth-primitives since it will cause a cyclic dependency error. |
mattsse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Rjected
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
thank you @guha-rahul! |
re: #8674
Fixes: #7929
The error i am facing -
Even tho i have
reth-chainspec = { path = "crates/storage/chainspec" }under[workspace.dependencies].