Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(client): use cmtservice for comet-validator-set command #17187

Merged
merged 4 commits into from
Aug 2, 2023

Conversation

zakir-code
Copy link
Contributor

  • replacement for deprecated PrintObjectLegacy method

* replacement for deprecated `PrintObjectLegacy` method
@zakir-code
Copy link
Contributor Author

Impact on "comet-validator-set" cmd cli

current:

{"block_height":"4541","validators":[{"address":"cosmosvalcons1uh0dwj3u8nx9yh7dcj8lzlmpcunap7vrz94y43","pub_key":{"type":"tendermint/PubKeyEd25519","value":"Nnj0I8yxNgpe2swLEqDPqfNnJCDRenFXROCPp2ay87w="},"proposer_priority":"0","voting_power":"1"}],"total":"1"}

This PR:

{"block_height":"4004","validators":[{"address":"E5DED74A3C3CCC525FCDC48FF17F61C727D0F983","pub_key":{"type":"tendermint/PubKeyEd25519","value":"Nnj0I8yxNgpe2swLEqDPqfNnJCDRenFXROCPp2ay87w="},"voting_power":"1","proposer_priority":"0"}],"count":"1","total":"1"}

@julienrbrt
Copy link
Member

julienrbrt commented Jul 29, 2023

Based on the output of above, I think we probably still want to display the bech32 address.

@zakir-code
Copy link
Contributor Author

Can use cmtservice.GetLatestValidatorSetResponse as output of comet-validator-set cmd.

{"block_height":"5307","validators":[{"address":"cosmosvalcons1uh0dwj3u8nx9yh7dcj8lzlmpcunap7vrz94y43","pub_key":{"@type":"/cosmos.crypto.ed25519.PubKey","key":"Nnj0I8yxNgpe2swLEqDPqfNnJCDRenFXROCPp2ay87w="},"voting_power":"1","proposer_priority":"0"}],"pagination":{"next_key":null,"total":"1"}}

@zakir-code
Copy link
Contributor Author

Also I think status and comet-validator-set commands can be moved to server.cmt_cmds.go

@julienrbrt
Copy link
Member

Can use cmtservice.GetLatestValidatorSetResponse as output of comet-validator-set cmd.

{"block_height":"5307","validators":[{"address":"cosmosvalcons1uh0dwj3u8nx9yh7dcj8lzlmpcunap7vrz94y43","pub_key":{"@type":"/cosmos.crypto.ed25519.PubKey","key":"Nnj0I8yxNgpe2swLEqDPqfNnJCDRenFXROCPp2ay87w="},"voting_power":"1","proposer_priority":"0"}],"pagination":{"next_key":null,"total":"1"}}

Feels a bit hacky, but works! Eventually, I'll investigate how to use AutoCLI for these anyway.
Let's not move them just yet however as they will probably get replaced soon.

@zakir-code
Copy link
Contributor Author

Using AutoCLI is a good idea

@julienrbrt julienrbrt self-assigned this Jul 30, 2023
@julienrbrt
Copy link
Member

Could you fix the e2e tests and implement this comment: #17187 (comment)

@zakir-code
Copy link
Contributor Author

I have fixed the e2e tests. Please review the changes in the latest commit. Thank you for pointing out the issue in #17187 (comment).

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The address still does not have the same format as previously.

@zakir-code
Copy link
Contributor Author

I test again, The address is the same, the pub_key and total are different, I think it is necessary to keep the pub_key consistent with the type returned by other commands.

current:

{"block_height":"33","validators":[{"address":"cosmosvalcons1kpfyd2mxvse4wnq652cam3a73pwxk4a2uqaaws","pub_key":{"type":"tendermint/PubKeyEd25519","value":"YAmXz9KUXRRpNM1lDQEzt3gxHf9iFSVzRuiKFU4ExEA="},"proposer_priority":"0","voting_power":"1"}],"total":"1"}

This PR:

{"block_height":"6","validators":[{"address":"cosmosvalcons1kpfyd2mxvse4wnq652cam3a73pwxk4a2uqaaws","pub_key":{"@type":"/cosmos.crypto.ed25519.PubKey","key":"YAmXz9KUXRRpNM1lDQEzt3gxHf9iFSVzRuiKFU4ExEA="},"voting_power":"1","proposer_priority":"0"}],"pagination":{"next_key":null,"total":"1"}}

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK!

@julienrbrt julienrbrt changed the title refactor: get validators from CometRPC refactor(client): use cmtservice for comet-validator-set command Aug 2, 2023
@julienrbrt julienrbrt added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Aug 2, 2023
@julienrbrt julienrbrt added this pull request to the merge queue Aug 2, 2023
Merged via the queue into cosmos:main with commit 8503c18 Aug 2, 2023
46 of 47 checks passed
mergify bot pushed a commit that referenced this pull request Aug 2, 2023
…17187)

Co-authored-by: Julien Robert <julien@rbrt.fr>
(cherry picked from commit 8503c18)
julienrbrt added a commit that referenced this pull request Aug 2, 2023
…ackport #17187) (#17257)

Co-authored-by: zakir <80246097+zakir-code@users.noreply.github.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
@faddat faddat mentioned this pull request Nov 8, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release C:CLI C:x/gov
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants