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

Migrate validator client to clap derive #6300

Open
wants to merge 46 commits into
base: unstable
Choose a base branch
from
Open

Migrate validator client to clap derive #6300

wants to merge 46 commits into from
+644 −762

Conversation

eserilev
Copy link
Collaborator

@eserilev eserilev commented Aug 23, 2024 •
edited
Loading

Issue Addressed

Partially #5900

Proposed Changes

Migrate the validator client cli to clap derive

@eserilev eserilev changed the title Migrate validator manager to clap derive Migrate validator client to clap derive Aug 23, 2024
eserilev
eserilev commented Aug 23, 2024
View reviewed changes
validator_client/src/config.rs Outdated
Comment on lines 270 to 275
if validator_client_config.web3_signer_keep_alive_timeout == 0 {
config.web3_signer_keep_alive_timeout = None
} else {
config.web3_signer_keep_alive_timeout = Some(Duration::from_millis(
validator_client_config.web3_signer_keep_alive_timeout,
));
Copy link
Collaborator Author

@eserilev eserilev Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Note the breaking change here. Since I've defined web3_signer_keep_alive_timeout as a u64, null is no longer a valid option. Users will need to use 0 instead.

If this breaking change is too intrusive I can define web3_signer_keep_alive_timeout as a string and we can keep functionality as is.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably ok, not sure if anyone actually remove the web3signer timeout? (cc @michaelsproul)
I've added backwards-incompat label.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is probably obscure enough that we would be OK to change it. It's a bit dodgy because it flips the meaning of 0 from "timeout immediately" to "never timeout", but I doubt anyone is using 0

@eserilev eserilev mentioned this pull request Aug 23, 2024
Open
8 tasks
@eserilev eserilev marked this pull request as ready for review August 23, 2024 02:50
@eserilev eserilev added the ready-for-review The code is ready for review label Aug 23, 2024
@eserilev eserilev added work-in-progress PR is a work-in-progress and removed ready-for-review The code is ready for review labels Aug 23, 2024
@eserilev eserilev added ready-for-review The code is ready for review work-in-progress PR is a work-in-progress and removed work-in-progress PR is a work-in-progress ready-for-review The code is ready for review labels Sep 3, 2024
@eserilev eserilev removed the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Nov 1, 2024
@jimmygchen jimmygchen added the backwards-incompat Backwards-incompatible API change label Nov 5, 2024
jimmygchen
jimmygchen requested changes Nov 5, 2024
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Looks good overall! I've added some comments, let me know what you think.

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Nov 5, 2024
@eserilev
Copy link
Collaborator Author

eserilev commented Nov 7, 2024

thanks for the review Jimmy, I've pushed up changes based on your feedback
02e305a

@eserilev eserilev added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Nov 7, 2024
@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Nov 7, 2024
jimmygchen
jimmygchen approved these changes Nov 7, 2024
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Thanks @eserilev, looks great!

@michaelsproul
Copy link
Member

Maybe we should merge this PR first, as it will conflict:

@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-merge This PR is ready to merge. labels Nov 8, 2024
@michaelsproul
Copy link
Member

Age's PR is merged now. Some conflicts to resolve and then we can merge this.

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

@macladson macladson macladson left review comments

@michaelsproul michaelsproul michaelsproul left review comments

@jimmygchen jimmygchen jimmygchen approved these changes

Assignees
No one assigned
Labels
backwards-incompat Backwards-incompatible API change code-quality waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eserilev @michaelsproul @jimmygchen @macladson