EVM Callbacks #150

AdityaSripal · 2025-05-16T13:27:33Z

Description

TODO for EVM team:

Approve Receiver funds from isolated address to contract onRecvPacket
Build successful test case to prove that EVM call works
Ensure that state is properly reverted when a packet is genuinely sent t the chain and the callback fails

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

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

AdityaSripal · 2025-05-16T13:39:22Z

x/vm/statedb/statedb.go

@@ -111,7 +111,7 @@ func (s *StateDB) MultiStoreSnapshot() storetypes.CacheMultiStore {
 	// the cacheCtx multi store is already a CacheMultiStore
 	// so we need to pass a copy of the current state of it
 	cms := s.cacheCtx.MultiStore().(storetypes.CacheMultiStore)
-	snapshot := cms.Copy()
+	snapshot := cms.CacheMultiStore()


Had to change this to fix the SDK upgrade. Not sure the correct way to fix this

This should fixed on main already

x/ibc/callbacks/keeper/keeper.go

AdityaSripal · 2025-05-16T14:25:33Z

x/ibc/callbacks/keeper/keeper.go

+
+	// can only call callback if the receiver is the module address
+	receiver := sdk.MustAccAddressFromBech32(data.Receiver)
+	if !receiver.Equals(k.authKeeper.GetModuleAddress(types.ModuleName)) {


Only continue callback logic on recvPacket if the receiver of the tokens is the module account address. This is because we set the msg.sender on the contract callback to the module address

AdityaSripal · 2025-05-16T14:57:25Z

x/ibc/callbacks/keeper/keeper.go

+	contractAddr := common.HexToAddress(contractAddress)
+
+	// TODO: Do something with the response
+	_, err = k.evmKeeper.CallEVMWithData(cachedCtx, sender, &contractAddr, cbData.Calldata, true)


Here the contract is getting the calldata in the packet regardless of what the acknowledgment is. Question: Should we allow packet senders to provide different calldata for successful and unsuccessful acknowledgements

I assume people will need different behaviors to either finalize or unwind application logic here, but we should get some more product feedback around exactly what is required since I don't have a clear picture of what that would look like.

What currently sets the calldata used here? Is it the contract that handles the receive packet?

This is all being set by the original packet sender at the start of the packet lifecycle

Hmm maybe I'm missing something, but if the source calldata is chosen prior to packet creation, how does a contract determine for which packet this source callback is associated with? For example, our entrypoint contract has a usecase where when handling a timed out or failed transfer, it will send the recovered assets to an address that is stored in state with an association to the packet seq no / channel of the transfer. How would our entrypoint contract determine what packet is failing in the evm callbacks case?

AdityaSripal · 2025-05-16T14:59:23Z

x/ibc/callbacks/keeper/keeper.go

+	return nil
+}
+
+func (k ContractKeeper) IBCOnAcknowledgementPacketCallback(


Note: Here we can use the packetsenderAddress safely as msg.sender in AcknowledgePacket and TimeoutPacket

To clarify, we can use msg.sender here because we assume there will be no logic around who is sending the ack or timeout?

Not sure I understand this question

I don't think there's a question here. It seems like msg.sender is not a "trustable" field. So we can't actually use this field for anything, which is why it seems prudent to have that field be the hashed value of something.

Here we do not want the packet sender to spoof a msg.sender on the receiving side

But on the source side, we can allow the packet sender to approve contract executions on behalf of himself. So there is no need for a hashed value here imo

AdityaSripal · 2025-05-16T15:00:01Z

x/ibc/callbacks/keeper/keeper.go

+	contractAddr := common.HexToAddress(contractAddress)
+
+	// TODO: Do something with the response
+	_, err = k.evmKeeper.CallEVMWithData(cachedCtx, sender, &contractAddr, cbData.Calldata, true)


If we support different success and failure calldata for acknowledgement, we can use the failure calldata here as well.

Eric-Warehime · 2025-05-19T17:48:48Z

x/ibc/callbacks/keeper/keeper.go

+	return nil
+}
+
+func (k ContractKeeper) IBCOnAcknowledgementPacketCallback(


To clarify, we can use msg.sender here because we assume there will be no logic around who is sending the ack or timeout?

x/ibc/callbacks/keeper/keeper.go

Eric-Warehime · 2025-05-19T17:55:01Z

x/vm/statedb/statedb.go

@@ -111,7 +111,7 @@ func (s *StateDB) MultiStoreSnapshot() storetypes.CacheMultiStore {
 	// the cacheCtx multi store is already a CacheMultiStore
 	// so we need to pass a copy of the current state of it
 	cms := s.cacheCtx.MultiStore().(storetypes.CacheMultiStore)
-	snapshot := cms.Copy()
+	snapshot := cms.CacheMultiStore()


This should fixed on main already

Eric-Warehime · 2025-05-19T17:56:05Z

ante/interfaces/cosmos.go

+	UnorderedTransactionsEnabled() bool
+	RemoveExpiredUnorderedNonces(ctx sdk.Context) error
+	TryAddUnorderedNonce(ctx sdk.Context, sender []byte, timestamp time.Time) error


Why do we now depend on unordered transactions? I don't see it being used in this PR.

Eric-Warehime · 2025-05-19T17:57:24Z

x/ibc/callbacks/keeper/keeper.go

+	contractAddr := common.HexToAddress(contractAddress)
+
+	// TODO: Do something with the response
+	_, err = k.evmKeeper.CallEVMWithData(cachedCtx, sender, &contractAddr, cbData.Calldata, true)


I assume people will need different behaviors to either finalize or unwind application logic here, but we should get some more product feedback around exactly what is required since I don't have a clear picture of what that would look like.

AdityaSripal

Self-review to create TODOs and further explanations

AdityaSripal · 2025-05-30T18:28:33Z

contracts/solidity/evmd/eips/testdata/Counter.sol

All of these files get deleted if you call:

make contracts-all

If you want to add them back, you can just call:

make contracts-compile

AdityaSripal · 2025-05-30T18:30:09Z

evmd/app.go

 			- IBC Transfer

 		SendPacket, since it is originating from the application to core IBC:
-		 	transferKeeper.SendPacket -> erc20.SendPacket -> channel.SendPacket
+		 	transferKeeper.SendPacket -> callbacks.SendPacket -> erc20.SendPacket -> channel.SendPacket


The SendPacket flow doesn't happen this way because the ICS4 stack is not wired correctly.

ERC20Keeper must implement ICS4Wrapper

and then we can wire this correctly.

It doesn't have an effect at the moment, but we can make this fix later

AdityaSripal · 2025-05-30T18:30:27Z

evmd/app.go

 	*/

 	// create IBC module from top to bottom of stack
 	var transferStack porttypes.IBCModule

 	transferStack = transfer.NewIBCModule(app.TransferKeeper)
+	maxCallbackGas := uint64(1_000_000)


Not used in EVM contractKeeper

AdityaSripal · 2025-05-30T18:31:13Z

precompiles/callbacks/ICallbacks.sol

+// SPDX-License-Identifier: LGPL-3.0-only
+pragma solidity >=0.8.18;
+
+interface ICallbacks {


Here is the ABI interface for source side callbacks. Looking for feedback here

AdityaSripal · 2025-05-30T18:31:54Z

precompiles/callbacks/callbacks.go

This file ended up being unnecessary. It was generated by abigen. We can remove if desired

AdityaSripal · 2025-05-30T18:34:20Z

x/ibc/callbacks/keeper/keeper.go

+	return nil
+}
+
+func (k ContractKeeper) IBCOnAcknowledgementPacketCallback(


Here we do not want the packet sender to spoof a msg.sender on the receiving side

But on the source side, we can allow the packet sender to approve contract executions on behalf of himself. So there is no need for a hashed value here imo

AdityaSripal · 2025-05-30T19:42:46Z

x/ibc/callbacks/keeper/keeper.go

+	receiver := sdk.MustAccAddressFromBech32(data.Receiver)
+	isolatedAddr := types.GenerateIsolatedAddress(packet.GetDestChannel(), data.Sender)


This should be converted into an ETH address but not sure exactly how this should work.

Added: 5b01591

AdityaSripal · 2025-05-30T19:43:23Z

x/ibc/callbacks/keeper/keeper.go

+	ethReceiver := common.BytesToAddress(receiver)
+	contractAddr := common.HexToAddress(contractAddress)
+
+	// TODO: Approve the ERC20 tokens in the transfer packet for the contract on behalf of the receiver


Wasn't sure how this should happen in cosmos/evm so I left as a TODO now

Added: 5b01591

AdityaSripal · 2025-05-30T19:43:47Z

x/ibc/callbacks/keeper/keeper_test.go

+		{
+			"success",
+			func() {},
+			nil, // TODO: This is unexpected. Not sure why this is passing without a valid contract address


I don't understand why the EVM didn't throw an error here

If you send a transaction with packed data to an address that has no code, the transaction will still succeed, but the data will be ignored.

Added a check that fails if there's no code at the contract account.

AdityaSripal · 2025-05-30T19:44:02Z

x/ibc/callbacks/keeper/keeper_test.go

+		{
+			"success",
+			func() {},
+			nil, // TODO: This is unexpected. Not sure why this is passing without a valid contract address


I don't understand why this doesn't throw an error in EVM

If you send a transaction with packed data to an address that has no code, the transaction will still succeed, but the data will be ignored.

Added a check that fails if there's no code at the contract account.

AdityaSripal · 2025-06-03T14:01:56Z

x/ibc/callbacks/keeper/keeper.go

+	// before calling the callback.
+
+	// TODO: Do something with the response
+	_, err = k.evmKeeper.CallEVMWithData(cachedCtx, ethReceiver, &contractAddr, cbData.Calldata, true)


TODO: Check if EVM reversion matches SDK transaction expectations:

If the EVM tx reverts, does the EVM state also revert.

If the SDK transaction fails at a later point after a successful EVM call, does the EVM state revert along with the Cosmos state

AdityaSripal · 2025-06-03T14:13:01Z

x/ibc/callbacks/keeper/keeper.go

+
+type ContractKeeper struct {
+	authKeeper            types.AccountKeeper
+	evmKeeper             types.EVMKeeper


TODO: We need to fail if the contract address has no code before calling into EVM since this will fail silently in the EVM side

Added a check that fails if there's no code at the contract account.

Create ERC20 precompiles for tokens that have more than 1 hop

- checks if erc20 exists (should exist because this callback is after the erc20 callback) - sets allowance for ibc'd erc20 for the full transfer amount to the contract on behalf of the isolated address

fails out if the contract address provided has no code. this prevents silent successes if the contract address is an EOA

include erc20 keeper

and fix tests

x/ibc/callbacks/keeper/keeper.go

zsystm · 2025-06-09T05:58:59Z

x/erc20/keeper/ibc_callbacks.go

@@ -111,7 +111,7 @@ func (k Keeper) OnRecvPacket(
 	// 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):
+	case !found && strings.HasPrefix(coin.Denom, "ibc/"):


@vladjdk @AdityaSripal
Could you clarify why this condition was removed?
If we're removing it intentionally, we should also update the comments in Case 1 to reflect the change.

Let's update the docs here. I don't think it makes sense to only accept single hop tokens, given the fact that Eureka routes through the Hub and recipient chains may be using Eureka.

x/ibc/callbacks/README.md

Eric-Warehime · 2025-06-09T18:45:43Z

precompiles/erc20/erc20.go

-	GasTotalSupply       = 2_477
-	GasBalanceOf         = 2_851
-	GasAllowance         = 3_246
+	GasTotalSupply       = 2_480
+	GasBalanceOf         = 2_870
+	GasAllowance         = 3_225


Why are we using more gas for these now?

Eric-Warehime · 2025-06-09T18:46:37Z

precompiles/erc20/erc20.go

-	GasTransfer          = 3_000_000
-	GasApprove           = 30_956
-	GasIncreaseAllowance = 34_605
-	GasDecreaseAllowance = 34_519
+	// NOTE: These gas values have been derived from tests that have been concluded on a testing branch, which
+	// is not being merged to the main branch. The reason for this was to not clutter the repository with the
+	// necessary tests for this use case.
+	//
+	// The results can be inspected here:
+	// https://github.com/evmos/evmos/blob/malte/erc20-gas-tests/precompiles/erc20/plot_gas_values.ipynb
+
+	GasTransfer          = 9_000
+	GasTransferFrom      = 30_500
+	GasApprove           = 8_100
+	GasIncreaseAllowance = 8_580
+	GasDecreaseAllowance = 3_620


Why are these values changing so much?

Based on this: evmos/evmos#2142

Malte was saying that he missed this in Evmos/OS when he was cherry picking commits

Eric-Warehime · 2025-06-09T23:28:11Z

x/ibc/callbacks/keeper/keeper.go

+	cachedCtx = evmante.BuildEvmExecutionCtx(cachedCtx).
+		WithGasMeter(types2.NewInfiniteGasMeterWithLimit(cbData.CommitGasLimit))
+
+	receiver := sdk.MustAccAddressFromBech32(data.Receiver)


Why is it ok to panic here?

Maybe because data.Receiver has already been validated somewhere before this in the stack?

Not ok - we should error out instead

Eric-Warehime · 2025-06-09T23:29:31Z

x/ibc/callbacks/keeper/keeper.go

+		return errorsmod.Wrap(types.ErrInvalidCalldata, "timeout callback data should not contain calldata")
+	}
+
+	sender := common.BytesToAddress(sdk.MustAccAddressFromBech32(packetSenderAddress))


Also panics here?

Same as above

Eric-Warehime · 2025-06-10T00:03:43Z

x/ibc/callbacks/keeper/keeper.go

+	var allowance *big.Int
+	err = erc20.ABI.UnpackIntoInterface(&allowance, "allowance", res.Ret)
+	if err != nil {
+		return errorsmod.Wrapf(types.ErrAllowanceFailed, "failed to unpack allowance: %v", err)
+	}
+
+	if allowance.Cmp(big.NewInt(1)) != 0 {
+		return errorsmod.Wrapf(types.ErrAllowanceFailed, "failed to set allowance")
+	}


I think this is really confusing to read. It's not clear to me that the allowance value being returned here is actually a boolean for success and not the actual allowance value being set by the approve call.

Are we not able to actually unpack to a bool here instead?

Let's unpack a bool and rename it to success. The naming is an artifact of me initially believing that you got an allowance amount back, and then changing the check after I realized it was a success/fail

Eric-Warehime · 2025-06-10T00:13:41Z

x/ibc/callbacks/keeper/keeper.go

+	// Check that the contract has received all the tokens.
+	// NOTE: contracts must implement an IERC20(token).transferFrom(msg.sender, address(this), amount)
+	// for the total amount, or the callback will fail.
+	// This check is here to prevent funds from getting stuck in the isolated address,
+	// since they would become irretrievable.
+	contractTokenBalance := k.erc20Keeper.BalanceOf(ctx, erc20.ABI, tokenPair.GetERC20Contract(), contractAddr) // here, we can use the original ctx and skip manually adding the gas
+	if contractTokenBalance.Cmp(amountInt.BigInt()) != 0 {
+		return errorsmod.Wrapf(erc20types.ErrEVMCall, "contract balance %d does not equal sent amount %d", contractTokenBalance, amountInt.BigInt())
+	}


How does this work if the callback contract already had tokens of this denom before the callback, or if the callback has already forwarded the tokens out to another account? I think you just need to check that the sender account has no tokens in it rather than assuming anything about the state of the callback contract.

Good call - current code would only allow callbacks on a contract one time and would break afterwards

x/ibc/callbacks/keeper/keeper.go

Co-authored-by: Eric Warehime <[email protected]> Co-authored-by: Hyunwoo Lee <[email protected]>

AdityaSripal added 2 commits May 15, 2025 16:29

start building

c6d663e

do packet callbacks

147dee1

AdityaSripal commented May 16, 2025

View reviewed changes

Eric-Warehime reviewed May 19, 2025

View reviewed changes

vladjdk mentioned this pull request May 21, 2025

[EPIC] IBC Callbacks in EVM #19

Open

AdityaSripal added 8 commits May 29, 2025 17:27

push change sources side to have specific callbacks

dcb3550

cleanup

bdb054f

add documentation

add8c3f

start testing framework

bf1cc7e

fix merge

9aaa61b

finish up test framework

e5c9d57

better error testing

3b245be

success test cases

24d54e9

AdityaSripal commented May 30, 2025

View reviewed changes

fix address issue

95ecd45

AdityaSripal commented Jun 3, 2025

View reviewed changes

vladjdk and others added 13 commits June 3, 2025 14:11

Allow multihop tokens

c49542c

Create ERC20 precompiles for tokens that have more than 1 hop

Merge branch 'main' into aditya/evm-callbacks

e7d87f1

set allowance on erc20

5b01591

- checks if erc20 exists (should exist because this callback is after the erc20 callback) - sets allowance for ibc'd erc20 for the full transfer amount to the contract on behalf of the isolated address

fail if contract address is not contract

71006a9

fails out if the contract address provided has no code. this prevents silent successes if the contract address is an EOA

fix initialization in app.go

de933ce

include erc20 keeper

check for contract in timeout and ack

52324ed

and fix tests

use remaining context gas as max gas

8345eea

re-add compiled contracts

5e37567

lints

348283b

md lints

b18796b

wip: happy path testing

96c92b9

change middleware order

6c7af90

update erc20 gas

596d7c2

vladjdk added 4 commits June 5, 2025 23:43

fix call

c3f3050

wip: tests, failing out of gas every evm call

a121e91

wip: tests, failing out of gas every evm call

1233ed3

change param ordering

1b89cbb

github-advanced-security bot found potential problems Jun 6, 2025

View reviewed changes

x/ibc/callbacks/keeper/keeper.go Fixed Show fixed Hide fixed

vladjdk added 6 commits June 6, 2025 13:32

Happy path testing + lints

a88c1b2

uncomment previous test

a0c814a

clean up

0375f35

gas handling and docs

bf7cf35

add tests for recv, ack, timeout

e00c698

lints

e4c2a63

vladjdk marked this pull request as ready for review June 7, 2025 02:21

vladjdk added 2 commits June 6, 2025 22:40

small fixes

2d83bcb

fix errors in testcase after update

a201741

zsystm reviewed Jun 9, 2025

View reviewed changes

x/ibc/callbacks/README.md Outdated Show resolved Hide resolved

Eric-Warehime reviewed Jun 10, 2025

View reviewed changes

Eric-Warehime and others added 4 commits June 9, 2025 18:17

Rename approve output decoding

286190b

Check receiver tokens instead of callback contract

2802611

Don't panic in ibc middleware

66f34f0

Apply suggestions from code review

5cac51b

Co-authored-by: Eric Warehime <[email protected]> Co-authored-by: Hyunwoo Lee <[email protected]>

Eric-Warehime approved these changes Jun 10, 2025

View reviewed changes

Eric-Warehime merged commit 5cfe96d into main Jun 10, 2025
17 checks passed

Eric-Warehime deleted the aditya/evm-callbacks branch June 10, 2025 15:23

		receiver := sdk.MustAccAddressFromBech32(data.Receiver)
		isolatedAddr := types.GenerateIsolatedAddress(packet.GetDestChannel(), data.Sender)

EVM Callbacks #150

EVM Callbacks #150

Uh oh!

Conversation

AdityaSripal commented May 16, 2025 • edited by zsystm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Author Checklist

Reviewers Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdityaSripal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AdityaSripal commented May 16, 2025 •

edited by zsystm

Loading