bump up mods.irisnet.org/simapp to cosmos-sdk v0.50.10 #450
Conversation
WalkthroughThe pull request introduces significant updates to the Changes
Assessment against linked issues
Possibly related issues
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: 5
🧹 Outside diff range and nitpick comments (3)
simapp/helpers/test_helpers.go (1)
Line range hint
59-77: Consider improving error handling in signing processWhile the changes to use
SignerDatastruct andGetSignBytesAdapterare good improvements, the code still usespanicfor error handling in test helpers.Consider returning errors instead of panicking:
signBytes, err := authsigning.GetSignBytesAdapter(context.Background(), gen.SignModeHandler(), defaultSignMode, signerData, tx.GetTx()) if err != nil { - panic(err) + return nil, fmt.Errorf("failed to get sign bytes: %w", err) } sig, err := p.Sign(signBytes) if err != nil { - panic(err) + return nil, fmt.Errorf("failed to sign bytes: %w", err) } sigs[i].Data.(*signing.SingleSignatureData).Signature = sig err = tx.SetSignatures(sigs...) if err != nil { - panic(err) + return nil, fmt.Errorf("failed to set signatures: %w", err) }simapp/export.go (1)
Line range hint
78-195: Replace panics with proper error handling inprepForZeroHeightGenesisThe function
prepForZeroHeightGenesiscontains multiple instances where errors are handled usingpanic(err)orlog.Fatal(err). Using panics or abrupt termination can lead to unexpected crashes and poor user experience. It's recommended to modifyprepForZeroHeightGenesisto return an error and handle it appropriately in the calling functionExportAppStateAndValidators, allowing for graceful error propagation and better control over application flow.As a follow-up, consider changing the function signature and error handling:
-func (app *SimApp) prepForZeroHeightGenesis(ctx sdk.Context, jailAllowedAddrs []string) { +func (app *SimApp) prepForZeroHeightGenesis(ctx sdk.Context, jailAllowedAddrs []string) error { // ... within the function, replace panic and log.Fatal with error returns ... - panic(err) + return err - log.Fatal(err) + return err // ... }In the calling function
ExportAppStateAndValidators, handle the error:if forZeroHeight { - app.prepForZeroHeightGenesis(ctx, jailAllowedAddrs) + if err := app.prepForZeroHeightGenesis(ctx, jailAllowedAddrs); err != nil { + return servertypes.ExportedApp{}, err + } }This change promotes better error management and improves application reliability.
simapp/go.mod (1)
Line range hint
196-203: Address Outstanding TODOs in Replace DirectivesThe
replacedirectives include TODO comments regarding deprecated dependencies and known vulnerabilities. Given the update tocosmos-sdkv0.50.10, this may be an opportune time to address these:
Remove Deprecated
jwt-goDependency: Thegithub.aaakk.us.kg/dgrijalva/jwt-gopackage is deprecated and doesn't receive security updates. Consider removing it as noted in issue #13134.Update or Remove
gin-gonic/ginReplace Directive: The replace directive forgithub.aaakk.us.kg/gin-gonic/ginwas added to fix the GHSA-h395-qcrw-5vmq vulnerability. Check if the vulnerability has been resolved in newer versions and update accordingly, potentially removing the replace directive as mentioned in issue #10409.Would you like assistance in updating these dependencies and refining the replace directives?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
simapp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
simapp/app_v2.go(5 hunks)simapp/export.go(6 hunks)simapp/go.mod(1 hunks)simapp/helpers/test_helpers.go(3 hunks)simapp/network.go(1 hunks)simapp/state.go(2 hunks)simapp/test_helpers.go(18 hunks)
🔇 Additional comments (24)
simapp/helpers/test_helpers.go (4)
4-4: LGTM: Import changes align with cosmos-sdk v0.50.10
The import changes correctly reflect the package structure in cosmos-sdk v0.50.10, where:
contextis required for the new signing adapterauthsignhas been renamed toauthsigning
Also applies to: 12-12
38-38: LGTM: Consistent use of converted sign mode
The change correctly uses the validated defaultSignMode from the earlier conversion, maintaining consistency with the new error handling pattern.
Line range hint 45-52: LGTM: Enhanced error handling for transaction building
The addition of proper error handling for SetMsgs and SetSignatures improves the robustness of the test helper. This change aligns with Go best practices for error handling.
27-30: Improved error handling for signature mode conversion
The change properly handles potential errors from APISignModeToInternal, preventing silent failures in test transaction generation.
Let's verify the usage of this new error handling pattern across the codebase:
✅ Verification successful
No other instances of APISignModeToInternal require error handling updates
The verification shows that APISignModeToInternal is only used in simapp/helpers/test_helpers.go and proper error handling has already been implemented at this location. No other instances of this method exist in the codebase that would require similar error handling updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other files might need similar error handling for APISignModeToInternal
rg "APISignModeToInternal" --type go -C 3
Length of output: 474
simapp/network.go (1)
29-31: LGTM! Struct field type updated correctly for cosmos-sdk v0.50.10.
The change from ResponseDeliverTx to ExecTxResult aligns with the cosmos-sdk v0.50.10 update and is implemented correctly.
Let's verify that all usages of this type have been updated in the codebase:
✅ Verification successful
All references to ResponseDeliverTx have been properly migrated to ExecTxResult
The verification confirms that:
- No remaining references to
ResponseDeliverTxexist in the codebase ExecTxResultis consistently used across all relevant files (simapp/network.goandsimapp/test_helpers.go)- The migration is complete and consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to ResponseDeliverTx
# that might need updating to ExecTxResult
# Search for ResponseDeliverTx references
echo "Checking for any remaining ResponseDeliverTx references:"
rg "ResponseDeliverTx" -A 3
# Search for ExecTxResult references to verify the migration
echo "Verifying ExecTxResult usage:"
rg "ExecTxResult" -A 3
Length of output: 827
simapp/state.go (2)
113-113: LGTM: Correct migration to sdkmath package
The change from sdk.ZeroInt() to sdkmath.ZeroInt() aligns with Cosmos SDK v0.50.x's standardization of mathematical operations in the dedicated sdkmath package.
177-182: LGTM: Parameter generation updated correctly
The changes correctly:
- Remove the unused codec parameter from
GetOrGeneratecalls - Migrate to
sdkmath.NewIntfor stake generation
Let's verify that these parameters are still being generated and used correctly:
✅ Verification successful
Parameter generation changes verified successfully
The changes are correctly implemented and the parameters are properly integrated:
- Parameters are defined as constants and used consistently
- Generated values are passed to
SimulationStateviaInitialStakeandNumBondedfields - Values are logged during genesis generation for transparency
- Parameter ranges remain appropriate (max 1e12 for stake, max 300 for validators)
- The removal of codec parameter and migration to
sdkmath.NewIntdon't affect the parameter generation logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other parameter generation patterns in the codebase
rg "GetOrGenerate.*initialStake|GetOrGenerate.*numInitiallyBonded" --type go
# Verify the usage of these parameters
rg "StakePerAccount|InitiallyBondedValidators" --type go
Length of output: 399
Script:
#!/bin/bash
# Let's check how these parameters are used in the initialization context
rg -B 5 -A 5 "StakePerAccount|InitiallyBondedValidators" simapp/state.go
# Also check for any related test files that verify this functionality
fd -e go -E 'vendor/*' test | xargs rg "StakePerAccount|InitiallyBondedValidators"
Length of output: 1048
Script:
#!/bin/bash
# Let's check how these parameters are used in the actual state generation
rg -B 10 -A 10 "initialStake.*numInitiallyBonded" simapp/state.go
# Also check for any related simulation test files
fd simulation_test.go | xargs rg "StakePerAccount|InitiallyBondedValidators"
# Check if there are any other files using these parameters
ast-grep --pattern 'GetOrGenerate($_, $_, $_, $_)'
Length of output: 940
simapp/export.go (4)
8-9: Approved: Correct import statements added
The addition of import statements for storetypes and cmtproto is appropriate and aligns with the updated dependencies.
21-21: Approved: Updated context creation with NewContextLegacy
Using app.NewContextLegacy with cmtproto.Header is appropriate for the updated consensus parameters and ensures compatibility with the latest CometBFT version.
31-34: Approved: Added error handling for ExportGenesisForModules
Adding error handling for app.ModuleManager.ExportGenesisForModules enhances the robustness of the export process by properly handling potential errors during genesis export.
221-224: Approved: Proper error handling when closing the iterator
Adding error handling for iter.Close() ensures any errors during iterator closure are logged, improving resource management and application stability.
simapp/go.mod (2)
3-3: Ensure Build and CI Environments are Updated to Go 1.21
The Go version has been updated to 1.21. Please verify that all development, build, and CI/CD environments are configured to use Go 1.21 to prevent any compatibility or build issues.
16-16: Verify Compatibility with cosmos-sdk v0.50.10
Updating github.com/cosmos/cosmos-sdk to version v0.50.10 is a significant change that may introduce breaking changes. Ensure that all modules and custom code are compatible with this new version.
simapp/test_helpers.go (6)
97-97: Confirm the use of sdkmath.NewInt for coin amounts
Coin amounts are now created using sdkmath.NewInt. This update ensures consistency with the new sdkmath package introduced in Cosmos SDK v0.50.10.
Also applies to: 131-131
244-253: Ensure proper usage of sdkmath.LegacyOneDec() and sdkmath.LegacyZeroDec()
The code now uses sdkmath.LegacyOneDec() and sdkmath.LegacyZeroDec() for decimal values. Verify that these functions are intended for use and consider if transitioning to the non-legacy versions is appropriate.
Check the definitions of these functions to ensure they align with the desired behavior and consider updating to sdkmath.OneDec() and sdkmath.ZeroDec() if applicable.
Line range hint 737-758: Review and update the commented-out code in query functions
Several lines in QueryBalancesExec, QueryBalanceExec, and QueryAccountExec are commented out. Determine if this code is obsolete or needs updating to be compatible with the new Cosmos SDK version.
If the code is no longer needed, consider removing it to clean up the codebase. Otherwise, update it to work with the latest SDK changes.
Also applies to: 770-783
804-807: Verify return types of QueryTx and QueryTxWithHeight functions
The return types of QueryTx and QueryTxWithHeight have been changed to abci.ExecTxResult. Ensure that this aligns with the updated SDK's expected return types and that any calling code handles these changes appropriately.
Confirm that abci.ExecTxResult is the correct return type according to the updated SDK documentation.
Also applies to: 815-819
799-801: Confirm correct initialization of Bech32 codec
In the MsgSendExec function, a Bech32 codec is initialized using the account address prefix from the configuration. Verify that sdk.GetConfig().GetBech32AccountAddrPrefix() returns the correct prefix and that address.NewBech32Codec is the appropriate method.
Run the following script to confirm the Bech32 prefix:
#!/bin/bash
# Description: Print the Bech32 account address prefix from the configuration.
# Expected: The correct Bech32 prefix used across the application.
grep -rn 'SetBech32PrefixForAccount' .581-586: Verify the replacement of BeginBlock and EndBlock with FinalizeBlock
The BeginBlock and EndBlock calls have been commented out and replaced with FinalizeBlock. Ensure that this change aligns with the updates in Cosmos SDK v0.50.10, as FinalizeBlock may have different behavior.
Run the following script to check if FinalizeBlock is the correct replacement:
✅ Verification successful
Let me try a different search strategy to verify this change.
Let me search for the go.mod file to verify the Cosmos SDK version being used.
Let me try a different approach to find dependency information.
Let me check the simapp's go.mod since that's where the change is located.
The replacement of BeginBlock and EndBlock with FinalizeBlock is correct
The change aligns with Cosmos SDK v0.50.10, which is the version being used in the project (as seen in simapp/go.mod). In SDK v0.50+, the ABCI interface was updated to use FinalizeBlock instead of the separate BeginBlock and EndBlock calls as part of the ABCI++ changes. The parameters being passed to FinalizeBlock (Height, Hash, and NextValidatorsHash) are also correct according to the new interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the usage of 'FinalizeBlock' in the codebase.
# Expected: Confirm that 'FinalizeBlock' is used according to the new SDK version.
rg 'FinalizeBlock\('
Length of output: 127
Script:
#!/bin/bash
# Search for BeginBlock and EndBlock patterns to understand the migration
rg -A 2 'BeginBlock|EndBlock' --type go
# Also search for FinalizeBlock with more context
rg -A 2 'FinalizeBlock' --type go
Length of output: 15309
Script:
#!/bin/bash
# Check the Cosmos SDK version in go.mod
cat go.mod | grep cosmos-sdk
Length of output: 69
Script:
#!/bin/bash
# Find and check any go.mod files in the repository
fd -t f "go.mod$"
# Also search for any ABCI interface or type definitions that might help understand the change
rg -A 5 'type.*ABCI.*Interface' --type go
Length of output: 310
Script:
#!/bin/bash
# Check the Cosmos SDK version in simapp/go.mod
cat simapp/go.mod | grep cosmos-sdk
# Also check for any migration guides or comments about FinalizeBlock
rg -B 2 -A 2 'FinalizeBlock.*migration|ABCI.*migration' --type go
Length of output: 285
simapp/app_v2.go (5)
157-157: Include logger in dependency injection providers
The logger has been added to the providers slice in dependency injection. Ensure that this addition is necessary and that the logger is properly utilized throughout the application.
77-77:
Confirm the removal of the capability module
The line capability.AppModuleBasic{} has been commented out in ModuleBasics, indicating the removal of the capability module. Ensure that this module is no longer required and that its removal does not impact the application's functionality.
Run the following script to check for any remaining references to the capability module:
#!/bin/bash
# Description: Search for references to the capability module.
# Expected: No remaining references to `capability` should be found.
rg 'capability' simapp/246-246: Adjust appBuilder.Build method call
The appBuilder.Build method has been updated to omit the logger parameter:
app.App = appBuilder.Build(db, traceStore, baseAppOptions...)Confirm that this change aligns with the updated method signature and that logging functionality remains unaffected.
Run the following script to verify the Build method's signature and usage:
#!/bin/bash
# Description: Verify the `appBuilder.Build` method signature.
# Expected: The method signature should match the new usage without `logger`.
rg 'func\s+\(.*\)\s+Build\(' -A 5301-301:
Update InitChainer function signature and ensure compatibility
The InitChainer function signature has changed to accept a pointer and return a pointer with an error:
func (app *SimApp) InitChainer(ctx sdk.Context, req *abci.RequestInitChain) (*abci.ResponseInitChain, error)Ensure that:
- All overrides of
InitChainermatch the new signature. - Callers handle the new error return value appropriately.
- The application logic accounts for the possibility of errors during initialization.
Run the following script to locate all usages and overrides of InitChainer:
#!/bin/bash
# Description: Find all references to `InitChainer` to ensure they match the new signature.
# Expected: All references should align with the updated function signature.
rg 'InitChainer' -A 310-17: Verify the integration of new modules and updated import paths
New modules have been imported from cosmossdk.io, including log, evidence, feegrant, and upgrade. The dbm import path has also been updated to github.com/cosmos/cosmos-db.
Run the following script to ensure that these new imports are correctly utilized in the codebase:
✅ Verification successful
New modules are correctly integrated and utilized
The verification shows that all the imported modules are properly integrated and used within the codebase:
- Evidence module: Initialized as
AppModuleBasicand has a dedicated keeper (EvidenceKeeper) - Feegrant module: Initialized as
AppModuleBasicand has a dedicated keeper (FeeGrantKeeper) - Upgrade module: Initialized as
AppModuleBasicand has a dedicated keeper (UpgradeKeeper) - Log package: Used for logger initialization
- Store types: Used for KV store management and store key operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new imports are used appropriately.
# Expected: References to the new modules should be found in the codebase.
# Check for usage of the new modules
rg 'evidence|feegrant|upgrade|log' simapp/
# Verify that the updated dbm import is compatible
rg 'github.com/cosmos/cosmos-db' simapp/
Length of output: 2345
Script:
#!/bin/bash
# Let's verify the usage of these modules more precisely with separate searches
echo "=== Checking evidence module usage ==="
rg "evidence" simapp/app_v2.go -A 2
echo -e "\n=== Checking feegrant module usage ==="
rg "feegrant" simapp/app_v2.go -A 2
echo -e "\n=== Checking upgrade module usage ==="
rg "upgrade" simapp/app_v2.go -A 2
echo -e "\n=== Checking log package usage ==="
rg "log\." simapp/app_v2.go -A 2
echo -e "\n=== Checking store types usage ==="
rg "storetypes" simapp/app_v2.go -A 2
Length of output: 2920
| _, err = app.StakingKeeper.ApplyAndReturnValidatorSetUpdates(ctx) | ||
| if err != nil { | ||
| log.Fatal(err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using log.Fatal for error handling
Using log.Fatal(err) in ApplyAndReturnValidatorSetUpdates can cause the application to exit abruptly upon encountering an error. Instead, return the error to the caller for proper handling.
As a follow-up, modify the error handling:
_, err = app.StakingKeeper.ApplyAndReturnValidatorSetUpdates(ctx)
if err != nil {
- log.Fatal(err)
+ return err
}
Ensure that the calling function handles the returned error appropriately.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, err = app.StakingKeeper.ApplyAndReturnValidatorSetUpdates(ctx) | |
| if err != nil { | |
| log.Fatal(err) | |
| _, err = app.StakingKeeper.ApplyAndReturnValidatorSetUpdates(ctx) | |
| if err != nil { | |
| return err |
| if err := app.SlashingKeeper.IterateValidatorSigningInfos( | ||
| ctx, | ||
| func(addr sdk.ConsAddress, info slashingtypes.ValidatorSigningInfo) (stop bool) { | ||
| info.StartHeight = 0 | ||
| app.SlashingKeeper.SetValidatorSigningInfo(ctx, addr, info) | ||
| _ = app.SlashingKeeper.SetValidatorSigningInfo(ctx, addr, info) | ||
| return false | ||
| }, | ||
| ) | ||
| // htlc.PrepForZeroHeightGenesis(ctx, app.HTLCKeeper) | ||
| // random.PrepForZeroHeightGenesis(ctx, app.RandomKeeper) | ||
| // oracle.PrepForZeroHeightGenesis(ctx, app.OracleKeeper) | ||
| // service.PrepForZeroHeightGenesis(ctx, app.ServiceKeeper) | ||
| ); err != nil { | ||
| log.Fatal(err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistent error handling in IterateValidatorSigningInfos
Errors in IterateValidatorSigningInfos are handled using log.Fatal(err), which terminates the application. Consider returning the error instead to allow for graceful error handling.
As a follow-up, adjust the error handling:
); err != nil {
- log.Fatal(err)
+ return err
}
Update the function signature if necessary and handle the error in the caller.
Committable suggestion skipped: line range outside the PR's diff.
simapp/test_helpers.go
Outdated
| bondDemo, err := app.StakingKeeper.BondDenom(ctx) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| initCoins := sdk.NewCoins(sdk.NewCoin(bondDemo, accAmt)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix typo in variable name: 'bondDemo' should be 'bondDenom'
The variable bondDemo appears to be a typo. It should be corrected to bondDenom to accurately reference the bond denomination.
Apply this diff to fix the typo:
- bondDemo, err := app.StakingKeeper.BondDenom(ctx)
+ bondDenom, err := app.StakingKeeper.BondDenom(ctx)
if err != nil {
panic(err)
}
- initCoins := sdk.NewCoins(sdk.NewCoin(bondDemo, accAmt))
+ initCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, accAmt))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bondDemo, err := app.StakingKeeper.BondDenom(ctx) | |
| if err != nil { | |
| panic(err) | |
| } | |
| initCoins := sdk.NewCoins(sdk.NewCoin(bondDemo, accAmt)) | |
| bondDenom, err := app.StakingKeeper.BondDenom(ctx) | |
| if err != nil { | |
| panic(err) | |
| } | |
| initCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, accAmt)) |
simapp/test_helpers.go
Outdated
| bondDemon, err := app.StakingKeeper.BondDenom(ctx) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| initCoins := sdk.NewCoins(sdk.NewCoin(bondDemon, accAmt)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix typo in variable name: 'bondDemon' should be 'bondDenom'
There's a typo in the variable name bondDemon. It should be corrected to bondDenom to properly reference the bond denomination.
Apply this diff to fix the typo:
- bondDemon, err := app.StakingKeeper.BondDenom(ctx)
+ bondDenom, err := app.StakingKeeper.BondDenom(ctx)
if err != nil {
panic(err)
}
- initCoins := sdk.NewCoins(sdk.NewCoin(bondDemon, accAmt))
+ initCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, accAmt))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bondDemon, err := app.StakingKeeper.BondDenom(ctx) | |
| if err != nil { | |
| panic(err) | |
| } | |
| initCoins := sdk.NewCoins(sdk.NewCoin(bondDemon, accAmt)) | |
| bondDenom, err := app.StakingKeeper.BondDenom(ctx) | |
| if err != nil { | |
| panic(err) | |
| } | |
| initCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, accAmt)) |
| // register streaming services | ||
| if err := app.RegisterStreamingServices(appOpts, app.kvStoreKeys()); err != nil { | ||
| logger.Error("failed to register streaming services", "err", err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for streaming service registration
The application now registers streaming services and handles errors by logging and exiting if registration fails:
if err := app.RegisterStreamingServices(appOpts, app.kvStoreKeys()); err != nil {
logger.Error("failed to register streaming services", "err", err)
os.Exit(1)
}Consider whether using os.Exit(1) is appropriate here. Exiting immediately may not allow for proper resource cleanup or graceful shutdown. It might be better to propagate the error upwards or handle it in a way that maintains application stability.
Consider refactoring the error handling to allow the application to shut down gracefully or to retry the registration.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
simapp/test_helpers.go (1)
581-586: Clean up commented BeginBlock/EndBlock codeThe transition to
FinalizeBlockis correct for ABCI++, but the commented-outBeginBlockandEndBlockcode should be removed for cleaner maintenance.Apply this diff to clean up the code:
- //app.BeginBlock(abci.RequestBeginBlock{Header: header}) app.FinalizeBlock(&abci.RequestFinalizeBlock{ Height: header.Height, Hash: header.AppHash, NextValidatorsHash: header.NextValidatorsHash, }) - //app.EndBlock(abci.RequestEndBlock{})Also applies to: 597-597
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
simapp/app_v2.go(5 hunks)simapp/test_helpers.go(18 hunks)
🔇 Additional comments (8)
simapp/app_v2.go (4)
157-157: LGTM: Logger injection and streaming services initialization
The changes properly integrate the logger through the dependency injection system and add streaming services initialization. The error handling with panic is consistent with other initialization-time error handling in the codebase.
Also applies to: 246-251
10-19: Verify database compatibility after migration to cosmos-db
The change from tendermint/tm-db to github.com/cosmos/cosmos-db is a significant migration. While the change is correct for cosmos-sdk v0.50.10, we should verify database compatibility.
#!/bin/bash
# Check for any remaining references to the old database package
rg "tendermint/tm-db" -l
# Check for database interface compatibility
rg "db\." -A 2 -B 2301-301: Verify InitChainer interface implementations
The InitChainer signature has been updated to return (*abci.ResponseInitChain, error). This is a breaking change that requires verification of all implementations.
#!/bin/bash
# Search for other InitChainer implementations
ast-grep --pattern 'func $_(ctx sdk.Context, req *abci.RequestInitChain)'
# Search for InitChainer interface usage
rg "InitChainer.*RequestInitChain.*ResponseInitChain"77-77: Verify removal of capability module dependencies
The capability module has been removed from ModuleBasics. While this aligns with the upgrade, we should verify that no other modules depend on it.
✅ Verification successful
Let me gather more information to verify if there are any remaining capability module dependencies.
Let me check one more thing to verify if these capability imports in test files are actually being used or if they're just unused imports.
Capability module removal is safe to proceed
The capability module references only exist in test files (depinject_test.go) and E2E testing configurations. These test-only dependencies don't affect the production code and will be updated separately. The main application code in simapp/app_v2.go has correctly removed the capability module from ModuleBasics.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining capability module references
rg "capability\." -A 2 -B 2
# Search for capability keeper references
rg "CapabilityKeeper" -A 2 -B 2
Length of output: 373
Script:
#!/bin/bash
# Search for capability imports
rg "\"github.com/cosmos/cosmos-sdk/x/capability"
# Search for capability module initialization or configuration
rg "capability\.(New|Module|Keeper)" -A 2 -B 2
# Search for capability store keys
rg "capability\.StoreKey|capability\.MemStoreKey"
# Search specifically in app.go files for any capability related code
fd "app\.go$" --exec grep -l "capability"
Length of output: 1318
Script:
#!/bin/bash
# Check actual usage of capability types in test files
rg "capabilitytypes\." -A 2 -B 2
# Check for any capability module configuration in app setup
fd "app\.go$" -x cat {} | rg -A 5 -B 5 "capability"
# Check for any capability keeper initialization or usage
ast-grep --pattern 'type $_ struct {
$$$
CapabilityKeeper $$$
$$$
}'
Length of output: 15839
simapp/test_helpers.go (4)
18-21: LGTM: Type updates align with Cosmos SDK v0.50.x
The changes correctly migrate to the new math package types:
- Using
cosmossdk.io/mathand its aliassdkmath - Replacing
sdk.NewIntwithsdkmath.NewInt - Replacing decimal types with their legacy counterparts (e.g.,
sdkmath.LegacyOneDec)
Also applies to: 97-97, 131-131, 244-244, 249-251, 253-253
152-158: LGTM: Improved error handling in chain initialization
The changes enhance error handling:
- InitChain errors are now properly checked
- BondDenom queries include error handling
Also applies to: 321-328, 438-442, 474-478
804-815: LGTM: Updated transaction response types
The changes correctly update the transaction response types from ResponseDeliverTx to ExecTxResult to align with ABCI++ changes.
799-801: LGTM: Improved address handling with Bech32 codec
The change correctly uses the new address codec system for better address handling in transaction execution.
| @@ -737,7 +734,7 @@ func QueryBalancesExec( | |||
| args = append(args, extraArgs...) | |||
|
|
|||
| result := &banktypes.QueryAllBalancesResponse{} | |||
| network.ExecQueryCmd(t, clientCtx, bankcli.GetBalancesCmd(), args, result) | |||
| //network.ExecQueryCmd(t, clientCtx, bankcli.GetBalancesCmd(), args, result) | |||
There was a problem hiding this comment.
Address commented-out query implementation code
Several query-related functions contain large blocks of commented code that should either be:
- Implemented properly if the functionality is needed
- Removed if the functionality is no longer required
Please clarify the intended behavior of these query functions and either implement them or remove the commented code.
Also applies to: 750-755, 770-783
Closed: #437
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores