feat: bump up ibc-go from v8 to v10 #51
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Copilot reviewed 67 out of 69 changed files in this pull request and generated no comments.
Files not reviewed (2)
- contracts/package-lock.json: Language not supported
- go.mod: Language not supported
Comments suppressed due to low confidence (1)
ibc/errors.go:7
- The updated error message 'denom not found' is less specific than the previous 'denom trace not found'. Please ensure that downstream error handling is updated accordingly to avoid ambiguity.
ErrDenomNotFound = errors.New("denom not found")
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
zsystm
force-pushed
the
feat/ibc-go-v8-to-v10
branch
2 times, most recently
from
c18b6b6 to
9f3013f
Compare
There was a problem hiding this comment.
Copilot reviewed 70 out of 72 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- contracts/package-lock.json: Language not supported
- go.mod: Language not supported
Comments suppressed due to low confidence (3)
ibc/errors.go:7
- The error message was renamed from 'ErrDenomTraceNotFound' to 'ErrDenomNotFound'. Ensure that this change is well documented in the migration notes to prevent confusion for developers upgrading to v10.
ErrDenomNotFound = errors.New("denom not found")
evmd/app.go:287
- The removal of the CapabilityKeeper initialization and its scoped keepers may affect modules that expect capability management to be set up. Verify that all dependent modules have been updated to reflect this change and that no unexpected runtime issues will occur.
- CapabilityKeeper *capabilitykeeper.Keeper
evmd/precompiles.go:45
- The change in the channelKeeper parameter type from a value to a pointer may require updates in caller functions to pass the correct type. Double-check that all invocations align with the new pointer-based API to avoid type mismatches.
channelKeeper "github.com/cosmos/ibc-go/v10/modules/core/04-channel/keeper"
db82eff to
a458036
Compare
|
@dongsam |
dongsam
reviewed
Apr 7, 2025
zsystm
force-pushed
the
feat/ibc-go-v8-to-v10
branch
from
5415143 to
a458036
Compare
zsystm
changed the title
dongsam
reviewed
Apr 8, 2025
dongsam
reviewed
Apr 8, 2025
dongsam
reviewed
Apr 8, 2025
| PageResponse memory pageResponse | ||
| ); | ||
|
|
||
| /// @dev DenomTrace defines a method for returning a denom trace. | ||
| function denomTrace( | ||
| function denom( |
There was a problem hiding this comment.
Since this is an interface-breaking change, I thought it needed discussion, so I left a note in the issue #44
DenomTrace Endpoint Renaming in IBC v10
- It is necessary to first check whether any existing app chains are currently using the
ics20precompile contract from eithercosmos/evmorevmos/os.- If no such usage exists, it would be a good opportunity to align with IBC v10's new endpoint by updating the Solidity interface to use
Denom.- If there are any, it would be a breaking change, so we may need to decide whether to proceed with a rename on the solidity level or manage versioning for the precompile accordingly.
|
@zsystm Could you bump to ibc-go v10.1.1?
|
Thanks for sharing. Will do. |
|
dongsam
approved these changes
Apr 10, 2025
There was a problem hiding this comment.
LGTM as a temporary bump for follow-up work
Note: Since there may be additional changes in v10 and further modifications could arise during the IBC v2 work and test case PR progress, it seems good as a temporary bump targeting feat/ibc-v10 for follow-up work, rather than merging directly into main.
- bumped up ibc-go from v8 to v10 - removed unused ibc test codes because bumping unused testing codes are wasting time. we should use ibc testing package instead.
added ibc v1 test cases to make sure ExampleChain works with ibc v1. disabled basefee param as default for ExampleChain to make test easier.
- added ibc v2 components - copied basic v2 test cases from ibc-go v10 to make sure v2 components of ExampleChain works well.
- added nil checks for app and keeper in NewIBCMiddleware to prevent nil pointer dereference. - changed erc20 keeper from struct to interface type to enable proper nil checking.
To test certain key scenarios involving EVM messages (e.g., deploying an ERC20 contract), we needed a block header context with a proposer address. This required: - A custom `TestChain` to handle these messages. - A custom `SignAndDeliver` function to support the transaction signing and delivery process. - A custom `Coordinator` to integrate this tailored `TestChain`. Since `TestChain` and `SignAndDeliver` are directly or indirectly tied to most components in the testing package, and ibc-go cannot use a `TestChain` struct defined in our separate package, we had to copy and adapt nearly all related files to ensure compatibility and functionality.
Disabling the base fee to simplify testing is not ideal for a reference chain intended for developers building EVM-compatible chains. Ethereum relies on a fee market mechanism, and as a reference implementation, this chain should enable it by default to align with expected behavior.
Updated TestGetReceivedCoin to use the newly introduced interface instead of hardcoded strings. This improves maintainability by making the test logic more aligned with the actual send/receive flow. It also enhances readability and helps developers better understand how denoms are constructed and handled in real IBC transfers.
zsystm
force-pushed
the
feat/ibc-go-v8-to-v10
branch
from
e3c8858 to
a84a59d
Compare
This was
linked to
issues
Apr 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
TestChainto process these messages.SignAndDeliverfunction for transaction signing and delivery with proposer address.Coordinatorto integrate the modifiedTestChain.TestChain,SignAndDeliver, and thetestingpackage—and because ibc-go cannot use aTestChaindefined externally—the relevant files were duplicated and tailored to ensure compatibility.Additional Notes
Closes: #44, #45, #50
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
mainbranchReviewers Checklist
All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.
I have...
Unreleasedsection inCHANGELOG.md