feat(ibc)!: ibc-go v10 is not supported #1804
Conversation
Solution: - upgrade to v9 to try new features fix middleware build cleanup upgrade bump version bump deps fix middleware
align events
|
""" WalkthroughThis change upgrades the IBC-Go dependency from v8 to v10, removes the IBC fee and capability modules, and adapts the codebase to the new IBC-Go APIs. It updates protobuf definitions, removes related fee logic, adjusts middleware and relayer contracts, and revises integration tests and configuration for compatibility with the upgraded IBC protocol. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant IBC-Go v10
participant RelayerContract
participant Middleware
participant Keeper
App->>IBC-Go v10: Initialize IBC modules (no fee/capability modules)
App->>Middleware: Set up conversion middleware (v10 API)
RelayerContract->>IBC-Go v10: Relay IBC packets (no fee registration)
Middleware->>Keeper: Handle OnRecvPacket/OnAcknowledgement/OnTimeout (with new version param)
Keeper->>Middleware: Process packet, convert tokens (new denom/trace structure)
Middleware->>IBC-Go v10: Return ack/errors as needed
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1804 +/- ##
===========================================
+ Coverage 16.87% 35.78% +18.91%
===========================================
Files 72 127 +55
Lines 6163 11811 +5648
===========================================
+ Hits 1040 4227 +3187
- Misses 5000 7161 +2161
- Partials 123 423 +300
🚀 New features to boost your workflow:
|
JayT106
force-pushed
the
jt/ibc-go-v10
branch
2 times, most recently
from
cd8f46a to
ea1599d
Compare
JayT106
force-pushed
the
jt/ibc-go-v10
branch
2 times, most recently
from
e2fa02e to
03b2b6e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
integration_tests/ibc_utils.py (1)
217-224: Verify ICA version should be "ics27-1" instead of "ics20-1".The ICA version is still set to "ics20-1" in the channel_version dictionary. Based on the previous review comment and IBC protocol standards, this should likely be "ics27-1" for ICA (Interchain Accounts) rather than "ics20-1" which is for token transfers.
What is the correct ICA (Interchain Accounts) version string for ibc-go v10? Should it be "ics27-1" or "ics20-1"?
🧹 Nitpick comments (1)
nix/hermes.nix (1)
84-90: Add missing maintainer information.The meta section has an empty maintainers list. Consider adding appropriate maintainers for this package.
- maintainers = with maintainers; [ ]; + maintainers = with maintainers; [ /* add appropriate maintainers */ ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
integration_tests/ibc_utils.py(10 hunks)integration_tests/test_ibc_rly.py(11 hunks)nix/hermes.nix(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- integration_tests/test_ibc_rly.py
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: unittest
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: gomod2nix
- GitHub Check: Run golangci-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
nix/hermes.nix (2)
12-35: LGTM: Platform detection and hash handling look correct.The platform detection logic properly handles Darwin (both x86_64 and aarch64) and Linux architectures. The conditional SHA256 hash selection is appropriate for the multi-platform approach.
63-82: Verify QEMU emulation approach for ARM64 Linux.The QEMU wrapper approach for ARM64 Linux is interesting - it wraps the x86_64 binary with
qemu-x86_64emulation. While this should work, it may have performance implications.Consider documenting this approach in the meta description or comments, as it's not immediately obvious why ARM64 Linux uses x86_64 binaries with emulation rather than native ARM64 binaries (if available).
#!/bin/bash # Check if native ARM64 Linux binaries are available for Hermes v1.13.1 curl -s "https://api.github.com/repos/informalsystems/hermes/releases/tags/v1.13.1" | jq -r '.assets[].name' | grep -i aarch64 || echo "No native ARM64 binaries found"integration_tests/ibc_utils.py (6)
58-58: LGTM: New parameter for transfer type differentiation.The
is_ibc_transferparameter addition allows proper differentiation between regular IBC transfers and other channel types, which is essential for correct version handling.
94-100: Correct version format handling for different transfer types.The conditional logic properly handles the version format based on transfer type - using string format for IBC transfers and JSON format for other types. This aligns with the protocol requirements.
165-173: Good config file remapping logic for Hermes compatibility.The logic to remap config files when using Hermes relayer (e.g.,
ibc_rly_evm→ibc_rly,ibc_timeout→ibc_timeout_hermes) ensures proper configuration handling for different relayer types.
215-215: Connection ID is now properly configurable.The change from hardcoded
"connection-0"toos.getenv("CONNECTION_ID", "connection-0")addresses the previous review comment about making the connection ID configurable.
419-419: User count reduction in multi-transfer tests.The reduction from 100 to 49 users (
range(1, 50)) in multi-transfer tests may be intentional for performance reasons or to align with new test requirements.Could you clarify the reason for reducing the user count in multi-transfer tests? Is this for performance optimization or due to protocol limitations in IBC-Go v10?
459-459: Denom trace query correctly targets chainmain node.The change to use
ibc.chainmain.node_rpc(0)as the RPC endpoint for denom trace queries is correct, ensuring queries are directed to the appropriate chain for trace information.Also applies to: 564-564
thomas-nguy
left a comment
thomas-nguy
left a comment
There was a problem hiding this comment.
first parse looks good
de23491
3 participants
Integrated ibc-go v10.1.1
For the integration tests, using hermes relayer 1.13.1 instead of the customized ibc-go-relayer. So the relayer precompiled contract and the ica precompiled contract will not working with the hermes relayer.
ref #1731
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit