Callbacks Eureka by AdityaSripal · Pull Request #7934 · cosmos/ibc-go

AdityaSripal · 2025-02-07T20:54:56Z

Description

The following is a cherrypick of relevant commits from #7927

Please pay close attention while reviewing this PR. It is recommended to review the ibc callbacks v1 middleware in parallel and ensure all equivalent checks and logic are being made

closes: #7855

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

modules/apps/callbacks/callbacks_test.go

AdityaSripal · 2025-02-07T20:56:54Z

modules/apps/callbacks/v2/ibc_middleware.go

+
+	return IBCMiddleware{
+		app:             packetDataUnmarshalerApp,
+		writeAckWrapper: writeAckWrapper,


i allow writeAckWrapper to be nil and use chanKeeperV2 if so. cc: @srdtrk for feedback

Lets force them to pass it twice

AdityaSripal · 2025-02-07T20:57:50Z

modules/apps/callbacks/v2/ibc_middleware.go

+			TimeoutHeight:      clienttypes.Height{},
+			TimeoutTimestamp:   0,


timeout values zeroed out everywhere. So we preserve interface, but contracts should not rely on these values for v2 packets

modules/apps/callbacks/v2/ibc_middleware.go

AdityaSripal · 2025-02-07T20:59:10Z

modules/apps/callbacks/v2/ibc_middleware.go

+			TimeoutTimestamp:   0,
+		}
+		return im.contractKeeper.IBCOnAcknowledgementPacketCallback(
+			cachedCtx, packetv1, acknowledgement, relayer, cbData.CallbackAddress, cbData.SenderAddress, payload.Version,


However, here acknowledgement callback just expects []byte

This is bad because old apps won't recognize the new error ack we have, we should consider if this is ok or we need to do something about this such as passing nil if we get universal error ack

Indeed, discussed and the plan is to document and keep as-is for now. We will check with confio team on what they would prefer

modules/apps/callbacks/v2/ibc_middleware.go

gjermundgaraba

The setup looks logical enough to me. I don't know the callbacks middleware to feel very confident without a proper walkthrough. So I guess we will do another internal audit where we can walk through this a bit more.
Until then, I want @srdtrk to do the main review of this PR.

modules/apps/callbacks/v2/ibc_middleware.go

gjermundgaraba · 2025-02-10T15:25:06Z

Would also reccommend updating branch and rerunning the linter locally

…k-callbacks-eureka

srdtrk

Looking good. I see some issues which I mentioned

srdtrk · 2025-02-10T17:56:00Z

modules/apps/callbacks/types/callbacks.go

+// ProcessCallback executes the callbackExecutor and reverts contract changes if the callbackExecutor fails.
+//
+// Error Precedence and Returns:
+//   - oogErr: Takes the highest precedence. If the callback runs out of gas, an error wrapped with types.ErrCallbackOutOfGas is returned.
+//   - panicErr: Takes the second-highest precedence. If a panic occurs and it is not propagated, an error wrapped with types.ErrCallbackPanic is returned.
+//   - callbackErr: If the callbackExecutor returns an error, it is returned as-is.
+//
+// panics if
+//   - the contractExecutor panics for any reason, and the callbackType is SendPacket, or
+//   - the contractExecutor runs out of gas and the relayer has not reserved gas grater than or equal to
+//     CommitGasLimit.
+func ProcessCallback(
+	ctx sdk.Context, callbackType CallbackType,
+	callbackData CallbackData, callbackExecutor func(sdk.Context) error,
+) (err error) {
+	cachedCtx, writeFn := ctx.CacheContext()
+	cachedCtx = cachedCtx.WithGasMeter(storetypes.NewGasMeter(callbackData.ExecutionGasLimit))
+
+	defer func() {
+		// consume the minimum of g.consumed and g.limit
+		ctx.GasMeter().ConsumeGas(cachedCtx.GasMeter().GasConsumedToLimit(), fmt.Sprintf("ibc %s callback", callbackType))
+
+		// recover from all panics except during SendPacket callbacks
+		if r := recover(); r != nil {
+			if callbackType == CallbackTypeSendPacket {
+				panic(r)
+			}
+			err = errorsmod.Wrapf(ErrCallbackPanic, "ibc %s callback panicked with: %v", callbackType, r)
+		}
+
+		// if the callback ran out of gas and the relayer has not reserved enough gas, then revert the state
+		if cachedCtx.GasMeter().IsPastLimit() {
+			if callbackData.AllowRetry() {
+				panic(storetypes.ErrorOutOfGas{Descriptor: fmt.Sprintf("ibc %s callback out of gas; commitGasLimit: %d", callbackType, callbackData.CommitGasLimit)})
+			}
+			err = errorsmod.Wrapf(ErrCallbackOutOfGas, "ibc %s callback out of gas", callbackType)
+		}
+
+		// allow the transaction to be committed, continuing the packet lifecycle
+	}()
+
+	err = callbackExecutor(cachedCtx)
+	if err == nil {
+		writeFn()
+	}
+
+	return err
+}
+


This probably shouldn't be part of the public API. Can we create an internal package that can be shared between v1 and v2?

srdtrk · 2025-02-10T17:59:48Z

modules/apps/callbacks/v2/ibc_middleware.go

+
+	return IBCMiddleware{
+		app:             packetDataUnmarshalerApp,
+		writeAckWrapper: writeAckWrapper,


Lets force them to pass it twice

srdtrk · 2025-02-10T18:09:13Z

modules/apps/callbacks/v2/ibc_middleware.go

+		}
+		// wrap the individual acknowledgement into the channeltypesv2.Acknowledgement since it implements the exported.Acknowledgement interface
+		var ack channeltypesv2.Acknowledgement
+		if recvResult.Status == channeltypesv2.PacketStatus_Failure {


Don't we return early if this is the case anyway?? So remove the if statement and only keep the else case. We don't make callbacks if an error ack occurred

srdtrk · 2025-02-10T18:13:24Z

modules/apps/callbacks/v2/ibc_middleware.go

+			TimeoutTimestamp:   0,
+		}
+		return im.contractKeeper.IBCOnAcknowledgementPacketCallback(
+			cachedCtx, packetv1, acknowledgement, relayer, cbData.CallbackAddress, cbData.SenderAddress, payload.Version,


This is bad because old apps won't recognize the new error ack we have, we should consider if this is ok or we need to do something about this such as passing nil if we get universal error ack

srdtrk · 2025-02-10T18:14:57Z

modules/apps/callbacks/v2/ibc_middleware.go

+	sequence uint64,
+	ack channeltypesv2.Acknowledgement,
+) error {
+	packet, found := im.chanKeeperV2.GetAsyncPacket(ctx, clientID, sequence)


srdtrk · 2025-02-10T18:17:57Z

modules/apps/callbacks/v2/ibc_middleware.go

+	// NOTE: use first payload as the payload that is being handled by callbacks middleware
+	// must reconsider if multipacket data gets supported with async packets
+	payload := packet.Payloads[0]
+


somewhere before this we should assert packet.Payloads.length == 1. And link multi-payload packet tracking issue

modules/core/api/module.go

srdtrk · 2025-02-10T18:28:51Z

modules/core/api/module.go

+
+// PacketDataUnmarshaler defines an optional interface which allows a middleware
+// to request the packet data to be unmarshaled by the base application.
+type PacketDataUnmarshaler interface {


Does this need to carry the same name as the old interface? We can call it PayloadUnmarshaler

Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

sonarqubecloud · 2025-02-10T20:16:54Z

Quality Gate passed for 'ibc-go'

Issues
82 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

srdtrk

Let's get this merged

AdityaSripal added 12 commits February 7, 2025 15:39

callbacks middleware compatible for eureka

be8cdee

convert v2 packets to v1 to preserve contractKeeper interface

a9c28f5

remove ContractKeeperV2 interface

bcc4b36

pass in channeltypesv2.Acknowledgement

afa88ab

testing progress

e3eab32

start sendpacket test pass

7d6f7dd

test events

bb73c5f

acknowledgement tests

bfea28d

fix timeout tests

bc83db1

fix recvPacket test

ec65620

fix writeack tests

0dcc001

fix writeack fail testcase

9403192

AdityaSripal requested review from gjermundgaraba and srdtrk as code owners February 7, 2025 20:54

AdityaSripal commented Feb 7, 2025

View reviewed changes

lint

1d22ea9

This was referenced Feb 7, 2025

Callbacks Eureka Conversion #7927

Closed

Eureka-compatible Callbacks Middleware #7899

Closed

AdityaSripal added 2 commits February 7, 2025 17:21

lint moar

0c27be8

lint with tool finally

c067db3

gjermundgaraba reviewed Feb 10, 2025

View reviewed changes

modules/apps/callbacks/v2/ibc_middleware.go Show resolved Hide resolved

modules/apps/callbacks/v2/ibc_middleware.go Outdated Show resolved Hide resolved

AdityaSripal added 2 commits February 10, 2025 12:01

Merge branch 'main' of github.com:cosmos/ibc-go into aditya/cherrypic…

684fe73

…k-callbacks-eureka

update and lint

b0e0aaf

srdtrk reviewed Feb 10, 2025

View reviewed changes

AdityaSripal and others added 3 commits February 10, 2025 15:02

address serdar review

e93d5e1

Update modules/core/api/module.go

348fe72

Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

Merge branch 'main' into aditya/cherrypick-callbacks-eureka

170ccfe

AdityaSripal enabled auto-merge February 10, 2025 20:09

srdtrk approved these changes Feb 10, 2025

View reviewed changes

AdityaSripal added this pull request to the merge queue Feb 10, 2025

Merged via the queue into main with commit 5ee5783 Feb 10, 2025
68 of 69 checks passed

AdityaSripal deleted the aditya/cherrypick-callbacks-eureka branch February 10, 2025 22:44

Conversation

AdityaSripal commented Feb 7, 2025

Description

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!

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!

gjermundgaraba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gjermundgaraba commented Feb 10, 2025

Uh oh!

srdtrk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sonarqubecloud bot commented Feb 10, 2025

Quality Gate passed for 'ibc-go'

Uh oh!

srdtrk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

srdtrk left a comment •

edited

Loading