Add IBC hooks for every IBC method #2296

nicolaslara · 2022-09-15T12:38:25Z

Description

This introduces a middleware for IBC that allows users to write their own hooks.

This was developed to allow the packet forward middleware send the tokens back if a hop fails, but it useful in general for simplifying middleware development.

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.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

crodriguezvega

Thanks for this new middleware, @nicolaslara! I have left some comments after doing a first review.

As a general remark, we would appreciate if you can groups the import like this:

import (
    // standard libraries
  
    // external libraries

    // ibc-go
}

This is just the code style we are trying to keep in the project. :)

It would be also nice to add documentation for this new middleware. And we also need a changelog entry (I would say under Features).

Looks also like gofumpt is complaining a bit on CI as well.

crodriguezvega · 2022-09-16T08:05:15Z

modules/apps/hooks/hooks.go

+}
+
+type IBCAppHooksOnChanOpenInitOverride interface {
+	OnChanOpenInitOverride(im IBCMiddleware, ctx sdk.Context, order channeltypes.Order, connectionHops []string, portID string, channelID string, channelCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version string) (string, error)


nit: do you mind to put each argument on a new line (like we do here)? I think it's easier for readability.

changed it to only one line specifically for readability (so it's easy to scan which methods are defined on each interface, considering the arguments are the same as for the functions they override), but happy to change it back to one arg per line

crodriguezvega · 2022-09-16T19:27:04Z

modules/apps/hooks/hooks.go

+	OnChanCloseConfirmAfterHook(ctx sdk.Context, portID, channelID string, err error)
+}
+
+// OnAcknowledgementPacket Hooks


I think I would change the order and put the OnRecvPacket hooks before the OnAcknowledgementPacket hooks.

crodriguezvega · 2022-09-16T19:31:56Z

modules/apps/hooks/ibc_middleware_test.go

+	chainC *ibctesting.TestChain
+
+	path     *ibctesting.Path
+	pathAToC *ibctesting.Path


I think chainC and pathAToC are not used, right? So maybe you can just remove them...

yes! that was left over from copy pasting a transfer test. Removed.

crodriguezvega · 2022-09-17T11:34:03Z

modules/apps/hooks/testutils/testing_hooks.go

+}
+
+// Recv
+type TestRecvOverrides struct{ Status *Status }


Suggested change

type TestRecvOverrides struct{ Status *Status }

type TestRecvOverrideHooks struct{ Status *Status }

crodriguezvega · 2022-09-17T11:34:40Z

modules/apps/hooks/testutils/testing_hooks.go

+}
+
+// Send
+type TestSendOverrides struct{ Status *Status }


Suggested change

type TestSendOverrides struct{ Status *Status }

type TestSendOverrideHooks struct{ Status *Status }

crodriguezvega · 2022-09-18T09:27:33Z

modules/apps/hooks/hooks.go

+	ibcexported "github.com/cosmos/ibc-go/v5/modules/core/exported"
+)
+
+type IBCAppHooks interface {


Wondering if we could shave off the IBCApp prefix from all these types...

crodriguezvega · 2022-09-18T09:49:25Z

modules/apps/hooks/ibc_middleware_test.go

+	testCases := []struct {
+		msg          string
+		malleate     func(*testutils.Status)
+		recvIsSource bool // the receiving chain is the source of the coin originally


Do you actually need to test the scenario of the receiving chain being the original source of the tokens? If not, maybe this can be removed to simplify the logic of the test.

Good call! simplifying.

crodriguezvega · 2022-09-18T09:56:16Z

modules/apps/hooks/testutils/testing_hooks.go

+}
+
+// Recv
+type TestRecvOverrides struct{ Status *Status }


Just a nit, but I think that you could also make Status unexported.

you can... but then you have to add a NewTestRecvOverrideHooks, which I find less clean. Since it's only used in testing, I don't see the harm in having it be exported.

Don't feel strongly either way, though. Happy to change it if you do

crodriguezvega · 2022-09-18T10:04:17Z

modules/apps/hooks/testutils/testing_hooks.go

+	return err
+}
+
+type TestSendBeforeAfterHooks struct{ Status *Status }


Both TestSendOverrides and TestSendBeforeAfterHooks are not used in the tests. Do you plan to write tests for them as well?

I was planning to add those, but realized that that's quite hard to test because the ICS4Middlware (that should contain the hooks) is inside the IBCFeeKeeper (which doesn't export the ics4Wrapper).

The easiest way to test this would be to add the testing hook directly on simapp, but I'm not sure if that's too intrusive.

I've removed those for now. We will probably test all of these functions with an app that uses them (i.e.: the packet forwarding middleware)

crodriguezvega · 2022-09-18T10:09:05Z

modules/apps/hooks/hooks.go

@@ -0,0 +1,142 @@
+package ibc_hooks


Would it make sense to move this file inside the types folder?

I think that would create an import cycle, as ibc_module.go depends on the hooks and the hooks on the middleware struct defined in that file

codecov-commenter · 2022-09-19T08:06:34Z

Codecov Report

Merging #2296 (4142d48) into main (9fa6008) will decrease coverage by 0.70%.
The diff coverage is 23.56%.

❗ Current head 4142d48 differs from pull request most recent head 1a6a518. Consider uploading reports for the commit 1a6a518 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2296      +/-   ##
==========================================
- Coverage   79.52%   78.82%   -0.71%     
==========================================
  Files         175      177       +2     
  Lines       12069    12226     +157     
==========================================
+ Hits         9598     9637      +39     
- Misses       2047     2152     +105     
- Partials      424      437      +13

