-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: implement clientVersionV1 in engine API #8016
Conversation
After some debugging i think i have solved those issues but I am getting this error now:-
any ideas on how to solve this? |
Yes, you need to mark the fields as public. Please see https://doc.rust-lang.org/rust-by-example/mod/struct_visibility.html |
Hey , when I try to run
If I remove it, then it shows me a cyclic package error
Any pointers on how I can get rid of this? |
@guha-rahul Yes, don't use |
@onbjerg Resolved all the errors, should i change it from draft to normal? |
@guha-rahul could you solve merge conflicts here? bump @onbjerg for review |
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.
one suggestion, the implementation looks good! otherwise, I think putting this is reth rpc-types is fine for now given the only added dep is for node-builder, which imports a lot already. I do wonder whether or not it fits better in another crate, but that should not be a blocker for this pr.
@Rjected made the changes and i commented out the CLIENTVERSIONV1 struct from version.rs since it was causing many problems and defined it where the engine-api is initialised. I think we can make do with it being in the rpc-types but do let me know if you want to put it in another crate. |
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.
ty
pending @Rjected
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.
just one thing, otherwise lgtm
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, thanks!
fixes #7668 and based on #7708 which I closed because of branch problems
Added ClientVersionV1to engine.rs and engine_api.rs
Need to add ClientVersionV1 in crates/node/builder/src/launch/mod.rs.
Adding
reth-node-core.workspace =true
inrpc/rpc-engine-api
gives me a cyclic dependency issue.