-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 op-reth
binary to separate crate
#10641
Conversation
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.
great job
do mind trying to fix the commit history on this branch? usually does it by checking out a backup branch of your diff from main, then on this branch |
you mean squashing the diff into less number of commits without merge commits? |
no need to touch up the history of the branch imo, this all gets squashed into main anyway |
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.
as long as the diff in 'Files changed' isn't showing unrelated changes, and other people are comfortable reviewing (@joshieDo), then I'm fine with unrelated changes showing in GitHub commit history for the pr
@@ -71,7 +71,7 @@ jobs: | |||
- uses: Swatinem/rust-cache@v2 | |||
with: | |||
cache-on-failure: true | |||
- run: cargo hack check | |||
- run: cargo hack check --workspace --exclude op-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.
think you will have to add a new job for op-reth, we still want to check lint for op code
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.
added a step to run cargo check
on it with optimism feature
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.
hell yeah, let's try this again hehe
ref #9439
I've changed
OpChainSpecParser
to return an innerChainSpec
for now to avoid introducing more complex bounds on cli types. We can change it back once we figure out how the bounds should look like on underlying types