fix: validate validator addresses in update-validator-auths proposal #411

0Tech · 2021-12-28T01:22:18Z

Description

I intended to add tests to the module, but I have found & fixed a bug along the way.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2021-12-28T01:51:10Z

Codecov Report

❗ No coverage uploaded for pull request base (main@0e7b3cf). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #411   +/-   ##
=======================================
  Coverage        ?   54.14%           
=======================================
  Files           ?      687           
  Lines           ?    70331           
  Branches        ?        0           
=======================================
  Hits            ?    38081           
  Misses          ?    29225           
  Partials        ?     3025

0Tech

Reviewed my changes and added some explanations

0Tech · 2022-01-04T04:53:58Z

docs/core/proto-docs.md

-| `ValidatorAuths` | [QueryValidatorAuthsRequest](#lbm.consortium.v1.QueryValidatorAuthsRequest) | [QueryValidatorAuthsResponse](#lbm.consortium.v1.QueryValidatorAuthsResponse) | ValidatorAuths queries authorization infos of validators. | GET|/lbm/consortium/v1/validators|
 | `ValidatorAuth` | [QueryValidatorAuthRequest](#lbm.consortium.v1.QueryValidatorAuthRequest) | [QueryValidatorAuthResponse](#lbm.consortium.v1.QueryValidatorAuthResponse) | ValidatorAuth queries authorization info of a validator. | GET|/lbm/consortium/v1/validators/{validator_address}|
+| `ValidatorAuths` | [QueryValidatorAuthsRequest](#lbm.consortium.v1.QueryValidatorAuthsRequest) | [QueryValidatorAuthsResponse](#lbm.consortium.v1.QueryValidatorAuthsResponse) | ValidatorAuths queries authorization infos of validators. | GET|/lbm/consortium/v1/validators|


reposition the two functions

0Tech · 2022-01-04T04:55:17Z

proto/lbm/consortium/v1/query.proto

@@ -14,15 +14,15 @@ service Query {
    option (google.api.http).get = "/lbm/consortium/v1/params";
  }

-  // ValidatorAuths queries authorization infos of validators.


reposition the two functions

0Tech · 2022-01-04T04:55:48Z

testutil/rest/rest.go

@@ -0,0 +1,44 @@
+// Package rest provides HTTP types and primitives for REST


import rest.go from cosmos-sdk

0Tech · 2022-01-04T04:56:59Z

x/consortium/client/cli/query.go

-		GetParamsCmd(),
-		GetValidatorAuthsCmd(),
-		GetValidatorAuthCmd(),
+		NewQueryCmdParams(),
+		NewQueryCmdValidatorAuth(),
+		NewQueryCmdValidatorAuths(),


rename the functions

Why did you change the name with prefix New?
I understand the New prefix of function creates some instance. But I think this functions doesn't seems to do that.

I think it's a matter of choice. You can use both of New and Get, then they would mean creating a new command instance and get a command instance, respectively.

An example of New:
https://github.com/cosmos/cosmos-sdk/blob/a7c09a3278613a2c383968e73ee36150245cbdc1/x/nft/client/cli/tx.go#L27-L29

An example of Get:
https://github.com/cosmos/cosmos-sdk/blob/a7c09a3278613a2c383968e73ee36150245cbdc1/x/nft/client/cli/query.go#L31-L39

It is interesting that they are both in the same module.

I understood. Thank you.

0Tech · 2022-01-04T04:57:26Z

x/consortium/client/cli/query.go

@@ -56,27 +56,27 @@ func GetParamsCmd() *cobra.Command {
 	return cmd
 }

-// GetValidatorAuthsCmd returns validator authorization by consortium


reposition the two functions

0Tech · 2022-01-04T05:06:43Z

x/consortium/client/testutil/tx.go

+		respType     proto.Message
+	}{
+		{
+			"with wrong # of args",


it does not accept any argument as of now

0Tech · 2022-01-04T05:07:32Z

x/consortium/client/testutil/tx.go

+		respType     proto.Message
+	}{
+		{
+			"with no args",


an empty proposal is forbidden

0Tech · 2022-01-04T05:08:33Z

x/consortium/client/testutil/query.go

@@ -0,0 +1,173 @@
+package testutil


add query tests

0Tech · 2022-01-04T05:08:48Z

x/consortium/client/testutil/grpc.go

@@ -0,0 +1,135 @@
+package testutil


add gRPC query tests

0Tech · 2022-01-04T05:11:30Z

x/consortium/client/testutil/suite.go

+	var consortiumData types.GenesisState
+	s.Require().NoError(s.cfg.Codec.UnmarshalJSON(genesisState[types.ModuleName], &consortiumData))
+
+	// enable consortium
+	params := &types.Params{
+		Enabled: true,
+	}
+	consortiumData.Params = params
+
+	consortiumDataBz, err := s.cfg.Codec.MarshalJSON(&consortiumData)
+	s.Require().NoError(err)
+	genesisState[types.ModuleName] = consortiumDataBz
+	s.cfg.GenesisState = genesisState


modify the genesis state to enable the consortium module in the tests

iproudhon

LGTM

zemyblue

LGTM

* main: (44 commits) feat: rewrite issue template and move PR template (#410) fix: validate validator addresses in update-validator-auths proposal (#411) feat: add `bankplus` function to restrict to send coin to inactive smart contract. (#400) build(deps): bump actions/setup-go from 2.1.4 to 2.1.5 (#408) ci: fix branch name on ci script (#409) feat: Add CreateValidator access control feature (#406) build(deps): bump github.com/spf13/viper from 1.9.0 to 1.10.1 (#403) build(deps): bump github.com/rs/zerolog from 1.26.0 to 1.26.1 (#402) fix: fix query signing infos command (#407) build(deps): bump github.com/spf13/cobra from 1.1.3 to 1.3.0 (#399) build(deps): github.com/ulikunitz/xz from 0.5.5 to 0.5.10 (#398) build(deps): bump actions/stale from 3 to 4.1.0 (#396) build(deps): bump actions/cache from 2.1.3 to 2.1.7 (#386) feat: Add the `instantiate_permission` in the `CodeInfoResponse` (#395) fix: fix bug where `StoreCodeAndInstantiateContract`, `UpdateContractStatus`, `UpdateContractStatusProposal` API does not work (#393) docs: modify with latest version of swagger REST interface docs. (#392) fix: fix invalid root hash by bumping up tm-db (#388) chore: fix swagger's config path for wasm (#391) build(deps): bump technote-space/get-diff-action from 5.0.1 to 5.0.2 (#379) fix: update allowance inside AllowedMsgAllowance (#383) ...

0Tech added 10 commits December 27, 2021 22:03

refactor: edit cli function names and use same conversion rules

3ff04ac

test: add consortium query tests

9d55654

fix: validate validator addresses in update-validator-auths proposal

cafa3e1

test: add consortium tx tests

b15f23f

chore: lint

c7d0e87

refactor: switch function locations

2b49756

test: add consortium grpc query tests

ed23de1

feat: remove legacy REST

f499052

refactor: switch locations of grpc queries

5a24745

chore: lint

2ef6d70

0Tech self-assigned this Dec 28, 2021

0Tech changed the title ~~fix: refactor & add tests to the consortium module~~ fix: add tests to the consortium module Dec 28, 2021

0Tech changed the title ~~fix: add tests to the consortium module~~ fix: validate validator addresses in update-validator-auths proposal Dec 28, 2021

0Tech added 2 commits December 28, 2021 01:27

docs: update CHANGELOG

b7c2deb

test: use ioutil

d5aabc1

0Tech added 3 commits January 3, 2022 06:17

fix: add address validation into ValidateBasic()

3dcb48f

refactor: make cli rely on the content's validation logic

e3b7433

refactor: use same conversion rules

5b53414

0Tech added the A: bug Something isn't working label Jan 3, 2022

0Tech requested review from dudong2 and iproudhon January 3, 2022 09:55

0Tech marked this pull request as ready for review January 3, 2022 09:55

0Tech commented Jan 4, 2022

View reviewed changes

iproudhon approved these changes Jan 7, 2022

View reviewed changes

zemyblue approved these changes Jan 17, 2022

View reviewed changes

0Tech merged commit 212427a into Finschia:main Jan 17, 2022

0Tech deleted the consortium-test branch January 17, 2022 09:54

zemyblue mentioned this pull request Jan 26, 2022

docs: release 0.44.0-rc0 #420

Merged

5 tasks

0Tech added the C:x/foundation x/foundation module label Dec 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: validate validator addresses in update-validator-auths proposal #411

fix: validate validator addresses in update-validator-auths proposal #411

0Tech commented Dec 28, 2021 •

edited

Loading

codecov bot commented Dec 28, 2021 •

edited

Loading

0Tech left a comment

0Tech Jan 4, 2022

0Tech Jan 4, 2022

0Tech Jan 4, 2022

0Tech Jan 4, 2022

zemyblue Jan 4, 2022

0Tech Jan 6, 2022

zemyblue Jan 17, 2022

0Tech Jan 4, 2022

0Tech Jan 4, 2022

0Tech Jan 4, 2022

0Tech Jan 4, 2022

0Tech Jan 4, 2022

0Tech Jan 4, 2022

iproudhon left a comment

zemyblue left a comment

		@@ -0,0 +1,44 @@
		// Package rest provides HTTP types and primitives for REST

fix: validate validator addresses in update-validator-auths proposal #411

fix: validate validator addresses in update-validator-auths proposal #411

Conversation

0Tech commented Dec 28, 2021 • edited Loading

Description

codecov bot commented Dec 28, 2021 • edited Loading

Codecov Report

0Tech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iproudhon left a comment

Choose a reason for hiding this comment

zemyblue left a comment

Choose a reason for hiding this comment

0Tech commented Dec 28, 2021 •

edited

Loading

codecov bot commented Dec 28, 2021 •

edited

Loading