Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Interchain end-to-end testing: a new GitHub Actions workflow (.github/workflows/interchaintest-e2e.yml) that builds and exports a Docker image and runs a matrix of interchaintest targets; Makefile targets to run individual and aggregated interchaintest suites; a new tests/interchaintest Go module (go.mod) with extensive scaffolding (setup.go) and multiple new test files (basic, debug, ibc, ibc_rate_limit, packetforward, cosmwasm, tokenfactory) implementing various IBC, CosmWasm, tokenfactory, and packet-forwarding scenarios; updates CHANGELOG and removes the documented proto field Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In @.github/workflows/interchaintest-e2e.yml:
- Around line 48-51: The "Cleanup Go caches" step currently runs "go clean
-cache", which removes restored build caches and defeats actions/setup-go cache
benefits; remove the step(s) that run "go clean -cache" (the step named "Cleanup
Go caches") at both occurrences so the CI relies on setup-go's caching behavior
(or alternatively disable caching entirely if you prefer to avoid stale cache
issues).
- Line 19: The workflow's GO_VERSION is set to 1.23 but go.mod and the Makefile
require Go 1.24; update the GO_VERSION environment variable in the workflow (the
GO_VERSION entry) to match the module requirement (use 1.24 or the exact 1.24.11
from go.mod) so the CI installs a compatible Go version and aligns with
REQUIRE_GO_VERSION in the Makefile.
In `@tests/interchaintest/cosmwasm_test.go`:
- Around line 60-68: StdExecute currently ignores the error returned by
chain.ExecuteContract and dereferences res.Data without nil-checking; update
StdExecute to capture and require no error from chain.ExecuteContract (call
ExecuteContract and check its error) and to validate that the SmartQueryString
result contains non-nil Data before accessing res.Data.Count (e.g.,
require.NotNil/handle error if res.Data is nil when unmarshaling into
GetCountResponse), using the existing SmartQueryString/GetCountResponse symbols
and require helpers; optionally change the t parameter type to *testing.T across
helpers so require.* can be used directly in StdExecute and related functions.
In `@tests/interchaintest/go.mod`:
- Around line 13-14: The go.mod replace directive pins github.com/btcsuite/btcd
to v0.22.1 which contains consensus-critical vulnerabilities; update the replace
for the module name "github.com/btcsuite/btcd" to use v0.25.0 (or remove the
replace if unnecessary) so tests use the fixed release, and ensure any
references to the replace directive in tests/interchaintest go.mod are adjusted
accordingly.
In `@tests/interchaintest/ibc_rate_limit_test.go`:
- Around line 30-31: The test currently takes a pointer to the package-level
DefaultChainSpec (cs := &DefaultChainSpec) and then mutates cs.ModifyGenesis
which causes a data race when tests run in parallel; instead make a copy of
DefaultChainSpec by value (e.g., cs := DefaultChainSpec) and then set
cs.ModifyGenesis using cosmos.ModifyGenesis and cosmos.NewGenesisKV to avoid
mutating the shared DefaultChainSpec; update all references in this test to use
the copied cs value so the package-level DefaultChainSpec remains unchanged.
In `@tests/interchaintest/ibc_test.go`:
- Around line 65-73: This test calls ic.Build(...) to start the interchain but
doesn't register a cleanup, risking leaked resources; after the
require.NoError(t, ic.Build(...)) call add a t.Cleanup(func() { _ = ic.Close()
}) to ensure ic.Close() is invoked on test teardown (ignore the returned error),
similar to the pattern used in cosmwasm_test.go, keeping the cleanup immediately
after the interchain build so containers are always torn down.
- Around line 32-33: Don't take the address of the package-level
DefaultChainSpec (which causes a data race when modified); instead make a local
copy and mutate that. Replace "cs := &DefaultChainSpec" with a local value copy
like "cs := DefaultChainSpec" and then set "cs.ModifyGenesis =
cosmos.ModifyGenesis([]cosmos.GenesisKV{cosmos.NewGenesisKV(\"app_state.ratelimit.blacklisted_denoms\",
[]string{})})" so you modify the local cs rather than the shared
DefaultChainSpec.
In `@tests/interchaintest/packetforward_test.go`:
- Around line 70-89: The ChainSpec entries passed to
interchaintest.NewBuiltinChainFactory incorrectly reuse configA.Name for all
three chains; update the second and third ChainSpec so their Name fields
reference configB.Name and configC.Name (or assign unique names like
"kiichain-b"/"kiichain-c") instead of configA.Name so container names are
distinct; locate the ChainSpec slice around the NewBuiltinChainFactory call and
change the Name values for the entries tied to configB and configC accordingly.
In `@tests/interchaintest/setup.go`:
- Around line 133-148: ExecuteExec currently prints errors from chain.Exec and
json.Unmarshal but does not return them, allowing callers to proceed with
invalid/zero-valued results; change ExecuteExec to return (or propagate) an
error in its signature and return any error from chain.Exec and from
json.Unmarshal (with context about the command and stdout) instead of just
printing, update its declaration and all callers such as ExecuteQuery to handle
the returned error, and ensure callers propagate or fail tests when ExecuteExec
returns an error so failures aren’t silently ignored.
In `@tests/interchaintest/tokenfactory_test.go`:
- Around line 55-61: The call to node.TokenFactoryMintDenom in the "Mint TF
Denom to user" subtest ignores its returned error; update the test to capture
and assert the error from TokenFactoryMintDenom (e.g., err :=
node.TokenFactoryMintDenom(...)) and call t.Fatal or fail the test if err != nil
before proceeding to check balances so failures surface the actual minting
error; modify the code around the TokenFactoryMintDenom invocation and
subsequent balance check in the subtest to perform this error handling.
- Around line 64-71: The call to TokenFactoryMintDenomTo in the test is
discarding its error return; update the test to capture the returned error
(e.g., err := node.TokenFactoryMintDenomTo(...)) and fail the test if err != nil
(using t.Fatal or t.Fatalf) before proceeding to check balances; ensure you
reference the TokenFactoryMintDenomTo call in the "Mint TF Denom to another
user" subtest and handle its error consistently with the other tests.
🧹 Nitpick comments (14)
.github/workflows/interchaintest-e2e.yml (2)
31-34:go-cache-pathsoutputs are computed but never used.The
go-buildandgo-modoutputs from this step are not referenced by any subsequent step. Also, these run beforeactions/setup-goso they capture the runner's default Go paths rather than the configured version's paths. Consider removing these steps to reduce noise.Proposed fix (remove from both jobs)
build-docker: runs-on: ubuntu-latest steps: - - id: go-cache-paths - run: | - echo "go-build=$(go env GOCACHE)" >> $GITHUB_OUTPUT - echo "go-mod=$(go env GOMODCACHE)" >> $GITHUB_OUTPUT - - name: Checkout uses: actions/checkout@v4Also applies to: 90-93
123-124: Missing timeout on the test run step and job.E2E tests involving Docker, chain startups, and IBC relayers can hang indefinitely. Consider adding a
timeout-minuteson the job or step to prevent stuck workflows from burning CI resources.Proposed fix
interchain-tests: needs: build-docker runs-on: ubuntu-latest + timeout-minutes: 30 strategy:Makefile (1)
313-335: Consider adding explicit-timeoutflags to interchain test targets.Docker-based E2E tests can take significantly longer than the default 10-minute Go test timeout. Without an explicit timeout, tests may fail unexpectedly or hang indefinitely.
Example for one target (apply to all)
ictest-basic: `@echo` "Running basic interchain e2e test" - `@cd` tests/interchaintest && go test -race -v -run TestBasicChain . + `@cd` tests/interchaintest && go test -race -v -timeout 30m -run TestBasicChain .tests/interchaintest/tokenfactory_test.go (1)
19-19: Missing context timeout.Unlike
TestBasicChain(5 min) andTestDebugChain(10 min), this test uses a barecontext.Background()with no deadline. A hung Docker container or chain node could cause this test to run indefinitely.Proposed fix
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) + defer cancel()tests/interchaintest/debug_test.go (2)
45-46: Hardcodedtime.Sleepis fragile.
ic.Buildshould ensure the chain is ready. If a startup delay is truly needed, a polling loop onchain.Height(ctx)with a short interval would be more robust and faster on average.
15-60: This test has no corresponding Makefile target or CI matrix entry.
TestDebugChainis not referenced by anyictest-*Makefile target or the CI workflow matrix. If this is a developer-only debugging tool, consider documenting that intent. Otherwise, it won't be exercised in CI.tests/interchaintest/ibc_rate_limit_test.go (2)
78-78: Uset.Loginstead offmt.Printlnfor test output.
fmt.Printlnoutput is not associated with the test and won't be suppressed when tests pass (without-v). Uset.Log/t.Logffor proper test output integration.Proposed fix
- fmt.Println("userAInitial", userAInitial) + t.Log("userAInitial", userAInitial) require.NoError(t, err) ... - fmt.Println("aChannelID", aChannelID) + t.Log("aChannelID", aChannelID)Also applies to: 87-87
25-25: Missing context timeout.Same as the token factory test — a bare
context.Background()with no deadline in a parallel E2E test can lead to indefinite hangs.Proposed fix
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Minute) + defer cancel()You'll also need to add
"time"to the import block.tests/interchaintest/go.mod (1)
21-21: Upgrade to stable release v10.0.1 for interchaintest.The RC version v10.0.0-rc.1 can be upgraded to the stable release v10.0.1 (published Oct 28, 2025), which is now available.
tests/interchaintest/packetforward_test.go (2)
54-64: Shallow copy ofDefaultChainConfigshares slice backing arrays.
baseCfg := DefaultChainConfigis a shallow copy. TheImagesslice (and any other reference-type fields likeModifyGenesis) will share the same backing data across all three configs. This is safe here since they're not mutated after copy, but it's fragile — any future mutation of one config'sImagesslice would affect all.
219-221: UsetransferAmountconstant instead of hardcoded100000.Line 221 hardcodes
100000instead of usingtransferAmount.Int64(), which is defined on Line 165 asmath.NewInt(100_000). This is a maintenance risk if the amount changes.Proposed fix
- require.Equal(t, int64(100000), chainCBalance.Int64()) + require.Equal(t, transferAmount.Int64(), chainCBalance.Int64())tests/interchaintest/setup.go (3)
107-108: Unpinned relayer version"main"makes tests non-reproducible.Using
"main"as the relayer image tag means tests could break at any time due to upstream changes. Consider pinning to a specific release tag.
212-231:t interface{}parameter is unused inSetupContractandSmartQueryString.The
tparameter is declared asinterface{}but never used in either function. If it's meant fortesting.Tto callt.Fatal/t.Helper, type it as*testing.Tand use it instead ofpanic. If it's not needed, remove it to avoid confusion.Also applies to: 233-239
110-111: Remove unused duplicate variablesvalsandfNodes.These variables on lines 110-111 duplicate the values of
NumberValsandNumberFullNodesdefined on lines 34-35 and are never referenced anywhere in the codebase.
| # Use go clean instead of manual deletion to avoid permission errors | ||
| - name: Cleanup Go caches | ||
| run: | | ||
| go clean -cache |
There was a problem hiding this comment.
go clean -cache negates the benefit of Go build caching.
actions/setup-go with cache: true restores cached build artifacts, but immediately running go clean -cache wipes them. This forces a full rebuild every time, defeating the purpose of caching.
If the intent is to avoid stale cache issues, consider removing the cache clean step and relying on setup-go's cache key (which is based on go.sum changes). Otherwise, disable caching entirely to avoid the wasted restore time.
Proposed fix: remove the clean step
- # Use go clean instead of manual deletion to avoid permission errors
- - name: Cleanup Go caches
- run: |
- go clean -cacheAlso applies to: 107-110
🤖 Prompt for AI Agents
In @.github/workflows/interchaintest-e2e.yml around lines 48 - 51, The "Cleanup
Go caches" step currently runs "go clean -cache", which removes restored build
caches and defeats actions/setup-go cache benefits; remove the step(s) that run
"go clean -cache" (the step named "Cleanup Go caches") at both occurrences so
the CI relies on setup-go's caching behavior (or alternatively disable caching
entirely if you prefer to avoid stale cache issues).
| // Fix btcd package | ||
| replace github.com/btcsuite/btcd => github.com/btcsuite/btcd v0.22.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
btcd v0.22.1 vulnerabilities CVE GHSA latest version 2025
💡 Result:
btcd v0.22.1 — known public vulns (CVE / GHSA) impacting it
Because v0.22.1 < v0.24.0, it is affected by at least these consensus-critical issues:
-
CVE-2024-34478 (GHSA-3jgf-r68h-xfqm / GO-2024-2818)
Impact: consensus failure / potential chain split due to incorrect BIP68/BIP112 handling (tx version treated as signed vs unsigned).
Affected: btcd < 0.24.0 (so includes 0.22.1).
Fixed in: 0.24.0+.
Sources: [1], [2] -
CVE-2024-38365 (GHSA-27vh-h6mc-q6g8 / GO-2024-3189)
Impact: consensus failure / potential chain split or DoS due to mismatch vs Bitcoin Core’sFindAndDelete()behavior.
Affected: btcd 0.10 to 0.24 (so includes 0.22.1).
Fixed in: versions before v0.24.2-beta.rc1 are affected (Go vuln record), with releases pointing to v0.24.2 as the fix.
Sources: [3], [4], [5]
Latest btcd version in 2025
- btcd v0.25.0 (released Nov 4, 2025) is shown as the Latest release on GitHub. Sources: [6], [7]
If you’re on v0.22.1, upgrading to >= v0.25.0 covers both fixes above (and other security-critical fixes noted in release history). [6]
Sources
[1] https://osv.dev/vulnerability/CVE-2024-34478
[2] https://osv.dev/vulnerability/GHSA-3jgf-r68h-xfqm
[3] https://nvd.nist.gov/vuln/detail/cve-2024-38365
[4] https://pkg.go.dev/vuln/GO-2024-3189
[5] https://advisories.gitlab.com/pkg/golang/github.com/btcsuite/btcd/CVE-2024-38365/
[6] https://github.com/btcsuite/btcd/releases/tag/v0.25.0
[7] https://github.com/btcsuite/btcd/discussions
🌐 Web query:
github.com/btcsuite/btcd latest version releases
💡 Result:
The latest btcsuite/btcd release on GitHub is v0.25.0 (marked “Latest”; published Nov 4, 2024). [1]
The most recent previous stable release is v0.24.2 (published Jun 25, 2024), which includes security-critical fixes (also referenced in security advisories). [1][2]
There is also a pre-release: v0.25.0-beta.rc1. [1]
🌐 Web query:
GHSA-27vh-h6mc-q6g8 btcd consensus failure vulnerability
💡 Result:
GHSA-27vh-h6mc-q6g8 (CVE-2024-38365 / GO-2024-3189) — btcd consensus failure
- What it is: A consensus-critical validation bug in
github.com/btcsuite/btcdcaused by an incorrect re-implementation of Bitcoin Core’sFindAndDelete()behavior in btcd’s script handling. This can make btcd compute a different signature hash than other nodes for certain transactions. [1][2] - Impact: Can be exploited (via a “standard” transaction relayed over the network) to make btcd nodes accept an invalid block (chain split) or reject a valid block (DoS)—no hash power required. [1][2]
- Root cause (high level): btcd’s
removeOpcodeByData()removed data pushes that contain the target bytes, while Bitcoin Core’sFindAndDelete()removes only exact matches, creating validation discrepancies (notably aroundOP_CODESEPARATOR-related behavior). [1] - Affected versions: btcd < v0.24.2 (advisory describes btcd versions 0.10 through 0.24 as affected). [1][2]
- Fixed in: btcd v0.24.2 (patched). [1][2]
- Severity: CVSS v3.1 7.4 (High). [3]
Recommended action: Upgrade all btcd-based nodes/services to v0.24.2 or later and redeploy any systems embedding the affected btcd/txscript logic. [1][2]
Sources: GitHub Security Advisory (GHSA-27vh-h6mc-q6g8) [1], Go vulnerability report (GO-2024-3189) [2], OSV entry for CVE-2024-38365 (CVSS vector/score) [3].
🏁 Script executed:
grep -r "go-schnorrkel" tests/interchaintest/go.mod -A 2 -B 2Repository: KiiChain/kiichain
Length of output: 284
🌐 Web query:
go-schnorrkel github.com/btcsuite/btcd version compatibility constraints
💡 Result:
github.com/ChainSafe/go-schnorrkel does not depend on (or import) github.com/btcsuite/btcd, so there are no direct version-compatibility constraints between the two modules. go-schnorrkel’s non-stdlib imports are github.com/cosmos/go-bip39, github.com/gtank/merlin, github.com/gtank/ristretto255, and golang.org/x/crypto/pbkdf2—not btcd. [1]
If you see them “constrained” together in a project, that constraint is coming from your (or an upstream project’s) go.mod, not from go-schnorrkel—for example, Tendermint pins both github.com/ChainSafe/go-schnorrkel and github.com/btcsuite/btcd v0.22.1. [2]
Practical things to watch for when using both in one build:
- Go toolchain version: btcd currently requires Go 1.22+. [3] (go-schnorrkel itself is fine with older Go versions, but btcd will force the minimum.)
- btcd submodules: btcd has separate Go modules such as
github.com/btcsuite/btcd/btcec/v2with their own versions (e.g.,v2.3.x). [4] So you should manage btcd per-module (btcd vs btcec/v2) rather than expecting go-schnorrkel to “align” anything.
Sources:
[1] go-schnorrkel imports list (no btcd) (pkg.go.dev)
[2] Example go.mod using both (Tendermint) (cocalc.com)
[3] btcd README (Go 1.22+ requirement) (github.com)
[4] btcec/v2 module/versioning on pkg.go.dev (pkg.go.dev)
Citations:
- 1: https://pkg.go.dev/github.com/ChainSafe/go-schnorrkel?tab=imports
- 2: https://cocalc.com/github/tendermint/tendermint/blob/main/go.mod?utm_source=openai
- 3: https://github.com/btcsuite/btcd?utm_source=openai
- 4: https://pkg.go.dev/github.com/btcsuite/btcd/btcec/v2?utm_source=openai
Pinned btcd v0.22.1 has consensus-critical vulnerabilities with no justifying dependency constraint.
The replace directive forces btcd to v0.22.1, which is affected by at least two consensus-critical vulnerabilities:
- CVE-2024-34478 (GHSA-3jgf-r68h-xfqm): incorrect BIP68/BIP112 handling causing potential chain splits; fixed in v0.24.0+
- CVE-2024-38365 (GHSA-27vh-h6mc-q6g8): mismatch in
FindAndDelete()behavior causing chain splits or DoS (CVSS 7.4); fixed in v0.24.2+
go-schnorrkel does not import btcd, so there is no dependency constraint justifying this pin. Upgrade to v0.25.0 (the current stable release).
🤖 Prompt for AI Agents
In `@tests/interchaintest/go.mod` around lines 13 - 14, The go.mod replace
directive pins github.com/btcsuite/btcd to v0.22.1 which contains
consensus-critical vulnerabilities; update the replace for the module name
"github.com/btcsuite/btcd" to use v0.25.0 (or remove the replace if unnecessary)
so tests use the fixed release, and ensure any references to the replace
directive in tests/interchaintest go.mod are adjusted accordingly.
| func ExecuteExec(ctx context.Context, chain *cosmos.CosmosChain, cmd []string, i interface{}, extraFlags ...string) { | ||
| command := []string{chain.Config().Bin} | ||
| command = append(command, cmd...) | ||
| command = append(command, extraFlags...) | ||
| fmt.Println(command) | ||
|
|
||
| stdout, _, err := chain.Exec(ctx, command, nil) | ||
| if err != nil { | ||
| fmt.Println(err) | ||
| } | ||
|
|
||
| fmt.Println(string(stdout)) | ||
| if err := json.Unmarshal(stdout, &i); err != nil { | ||
| fmt.Println(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
ExecuteExec silently swallows errors — tests may pass with garbage data.
Both the Exec error (Line 140) and the json.Unmarshal error (Line 145) are only printed to stdout, not propagated. If the command fails, stdout may be empty or contain an error message, and i will remain zero-valued. Callers will silently get a zero-value result, leading to confusing test failures (or worse, false passes).
Proposed fix
-func ExecuteExec(ctx context.Context, chain *cosmos.CosmosChain, cmd []string, i interface{}, extraFlags ...string) {
+func ExecuteExec(ctx context.Context, chain *cosmos.CosmosChain, cmd []string, i interface{}, extraFlags ...string) error {
command := []string{chain.Config().Bin}
command = append(command, cmd...)
command = append(command, extraFlags...)
- fmt.Println(command)
stdout, _, err := chain.Exec(ctx, command, nil)
if err != nil {
- fmt.Println(err)
+ return fmt.Errorf("exec failed: %w", err)
}
- fmt.Println(string(stdout))
if err := json.Unmarshal(stdout, &i); err != nil {
- fmt.Println(err)
+ return fmt.Errorf("unmarshal failed: %w", err)
}
+ return nil
}This would require updating ExecuteQuery and other callers to propagate the error as well.
📝 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 ExecuteExec(ctx context.Context, chain *cosmos.CosmosChain, cmd []string, i interface{}, extraFlags ...string) { | |
| command := []string{chain.Config().Bin} | |
| command = append(command, cmd...) | |
| command = append(command, extraFlags...) | |
| fmt.Println(command) | |
| stdout, _, err := chain.Exec(ctx, command, nil) | |
| if err != nil { | |
| fmt.Println(err) | |
| } | |
| fmt.Println(string(stdout)) | |
| if err := json.Unmarshal(stdout, &i); err != nil { | |
| fmt.Println(err) | |
| } | |
| } | |
| func ExecuteExec(ctx context.Context, chain *cosmos.CosmosChain, cmd []string, i interface{}, extraFlags ...string) error { | |
| command := []string{chain.Config().Bin} | |
| command = append(command, cmd...) | |
| command = append(command, extraFlags...) | |
| stdout, _, err := chain.Exec(ctx, command, nil) | |
| if err != nil { | |
| return fmt.Errorf("exec failed: %w", err) | |
| } | |
| if err := json.Unmarshal(stdout, &i); err != nil { | |
| return fmt.Errorf("unmarshal failed: %w", err) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@tests/interchaintest/setup.go` around lines 133 - 148, ExecuteExec currently
prints errors from chain.Exec and json.Unmarshal but does not return them,
allowing callers to proceed with invalid/zero-valued results; change ExecuteExec
to return (or propagate) an error in its signature and return any error from
chain.Exec and from json.Unmarshal (with context about the command and stdout)
instead of just printing, update its declaration and all callers such as
ExecuteQuery to handle the returned error, and ensure callers propagate or fail
tests when ExecuteExec returns an error so failures aren’t silently ignored.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/interchaintest/setup.go`:
- Around line 122-130: ExecuteQuery currently calls ExecuteExec which swallows
errors, so update ExecuteQuery to propagate failures: change its signature to
accept a testing.TB (or return an error) and then either return the error from
ExecuteExec or call t.Fatalf on error; specifically modify func ExecuteQuery(ctx
context.Context, chain *cosmos.CosmosChain, cmd []string, i interface{},
extraFlags ...string) to include t testing.TB (or to return error), build flags
as before, call ExecuteExec and handle its returned error (or propagate it)
instead of ignoring it so callers no longer silently succeed.
- Around line 186-198: The gas flag detection in the loop over extraFlags only
checks for exact "--gas" and misses the "--gas=VALUE" form, causing duplicate
flags; update the loop in tests/interchaintest/setup.go to treat a flag as
present when flag == "--gas" OR strings.HasPrefix(flag, "--gas="), import the
"strings" package, and keep the existing behavior that appends "--gas","500000"
only when neither form is found before appending extraFlags to command.
🧹 Nitpick comments (2)
tests/interchaintest/setup.go (2)
211-239:tparameter is unused in bothSetupContractandSmartQueryString;panicgives poor test diagnostics.Both functions accept
t interface{}but never reference it. Meanwhile, errors are handled withpanic()which bypasses test cleanup and produces unhelpful stack traces. Either removetor type it astesting.TBand uset.Fatal():Suggested approach (using testing.TB)
-func SetupContract(t interface{}, ctx context.Context, chain *cosmos.CosmosChain, keyname string, fileLoc string, message string, extraFlags ...string) (codeId, contract string) { +func SetupContract(t testing.TB, ctx context.Context, chain *cosmos.CosmosChain, keyname string, fileLoc string, message string, extraFlags ...string) (codeId, contract string) { + t.Helper() codeId, err := chain.StoreContract(ctx, keyname, fileLoc) if err != nil { - panic(err) + t.Fatal(err) } // ... contractAddr, err := chain.InstantiateContract(ctx, keyname, codeId, message, needsNoAdminFlag, extraFlags...) if err != nil { - panic(err) + t.Fatal(err) } return codeId, contractAddr } -func SmartQueryString(t interface{}, ctx context.Context, chain *cosmos.CosmosChain, contractAddr, queryMsg string, res interface{}) error { +func SmartQueryString(t testing.TB, ctx context.Context, chain *cosmos.CosmosChain, contractAddr, queryMsg string, res interface{}) error { + t.Helper() var jsonMap map[string]interface{} if err := json.Unmarshal([]byte(queryMsg), &jsonMap); err != nil { - panic(err) + t.Fatal(err) }This requires updating callers to pass a
*testing.Tinstead of an untyped value. Thetesting.TBinterface covers both*testing.Tand*testing.B.
109-110: Remove unusedvalsandfNodesconstants—they duplicateNumberValsandNumberFullNodes.Lines 109–110 declare
vals = 1andfNodes = 0, which are never referenced anywhere in the codebase. Lines 34–35 already declareNumberVals = 1andNumberFullNodes = 0with the same values, and these are actively used (see lines 90–91). Delete the unusedvalsandfNodesdeclarations to avoid confusion.
| func ExecuteQuery(ctx context.Context, chain *cosmos.CosmosChain, cmd []string, i interface{}, extraFlags ...string) { | ||
| flags := []string{ | ||
| "--node", chain.GetRPCAddress(), | ||
| "--output=json", | ||
| } | ||
| flags = append(flags, extraFlags...) | ||
|
|
||
| ExecuteExec(ctx, chain, cmd, i, flags...) | ||
| } |
There was a problem hiding this comment.
ExecuteQuery inherits the silent-error problem from ExecuteExec.
Since ExecuteExec swallows errors, ExecuteQuery also silently succeeds on failure. Once ExecuteExec is fixed to return an error, this function should propagate it (or call t.Fatal). Note that the t parameter is available in callers but not passed here — consider adding a testing.TB parameter for fail-fast behavior.
🤖 Prompt for AI Agents
In `@tests/interchaintest/setup.go` around lines 122 - 130, ExecuteQuery currently
calls ExecuteExec which swallows errors, so update ExecuteQuery to propagate
failures: change its signature to accept a testing.TB (or return an error) and
then either return the error from ExecuteExec or call t.Fatalf on error;
specifically modify func ExecuteQuery(ctx context.Context, chain
*cosmos.CosmosChain, cmd []string, i interface{}, extraFlags ...string) to
include t testing.TB (or to return error), build flags as before, call
ExecuteExec and handle its returned error (or propagate it) instead of ignoring
it so callers no longer silently succeed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/interchaintest/packetforward_test.go`:
- Around line 219-221: The test is calling .Int64() on GenesisFundsAmount which
overflows int64; replace the int64 comparisons with big.Int comparisons so the
high-precision value is checked correctly: compute expected :=
new(big.Int).Sub(GenesisFundsAmount, transferAmount) and assert
expected.Cmp(chainABalance) == 0 (or use expected.Equal(chainABalance) with
require.True/require.Equal from the test), while leaving the small-value
assertions for chainBBalance and chainCBalance unchanged; reference the symbols
GenesisFundsAmount, transferAmount, chainABalance, chainBBalance, and
chainCBalance to locate the change.
🧹 Nitpick comments (6)
tests/interchaintest/setup.go (4)
217-236:SetupContract: unusedtparameter andpanicinstead of test failure.The
t interface{}parameter is never used. Meanwhile, errors on Lines 220 and 232 callpanic(err)instead of failing the test properly. Either passt *testing.T(ortesting.TB) and uset.Fatal(err), or removetfrom the signature and document that panics are intentional.Proposed fix
-func SetupContract(t interface{}, ctx context.Context, chain *cosmos.CosmosChain, keyname string, fileLoc string, message string, extraFlags ...string) (codeId, contract string) { +func SetupContract(t testing.TB, ctx context.Context, chain *cosmos.CosmosChain, keyname string, fileLoc string, message string, extraFlags ...string) (codeId, contract string) { codeId, err := chain.StoreContract(ctx, keyname, fileLoc) if err != nil { - panic(err) + t.Fatal(err) } needsNoAdminFlag := true @@ contractAddr, err := chain.InstantiateContract(ctx, keyname, codeId, message, needsNoAdminFlag, extraFlags...) if err != nil { - panic(err) + t.Fatal(err) } return codeId, contractAddr }
238-244:SmartQueryString: same unusedt interface{}issue.Same pattern as
SetupContract—tis declared but unused. Either type it astesting.TBand use it for failure reporting, or drop it from the signature.
155-175:ExecuteTransactiondoesn't check theTxResponse.Codefor non-zero (application-level) errors.After unmarshalling the
TxResponse, the function returnsnilerror even ifres.Code != 0(which indicates the transaction was rejected by the chain). Callers would need to manually inspect the code, which is easy to forget in tests.Proposed enhancement
var res sdk.TxResponse if err := json.Unmarshal(stdout, &res); err != nil { return res, err } + if res.Code != 0 { + return res, fmt.Errorf("transaction failed with code %d: %s", res.Code, res.RawLog) + } return res, err
115-116: Remove unused duplicate variablesvalsandfNodes.Lines 115–116 declare
valsandfNodeswith the same values already defined asNumberValsandNumberFullNodeson lines 35–36. These duplicates are unused throughout the codebase and should be removed to reduce confusion.tests/interchaintest/packetforward_test.go (2)
70-89: All three chain specs share the sameName("kiichain").
configA.Name,configB.Name, andconfigC.Nameall resolve to"kiichain"sincebaseCfg.Nameis never changed. If the interchaintest framework uses theChainSpec.Namefor Docker container naming or resource identification, this could cause collisions. Consider assigning distinct names for each chain.
171-175:ParseDenomTraceandIBCDenom()are deprecated in ibc-go v10—useExtractDenomFromPath()instead.
transfertypes.ParseDenomTrace()andDenomTrace.IBCDenom()are deprecated in ibc-go v10. Replace withExtractDenomFromPath()to parse full denom paths into theDenomtype, which provides access to the IBC denom via theIBCDenom()method.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/interchaintest/setup.go`:
- Around line 100-106: SecondDefaultChainSpec copies DefaultChainSpec but only
mutates spec-level fields, leaving the embedded ChainConfig.ChainID unchanged so
both chains share the same ID; update the copied spec to set
SecondChainSpec.ChainConfig.ChainID to a distinct value (e.g., derive from
DefaultChainSpec.ChainConfig.ChainID + "2" or construct a new meaningful ID) and
also ensure SecondChainSpec.ChainID is initialized to the intended second-chain
identifier so tests that call chain.Config().ChainID see the new value.
- Around line 207-215: getTransferChannel is comparing channel state using the
connection enum (ibcconntypes.OPEN.String()); replace that with the channel enum
by importing ibcchanneltypes and change the comparison to use
ibcchanneltypes.OPEN.String() inside getTransferChannel so the PortID ==
"transfer" check uses the proper channel state enum.
🧹 Nitpick comments (2)
tests/interchaintest/setup.go (2)
217-236:tparameter is unused and errorspanicinstead of failing the test.
SetupContractacceptst interface{}but never references it. The panics on Lines 220 and 232 will crash the entire test process rather than producing a clean test failure. Consider typingtastesting.TBand callingt.Fatal(err)instead, or removingtentirely if you prefer to keep panics.Proposed fix using testing.TB
-func SetupContract(t interface{}, ctx context.Context, chain *cosmos.CosmosChain, keyname string, fileLoc string, message string, extraFlags ...string) (codeId, contract string) { +func SetupContract(t testing.TB, ctx context.Context, chain *cosmos.CosmosChain, keyname string, fileLoc string, message string, extraFlags ...string) (codeId, contract string) { + t.Helper() codeId, err := chain.StoreContract(ctx, keyname, fileLoc) if err != nil { - panic(err) + t.Fatal(err) } needsNoAdminFlag := true @@ contractAddr, err := chain.InstantiateContract(ctx, keyname, codeId, message, needsNoAdminFlag, extraFlags...) if err != nil { - panic(err) + t.Fatal(err) }
238-244: Same unusedtparameter; inconsistent error handling —panicon unmarshal butreturnon query error.
SmartQueryStringpanics if the query message JSON is malformed (Line 241) but returns the error fromQueryContract(Line 243). Consider making error handling consistent (either always return errors or always uset.Fatalwith a properly typedtesting.TB).
| SecondDefaultChainSpec = func() interchaintest.ChainSpec { | ||
| SecondChainSpec := DefaultChainSpec | ||
| SecondChainSpec.ChainID += "2" | ||
| SecondChainSpec.Name += "2" | ||
| SecondChainSpec.ChainName += "2" | ||
| return SecondChainSpec | ||
| }() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "SecondDefaultChainSpec" --type=go -C 3Repository: KiiChain/kiichain
Length of output: 1424
🏁 Script executed:
rg -n "DefaultChainSpec\|ChainConfig.*ChainID" tests/interchaintest/setup.go -C 2Repository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
rg -A 20 "type ChainSpec struct" --type=goRepository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
head -n 30 tests/interchaintest/setup.go | cat -nRepository: KiiChain/kiichain
Length of output: 1054
🏁 Script executed:
sed -n '80,120p' tests/interchaintest/setup.go | cat -nRepository: KiiChain/kiichain
Length of output: 1710
🏁 Script executed:
rg "ChainConfig\|ChainID" tests/interchaintest/setup.goRepository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
rg "interchaintest" tests/interchaintest/setup.go | head -20Repository: KiiChain/kiichain
Length of output: 375
🏁 Script executed:
rg "type ChainSpec" --type go -B 2 -A 30Repository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Check if there are any test failures or if this compiles
find . -name "*.go" -type f -exec grep -l "ChainSpec" {} \; | head -5Repository: KiiChain/kiichain
Length of output: 383
🏁 Script executed:
# Look at what variables are defined before DefaultChainSpec
sed -n '24,90p' tests/interchaintest/setup.go | cat -nRepository: KiiChain/kiichain
Length of output: 3713
🏁 Script executed:
# Check if interchaintest is imported and where ChainSpec comes from
grep -n "import\|ChainSpec\|ChainConfig" tests/interchaintest/setup.go | head -30Repository: KiiChain/kiichain
Length of output: 447
🏁 Script executed:
sed -n '30,50p' tests/interchaintest/ibc_test.go | cat -nRepository: KiiChain/kiichain
Length of output: 1017
🏁 Script executed:
grep -n "ChainID\|chain\|Chain" tests/interchaintest/ibc_test.go | head -20Repository: KiiChain/kiichain
Length of output: 1115
🏁 Script executed:
# Check if there are references to chain IDs in the actual test code
rg "localchain_1010" tests/interchaintest/ -C 2Repository: KiiChain/kiichain
Length of output: 345
🏁 Script executed:
# Look at the actual interchaintest.ChainSpec definition by searching in go.mod/go.sum
cat go.mod | grep interchaintestRepository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Check go.mod for interchaintest version
grep "interchaintest" go.modRepository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Let's see what happens when DefaultChainSpec.ChainID is not set
# It defaults to empty string, so SecondChainSpec.ChainID += "2" becomes "2"
# Let's verify if ChainConfig.ChainID is actually used
rg "\.ChainID" tests/interchaintest/ --type=go -B 2 -A 2Repository: KiiChain/kiichain
Length of output: 2991
🏁 Script executed:
# Check if there's any initialization or post-processing of ChainSpec
rg "ChainSpec.*ChainID\|ChainID.*ChainSpec" tests/interchaintest/ --type=goRepository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Look for any place where chain identity/names are verified in tests
rg "ChainID\|ChainName\|chainID" tests/interchaintest/ibc_test.goRepository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Check if tests actually run and pass - look for test execution results
find . -name "*.log" -o -name "*test*.txt" 2>/dev/null | headRepository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Look at the DefaultChainConfig to see what ChainID it has
sed -n '42,66p' tests/interchaintest/setup.go | cat -nRepository: KiiChain/kiichain
Length of output: 1689
🏁 Script executed:
# Check if there's a ChainID field that gets initialized somewhere
rg "ChainID\s*:" tests/interchaintest/setup.goRepository: KiiChain/kiichain
Length of output: 87
🏁 Script executed:
# Check the actual ibc.ChainConfig structure - search for how it's used
rg "ibc\.ChainConfig\|ChainConfig{" tests/interchaintest/ -A 5Repository: KiiChain/kiichain
Length of output: 184
🏁 Script executed:
# Let's see how the second chain actually gets its chain ID
# Look at packetforward_test to see how they set ChainID for different chains
sed -n '1,50p' tests/interchaintest/packetforward_test.go | cat -nRepository: KiiChain/kiichain
Length of output: 1847
🏁 Script executed:
# Check the full context of how chainID is set in packetforward_test.go
rg "chainID_A\|chainID_B\|ChainSpec" tests/interchaintest/packetforward_test.go -B 2 -A 2Repository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Search for how ChainSpec fields are actually used/documented
rg "type ChainSpec struct" . --type=goRepository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Check if the code imports ChainSpec from external library
grep -rn "import.*ChainSpec\|from.*ChainSpec" tests/ || echo "Not found in imports"Repository: KiiChain/kiichain
Length of output: 81
🏁 Script executed:
# Let's look for documentation or comments about what ChainID field on ChainSpec does
rg "ChainSpec" tests/interchaintest/setup.go -B 1 -A 1Repository: KiiChain/kiichain
Length of output: 360
🏁 Script executed:
# Let's look at the actual chain operations to see if there are any chain ID conflicts
rg "localchain_1010" tests/interchaintest/ -B 1 -A 1Repository: KiiChain/kiichain
Length of output: 241
🏁 Script executed:
# Check how the chains are actually instantiated and if ChainID from ChainSpec vs ChainConfig matters
sed -n '90,110p' tests/interchaintest/setup.go | cat -nRepository: KiiChain/kiichain
Length of output: 1081
🏁 Script executed:
# Let's check if there are any test results or CI logs that might indicate failures
find . -type f \( -name "*.yaml" -o -name "*.yml" \) | grep -E "action|workflow|test" | head -5Repository: KiiChain/kiichain
Length of output: 228
🏁 Script executed:
# Try to understand: does interchaintest.ChainSpec use ChainID field or is it just for display?
# Check what fields are actually initialized
python3 << 'EOF'
import re
with open('tests/interchaintest/setup.go', 'r') as f:
content = f.read()
# Find DefaultChainSpec initialization
match = re.search(r'DefaultChainSpec = interchaintest\.ChainSpec\{([^}]+)\}', content, re.DOTALL)
if match:
spec_init = match.group(1)
fields = re.findall(r'(\w+)\s*:', spec_init)
print("DefaultChainSpec fields initialized:")
for field in fields:
print(f" - {field}")
print(f"\nChainID field present: {'ChainID' in fields}")
else:
print("Could not find DefaultChainSpec")
# Find DefaultChainConfig initialization
match = re.search(r'DefaultChainConfig = ibc\.ChainConfig\{([^}]+)\}', content, re.DOTALL)
if match:
config_init = match.group(1)
fields = re.findall(r'(\w+)\s*:', config_init)
print("\nDefaultChainConfig fields initialized:")
for field in fields:
print(f" - {field}")
print(f"\nChainID field present: {'ChainID' in fields}")
EOFRepository: KiiChain/kiichain
Length of output: 294
🏁 Script executed:
# Let's verify - DefaultChainConfig DOES have ChainID set
sed -n '42,66p' tests/interchaintest/setup.go | grep -n "ChainID"Repository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# So the issue is: SecondChainSpec doesn't update ChainConfig.ChainID
# Let's check if tests actually run and if there are issues
cat .github/workflows/interchaintest-e2e.ymlRepository: KiiChain/kiichain
Length of output: 3062
🏁 Script executed:
# Check if there's a way ChainID gets synchronized or if it's a real issue
rg "ChainID\s*=" tests/interchaintest/setup.goRepository: KiiChain/kiichain
Length of output: 96
🏁 Script executed:
# Let's understand the actual issue better - is ChainSpec.ChainID a separate field from ChainConfig.ChainID?
# Check if there's any synchronization between the two
rg "ChainID" tests/interchaintest/ibc_test.go -B 2 -A 2Repository: KiiChain/kiichain
Length of output: 378
🏁 Script executed:
# Check how the framework actually uses ChainSpec
# Look for any place where the ChainID fields might be synchronized or used
rg "Config\(\)\.ChainID\|\.ChainID" tests/interchaintest/ -lRepository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Let's check if there's any DeepCopy or similar mechanism being used
rg "DeepCopy\|Copy\|clone" tests/interchaintest/setup.go -iRepository: KiiChain/kiichain
Length of output: 43
🏁 Script executed:
# Most importantly: do the tests actually pass? Let's check test history or recent commits
git log --oneline tests/interchaintest/setup.go | head -5Repository: KiiChain/kiichain
Length of output: 98
🏁 Script executed:
# Check the recent commit that fixed chain ID setup
git show 518366f --statRepository: KiiChain/kiichain
Length of output: 28062
🏁 Script executed:
# See what changed in that commit for setup.go
git show 518366f -- tests/interchaintest/setup.goRepository: KiiChain/kiichain
Length of output: 9276
🏁 Script executed:
# Check if the current code in setup.go matches the fix or is still broken
git log -1 --format=%H tests/interchaintest/setup.goRepository: KiiChain/kiichain
Length of output: 101
SecondDefaultChainSpec doesn't update ChainConfig.ChainID — second chain will incorrectly reuse the first chain's ID.
When SecondDefaultChainSpec copies DefaultChainSpec and modifies only the spec-level fields (lines 102-104), the underlying ChainConfig remains unchanged. Since tests resolve chain identity via chain.Config().ChainID (which returns ChainConfig.ChainID), both chains will have the same ID ("localchain_1010-1"). Additionally, SecondChainSpec.ChainID += "2" results in "2" rather than a meaningful second chain ID, since ChainID is not initialized in DefaultChainSpec.
Update ChainConfig.ChainID to differentiate the second chain:
Proposed fix
SecondDefaultChainSpec = func() interchaintest.ChainSpec {
SecondChainSpec := DefaultChainSpec
SecondChainSpec.ChainID += "2"
SecondChainSpec.Name += "2"
SecondChainSpec.ChainName += "2"
+ cfg := SecondChainSpec.ChainConfig
+ cfg.ChainID = SecondChainSpec.ChainID
+ SecondChainSpec.ChainConfig = cfg
return SecondChainSpec
}()📝 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.
| SecondDefaultChainSpec = func() interchaintest.ChainSpec { | |
| SecondChainSpec := DefaultChainSpec | |
| SecondChainSpec.ChainID += "2" | |
| SecondChainSpec.Name += "2" | |
| SecondChainSpec.ChainName += "2" | |
| return SecondChainSpec | |
| }() | |
| SecondDefaultChainSpec = func() interchaintest.ChainSpec { | |
| SecondChainSpec := DefaultChainSpec | |
| SecondChainSpec.ChainID += "2" | |
| SecondChainSpec.Name += "2" | |
| SecondChainSpec.ChainName += "2" | |
| cfg := SecondChainSpec.ChainConfig | |
| cfg.ChainID = SecondChainSpec.ChainID | |
| SecondChainSpec.ChainConfig = cfg | |
| return SecondChainSpec | |
| }() |
🤖 Prompt for AI Agents
In `@tests/interchaintest/setup.go` around lines 100 - 106, SecondDefaultChainSpec
copies DefaultChainSpec but only mutates spec-level fields, leaving the embedded
ChainConfig.ChainID unchanged so both chains share the same ID; update the
copied spec to set SecondChainSpec.ChainConfig.ChainID to a distinct value
(e.g., derive from DefaultChainSpec.ChainConfig.ChainID + "2" or construct a new
meaningful ID) and also ensure SecondChainSpec.ChainID is initialized to the
intended second-chain identifier so tests that call chain.Config().ChainID see
the new value.
| func getTransferChannel(channels []ibc.ChannelOutput) (string, error) { | ||
| for _, channel := range channels { | ||
| if channel.PortID == "transfer" && channel.State == ibcconntypes.OPEN.String() { | ||
| return channel.ChannelID, nil | ||
| } | ||
| } | ||
|
|
||
| return "", fmt.Errorf("no open transfer channel found: %+v", channels) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and check the file
find . -type f -name "setup.go" | grep interchaintest
# Check if the file exists and view the relevant section
if [ -f "tests/interchaintest/setup.go" ]; then
echo "=== File found, checking imports and function ==="
head -30 tests/interchaintest/setup.go | grep -E "^import|ibcconntypes|channeltypes"
echo ""
echo "=== Function getTransferChannel ==="
sed -n '207,215p' tests/interchaintest/setup.go
fiRepository: KiiChain/kiichain
Length of output: 572
🌐 Web query:
ibc-go v10 modules core 03-connection types OPEN constant
💡 Result:
In ibc-go v10, modules/core/03-connection/types, OPEN is the connection state enum value:
OPEN types.State = 3(withUNINITIALIZED=0,INIT=1,TRYOPEN=2). [1]- It’s used when constructing a
ConnectionEndto indicate the connection handshake has completed (e.g.,types.NewConnectionEnd(types.OPEN, ...)). [2]
Refs
github.com/cosmos/ibc-go/v10/modules/core/03-connection/types(pkg.go.dev) —type State int32constants includeOPEN State = 3. [1]- IBC-Go docs example showing
types.NewConnectionEnd(types.OPEN, ...). [2]
🌐 Web query:
ibc-go v10 modules core 04-channel types OPEN constant string representation
💡 Result:
In ibc-go v10, the channel State enum constant OPEN (value 3) stringifies to:
"STATE_OPEN"
This is because the generated proto maps 3 -> "STATE_OPEN" in State_name, and func (x State) String() returns proto.EnumName(State_name, int32(x)). [1]
Source (v10.5.0):
github.com/cosmos/ibc-go/v10/modules/core/04-channel/migrations/v10/channel.pb.go(State_name+String()implementation). [1]
References
Citations:
🌐 Web query:
ibc-go v10 connection types OPEN State String() method return value
💡 Result:
In ibc-go v10 (github.com/cosmos/ibc-go/v10/modules/core/03-connection/types), the State enum’s String() for the OPEN value returns:
types.OPEN.String()→"STATE_OPEN"(becauseOPENis3andState_name[3] == "STATE_OPEN"). [1]
Source: [1] (pkg.go.dev)
Citations:
🌐 Web query:
ibc-go v10 modules core 03-connection types State_name enum proto mapping
💡 Result:
In ibc-go v10 (modules/core/03-connection/types), the Go enum State values map to the protobuf enum name strings (State_name) like this:
Go State constant |
int32 | Protobuf enum name (State_name[int32]) |
|---|---|---|
UNINITIALIZED |
0 | STATE_UNINITIALIZED_UNSPECIFIED |
INIT |
1 | STATE_INIT |
TRYOPEN |
2 | STATE_TRYOPEN |
OPEN |
3 | STATE_OPEN |
The reverse map (State_value) is the exact inverse (string → int32). [1][2]
Channel state compared using connection types (03-connection) instead of channel types (04-channel).
The code uses ibcconntypes.OPEN.String() to compare channel state. While both connection types and channel types define OPEN = 3 and currently stringify to "STATE_OPEN" (so the code works today), using connection types for a channel state enum is semantically incorrect. This creates a maintenance risk: if ibc-go ever changes the string mappings differently for these two enum types, the comparison will break.
Proposed fix
Replace the import:
- ibcconntypes "github.com/cosmos/ibc-go/v10/modules/core/03-connection/types"
+ channeltypes "github.com/cosmos/ibc-go/v10/modules/core/04-channel/types"And update the usage:
- if channel.PortID == "transfer" && channel.State == ibcconntypes.OPEN.String() {
+ if channel.PortID == "transfer" && channel.State == channeltypes.OPEN.String() {🤖 Prompt for AI Agents
In `@tests/interchaintest/setup.go` around lines 207 - 215, getTransferChannel is
comparing channel state using the connection enum (ibcconntypes.OPEN.String());
replace that with the channel enum by importing ibcchanneltypes and change the
comparison to use ibcchanneltypes.OPEN.String() inside getTransferChannel so the
PortID == "transfer" check uses the proper channel state enum.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/interchaintest/setup.go`:
- Around line 121-123: Remove the duplicate, unused constant declarations vals
and fNodes: locate the block where vals = 1 and fNodes = 0 are defined and
delete those two constants; keep and use the existing canonical constants
NumberVals and NumberFullNodes (already defined) so there are no duplicate
definitions or unused variables remaining.
🧹 Nitpick comments (3)
tests/interchaintest/setup.go (3)
223-242:t interface{}parameter is unused — usetesting.TBand replacepanicwitht.Fatal.The
tparameter is declared but never referenced. Meanwhile, the function usespanic(err)for error handling, which produces poor test failure output (stack traces instead of clean test failures). Accepttesting.TBand callt.Fatal(err)for idiomatic test helpers.Proposed fix
-func SetupContract(t interface{}, ctx context.Context, chain *cosmos.CosmosChain, keyname string, fileLoc string, message string, extraFlags ...string) (codeId, contract string) { +func SetupContract(t testing.TB, ctx context.Context, chain *cosmos.CosmosChain, keyname string, fileLoc string, message string, extraFlags ...string) (codeId, contract string) { + t.Helper() codeId, err := chain.StoreContract(ctx, keyname, fileLoc) if err != nil { - panic(err) + t.Fatal(err) } needsNoAdminFlag := true for _, flag := range extraFlags { if flag == "--admin" { needsNoAdminFlag = false } } contractAddr, err := chain.InstantiateContract(ctx, keyname, codeId, message, needsNoAdminFlag, extraFlags...) if err != nil { - panic(err) + t.Fatal(err) } return codeId, contractAddr }This requires adding
"testing"to imports and updating callers to pass a*testing.Tinstead of an unused value.
244-251: Samet interface{}unused parameter; inconsistent error handling.
tis unused (same issue asSetupContract). Error handling is inconsistent:json.Unmarshalfailure panics whileQueryContractfailure returns an error. Consider acceptingtesting.TBand usingt.Fatalfor the parse error, or returning an error for both.Proposed fix (option: return errors consistently)
-func SmartQueryString(t interface{}, ctx context.Context, chain *cosmos.CosmosChain, contractAddr, queryMsg string, res interface{}) error { +func SmartQueryString(ctx context.Context, chain *cosmos.CosmosChain, contractAddr, queryMsg string, res interface{}) error { var jsonMap map[string]interface{} if err := json.Unmarshal([]byte(queryMsg), &jsonMap); err != nil { - panic(err) + return fmt.Errorf("invalid query JSON: %w", err) } - err := chain.QueryContract(ctx, contractAddr, jsonMap, &res) - return err + return chain.QueryContract(ctx, contractAddr, jsonMap, &res) }This would require updating callers to drop the
targument.
187-188: Remove unusedctxparameter fromTxCommandBuilderNodeandTxCommandBuilder.The
ctx context.Contextparameter is accepted but never used in either function. Removing it will clean up the API and eliminate the misleading parameter.Proposed fix
-func TxCommandBuilder(ctx context.Context, chain *cosmos.CosmosChain, cmd []string, fromUser string, extraFlags ...string) []string { - return TxCommandBuilderNode(ctx, chain.GetNode(), cmd, fromUser, extraFlags...) +func TxCommandBuilder(chain *cosmos.CosmosChain, cmd []string, fromUser string, extraFlags ...string) []string { + return TxCommandBuilderNode(chain.GetNode(), cmd, fromUser, extraFlags...)-func TxCommandBuilderNode(ctx context.Context, node *cosmos.ChainNode, cmd []string, fromUser string, extraFlags ...string) []string { +func TxCommandBuilderNode(node *cosmos.ChainNode, cmd []string, fromUser string, extraFlags ...string) []string {
| vals = 1 | ||
| fNodes = 0 | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the actual code in the setup.go file around the mentioned lines
echo "=== Code around lines 121-123 ==="
sed -n '115,130p' tests/interchaintest/setup.go
echo ""
echo "=== Code around lines 35-36 ==="
sed -n '30,40p' tests/interchaintest/setup.goRepository: KiiChain/kiichain
Length of output: 1260
🏁 Script executed:
#!/bin/bash
# Check if vals and fNodes are used anywhere in the test files
echo "=== Usage of 'vals' in tests/interchaintest/ ==="
rg -n --type=go '\bvals\b' tests/interchaintest/
echo ""
echo "=== Usage of 'fNodes' in tests/interchaintest/ ==="
rg -n --type=go '\bfNodes\b' tests/interchaintest/Repository: KiiChain/kiichain
Length of output: 253
🏁 Script executed:
#!/bin/bash
# Check if vals and fNodes are used anywhere in the entire repository
echo "=== Global usage of 'vals' ==="
rg -n --type=go '\bvals\b' . 2>/dev/null | grep -v node_modules | head -20
echo ""
echo "=== Global usage of 'fNodes' ==="
rg -n --type=go '\bfNodes\b' . 2>/dev/null | grep -v node_modules | head -20Repository: KiiChain/kiichain
Length of output: 221
Remove unused duplicate constants vals and fNodes.
Lines 121-122 define vals = 1 and fNodes = 0, which are never used anywhere in the codebase. They duplicate NumberVals = 1 and NumberFullNodes = 0 (defined at lines 35-36) with identical values. Delete these unused constants to avoid confusion and reduce unnecessary code.
🤖 Prompt for AI Agents
In `@tests/interchaintest/setup.go` around lines 121 - 123, Remove the duplicate,
unused constant declarations vals and fNodes: locate the block where vals = 1
and fNodes = 0 are defined and delete those two constants; keep and use the
existing canonical constants NumberVals and NumberFullNodes (already defined) so
there are no duplicate definitions or unused variables remaining.
1 participant
Description
Adds sample and starting E2E tests via interchain. Brought from audit
Type of change
How Has This Been Tested?
pipelines