add CLI command verifyValidator
#4933
Conversation
chrlschwb
commented
Oct 15, 2023
•
chrlschwb
commented
- CLI: Add a command for the membership WG to verify validator profile #4930
There was a problem hiding this comment.
TBH I don't know anything about the CLI. @zeeshanakram3 could you check this PR please ?
thesan
added
CLI
| static args = [{ | ||
| name: "memberId", | ||
| required: true, | ||
| description: 'Membership ID of the validator to verify', | ||
| }, | ||
| { | ||
|
|
||
| name: "isVerified", | ||
| required: true, | ||
| description: 'Verification state of the validator', | ||
| } | ||
| ] |
There was a problem hiding this comment.
Please use flags instead of args. Using flags you have to explicitly write each flag name before providing the value, so less chance of errors, as compared to args which are just order dependant positional arguments
| if (!worker) { | ||
| this.error('Only membership WG lead/worker can perform this command') | ||
| } else { | ||
| this.getOriginalApi().tx.membershipWorkingGroup.workerRemark(Number(worker), message) |
There was a problem hiding this comment.
This line just only prepares the encodedtx. It actually does not sends/broadcasts it.
There was a problem hiding this comment.
This comment is still not resolved, please look at any example CLI implementation (e.g. memberRemark) to see how transactions are being constructed & dispatched
| if (!worker) { | ||
| this.error('Only membership WG lead/worker can perform this command') | ||
| } else { | ||
| this.getOriginalApi().tx.membershipWorkingGroup.workerRemark(Number(worker), message) |
There was a problem hiding this comment.
worker is a type of GroupMember, while you are converting it to Number. Which is obviously incorrect.
There was a problem hiding this comment.
- Please see this comment add CLI command
verifyValidator#4933 (comment) - I don't think this PR should alter
yarn.lockfile? Can you explain why changes are being made toyarn.lockfile?
| const nowGroup = this.group | ||
|
|
||
| await this.setPreservedState({ defaultWorkingGroup: WorkingGroups.Membership }) |
There was a problem hiding this comment.
Hey @zeeshanakram3 it looks to me like the code from this PR should work. I'm just wondering whether these 2 lines and the line 59 are necessary.
Could you have another look please ? This is blocking the validator dashboard QA
There was a problem hiding this comment.
Right, we can actually specify the working group using --group flag, so this is not necessary at all. Please remove these
| description: 'Membership ID of the validator to verify', | ||
| }), | ||
| isVerified: flags.boolean({ | ||
| required: true, |
There was a problem hiding this comment.
| required: true, | |
| default: false, |
We can't specify boolean flags as isVerified=true or isVerified=false, so the only way to signal that isVerified is true/false is by presence/absence of the flag itself, i.e.
./bin/run working-groups:verifyValidator --memberId=2 --isVerified # isVerified is 'true'
./bin/run working-groups:verifyValidator --memberId=2 # isVerified is 'false'
Since we would define the default value of isVerified false so if we omit the flag then it was be assumed false in the command.
|
I'm closing this one in favour of #5057 |