Impacted Files	Coverage Δ
modules/apps/hooks/ibc_module.go	`21.08% <21.08%> (ø)`
modules/apps/hooks/ics4_middleware.go	`60.00% <60.00%> (ø)`
modules/apps/27-interchain-accounts/module.go	`52.57% <0.00%> (+2.06%)`	⬆️

Co-authored-by: Carlos Rodriguez <[email protected]>

AdityaSripal

Hi @nicolaslara, thanks for your PR! It seems to me like the intention of this middleware is to make writing middleware more standardized. Since people now implement the before/after/override for each function (most will be no-op) and your middleware then orders the calls.

My concern is that to implement the hook interface, we triple the number of functions expected to be implemented.

Why is this approach better than just having folks implement the interface as-is, and documenting how they can use the callback to execute logic before/after the underlying layer or override entirely? Especially since this middleware is not doing anything other than ordering the logic for them.

AdityaSripal · 2022-09-19T09:58:00Z

modules/apps/hooks/hooks.go

+type OnChanOpenInitOverrideHooks interface {
+	OnChanOpenInitOverride(im IBCMiddleware, ctx sdk.Context, order channeltypes.Order, connectionHops []string, portID string, channelID string, channelCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version string) (string, error)
+}
+type OnChanOpenInitBeforeHooks interface {


Why don't the before and after hooks return errors?

maybe they should. I can update them to return errors

AdityaSripal · 2022-10-26T16:43:26Z

Hey @nicolaslara can you respond to the above? Seems like this can be closed, thanks

nicolaslara · 2022-10-26T18:16:03Z

hey @AdityaSripal, yeah, this can be closed. I still like this way of extending the ibc module better but we can implement it on our end if we want (and so can anyone else who prefers it).

My concern is that to implement the hook interface, we triple the number of functions expected to be implemented.

I think the opposite is true. Since we're matching based on the interface, you can implement only the functions you want to override, instead of implementing a whole IBCModule/ICSWrapper structs. The structs in testing_hooks.go are an example of what a user would need to implement.

Now, after re-reading this, I think if we want to provide an interface to better extend the core IBC modules, it doesn't make sense to do it as a middleware, the hooks could instead be baked in on the core implementation

crodriguezvega · 2022-10-26T20:28:36Z

Thank you, @nicolaslara.

nicolaslara added 4 commits September 14, 2022 12:52

initial implementation of hooks

6f00b34

added hooks for all methods

bb4f72c

added proper tests

58d094a

test cleanup

7876e32

nicolaslara requested review from colin-axner, AdityaSripal, crodriguezvega, seantking, charleenfei, damiannolan and chatton as code owners September 15, 2022 12:38

removed unnecessary files

f1e12bf

crodriguezvega reviewed Sep 18, 2022

View reviewed changes

nicolaslara changed the title ~~Add IBC for every IBC method~~ Add IBC hooks for every IBC method Sep 18, 2022

nicolaslara added 3 commits September 19, 2022 09:49

changed order of hook definition

2322792

removed unused chain

4142d48

renames

1a6a518

nicolaslara and others added 11 commits September 19, 2022 10:06

Update modules/apps/hooks/ibc_middleware_test.go

3ea1a3f

Co-authored-by: Carlos Rodriguez <[email protected]>

fixed test with new name

9de2964

reordered

88efd13

added missing hook for OnAcknowledgementPacket

314be04

simplified initialization

3ec6fcd

simplified tests

facdb73

using new constructor

e4e8dad

moved send hooks to ICS4Wrapper

bba15bc

gofumpt

d9ef44a

reorged imports

0b08a17

gofumpt

a1f93b3

AdityaSripal reviewed Sep 19, 2022

View reviewed changes

nicolaslara mentioned this pull request Sep 22, 2022

CrossChainSwaps: Add a middleware to handle IBC metadata for the swap osmosis-labs/osmosis#2661

Closed

crodriguezvega closed this Oct 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IBC hooks for every IBC method #2296

Add IBC hooks for every IBC method #2296

nicolaslara commented Sep 15, 2022

crodriguezvega left a comment

crodriguezvega Sep 16, 2022

nicolaslara Sep 19, 2022

crodriguezvega Sep 16, 2022

crodriguezvega Sep 16, 2022

nicolaslara Sep 19, 2022

crodriguezvega Sep 17, 2022

nicolaslara Sep 19, 2022

crodriguezvega Sep 17, 2022

nicolaslara Sep 19, 2022

crodriguezvega Sep 18, 2022

nicolaslara Sep 19, 2022

crodriguezvega Sep 18, 2022

nicolaslara Sep 19, 2022

crodriguezvega Sep 18, 2022

nicolaslara Sep 19, 2022

nicolaslara Sep 19, 2022

crodriguezvega Sep 18, 2022

nicolaslara Sep 19, 2022

crodriguezvega Sep 18, 2022

nicolaslara Sep 19, 2022

codecov-commenter commented Sep 19, 2022

AdityaSripal left a comment

AdityaSripal Sep 19, 2022

nicolaslara Oct 4, 2022

AdityaSripal commented Oct 26, 2022

nicolaslara commented Oct 26, 2022

crodriguezvega commented Oct 26, 2022

	type TestRecvOverrides struct{ Status *Status }
	type TestRecvOverrideHooks struct{ Status *Status }

	type TestSendOverrides struct{ Status *Status }
	type TestSendOverrideHooks struct{ Status *Status }

Add IBC hooks for every IBC method #2296

Add IBC hooks for every IBC method #2296

Conversation

nicolaslara commented Sep 15, 2022

Description

crodriguezvega 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 19, 2022

Codecov Report

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Oct 26, 2022

nicolaslara commented Oct 26, 2022

crodriguezvega commented Oct 26, 2022