Wallet edit CLI Manager migration #11136
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
james-prysm
reviewed
Aug 1, 2022
cmd/validator/wallet/edit.go
Outdated
| "github.com/urfave/cli/v2" | ||
| ) | ||
|
|
||
| func walletEdit(c *cli.Context) error { |
There was a problem hiding this comment.
can we call this remote wallet edit or something? wallet is pretty generic if the feature is only really used for the remote keymanager configs, might be kind of confusing in the future. one of the old functions is called edit remote wallet which shows more what it's meant for.
There was a problem hiding this comment.
renamed to remoteWalletEdit! good call, thanks!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Refactor/code health
What does this PR do? Why is it needed?
This PR addresses the wallet edit functionality and refactors it as part of #9883. It follows up on #10554, #10686, #10824, #10841, #10890 which do similar migration for the accounts list, delete, backup, exit, and import.
cmd/validator/wallet/edit.go. This is a new file that parses the CLI flags and constructs the CLI manager with the appropriate options. On the CLI manager, it simply calls theWalletEditmethod with thecontext.Context(no longer a need for thecli.Context). The options specific to the import functionality are:WithWalletandWithKeymangerOpts.cmd/validator/wallet/edit_test.go. This is the new location the test that was invalidator/accounts/wallet_edit_test.go. As written, this tests are built for the CLI context interface, so if we are moving the logic for handling to thecmd/validator/walletdirectory, it makes sense to also move the test here. Note that we duplicate a few functions and a struct into this file:setupWalletAndPasswordsDir,testWalletConfig, andsetupWalletCtx. These are defined in different tests in thevalidator/accountsdirectory, and thus aren't importable into this package. One weird aside, these functions are actually also defined in thecmd/validator/accounts/delete_test.gotoo for the same reason (the test depended on them and couldn't import from test files invalidator/acconts. When I tried to create a shared library for bothaccounts/delete_test.goandwallet/edit_test.goI kept running into the following compile error: https://pastebin.com/2n2VB7Ez. I left a TODO in the code to figure this out to remove the duplication.validator/accounts/wallet_edit.go. This is where we see the benefit of this refactor. We chop out all the CLI parsing stuff from theEditWalletConfigurationClifunction, and make it theWalletEditmethod on the AccountsCLIManager pointer receiver.validator/accounts/cli_manager.goandvalidator/accounts/cli_options.go. I added a new option that the wallet edit function depends on:keymanagerOpts *remote.KeymanagerOpts.Which issues(s) does this PR fix?
This is part of the keymanager refactor defined in #9883.
It is also described in #10339.