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 reviewers
Poem
✨ Finishing Touches
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 (
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1804 +/- ##
===========================================
+ Coverage 16.87% 35.75% +18.88%
===========================================
Files 72 127 +55
Lines 6163 11801 +5638
===========================================
+ Hits 1040 4220 +3180
- Misses 5000 7160 +2160
- Partials 123 421 +298
🚀 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: 0
♻️ Duplicate comments (1)
integration_tests/test_ibc_rly.py (1)
122-122: Fix default mutable argument.Using a mutable list as a default argument can lead to unexpected behavior.
-def fungible(dst, src, amt, denom, trace=[]): +def fungible(dst, src, amt, denom, trace=None): + if trace is None: + trace = []
🧹 Nitpick comments (2)
integration_tests/ibc_utils.py (1)
95-98: Fix formatting issue in conditional expression.Apply Black formatting to fix the spacing:
- if incentivized or is_ibc_transfer else [] + if incentivized or is_ibc_transfer + else []integration_tests/test_ibc_rly.py (1)
19-20: Fix import order.Move the
hermes_transferimport to maintain alphabetical order within the group:from .ibc_utils import ( RATIO, assert_duplicate, cronos_transfer_source_tokens, cronos_transfer_source_tokens_with_proxy, + hermes_transfer, ibc_denom, ibc_incentivized_transfer, ibc_multi_transfer, ibc_transfer, prepare_network, rly_transfer, - hermes_transfer, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
CHANGELOG.md(1 hunks)app/app.go(12 hunks)integration_tests/configs/ibc_timeout_hermes.jsonnet(1 hunks)integration_tests/cosmoscli.py(1 hunks)integration_tests/ibc_utils.py(10 hunks)integration_tests/test_ibc.py(2 hunks)integration_tests/test_ibc_rly.py(12 hunks)integration_tests/test_ibc_rly_gas.py(2 hunks)integration_tests/test_ibc_timeout.py(2 hunks)integration_tests/test_ibc_update_client.py(2 hunks)integration_tests/test_ica.py(2 hunks)integration_tests/test_ica_incentivized.py(1 hunks)integration_tests/test_ica_precompile.py(2 hunks)integration_tests/test_upgrade.py(2 hunks)nix/default.nix(1 hunks)nix/hermes.nix(3 hunks)nix/sources.json(2 hunks)versiondb/client/restore_app_db.go(0 hunks)versiondb/client/verify.go(1 hunks)versiondb/go.mod(0 hunks)x/cronos/keeper/keeper.go(6 hunks)x/cronos/keeper/precompiles/relayer.go(4 hunks)x/cronos/middleware/conversion_middleware.go(8 hunks)x/cronos/types/interfaces.go(2 hunks)
💤 Files with no reviewable changes (2)
- versiondb/go.mod
- versiondb/client/restore_app_db.go
✅ Files skipped from review due to trivial changes (1)
- integration_tests/test_ica_incentivized.py
🚧 Files skipped from review as they are similar to previous changes (14)
- integration_tests/test_ica_precompile.py
- versiondb/client/verify.go
- CHANGELOG.md
- integration_tests/test_ibc_timeout.py
- nix/hermes.nix
- integration_tests/test_ibc_rly_gas.py
- integration_tests/test_ibc_update_client.py
- integration_tests/configs/ibc_timeout_hermes.jsonnet
- integration_tests/cosmoscli.py
- integration_tests/test_ica.py
- integration_tests/test_upgrade.py
- nix/default.nix
- nix/sources.json
- integration_tests/test_ibc.py
🧰 Additional context used
🪛 GitHub Check: Lint python
integration_tests/ibc_utils.py
[failure] 98-98:
./integration_tests/ibc_utils.py:98:51: BLK100 Black would make changes.
integration_tests/test_ibc_rly.py
[failure] 19-19:
./integration_tests/test_ibc_rly.py:19:1: I001 isort found an import in the wrong position
[failure] 267-267:
./integration_tests/test_ibc_rly.py:267:25: BLK100 Black would make changes.
🪛 Ruff (0.11.9)
integration_tests/test_ibc_rly.py
122-122: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: unittest
- GitHub Check: Analyze (go)
🔇 Additional comments (27)
x/cronos/types/interfaces.go (2)
11-21: LGTM! Import paths correctly updated to IBC-Go v10.The IBC module imports have been properly updated from v8 to v10, aligning with the PR objective to integrate ibc-go version 10.1.1.
39-39: Interface method updated to match IBC-Go v10 changes.The
GetDenomTracemethod has been correctly renamed toGetDenomwith the return type changed fromtypes.DenomTracetotypes.Denom, which aligns with the new structured denomination handling in IBC-Go v10.x/cronos/keeper/precompiles/relayer.go (4)
16-17: LGTM! IBC imports properly upgraded to v10.The IBC transfer and channel types imports have been correctly updated to v10.
105-114: Correctly removed IBC fee keeper dependency.The
RelayerContractstruct and its constructor have been properly updated to remove the IBC fee keeper, which is consistent with the removal of the fee module from the codebase.
159-162: Denom extraction logic updated for IBC-Go v10.The logic has been correctly updated to use
ExtractDenomFromPathandHasPrefixmethods, which aligns with the new structured denom handling in IBC-Go v10 where denominations include trace hops.
199-246: Fee-related method handling properly removed.The removal of
RegisterPayeeandRegisterCounterpartyPayeecase handling is correct, as these methods are no longer needed without the IBC fee module.integration_tests/ibc_utils.py (4)
53-58: Good addition of is_ibc_transfer parameter for channel version handling.The new parameter properly distinguishes between IBC transfer channels (string version) and ICA channels (JSON version), which is necessary for correct Hermes relayer operation.
Also applies to: 154-154
164-171: Appropriate config file remapping for Hermes relayer.The conditional remapping of config files for Hermes ensures compatibility while maintaining backward compatibility with the rly relayer.
217-223: ICA version structure correctly updated.The ICA version has been properly updated to include all required fields for IBC-Go v10, including encoding, transaction type, and connection IDs.
458-458: Denom trace query correctly fixed.The query now properly targets the second chain (chainmain) where the denom trace should be queried, which is the correct behavior.
integration_tests/test_ibc_rly.py (3)
76-92: Token structure correctly updated for IBC-Go v10.The new
token_dictfunction properly creates tokens with the structured denom format that includes base and trace fields, aligning with the IBC-Go v10 token representation.
251-258: Conditional test execution properly handles relayer differences.The code correctly handles differences between Hermes and rly relayers, skipping log verification for Hermes since it doesn't emit EVM messages that trigger the Cronos precompiled contract.
322-322: Tests appropriately skipped for Hermes relayer.The incentivized transfer and source token transfer tests are correctly marked as skipped when using Hermes, as the precompiled contract behavior is not supported with this relayer.
Also applies to: 419-419, 424-424
x/cronos/keeper/keeper.go (4)
18-21: IBC imports correctly updated to v10.All IBC-related imports have been properly upgraded to v10, including the new
ibccallbacktypespackage needed for the callback interface.
51-51: Keeper correctly implements the IBC callbacks interface.The explicit declaration that
Keeperimplementsibccallbacktypes.ContractKeeperensures type safety and proper interface compliance.
316-323: Acknowledgement handling simplified after fee module removal.The method now directly unmarshals to
channeltypes.Acknowledgementwithout the fee middleware wrapping, which is the correct approach after removing the IBC fee module.
316-316: IBC callback signatures correctly updated with version parameter.All four IBC callback methods have been properly updated to include the new
versionstring parameter as required by IBC-Go v10.Also applies to: 331-331, 341-341, 355-355
app/app.go (5)
113-131: Import changes look goodThe IBC-Go imports have been correctly updated from v8 to v10, including the new
ibccallbacksmiddleware andibctmlight client module imports needed for v10 compatibility.
239-265: Store key management updated correctlyThe removal of capability and fee module store keys aligns with the v10 upgrade objectives. The addition of object store keys to the return signature properly supports the bank and EVM modules' object storage needs.
583-619: Keeper initialization properly migrated to v10 patternsAll IBC-related keepers have been correctly updated to use
runtime.NewKVStoreServiceinstead of direct store keys, which is the required pattern for IBC-Go v10. The removal of capability keeper dependencies is properly handled.
730-735: Light client module registration follows v10 patternsThe Tendermint light client module is correctly instantiated and registered with the client keeper using the new v10 pattern.
709-719: Verify gas limit control for IBC callbacksThe ICA controller stack correctly removes fee middleware and adds the callbacks middleware. However, using
math.MaxUint64as the gas limit for callbacks could pose a risk if not properly controlled elsewhere. The comment mentions that "cronos keeper will use network parameter to control it" - please ensure this gas control mechanism is properly implemented.#!/bin/bash # Description: Verify gas control implementation in cronos keeper # Search for gas limit control in cronos keeper ast-grep --pattern 'func ($_ *Keeper) $$$Gas$$$($$$) $$$' # Search for gas-related parameters in cronos module rg -A 5 "Gas|gas" --glob "x/cronos/types/*.go" # Check for gas limit validation in IBC callback handling rg -A 10 "OnIBCCallback|IBCCallback" --glob "x/cronos/keeper/*.go"x/cronos/middleware/conversion_middleware.go (5)
29-52: Channel lifecycle methods correctly updated for v10The removal of
chanCapcapability parameters fromOnChanOpenInitandOnChanOpenTryproperly aligns with IBC-Go v10's removal of the capability module.
94-125: OnRecvPacket correctly updated with v10 interfaceThe addition of
channelVersionparameter and its usage inUnmarshalPacketDatafollows the v10 pattern correctly. The refactoredconvertVoucherscall with explicit parameters improves code clarity.
127-193: Packet acknowledgement and timeout handling properly migratedBoth
OnAcknowledgementPacketandOnTimeoutPacketcorrectly implement the v10 interface with thechannelVersionparameter and properly handle packet data unmarshaling for refund scenarios.
195-216: Clean refactoring of convertVouchers methodThe refactoring to use explicit parameters instead of
FungibleTokenPacketDataimproves code clarity and reduces coupling to IBC transfer types.
227-244: Denom handling correctly modernized for v10The migration from string-based denom handling to structured traces with
Hoptypes is properly implemented. The logic correctly handles trace manipulation for both prefixed and non-prefixed denominations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
integration_tests/test_ibc_rly.py (1)
122-122: Fix mutable default argument (duplicate issue).This is the same mutable default argument issue that was flagged in previous reviews.
-def fungible(dst, src, amt, denom, trace=[]): +def fungible(dst, src, amt, denom, trace=None): + if trace is None: + trace = []
🧹 Nitpick comments (4)
integration_tests/ibc_utils.py (2)
58-58: Function parameter and version handling looks correct.The addition of
is_ibc_transferparameter and the conditional version formatting (string for IBC transfers, JSON for ICA) aligns with the different requirements of these protocols.Consider extracting the version formatting logic for better readability:
+ def format_version_for_hermes(version, is_ibc_transfer): + return str(version) if is_ibc_transfer else json.dumps(version) + + ( [ "--channel-version", - str(version) if is_ibc_transfer else json.dumps(version), + format_version_for_hermes(version, is_ibc_transfer), ] if incentivized or is_ibc_transfer else [] )Also applies to: 96-99
165-173: Config file remapping logic is sound.The conditional remapping for Hermes relayer appropriately handles compatibility issues with EVM-specific configurations.
Consider adding a comment explaining why these specific config files are remapped:
+ # Hermes doesn't support EVM-specific configurations, fallback to compatible configs if is_hermes: if file == "ibc_rly_evm": config_file = "ibc_rly" if file == "ibc_timeout": config_file = "ibc_timeout_hermes"integration_tests/test_ibc_rly.py (2)
250-316: Appropriate conditional testing approach for different relayers.The conditional logic correctly handles the difference between Hermes and rly relayers, with Hermes skipping EVM log verification since it doesn't trigger Cronos precompiled contracts.
Consider extracting the relayer-specific test logic into separate functions for better maintainability:
def test_ibc_with_hermes(ibc): ibc_transfer(ibc, hermes_transfer) # Hermes doesn't emit EVM messages for cronos precompiled contract def test_ibc_with_rly(ibc): # Current rly-specific logic with log verification pass def test_ibc(ibc): if ibc.hermes is not None: test_ibc_with_hermes(ibc) else: test_ibc_with_rly(ibc)
325-325: Appropriate test skipping for Hermes compatibility.The skipped tests correctly identify functionality that's incompatible with Hermes relayer due to precompiled contract dependencies.
Consider tracking these skipped tests for future Hermes support or alternative testing approaches. The current approach maintains test integrity while acknowledging architectural limitations.
Also applies to: 422-422, 427-427
📜 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/default.nix(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nix/default.nix
🧰 Additional context used
🪛 Ruff (0.11.9)
integration_tests/test_ibc_rly.py
122-122: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc)
- GitHub Check: unittest
- GitHub Check: Run golangci-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
integration_tests/ibc_utils.py (5)
4-4: LGTM: Required import for environment variable access.The
osimport is needed for theos.getenv("SIGNER1_MNEMONIC")call in the relayer key restore logic.
194-207: Conditional key restore logic is appropriate.Skipping the relayer key restore for Hermes is correct since different relayers have different key management approaches.
419-419: Verify impact of reduced test coverage.The reduction from 100 to 50 users in multi-transfer tests decreases the concurrency testing coverage.
Please confirm if this reduction is intentional and document the reasoning. Consider:
- Performance/resource constraints
- Test reliability improvements
- Impact on edge case detection
If this is a temporary change, consider making it configurable:
- users = [f"user{i}" for i in range(1, 50)] + num_users = int(os.getenv("IBC_TEST_USERS", "50")) + users = [f"user{i}" for i in range(1, num_users)]
459-459: LGTM: Consistent denom trace query targeting.Explicitly targeting
ibc.chainmain.node_rpc(0)ensures consistent denom trace queries against the correct chain.Also applies to: 564-564
868-876: LGTM: Correct ICA registration format for ibc-go v10.The updated ICA registration version structure with "ics27-1" properly aligns with IBC-go v10 requirements for ICA protocol.
Note: This correctly uses "ics27-1" for ICA, which should also be applied to the
ica_versiondefinition earlier in the file (lines 217-223).Likely an incorrect or invalid review comment.
integration_tests/test_ibc_rly.py (4)
5-5: LGTM: Import additions and variable usage improvements.The new imports support dual relayer functionality, and using
port_idvariable instead of hardcoded "transfer" improves maintainability.Also applies to: 13-13, 19-19, 41-41
58-60: LGTM: Fixture updated for Hermes relayer default.The fixture correctly defaults to Hermes relayer and includes the
is_ibc_transferparameter as expected for IBC transfer tests.
76-91: LGTM: Enhanced token representation with trace support.The
token_dictfunction properly supports token representation with trace information, which is required for the updated IBC protocol structures.
334-334: LGTM: Updated token representation with trace information.The enhanced fungible token events with trace information (portId and channelId) correctly implement the IBC-go v10 token representation requirements.
Also applies to: 390-390, 352-357, 396-401
JayT106
force-pushed
the
jt/ibc-go-v10
branch
5 times, most recently
from
bb4a596 to
c4f39d0
Compare
JayT106
changed the title
JayT106
force-pushed
the
jt/ibc-go-v10
branch
3 times, most recently
from
5858e3a to
125820c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
nix/hermes.nix (1)
61-68: Consider the performance implications of QEMU emulation for ARM64 Linux.The QEMU emulation approach for ARM64 Linux systems is technically sound but may introduce significant performance overhead. Consider documenting this limitation or providing guidance on when native ARM64 binaries might be available.
Consider adding a comment to document this limitation:
${if (stdenv.isLinux && stdenv.isAarch64) then '' + # Note: Using x86_64 binary with QEMU emulation due to lack of native ARM64 Linux binary + # This may result in reduced performance compared to native execution # for ARM64 uses qemu to simulate x86_64 mv $out/bin/hermes $out/bin/hermes.x86_64
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nix/hermes.nix(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Run golangci-lint
- GitHub Check: gomod2nix
- GitHub Check: build (macos-14)
- GitHub Check: unittest
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
nix/hermes.nix (4)
2-10: Input parameters look appropriate for binary packaging.The transition from
rustPlatform.buildRustPackageinputs to binary packaging inputs (fetchurl,makeWrapper,qemu) aligns well with the new approach of using prebuilt Hermes binaries.
36-58: Installation phase is clean and straightforward.The derivation setup correctly uses
fetchurlfor the source and the installation phase properly extracts and installs the binary with appropriate permissions.
76-82: Metadata is comprehensive and well-structured.The metadata section includes all necessary information including description, homepage, license, supported platforms, and maintainer information.
13-33: ```shell
#!/usr/bin/env bash
set -euo pipefailecho "Verifying SHA256 hashes for Hermes v1.13.1 binaries using Python..."
Determine Python interpreter
if command -v python3 &>/dev/null; then
PY=python3
elif command -v python &>/dev/null; then
PY=python
else
echo "Error: No Python interpreter found" >&2
exit 1
fi
echo "Using interpreter: $($PY --version 2>&1)"Expected hashes
declare -A expected=(
["aarch64-apple-darwin"]="1j87ikp29008f6x1pcbp8bc77yfhf40sa13d6iliglsisrgsjcas"
["x86_64-apple-darwin"]="0f9m8g2xg9l3ghvj42kwa7yn6gr3ralylscmz5bs99qdd5hc8fbd"
["x86_64-unknown-linux-gnu"]="0a5anc32brrl390i1aiz3yaar1s9lh3s8r70liw3v7lgd5fnpzgg"
)for arch in "${!expected[@]}"; do
url="https://github.com/informalsystems/hermes/releases/download/v1.13.1/hermes-v1.13.1-${arch}.tar.gz"
echo
echo "Checking ${arch}:"
sha=$(
curl -sL "$url" | $PY - << 'EOF'
import sys, hashlib
h = hashlib.sha256()
for chunk in iter(lambda: sys.stdin.buffer.read(8192), b""):
h.update(chunk)
print(h.hexdigest())
EOF
)
echo "Computed: $sha"
echo "Expected: ${expected[$arch]}"
done</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
nix/hermes.nix (2)
2-9: Remove unused import.The
pkg-configimport is not used anywhere in the derivation and should be removed to keep the imports clean.{ lib , stdenv , fetchurl , makeWrapper , openssl -, pkg-config , darwin , qemu }:
61-68: Consider documenting the QEMU emulation approach.The QEMU emulation for ARM64 Linux systems is a clever workaround but may have performance implications. Consider adding a comment explaining why this approach was chosen over building native ARM64 binaries.
postFixup = '' ${if (stdenv.isLinux && stdenv.isAarch64) then '' + # Use QEMU emulation since no native ARM64 Linux binaries are available # for ARM64 uses qemu to simulate x86_64 mv $out/bin/hermes $out/bin/hermes.x86_64
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
nix/default.nix(1 hunks)nix/hermes.nix(1 hunks)nix/sources.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- nix/default.nix
- nix/sources.json
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: unittest
- GitHub Check: gomod2nix
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
nix/hermes.nix (6)
13-13: Version alignment confirmed.The version "1.13.1" correctly aligns with the PR objectives which specify using Hermes relayer version 1.13.1.
15-22: Platform detection logic is sound.The platform detection correctly handles the supported platforms and provides clear error messages for unsupported ones.
36-58: Binary installation approach is well-implemented.The switch from building from source to using prebuilt binaries is correctly implemented with proper binary extraction and permission setting.
69-74: Library path configuration is appropriate.The OpenSSL library path configuration for Linux and the wrapper setup for other platforms is correctly implemented.
76-82: Metadata section is comprehensive.The metadata provides good information about the package, though the maintainers field is empty which might need attention for future maintenance.
26-33: ```shell
#!/usr/bin/env bash
set -eVERSION="1.13.1"
declare -a platforms=(
"aarch64-apple-darwin"
"x86_64-apple-darwin"
"x86_64-unknown-linux-gnu"
)echo "Verifying SHA256 for Hermes v${VERSION} binaries..."
for platform in "${platforms[@]}"; do
url="https://github.com/informalsystems/hermes/releases/download/v${VERSION}/hermes-v${VERSION}-${platform}.tar.gz"
echo -n "$platform: "Compute SHA256 via Python
curl -sL "$url" | python3 - << 'EOF'
import sys, hashlib
h = hashlib.sha256()
for chunk in iter(lambda: sys.stdin.buffer.read(8192), b''):
h.update(chunk)
print(h.hexdigest())
EOF
done</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
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
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