chore: fix ibc tests with chain-main ibc-go-v10 #1871
Conversation
JayT106
requested review from
calvinaco and
XinyuCRO
and removed request for
a team
WalkthroughQuery IBC transfer responses' Changes
Sequence Diagram(s)sequenceDiagram
participant Test as integration_tests
participant CLI as cosmoscli
participant Node as ibc-transfer-endpoint
Note over Test,CLI: Test requests denom info for an IBC transfer
Test->>CLI: request_ibc_denom_trace(tx_hash)
CLI->>Node: GET /ibc-transfer/... (query `denom` field)
Node-->>CLI: return { "denom": { "base":"...", "trace":[{ "port_id":"transfer","channel_id":"chan" }] } }
CLI-->>Test: return denom object
Note over Test: Tests assert denom["base"] and denom["trace"] structure
Note over Test: Looped balance waits now use local check functions to avoid late-binding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
nix/sources.json (1)
3-9: Fork pinning and unusedrev: tighten supply chain and determinism.
- Using a personal fork is fine temporarily, but plan to move back to
crypto-org-chain/chain-mainonce ibc‑go‑v10 lands.- With the current URL not parameterized by
rev, therevfield is effectively unused. Ensureurl_templatereferences<rev>(see other comment) so future bumps are tracked byrev.When the official tag is published, switch:
- "owner": "JayT106", - "rev": "293428e4d63c0fd10ffda4c07636b0f1401d648b", + "owner": "crypto-org-chain", + "rev": "refs/tags/ibc-go-v10" // or the final release tagThen refresh
urlto match the template and recomputesha256.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
integration_tests/cosmoscli.py(1 hunks)integration_tests/ibc_utils.py(1 hunks)nix/sources.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
nix/sources.json (1)
11-12: Pin source tarball to commit; avoid platform-specific assetFile: nix/sources.json Lines: 11-12
Replace the Darwin/arm64 release asset with the repo archive tarball pinned to a specific and update sha256 to ensure reproducible, cross‑platform builds.
- "url": "https://github.com/JayT106/chain-main/archive/refs/tags/ibc-go-v10/chain-main_devel_Darwin_arm64.tar.gz", - "url_template": "https://github.com/JayT106/chain-main/archive/refs/tags/ibc-go-v10/chain-main_devel_Darwin_arm64.tar.gz" + "url": "https://github.com/JayT106/chain-main/archive/293428e4d63c0fd10ffda4c07636b0f1401d648b.tar.gz", + "url_template": "https://github.com/<owner>/<repo>/archive/<rev>.tar.gz"Compute the base32 sha256 and update
sha256(nix-prefetch-url unavailable in this environment — run locally):nix-prefetch-url --unpack https://github.com/JayT106/chain-main/archive/293428e4d63c0fd10ffda4c07636b0f1401d648b.tar.gz
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1871 +/- ##
==========================================
+ Coverage 16.87% 17.47% +0.60%
==========================================
Files 72 75 +3
Lines 6163 6059 -104
==========================================
+ Hits 1040 1059 +19
+ Misses 5000 4877 -123
Partials 123 123 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
integration_tests/ibc_utils.py (2)
460-464: Drop debug print; new denom-trace shape checks look goodRemove the
print("denom_trace", ...)to keep CI logs clean. The subsequent assertions are correct for the new{"base","trace"}structure.Apply this diff:
- print("denom_trace", denom_trace)
571-574: Update legacy assertion to new{"base","trace"}denom structureThis still asserts the old
{"path","base_denom"}shape and will break with ibc-go v10 responses. Align with the new structure.Apply this diff:
- assert denom_trace == {"path": f"transfer/{dst_channel}", "base_denom": base_denom0} + assert denom_trace["base"] == base_denom0 + assert denom_trace["trace"] == [{"port_id": "transfer", "channel_id": dst_channel}]Run to ensure no remaining legacy assertions:
#!/bin/bash rg -n -C2 -e 'base_denom"|base_denom\'|denom_trace\s*==\s*\{.*base_denom' integration_tests/
🧹 Nitpick comments (2)
integration_tests/ibc_utils.py (2)
466-469: Bind loop variable to satisfy Ruff B023 and harden against late-bindingBind
ivia a default argument so the nested function doesn’t capture the loop var.Apply this diff:
- def check_balance(): - return assert_trace_balance(addrs1[i]) + def check_balance(addr=addrs1[i]): + return assert_trace_balance(addr)
493-496: Repeat: bind loop variable in nested defSame issue as above; bind
ito avoid late-binding and silence Ruff B023.Apply this diff:
- def check_balance(): - return assert_trace_balance(addrs0[i]) + def check_balance(addr=addrs0[i]): + return assert_trace_balance(addr)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration_tests/ibc_utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/ibc_utils.py (1)
integration_tests/utils.py (1)
wait_for_fn(96-103)
🪛 GitHub Check: Lint python
integration_tests/ibc_utils.py
[failure] 466-466:
./integration_tests/ibc_utils.py:466:1: BLK100 Black would make changes.
🪛 Ruff (0.12.2)
integration_tests/ibc_utils.py
467-467: Function definition does not bind loop variable i
(B023)
494-494: Function definition does not bind loop variable i
(B023)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: semgrep-cloud-platform/scan
JayT106
force-pushed
the
jt/ibc-v10-test-dep-update
branch
from
c60d6e2 to
be039be
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
integration_tests/ibc_utils.py (6)
459-463: Rename variable to reflect new API shape (nit).
ibc_denom_tracenow returns adenomobject (with "base"/"trace"), not a legacy "denom_trace". Rename for clarity.- denom_trace = chains[1].ibc_denom_trace(path, ibc.chainmain.node_rpc(0)) + denom = chains[1].ibc_denom_trace(path, ibc.chainmain.node_rpc(0)) - assert denom_trace["base"] == denom0 - assert denom_trace["trace"] == [{"port_id": "transfer", "channel_id": channel1}] + assert denom["base"] == denom0 + assert denom["trace"] == [{"port_id": "transfer", "channel_id": channel1}]
465-465: Drop noisy debug print in test loop (CI noise).Recommend removing this print.
- print(f"user{i+1} -> {addrs1[i]}")
468-472: Bind loop variable into closure to avoid late‑binding (Ruff B023).Bind
addras a default arg so the checker is happy and future refactors stay safe.- def check_balance(): - return assert_trace_balance(addr) + def check_balance(addr=addr): + return assert_trace_balance(addr)
496-496: Drop noisy debug print in test loop (CI noise).Same as above.
- print(f"user{i+1} -> {addrs0[i]}")
499-503: Bind loop variable into closure to avoid late‑binding (Ruff B023).Same pattern here.
- def check_balance(): - return assert_balance(addr) + def check_balance(addr=addr): + return assert_balance(addr)
579-581: Nit: rename local to match returned object.For consistency with the earlier block, rename
denom_tracetodenom.- denom_trace = chains[1].ibc_denom_trace(path, ibc.chainmain.node_rpc(0)) - assert denom_trace["base"] == base_denom0 - assert denom_trace["trace"] == [{"port_id": "transfer", "channel_id": dst_channel}] + denom = chains[1].ibc_denom_trace(path, ibc.chainmain.node_rpc(0)) + assert denom["base"] == base_denom0 + assert denom["trace"] == [{"port_id": "transfer", "channel_id": dst_channel}]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
integration_tests/ibc_utils.py(3 hunks)nix/sources.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nix/sources.json
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/ibc_utils.py (1)
integration_tests/utils.py (1)
wait_for_fn(96-103)
🪛 Ruff (0.12.2)
integration_tests/ibc_utils.py
469-469: Function definition does not bind loop variable addr
(B023)
500-500: Function definition does not bind loop variable addr
(B023)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc)
🔇 Additional comments (2)
integration_tests/ibc_utils.py (2)
461-462: Denom structure assertions look correct for ibc-go v10.Good switch to asserting the new {"base","trace"} shape.
580-581: Denom structure assertions look correct for ibc-go v10.Matches the new {"base","trace"} format.
JayT106
force-pushed
the
jt/ibc-v10-test-dep-update
branch
2 times, most recently
from
c403794 to
0b53318
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
integration_tests/ibc_utils.py (2)
465-471: Bind loop var to avoid late-binding (B023) and gate noisy printsCapture addr via default arg to silence B023 and make logs optional.
Apply:
- print(f"user{i+1} -> {addrs1[i]}") + if os.getenv("VERBOSE_TEST_LOGS"): + print(f"user{i+1} -> {addrs1[i]}") addr = addrs1[i] - def check_balance(): - return assert_trace_balance(addr) + def check_balance(addr=addr): + return assert_trace_balance(addr) - wait_for_fn("assert balance", check_balance) + wait_for_fn(f"assert balance for {addr}", check_balance)
496-503: Repeat: bind loop var (B023) and gate printsSame fix for the second loop.
Apply:
- print(f"user{i+1} -> {addrs0[i]}") + if os.getenv("VERBOSE_TEST_LOGS"): + print(f"user{i+1} -> {addrs0[i]}") addr = addrs0[i] - def check_balance(): - return assert_balance(addr) + def check_balance(addr=addr): + return assert_balance(addr) - wait_for_fn("assert balance", check_balance) + wait_for_fn(f"assert balance for {addr}", check_balance)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
integration_tests/ibc_utils.py(3 hunks)nix/sources.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nix/sources.json
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/ibc_utils.py (1)
integration_tests/utils.py (1)
wait_for_fn(96-103)
🪛 Ruff (0.12.2)
integration_tests/ibc_utils.py
469-469: Function definition does not bind loop variable addr
(B023)
500-500: Function definition does not bind loop variable addr
(B023)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
integration_tests/ibc_utils.py (2)
459-463: Denom trace assertions updated to ibc-go v10 shape — LGTMChecks against {"base","trace"} look correct for the new denom object.
580-581: Denom trace assertions aligned to returned denom object — LGTMMatches expected {"base","trace"} structure for dst_channel.
JayT106
force-pushed
the
jt/ibc-v10-test-dep-update
branch
from
0b53318 to
46c2016
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
the ibc-rly integration test is flaky, need time to find the root cause. |
Update chain-main resource to support ibc-go-v10,
It's a template fix once ibc-go-v10 has official released in chain-main repo.
Summary by CodeRabbit
Bug Fixes
Tests
Chores