feat: add cosmwasm #118
feat: add cosmwasm #118
Conversation
WalkthroughThis pull request updates the project to use Go version 1.23.6 across multiple workflows and dependency files. It integrates CosmWasm support by introducing new fields in the application’s configuration, adding WASM-related test cases, and updating upgrade handling to include a WASM keeper. Updates span CI/CD workflow files, linters, documentation, Makefile test targets, application initialization, and test helpers. Additional scripts and Dockerfile adjustments improve WASM library retrieval and smart contract deployment. Dependency versions and genesis parameters have also been revised accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Script as UploadContract.sh
participant Node as Blockchain Node
participant WASM as WASM Keeper
participant Contract as Smart Contract
User->>Script: Execute smart contract upload
Script->>Node: Submit "store contract" transaction
Node->>WASM: Forward request to store WASM code
WASM-->>Node: Return code ID confirmation
Node-->>Script: Transaction success response
Script->>Node: Submit instantiate contract transaction with code ID
Node->>WASM: Forward request for contract instantiation
WASM-->>Node: Return contract address
Node-->>Script: Transaction success response
Script->>User: Output contract address and details
sequenceDiagram
participant Tester as Chain Upgrade Test
participant Gov as Governance Module
participant App as Upgrade Handler
participant WASM as WASM Keeper
Tester->>Gov: Submit upgrade proposal (including WASM parameters)
Gov->>App: Forward proposal for handling upgrade
App->>WASM: Set new CosmWasm parameters (e.g., CodeUploadAccess)
WASM-->>App: Confirmation of parameter update
App-->>Gov: Acknowledge upgrade readiness
Gov-->>Tester: Approval and upgrade initiation
Possibly related PRs
✨ Finishing Touches
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: 4
🧹 Nitpick comments (10)
cmd/manifestd/cmd/root.go (1)
103-103: Check for potential error handling
Though initWasmConfig() doesn’t return an error, verifying that the returned config is valid before proceeding could improve resilience in case future changes introduce error conditions.app/app.go (2)
652-672: Construct the WasmKeeper
The keeper is set up with relevant modules and a path for storing contracts (wasm/). Ensure that directory is secure and restricted in production environments. Also verify that the modules used for IBC and distribution queries are correct for your chain’s needs.
969-996: WASM configuration in ante handler
Reading the WASM config and inserting specialized decorators is a neat design. Consider graceful handling for misconfigured or missing WASM parameters to prevent unexpected panics in production.interchaintest/cosmwasm_test.go (2)
67-67: Follow Go naming conventions for acronyms.The variable name
codeIdshould becodeIDas per Go naming conventions where acronyms should be uppercase.-codeId, err := chain.StoreContract(ctx, user.KeyName(), string(wasmBytes)) +codeID, err := chain.StoreContract(ctx, user.KeyName(), string(wasmBytes))🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 67-67: var-naming: var codeId should be codeID
(revive)
17-17: Add test documentation.Add a comment explaining the purpose and scope of this test function to improve maintainability.
+// TestCosmWasm verifies the functionality of CosmWasm smart contract operations +// by uploading a contract, instantiating it, and performing basic interactions. func TestCosmWasm(t *testing.T) {app/test_helpers.go (1)
361-369: Document the helper function.Add documentation to explain the purpose and behavior of the
makeTestDirfunction.+// makeTestDir creates a unique temporary directory for testing purposes and ensures +// its cleanup after the test completes. It returns the path to the created directory. func makeTestDir(t *testing.T) string {app/sim_test.go (1)
245-245: Address blank identifier warnings.Multiple blank identifiers in function calls make the code less maintainable and hide potentially useful values.
Consider naming the ignored values or documenting why they're ignored:
-newDB, _, _, _, err := simtestutil.SetupSimulation(...) +newDB, dir, logger, skip, err := simtestutil.SetupSimulation(...) +// dir, logger, and skip are unused in this contextAlso applies to: 382-382
🧰 Tools
🪛 GitHub Check: build amd64 for linux
[failure] 245-245:
declaration has 3 blank identifiers (dogsled)🪛 GitHub Actions: Build Manifest
[error] 245-245: declaration has 3 blank identifiers (dogsled)
scripts/upload_contract.sh (2)
7-7: Refactor HOME_DIR assignment to avoid masking return values (SC2155).
Using command substitution directly in the export statement may mask important return values. Consider declaring a temporary variable first. For example:- export HOME_DIR=$(eval echo "${HOME_DIR:-"~/.manifest"}") + HOME_DIR_DEFAULT="${HOME_DIR:-"~/.manifest"}" + export HOME_DIR=$(eval echo "$HOME_DIR_DEFAULT")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 7-7: Declare and assign separately to avoid masking return values.
(SC2155)
11-11: Avoid alias pitfalls in non-interactive scripts (SC2139).
Aliases in scripts are expanded at definition time and may lead to unexpected behavior. A more robust pattern is to define a wrapper function that appends the required flag. For example:- alias BINARY="$BINARY --home=$HOME_DIR" + function runBinary() { + $BINARY --home="$HOME_DIR" "$@" + }Then replace calls to “$BINARY” with “runBinary”.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 11-11: This expands when defined, not when used. Consider escaping.
(SC2139)
MODULE.md (1)
255-316: New COSM WASM Module Documentation
The new COSM WASM Module section (lines 255–316) is detailed and covers contract deployment, administration, and execution commands. To comply with markdown style guidelines (MD026), consider removing trailing punctuation (e.g., the colons at the end of headings like "#### Contract Deployment:", "#### Contract Administration:", and "#### Contract Execution:").🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
263-263: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
269-269: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
276-276: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
284-284: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
294-294: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
306-306: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
scripts/cw_template.wasmis excluded by!**/*.wasm
📒 Files selected for processing (15)
.github/workflows/e2e.yml(2 hunks).golangci.yml(1 hunks)MODULE.md(3 hunks)Makefile(1 hunks)README.md(4 hunks)app/ante.go(3 hunks)app/app.go(14 hunks)app/sim_test.go(13 hunks)app/test_helpers.go(2 hunks)cmd/manifestd/cmd/root.go(3 hunks)go.mod(5 hunks)interchaintest/cosmwasm_test.go(1 hunks)network/manifest-1/set-genesis-params.sh(1 hunks)scripts/test_node.sh(1 hunks)scripts/upload_contract.sh(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Codecov
Makefile
[error] 140-140: The command 'make local-image' failed with exit code 2.
🪛 GitHub Actions: Simulation Tests
Makefile
[error] 301-301: Makefile:301: sim-after-import failed with exit code 2.
🪛 Shellcheck (0.10.0)
scripts/upload_contract.sh
[warning] 7-7: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 11-11: This expands when defined, not when used. Consider escaping.
(SC2139)
🪛 LanguageTool
MODULE.md
[duplication] ~7-~7: Possible typo: you repeated a word.
Context: ... of Contents - Manifest Module - Module Functionality ...
(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~12-~12: Possible typo: you repeated a word.
Context: ...okens-burn-coins) - Proof of Authority Module - Module Functionality...
(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~21-~21: Possible typo: you repeated a word.
Context: ...move-validator-remove) - Token Factory Module - Module Functionality...
(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~35-~35: Possible typo: you repeated a word.
Context: ...-metadata-modify-metadata) - COSM WASM Module - Module Functionality...
(ENGLISH_WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.17.2)
MODULE.md
263-263: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
269-269: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
276-276: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
284-284: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
294-294: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
306-306: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 golangci-lint (1.62.2)
app/ante.go
20-20: could not import github.com/CosmWasm/wasmd/x/wasm/keeper (-: # github.com/CosmWasm/wasmd/x/wasm/keeper
/tmp/go/.go-mod-cache/github.com/!cosm!wasm/[email protected]/x/wasm/keeper/test_common.go:386:3: cannot use runtime.NewKVStoreService(keys[types.StoreKey]) (value of type "cosmossdk.io/core/store".KVStoreService) as "cosmossdk.io/store/types".StoreKey value in argument to NewKeeper: "cosmossdk.io/core/store".KVStoreService does not implement "cosmossdk.io/store/types".StoreKey (missing method Name)
/tmp/go/.go-mod-cache/github.com/!cosm!wasm/[email protected]/x/wasm/keeper/test_common.go:400:3: cannot use availableCapabilities (variable of type []string) as string value in argument to NewKeeper)
(typecheck)
interchaintest/cosmwasm_test.go
118-118: File is not gofumpt-ed
(gofumpt)
[warning] 67-67: var-naming: var codeId should be codeID
(revive)
🪛 GitHub Check: build amd64 for linux
app/sim_test.go
[failure] 245-245:
declaration has 3 blank identifiers (dogsled)
[failure] 382-382:
declaration has 3 blank identifiers (dogsled)
🪛 GitHub Actions: Unit Tests
app/sim_test.go
[warning] 143-143: TestFullAppSimulation is skipped.
[warning] 190-190: TestAppImportExport is skipped.
[warning] 331-331: TestAppSimulationAfterImport is skipped.
[warning] 419-419: TestAppStateDeterminism is skipped.
🪛 GitHub Actions: Build Manifest
app/sim_test.go
[error] 245-245: declaration has 3 blank identifiers (dogsled)
🔇 Additional comments (29)
cmd/manifestd/cmd/root.go (3)
16-16: Add new imports for serverconfig, crisis, wasm, and wasmtypes
No issues found. This import set appears appropriate for WASM integration and crisis handling.Also applies to: 23-23, 27-28
125-142: Validate zero gas price in production
Here, the MinGasPrices is set to "0stake," which may be acceptable for testing but can introduce spam or DOS risks in production. Ensure this configuration is intentional. Returning an interface{} for the config can reduce type safety. If possible, consider returning a concrete struct to prevent runtime casting errors.
144-146: Initialize WASM and crisis flags together
Adding both WASM and crisis module init flags is straightforward and consistent. Ensure no overlap with existing flags.app/app.go (8)
141-143: New imports for CosmWasm
Importing wasm, wasmkeeper, and wasmtypes is consistent with the new WASM integration.
221-221: Grant burner permission to WASM module account
Granting WASM the burner permission allows contract code to burn tokens. Confirm this is intended, as it can be powerful.
277-277: Added WasmKeeper field
Introducing WasmKeeper in ManifestApp is a clean approach to integrate WASM functionality.
338-338: Add WASM store key
Adding wasmtypes.StoreKey ensures proper state isolation for storing WASM data.
386-400: Scoped keeper and skip-upgrades
Defining scopedWasmKeeper and reading skipUpgradeHeights align with standard patterns. The code for loading the wasm configuration is clear. No issues noted.
743-743: Added crisis, manifest, and wasm modules to ModuleManager
Integrating these modules is standard. Confirm that the order of insertion meets each module’s dependency requirements.Also applies to: 745-746
748-775: Register WASM for lifecycle management
Appending wasm to the begin/end blocker and init-genesis sequences properly integrates it into the chain lifecycle. This is a key step in ensuring modules can interact with WASM code during every block phase.
925-925: Ante handler invocation
Calling setAnteHandler with additional WASM parameters is a proper approach for hooking in WASM capabilities at transaction level.app/ante.go (3)
12-12: New imports for store, runtime, and WASM
These added imports accurately reflect the extended WASM-dependent features and store interactions.Also applies to: 16-16, 20-21
37-38: Expanded HandlerOptions
Adding WasmConfig and StoreKey aligns with the new CosmWasm functionality. Ensure both fields remain non-nil to avoid runtime panics.
71-72: New WASM ante decorators
The LimitSimulationGasDecorator uses SimulationGasLimit, and CountTXDecorator relies on a KV store service. Confirm that WasmConfig is always defined, as referencing a nil config could cause errors..golangci.yml (1)
45-46: LGTM! Good addition of CosmWasm prefixes.The addition of CosmWasm-related prefixes to the GCI sections is appropriate for the integration of the CosmWasm module.
scripts/upload_contract.sh (1)
1-32: Overall, the upload contract script is structured well.
The script reliably sets environment variables, adds the required key if missing, and follows the workflow for storing, instantiating, and querying the contract. Make sure that the sleep durations (lines 21, 25, 29) are aligned with the blockchain’s expected response times.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 7-7: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 11-11: This expands when defined, not when used. Consider escaping.
(SC2139)
.github/workflows/e2e.yml (1)
69-69: Integrating the CosmWasm test into the E2E workflow.
The addition of the "ictest-cosmwasm" command in the test matrix ensures that the new CosmWasm module is validated along with other tests. Confirm that a corresponding Makefile target exists and that all necessary dependencies are met.network/manifest-1/set-genesis-params.sh (1)
75-78: CosmWasm Genesis Parameter Configuration.
The new lines configuring the WASM module—setting “code_upload_access” and “instantiate_default_permission” to "Everybody"—are clear and in line with the objective of enhancing contract accessibility. Please verify that these permission values conform with the blockchain’s expected configuration and security requirements.README.md (2)
59-62: Clarify system requirements for CosmWasm compatibility.
The update specifying “Linux (x86_64 with SSSE3 support) or Linux (arm64 with NEON support)” adds important detail regarding processor instruction support. This helps users ensure their hardware is compatible with CosmWasm.
181-190: Addition of the Upload Contract Script section.
The new “Upload Contract script” section clearly explains the purpose and usage of thescripts/upload_contract.shfile. The provided usage example and linkage with existing environment variables (shared with test_node.sh) improve clarity. Consider including a brief troubleshooting note if the contract upload fails.scripts/test_node.sh (1)
88-91: Consistent WASM configuration in test genesis updates.
The appended updates to the genesis parameters for the WASM module, setting both “code_upload_access” and “instantiate_default_permission” to "Everybody", are consistent with the other configuration files. This unified approach should facilitate smoother integration of CosmWasm functionalities.go.mod (4)
36-36: CosmWasm Dependency Addition
The addition of "github.com/CosmWasm/wasmd v0.51.0" on line 36 is well aligned with the PR’s objective to integrate CosmWasm functionality. Please verify that version v0.51.0 is compatible with the rest of your modules and does not introduce unintended breaking changes.
68-68: WasmVM Dependency Inclusion
The inclusion of "github.com/CosmWasm/wasmvm/v2 v2.0.0" as an indirect dependency supports CosmWasm-related operations. Ensure that using an indirect dependency here is intentional and that this version is tested in your WASM contract execution scenarios.
103-103: Distribution Reference Dependency
The new indirect dependency "github.com/distribution/reference v0.5.0" added on line 103 appears to support container or image-related operations. Please double-check that it is truly required and that its version does not conflict with other dependencies.
175-175: OpenContainers Digest Dependency
"Github.com/opencontainers/go-digest v1.0.0" has been added as an indirect dependency (line 175). Verify that this addition meets the requirements for handling container digests and does not lead to compatibility issues.Makefile (3)
123-125: New CosmWasm Test Target
The addition of the "ictest-cosmwasm" target (lines 123–125) is consistent with integrating CosmWasm tests into the CI workflow. Ensure that the "TestCosmWasm" test function is fully implemented and passes all necessary validations in your local and CI environments.
299-304: Review Simulation Target: sim-after-import
The "sim-after-import" target (lines 299–304) runs "TestAppSimulationAfterImport." Given that the CI pipeline reported an exit code 2 on line 301, please review the test parameters (SIM_COMMON_ARGS) and the test implementation to ensure that the simulation is configured correctly.🧰 Tools
🪛 GitHub Actions: Simulation Tests
[error] 301-301: Makefile:301: sim-after-import failed with exit code 2.
132-141: Local Image Build Target Issue
The "local-image" target (lines 132–141) is used to clone and build the heighliner image, but the CI pipeline reported a failure (exit code 2). Please investigate whether the heighliner repository is accessible, the clone/build steps complete successfully, and the binary is installed correctly.🧰 Tools
🪛 GitHub Actions: Codecov
[error] 140-140: The command 'make local-image' failed with exit code 2.
MODULE.md (1)
5-43: Updated Table of Contents
The revised TOC thoroughly reorganizes the modules and introduces the new COSM WASM Module. Please double-check that all links resolve correctly and review any potential duplicate wordings flagged by static analysis.🧰 Tools
🪛 LanguageTool
[duplication] ~7-~7: Possible typo: you repeated a word.
Context: ... of Contents - Manifest Module - Module Functionality ...(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~12-~12: Possible typo: you repeated a word.
Context: ...okens-burn-coins) - Proof of Authority Module - Module Functionality...(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~21-~21: Possible typo: you repeated a word.
Context: ...move-validator-remove) - Token Factory Module - Module Functionality...(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~35-~35: Possible typo: you repeated a word.
Context: ...-metadata-modify-metadata) - COSM WASM Module - Module Functionality...(ENGLISH_WORD_REPEAT_RULE)
interchaintest/cosmwasm_test.go
Outdated
| wasmFile := "../contracts/cw_template.wasm" | ||
| wasmBytes, err := os.ReadFile(wasmFile) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve contract file handling.
The contract file path is hardcoded and lacks existence validation, which could lead to unclear errors if the file is missing.
-wasmFile := "../contracts/cw_template.wasm"
+wasmFile := os.Getenv("TEST_CONTRACT_PATH")
+if wasmFile == "" {
+ wasmFile = "../contracts/cw_template.wasm"
+}
+
+// Verify contract file exists
+if _, err := os.Stat(wasmFile); os.IsNotExist(err) {
+ t.Fatalf("Contract file not found at %s", wasmFile)
+}
wasmBytes, err := os.ReadFile(wasmFile)📝 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.
| wasmFile := "../contracts/cw_template.wasm" | |
| wasmBytes, err := os.ReadFile(wasmFile) | |
| require.NoError(t, err) | |
| wasmFile := os.Getenv("TEST_CONTRACT_PATH") | |
| if wasmFile == "" { | |
| wasmFile = "../contracts/cw_template.wasm" | |
| } | |
| // Verify contract file exists | |
| if _, err := os.Stat(wasmFile); os.IsNotExist(err) { | |
| t.Fatalf("Contract file not found at %s", wasmFile) | |
| } | |
| wasmBytes, err := os.ReadFile(wasmFile) | |
| require.NoError(t, err) |
app/sim_test.go
Outdated
| @@ -415,110 +419,54 @@ | |||
| t.Skip("skipping application simulation") | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Document test skip conditions.
Multiple simulation tests are being skipped. Add documentation explaining the conditions under which these tests should run.
🏁 Script executed:
#!/bin/bash
# Find all skipped tests and their conditions
rg -A 1 "t\.Skip\(" app/sim_test.goLength of output: 261
Action Required: Document Simulation Test Skip Conditions
The simulation tests in app/sim_test.go (e.g., lines showing "t.Skip("skipping application simulation")", "t.Skip("skipping application import/export simulation")", and "t.Skip("skipping application simulation after import")") are being skipped without any accompanying documentation detailing when or why these tests should be run. Please add clear documentation (e.g., conditions, environment settings, or dependency requirements) that explains the circumstances under which each skipped test should execute. This will enhance code maintainability and clarity for future maintainers.
🧰 Tools
🪛 GitHub Actions: Unit Tests
[warning] 419-419: TestAppStateDeterminism is skipped.
|
@fmorency most of these tests are passing locally, but the CI is bugging out about the libwasmvm download link |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/app.go (2)
388-394: Consider using constants for upgrade heights.The upgrade heights are currently hardcoded in the code. Consider moving them to a configuration file or constants for better maintainability.
-skipUpgradeHeights := map[int64]bool{} -for _, h := range cast.ToIntSlice(appOpts.Get(server.FlagUnsafeSkipUpgrades)) { - skipUpgradeHeights[int64(h)] = true -} +const ( + UpgradeHeight1 int64 = 1000000 + UpgradeHeight2 int64 = 2000000 +) + +var defaultSkipUpgradeHeights = map[int64]bool{ + UpgradeHeight1: true, + UpgradeHeight2: true, +} + +skipUpgradeHeights := defaultSkipUpgradeHeights +if heights := cast.ToIntSlice(appOpts.Get(server.FlagUnsafeSkipUpgrades)); len(heights) > 0 { + skipUpgradeHeights = make(map[int64]bool) + for _, h := range heights { + skipUpgradeHeights[int64(h)] = true + } +}
395-399: Add error details to panic message.When reading the wasm config fails, include the error details in the panic message for better debugging.
-panic(fmt.Errorf("error while reading wasm config: %w", err)) +panic(fmt.Errorf("failed to read wasm config from %s: %v", appOpts.Get(flags.FlagHome), err))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/app.go(16 hunks)app/test_helpers.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/test_helpers.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: e2e-tests (ictest-group-poa)
- GitHub Check: e2e-tests (ictest-manifest)
- GitHub Check: e2e-tests (ictest-ibc)
🔇 Additional comments (8)
app/app.go (8)
138-140: LGTM! CosmWasm imports added correctly.The required imports for the CosmWasm module are properly added.
221-221: LGTM! Module account permissions configured correctly.The wasm module account is correctly configured with the
Burnerpermission, which is the standard permission set for CosmWasm modules.
276-276: LGTM! WasmKeeper field added to ManifestApp.The WasmKeeper field is correctly added to the ManifestApp struct.
337-337: LGTM! Store key added for wasm module.The wasm module's store key is correctly added to the KVStoreKeys.
969-990: LGTM! Ante handler updated with wasm configuration.The ante handler is correctly updated to include wasm configuration and store key.
385-386: LGTM! Capability scoping for wasm module.The wasm module's capabilities are correctly scoped using the capability keeper.
745-775: Verify module ordering in genesis and block processing.The wasm module is correctly added to the module manager and block processing order. However, verify that:
- The module is added at the end of InitGenesis as required for contract instantiation
- The module is added to both BeginBlockers and EndBlockers
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check module ordering in genesis rg -A 10 'genesisModuleOrder.*:=.*\[' app/app.go # Check module ordering in begin/end blockers rg -A 10 'OrderBeginBlockers.*=.*append' app/app.go rg -A 10 'OrderEndBlockers.*=.*append' app/app.goLength of output: 1292
The wasm module ordering is correctly implemented.
- The wasm module is properly appended to OrderBeginBlockers, OrderEndBlockers, and OrderInitGenesis, ensuring it is processed at the intended order and at the end of genesis for proper contract instantiation.
652-670: Verify WasmKeeper initialization parameters.The WasmKeeper initialization looks correct but has some points to verify:
- The supported features list includes versions up to "cosmwasm_2_0"
- The wasm directory path is set to
filepath.Join(homePath, "wasm")✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if the wasm directory exists and is writable test -d "wasm" || mkdir wasm if [ ! -w "wasm" ]; then echo "Warning: wasm directory is not writable" exit 1 fi # Check for any existing wasm contracts find wasm -type f -name "*.wasm" | while read -r contract; do echo "Found contract: $contract" doneLength of output: 103
I'm going to run an additional shell script that checks for "cosmwasm_2_0" in "app/app.go" to verify that the WasmKeeper is indeed configured with the expected supported features.
🏁 Script executed:
#!/bin/bash # Check that the WasmKeeper initialization in app/app.go includes the "cosmwasm_2_0" feature. rg "cosmwasm_2_0" -n app/app.goLength of output: 162
WasmKeeper configuration verified
- The supported features list correctly includes "cosmwasm_2_0".
- The wasm directory path is indeed set using filepath.Join(homePath, "wasm").
Don't run wasm proposal in the simulator as it clashes with the POA governance.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/sim_test.go (1)
529-540: Consider using test-scoped POA admin configuration.Setting environment variables in tests can lead to test interference if tests run in parallel. Consider:
- Using a test-scoped configuration mechanism
- Cleaning up the environment variable in test cleanup
func setPOAAdmin(config simulationtypes.Config) error { r := rand.New(rand.NewSource(config.Seed)) params := simulation.RandomParams(r) accs := simulationtypes.RandomAccounts(r, params.NumKeys()) poaAdminAddr := accs[0] - err := os.Setenv("POA_ADMIN_ADDRESS", poaAdminAddr.Address.String()) - if err != nil { - return err - } - return nil + return setTestPOAAdmin(poaAdminAddr.Address.String()) } +// setTestPOAAdmin sets the POA admin address in a test-scoped configuration +func setTestPOAAdmin(address string) error { + // TODO: Implement test-scoped configuration + return nil +}
🧹 Nitpick comments (2)
simulation/sim_params.json (1)
9-16: New Simulation Parameters for Contract ProposalsThe addition of the following keys:
• "weight_instantiate_contract_proposal"
• "weight_update_admin_proposal"
• "weight_execute_contract_proposal"
• "weight_clear_admin_proposal"
• "weight_migrate_contract_proposal"
• "weight_pin_codes_proposal"
• "weight_unpin_codes_proposal"
• "weight_update_instantiate_config_proposal"enhances the simulation configuration for contract-related proposals in the CosmWasm module. They are all set to 0 by default, which appears to represent placeholder values. Please verify that these defaults are the intended initial weights and consider whether a consistent naming convention is desired. Notably, the earlier keys follow the "op_weight_" prefix, whereas these new parameters omit it—if this naming difference is intentional, a brief comment or documentation update might be useful for clarity.
app/sim_test.go (1)
435-435: Document the temporary reduction in simulation runs.The comment indicates that the number of simulation runs was reduced from 5 to 3 for faster CI. Consider:
- Adding a configuration flag to control this value
- Documenting when to use the full number of runs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/sim_test.go(9 hunks)simulation/sim_params.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: e2e-tests (ictest-cosmwasm)
- GitHub Check: e2e-tests (ictest-group-poa)
- GitHub Check: e2e-tests (ictest-manifest)
- GitHub Check: e2e-tests (ictest-tokenfactory)
- GitHub Check: e2e-tests (ictest-poa)
- GitHub Check: e2e-tests (ictest-ibc)
- GitHub Check: tests
- GitHub Check: build
🔇 Additional comments (2)
app/sim_test.go (2)
83-83: LGTM! Good use of temporary directories.The change to use temporary directories for each test is a good practice as it ensures proper test isolation and automatic cleanup.
Also applies to: 100-100, 154-154, 217-217, 260-260, 344-344, 392-392, 453-453, 476-477
89-91: Document test skip conditions.Multiple simulation tests are being skipped without clear documentation explaining:
- When these tests should run
- What conditions need to be met
- How to enable these tests
Also applies to: 143-145, 197-199, 333-335, 417-419
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/ante.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
app/ante.go
20-20: could not import github.com/CosmWasm/wasmd/x/wasm/keeper (-: # github.com/CosmWasm/wasmd/x/wasm/keeper
/tmp/go/.go-mod-cache/github.com/!cosm!wasm/[email protected]/x/wasm/keeper/test_common.go:386:3: cannot use runtime.NewKVStoreService(keys[types.StoreKey]) (value of type "cosmossdk.io/core/store".KVStoreService) as "cosmossdk.io/store/types".StoreKey value in argument to NewKeeper: "cosmossdk.io/core/store".KVStoreService does not implement "cosmossdk.io/store/types".StoreKey (missing method Name)
/tmp/go/.go-mod-cache/github.com/!cosm!wasm/[email protected]/x/wasm/keeper/test_common.go:400:3: cannot use availableCapabilities (variable of type []string) as string value in argument to NewKeeper)
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: e2e-tests (ictest-cosmwasm)
- GitHub Check: e2e-tests (ictest-group-poa)
- GitHub Check: e2e-tests (ictest-manifest)
- GitHub Check: e2e-tests (ictest-tokenfactory)
- GitHub Check: e2e-tests (ictest-poa)
- GitHub Check: e2e-tests (ictest-ibc)
- GitHub Check: tests
- GitHub Check: build
🔇 Additional comments (2)
app/ante.go (2)
20-21: Verify CosmWasm dependency versions.The static analysis indicates potential compatibility issues with the CosmWasm dependencies. Let's verify the versions and compatibility.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check CosmWasm dependency versions and compatibility # Check go.mod for CosmWasm version echo "CosmWasm version in go.mod:" go list -m github.com/CosmWasm/wasmd # Check for any dependency conflicts go mod graph | grep CosmWasmLength of output: 16002
CosmWasm Dependency Versions Verified Successfully
The go.mod file specifies CosmWasm/wasmd at v0.51.0, which aligns with the imports in app/ante.go. Although the dependency graph shows that one test module (interchaintest) references an earlier wasmd version (v0.42.1), this does not affect the runtime configuration of our ante handler. No compatibility issues impacting the core functionality were found.
🧰 Tools
🪛 golangci-lint (1.62.2)
20-20: could not import github.com/CosmWasm/wasmd/x/wasm/keeper (-: # github.com/CosmWasm/wasmd/x/wasm/keeper
/tmp/go/.go-mod-cache/github.com/!cosm!wasm/[email protected]/x/wasm/keeper/test_common.go:386:3: cannot use runtime.NewKVStoreService(keys[types.StoreKey]) (value of type "cosmossdk.io/core/store".KVStoreService) as "cosmossdk.io/store/types".StoreKey value in argument to NewKeeper: "cosmossdk.io/core/store".KVStoreService does not implement "cosmossdk.io/store/types".StoreKey (missing method Name)
/tmp/go/.go-mod-cache/github.com/!cosm!wasm/[email protected]/x/wasm/keeper/test_common.go:400:3: cannot use availableCapabilities (variable of type []string) as string value in argument to NewKeeper)(typecheck)
71-72: Verify decorator order and KVStoreService compatibility.Two concerns need to be addressed:
The placement of WASM decorators at the beginning of the chain needs verification. Typically, setup and basic validation decorators should run first.
There's a potential compatibility issue with
runtime.NewKVStoreService. The static analysis indicates that the returnedKVStoreServicemight not implement the required interface.Let's verify the CosmWasm decorator order in other projects:
Consider moving the WASM decorators after the basic setup and validation decorators to ensure proper transaction processing order.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check CosmWasm decorator order in other projects # Search for NewLimitSimulationGasDecorator usage echo "Searching for NewLimitSimulationGasDecorator usage patterns:" rg --type go "NewLimitSimulationGasDecorator.*anteDecorators" -A 10 -B 10 # Search for NewCountTXDecorator usage echo "Searching for NewCountTXDecorator usage patterns:" rg --type go "NewCountTXDecorator.*anteDecorators" -A 10 -B 10Length of output: 380
🏁 Script executed:
#!/bin/bash # New Script: Gather additional context on CosmWasm decorator usage and KVStoreService # Search for any usage of NewLimitSimulationGasDecorator across the repository. echo "Searching for any usage of wasmkeeper.NewLimitSimulationGasDecorator:" rg --type go "wasmkeeper\.NewLimitSimulationGasDecorator" -A 5 -B 5 # Search for any usage of NewCountTXDecorator across the repository. echo "Searching for any usage of wasmkeeper.NewCountTXDecorator:" rg --type go "wasmkeeper\.NewCountTXDecorator" -A 5 -B 5 # Search for any usage of runtime.NewKVStoreService across the repository. echo "Searching for any usage of runtime.NewKVStoreService:" rg --type go "runtime\.NewKVStoreService" -A 5 -B 5Length of output: 9257
Decorator order and KVStoreService integration verified.
• The new WASM decorators in app/ante.go are positioned after the context setup decorator and before subsequent validation and circuit breaker decorators, which follows the established pattern for proper transaction processing.
• The usage of runtime.NewKVStoreService in NewCountTXDecorator is consistent with its use elsewhere in the codebase (e.g., in app/app.go for various keepers), indicating that the returned service properly implements the required interfaces.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
app/app.go (1)
1224-1237: Add wasm module parameter store subspace.The wasm module's parameter store subspace is not registered in the
initParamsKeeperfunction. This could cause issues with wasm module configuration.Add the wasm subspace registration:
paramsKeeper.Subspace(authtypes.ModuleName) paramsKeeper.Subspace(banktypes.ModuleName) paramsKeeper.Subspace(stakingtypes.ModuleName) paramsKeeper.Subspace(minttypes.ModuleName) paramsKeeper.Subspace(distrtypes.ModuleName) paramsKeeper.Subspace(slashingtypes.ModuleName) paramsKeeper.Subspace(govtypes.ModuleName) paramsKeeper.Subspace(crisistypes.ModuleName) paramsKeeper.Subspace(ibctransfertypes.ModuleName) paramsKeeper.Subspace(ibcexported.ModuleName) paramsKeeper.Subspace(icahosttypes.SubModuleName) paramsKeeper.Subspace(tokenfactorytypes.ModuleName) paramsKeeper.Subspace(manifesttypes.ModuleName) paramsKeeper.Subspace(icacontrollertypes.SubModuleName) +paramsKeeper.Subspace(wasmtypes.ModuleName)
🧹 Nitpick comments (1)
app/app.go (1)
277-277: LGTM! WasmKeeper is properly initialized with all required dependencies.The keeper is correctly configured with all necessary dependencies and the supported features list is comprehensive. However, consider extracting the supported features list to a constant for better maintainability.
+const SupportedWasmFeatures = []string{ + "iterator", + "staking", + "stargate", + "cosmwasm_1_1", + "cosmwasm_1_2", + "cosmwasm_1_3", + "cosmwasm_1_4", + "cosmwasm_2_0", +} app.WasmKeeper = wasmkeeper.NewKeeper( appCodec, runtime.NewKVStoreService(keys[wasmtypes.StoreKey]), app.AccountKeeper, app.BankKeeper, app.StakingKeeper, distrkeeper.NewQuerier(app.DistrKeeper), app.IBCKeeper.ChannelKeeper, app.IBCKeeper.ChannelKeeper, app.IBCKeeper.PortKeeper, scopedWasmKeeper, app.TransferKeeper, app.MsgServiceRouter(), app.GRPCQueryRouter(), filepath.Join(homePath, "wasm"), wasmConfig, - []string{"iterator", "staking", "stargate", "cosmwasm_1_1", "cosmwasm_1_2", "cosmwasm_1_3", "cosmwasm_1_4", "cosmwasm_2_0"}, + SupportedWasmFeatures, GetPoAAdmin(), )Also applies to: 653-671
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/app.go(16 hunks)app/test_helpers.go(3 hunks)cmd/manifestd/cmd/root.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/test_helpers.go
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: e2e-tests (ictest-cosmwasm)
- GitHub Check: e2e-tests (ictest-group-poa)
- GitHub Check: e2e-tests (ictest-manifest)
- GitHub Check: e2e-tests (ictest-tokenfactory)
- GitHub Check: e2e-tests (ictest-poa)
- GitHub Check: e2e-tests (ictest-ibc)
- GitHub Check: build amd64 for linux
- GitHub Check: build amd64 for darwin
- GitHub Check: tests
- GitHub Check: Analyze
- GitHub Check: build
🔇 Additional comments (9)
cmd/manifestd/cmd/root.go (5)
16-17: LGTM! Import changes look good.The new imports are correctly organized and necessary for the WASM integration.
Also applies to: 23-26
104-108: LGTM! Configuration initialization looks good.The change correctly integrates WASM configuration with the existing CometBFT configuration.
133-133: Verify minimum gas price configuration.The minimum gas price is set to "0stake", but based on past review comments, this value might need to match the validator minimum gas price (e.g., "0.0011umfx").
126-143: LGTM! WASM configuration structure looks good.The function correctly:
- Embeds the server config
- Adds WASM configuration
- Combines templates
145-148: LGTM! Flag initialization looks good.The function correctly adds initialization flags for both crisis and WASM modules.
app/app.go (4)
138-140: LGTM! CosmWasm imports and module permissions are correctly configured.The CosmWasm module is properly imported and the module account is correctly configured with burner capability for contract management.
Also applies to: 222-222
338-338: LGTM! Store key and capability scoping are correctly configured.The wasm module's store key is properly added and the capability keeper is correctly scoped for the wasm module.
Also applies to: 387-387
970-991: LGTM! Ante handler is properly configured with wasm support.The ante handler is correctly updated to include wasm configuration, with proper error handling in place.
761-776: LGTM! Module ordering is correctly configured.The wasm module is properly added to begin blockers, end blockers, and init genesis orderings. The placement after other modules is appropriate as wasm contracts might depend on other module functionalities.
Also applies to: 823-823, 842-842, 869-869
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
interchaintest/cosmwasm_test.go (3)
64-64: 🛠️ Refactor suggestionImprove contract file handling.
The contract file path is hardcoded and lacks existence validation, which could lead to unclear errors if the file is missing.
132-132:⚠️ Potential issueFix incorrect assertion after increment operation.
The test expects the count to be 0 after incrementing it, which is logically incorrect. After incrementing, the count should be 1.
137-137: 🛠️ Refactor suggestionHandle cleanup errors properly.
The error from
ic.Close()is silently ignored, which could hide cleanup issues.
🧹 Nitpick comments (1)
interchaintest/cosmwasm_test.go (1)
59-59: Follow Go naming conventions for variables.Variables should follow Go naming conventions for abbreviations:
codeIdshould becodeIDcodeIdStrshould becodeIDStrAlso applies to: 66-66
🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 59-59: var-naming: var codeId should be codeID
(revive)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
interchaintest/cosmwasm_test.go(1 hunks)interchaintest/setup.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
interchaintest/setup.go
9-9: File is not gofumpt-ed
(gofumpt)
interchaintest/cosmwasm_test.go
139-139: File is not gofumpt-ed
(gofumpt)
[warning] 59-59: var-naming: var codeId should be codeID
(revive)
[warning] 66-66: var-naming: var codeIdStr should be codeIDStr
(revive)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e-tests (ictest-cosmwasm)
- GitHub Check: e2e-tests (ictest-group-poa)
- GitHub Check: e2e-tests (ictest-manifest)
- GitHub Check: e2e-tests (ictest-tokenfactory)
- GitHub Check: e2e-tests (ictest-poa)
- GitHub Check: e2e-tests (ictest-ibc)
- GitHub Check: tests
🔇 Additional comments (4)
interchaintest/setup.go (1)
9-9: LGTM!The CosmWasm integration changes are correctly implemented:
- Import statement follows Go conventions
- Interface registration is properly placed with other module registrations
Also applies to: 97-97
🧰 Tools
🪛 golangci-lint (1.62.2)
9-9: File is not
gofumpt-ed(gofumpt)
interchaintest/cosmwasm_test.go (3)
16-57: LGTM!The test setup follows best practices with proper chain configuration and user setup.
74-92: LGTM!The contract instantiation is well-implemented with proper error handling and assertions.
94-107: LGTM!The query implementation is correct with proper JSON handling and response parsing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
==========================================
- Coverage 77.85% 75.25% -2.61%
==========================================
Files 33 34 +1
Lines 2190 2287 +97
==========================================
+ Hits 1705 1721 +16
- Misses 397 475 +78
- Partials 88 91 +3 ☔ View full report in Codecov by Sentry. |
scripts/cw_template.wasm
Outdated
There was a problem hiding this comment.
you should add a readme for where this file is, which commit it was used to build, and the machine that built it (i.e. x86/64 or m1 mac)
…manifest-ledger into chalabi/add-cosmwasm
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
interchaintest/chain_upgrade_test.go (3)
34-37: Consider using version constants for better maintainability.The base chain version is hardcoded. Consider extracting version strings into constants or configuration variables for easier maintenance and updates.
+const ( + baseChainVersion = "v0.0.1-rc.4" +) var ( baseChain = ibc.DockerImage{ Repository: "ghcr.io/liftedinit/manifest-ledger", - Version: "v0.0.1-rc.4", + Version: baseChainVersion, UIDGID: "1025:1025", } )
192-192: Use configurable contract path instead of hardcoded value.The wasm file path is hardcoded, which could cause issues in different environments or test configurations.
+const ( + defaultContractPath = "../scripts/cw_template.wasm" +) +func StoreAndInstantiateContract(t *testing.T, ctx context.Context, chain *cosmos.CosmosChain, user ibc.Wallet, accAddr string, contractPath string) string { + if contractPath == "" { + contractPath = defaultContractPath + } // Store contract - wasmFile := "../scripts/cw_template.wasm" + wasmFile := contractPath
197-198: Follow Go naming conventions for acronyms.The variable
codeIdshould becodeIDaccording to Go naming conventions.- codeId := queryLatestCodeId(t, ctx, chain) - require.Equal(t, uint64(1), codeId) + codeID := queryLatestCodeId(t, ctx, chain) + require.Equal(t, uint64(1), codeID)🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 197-197: var-naming: var codeId should be codeID
(revive)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
interchaintest/chain_upgrade_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
interchaintest/chain_upgrade_test.go
68-68: appendAssign: append result not assigned to the same slice
(gocritic)
[warning] 197-197: var-naming: var codeId should be codeID
(revive)
41-41: init$1 - result 0 (error) is always nil
(unparam)
50-50: init$2 - result 0 (error) is always nil
(unparam)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e-tests (ictest-chain-upgrade)
- GitHub Check: e2e-tests (ictest-group-poa)
- GitHub Check: e2e-tests (ictest-tokenfactory)
- GitHub Check: e2e-tests (ictest-ibc)
- GitHub Check: tests
- GitHub Check: build
🔇 Additional comments (2)
interchaintest/chain_upgrade_test.go (2)
41-59: Refactor initialization code to improve error handling and readability.The current implementation uses immediately executed anonymous functions with panic statements, which is not ideal for test setup.
🧰 Tools
🪛 golangci-lint (1.62.2)
41-41: init$1 - result 0 (error) is always nil
(unparam)
50-50: init$2 - result 0 (error) is always nil
(unparam)
68-74: Fix append assignment and add test documentation.The append operation's result needs to be assigned back to the variable.
🧰 Tools
🪛 golangci-lint (1.62.2)
68-68: appendAssign: append result not assigned to the same slice
(gocritic)
interchaintest/chain_upgrade_test.go
Outdated
| t.Log("Waiting for chain to stop...") | ||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace fixed sleep durations with proper wait mechanisms.
Using fixed sleep durations can make tests flaky. Consider using a proper wait mechanism or exponential backoff.
-time.Sleep(5 * time.Second)
+require.Eventually(t, func() bool {
+ // Check if chain has stopped/started
+ return checkChainStatus(ctx, chain)
+}, 30*time.Second, time.Second)Also applies to: 164-165
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
interchaintest/chain_upgrade_test.go (5)
24-30: Add documentation for constants.Add detailed documentation for each constant to explain their purpose and any constraints.
const ( - // Hardcoding the upgrade name to match what's registered in app.RegisterUpgradeHandlers() + // upgradeName is the target version for the chain upgrade. + // IMPORTANT: Must match the version registered in app.RegisterUpgradeHandlers() upgradeName = "v0.0.1-rc.5" + // haltHeightDelta is the number of blocks to wait before proposing the upgrade. + // This provides enough time for the proposal to be processed. haltHeightDelta = int64(15) + // blocksAfterUpgrade is the number of blocks to wait after upgrade + // to verify chain continues to produce blocks. blocksAfterUpgrade = int64(7) )
34-38: Consider making version configurable.Hard-coding the version string makes tests brittle. Consider making it configurable through environment variables or test parameters.
+// BaseChainVersion can be overridden through environment variable +var BaseChainVersion = getEnvOrDefault("BASE_CHAIN_VERSION", "v0.0.1-rc.4") var ( baseChain = ibc.DockerImage{ Repository: "ghcr.io/liftedinit/manifest-ledger", - Version: "v0.0.1-rc.4", + Version: BaseChainVersion, UIDGID: "1025:1025", } )
192-194: Make contract path configurable.The contract path is hard-coded, which could cause issues in different environments. Consider making it configurable.
+ // ContractPath can be overridden through environment variable + contractPath := getEnvOrDefault("CONTRACT_PATH", "../scripts/cw_template.wasm") - wasmFile := "../scripts/cw_template.wasm" + wasmFile := contractPath
197-198: Follow Go naming conventions for ID.According to Go naming conventions, "ID" should be capitalized when it's an acronym.
- codeId := queryLatestCodeId(t, ctx, chain) - require.Equal(t, uint64(1), codeId) + codeID := queryLatestCodeID(t, ctx, chain) + require.Equal(t, uint64(1), codeID)🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 197-197: var-naming: var codeId should be codeID
(revive)
224-226: Add retry mechanism for contract queries.Contract queries might fail temporarily after instantiation. Consider adding a retry mechanism with timeout.
- err = chain.QueryContract(ctx, contractAddr, string(queryMsgBz), &resp) - require.NoError(t, err) + require.Eventually(t, func() bool { + err = chain.QueryContract(ctx, contractAddr, string(queryMsgBz), &resp) + return err == nil + }, 30*time.Second, time.Second, "failed to query contract state")app/upgrades/testnet/v0.0.1-rc.5/upgrades.go (1)
20-31: Add documentation for the upgrade configuration.Consider adding documentation to explain:
- The purpose of this upgrade
- The significance of adding the wasm module
- Any prerequisites or post-upgrade steps
Example:
+// NewUpgrade creates a new upgrade handler for v0.0.1-rc.5 that adds CosmWasm support. +// This upgrade adds the wasm module to the store and configures access permissions +// for contract uploads and instantiations. func NewUpgrade(name string) upgrades.Upgrade {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/upgrades.go(1 hunks)app/upgrades/testnet/v0.0.1-rc.5/upgrades.go(1 hunks)interchaintest/chain_upgrade_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/upgrades.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
interchaintest/chain_upgrade_test.go
68-68: appendAssign: append result not assigned to the same slice
(gocritic)
[warning] 197-197: var-naming: var codeId should be codeID
(revive)
41-41: init$1 - result 0 (error) is always nil
(unparam)
50-50: init$2 - result 0 (error) is always nil
(unparam)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: e2e-tests (ictest-chain-upgrade)
- GitHub Check: e2e-tests (ictest-cosmwasm)
- GitHub Check: e2e-tests (ictest-group-poa)
- GitHub Check: e2e-tests (ictest-manifest)
- GitHub Check: e2e-tests (ictest-tokenfactory)
- GitHub Check: e2e-tests (ictest-poa)
- GitHub Check: e2e-tests (ictest-ibc)
- GitHub Check: build amd64 for linux
- GitHub Check: build amd64 for darwin
- GitHub Check: build
🔇 Additional comments (1)
app/upgrades/testnet/v0.0.1-rc.5/upgrades.go (1)
1-17: LGTM!The package name and imports are well-organized and include all necessary dependencies for the upgrade functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/helpers/utils.go (1)
12-14: Add logging to track the source of admin address.For better observability, consider adding logging to indicate whether the admin address is coming from the environment variable or defaulting to the governance module.
+import "cosmossdk.io/log" -func GetPoAAdmin() string { +func GetPoAAdmin(logger log.Logger) string { if addr := os.Getenv("POA_ADMIN_ADDRESS"); addr != "" { + logger.Info("using POA admin address from environment", "address", addr) // ... rest of the code ... } + defaultAddr := authtypes.NewModuleAddress(govtypes.ModuleName).String() + logger.Info("using default POA admin address", "address", defaultAddr) return defaultAddr }app/app.go (1)
375-386: Consider adding error handling for wasm config.The wasm config reading could benefit from more detailed error handling and validation.
// Read wasm config wasmConfig, err := wasm.ReadNodeConfig(appOpts) if err != nil { - panic(fmt.Errorf("error while reading wasm config: %w", err)) + panic(fmt.Errorf("failed to read wasm config: %w", err)) } + +// Validate critical config values +if wasmConfig.ContractDebugMode { + logger.Info("warning: wasm contract debug mode enabled") +} +if wasmConfig.MemoryCacheSize == 0 { + logger.Info("warning: wasm memory cache size is 0") +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(3 hunks)app/app.go(27 hunks)app/helpers/utils.go(1 hunks)app/upgrades/testnet/v0.0.1-rc.5/upgrades.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/upgrades/testnet/v0.0.1-rc.5/upgrades.go
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build amd64 for linux
- GitHub Check: tests
- GitHub Check: build amd64 for darwin
- GitHub Check: Analyze
- GitHub Check: build
🔇 Additional comments (7)
app/app.go (4)
138-140: LGTM! Imports are correctly organized.The CosmWasm imports are properly organized and use specific package names to avoid confusion.
208-208: LGTM! Module permissions are correctly configured.The wasm module is granted the
Burnerpermission, which is the standard permission set for CosmWasm modules.
783-783: LGTM! Module registration order is correct.The wasm module is correctly registered in:
- BeginBlockers after other modules
- EndBlockers after other modules
- Genesis initialization at the end (as required for wasm)
Also applies to: 802-802, 829-829
641-660: Verify WasmKeeper configuration.The WasmKeeper initialization looks correct but please verify:
- The order of dependencies matches the wasmd reference implementation
- The capabilities are appropriate for your use case
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Compare WasmKeeper initialization with wasmd reference. # Search for WasmKeeper initialization in wasmd codebase. rg -A 20 "NewKeeper\(" --type goLength of output: 13834
WasmKeeper Initialization Verified
The WasmKeeper configuration matches the wasmd reference implementation. The order of dependencies (including the use of the distribution querier, IBC fee keeper, channel and port keepers, scoped keeper, and other injected parameters) is in line with the reference, and the built‐in capabilities being used are appropriate for this use case.
Makefile (3)
123-128: Update .PHONY declaration to include new targets.The new test targets should be included in the .PHONY declaration.
-.PHONY: ictest-ibc ictest-tokenfactory +.PHONY: ictest-ibc ictest-tokenfactory ictest-cosmwasm ictest-chain-upgrade
172-172: LGTM! Coverage test improvements.The coverage test command has been improved with:
- Race detection enabled
- Single package testing (-p 1) to avoid test interference
- CPU utilization optimization
219-219: Verify the latest golangci-lint version.The golangci-lint version has been updated. Let's verify this is the latest stable version.
✅ Verification successful
🌐 Web query:
What is the latest stable version of golangci-lint?💡 Result:
The latest stable version of
golangci-lintas of February 2025 is v1.63.4, released on January 3, 2025[8][6]. This version includes bug fixes for linters likedupl,gomodguard, andrevive[1][2]. It is available via Homebrew[6], GitHub Releases[8], and official installation scripts[4].For reproducibility in CI environments, it is recommended to explicitly specify this version during installation rather than using floating tags like
latest[4][6].Key sources:
- Homebrew lists v1.63.4 as the current stable formula[6]
- GitHub shows v1.63.4 as the latest release with January 2025 timestamps[8]
- The changelog confirms v1.63.x series as active maintenance releases[1][2]
Citations:
- 1: https://golangci-lint.run/product/changelog/
- 2: https://github.com/golangci/golangci-lint/blob/master/CHANGELOG.md
- 3: https://gitee.com/sanallen/golangci-lint
- 4: https://golangci-lint.run/welcome/install/
- 5: https://golangci-lint.run/product/roadmap/
- 6: https://formulae.brew.sh/formula/golangci-lint
- 7: https://golangci-lint.run
- 8: https://github.com/golangci/golangci-lint/releases
- 9: https://github.com/golangci/golangci-lint
- 10: https://pkg.go.dev/github.com/golangci/golangci-lint
Verified: golangci-lint version is up-to-date
The Makefile’s version specification (
golangci_version=v1.63.4) matches the latest stable release as confirmed by the web query.
| func GetPoAAdmin() string { | ||
| if addr := os.Getenv("POA_ADMIN_ADDRESS"); addr != "" { | ||
| // Panic if the address is invalid | ||
| _, err := types.AccAddressFromBech32(addr) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("invalid POA_ADMIN_ADDRESS: %s", addr)) | ||
| } | ||
| return addr | ||
| } | ||
|
|
||
| return authtypes.NewModuleAddress(govtypes.ModuleName).String() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning error instead of panicking.
The function currently panics if the POA_ADMIN_ADDRESS environment variable contains an invalid address. In a production environment, this could be disruptive. Consider returning an error that can be handled by the caller.
-func GetPoAAdmin() string {
+func GetPoAAdmin() (string, error) {
if addr := os.Getenv("POA_ADMIN_ADDRESS"); addr != "" {
- // Panic if the address is invalid
_, err := types.AccAddressFromBech32(addr)
if err != nil {
- panic(fmt.Sprintf("invalid POA_ADMIN_ADDRESS: %s", addr))
+ return "", fmt.Errorf("invalid POA_ADMIN_ADDRESS %s: %w", addr, err)
}
- return addr
+ return addr, nil
}
- return authtypes.NewModuleAddress(govtypes.ModuleName).String()
+ return authtypes.NewModuleAddress(govtypes.ModuleName).String(), nil
}📝 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.
| func GetPoAAdmin() string { | |
| if addr := os.Getenv("POA_ADMIN_ADDRESS"); addr != "" { | |
| // Panic if the address is invalid | |
| _, err := types.AccAddressFromBech32(addr) | |
| if err != nil { | |
| panic(fmt.Sprintf("invalid POA_ADMIN_ADDRESS: %s", addr)) | |
| } | |
| return addr | |
| } | |
| return authtypes.NewModuleAddress(govtypes.ModuleName).String() | |
| } | |
| func GetPoAAdmin() (string, error) { | |
| if addr := os.Getenv("POA_ADMIN_ADDRESS"); addr != "" { | |
| _, err := types.AccAddressFromBech32(addr) | |
| if err != nil { | |
| return "", fmt.Errorf("invalid POA_ADMIN_ADDRESS %s: %w", addr, err) | |
| } | |
| return addr, nil | |
| } | |
| return authtypes.NewModuleAddress(govtypes.ModuleName).String(), nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
interchaintest/poa_group_test.go (1)
475-489: 🛠️ Refactor suggestionImprove error handling in createWasmStoreProposal.
The function panics on file read error, which is not ideal for a test helper.
Apply this diff to return an error instead of panicking:
-func createWasmStoreProposal(sender string, wasmFile string) wasmtypes.MsgStoreCode { +func createWasmStoreProposal(sender string, wasmFile string) (wasmtypes.MsgStoreCode, error) { wasmBytes, err := os.ReadFile(wasmFile) if err != nil { - panic(fmt.Sprintf("failed to read wasm file: %v", err)) + return wasmtypes.MsgStoreCode{}, fmt.Errorf("failed to read wasm file: %w", err) } - return wasmtypes.MsgStoreCode{ + return wasmtypes.MsgStoreCode{ Sender: sender, WASMByteCode: wasmBytes, InstantiatePermission: &wasmtypes.AccessConfig{ Permission: wasmtypes.AccessTypeAnyOfAddresses, Addresses: []string{groupAddr}, }, - } + }, nil }
🧹 Nitpick comments (1)
interchaintest/poa_group_test.go (1)
147-185: Enhance contract state verification.While the test verifies the initial state, consider adding more test cases to verify contract state changes through execute messages.
Add test cases for:
- Executing contract messages
- Verifying state changes
- Testing contract query error cases
Example test case to add:
// Execute contract message execMsg := map[string]interface{}{ "increment": struct{}{}, } execMsgBz, err := json.Marshal(execMsg) require.NoError(t, err) wasmExecProposal := createWasmExecuteProposal(groupAddr, contractAddr, string(execMsgBz)) createAndRunProposalSuccess(t, ctx, chain, config, accAddr, []*types.Any{createAny(t, &wasmExecProposal)}) // Verify state change err = chain.QueryContract(ctx, contractAddr, string(queryMsgBz), &resp) require.NoError(t, err) require.Equal(t, 1, resp.Count)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
interchaintest/poa_group_test.go(9 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
interchaintest/poa_group_test.go
103-103: appendAssign: append result not assigned to the same slice
(gocritic)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: e2e-tests (ictest-chain-upgrade)
- GitHub Check: e2e-tests (ictest-cosmwasm)
- GitHub Check: e2e-tests (ictest-group-poa)
- GitHub Check: e2e-tests (ictest-manifest)
- GitHub Check: e2e-tests (ictest-tokenfactory)
- GitHub Check: e2e-tests (ictest-poa)
- GitHub Check: e2e-tests (ictest-ibc)
- GitHub Check: build amd64 for linux
- GitHub Check: build amd64 for darwin
- GitHub Check: tests
- GitHub Check: build
🔇 Additional comments (2)
interchaintest/poa_group_test.go (2)
127-130: LGTM! Well-structured test coverage.The CosmWasm test functions are well-organized and cover essential scenarios:
- Contract store and instantiation
- Unauthorized contract upload
- Unauthorized contract instantiation
666-696: LGTM! Query functions have improved error messages.The query functions now include descriptive error messages when results are empty, as suggested in past review comments.
| wasmGenesis := append(groupGenesis, | ||
| cosmos.NewGenesisKV("app_state.wasm.params.code_upload_access.permission", "AnyOfAddresses"), | ||
| cosmos.NewGenesisKV("app_state.wasm.params.code_upload_access.addresses", []string{groupAddr}), // Only the Group address can upload code | ||
| ) |
There was a problem hiding this comment.
Fix append result assignment.
The result of the append operation is not being assigned back to wasmGenesis. In Go, append returns a new slice that needs to be assigned back to the variable.
Apply this diff to fix the issue:
- wasmGenesis := append(groupGenesis,
+ wasmGenesis = append(groupGenesis,
cosmos.NewGenesisKV("app_state.wasm.params.code_upload_access.permission", "AnyOfAddresses"),
cosmos.NewGenesisKV("app_state.wasm.params.code_upload_access.addresses", []string{groupAddr}), // Only the Group address can upload code
)📝 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.
| wasmGenesis := append(groupGenesis, | |
| cosmos.NewGenesisKV("app_state.wasm.params.code_upload_access.permission", "AnyOfAddresses"), | |
| cosmos.NewGenesisKV("app_state.wasm.params.code_upload_access.addresses", []string{groupAddr}), // Only the Group address can upload code | |
| ) | |
| wasmGenesis = append(groupGenesis, | |
| cosmos.NewGenesisKV("app_state.wasm.params.code_upload_access.permission", "AnyOfAddresses"), | |
| cosmos.NewGenesisKV("app_state.wasm.params.code_upload_access.addresses", []string{groupAddr}), // Only the Group address can upload code | |
| ) |
🧰 Tools
🪛 golangci-lint (1.62.2)
103-103: appendAssign: append result not assigned to the same slice
(gocritic)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
interchaintest/chain_upgrade_test.go (4)
40-59: 🛠️ Refactor suggestionRefactor initialization code to improve error handling and readability.
The current implementation uses immediately executed anonymous functions with panic statements, which is not ideal for test setup.
Consider moving this initialization logic into proper setup functions as previously suggested.
🧰 Tools
🪛 golangci-lint (1.62.2)
40-40: init$1 - result 0 (error) is always nil
(unparam)
49-49: init$2 - result 0 (error) is always nil
(unparam)
67-73:⚠️ Potential issueFix append assignment.
The append operation's result needs to be assigned back to the variable.
Apply this diff to fix the append assignment:
-previousVersionGenesis := append(DefaultGenesis, +previousVersionGenesis = append(DefaultGenesis, cosmos.NewGenesisKV("app_state.group.group_seq", "1"), // ... other genesis KVs ... )🧰 Tools
🪛 golangci-lint (1.62.2)
67-67: appendAssign: append result not assigned to the same slice
(gocritic)
155-155: 🛠️ Refactor suggestionReplace fixed sleep durations with proper wait mechanisms.
Using fixed sleep durations can make tests flaky.
Consider using a proper wait mechanism or exponential backoff as previously suggested.
Also applies to: 162-162
96-98:⚠️ Potential issueAdd cleanup for Docker resources.
The test creates Docker resources but doesn't ensure they're cleaned up after the test.
Add cleanup to prevent resource leaks:
ctx := context.Background() +t.Cleanup(func() { + if err := ic.Close(); err != nil { + t.Logf("Error closing interchain: %v", err) + } +})
🧹 Nitpick comments (2)
interchaintest/chain_upgrade_test.go (2)
189-189: Validate WASM file existence and make path configurable.The hardcoded WASM file path might cause issues in different environments.
Consider:
- Making the path configurable through environment variables or test parameters.
- Adding file existence validation before attempting to store the contract.
-wasmFile := "../scripts/cw_template.wasm" +wasmFile := os.Getenv("WASM_CONTRACT_PATH") +if wasmFile == "" { + wasmFile = "../scripts/cw_template.wasm" +} + +if _, err := os.Stat(wasmFile); os.IsNotExist(err) { + t.Fatalf("WASM contract file not found: %s", wasmFile) +}
61-182: Add test documentation.The test lacks documentation explaining its purpose and requirements.
Add a comprehensive test documentation:
+// TestBasicManifestUpgrade verifies that the chain can be successfully upgraded +// to include CosmWasm functionality. It tests: +// 1. Chain setup with group-based governance +// 2. Upgrade proposal submission and execution +// 3. Post-upgrade CosmWasm contract deployment and interaction +// +// Requirements: +// - Local build of the chain must be available +// - WASM contract file must be present at ../scripts/cw_template.wasm func TestBasicManifestUpgrade(t *testing.T) {🧰 Tools
🪛 golangci-lint (1.62.2)
67-67: appendAssign: append result not assigned to the same slice
(gocritic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(4 hunks)app/upgrades.go(1 hunks)app/upgrades/next/upgrades.go(3 hunks)interchaintest/chain_upgrade_test.go(1 hunks)network/manifest-1/set-genesis-params.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/upgrades.go
- network/manifest-1/set-genesis-params.sh
- Makefile
🧰 Additional context used
🪛 golangci-lint (1.62.2)
interchaintest/chain_upgrade_test.go
67-67: appendAssign: append result not assigned to the same slice
(gocritic)
40-40: init$1 - result 0 (error) is always nil
(unparam)
49-49: init$2 - result 0 (error) is always nil
(unparam)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: e2e-tests (ictest-chain-upgrade)
- GitHub Check: e2e-tests (ictest-cosmwasm)
- GitHub Check: e2e-tests (ictest-group-poa)
- GitHub Check: e2e-tests (ictest-manifest)
- GitHub Check: e2e-tests (ictest-tokenfactory)
- GitHub Check: e2e-tests (ictest-poa)
- GitHub Check: e2e-tests (ictest-ibc)
- GitHub Check: build amd64 for linux
- GitHub Check: build amd64 for darwin
- GitHub Check: tests
- GitHub Check: build
🔇 Additional comments (8)
app/upgrades/next/upgrades.go (8)
6-6: Adoption oferrorsmodis valid.
Usingerrorsmodfrom the Cosmos SDK ecosystem for error wrapping and propagation is a standard practice.
12-12: Confirmed import for CosmWasm.
The addition ofwasmtypesis necessary for CosmWasm integrations. Ensure the corresponding dependency version aligns with the rest of the codebase.
14-14: Helpers import recognized.
Introducing the helpers package is consistent with retrieving addresses for PoA. No issues observed.
23-25: WASM module added to store upgrades.
Listingwasmtypes.ModuleNameinAddedis appropriate to handle new module data. Verify any required migrations are handled if updating from a prior WASM version.
34-34: AppKeepers parameter.
Switching to a named parameterkeepersmakes the code more explicit. This approach is clear and consistent.
37-40: Error handling after migrations.
Capturing and returning errors frommm.RunMigrationsis correct. The logic is concise and robust.
42-52: CosmWasm parameters set with restricted access.
DefiningCodeUploadAccessasAccessTypeAnyOfAddressesreferencinghelpers.GetPoAAdmin()restricts operations to a specific address. Ensure:
helpers.GetPoAAdmin()always returns a valid address.- Any additional addresses that need code upload privileges are included or configured as needed.
54-54: Clean final return.
The final return offromVM, nilneatly completes the upgrade handler.
|
The I tagged cc @chalabi2 Edit: the error seems to be a known issue. I logged a new issue in cosmos/cosmos-sdk#23683. I'm curious to know why it fails intermittently. Edit2: The test sometimes fails because one node reaches the upgrade height before the other, shutdowns, and consensus breaks. The other node shuts down as well and panics when restarting. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
interchaintest/chain_upgrade_test.go (4)
40-59: 🛠️ Refactor suggestionRefactor initialization code to improve error handling and readability.
The current implementation uses immediately executed anonymous functions with panic statements, which is not ideal for test setup.
Apply this diff to improve error handling and readability:
-// Initialize group policy with decision policy -_ = func() error { - err := groupPolicy.SetDecisionPolicy(createThresholdDecisionPolicy("1", 10*time.Second, 0*time.Second)) - if err != nil { - panic(err) - } - return nil -}() - -// Initialize codec for proper group policy serialization -_ = func() error { - enc := AppEncoding() - grouptypes.RegisterInterfaces(enc.InterfaceRegistry) - cdc := codec.NewProtoCodec(enc.InterfaceRegistry) - _, err := cdc.MarshalJSON(groupPolicy) - if err != nil { - panic(err) - } - return nil -}() +func initGroupPolicy() { + if err := groupPolicy.SetDecisionPolicy( + createThresholdDecisionPolicy("1", 10*time.Second, 0*time.Second), + ); err != nil { + log.Fatalf("failed to set decision policy: %v", err) + } +} + +func initCodec() { + enc := AppEncoding() + grouptypes.RegisterInterfaces(enc.InterfaceRegistry) + cdc := codec.NewProtoCodec(enc.InterfaceRegistry) + if _, err := cdc.MarshalJSON(groupPolicy); err != nil { + log.Fatalf("failed to marshal group policy: %v", err) + } +} + +func init() { + initGroupPolicy() + initCodec() +}🧰 Tools
🪛 golangci-lint (1.62.2)
40-40: init$1 - result 0 (error) is always nil
(unparam)
49-49: init$2 - result 0 (error) is always nil
(unparam)
67-73:⚠️ Potential issueFix append assignment and add test documentation.
The append operation's result needs to be assigned back to the variable.
Apply this diff:
+// TestBasicManifestUpgrade verifies that the chain can be successfully upgraded +// to include CosmWasm functionality. It tests: +// 1. Chain setup with group-based governance +// 2. Upgrade proposal submission and execution +// 3. Post-upgrade CosmWasm contract deployment +func TestBasicManifestUpgrade(t *testing.T) { + -previousVersionGenesis := append(DefaultGenesis, +previousVersionGenesis = append(DefaultGenesis, cosmos.NewGenesisKV("app_state.group.group_seq", "1"), cosmos.NewGenesisKV("app_state.group.groups", []grouptypes.GroupInfo{groupInfo}), cosmos.NewGenesisKV("app_state.group.group_members", []grouptypes.GroupMember{groupMember1, groupMember2}), cosmos.NewGenesisKV("app_state.group.group_policy_seq", "1"), cosmos.NewGenesisKV("app_state.group.group_policies", []*grouptypes.GroupPolicyInfo{groupPolicy}), )🧰 Tools
🪛 golangci-lint (1.62.2)
67-67: appendAssign: append result not assigned to the same slice
(gocritic)
150-150: 🛠️ Refactor suggestionReplace fixed sleep duration with proper wait mechanism.
Using fixed sleep durations can make tests flaky.
Apply this diff:
-time.Sleep(10 * time.Second) +require.Eventually(t, func() bool { + // Check if chain has stopped + height, err := chain.Height(ctx) + return err != nil // Error indicates chain has stopped +}, 30*time.Second, time.Second)
97-98: 🛠️ Refactor suggestionAdd cleanup for Docker resources.
The test creates Docker resources but doesn't ensure they're cleaned up after the test.
Apply this diff:
ctx := context.Background() +t.Cleanup(func() { + if err := ic.Close(); err != nil { + t.Logf("Error closing interchain: %v", err) + } +})
🧹 Nitpick comments (2)
interchaintest/chain_upgrade_test.go (2)
195-196: Follow Go naming conventions for acronyms.According to Go conventions, acronyms in variable names should be capitalized.
Apply this diff:
-codeId := queryLatestCodeId(t, ctx, chain) -require.Equal(t, uint64(1), codeId) +codeID := queryLatestCodeID(t, ctx, chain) +require.Equal(t, uint64(1), codeID, "unexpected code ID value")🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 195-195: var-naming: var codeId should be codeID
(revive)
222-224: Improve error message in contract query assertion.Add a descriptive error message to help debug test failures.
Apply this diff:
-err = chain.QueryContract(ctx, contractAddr, string(queryMsgBz), &resp) -require.NoError(t, err) -require.Equal(t, 0, resp.Count) +err = chain.QueryContract(ctx, contractAddr, string(queryMsgBz), &resp) +require.NoError(t, err, "failed to query contract state") +require.Equal(t, 0, resp.Count, "unexpected initial count value")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
interchaintest/chain_upgrade_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
interchaintest/chain_upgrade_test.go
67-67: appendAssign: append result not assigned to the same slice
(gocritic)
[warning] 195-195: var-naming: var codeId should be codeID
(revive)
40-40: init$1 - result 0 (error) is always nil
(unparam)
49-49: init$2 - result 0 (error) is always nil
(unparam)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: e2e-tests (ictest-chain-upgrade)
- GitHub Check: e2e-tests (ictest-cosmwasm)
- GitHub Check: e2e-tests (ictest-group-poa)
- GitHub Check: e2e-tests (ictest-manifest)
- GitHub Check: e2e-tests (ictest-tokenfactory)
- GitHub Check: e2e-tests (ictest-poa)
- GitHub Check: e2e-tests (ictest-ibc)
- GitHub Check: build amd64 for linux
- GitHub Check: build amd64 for darwin
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/manifestd/cmd/commands.go(3 hunks)cmd/manifestd/cmd/root.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/manifestd/cmd/commands.go
8-8: File is not gci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos) -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/strangelove-ventures/poa) -s prefix(github.com/strangelove-ventures/tokenfactory) -s prefix(github.com/CosmWasm/wasmd) -s prefix(github.com/CosmWasm/wasmvm) -s prefix(github.com/liftedinit/manifest-ledger) --custom-order
(gci)
113-113: File is not gci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos) -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/strangelove-ventures/poa) -s prefix(github.com/strangelove-ventures/tokenfactory) -s prefix(github.com/CosmWasm/wasmd) -s prefix(github.com/CosmWasm/wasmvm) -s prefix(github.com/liftedinit/manifest-ledger) --custom-order
(gci)
🪛 GitHub Check: build amd64 for linux
cmd/manifestd/cmd/commands.go
[failure] 8-8:
File is not properly formatted (gci)
[failure] 113-113:
File is not properly formatted (gci)
🪛 GitHub Check: build amd64 for darwin
cmd/manifestd/cmd/commands.go
[failure] 8-8:
File is not properly formatted (gci)
[failure] 113-113:
File is not properly formatted (gci)
🪛 GitHub Actions: Build Manifest
cmd/manifestd/cmd/commands.go
[error] 8-8: File is not properly formatted (gci)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: e2e-tests (ictest-chain-upgrade)
- GitHub Check: e2e-tests (ictest-cosmwasm)
- GitHub Check: e2e-tests (ictest-group-poa)
- GitHub Check: e2e-tests (ictest-manifest)
- GitHub Check: e2e-tests (ictest-tokenfactory)
- GitHub Check: e2e-tests (ictest-poa)
- GitHub Check: e2e-tests (ictest-ibc)
- GitHub Check: Analyze
- GitHub Check: build
🔇 Additional comments (4)
cmd/manifestd/cmd/root.go (2)
130-130: Verify the minimum gas price configuration.The minimum gas price is set to "0umfx". Based on the past review comments, this value needs verification to ensure it matches the validator minimum gas price (e.g., 0.0011umfx).
123-140: LGTM! Well-structured WASM configuration.The WASM configuration is properly integrated:
- Embeds the default server config
- Includes WASM-specific node configuration
- Combines default config templates
cmd/manifestd/cmd/commands.go (2)
141-144: LGTM! Proper integration of WASM module initialization.The WASM module initialization flags are correctly added to the start command.
113-114:✅ Verification successful
Verify the impact of commented-out parameters.
The
interfaceRegistryandappCodecparameters are commented out. Please verify that these parameters are no longer needed and won't impact the functionality.
🏁 Script executed:
#!/bin/bash # Description: Check if these parameters are used in other parts of the codebase. # Search for interfaceRegistry usage echo "Searching for interfaceRegistry usage..." rg -A 5 "interfaceRegistry.*codectypes\.InterfaceRegistry" # Search for appCodec usage echo "Searching for appCodec usage..." rg -A 5 "appCodec.*codec\.Codec"Length of output: 1126
Verified Removal: No Impact from Commented-Out Parameters
The repository search confirms that the parameters
interfaceRegistry codectypes.InterfaceRegistryandappCodec codec.Codecincmd/manifestd/cmd/commands.go(lines 113-114) are no longer used in that context. Their functionality appears to be managed elsewhere in the codebase (for example,app/app.gostill utilizes them appropriately). This change seems safe as no dependent functionality is affected in this commands file.🧰 Tools
🪛 golangci-lint (1.62.2)
113-113: File is not
gci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos) -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/strangelove-ventures/poa) -s prefix(github.com/strangelove-ventures/tokenfactory) -s prefix(github.com/CosmWasm/wasmd) -s prefix(github.com/CosmWasm/wasmvm) -s prefix(github.com/liftedinit/manifest-ledger) --custom-order(gci)
🪛 GitHub Check: build amd64 for linux
[failure] 113-113:
File is not properly formatted (gci)🪛 GitHub Check: build amd64 for darwin
[failure] 113-113:
File is not properly formatted (gci)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/manifestd/cmd/root.go (1)
124-140: Consider adding validation for WASM configuration values.While the implementation looks good, consider adding validation for the WASM configuration values to ensure they meet the requirements of your application.
You could add validation like this:
func initWasmConfig() (string, interface{}) { type CustomAppConfig struct { serverconfig.Config WASM wasmtypes.NodeConfig `mapstructure:"wasm"` } srvCfg := serverconfig.DefaultConfig() + wasmConfig := wasmtypes.DefaultNodeConfig() + + // Add validation for WASM configuration + if err := wasmConfig.Validate(); err != nil { + panic(fmt.Sprintf("error in wasm config: %s", err)) + } customAppConfig := CustomAppConfig{ Config: *srvCfg, - WASM: wasmtypes.DefaultNodeConfig(), + WASM: wasmConfig, } customAppTemplate := serverconfig.DefaultConfigTemplate + wasmtypes.DefaultConfigTemplate() return customAppTemplate, customAppConfig }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/manifestd/cmd/root.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: e2e-tests (ictest-chain-upgrade)
- GitHub Check: e2e-tests (ictest-cosmwasm)
- GitHub Check: e2e-tests (ictest-group-poa)
- GitHub Check: e2e-tests (ictest-manifest)
- GitHub Check: e2e-tests (ictest-tokenfactory)
- GitHub Check: e2e-tests (ictest-poa)
- GitHub Check: e2e-tests (ictest-ibc)
- GitHub Check: build amd64 for linux
- GitHub Check: tests
- GitHub Check: build amd64 for darwin
- GitHub Check: build
🔇 Additional comments (2)
cmd/manifestd/cmd/root.go (2)
16-16: LGTM! Import changes look good.The new imports are correctly aliased and necessary for WASM integration.
Also applies to: 24-24
102-105: LGTM! Configuration initialization looks good.The WASM configuration is correctly initialized and integrated with the server's configuration interceptor.
Add cosmwasm to Manifest-ledger
Description
This PR adds the cosmwasm module to the manfiest ledger, adds cosmwasm scripts, updates simulations, add cosm wasm interchain test.
Testing
For testing locally:
test_node.shscriptupload_contract.shscriptOtherwise view the CI/Docker tests.
Breaking Changes (if applicable)
@fmorency
Checklist:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores