chore: cleanup unused globals #8804
Conversation
github-actions
bot
added
C:app-wiring
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (5)
tests/simulator/osmosis_helper.go (1)
27-27: Consider adding a comment to document the nil parameter.Since this is a test helper used for simulations, it would be helpful to document that this parameter is for Wasm options.
- nil, + nil, // Wasm options (disabled for simulations)app/config.go (1)
66-66: LGTM! Consider improving function call readability.The change from
EmptyWasmOptstonilaligns with the PR objectives of cleaning up unused Wasm globals.Consider improving the readability of the
NewOsmosisAppfunction call by using named parameters or a builder pattern, as the current call has many parameters which makes it harder to understand what each value represents.Example with a builder pattern:
func NewOsmosisAppBuilder() *OsmosisAppBuilder { return &OsmosisAppBuilder{} } type OsmosisAppBuilder struct { logger log.Logger db dbm.DB traceStore io.Writer loadLatest bool skipUpgradeHeights map[int64]bool homePath string invCheckPeriod uint encodingConfig sims.AppOptions wasmOpts []wasm.Option baseAppOptions []func(*baseapp.BaseApp) } // Usage: app := NewOsmosisAppBuilder(). WithLogger(valCtx.Logger). WithDB(dbm.NewMemDB()). WithWasmOpts(nil). // ... other setters Build()CHANGELOG.md (3)
Line range hint
1-24: Ensure changelog follows Keep a Changelog format.The changelog follows the Keep a Changelog format and semantic versioning, which is good practice. However, consider adding:
- Release dates for each version
- More detailed descriptions of breaking changes
# Changelog All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] +### Added +### Changed +### Deprecated +### Removed +### Fixed +### Security + ## v19.2.0 - YYYY-MM-DD
56-59: Add more context to the changelog entry.The changelog entry for v19.2.0 lacks sufficient detail about the band-aid fix. Consider adding:
- What was the issue with cwpool gauges?
- What are the implications of the fix?
- Any follow-up work needed?
### Misc Improvements -* [#6476](https://github.com/osmosis-labs/osmosis/pull/6476) band-aid state export fix for cwpool gauges +* [#6476](https://github.com/osmosis-labs/osmosis/pull/6476) band-aid state export fix for cwpool gauges: + * Fixed issue: <description> + * Impact: <what this means for users> + * Future work: <any planned permanent fixes>
48-54: Consider grouping API breaks by module.The API breaks section would be more readable if organized by module. This helps developers understand the impact on specific parts of the system.
### API Breaks +#### Poolmanager Module * [#6487](https://github.com/osmosis-labs/osmosis/pull/6487) make PoolModuleI CalculateSpotPrice API return BigDec +#### Incentives Module * [#6511](https://github.com/osmosis-labs/osmosis/pull/6511) remove redundant param from CreateGaugeRefKeys in incentives +#### Osmoutils * [#6510](https://github.com/osmosis-labs/osmosis/pull/6510) remove redundant ctx param from DeleteAllKeysFromPrefix in osmoutils
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)app/app.go(0 hunks)app/config.go(1 hunks)app/modules_test.go(1 hunks)app/test_helpers.go(2 hunks)cmd/osmosisd/cmd/root.go(1 hunks)tests/simulator/osmosis_helper.go(1 hunks)
💤 Files with no reviewable changes (1)
- app/app.go
🔇 Additional comments (5)
app/modules_test.go (2)
Line range hint 18-22: LGTM! Test reliability is maintained.
The core test logic remains unchanged and the replacement of EmptyWasmOpts with nil doesn't affect the determinism check, as it's only part of the app initialization and not the actual test assertions.
16-16: Verify the consistency of nil usage across test files.
The change from EmptyWasmOpts to nil aligns with the PR objective of cleaning up unused Wasm globals. Let's verify this change is consistent across all test files.
✅ Verification successful
Based on the search results, I can now generate the final response:
The change from EmptyWasmOpts to nil is consistent with the codebase cleanup
The search results show that:
- The change is part of a broader cleanup of unused Wasm globals (confirmed by CHANGELOG.md)
- The only remaining reference to
EmptyWasmOptsis incmd/osmosisd/cmd/root.go, which is not a test file - The test file in question (
app/modules_test.go) is the only test file usingNewOsmosisAppconstructor, and it's usingnilconsistently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that EmptyWasmOpts has been consistently replaced with nil
# across all test files and there are no remaining references.
# Test 1: Search for any remaining references to EmptyWasmOpts
echo "Checking for remaining EmptyWasmOpts references:"
rg "EmptyWasmOpts"
# Test 2: Look for similar NewOsmosisApp constructor calls in test files
echo "Checking NewOsmosisApp constructor calls in test files:"
rg -A 2 "NewOsmosisApp.*nil.*baseapp" "**/*_test.go"
Length of output: 698
Script:
#!/bin/bash
# Let's try a more specific search for NewOsmosisApp constructor calls in test files
# Search for test files first
echo "Finding test files:"
fd "_test\.go$"
# Then search for NewOsmosisApp constructor calls in test files
echo -e "\nChecking NewOsmosisApp constructor calls:"
fd "_test\.go$" --exec rg -l "NewOsmosisApp"
# Get the actual context of these calls
echo -e "\nDetailed constructor calls:"
fd "_test\.go$" --exec rg -B 1 -A 3 "NewOsmosisApp"
Length of output: 14007
tests/simulator/osmosis_helper.go (1)
27-27: LGTM! Let's verify the parameter position.
The change from EmptyWasmOpts to nil aligns with the PR objective of cleaning up unused globals.
Let's verify this is the correct parameter position for Wasm options:
✅ Verification successful
Parameter position for Wasm options is correct, change is safe
The verification confirms that wasmOpts is indeed the 9th parameter in the NewOsmosisApp constructor, and the change from EmptyWasmOpts to nil in tests/simulator/osmosis_helper.go is consistent with other usages across the codebase where nil is commonly used for this parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the parameter position in NewOsmosisApp constructor
# Expected: The 9th parameter should be for Wasm options
# Search for NewOsmosisApp function definition
ast-grep --pattern 'func NewOsmosisApp($$$) $$ {'
Length of output: 51
Script:
#!/bin/bash
# Let's try a different approach to find the NewOsmosisApp constructor
# First, let's use ripgrep to find the function definition with context
rg "func NewOsmosisApp" -A 10
# Also search for any usage of NewOsmosisApp to understand the parameter order
rg "NewOsmosisApp\(" -A 3
Length of output: 2899
app/test_helpers.go (2)
126-126: LGTM! Replacing EmptyWasmOpts with nil.
The change aligns with the PR objective to cleanup unused Wasm globals.
170-170: LGTM! Consistent replacement of EmptyWasmOpts with nil.
The change maintains consistency with other test setup functions and aligns with the PR objective.
| @@ -358,7 +358,7 @@ | |||
| WithViper("OSMOSIS") | |||
|
|
|||
| tempDir := tempDir() | |||
| tempApp := osmosis.NewOsmosisApp(log.NewNopLogger(), cosmosdb.NewMemDB(), nil, true, map[int64]bool{}, tempDir, 5, sims.EmptyAppOptions{}, osmosis.EmptyWasmOpts, baseapp.SetChainID("osmosis-1")) | |||
| tempApp := osmosis.NewOsmosisApp(log.NewNopLogger(), cosmosdb.NewMemDB(), nil, true, map[int64]bool{}, tempDir, 5, sims.EmptyAppOptions{}, nil, baseapp.SetChainID("osmosis-1")) | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage found: EmptyWasmOpts still present in root.go
The verification revealed that EmptyWasmOpts is still being used in cmd/osmosisd/cmd/root.go while other instances of NewOsmosisApp consistently use nil. This inconsistency needs to be addressed.
cmd/osmosisd/cmd/root.go: Still usesEmptyWasmOptsin the app initialization- Other files (
app/test_helpers.go,app/modules_test.go,app/config.go): Correctly usenil
🔗 Analysis chain
LGTM! Verify consistent usage of nil for Wasm options.
The change from EmptyWasmOpts to nil for the temporary app initialization is safe and aligns with the PR's objective of cleaning up unused Wasm globals.
Let's verify that this change is consistently applied across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of EmptyWasmOpts
rg "EmptyWasmOpts"
# Search for NewOsmosisApp calls to verify nil usage pattern
ast-grep --pattern 'NewOsmosisApp($$$, $_, $$$)'
Length of output: 1297
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
app/modules_test.go (1)
16-16: Consider adding a comment explaining the nil parameter.Since
NewOsmosisAppaccepts multiple parameters, it would be helpful to add a comment explaining what thenilparameter represents (Wasm options).- app := NewOsmosisApp(log.NewNopLogger(), db, nil, true, map[int64]bool{}, DefaultNodeHome, 5, sims.EmptyAppOptions{}, nil, baseapp.SetChainID("osmosis-1")) + app := NewOsmosisApp( + log.NewNopLogger(), + db, + nil, // encoding config + true, + map[int64]bool{}, + DefaultNodeHome, + 5, + sims.EmptyAppOptions{}, + nil, // wasm options + baseapp.SetChainID("osmosis-1"), + )CHANGELOG.md (2)
56-59: Clean up formatting in Unreleased section.Consider removing extra blank lines between sections and maintaining consistent spacing around headers to improve readability.
### API * [#8804](https://github.com/osmosis-labs/osmosis/pull/8804) chore: cleanup `app` package unused globals: `WasmProposalsEnabled`, `EnableSpecificWasmProposals`, `EmptyWasmOpts`. - - ### Config
Line range hint
1-47: Consider using standard markdown comments instead of HTML comments.The guiding principles section uses HTML comments which is not typical for markdown files. Consider using standard markdown syntax for better compatibility with markdown parsers and editors.
- <!-- + # Guiding Principles: - -->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)app/app.go(0 hunks)app/config.go(1 hunks)app/modules_test.go(1 hunks)app/test_helpers.go(2 hunks)cmd/osmosisd/cmd/root.go(1 hunks)tests/simulator/osmosis_helper.go(1 hunks)
💤 Files with no reviewable changes (1)
- app/app.go
🔇 Additional comments (3)
app/test_helpers.go (2)
170-170: LGTM! Consistent with other test setup changes.
The modification maintains consistency with other test setup functions, replacing EmptyWasmOpts with nil.
126-126: LGTM! Verify test behavior with nil WasmOpts.
The change aligns with the PR objective to cleanup unused globals. However, let's verify that using nil instead of EmptyWasmOpts doesn't affect test behavior.
✅ Verification successful
Let me gather more context about the EmptyWasmOpts usage and its removal pattern.
Let me check the PR diff to understand the complete context of these changes.
Based on the gathered information, I can now provide the final response:
Replacing EmptyWasmOpts with nil is safe and correct
The change is verified to be safe because:
NewOsmosisAppconstructor acceptswasmOpts []wasmkeeper.Optionwhich is a slice type, makingnila valid value- The change is consistent with the test setup context where no specific wasm options are needed
- Only one other reference exists in
cmd/osmosisd/cmd/root.gowhich is not affected by this test helper change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any test files that might be affected by this change
# and verify there are no remaining references to EmptyWasmOpts
# Search for test files that might use this setup function
rg -t go "SetupWithCustomHomeAndChainId|NewOsmosisApp.*EmptyWasmOpts" --type go
# Look for any remaining references to EmptyWasmOpts
rg "EmptyWasmOpts" --type go
Length of output: 702
Script:
#!/bin/bash
# Check the definition and usage of EmptyWasmOpts
ast-grep --pattern 'var EmptyWasmOpts'
# Check the NewOsmosisApp function signature and usage
ast-grep --pattern 'func NewOsmosisApp'
# Look at the content of the test helper file
cat app/test_helpers.go
Length of output: 6453
Script:
#!/bin/bash
# Get the PR diff to see the complete changes
git diff
# Check if there are any tests specifically using EmptyWasmOpts
rg -B 5 -A 5 "EmptyWasmOpts" app/app_test.go
# Look at the app.go to understand the NewOsmosisApp constructor
cat app/app.go | grep -A 10 "func NewOsmosisApp"
Length of output: 438
CHANGELOG.md (1)
Line range hint 1-6000: LGTM for content and structure.
The changelog follows the Keep a Changelog format well and provides comprehensive documentation of changes across versions. The content is well organized and includes all necessary information like PR references, version numbers, and descriptions.
| @@ -13,7 +13,7 @@ import ( | |||
|
|
|||
| func TestOrderEndBlockers_Determinism(t *testing.T) { | |||
| db := dbm.NewMemDB() | |||
| app := NewOsmosisApp(log.NewNopLogger(), db, nil, true, map[int64]bool{}, DefaultNodeHome, 5, sims.EmptyAppOptions{}, EmptyWasmOpts, baseapp.SetChainID("osmosis-1")) | |||
| app := NewOsmosisApp(log.NewNopLogger(), db, nil, true, map[int64]bool{}, DefaultNodeHome, 5, sims.EmptyAppOptions{}, nil, baseapp.SetChainID("osmosis-1")) | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage found in cmd/osmosisd/cmd/root.go
The change from EmptyWasmOpts to nil is not consistently applied across the codebase. While most NewOsmosisApp calls use nil, there's still one instance in cmd/osmosisd/cmd/root.go that uses EmptyWasmOpts.
cmd/osmosisd/cmd/root.go: Still usingEmptyWasmOptsinstead ofnil
app := osmosis.NewOsmosisApp(logger, db, traceStore, loadLatest, map[int64]bool{}, homeDir, 0, appOpts, osmosis.EmptyWasmOpts)🔗 Analysis chain
LGTM! Verify consistency across the codebase.
The change from EmptyWasmOpts to nil aligns with the PR objective of cleaning up unused globals.
Let's verify that this change is consistent across all NewOsmosisApp calls:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of EmptyWasmOpts
# to ensure consistent replacement with nil
# Search for any remaining EmptyWasmOpts usage
rg "EmptyWasmOpts"
# Search for NewOsmosisApp calls to verify nil usage
ast-grep --pattern 'NewOsmosisApp($$$)'
Length of output: 1288
| @@ -24,7 +24,7 @@ func OsmosisAppCreator(logger log.Logger, db db.DB) simtypes.AppCreator { | |||
| homepath, | |||
| legacyInvariantPeriod, | |||
| emptyAppOptions{}, | |||
| app.EmptyWasmOpts, | |||
| nil, | |||
There was a problem hiding this comment.
💡 Codebase verification
Found inconsistency: EmptyWasmOpts is still used in cmd/osmosisd/cmd/root.go
The change to use nil instead of EmptyWasmOpts is incomplete. While most of the codebase has been updated to use nil, there's still one instance in cmd/osmosisd/cmd/root.go that uses EmptyWasmOpts, which needs to be updated for consistency.
cmd/osmosisd/cmd/root.go: Replaceosmosis.EmptyWasmOptswithnilto maintain consistency with other changes
🔗 Analysis chain
LGTM! The change aligns with the cleanup objectives.
The replacement of app.EmptyWasmOpts with nil is consistent with the PR's goal of removing unused globals and simplifies the code by clearly indicating "no options" rather than maintaining an empty options constant.
Let's verify that this change is consistent across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that EmptyWasmOpts is not used anywhere else in the codebase
# and that NewOsmosisApp calls consistently use nil for this parameter
# Test 1: Check for any remaining usage of EmptyWasmOpts
rg "EmptyWasmOpts"
# Test 2: Check the pattern of NewOsmosisApp usage
ast-grep --pattern 'NewOsmosisApp($$$)'
Length of output: 1288
| @@ -63,7 +63,7 @@ func NewAppConstructor(chainId string) network.AppConstructor { | |||
| return NewOsmosisApp( | |||
| valCtx.Logger, dbm.NewMemDB(), nil, true, make(map[int64]bool), valCtx.Config.RootDir, 0, | |||
| sims.EmptyAppOptions{}, | |||
| EmptyWasmOpts, | |||
| nil, | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of EmptyWasmOpts found in cmd/osmosisd/cmd/root.go
The change from EmptyWasmOpts to nil is not consistently applied across the codebase. While test files and app/config.go correctly use nil, there's still a reference using EmptyWasmOpts in:
cmd/osmosisd/cmd/root.go: Still usingosmosis.EmptyWasmOptsinNewOsmosisAppcall
🔗 Analysis chain
LGTM! Verify consistent usage across the codebase.
The change from EmptyWasmOpts to nil aligns with the PR objectives of cleaning up unused globals.
Let's verify that this change is consistent across the codebase and that all callers properly handle nil:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that EmptyWasmOpts is completely removed and all NewOsmosisApp calls use nil
# Test 1: Ensure EmptyWasmOpts is removed
rg "EmptyWasmOpts"
# Test 2: Check all NewOsmosisApp calls to ensure they use nil
ast-grep --pattern 'NewOsmosisApp($$$)'
Length of output: 1288
| @@ -358,7 +358,7 @@ | |||
| WithViper("OSMOSIS") | |||
|
|
|||
| tempDir := tempDir() | |||
| tempApp := osmosis.NewOsmosisApp(log.NewNopLogger(), cosmosdb.NewMemDB(), nil, true, map[int64]bool{}, tempDir, 5, sims.EmptyAppOptions{}, osmosis.EmptyWasmOpts, baseapp.SetChainID("osmosis-1")) | |||
| tempApp := osmosis.NewOsmosisApp(log.NewNopLogger(), cosmosdb.NewMemDB(), nil, true, map[int64]bool{}, tempDir, 5, sims.EmptyAppOptions{}, nil, baseapp.SetChainID("osmosis-1")) | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of EmptyWasmOpts found in the codebase
There's still one instance in cmd/osmosisd/cmd/root.go using osmosis.EmptyWasmOpts while other instances use nil:
- Line 361:
NewOsmosisApp(..., nil) - Another call in the same file:
NewOsmosisApp(..., osmosis.EmptyWasmOpts)
This inconsistency should be addressed by replacing EmptyWasmOpts with nil in all instances to maintain consistency with the PR's objective of removing unused Wasm-related globals.
🔗 Analysis chain
LGTM! Verify consistency of nil usage.
The replacement of EmptyWasmOpts with nil aligns with the PR objective of removing unused Wasm-related globals. This is a cleaner approach when no specific Wasm options are needed.
Let's verify that this change is consistent across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of EmptyWasmOpts
rg "EmptyWasmOpts"
# Search for NewOsmosisApp calls to ensure consistent nil usage
rg "NewOsmosisApp.*\(" -A 1
Length of output: 1895
| ### State Machine Breaking | ||
|
|
||
| * [#8732](https://github.com/osmosis-labs/osmosis/pull/8732) fix: iterate delegations continue instead of erroring | ||
|
|
There was a problem hiding this comment.
I think this was mistakenly removed?
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
Closes: #XXX
What is the purpose of the change
In app.go we have few wasm globals that are probably old artifacts, are not used anywhere and can be removed:
WasmProposalsEnabled,EnableSpecificWasmProposals.The
EmptyWasmOptscan be easily changed tonilwhere it is used (mainly tests) .This way we reduce amount of not needed globals.
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)