Delete the params module and the umber stuff. #2176
Conversation
…kily, the line after it was empty, so the target still worked, but it shouldn't be there.
WalkthroughThe pull request involves the removal of the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
.changelog/unreleased/api-breaking/2176-delete-params.md (1)
1-2: LGTM! Consider adding a note about potential impact.The changelog entry accurately summarizes the removal of the params module and its implications. It's concise and informative, which is great for a changelog.
Consider adding a brief note about the potential impact on users or developers who might be affected by this change. For example:
* The params module has been removed [PR 2176](https://github.com/provenance-io/provenance/pull/2176). All params module endpoints have been removed. All modules now manage their params on their own. + Developers using the params module or its endpoints will need to update their code to work with individual module parameters.This addition would help users quickly understand if and how they might need to adapt their code.
x/msgfees/types/params_test.go (1)
19-19: LGTM: Hardcoded value simplifies the test, but consider adding a comment.The change from
pioconfig.GetProvenanceConfig().FeeDenomto"nhash"simplifies the test and aligns with the PR objectives. However, it might be helpful to add a comment explaining why "nhash" is used as the hardcoded value.Consider adding a comment like this:
// "nhash" is used as the hardcoded ConversionFeeDenom for simplicity in tests.golangci.yml (1)
148-150: Approve the new deny rule with a minor suggestion.The addition of this deny rule for the params module is excellent. It aligns perfectly with the PR objective of removing the params module and will help prevent accidental usage of the deprecated module in the codebase.
Consider adding a link to the deprecation announcement or migration guide in the description. This could provide developers with more context and guidance if they encounter this rule. For example:
deny: - pkg: "github.com/cosmos/cosmos-sdk/x/params" desc: "The params module is deprecated. See [link to announcement/guide] for migration details."x/marker/types/params.go (1)
Line range hint
42-47: Reintroduce validation for allParamsfieldsThe
Validatemethod now only validatesUnrestrictedDenomRegexbut no longer validatesEnableGovernanceandMaxSupply. Omitting validation for these fields could allow invalid configurations to pass unnoticed, potentially leading to unintended behavior.To ensure all parameters are correctly validated, consider reintroducing validation checks for
EnableGovernanceandMaxSupply. Here's a suggested modification:func (p Params) Validate() error { + if err := validateEnableGovernance(p.EnableGovernance); err != nil { + return err + } + if err := validateBigIntParam(p.MaxSupply); err != nil { + return err + } exp := p.UnrestrictedDenomRegex if len(exp) > 0 && (exp[0:1] == "^" || exp[len(exp)-1:] == "$") { return fmt.Errorf("invalid parameter, validation regex must not contain anchors ^,$") } _, err := regexp.Compile(fmt.Sprintf(`^%s$`, exp)) return err }Ensure that the validation functions are defined as follows:
func validateEnableGovernance(i interface{}) error { _, ok := i.(bool) if !ok { return fmt.Errorf("invalid parameter type for EnableGovernance: %T", i) } return nil } func validateBigIntParam(i interface{}) error { _, ok := i.(sdkmath.Int) if !ok { return fmt.Errorf("invalid parameter type for MaxSupply: %T", i) } return nil }app/upgrades.go (1)
35-38: Consolidate comment blocks for improved clarityThere's an empty comment line at line 35. Consider removing it to make the comment block more concise and enhance readability.
app/upgrades_test.go (1)
Line range hint
610-745: Refactor the long test function into smaller, reusable functionsThe
TestMetadataMigrationfunction is lengthy and contains multiple nested functions and logic blocks. Breaking it down into smaller, well-named helper functions would improve readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
app/upgrade_files/umber/funding_trading_bridge_smart_contract.wasmis excluded by!**/*.wasmapp/upgrade_files/umber/mainnet_scope_navs.csvis excluded by!**/*.csvapp/upgrade_files/umber/testnet_scope_navs.csvis excluded by!**/*.csv
📒 Files selected for processing (25)
- .changelog/unreleased/api-breaking/2176-delete-params.md (1 hunks)
- .changelog/unreleased/improvements/2176-delete-umber.md (1 hunks)
- .golangci.yml (1 hunks)
- Makefile (1 hunks)
- app/app.go (11 hunks)
- app/embed.go (0 hunks)
- app/scope_navs_updater.go (0 hunks)
- app/scope_navs_updater_test.go (0 hunks)
- app/upgrades.go (3 hunks)
- app/upgrades_test.go (1 hunks)
- client/docs/config.json (1 hunks)
- client/docs/swagger-ui/swagger.yaml (0 hunks)
- internal/provwasm/simulation.go (1 hunks)
- x/attribute/types/genesis_test.go (0 hunks)
- x/attribute/types/params.go (0 hunks)
- x/ibchooks/types/params.go (1 hunks)
- x/marker/types/params.go (1 hunks)
- x/marker/types/params_test.go (0 hunks)
- x/metadata/types/oslocatorparams.go (1 hunks)
- x/metadata/types/oslocatorparams_test.go (0 hunks)
- x/metadata/types/params.go (0 hunks)
- x/msgfees/types/params.go (0 hunks)
- x/msgfees/types/params_test.go (1 hunks)
- x/name/types/params.go (0 hunks)
- x/name/types/params_test.go (0 hunks)
💤 Files with no reviewable changes (12)
- app/embed.go
- app/scope_navs_updater.go
- app/scope_navs_updater_test.go
- client/docs/swagger-ui/swagger.yaml
- x/attribute/types/genesis_test.go
- x/attribute/types/params.go
- x/marker/types/params_test.go
- x/metadata/types/oslocatorparams_test.go
- x/metadata/types/params.go
- x/msgfees/types/params.go
- x/name/types/params.go
- x/name/types/params_test.go
✅ Files skipped from review due to trivial changes (1)
- .changelog/unreleased/improvements/2176-delete-umber.md
🧰 Additional context used
🔇 Additional comments (19)
x/msgfees/types/params_test.go (2)
6-7: LGTM: Import statement changes are consistent with test simplification.The removal of the
requirepackage and retention of theassertpackage align with the simplified structure of the test file after the removal of several test functions.
Line range hint
1-35: Verify test coverage after removal of several test functions.The removal of multiple test functions (TestCreateParamSet, TestValidateMinGasPriceParamI, TestValidateUsdConversionRateParamI, TestValidateConversionFeeDenomParamI, and TestMsgFeeParamKeyTable) significantly reduces the test coverage for this package. While this aligns with the PR objectives of removing the params module, it's crucial to ensure that all critical functionality is still being tested, possibly in other parts of the codebase.
Please run the following script to check the test coverage for the msgfees module:
If the coverage has significantly decreased, consider adding new tests to cover critical functionality that may have been lost with the removed tests.
client/docs/config.json (2)
Line range hint
1-268: LGTM! Improved API documentation structure and clarity.The changes to the API configurations are well-structured and consistent:
- Addition of specific tags for each module (e.g., "Attribute", "Exchange", "IBC") improves organization.
- Renaming of operation IDs from generic "Params" to more specific names (e.g., "AttributeParams", "ExchangeParams") enhances clarity.
These modifications align with the PR objectives of removing the params module and will make the API documentation more intuitive and easier to navigate.
269-270: Appropriate exclusion of removed params module.The addition of
"cosmos/params/.*"to the excluded paths in the third-party Swagger configuration is correct and necessary:
- It aligns with the PR objective of removing the params module.
- It ensures that the API documentation won't include endpoints for the removed params module.
This change helps maintain consistency between the codebase and its documentation.
internal/provwasm/simulation.go (1)
Line range hint
54-62: LGTM. Verify impact ofnilparameter inwasm.NewAppModule.The removal of the
ss exported.Subspaceparameter aligns with the PR objective of removing the params module. However, ensure that passingniltowasm.NewAppModuledoesn't have any negative impacts on the wasm module's configuration or behavior.To verify the impact, you can run the following script:
✅ Verification successful
Approved: Confirmed that passing
niltowasm.NewAppModuleis consistent with existing implementations and does not negatively impact the module's configuration or behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any error handling or default behavior when passing nil to NewAppModule rg -U --multiline --type go 'func NewAppModule.*\n.*\n.*\n.*\n.*\n' x/wasmLength of output: 76
Script:
#!/bin/bash # Search for calls to NewAppModule with nil as an argument to identify error handling or default behaviors rg -U --type go 'NewAppModule\s*\(.*nil.*\)'Length of output: 1336
Makefile (3)
522-522: LGTM: Improved formatting in proto-format targetThe removal of the space before the semicolon in the
clang-formatcommand is a minor but welcome improvement. It enhances the consistency of the Makefile without affecting functionality.
Line range hint
1-522: Request: Show changes in proto-gen targetThe AI summary mentions updates to the
proto-gentarget, specifically creating a backup of thego.modfile before generating Protobuf files and restoring it afterward. This is a good practice that improves the reliability of the build process. However, these changes are not visible in the provided code. Could you please show the actual modifications to this target?If implemented correctly, this change is approved in concept as it helps prevent unintended modifications to
go.modduring Protobuf generation.To locate the
proto-gentarget and its changes, you can run:#!/bin/bash # Find the proto-gen target and show its context rg -A 15 "proto-gen:" Makefile
Line range hint
1-522: Request: Show changes in validate-go-version targetThe AI summary mentions improvements to the
validate-go-versiontarget, specifically clearer error messages if the installed Go version doesn't meet the requirements. However, these changes are not visible in the provided code. Could you please show the actual modifications to this target?To locate the
validate-go-versiontarget and its changes, you can run:x/metadata/types/oslocatorparams.go (1)
12-14: Ensure all references to the renamed functionDefaultOSLocatorParamsare updatedThe function
DefaultParamshas been renamed toDefaultOSLocatorParams. Please verify that all references toDefaultParamshave been updated accordingly.Run the following script to find any remaining references to
DefaultParams:#!/bin/bash # Description: Find all references to 'DefaultParams' in Go files. rg --type go 'DefaultParams\('x/ibchooks/types/params.go (1)
22-26: Ensure all references to removed parameter methods are updatedWith the removal of
ParamKeyTableandParamSetPairs, please verify that all references to these methods in other parts of the codebase have been updated appropriately to prevent build or runtime errors.Run the following script to check for any remaining references:
app/app.go (9)
385-385: Confirm Usage ofparamsTKeyin Transient Store KeysPlease verify that the inclusion of
paramsTKeyin transient store keys is necessary for the application's functionality post-removal of theparamsmodule.
686-686: Adjustments to Governance Router Are AppropriateThe update to the governance router by removing references to the
paramsmodule is correct and aligns with the intended removal.
Line range hint
731-755: Module Initializations Reflect Removal ofparamsModuleThe changes in module initializations correctly omit the
paramsmodule and adjust dependencies accordingly.
933-934: Simulation Manager Overrides Are CorrectThe modifications to the simulation manager for
authandwasmmodules are appropriate and ensure proper initialization without theparamsmodule.
Line range hint
509-612: Middleware and Keeper Configurations Updated AppropriatelyThe adjustments to various middleware and keeper configurations correctly remove dependencies on the
paramsmodule and maintain application functionality.
Line range hint
598-612: IBC Module Integrations Remain ConsistentThe integration of IBC modules continues to function correctly without the
paramsmodule.
755-755: WASM Module Initialization Is CorrectThe initialization of the WASM module appropriately reflects the removal of the
paramsmodule.
361-361: Validate Necessity of IncludingparamsKeyin Store KeysEnsure that including
paramsKeyin the list of store keys is essential and does not reintroduce unintended dependencies on the removedparamsmodule.Run the following script to check for any residual dependencies:
#!/bin/bash # Description: Verify there are no remaining dependencies on the params module. # Expected result: No matches found. # Search for references to the params module imports. rg --type go 'import.*params(keeper|client|types)?'
210-218: Ensure Copied Constants Match Original DefinitionsTo prevent inconsistencies during the transition away from the
paramsmodule, please verify that the constantsparamsName,paramsKey, andparamsTKeyexactly match their original definitions inparamstypes.Run the following script to confirm the constants match:
Description
This PR does the following:
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.
docs/) or specification (x/<module>/spec/).godoccomments..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Documentation
Tests