Draft
Conversation
- 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.
dongsam
reviewed
Mar 31, 2025
Member
dongsam
left a comment
dongsam
left a comment
There was a problem hiding this comment.
LGTM,
- Regarding linting, except for significant or critical mistakes, we can gradually apply lint rules later.
- The changelog can be added just before merging.
Instead of immediately merging this PR, it would be preferable to include test cases for existing features like ics20 Precompile and erc20 Middleware, along with new additions/verification planned for version 10, and then merge everything at once.
Thus, we have the following options:
- Create a separate PR for adding test code, setting the base branch as the current PR's branch (
tests/ibc-e2e), and proceed accordingly. - Alternatively, change the base branch of the current PR to something like
feat/ibcv10, merge first, and then continue subsequent PRs for adding test cases targetingfeat/ibcv10.
- 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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+78
to
+80
| for _, chain := range coord.Chains { | ||
| coord.UpdateTimeForChain(chain) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning test
| ) (*abci.ResponseFinalizeBlock, error) { | ||
| tb.Helper() | ||
| tx, err := simtestutil.GenSignedMockTx( | ||
| rand.New(rand.NewSource(time.Now().UnixNano())), |
Check warning
Code scanning / CodeQL
Calling the system time Warning test
2 tasks
dudong2
added a commit
that referenced
this pull request
May 28, 2025
zsystm
pushed a commit
that referenced
this pull request
Dec 9, 2025
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
ibctestingpackage.Notes
ReceiverChainIsSourceto new will also be covered in another PR.Closes: #XXXX
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