-
Notifications
You must be signed in to change notification settings - Fork 182
Conversation
@ian0371, New one pushed again for all of your comments. Please resolve the comment one by one if they are okay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all resolved. LGTM
cmd/utils/nodecmd/util.go
Outdated
return nil | ||
} | ||
|
||
func decode(ctx *cli.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have added some utility functions
for decode, right?
If I understand right, you can rename this command as decode
and use subcommands pattern.
Reference https://github.com/klaytn/klaytn/blob/dev/cmd/utils/nodecmd/accountcmd.go#L60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smarter way :) Refactored. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just put the actual function name
to the action field
of the subcommand. You don't need to switch each cases.
@hyunsooda Please proceed this PR if you don't have additional concerns |
@markyim-klaytn This command can be used to parse voting data and governance data. Please see the command example in the description. |
Proposed changes
The header struct contains byte arrays which is specific data in Klaytn implementation. This change adds a utility to easily decode them in a human-readable format.
Extra bytes parsing
Governance bytes parsing
./ken util gov 0x9d7b22697374616e62756c2e636f6d6d697474656573697a65223a32317d { "istanbul.committeesize": 21 }
Vote bytes parsing
Extract keys
Types of changes
Please put an x in the boxes related to your change.
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
$ make test
)Related issues
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...