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(community): add MsgUpdateParams for governance #1745

Merged
merged 10 commits into from Oct 11, 2023

Conversation

drklee3
Copy link
Member

@drklee3 drklee3 commented Oct 5, 2023

Description

  • Add MsgUpdateParams and message handler
  • Add e2e test for proposal submission

Checklist

  • Changelog has been updated as necessary.

@drklee3 drklee3 marked this pull request as ready for review October 5, 2023 21:16
app/app.go Outdated Show resolved Hide resolved
tests/e2e/e2e_community_update_params_test.go Show resolved Hide resolved
x/community/keeper/keeper.go Show resolved Hide resolved
x/community/keeper/msg_server.go Show resolved Hide resolved
x/community/keeper/msg_server.go Show resolved Hide resolved
x/community/types/codec.go Show resolved Hide resolved
proto/kava/community/v1beta1/tx.proto Outdated Show resolved Hide resolved
tests/e2e/e2e_community_update_params_test.go Outdated Show resolved Hide resolved

if s.keeper.GetAuthority().String() != msg.Authority {
return nil, errors.Wrapf(
govtypes.ErrInvalidSigner,
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's worth using an error defined in the community module (or a generic one like sdk.ErrUnauthorized) instead of the gov one?
Removing the govtypes import would avoid coupling to the gov module (making it easier to swap out gov for another implementation). Also the authority doesn't strictly need to be the gov module account but the error message implies it should be.
Downsides are that it would differ from the sdk modules, which use the gov error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I think for now we can be consistent with other modules -- if we switch out gov later then we should also be able to update the error response then, assuming a gov switchout is a significant breaking change

@drklee3 drklee3 merged commit 395b69a into master Oct 11, 2023
10 checks passed
@drklee3 drklee3 deleted the dl-community-params-msg branch October 11, 2023 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants