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

refactor: move crates into crates folder and remove redundant prefixes #1624

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

KolbyML
Copy link
Member

@KolbyML KolbyML commented Jan 7, 2025

What was wrong?

  • Having all the crates in / made it so we always had to update our dockerfiles/dockerignore when new crates are added
  • Our root project directory was getting pretty big
  • we had a lot of redundant prefixes on our paths, trin-* x,y,z

Both https://github.com/paradigmxyz/reth and https://github.com/lambdaclass/ethrex

use this general structure, I think it is very clean and makes it easier to expand our project going forward

How was it fixed?

moving folders, renaming a few as well

To-Do in a future PR

I am leaving

  • portal bridge and trin execution for now, eventually we can put there binaries in the bin folder and make crates out of them and put them in the crates folder. I kinda like trin-execution being in its own folder though, but I think the refactoring would make sense

I want to move utp-testing and ethportal-peertests to the testing folder added in #1621

But #1621 would need to be merged first so that will probably happen in a follow up PR

@KolbyML KolbyML self-assigned this Jan 7, 2025
@KolbyML KolbyML force-pushed the move-crates-into-crates-folder branch 6 times, most recently from c4df32f to 0dd6390 Compare January 7, 2025 18:53
Copy link
Collaborator

@njgheorghita njgheorghita left a comment

Choose a reason for hiding this comment

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

I'm 👍 for the direction of this pr, but I also think this needs a 👍 from everyone before merging

Cargo.toml Outdated
"crates/rpc",
"crates/beacon",
"crates/evm",
"crates/history",
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick ... maybe we could leave the prefix on the history / state / beacon crates? Sometimes when you have to make a change to one of these, you typically have to replicate it across all three, so it's nice when they're grouped together alphabetically

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking of putting them in a folder
so
crates/subnetworks/history
crates/subnetworks/state
crates/subnetworks/history

What do you think? I am open to ideas, I am fine doing the suggestion above, I think a folder would be a little cleaner though

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, works for me!

@KolbyML KolbyML force-pushed the move-crates-into-crates-folder branch 2 times, most recently from 17a810b to fd1509f Compare January 9, 2025 17:27
@KolbyML
Copy link
Member Author

KolbyML commented Jan 9, 2025

the book ci is failing here is a PR to fix it #1628

@KolbyML KolbyML force-pushed the move-crates-into-crates-folder branch from fd1509f to e4fa0f2 Compare January 9, 2025 17:45
Copy link
Member

@ogenev ogenev left a comment

Choose a reason for hiding this comment

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

I'm OK with these changes, but as Nick suggested, everyone from the team needs to review this.

Copy link
Member

Choose a reason for hiding this comment

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

I still see the ethportal-api/src/test_utils folder in the root folder on my GitHub review dashboard, could you confirm that the whole ethportal-api folder is removed from the root?

Copy link
Member Author

Choose a reason for hiding this comment

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

image

I don't see it, maybe it is a visual bug on your dashboard?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, maybe, It looks good now.

@KolbyML KolbyML force-pushed the move-crates-into-crates-folder branch from e4fa0f2 to e8eb455 Compare January 10, 2025 11:39
Copy link
Collaborator

@morph-dev morph-dev left a comment

Choose a reason for hiding this comment

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

LGTM

I like this change 🚢

@KolbyML KolbyML force-pushed the move-crates-into-crates-folder branch from e8eb455 to a9c7769 Compare January 13, 2025 15:54
@KolbyML KolbyML force-pushed the move-crates-into-crates-folder branch from a9c7769 to 1ebb006 Compare January 13, 2025 15:59
Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

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

Discussed live. GTG with follow-ups to match crate names to paths, and have consistent crate names.

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.

6 participants