feat: simapp/v2 #20412
feat: simapp/v2 #20412
Conversation
WalkthroughThe series of changes across various files and directories encompass substantial updates to the Changes
Sequence Diagram(s)Silently ignoring the generation of sequence diagrams because the changes are too varied and extensive. Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
C:CLI
C:x/auth
C:x/upgrade
C:x/genutil
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (5)
contrib/images/simd-env/Dockerfile (3)
Line range hint
3-3: Pin versions inapk addto ensure reproducible builds.- RUN apk add build-base git linux-headers + RUN apk add build-base=1.0.0 git=2.24.3 linux-headers=4.19.36
Line range hint
39-39: Always tag the version of an image explicitly to ensure consistency across builds.- FROM alpine AS run + FROM alpine:3.12 AS run
Line range hint
40-40: Pin versions inapk addto avoid potential issues with package updates.- RUN apk add bash curl jq + RUN apk add bash=5.0 curl=7.67.0 jq=1.6server/v2/streaming/streaming_test.go (1)
15-15: Missing documentation for thePluginTestSuitestruct and its fields.Consider adding comments to explain the purpose and usage of this test suite and its fields.
simapp/v2/app_di.go (1)
43-43: Consider adding a docstring forDefaultNodeHometo explain its purpose.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
server/v2/cometbft/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sum
Files selected for processing (9)
- .github/workflows/test.yml (1 hunks)
- contrib/images/simd-env/Dockerfile (1 hunks)
- runtime/v2/module.go (2 hunks)
- scripts/simapp-v2-init.sh (1 hunks)
- server/v2/cometbft/go.mod (1 hunks)
- server/v2/streaming/context.go (1 hunks)
- server/v2/streaming/streaming_test.go (1 hunks)
- simapp/v2/app_di.go (1 hunks)
- simapp/v2/go.mod (1 hunks)
Files skipped from review due to trivial changes (1)
- server/v2/streaming/context.go
Additional context used
Path-based instructions (3)
server/v2/streaming/streaming_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"runtime/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/app_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Hadolint
contrib/images/simd-env/Dockerfile
[warning] 3-3: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>(DL3018)
[info] 3-3: Use the
--no-cacheswitch to avoid the need to use--updateand remove/var/cache/apk/*when done installing packages (DL3019)
[warning] 39-39: Always tag the version of an image explicitly (DL3006)
[warning] 40-40: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>(DL3018)
[info] 40-40: Use the
--no-cacheswitch to avoid the need to use--updateand remove/var/cache/apk/*when done installing packages (DL3019)
Additional comments not posted (13)
scripts/simapp-v2-init.sh (2)
24-30: Optimize thejqcommands by combining them into a single invocation.
18-18: Ensure that theCONFIGvariable is properly quoted to prevent word splitting and globbing issues.runtime/v2/module.go (2)
104-104: ReturningnilforProvideAppVersionModifieris consistent with the intended functionality.
168-168: Ensure that appending toStoreKeysis necessary and does not introduce redundancy.simapp/v2/go.mod (6)
3-3: Go version1.22.2is specified. Verify that this version is compatible with all dependencies and the overall project requirements.
1-1: Ensure that the module pathcosmossdk.io/simapp/v2is correctly set up in DNS or a similar service for package fetching.Verification successful
The module path
cosmossdk.io/simapp/v2resolves correctly, as confirmed by the successful HTTP response fromcosmossdk.io.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the module path resolves correctly. dig +short cosmossdk.ioLength of output: 96
Script:
#!/bin/bash # Description: Check if the module path resolves correctly using curl. curl -I cosmossdk.ioLength of output: 991
245-271: Verify that local paths in thereplacedirective are correct and accessible. This is crucial for local development and testing.
286-298: Thereplacedirectives for server v2 integration seem comprehensive. Ensure that all specified paths are valid and the modules at those paths are at the expected versions.
276-276: Thereplacedirectives are used to override module versions:
- Verify the forked version of
github.com/99designs/keyringatgit.luolix.top/cosmos/keyring v1.2.0is maintained and up-to-date.- Ensure that the local path replacement for
github.com/cosmos/cosmos-sdkpoints to the correct and latest version of the SDK.- Check the
github.com/syndtr/goleveldbreplacement for any potential issues with the specified version.Also applies to: 278-278, 283-283
5-48: Review the versions and the necessity of each dependency in therequireblock. Ensure that all versions are consistent and there are no conflicts or known security vulnerabilities..github/workflows/test.yml (3)
491-491: Using the latest version ofactions/checkout@v4is appropriate for ensuring the latest features and fixes are utilized.
492-497: Correct usage ofactions/setup-go@v5with Go version 1.22 ensures the build is using the latest stable Go release. Caching is enabled to improve build times.
498-506: Usingtechnote-space/get-diff-action@v6.1.2to check for relevant file changes before running tests is efficient and prevents unnecessary test executions.
| app.App, err = appBuilder.Build() | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Again, consider replacing panic with a more user-friendly error handling strategy during the application build process.
- panic(err)
+ log.Fatalf("Failed to build application: %v", err)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.
| app.App, err = appBuilder.Build() | |
| if err != nil { | |
| panic(err) | |
| app.App, err = appBuilder.Build() | |
| if err != nil { | |
| log.Fatalf("Failed to build application: %v", err) |
| if err := app.LoadLatest(); err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
The error handling for app.LoadLatest() uses panic, which could be improved by implementing a more graceful error handling approach.
- panic(err)
+ log.Fatalf("Failed to load latest application state: %v", err)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.
| if err := app.LoadLatest(); err != nil { | |
| panic(err) | |
| if err := app.LoadLatest(); err != nil { | |
| log.Fatalf("Failed to load latest application state: %v", err) |
| userHomeDir, err := os.UserHomeDir() | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| DefaultNodeHome = filepath.Join(userHomeDir, ".simappv2") |
There was a problem hiding this comment.
The initialization of DefaultNodeHome uses panic on error. Consider using a more graceful error handling mechanism to improve robustness.
- panic(err)
+ log.Fatalf("Failed to get user home directory: %v", err)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.
| userHomeDir, err := os.UserHomeDir() | |
| if err != nil { | |
| panic(err) | |
| } | |
| DefaultNodeHome = filepath.Join(userHomeDir, ".simappv2") | |
| userHomeDir, err := os.UserHomeDir() | |
| if err != nil { | |
| log.Fatalf("Failed to get user home directory: %v", err) | |
| } | |
| DefaultNodeHome = filepath.Join(userHomeDir, ".simappv2") |
| ); err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Using panic in the dependency injection setup can lead to abrupt termination. Consider handling this error more gracefully.
- panic(err)
+ log.Fatalf("Dependency injection failed: %v", err)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 != nil { | |
| panic(err) | |
| ); err != nil { | |
| log.Fatalf("Dependency injection failed: %v", err) |
| homeDir := appOpts.Get("home").(string) // TODO | ||
| scRawDb, err := db.NewGoLevelDB("application", filepath.Join(homeDir, "data"), nil) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
The error handling here also uses panic. It would be better to handle errors more gracefully, especially during initialization.
- panic(err)
+ log.Fatalf("Failed to initialize database: %v", err)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.
| homeDir := appOpts.Get("home").(string) // TODO | |
| scRawDb, err := db.NewGoLevelDB("application", filepath.Join(homeDir, "data"), nil) | |
| if err != nil { | |
| panic(err) | |
| homeDir := appOpts.Get("home").(string) // TODO | |
| scRawDb, err := db.NewGoLevelDB("application", filepath.Join(homeDir, "data"), nil) | |
| if err != nil { | |
| log.Fatalf("Failed to initialize database: %v", err) |
| @@ -9,6 +9,7 @@ replace ( | |||
| cosmossdk.io/depinject => ../../../depinject | |||
| cosmossdk.io/server/v2 => ../ | |||
| cosmossdk.io/server/v2/appmanager => ../appmanager | |||
| cosmossdk.io/store => ../../../store | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The paths specified in the replace section of the go.mod file are not found in the current directory structure. This suggests that the paths might be incorrectly specified or the directory structure has changed. Please verify the paths and update them accordingly.
- Paths not found:
../../../api../../../core../../../core/testing../../../depinject../appmanager../../../store../../../store/v2../../../x/accounts../../../x/auth../../../x/bank../../../x/consensus../../../x/staking
Analysis chain
Ensure the paths in the replace section are correct and accessible. Incorrect paths can lead to build failures.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the paths in the replace section of the go.mod file.
# Test: Check if paths are accessible. Expect: No errors or not found messages.
for path in ../../../api ../../../core ../../../core/testing ../../../depinject ../ ../appmanager ../../../store ../../../store/v2 ../../../x/accounts ../../../x/auth ../../../x/bank ../../../x/consensus ../../../x/staking; do
if [ ! -d "$path" ]; then
echo "Path $path not found."
fi
done
Length of output: 4262
| - name: simapp-v2-smoke-test | ||
| if: env.GIT_DIFF | ||
| run: | | ||
| ./scripts/simapp-v2-init.sh | ||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
The simapp-v2-init.sh script includes some good practices such as set -o errexit and set -o nounset for error handling. However, it lacks comprehensive logging and detailed error messages. Here are some suggestions to improve the script:
-
Initialization and Build Process:
- Check if directories exist before changing into them.
- Verify the success of the
go buildcommand.
-
Configuration Changes:
- Check the success of each
jqcommand to ensure the configuration changes are applied correctly.
- Check the success of each
-
Key Management:
- Verify the success of key addition and address retrieval commands.
-
Genesis Setup:
- Check the success of commands related to genesis account and transaction setup.
-
Block Query Loop:
- Add logging to indicate the progress of the block height query loop and any errors encountered.
Would you like me to provide a revised version of the script with these improvements?
Analysis chain
Ensure that simapp-v2-init.sh includes proper error handling and logging to facilitate debugging and reliability.
Would you like me to review the script for best practices in error handling and provide improvements?
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the content of `simapp-v2-init.sh` for proper error handling.
cat scripts/simapp-v2-init.sh
Length of output: 1362
| cd "$SIMAPP_DIR" | ||
| go build -o "$ROOT/build/simdv2" simdv2/main.go | ||
|
|
||
| $SIMD init simapp-v2-node --chain-id simapp-v2-chain |
There was a problem hiding this comment.
Tip
Codebase Verification
The verification script indicates that the expected directories (~/.simapp, ~/.simapp/config, ~/.simapp/data) do not exist, suggesting that the init command may have failed. This aligns with the review comment's suggestion to verify the success of the init command.
- Please ensure the
initcommand inscripts/simapp-v2-init.shis executed successfully and the necessary directories are created before proceeding with further steps.
Analysis chain
Consider verifying the success of the init command before proceeding.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the success of the `init` command.
# Test: Check if the `init` command executed successfully. Expect: No errors.
grep "Completed initializing" "$CONFIG/log.txt"
Length of output: 82
Script:
#!/bin/bash
# Description: Verify the success of the `init` command by checking for expected directories or files.
# Expected directories or files created by the `init` command
EXPECTED_DIRS=("~/.simapp" "~/.simapp/config" "~/.simapp/data")
# Check if the expected directories exist
for dir in "${EXPECTED_DIRS[@]}"; do
if [ ! -d "$dir" ]; then
echo "Directory $dir does not exist. The init command may have failed."
else
echo "Directory $dir exists."
fi
done
Length of output: 740
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
codec/depinject.go (1)
Line range hint
51-51: Consider handling missing configuration without using panic.Using
paniccan cause the application to terminate unexpectedly, which might not be desirable. It's better to return an error and let the caller handle it:- panic("auth config bech32 prefix cannot be empty if no custom address codec is provided") + return nil, nil, nil, fmt.Errorf("auth config bech32 prefix cannot be empty if no custom address codec is provided")
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- codec/depinject.go (1 hunks)
Additional context used
Path-based instructions (1)
codec/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
codec/depinject.go (2)
38-38: Ensure error messages are clear and provide sufficient context.While the error message is clear, always consider adding more specific details if the context allows. For instance, including which parameters caused the failure could enhance debugging.
42-42: Good use of error wrapping to maintain error context.This is a good practice as it helps in debugging by providing a stack trace that can lead to the root cause of the issue.
Description
Introduce simapp/v2 which runs on server/v2 infrastructure.
Depends on:
#20387Includes: #20483, #20485
Closes: #20492
Testing
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.
I have...
Summary by CodeRabbit
New Features
Appstruct.SimGenesisAccounttype for simulation accounts in the genesis state.Improvements
ReaderMapandWriterMapinterfaces for clarity.HashandAppHashfields in theBytesmethod of theInfostruct.Refactor
WriterMapstruct to handle state changes recursively.core_router_servicefor lazy initialization of the handler and reflection-based message handling.Integration
go.modfile.Documentation