chore: remove simapp v1 #23009
chore: remove simapp v1 #23009
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request involves a comprehensive refactor of the SimApp module in the Cosmos SDK, transitioning to version 2 (v2). Key changes include the removal of the existing SimApp implementation, updates to build configurations, modifications to workflow files, and the introduction of new build options and testing strategies. The changes encompass multiple files across various directories, focusing on enhancing the build process, updating documentation, and preparing for the new version of the application. Changes
Suggested labels
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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
Documentation and Community
|
julienrbrt
requested review from
a team,
auricom,
tac0turtle,
kocubinski and
facundomedica
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
simapp/v2/README.md (3)
88-88: Remove duplicate textThe sentence is duplicated: "The command will create a new
genesis.jsonfile that includes data from all the validators. The command will create a newgenesis.jsonfile, including data from all the validators"-The command will create a new `genesis.json` file that includes data from all the validators. The command will create a new `genesis.json` file, including data from all the validators +The command will create a new `genesis.json` file that includes data from all the validators.
43-43: Add missing commaAdd a comma before 'but' as it connects two independent clauses.
-The `moniker` and `chain-id` can be anything but you need to use the same `chain-id` subsequently. +The `moniker` and `chain-id` can be anything, but you need to use the same `chain-id` subsequently.🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...monikerandchain-idcan be anything but you need to use the samechain-idsub...(COMMA_COMPOUND_SENTENCE)
63-63: Format command parameters consistentlyUse angle brackets
<>for command parameters to distinguish them from literal values, following common documentation practices.-./simdv2 genesis add-genesis-account [key_name] [amount] +./simdv2 genesis add-genesis-account <key_name> <amount> -./simdv2 genesis gentx [key_name] [amount] --chain-id [chain-id] +./simdv2 genesis gentx <key_name> <amount> --chain-id <chain-id>Also applies to: 73-73
scripts/build/build.mk (1)
Line range hint
65-82: LGTM: Comprehensive build tag handlingThe build tag processing for various features (rocksdb, boltdb, bls12381, benchmark) is well-structured and maintainable.
Consider documenting these build options in the README or developer documentation to ensure proper usage across different architectures and build environments.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
simapp/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (39)
.github/workflows/build.yml(1 hunks).github/workflows/sims-047.yml(0 hunks).github/workflows/test.yml(0 hunks)docs/build/packages/README.md(0 hunks)go.work.example(1 hunks)scripts/build/build.mk(1 hunks)scripts/build/simulations.mk(2 hunks)scripts/init-simapp.sh(0 hunks)simapp/CHANGELOG.md(0 hunks)simapp/README.md(0 hunks)simapp/abci.go(0 hunks)simapp/ante.go(0 hunks)simapp/app.go(0 hunks)simapp/app_config.go(0 hunks)simapp/app_test.go(0 hunks)simapp/benchmark.go(0 hunks)simapp/export.go(0 hunks)simapp/genesis.go(0 hunks)simapp/genesis_account.go(0 hunks)simapp/genesis_account_test.go(0 hunks)simapp/go.mod(0 hunks)simapp/mint_fn.go(0 hunks)simapp/params/doc.go(0 hunks)simapp/params/encoding.go(0 hunks)simapp/sim_bench_test.go(0 hunks)simapp/sim_test.go(0 hunks)simapp/simd/cmd/commands.go(0 hunks)simapp/simd/cmd/config.go(0 hunks)simapp/simd/cmd/root.go(0 hunks)simapp/simd/cmd/root_test.go(0 hunks)simapp/simd/cmd/testnet.go(0 hunks)simapp/simd/cmd/testnet_test.go(0 hunks)simapp/simd/main.go(0 hunks)simapp/test_helpers.go(0 hunks)simapp/upgrades.go(0 hunks)simapp/upgrades_test.go(0 hunks)simapp/v2/README.md(1 hunks)tests/go.mod(0 hunks)tests/integration/store/rootmulti/rollback_test.go(0 hunks)
💤 Files with no reviewable changes (34)
- simapp/genesis.go
- simapp/sim_bench_test.go
- simapp/params/doc.go
- tests/integration/store/rootmulti/rollback_test.go
- docs/build/packages/README.md
- simapp/ante.go
- simapp/upgrades_test.go
- .github/workflows/sims-047.yml
- simapp/simd/cmd/testnet_test.go
- simapp/abci.go
- .github/workflows/test.yml
- simapp/simd/cmd/commands.go
- simapp/test_helpers.go
- simapp/README.md
- simapp/simd/main.go
- simapp/go.mod
- simapp/params/encoding.go
- simapp/benchmark.go
- simapp/genesis_account_test.go
- simapp/app_config.go
- simapp/export.go
- simapp/simd/cmd/root.go
- simapp/mint_fn.go
- simapp/CHANGELOG.md
- simapp/upgrades.go
- scripts/init-simapp.sh
- simapp/simd/cmd/root_test.go
- tests/go.mod
- simapp/genesis_account.go
- simapp/sim_test.go
- simapp/simd/cmd/testnet.go
- simapp/app_test.go
- simapp/simd/cmd/config.go
- simapp/app.go
✅ Files skipped from review due to trivial changes (1)
- scripts/build/simulations.mk
🧰 Additional context used
📓 Path-based instructions (1)
simapp/v2/README.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 LanguageTool
simapp/v2/README.md
[uncategorized] ~43-~43: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...moniker and chain-id can be anything but you need to use the same chain-id sub...
(COMMA_COMPOUND_SENTENCE)
🔇 Additional comments (4)
go.work.example (1)
17-17: Verify module path existence
The path change from ./simapp to ./simapp/v2 aligns with the PR objective. However, let's verify that all referenced module paths exist in the repository.
✅ Verification successful
Module path change is valid and properly structured
The path change from ./simapp to ./simapp/v2 is valid as both directories exist, and simapp/v2 contains the required go.mod file. This change aligns with the PR objective of transitioning to simapp v2 while maintaining backward compatibility by keeping the old simapp directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all module paths in go.work.example exist
while IFS= read -r line; do
if [[ $line =~ ^\s*\./[^[:space:]]+ ]]; then
path=$(echo "$line" | tr -d '\t ')
if [ ! -d "$path" ]; then
echo "Missing directory: $path"
fi
fi
done < go.work.example
Length of output: 3607
Script:
#!/bin/bash
# Let's verify the existence of simapp/v2 directory and its go.mod file
if [ -d "simapp/v2" ]; then
echo "simapp/v2 directory exists"
if [ -f "simapp/v2/go.mod" ]; then
echo "simapp/v2/go.mod exists"
else
echo "simapp/v2/go.mod is missing"
fi
else
echo "simapp/v2 directory is missing"
fi
# Also check if there's an old simapp directory
if [ -d "simapp" ]; then
echo "old simapp directory exists"
fi
Length of output: 257
.github/workflows/build.yml (1)
54-54: Ensure consistent build options concatenation
The build commands properly include v2 in the options. However, let's verify the build options handling in the Makefile to ensure correct parsing of comma-separated options.
Also applies to: 57-57, 60-60
✅ Verification successful
Build options are correctly handled in the Makefile
The Makefile properly handles comma-separated build options using findstring function to check for each option individually. The implementation in scripts/build/build.mk correctly processes:
- v2 option for simapp version
- rocksdb for database backend
- Other options like bls12381, secp, and debug flags
The concatenation of options in the workflow file is compatible with the Makefile's parsing mechanism.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify build options handling in Makefile
rg -A 5 'COSMOS_BUILD_OPTIONS' scripts/build/
Length of output: 2964
scripts/build/build.mk (2)
15-15: LGTM: Build option initialization for v2
The introduction of COSMOS_BUILD_OPTIONS with v2 aligns with the PR objective of transitioning to simapp v2.
Let's verify this change doesn't break existing build workflows:
✅ Verification successful
Build option v2 is properly integrated with CI workflows
The COSMOS_BUILD_OPTIONS := v2 change is well-aligned with the existing CI infrastructure:
- The docker workflow only contains a descriptive comment about simapp
- The test workflow already has separate jobs for both simapp versions
- The v2 tests explicitly use
COSMOS_BUILD_OPTIONS=v2, matching the new default
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any workflow files still reference the old simapp paths
rg -l "simapp[^/]" .github/workflows/
Length of output: 95
Script:
#!/bin/bash
# Check the actual references to simapp in the workflow files
rg "simapp[^/]" .github/workflows/test.yml .github/workflows/docker.yml -B 2 -A 2
Length of output: 1437
Line range hint 52-55: Verify the simapp path changes across the codebase
The conditional update of SIMAPP and APPNAME for v2 looks correct, but we should verify the impact.
Let's check for any hardcoded references to the old paths:
✅ Verification successful
Let me check the actual content of these files to understand if they are using the old simapp paths in a way that needs attention.
The simapp path changes are safe and properly handled
The references to "simapp" found in the codebase are either:
- Test files using simapp as a test fixture
- Docker image references
- Configuration checks that are version-aware (like
IsV2()) - Default values and test constants
None of these usages conflict with the v2 path changes in the build system, as they are either properly versioned or independent of the build path configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for hardcoded references to old simapp paths
rg -l "simapp[^/v2]" --type go
# Check for any import statements using the old path
ast-grep --pattern 'import _ "github.com/cosmos/cosmos-sdk/simapp"'
Length of output: 456
Script:
#!/bin/bash
# Check the content of non-v2 files that reference simapp
rg "simapp[^/v2]" x/simulation/log.go x/genutil/types/genesis_test.go tests/systemtests/upgrade_test.go testutil/sims/state_helpers.go tests/integration/auth/client/cli/suite_test.go systemtests/testnet_init.go -C 2
Length of output: 2811
github-actions
bot
added
C:server/v2
| @@ -1,85 +0,0 @@ | |||
| package rootmulti_test | |||
There was a problem hiding this comment.
nice, i was planning to remove this in the remove testutil/integration PR
There was a problem hiding this comment.
Yeah store v1 only needs to be tested in 052 branch. This was importing simapp so I had to :D
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
x/accounts/defaults/lockup/continuous_locking_account_test.go (1)
107-111: Consider adding test cases for invalid validator addressesWhile the current tests cover the happy path, consider adding test cases for invalid validator addresses to ensure proper error handling.
// Add test case for invalid validator address _, err = acc.Undelegate(sdkCtx, &lockuptypes.MsgUndelegate{ Sender: "owner", ValidatorAddress: "invalid_address", Amount: sdk.NewCoin("test", math.NewInt(1)), }) require.Error(t, err)Also applies to: 116-116
x/accounts/defaults/lockup/periodic_locking_account_test.go (1)
139-143: Consider adding test coverage for concurrent unbondingWhile the current tests cover basic unbonding scenarios, consider adding test cases for concurrent unbonding requests to ensure proper handling of multiple entries.
// Add test case for concurrent unbonding _, err = acc.Undelegate(sdkCtx, &lockuptypes.MsgUndelegate{ Sender: "owner", ValidatorAddress: valAddress, Amount: sdk.NewCoin("test", math.NewInt(1)), }) require.NoError(t, err) // Attempt another unbonding before the first one matures _, err = acc.Undelegate(sdkCtx, &lockuptypes.MsgUndelegate{ Sender: "owner", ValidatorAddress: valAddress, Amount: sdk.NewCoin("test", math.NewInt(1)), }) require.NoError(t, err) entries, err := acc.UnbondEntries.Get(sdkCtx, valAddress) require.NoError(t, err) require.Len(t, entries.Entries, 2)Also applies to: 148-148
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
x/accounts/defaults/lockup/continuous_locking_account_test.go(5 hunks)x/accounts/defaults/lockup/delayed_locking_account_test.go(4 hunks)x/accounts/defaults/lockup/periodic_locking_account_test.go(5 hunks)x/accounts/defaults/lockup/permanent_locking_account_test.go(3 hunks)x/accounts/defaults/lockup/utils_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
x/accounts/defaults/lockup/utils_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/accounts/defaults/lockup/permanent_locking_account_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/accounts/defaults/lockup/periodic_locking_account_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/accounts/defaults/lockup/delayed_locking_account_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/accounts/defaults/lockup/continuous_locking_account_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (6)
x/accounts/defaults/lockup/utils_test.go (1)
103-103: Ensure consistent validator address usage
The mock response uses the same undefined valAddress variable, which should match the constant we need to define.
✅ Verification successful
Let me check how valAddress is used in the test files within the same package to ensure consistency.
Variable valAddress is properly defined and used consistently
The valAddress constant is defined in continuous_locking_account_test.go as const valAddress = "val_address" and is used consistently across all test files in the lockup package. The variable in the mock response is correctly using this shared constant.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of validator address across test files
rg -l 'valAddress' .
Length of output: 854
Script:
#!/bin/bash
# Check the usage of valAddress in the lockup test files
rg -A 5 -B 5 'valAddress' ./x/accounts/defaults/lockup/
Length of output: 23858
x/accounts/defaults/lockup/delayed_locking_account_test.go (1)
44-44: Test structure and coverage look good
The test cases are well-structured with proper setup, time manipulation, and assertions. However, they use the same undefined valAddress variable.
Also applies to: 63-63, 87-87, 99-99, 104-104, 108-108, 113-113
✅ Verification successful
valAddress is properly defined in the same package
The valAddress constant is defined in continuous_locking_account_test.go within the same package (lockup), making it accessible to delayed_locking_account_test.go. The test structure and usage of this constant are correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify test coverage for validator operations
ast-grep --pattern 'func Test.*Account(Delegate|Undelegate)' .
Length of output: 64
Script:
#!/bin/bash
# Let's first check the test file content to understand the context
cat x/accounts/defaults/lockup/delayed_locking_account_test.go
# Also search for valAddress definition in test files
rg "valAddress :?=" --type go
Length of output: 5314
x/accounts/defaults/lockup/continuous_locking_account_test.go (2)
19-20: LGTM: Good practice to extract magic string into a constant
The introduction of valAddress constant improves code maintainability by eliminating the repeated string literal.
47-47: LGTM: Consistent usage of valAddress constant
The validator address is consistently referenced using the constant across all test cases, which improves maintainability and reduces the risk of typos.
Also applies to: 66-66, 90-90, 102-102
x/accounts/defaults/lockup/periodic_locking_account_test.go (2)
58-58: LGTM: Consistent usage of valAddress constant in delegate tests
The validator address is consistently referenced using the constant across all delegate test cases.
Also applies to: 77-77, 97-97
121-121: LGTM: Consistent usage of valAddress constant in undelegate tests
The validator address is consistently referenced using the constant in undelegate test cases.
Also applies to: 133-133
| @@ -40,7 +40,7 @@ func TestPermanentAccountDelegate(t *testing.T) { | |||
| acc := setupPermanentAccount(t, sdkCtx, ss) | |||
| _, err := acc.Delegate(sdkCtx, &lockuptypes.MsgDelegate{ | |||
| Sender: "owner", | |||
| ValidatorAddress: "val_address", | |||
| ValidatorAddress: valAddress, | |||
There was a problem hiding this comment.
Critical: Missing variable declaration for valAddress
The variable valAddress is used throughout the test file but is not declared. This will cause compilation errors.
Add the following declaration at the package level:
package lockup
+const valAddress = "cosmos1valoper1..."Also applies to: 63-63, 75-75, 81-81, 85-85, 90-90
There was a problem hiding this comment.
@julienrbrt Thank you for clarifying. It seems you have already handled the declaration or do not require it. Let’s move forward as you wish.
(ﾉ_；)՞
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Let's search the entire repository for occurrences of "valAddress" and see if it's declared somewhere.
rg "valAddress" -A 5
Length of output: 69675
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
x/accounts/defaults/lockup/lockup.go (2)
Line range hint
502-524: Add integer overflow protectionThe calculations in
TrackDelegationshould include checks for potential integer overflow when performing addition operations withdelLockingAmtanddelFreeAmt.if !x.IsZero() { xCoin := sdk.NewCoin(bondDenom, x) + // Check for potential overflow + if delLockingAmt.Add(x).IsNegative() { + return sdkerrors.ErrInvalidCoins.Wrap("integer overflow in delegation amount") + } newDelLocking := delLockingCoin.Add(xCoin) err = bva.DelegatedLocking.Set(ctx, bondDenom, newDelLocking.Amount) if err != nil { return err } }
Line range hint
376-420: Optimize unbonding entries processingThe current implementation performs multiple database operations within loops, which could be expensive for accounts with many unbonding entries. Consider:
- Batch processing the entries
- Caching the stakingUnbonding results
- Using bulk database operations for updates
+ // Cache unbonding delegations to reduce database queries + stakingUnbondingCache := make(map[string][]stakingtypes.UnbondingDelegationEntry) + err = bva.UnbondEntries.Walk(ctx, nil, func(key string, value lockuptypes.UnbondingEntries) (stop bool, err error) { + // Get cached or fetch unbonding entries + stakingUnbonding, ok := stakingUnbondingCache[key] + if !ok { + stakingUnbonding, err = bva.getUnbondingEntries(ctx, delAddr, key) + if err != nil && !errorsmod.IsOf(err, stakingtypes.ErrNoUnbondingDelegation) { + return true, err + } + stakingUnbondingCache[key] = stakingUnbonding + }
🧹 Nitpick comments (1)
x/accounts/defaults/lockup/lockup.go (1)
Line range hint
486-489: Enhance error handling specificityConsider defining custom error types for delegation-specific errors instead of using generic
sdkerrors.ErrInvalidCoins. This would make it easier for clients to handle specific error cases.+// Custom error types +var ( + ErrZeroDelegation = sdkerrors.Register("lockup", 1, "delegation amount is zero") + ErrInsufficientDelegation = sdkerrors.Register("lockup", 2, "insufficient funds for delegation") +) // return error if the delegation amount is zero or if the base coins does not // exceed the desired delegation amount. if delAmt.IsZero() || baseAmt.LT(delAmt) { - return sdkerrors.ErrInvalidCoins.Wrap("delegation attempt with zero coins for staking denom or insufficient funds") + if delAmt.IsZero() { + return ErrZeroDelegation + } + return ErrInsufficientDelegation }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/systemtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
tests/systemtests/go.mod(1 hunks)x/accounts/cli/cli_test.go(1 hunks)x/accounts/defaults/lockup/lockup.go(1 hunks)x/accounts/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/systemtests/go.mod
🧰 Additional context used
📓 Path-based instructions (2)
x/accounts/cli/cli_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/accounts/defaults/lockup/lockup.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (4)
x/accounts/cli/cli_test.go (2)
Line range hint 1-150: Test implementation looks good!
The test suite is well-structured with:
- Comprehensive test cases covering both valid and invalid scenarios
- Proper error handling and validation
- Good use of test utilities and mocks
9-9: Verify protobuf message compatibility with gogoproto
The import change from gogo/protobuf to cosmos/gogoproto aligns with the broader transition. Let's verify there are no breaking changes in protobuf message handling.
✅ Verification successful
Import change from gogo/protobuf to cosmos/gogoproto is safe
The change from github.com/gogo/protobuf/types to github.com/cosmos/gogoproto/types is a safe transition. The test file only uses the Empty type for protobuf message type URLs in test scenarios, and the package already uses other gogoproto imports (jsonpb and proto) from the same new source. No compatibility issues are present.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining gogo/protobuf imports and verify message type compatibility
# Check for any remaining gogo/protobuf imports
echo "Checking for remaining gogo/protobuf imports..."
rg "github.com/gogo/protobuf/types"
# Check for any type assertions or conversions involving these types
echo "Checking for type assertions/conversions..."
ast-grep --pattern 'types.$_'
Length of output: 65903
Script:
#!/bin/bash
# Let's verify the gogoproto types usage in the test file and related files
# Check for any remaining gogo/protobuf/types imports in the test file
echo "Checking test file for gogo/protobuf imports..."
rg -U "github.com/gogo/protobuf/types" x/accounts/cli/cli_test.go
# Check how the types import is used in the test file
echo "Checking usage of types in test file..."
rg -U "types\." x/accounts/cli/cli_test.go
# Check for any related test files that might be using the same types
echo "Checking related test files..."
fd cli_test.go | xargs rg -U "types\."
Length of output: 616
Script:
#!/bin/bash
# Let's check the broader context of the types usage and imports
# Check the full content of cli_test.go to understand the context
echo "Checking full content of cli_test.go..."
cat x/accounts/cli/cli_test.go
# Check for any gogoproto imports in the package
echo "Checking for gogoproto imports in the package..."
rg -U "github.com/cosmos/gogoproto" x/accounts/cli/
# Check for any protobuf types usage in the package
echo "Checking for protobuf types usage..."
rg -U "types\.(Empty|Any|Timestamp)" x/accounts/cli/
Length of output: 5107
x/accounts/go.mod (2)
83-83: Dependency update aligns with gogoproto transition
The change to mark github.com/gogo/protobuf as indirect is consistent with the transition to using github.com/cosmos/gogoproto as the primary protobuf implementation.
Line range hint 184-189: Module replacements are appropriately maintained
The local replacements for bank and staking modules are correctly retained with a clear TODO comment indicating future cleanup plans. This is necessary during the module separation process.
✅ Verification successful
Let me gather more information about the module replacements to verify their consistency.
Module replacements are correctly configured and consistent
The local replacements for bank and staking modules are properly maintained in x/accounts/go.mod, with corresponding modules present in the expected locations (../bank and ../staking). The replacements are also consistently referenced in their respective module go.mod files, confirming the interdependency during the module separation process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify module replacement consistency
# Check if the replaced modules exist in the expected locations
echo "Verifying module locations..."
fd -t d "^bank$|^staking$" ../
Length of output: 359
Script:
#!/bin/bash
# Check the content of go.mod to verify module replacements
echo "Checking go.mod content..."
cat x/accounts/go.mod
# Check if there are any other go.mod files with similar replacements
echo -e "\nChecking other go.mod files..."
fd -t f "go.mod$" | xargs grep -l "replace.*bank\|replace.*staking"
Length of output: 8829
| @@ -386,7 +386,7 @@ func (bva *BaseLockup) checkUnbondingEntriesMature(ctx context.Context) error { | |||
| found := false | |||
| // check if the entry is still exist in the unbonding entries | |||
| for _, e := range stakingUnbonding { | |||
| if e.CompletionTime.Equal(entry.EndTime) && entry.CreationHeight == entry.CreationHeight { | |||
| if e.CompletionTime.Equal(entry.EndTime) && e.CreationHeight == entry.CreationHeight { | |||
There was a problem hiding this comment.
Critical fix: Correct unbonding entry comparison
The comparison now correctly checks the creation height between the staking unbonding entry and the lockup unbonding entry, fixing a logical error that could have affected token unlocking timing.
There was a problem hiding this comment.
It is already a bug fix.
There was a problem hiding this comment.
@julienrbrt, understood! Let me know if there's anything else you need.
Description
ref: #22904
blocked on: #22679
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Release Notes
New Features
SimApp v2with updated instructions for running testnets.Bug Fixes
Documentation
SimApp v2to enhance clarity and usability.Chores