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

fix(x/distribution): add nil check in Params.ValidateBasic #17236

Merged
merged 6 commits into from
Aug 1, 2023
Merged

fix(x/distribution): add nil check in Params.ValidateBasic #17236

merged 6 commits into from
Aug 1, 2023

Conversation

n0b0dyCN
Copy link
Contributor

@n0b0dyCN n0b0dyCN commented Aug 1, 2023

Description

This change fixes a scenario in which Params.Validate() would panic when given a nil CommunityTax.

Fixes #17235


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • run make lint and make test
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@n0b0dyCN n0b0dyCN requested a review from a team as a code owner August 1, 2023 06:42
@github-actions github-actions bot added the C:x/distribution distribution module related label Aug 1, 2023
@n0b0dyCN n0b0dyCN changed the title fix(x/distribution): add nil check in types.ValidateBasic fix(x/distribution): add nil check in Params.ValidateBasic Aug 1, 2023
@julienrbrt
Copy link
Member

Hi! Could you add a test?

@julienrbrt julienrbrt added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Aug 1, 2023
@@ -18,6 +19,10 @@ func DefaultParams() Params {

// ValidateBasic performs basic validation on distribution parameters.
func (p Params) ValidateBasic() error {
if p.CommunityTax.IsNil() {
Copy link
Member

Choose a reason for hiding this comment

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

Why not using just doing return validateCommunityTax(p.CommunityTax) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. I'll add an update

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.

Awesome, thank you! Could you add a changelog entry under unreleased bug fixes?

@n0b0dyCN
Copy link
Contributor Author

n0b0dyCN commented Aug 1, 2023

Using function validateCommunityTax in Params.ValidateBasic and add test case.
I'm new to Golang and not sure if there is a better way to initiate the Params struct with nil field other than using Unmarshal.

@julienrbrt
Copy link
Member

julienrbrt commented Aug 1, 2023

Using function validateCommunityTax in Params.ValidateBasic and add test case. I'm new to Golang and not sure if there is a better way to initiate the Params struct with nil field other than using Unmarshal.

You could add the following test case in TestParams_ValidateBasic instead of your extra test:

{"community tax nil", fields{sdkmath.LegacyDec{}, toDec("0"), toDec("0"), false}, true},

@n0b0dyCN
Copy link
Contributor Author

n0b0dyCN commented Aug 1, 2023

Both test case and change log is updated.

@julienrbrt
Copy link
Member

Could you update the integration tests?

func TestMsgUpdateParams(t *testing.T) {

Add test case in integration test `TestMsgUpdateParams` and update
related error messages.
auto-merge was automatically disabled August 1, 2023 10:01

Head branch was pushed to by a user without write access

@julienrbrt julienrbrt added this pull request to the merge queue Aug 1, 2023
Merged via the queue into cosmos:main with commit b9b325e Aug 1, 2023
43 of 44 checks passed
mergify bot pushed a commit that referenced this pull request Aug 1, 2023
julienrbrt pushed a commit that referenced this pull request Aug 1, 2023
…17236) (#17241)

Co-authored-by: n0b0dy <n0b0dyCN@users.noreply.github.com>
@n0b0dyCN n0b0dyCN deleted the fix/x-distribution branch August 15, 2023 01:38
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:x/distribution distribution module related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: lack of nil check in ValidateBasic function in x/distribution/types/params.go
2 participants