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: deprecate x/params usage in x/mint #12363

Merged
merged 35 commits into from
Jul 4, 2022

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Jun 27, 2022

Description

Deprecate the usage of the now legacy x/params module within x/mint.

  • Remove usage of x/params functionality and implementation within x/mint. Note, there is zero dependency on x/params now apart from types/params_legacy and exported/, which should be removed after the next major release. These are only needed for migrations.
  • Introduce a new message type with an authority expected to be the x/gov module account that will execute the new MsgUpdateParams message.
  • Updated codec
  • Added migration

closes: #12286


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
  • 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)

@alexanderbez alexanderbez changed the title feature: deprecate x/params usage in x/mint feat: deprecate x/params usage in x/mint Jun 27, 2022
@alexanderbez alexanderbez marked this pull request as ready for review June 30, 2022 10:57
@alexanderbez alexanderbez requested a review from a team as a code owner June 30, 2022 10:57
@julienrbrt julienrbrt mentioned this pull request Jul 3, 2022
19 tasks
}

// RegisterLegacyAminoCodec registers concrete types on the LegacyAmino codec
func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
Copy link
Member

Choose a reason for hiding this comment

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

what's the reasoning for supporting legacy amino?

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

LGTM, seems you are missing a query to see the params, also this would be a good PR to start introducing a upgrading.md file. Any chances you can knock it out quickly

@julienrbrt
Copy link
Member

also this would be a good PR to start introducing a upgrading.md file.

My PR for the upgrading.md is coming tomorrow, sorry about the delay 🙈

@codecov
Copy link

codecov bot commented Jul 4, 2022

Codecov Report

Merging #12363 (828fcf2) into main (61dc023) will decrease coverage by 0.38%.
The diff coverage is 64.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #12363      +/-   ##
==========================================
- Coverage   66.04%   65.65%   -0.39%     
==========================================
  Files         692      683       -9     
  Lines       72603    71549    -1054     
==========================================
- Hits        47949    46977     -972     
+ Misses      21984    21929      -55     
+ Partials     2670     2643      -27     
Impacted Files Coverage Δ
x/mint/types/msgs.go 0.00% <0.00%> (ø)
x/mint/types/params.go 7.75% <0.00%> (+0.67%) ⬆️
x/mint/types/params_legacy.go 0.00% <0.00%> (ø)
x/mint/types/codec.go 63.15% <58.82%> (-36.85%) ⬇️
x/mint/keeper/migrator.go 71.42% <71.42%> (ø)
x/mint/migrations/v2/migrate.go 72.72% <72.72%> (ø)
x/mint/keeper/keeper.go 76.78% <80.76%> (-1.00%) ⬇️
x/mint/module.go 58.33% <88.00%> (+7.68%) ⬆️
x/mint/keeper/msg_server.go 100.00% <100.00%> (ø)
x/upgrade/types/codec.go 100.00% <100.00%> (ø)
... and 17 more

@alexanderbez alexanderbez merged commit 4675cb5 into main Jul 4, 2022
@alexanderbez alexanderbez deleted the bez/x-mint-migrate-params-12286 branch July 4, 2022 14:59
@alexanderbez
Copy link
Contributor Author

LGTM, seems you are missing a query to see the params, also this would be a good PR to start introducing a upgrading.md file. Any chances you can knock it out quickly

No it's not because the API layer remains unchanged -- all that changed was the storage and management mechanism. From the user's PoV nothing changed. This applies to all modules.

@kocubinski kocubinski mentioned this pull request Jul 5, 2022
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Rosetta Issues and PR related to Rosetta C:x/mint C:x/upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove params module from x/mint
5 participants