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

feat: Add CreateValidator access control feature #406

Merged
merged 21 commits into from
Dec 23, 2021
Merged

Conversation

0Tech
Copy link
Collaborator

@0Tech 0Tech commented Dec 17, 2021

Description

The first requirement of the consortium is an access control of CreateValidator message. To implement this feature while not modifying the existing modules, this patch introduces two new modules.

consortium module, the first one, has two proposals. UpdateConsortiumParamsProposal changes the parameter of the module by which you may disable the entire consortium related features. You CANNOT enable the module again after disabling it, because it would bring the public chain back to the consortium chain. Therefore, you must enable it by changing the default genesis state (the default is off). The second one, UpdateValidatorAuthsProposal changes the authorization information of the validators. It is incremental now, but the semantic is subject to change (it could be restful-like). The module has no transactions, but has three queries, one for querying the parameters, and the others for querying validator authorizations.

stakingplus module, which was not included in the first plan, was introduced to mitigate a possible DoS attack vector. Basically, it is a thin wrapper of the original staking module, and it queries validator auth infos from the consortium module. If an operator attempts to create a validator without being authorized first, the transaction will be rejected.

closes: #384


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

@0Tech 0Tech changed the title Add CreateValidator access control feature feat: Add CreateValidator access control feature Dec 17, 2021
@codecov
Copy link

codecov bot commented Dec 17, 2021

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #406   +/-   ##
=======================================
  Coverage        ?   53.87%           
=======================================
  Files           ?      683           
  Lines           ?    69906           
  Branches        ?        0           
=======================================
  Hits            ?    37659           
  Misses          ?    29225           
  Partials        ?     3022           

Comment on lines +14 to +18
// ValidatorAuth defines authorization info of a validator.
message ValidatorAuth {
string operator_address = 1 [(gogoproto.moretags) = "yaml:\"operator_address\""];
bool creation_allowed = 2 [(gogoproto.moretags) = "yaml:\"creation_allowed\""];
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will simplify the structure if the requirement of the consortium chain is fixed and no further details are added.

simapp/app.go Outdated Show resolved Hide resolved
Comment on lines -348 to +367
staking.NewAppModule(appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper),
stakingplus.NewAppModule(appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper, app.ConsortiumKeeper),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replace staking with the wrapper stakingplus.

simapp/app.go Outdated Show resolved Hide resolved
x/consortium/abci.go Outdated Show resolved Hide resolved
x/consortium/types/keys.go Outdated Show resolved Hide resolved
x/consortium/types/genesis.go Outdated Show resolved Hide resolved
x/consortium/types/expected_keepers.go Outdated Show resolved Hide resolved
x/consortium/types/errors.go Outdated Show resolved Hide resolved
x/consortium/keeper/validator.go Outdated Show resolved Hide resolved
@0Tech 0Tech marked this pull request as ready for review December 20, 2021 00:01
@egonspace egonspace added this to the Ebony alpha milestone Dec 20, 2021
@egonspace egonspace added the A: improvement Changes in existing functionality label Dec 20, 2021
Copy link

@egonspace egonspace left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Contributor

@dudong2 dudong2 left a comment

Choose a reason for hiding this comment

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

Great!

FlagAllowedValidatorDelete = "delete"
)

// NewCmdSubmitUpdateConsortiumParamsProposal implements the command to submit a update-consortium-params proposal
Copy link
Member

Choose a reason for hiding this comment

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

to submit a update-consortium-params
->
"an"

And below, I think all a need to be change to an in front of update or don't use an. 😅

deletingValidators := parseCommaSeparated(deletingValidatorsStr)

createAuths := func(addings, deletings []string) ([]*types.ValidatorAuth, error) {
auths := []*types.ValidatorAuth{}
Copy link
Member

Choose a reason for hiding this comment

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

How about changing like following.

var auths []*types.ValidatorAuth

return auths, nil
}

auths, err := createAuths(addingValidators, deletingValidators)
Copy link
Member

Choose a reason for hiding this comment

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

What happen if one of the lists to add and one to delete are the same?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was concerned about the case too, so it checks the duplication in the function and if it's the case, it will emit an error.

Copy link
Member

Choose a reason for hiding this comment

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

And please add a unittest for this case.

return
}

allowedOperators := []string{}
Copy link
Member

Choose a reason for hiding this comment

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

If possible, I think following code is good.

var allowedOperators []string

}

for _, auth := range validatorAuths {
keeper.SetValidatorAuth(ctx, auth)
Copy link
Member

Choose a reason for hiding this comment

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

This function can be occurred error. So need to handle the error.

return nil, status.Error(codes.InvalidArgument, "invalid request")
}

auths := []*types.ValidatorAuth{}
Copy link
Member

Choose a reason for hiding this comment

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

var auths []*types.ValidatorAuth

//
// CONTRACT: the parameter Subspace must have the param key table already initialized
func NewKeeper(
cdc codec.BinaryMarshaler, key sdk.StoreKey,
Copy link
Member

Choose a reason for hiding this comment

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

If possible, I think it's good to have each line for every params.

return ctx.Logger().With("module", "x/"+types.ModuleName)
}

// Clean up the states
Copy link
Member

Choose a reason for hiding this comment

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

to Cleanup

return []abci.ValidatorUpdate{}
}

//____________________________________________________________________________
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary the following codes?

Comment on lines 9 to 11
StakingKeeper interface {
IterateValidators(ctx sdk.Context, fn func(index int64, validator stakingtypes.ValidatorI) (stop bool))
}
Copy link
Member

Choose a reason for hiding this comment

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

Good 👍

I thought It's good to describe more explanation about this interface.

@zemyblue
Copy link
Member

The proposal_handler does not have event log.
I think It's good to add the handling result as event log.

@0Tech
Copy link
Collaborator Author

0Tech commented Dec 21, 2021

The proposal_handler does not have event log. I think It's good to add the handling result as event log.

I added events to the module. Thank you for your suggestion.

@@ -0,0 +1,211 @@
package cli
Copy link
Member

Choose a reason for hiding this comment

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

I didn't find unittest of this cli codes.
How about adding unittest of thease cli commands?

You can reference how to developing the cli unittest in https://github.com/line/lbm-sdk/blob/main/x/bank/client/cli/cli_test.go

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will make a new issue on this.
If you agree, please approve this PR :)

@zemyblue
Copy link
Member

zemyblue commented Dec 21, 2021

If you change some profo files. Please check the client/docs/statik/statik.go file is changed.
When I check it, the proto was changed, but the statik.go file was not changed. There may be no changes to the statik.go file, but I have no idea what was modified in statik.go. So please check it.

You can update statik.go file with update-swagger-docs command.

@0Tech 0Tech merged commit 0e7b3cf into Finschia:main Dec 23, 2021
@0Tech 0Tech deleted the consortium branch January 17, 2022 09:54
zemyblue added a commit that referenced this pull request Jan 25, 2022
* 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)
  ...
@zemyblue zemyblue mentioned this pull request Jan 26, 2022
5 tasks
@0Tech 0Tech added C:x/foundation x/foundation module A: feature New features C:x/stakingplus and removed A: improvement Changes in existing functionality labels Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: feature New features C:x/foundation x/foundation module C:x/stakingplus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validator whitelist feature
4 participants