Validate convert txs #2641
Validate convert txs #2641
Conversation
felipemadero
requested review from
sukantoraymond,
meaghanfitzgerald and
cwend-ava
| if doStrongInputChecks && subnetID != ids.Empty { | ||
| ux.Logger.PrintToUser("Subnet ID to be used is %s", subnetID) | ||
| if acceptValue, err := app.Prompt.CaptureYesNo("Is this value correct?"); err != nil { | ||
| return err | ||
| } else if !acceptValue { | ||
| subnetID = ids.Empty | ||
| } | ||
| } |
There was a problem hiding this comment.
i dont think we need to do this check because users will already check details of subnet id and blockchain id in line 444. A user not using skip check flag will have to verify three times
There was a problem hiding this comment.
A user will have to verify two times, not three times (unless I miss something). And
the second verification includes much more information.
This specific check and input for blockchain id was requested and accorded (i added the subnet id
which is in similar state), so probably the 444 check can be
removed. I will let devrel to vote on this cc @martineckardt after trying out the command.
cc @learyce
There was a problem hiding this comment.
This is a very critical operation. The more confirmation the better
| if doStrongInputChecks && blockchainID != ids.Empty { | ||
| ux.Logger.PrintToUser("Blockchain ID to be used is %s", blockchainID) | ||
| if acceptValue, err := app.Prompt.CaptureYesNo("Is this value correct?"); err != nil { | ||
| return err | ||
| } else if !acceptValue { | ||
| blockchainID = ids.Empty | ||
| } | ||
| } |
There was a problem hiding this comment.
same comment as above
There was a problem hiding this comment.
Same here. Better to double check. If this value is messed up, the L1 cannot be recovered
| import "errors" | ||
|
|
||
| var ( | ||
| ErrNoBlockchainID = errors.New("failed to find the blockchain ID for this subnet, has it been deployed/created on this network?\nyou can use 'avalanche blockchain import' if having partial information on a deployed subnet") |
There was a problem hiding this comment.
no blockchain ID found. Use 'avalanche blockchain import' to provide the required Subnet details
There was a problem hiding this comment.
blckchain ID can also be absent for the blockchain not being deployed. np changing if you also include reference to this option
|
|
||
| var ( | ||
| ErrNoBlockchainID = errors.New("failed to find the blockchain ID for this subnet, has it been deployed/created on this network?\nyou can use 'avalanche blockchain import' if having partial information on a deployed subnet") | ||
| ErrNoSubnetID = errors.New("failed to find the subnet ID for this subnet, has it been deployed/created on this network?\nyou can use 'avalanche blockchain import' if having partial information on a deployed subnet") |
There was a problem hiding this comment.
no Subnet ID found. Use 'avalanche blockchain import' to provide the required Subnet details
| @@ -0,0 +1,11 @@ | |||
| // Copyright (C) 2022, Ava Labs, Inc. All rights reserved. | |||
| // See the file LICENSE for licensing terms. | |||
| package clierrors | |||
There was a problem hiding this comment.
is it better to place this under pkg/constants/error.go?
There was a problem hiding this comment.
im fine either way
There was a problem hiding this comment.
lets keep it here but happy to hear @learyce opinion
| ux.Logger.PrintToUser(" Validators:") | ||
| for _, val := range bootstrapValidators { | ||
| ux.Logger.PrintToUser(" Node ID: %s", val.NodeID) | ||
| ux.Logger.PrintToUser(" Balance: %.5f", float64(val.Balance)/float64(units.Avax)) |
There was a problem hiding this comment.
Also show weight here
| if doStrongInputChecks && subnetID != ids.Empty { | ||
| ux.Logger.PrintToUser("Subnet ID to be used is %s", subnetID) | ||
| if acceptValue, err := app.Prompt.CaptureYesNo("Is this value correct?"); err != nil { | ||
| return err | ||
| } else if !acceptValue { | ||
| subnetID = ids.Empty | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a very critical operation. The more confirmation the better
| if doStrongInputChecks && blockchainID != ids.Empty { | ||
| ux.Logger.PrintToUser("Blockchain ID to be used is %s", blockchainID) | ||
| if acceptValue, err := app.Prompt.CaptureYesNo("Is this value correct?"); err != nil { | ||
| return err | ||
| } else if !acceptValue { | ||
| blockchainID = ids.Empty | ||
| } | ||
| } |
There was a problem hiding this comment.
Same here. Better to double check. If this value is messed up, the L1 cannot be recovered
| return false, fmt.Errorf("unexpected node ID on tx") | ||
| } | ||
| ux.Logger.PrintToUser(" Node ID: %s", nodeID) | ||
| ux.Logger.PrintToUser(" Balance: %.5f", float64(val.Balance)/float64(units.Avax)) |
Why this should be merged
a convert tx.
multisig management commands
How this works
How this was tested
How is this documented