Callbacks Eureka #7934
Callbacks Eureka #7934
Conversation
|
|
||
| return IBCMiddleware{ | ||
| app: packetDataUnmarshalerApp, | ||
| writeAckWrapper: writeAckWrapper, |
There was a problem hiding this comment.
i allow writeAckWrapper to be nil and use chanKeeperV2 if so. cc: @srdtrk for feedback
There was a problem hiding this comment.
Lets force them to pass it twice
| TimeoutHeight: clienttypes.Height{}, | ||
| TimeoutTimestamp: 0, |
There was a problem hiding this comment.
timeout values zeroed out everywhere. So we preserve interface, but contracts should not rely on these values for v2 packets
| TimeoutTimestamp: 0, | ||
| } | ||
| return im.contractKeeper.IBCOnAcknowledgementPacketCallback( | ||
| cachedCtx, packetv1, acknowledgement, relayer, cbData.CallbackAddress, cbData.SenderAddress, payload.Version, |
There was a problem hiding this comment.
However, here acknowledgement callback just expects []byte
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Would also reccommend updating branch and rerunning the linter locally |
| // 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 | ||
| } | ||
|
|
There was a problem hiding this comment.
This probably shouldn't be part of the public API. Can we create an internal package that can be shared between v1 and v2?
|
|
||
| return IBCMiddleware{ | ||
| app: packetDataUnmarshalerApp, | ||
| writeAckWrapper: writeAckWrapper, |
There was a problem hiding this comment.
Lets force them to pass it twice
| } | ||
| // 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 { |
There was a problem hiding this comment.
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
| TimeoutTimestamp: 0, | ||
| } | ||
| return im.contractKeeper.IBCOnAcknowledgementPacketCallback( | ||
| cachedCtx, packetv1, acknowledgement, relayer, cbData.CallbackAddress, cbData.SenderAddress, payload.Version, |
There was a problem hiding this comment.
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
| sequence uint64, | ||
| ack channeltypesv2.Acknowledgement, | ||
| ) error { | ||
| packet, found := im.chanKeeperV2.GetAsyncPacket(ctx, clientID, sequence) |
| // 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] | ||
|
|
There was a problem hiding this comment.
somewhere before this we should assert packet.Payloads.length == 1. And link multi-payload packet tracking issue
|
|
||
| // PacketDataUnmarshaler defines an optional interface which allows a middleware | ||
| // to request the packet data to be unmarshaled by the base application. | ||
| type PacketDataUnmarshaler interface { |
There was a problem hiding this comment.
Does this need to carry the same name as the old interface? We can call it PayloadUnmarshaler
|
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.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.