Spike error ack #1299

alpe · 2023-03-27T14:56:52Z

Spike that covers:

by the VM

alpe · 2023-03-29T09:16:54Z

x/wasm/ibc.go

+		// the `NewErrorAcknowledgement` redacts the error message, it is
+		// recommended by the ibc-module devs to log the raw error as event:
+		ctx.EventManager().EmitEvent(
+			sdk.NewEvent(


Should we always emit an event on rcv or on only in the error case?

What about 2 events:

EventTypePacketRecv: we receive a packet

EventTypePacketRecvContractErr: we got a contract error which is turned into an error ack

While EventTypePacketRecvContractErr is an error, the absence of it does not mean success.

This was also inspired by ics-20 and ics-27 . They use a single event with additional error attribute when set.

alpe · 2023-03-29T09:17:24Z

x/wasm/ibc.go

+		// recommended by the ibc-module devs to log the raw error as event:
+		ctx.EventManager().EmitEvent(
+			sdk.NewEvent(
+				types.EventTypePacketRecv,


fyi: the event name is "ibc-acknowledgement-error" on Osmosis

webmaster128

Good stuff. Some first thoughs here.

webmaster128 · 2023-03-30T14:16:54Z

x/wasm/keeper/relay.go

+	// success ACK
+	return ContractConfirmStateAck(data), nil


I think this code or comment is misleading because it can be either a success or and error or a non-binary acknowledgement type. We should not try to give it any meaning.

On packet receive, success can be defined as case 1 + 2 of #697 (comment) . The contract was able to fully handle the packet. State will be committed.

webmaster128 · 2023-03-30T14:27:49Z

x/wasm/keeper/relay.go

+
+func (w ContractConfirmStateAck) Success() bool {
+	return true // always commit state
+}


Is there any documentation what Success() in the ibcexported.Acknowledgement means?

The best source for Success() that I found is the code that calls this in ibc-go.

I raised the issue here: cosmos/ibc-go#3434

webmaster128 · 2023-03-30T14:34:44Z

x/wasm/ibc.go

@@ -267,23 +268,38 @@ func (i IBCHandler) OnRecvPacket(
 		return channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(err, "contract port id"))


Is this something where we can provide a better error message than the redacted error too?

Good 👁️ ! Unless we touch our contract-to-port mapping we should never run into this case. Either we panic here or we need to return the error event, too.
I would prefer panic to make clear in our code that this must not happen. This is not ideal as the packet may be re-submitted until timeout but it is not supposed to happen

webmaster128 · 2023-03-30T14:37:08Z

x/wasm/ibc.go

+			if types.IsAcceptedEventOnRecvPacketErrorAck(e.Type) {
+				ctx.EventManager().EmitEvent(e)
+			}
+		}


Do we really want to preserve those events? I might be messing something but to me it's clearer if we discard them and add all relevent information in the error event below.

In a more complex contract-calls-contracts environment, the event trace may have revealed some infos to track the root cause of an error. But there are some costs of maintaining this in the future.
👌 let's drop them and keep things simple.

webmaster128 · 2023-03-30T14:47:43Z

x/wasm/ibc.go

+		// the `NewErrorAcknowledgement` redacts the error message, it is
+		// recommended by the ibc-module devs to log the raw error as event:
+		ctx.EventManager().EmitEvent(
+			sdk.NewEvent(


What about 2 events:

EventTypePacketRecv: we receive a packet

EventTypePacketRecvContractErr: we got a contract error which is turned into an error ack

While EventTypePacketRecvContractErr is an error, the absence of it does not mean success.

webmaster128 · 2023-03-30T14:48:44Z

x/wasm/ibc.go

+			types.EventTypePacketRecv,
+			sdk.NewAttribute(types.AttributeKeyContractAddr, contractAddr.String()),
+			sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
+			sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),


We don't know if this us a success or not. It's just bytes from contract that can mean anything.

Should be sdk.NewAttribute(types.AttributeKeyAckSuccess, "true"), instead

This attribute was inspired by the ics-20 impl. At this stage we do know that the ibc packet was successfully handled by the contract (and any sub-messages succeeded). The state will be committed and the Ack data stored.

* main: Bump bufbuild/buf-setup-action from 1.15.1 to 1.16.0 Use gov v1 instead of v1beta1 for new proposals (#1269) Update OnRecvPacket method to panic when an error is returned (#1298) Bump actions/checkout from 3.4.0 to 3.5.0 Add source for Wasm magic number

alpe · 2023-03-31T09:25:12Z

Thanks for the feedback! 💐
I have added the EmitAcknowledgementEvent function to reduce some duplication. It is inspired by code in ics-27

Spike error ack - Updates

alpe · 2023-04-21T09:32:10Z

Implemented in #1353

pinosu and others added 2 commits March 27, 2023 14:57

Update OnRecvPacket method to panic when an error is returned

03c264c

by the VM

Spike on packet recieve unhappy paths

610c677

alpe added the spike Demo to showcase an idea. label Mar 27, 2023

alpe marked this pull request as draft March 27, 2023 14:57

alpe commented Mar 29, 2023

View reviewed changes

webmaster128 reviewed Mar 30, 2023

View reviewed changes

alpe added 2 commits March 31, 2023 11:12

Updates

a59ccb8

fix tests

2df007f

webmaster128 mentioned this pull request Apr 11, 2023

Define what Success() in the Acknowledgement interface means cosmos/ibc-go#3434

Closed

Merge pull request #1313 from CosmWasm/spike_error_ack_updates

dbc3483

Spike error ack - Updates

alpe mentioned this pull request Apr 21, 2023

Redesign IBC on packet recv error/ result.Err handling #1353

Merged

alpe closed this Apr 21, 2023

alpe deleted the spike_error_ack branch April 21, 2023 09:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spike error ack #1299

Spike error ack #1299

alpe commented Mar 27, 2023

alpe Mar 29, 2023

webmaster128 Mar 30, 2023

alpe Mar 31, 2023

alpe Mar 29, 2023

webmaster128 left a comment

webmaster128 Mar 30, 2023

alpe Mar 31, 2023 •

edited

Loading

webmaster128 Mar 30, 2023

alpe Mar 31, 2023

webmaster128 Apr 11, 2023

webmaster128 Mar 30, 2023

alpe Mar 31, 2023 •

edited

Loading

webmaster128 Mar 30, 2023

alpe Mar 31, 2023

webmaster128 Mar 30, 2023

webmaster128 Mar 30, 2023

alpe Mar 31, 2023 •

edited

Loading

alpe commented Mar 31, 2023

alpe commented Apr 21, 2023

		@@ -267,23 +268,38 @@ func (i IBCHandler) OnRecvPacket(
		return channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(err, "contract port id"))

Spike error ack #1299

Spike error ack #1299

Conversation

alpe commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webmaster128 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpe Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpe Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpe Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

alpe commented Mar 31, 2023

alpe commented Apr 21, 2023

alpe Mar 31, 2023 •

edited

Loading

alpe Mar 31, 2023 •

edited

Loading

alpe Mar 31, 2023 •

edited

Loading