Skip to content

tests: add ibc test cases #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Apr 14, 2025
Merged

Conversation

zsystm
Copy link

@zsystm zsystm commented Apr 10, 2025

Description

This PR adds test coverage to verify that both IBC v1 and IBC v2 flows are working as expected in ExampleChain.

Coverage

File Name Coverage
github.com/cosmos/evm/ibc/module.go 100.0%
github.com/cosmos/evm/ibc/utils.go 100.0%
github.com/cosmos/evm/tests/ibc/helper.go 81.0%
github.com/cosmos/evm/testutil/ibc.go 100.0%
github.com/cosmos/evm/x/erc20/ibc_middleware.go 91.7%
github.com/cosmos/evm/x/erc20/keeper/ibc_callbacks.go 87.9%
github.com/cosmos/evm/x/ibc/transfer/ibc_module.go 100.0%
github.com/cosmos/evm/x/ibc/transfer/keeper/keeper.go 100.0%
github.com/cosmos/evm/x/ibc/transfer/keeper/msg_server.go 100.0%
github.com/cosmos/evm/x/ibc/transfer/module.go 72.7%
github.com/cosmos/evm/x/ibc/transfer/v2/ibc_module.go 100.0%
# How to get coverage
go test -coverpkg=./... -coverprofile=coverage.out -tags=test ./... 
go tool cover -html=coverage.out -o coverage.html

Closes: #46


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...

  • tackled an existing issue or discussed with a team member
  • left instructions on how to review the changes
  • targeted the main branch

Reviewers 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...

  • added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • confirmed all author checklist items have been addressed
  • confirmed that this PR does not change production code
  • reviewed content
  • tested instructions (if applicable)
  • confirmed all CI checks have passed

@dongsam dongsam mentioned this pull request Apr 10, 2025
9 tasks
@zsystm zsystm self-assigned this Apr 10, 2025
@zsystm zsystm force-pushed the tests/ibc-testcases branch from 4266a7b to 2c2ff33 Compare April 10, 2025 15:43
and also update comments and variable name
@zsystm zsystm force-pushed the tests/ibc-testcases branch from 089b850 to 626aa09 Compare April 10, 2025 16:29
@zsystm zsystm requested a review from dongsam April 10, 2025 16:39
@zsystm zsystm marked this pull request as ready for review April 10, 2025 16:39
@zsystm zsystm force-pushed the tests/ibc-testcases branch from 4f04bdf to ffd131a Compare April 10, 2025 16:43
Copy link
Member

@dongsam dongsam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks good to me

  1. How about adding a test case to reach coverage this part as well?
  2. We also need to add test cases for the ICS20-related precompile, but since the removal of authz dependency is currently in progress, it seems more efficient to handle it after that change is completed.

make sure whether it is registered as dynamic precompiled contract or not
@zsystm zsystm force-pushed the tests/ibc-testcases branch from 61c91c8 to c098171 Compare April 14, 2025 03:55
@zsystm
Copy link
Author

zsystm commented Apr 14, 2025

Overall, this looks good to me

  1. How about adding a test case to reach coverage this part as well?
  2. We also need to add test cases for the ICS20-related precompile, but since the removal of authz dependency is currently in progress, it seems more efficient to handle it after that change is completed.

@dongsam

Thank you for your feedback. The part you mentioned is already covered by existing test cases.

Case1: Source native denom through IBC to ERC20 precompiled contract

	// Case 1. token pair is not registered and is a single hop IBC Coin
	// by checking the prefix we ensure that only coins not native from this chain are evaluated.
	// IsNativeFromSourceChain will check if the coin is native from the source chain.
	// If the coin denom starts with `factory/` then it is a token factory coin, and we should not convert it
	// NOTE: Check https://docs.osmosis.zone/osmosis-core/modules/tokenfactory/ for more information
	case !found && strings.HasPrefix(coin.Denom, "ibc/") && ibc.IsBaseDenomFromSourceChain(data.Denom):
		tokenPair, err := k.RegisterERC20Extension(ctx, coin.Denom)
		if err != nil {
			return channeltypes.NewErrorAcknowledgement(err)
		}

		ctx.EventManager().EmitEvents(
			sdk.Events{
				sdk.NewEvent(
					types.EventTypeRegisterERC20Extension,
					sdk.NewAttribute(types.AttributeCoinSourceChannel, packet.SourceChannel),
					sdk.NewAttribute(types.AttributeKeyERC20Token, tokenPair.Erc20Address),
					sdk.NewAttribute(types.AttributeKeyCosmosCoin, tokenPair.Denom),
				),
			},
		)
		return ack

This is tested in v2 and v1

Case2: Receiving ERC20 native coin through IBC

	// Case 2. native ERC20 token
	case found && pair.IsNativeERC20():
		// Token pair is disabled -> return
		if !pair.Enabled {
			return ack
		}

		balance := k.bankKeeper.GetBalance(ctx, recipient, coin.Denom)
		if err := k.ConvertCoinNativeERC20(ctx, pair, balance.Amount, common.BytesToAddress(recipient.Bytes()), recipient); err != nil {
			return channeltypes.NewErrorAcknowledgement(err)
		}

		// For now the only case we are interested in adding telemetry is a successful conversion.
		telemetry.IncrCounterWithLabels(
			[]string{types.ModuleName, "ibc", "on_recv", "total"},
			1,
			[]metrics.Label{
				telemetry.NewLabel("denom", coin.Denom),
				telemetry.NewLabel("source_channel", packet.SourceChannel),
				telemetry.NewLabel("source_port", packet.SourcePort),
			},
		)
	}

This is tested in v1
Note: Receiving ERC20 native coins via IBC is currently not supported in IBC v2, as it does not allow denoms containing /.

@zsystm
Copy link
Author

zsystm commented Apr 14, 2025

Overall, this looks good to me

  1. How about adding a test case to reach coverage this part as well?
  2. We also need to add test cases for the ICS20-related precompile, but since the removal of authz dependency is currently in progress, it seems more efficient to handle it after that change is completed.

@dongsam

Regarding your second point about adding test cases for the ICS20-related precompile:

  • Since the removal of the authz dependency is still in progress, I agree it would be more efficient to address the ICS20 test cases after that change is complete.
  • To keep this PR focused, I suggest we validate the core middleware logic at the e2e level here and handle the ICS20-related tests in a separate PR. This way, we can ensure clear scope separation and tackle the ICS20 precompile tests as a dedicated topic once the dependency removal is finalized.

What do you think about this approach?

@dongsam
Copy link
Member

dongsam commented Apr 14, 2025

@zsystm

already covered by existing test cases.

Ah, it seems like the coverage was mistakenly recognized as missing due to an issue with the measurement on my local environment. I confirmed that the breakpoint is hit and the coverage is reached in debug mode. 🙏

Regarding your second point about adding test cases for the ICS20-related precompile:

Yes, I agree that it would be better to handle the ICS20-related tests in a separate PR, as you suggested.

I’ll finish the review and proceed with the approval within today.

Copy link
Member

@dongsam dongsam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the main ibc middleware cases are covered.

I’m currently working separately on test cases for the ICS20 precompile's transfer related to this issue.

Once it’s complete, I’ll open a PR and request a review.

@zsystm zsystm merged commit e55937e into cosmos:feat/ibc-v10 Apr 14, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants