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.

Sign up for GitHub

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

Jump to bottom

refactor: extract Chainspec type from primitives #8055

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

refactor: extract Chainspec type from primitives #8055

wants to merge 13 commits into from
+62,109 −0

Conversation

guha-rahul
Copy link
Contributor

Fixes: #7929
The error i am facing -

error: failed to load manifest for workspace member `/home/rahul/reth/bin/reth`

Caused by:
  failed to load manifest for dependency `reth-basic-payload-builder`

Caused by:
  failed to load manifest for dependency `reth-engine-primitives`

Caused by:
  failed to load manifest for dependency `reth-chainspec`

Caused by:
  failed to load manifest for dependency `reth-primitives`

Caused by:
  failed to parse manifest at `/home/rahul/reth/crates/primitives/Cargo.toml`

Caused by:
  error inheriting `reth_chainspec` from workspace root manifest's `workspace.dependencies.reth_chainspec`

Caused by:
  `dependency.reth_chainspec` was not found in `workspace.dependencies`

Even tho i have reth-chainspec = { path = "crates/storage/chainspec" } under [workspace.dependencies] .

mattsse
mattsse requested changes May 2, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a 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 Show resolved Hide resolved
crates/storage/chainspec/Cargo.toml Outdated
license.workspace = true
repository.workspace = true
rust-version.workspace = true
description = "Commonly used types in reth."
Copy link
Collaborator

Choose a reason for hiding this comment

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

needs update

Copy link
Contributor Author

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.

crates/storage/chainspec/src/lib.rs Outdated
Copy link
Collaborator

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
@@ -18,7 +18,7 @@ reth-ethereum-forks.workspace = true
reth-rpc-types.workspace = true
revm.workspace = true
revm-primitives = { workspace = true, features = ["serde"] }

reth_chainspec.workspace = true
Copy link
Collaborator

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

Suggested change
reth_chainspec.workspace = true
reth-chainspec.workspace = true

Copy link
Contributor Author

@guha-rahul guha-rahul May 2, 2024 •
edited

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 mattsse added the C-debt A section of code is hard to understand or change label May 2, 2024
mattsse
mattsse requested changes May 2, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a 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

@guha-rahul
Copy link
Contributor Author

hey @mattsse , just a quick clarification , when i am removing reth-chainspec from primitives, there are some functions that are imported to other crates, eg: revm_spec_by_timestamp_after_merge, so for those we have to explicitly pass the 'reth-primitives' or there is something i am missing?

@mattsse
Copy link
Collaborator

mattsse commented May 3, 2024

these functions we should also move

@guha-rahul
Copy link
Contributor Author

these functions we should also move
I have identified 5 functions in the reth-primitives crate that are being used in other crates and they have chainspec in params.
eg: revm_spec_by_timestamp_after_merge is defined in primitive crate and i am having a mismatch type in reth-engine-primitives crate.
Can you please elaborate more on how we should move the functions?

@mattsse
Copy link
Collaborator

mattsse commented May 3, 2024 •
edited

all chainspec usage needs to be removed from reth-primitives

if these are functions then we can move them to the new crate

@guha-rahul guha-rahul requested a review from mattsse May 4, 2024 04:28
@guha-rahul
Copy link
Contributor Author

guha-rahul commented May 4, 2024 •
edited

hey @mattsse , any feedback on the above commits?

mattsse
mattsse reviewed May 4, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a 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 mattsse self-assigned this May 4, 2024
@mattsse mattsse mentioned this pull request May 6, 2024
Merged
mattsse
mattsse requested changes May 7, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a 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

@guha-rahul guha-rahul requested a review from mattsse May 8, 2024 21:16
mattsse
mattsse reviewed May 8, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this is great, ty.

need to think about how to phase out the primitives code, but this new crate is exactly what we want

@guha-rahul
Copy link
Contributor Author

this is great, ty.

need to think about how to phase out the primitives code, but this new crate is exactly what we want

Do we need to replace it with the alloy-primitives?

@Rjected
Copy link
Member

Rjected commented May 22, 2024

bump @mattsse, this looks ok, the old json genesis assets can be deduplicated probably?

@guha-rahul
Copy link
Contributor Author

Regarding the failed tests, i am struggling to understand why i am getting errors in crates/net/eth-wire-types/src/receipt.rs

@Rjected
Copy link
Member

Rjected commented May 24, 2024

just updated Cargo.lock which was preventing some tests from running

Rjected
Rjected requested changes May 24, 2024
View reviewed changes
Copy link
Member

@Rjected Rjected left a 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

@mattsse
Copy link
Collaborator

mattsse commented May 25, 2024

I also want merge this after we have merged all changes in #8214

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse requested changes

@Rjected Rjected Rjected requested changes

@rakita rakita Awaiting requested review from rakita

@joshieDo joshieDo Awaiting requested review from joshieDo

@shekhirin shekhirin Awaiting requested review from shekhirin

@onbjerg onbjerg Awaiting requested review from onbjerg

@emhane emhane Awaiting requested review from emhane

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@mattsse mattsse

Labels
C-debt A section of code is hard to understand or change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

extract Chainspec type from primitives
3 participants
@guha-rahul @mattsse @Rjected