Add async support to wasm hooks #5072

nicolaslara · 2023-05-03T17:20:06Z

What is the purpose of the change

Packet Froward middleware introduced "async" acknowledgements in IBC: when onRecvPacket runs, the ack doesn't need to be written to state right away. Instead, that function can return nil and someone else (ideally the same module that received the packet) will write the acknowledgement later. If this doesn't happen the packet will timeout.

We want to add support for this in ibc wasm hooks so that whitelisted contracts (specifically XCS) can do this and behave similarly to PFM. This will provide a better UX when interacting with contracts that forward tokens.

Brief Changelog

Added mechanism for contracts called via wasm hooks to emit acks asynchronously

Testing and Verifying

Added test

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

github-actions · 2023-05-20T00:01:57Z

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

nicolaslara · 2023-05-26T10:05:33Z

x/ibc-hooks/README.md

+
+---
+
+TODO: I think this is overcomplicated and I can't remember why I wrote it like this. 


@ValarDragon do you have any opinions on the design alternatives? (more info below this line and in the readme in general)

I'm probably going to leave the implementation as-is right now to avoid increasing the surface area to test, and we can iterate on unifying this into one message later on.

nicolaslara · 2023-05-26T10:05:51Z

x/ibc-hooks/wasm_hook.go

+	if err == nil {
+		// If unmarshalling succeeds, the contract is requesting for the ack to be async.
+		if asyncAckRequest.IsAsyncAck { // in which case IsAsyncAck is expected to be set to true
+			// TODO: Do we want to check that only whitelisted contracts can do AsyncAcks?


@ValarDragon also here. Any thoughts?

Asking to learn: why do we want to whitelist this functionality?

Discussed offline, whitelist makes sense to me

p0mvn

Only a high-level pass to acquire context with nits. Will do a more detailed pass in a subsequent review

cosmwasm/contracts/crosschain-swaps/src/execute.rs

tests/ibc-hooks/testutils/contracts/echo/src/ibc.rs

p0mvn · 2023-05-29T13:14:13Z

x/ibc-hooks/types/msgs.go

+func (m MsgEmitIBCAck) ValidateBasic() error {
+	_, err := sdk.AccAddressFromBech32(m.Sender)
+	if err != nil {
+		return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid sender address (%s)", err)
+	}
+	return nil
+}


Does it make sense to do additional validation of packet sequence and channel here?

I don't think we do. The packet sequence is an uint64, and the channel can be any string (even if they generally are channel-<number>

p0mvn · 2023-05-29T13:20:00Z

x/ibc-hooks/wasm_hook.go

+	if err == nil {
+		// If unmarshalling succeeds, the contract is requesting for the ack to be async.
+		if asyncAckRequest.IsAsyncAck { // in which case IsAsyncAck is expected to be set to true
+			// TODO: Do we want to check that only whitelisted contracts can do AsyncAcks?


Asking to learn: why do we want to whitelist this functionality?

x/ibc-hooks/wasm_hook.go

x/ibc-hooks/README.md

p0mvn

Great work! Was super interesting to learn about this.

I'm feeling pretty good about this change after stepping through the TestWasmHooksAsyncAcks test with a debugger.

However, I think it would be good to have another approval still

x/ibc-hooks/README.md

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

nicolaslara · 2023-07-17T12:56:11Z

This is ready for final review and merge

github-actions · 2023-08-01T00:02:32Z

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

ValarDragon

Great job! Feel terrible for how long this took me to review

* added initial async ack support for wasm hooks * added initial working implementation of async acks * removed debug prints * cleanner acks * Update x/ibc-hooks/README.md Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * added use case to readme * clippy * clippy * updated hooks to latest * added changelog * updated deps * added proto fix and todo * added initial implementation of ack errors * make protos * better readme * cleanup rust structs * initial params * fixed test * updated async acks based on feedback. Safer this way * add error ack test * added missing types * updated ibc-hooks * tidy * updated osmoutils * updated osmoutils and ibc-hooks after merge * merge main * regen proto * update osmoutils * update ibc hooks * run go get * rl bytecode --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Adam Tucker <adamleetucker@outlook.com>

nicolaslara added 2 commits May 3, 2023 19:09

added initial async ack support for wasm hooks

dc47387

added initial working implementation of async acks

45833e8

github-actions bot added the C:app-wiring Changes to the app folder label May 5, 2023

removed debug prints

447d8f1

github-actions bot added the Stale label May 20, 2023

nicolaslara removed the Stale label May 20, 2023

nicolaslara mentioned this pull request May 22, 2023

[v16]: Scoping issue #4700

Closed

nicolaslara self-assigned this May 22, 2023

p0mvn self-assigned this May 23, 2023

cleanner acks

b872a7d

github-actions bot added the C:docs Improvements or additions to documentation label May 26, 2023

Merge branch 'main' into nicolas/async-hooks

62c6097

nicolaslara marked this pull request as ready for review May 26, 2023 10:03

nicolaslara commented May 26, 2023

View reviewed changes

p0mvn reviewed May 29, 2023

View reviewed changes

p0mvn approved these changes May 29, 2023

View reviewed changes

ValarDragon reviewed May 30, 2023

View reviewed changes

x/ibc-hooks/README.md Show resolved Hide resolved

nicolaslara and others added 3 commits May 31, 2023 10:15

Update x/ibc-hooks/README.md

cd1e9ea

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

added use case to readme

67b59a4

clippy

07446e7

nicolaslara added the V:state/breaking State machine breaking PR label May 31, 2023

nicolaslara added 6 commits May 31, 2023 11:11

clippy

415a40c

updated hooks to latest

fa9d044

added changelog

25cf51a

updated deps

9be127c

added proto fix and todo

d3f7b1e

added initial implementation of ack errors

5aebc19

nicolaslara mentioned this pull request Jun 7, 2023

[EPIC] On-chain IBC routing #4344

Open

nicolaslara added 9 commits July 4, 2023 16:46

updated async acks based on feedback. Safer this way

fbb90dc

add error ack test

ef9bba1

Merge branch 'main' into nicolas/async-hooks

eea8083

added missing types

b6208c3

updated ibc-hooks

0ef4921

tidy

1c7208d

updated osmoutils

f056cfc

Merge branch 'main' into nicolas/async-hooks

c28fbec

updated osmoutils and ibc-hooks after merge

36ab0d6

p0mvn mentioned this pull request Jul 18, 2023

V17: Scoping Issue #5191

Closed

github-actions bot added the Stale label Aug 1, 2023

github-actions bot closed this Aug 1, 2023

czarcas7ic reopened this Aug 1, 2023

github-actions bot removed the Stale label Aug 2, 2023

ValarDragon approved these changes Aug 7, 2023

View reviewed changes

czarcas7ic and others added 8 commits August 7, 2023 13:16

Merge branch 'main' into nicolas/async-hooks

e3b3537

merge main

325906e

regen proto

01fa42d

update osmoutils

0ae9d16

update ibc hooks

16c217d

run go get

bd56132

Merge branch 'main' into nicolas/async-hooks

8b472a9

rl bytecode

8f54f66

czarcas7ic merged commit f94e6e1 into main Aug 7, 2023
1 check passed

czarcas7ic deleted the nicolas/async-hooks branch August 7, 2023 19:53

github-actions bot mentioned this pull request Feb 1, 2024

Broken Links Detected #7399

Closed

github-actions bot mentioned this pull request May 1, 2024

Broken Links Detected #8183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add async support to wasm hooks #5072

Add async support to wasm hooks #5072

nicolaslara commented May 3, 2023 •

edited

github-actions bot commented May 20, 2023

nicolaslara May 26, 2023

nicolaslara May 31, 2023

nicolaslara May 26, 2023

p0mvn May 29, 2023

p0mvn May 29, 2023

p0mvn left a comment

p0mvn May 29, 2023

nicolaslara May 31, 2023

p0mvn May 29, 2023

p0mvn left a comment

nicolaslara commented Jul 17, 2023

github-actions bot commented Aug 1, 2023

ValarDragon left a comment


		---

		TODO: I think this is overcomplicated and I can't remember why I wrote it like this.

Add async support to wasm hooks #5072

Add async support to wasm hooks #5072

Conversation

nicolaslara commented May 3, 2023 • edited

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

github-actions bot commented May 20, 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

Choose a reason for hiding this comment

p0mvn left a 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

p0mvn left a comment

Choose a reason for hiding this comment

nicolaslara commented Jul 17, 2023

github-actions bot commented Aug 1, 2023

ValarDragon left a comment

Choose a reason for hiding this comment

nicolaslara commented May 3, 2023 •

edited