[Test Coverage Improvement] Validator Manager Flags#2690
[Test Coverage Improvement] Validator Manager Flags#2690sukantoraymond merged 36 commits intomainfrom
Conversation
felipemadero
left a comment
There was a problem hiding this comment.
I would have preferred to move the flags removal to another PR, it seems an easy step to be done, and we can focus here in the flags redesign phase
| if validatorKind == validatorsdk.NonValidator { | ||
| // it may be unregistered from P-Chain, but registered on validator manager | ||
| // due to a previous partial removal operation | ||
| if rpcURL == "" { |
There was a problem hiding this comment.
This is great that we can remove these checks!
felipemadero
left a comment
There was a problem hiding this comment.
Making general observations and questions on this design. For @sukantoraymond @learyce to take into account. But having not strong opinion on either way, the subject is complicated and it is needed some tradeoff between being practical and having a design that takes into account all cases. Will happily agree with team opinion that surely will
keep the balance.
| remainingBalanceOwnerAddr string, | ||
| disableOwnerAddr string, | ||
| sc models.Sidecar, | ||
| rpcURL string, |
There was a problem hiding this comment.
What's the point of passing this and other global variables (flags in this case) to this function as arguments as opposed to just using them directly?
There was a problem hiding this comment.
the goal is to be able to call this command from another command, and pass to it only the needed values. this concept is best represented in the tokentransferrer deploy cmd, here the implementation is partial.
secondary goal is to avoid using global variables at all: refactoring of common code, and different dev styles, showed that the risk to use/set global variables among several functions is possible and happens, so, to avoid secondary effects, and difficultty to understand the code, it is best to just not use them.
| var err error | ||
| // TODO: modify check below to extend prompting for rpc to commands other than addValidator | ||
| if *rpc == "" { | ||
| if cmd.Name() == "addValidator" && len(args) == 0 { |
There was a problem hiding this comment.
Why do we need to check length of args here? Also, I think if we expect multiple cases here we should use switch on cmd.Name() and in that case addValidator should be a constant
| } | ||
| return nil | ||
| } | ||
| return prompts.ValidateURLFormat(*rpc) |
There was a problem hiding this comment.
Also, I noticed CaptureURL is doing validation (which is not explicitly mentioned, even though I think it should be) by calling ValidateURL which calls ValidateURLFormat and also calls the url expecting 2xx. I was wondering why don't we use ValidateURL here instead of ValidateUrlFormat?
| addValidatorFlags BlockchainAddValidatorFlags | ||
| ) | ||
|
|
||
| type BlockchainAddValidatorFlags struct { |
There was a problem hiding this comment.
What's the point of grouping these flags here?
There was a problem hiding this comment.
- to isolate variables among commands with the same flag, because cobra initializes all commands in sequence and
the variable retains only the value of the command that was last initialized (a conflict for command that have different initialization values) - to be able in a subsequent PR, to make clean calls into the command logic from another command, by passing the flags object to it (see eg cmd/interchaincmd/tokentransaferrercmd/deploy.go for CallDeploy)
the idea is for this struct to contain all command flags in the future
| } | ||
| } | ||
|
|
||
| if sc.Sovereign && removeValidatorFlags.RPC == "" { |
There was a problem hiding this comment.
Why did we add sc.Sovereign check?
| if rpcURL == "" { | ||
| rpcURL, err = app.Prompt.CaptureURL("What is the RPC endpoint?", false) | ||
| if localValidateFlags.RPC == "" { | ||
| localValidateFlags.RPC, err = app.Prompt.CaptureURL("What is the RPC endpoint?", false) |
There was a problem hiding this comment.
Since in RPC flag validation we use command specific logic, I think this validation should be done there (like we do for add_validator
felipemadero
left a comment
There was a problem hiding this comment.
Overall idea LGTM. Will approve once all comments for other team members are addressed.
|
I strongly believe that we should not mix in special to commands logic inside While if we decide to use switch for easier read, there will be redundancy if two commands have most of the special validation logic the same but with one small difference. This is a bit exaggerated example, but I think we could run into some similar case. Another drawback of such design is that it hides the details of special validation logic, thus potentially misleading someone who’s gonna read the code, since some validation will be abstracted away in the flag validation "middleware" and not shown in the linear flow inside the command execution function. Finally, it will be easier to make mistakes since validation is done in a separate place from the actual command execution logic, thus some validation or nuances may be missed. I propose two alternative approaches for validating input and reducing redundancy that we can go with:
Example of simple validation function: @sukantoraymond @felipemadero Please let me know what you guys think |
Totally agree with the problem. And this comment was very readable +1! I would propose something along this line (really the idea here is to continue the discussion):
|
Would like clarity something. Cobra requires default values for all cmdline flags. But on CLI code, we use flags in one of this two different ways:
That is reason why I made comments such as "the default flag value is not, marked as acceptable" (because cobra always require a default value). --Note: in 2) an explicit user action is always required only if the value is found to be needed by the cmd flow-- So I believe we can have an input abstraction, of different "types" as given by:
Where the last 3 can be modified by the user when instantiating it (eg remove default value or change it) So the command spec should start by defining a list of input instances for the command. Then use a function to With some rules:
Tell me what do you think on this last piece (ofc validation functions should be on pkg/validations, prompt functions |
|
Reply to the first comment from @felipemadero: The Agreed, all basic flag related things should be defined in that package: for
Not a fan of higher order functions in golang ( |
|
Reply to second comment: Otherwise, if input collection and validation is done separately, I would use the same function but without |
Signed-off-by: sukantoraymond <rsukanto@umich.edu>
I believe there is a base for a new design here, at least for default cases of validation and prompting. But the purpose of this PR is not to discuss this. #2736 Regarding dir location, also to be discussed and addressed somewhere. #2737 I personally like having all together under a main inputs package, but should put it on pkg, because we already use pkg for a lot of internal stuff, and really it is expected for |
felipemadero
left a comment
There was a problem hiding this comment.
Preaproving, but lets move command dependent logic back to the command.
pkg/utils/common.go
Outdated
| logName string, | ||
| logLevelStr string, | ||
| defaultLogLevelStr string, | ||
| aggregatorLogLevel string, |
There was a problem hiding this comment.
I believe we may want to keep this function a bit more generic, not so aggregator oriented.
It may be possible to use it on another packages. Probably it can be used to set up main app
log with a bit of refactor. For the moment lets keep the previos more generic arg names
There was a problem hiding this comment.
abstracted into NewSignatureAggregatorLogger
|
|
||
| func ValidateRPC(app *application.Avalanche, rpc *string, cmd *cobra.Command, args []string) error { | ||
| var err error | ||
| // TODO: modify check below to extend prompting for rpc to commands other than addValidator |
There was a problem hiding this comment.
lets avoid command dependent logic here, and defer how to do automatic prompting to following up work. #2736
There was a problem hiding this comment.
we are moving all input collection to cmd/flags. This will be done in subsequent PRs
This PR is the first PR in Test Coverage Improvement Epic. It finds common flags across commands and consolidates them into a common file to prevent inconsistencies in flag name and values.
Consolidate flags below across all commands into 1 file.
Removes the flags below, as they are no longer needed. They were initially implemented for the earliest implementation of ACP-77.
This PR is tested by manually creating an L1 on Fuji using local machine as bootstrap validator as well as using remote AWS nodes as bootstrap validators.