refactor: cleanup middleware stacks #1248

seantking · 2022-04-12T12:06:00Z

Description

Cleaning up the middleware stacks using evmos as inspiration.

closes: #773

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

testing/simapp/app.go

seantking · 2022-04-13T08:10:40Z

testing/simapp/app.go

+	// create IBC module from bottom to top of stack
+	var transferStack porttypes.IBCModule
+	transferStack = transfer.NewIBCModule(app.TransferKeeper)
+	transferStack = ibcfee.NewIBCMiddleware(transferStack, app.IBCFeeKeeper)


This works fine and I like the UX.

Maybe we should add the routing right after:

ibcRouter.AddRoute(ibctransfertypes.ModuleName, transferStack)

I love this UX as well ❤️

testing/mock/ibc_module.go

seantking · 2022-04-13T08:33:34Z

testing/simapp/app.go

 	// Create static IBC router, add app routes, then set and seal it
 	// pass in top-level (fully-wrapped) IBCModules to IBC Router
 	ibcRouter := porttypes.NewRouter()
-	ibcRouter.AddRoute(icacontrollertypes.SubModuleName, icaControllerIBCModule).


Wasn't sure if we needed this still. I'm guessing we do, but I'm not 100% sure why. We don't add the fee module middleware for example.

For any observers, we need this because the controller module owns the port capability and the ica-auth module owns the channel capability. thanks @colin-axner for the reminder

I think I'll add a comment explaining this

colin-axner · 2022-04-13T12:04:36Z

testing/simapp/app.go

+	// create IBC module from bottom to top of stack
+	var transferStack porttypes.IBCModule
+	transferStack = transfer.NewIBCModule(app.TransferKeeper)
+	transferStack = ibcfee.NewIBCMiddleware(transferStack, app.IBCFeeKeeper)


Maybe we should add the routing right after:

ibcRouter.AddRoute(ibctransfertypes.ModuleName, transferStack)

testing/simapp/app.go

modules/apps/27-interchain-accounts/controller/ibc_module.go

testing/mock/ibc_module.go

testing/simapp/app.go

codecov-commenter · 2022-04-13T12:36:10Z

Codecov Report

Merging #1248 (e0e405e) into main (311379f) will increase coverage by 0.04%.
The diff coverage is 57.89%.

@@            Coverage Diff             @@
##             main    #1248      +/-   ##
==========================================
+ Coverage   80.05%   80.10%   +0.04%     
==========================================
  Files         166      166              
  Lines       11910    11951      +41     
==========================================
+ Hits         9535     9573      +38     
- Misses       1914     1920       +6     
+ Partials      461      458       -3

Impacted Files	Coverage Δ
modules/apps/27-interchain-accounts/module.go	`57.69% <ø> (ø)`
modules/apps/29-fee/module.go	`55.55% <ø> (ø)`
...ps/27-interchain-accounts/controller/ibc_module.go	`86.00% <33.33%> (-7.48%)`	⬇️
modules/apps/29-fee/ibc_module.go	`95.10% <69.23%> (+3.67%)`	⬆️
modules/apps/29-fee/types/codec.go	`43.75% <0.00%> (-26.25%)`	⬇️
modules/apps/29-fee/types/keys.go	`96.66% <0.00%> (-3.34%)`	⬇️
modules/apps/29-fee/keeper/keeper.go	`92.34% <0.00%> (-0.91%)`	⬇️
modules/core/04-channel/types/packet.go	`67.56% <0.00%> (ø)`
modules/apps/29-fee/keeper/msg_server.go	`78.04% <0.00%> (+3.04%)`	⬆️
... and 1 more

damiannolan · 2022-04-13T13:11:10Z

testing/simapp/app.go

 	app.IBCFeeKeeper = ibcfeekeeper.NewKeeper(appCodec, keys[ibcfeetypes.StoreKey], app.GetSubspace(ibcfeetypes.ModuleName),
 		app.IBCKeeper.ChannelKeeper, app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper, app.AccountKeeper, app.BankKeeper,
 	)

+	// Create Transfer Stack


Should we add a comment similar to this?

I think even something simple like that makes the flow of control much clearer

fedekunze · 2022-04-14T13:12:15Z

awesome! feel free to use our BDD tests too ❤️

seantking · 2022-04-14T13:13:30Z

awesome! feel free to use our BDD tests too heart

Nice, I'll check em out. Can you link?

Great work btw, I really like the pattern you used. Thanks for the inspiration 🤝

fedekunze

Highly recommend creating an IBCModule struct and embed it to the IBCMiddleware to simplify the diff:

https://github.com/tharsis/evmos/blob/main/ibc/module.go
https://github.com/tharsis/evmos/blob/main/x/claims/ibc_middleware.go#L14-L21

colin-axner

Excellent!

testing/simapp/app.go

colin-axner · 2022-04-14T14:44:13Z

testing/simapp/app.go

-	)
+	// Create Interchain Accounts Stack
+	// SendPacket, since it is originating from the application to core IBC:
+	// icaAuthModuleKeeper.SendPacket -> icaControllerKeeper.SendPacket -> channel.SendPacket


technically the icaAuthModuleKeeper calls SendTx, maybe we should reflect this approach?

testing/simapp/app.go

AdityaSripal

Are we not incentivizing ICA packets? I think it would be a good example to show how to stack more than 1 middleware in case of the controller

AdityaSripal · 2022-04-19T12:11:25Z

modules/apps/27-interchain-accounts/controller/ibc_module.go

+	chanCap *capabilitytypes.Capability,
+	packet exported.PacketI,
+) error {
+	panic("SendPacket not supported for ICA controller module")


ICA auth SendPacket should call this function and then this function will pass the call one layer up.

Why would this occur? ICA auth calls SendTx. ICA auth shouldn't send non ICA packets?

ICA auth is being wrapped by ICA correct? In that case we will want the packet flow to be:

ICA-Auth -SendPacket-> ICA -SendPacket-> IBC

The ICA middleware in this case will do nothing but forward the packet one layer up. This is to keep consistency and to make sure ICA-auth packets are not missed by additional middleware that is wrapping the stack

ICA auth is being wrapped by ICA correct?

Only partially (not all callbacks/interfaces should be implemented). SendPacket usage is replaced by SendTx

ICA controller constructs the packet and thus calls SendPacket. ICA Auth shouldn't call SendPacket. It could bypass SendTx by calling SendPacket directly, if we panic, we disable this functionality. I don't understand the purpose of SendTx if we do allow bypassing. The spec doesn't indicate the appropriate functionality

AdityaSripal · 2022-04-19T12:12:02Z

testing/simapp/app.go

+	// ICA Controller keeper
+	app.ICAControllerKeeper = icacontrollerkeeper.NewKeeper(
+		appCodec, keys[icacontrollertypes.StoreKey], app.GetSubspace(icacontrollertypes.SubModuleName),
+		app.IBCKeeper.ChannelKeeper, // may be replaced with middleware such as ics29 fee


Should we include this comment for every IBC keeper so that it is clearer to people looking at this file?

damiannolan

Left a couple of small nits, but more or less looks good to go from what I can see!
Would be good to get a couple of more acks before merging.

Nice work 👍

damiannolan · 2022-04-20T09:47:17Z

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

+	"github.com/cosmos/ibc-go/v3/modules/core/exported"
 	ibcexported "github.com/cosmos/ibc-go/v3/modules/core/exported"


This is imported twice

we should add a linter for this

duplicate import is still there

damiannolan · 2022-04-20T09:49:07Z

modules/apps/29-fee/ibc_middleware.go

-func NewIBCModule(k keeper.Keeper, app porttypes.IBCModule) IBCModule {
-	return IBCModule{
-		keeper: k,
+// NewIBCMiddlware creates a new IBCMiddlware given the keeper and underlying application


Suggested change

// NewIBCMiddlware creates a new IBCMiddlware given the keeper and underlying application

// NewIBCMiddleware creates a new IBCMiddleware given the keeper and underlying application

damiannolan · 2022-04-20T09:55:55Z

testing/simapp/app.go


-	icaModule := ica.NewAppModule(&app.ICAControllerKeeper, &app.ICAHostKeeper)
+	// OnAckPacket as this is where fee's are paid out


nit: may as well be explicit

Suggested change

// OnAckPacket as this is where fee's are paid out

// OnAcknowledgementPacket as this is where fee's are paid out

colin-axner

LGTM! Nice work!

colin-axner · 2022-04-21T10:57:13Z

testing/simapp/app.go

+
+	// Create Interchain Accounts Stack
+	// SendPacket, since it is originating from the application to core IBC:
+	// icaAuthModuleKeeper.SendPacket -> fee.SendPacket -> icaControllerKeeper.SendPacket -> channel.SendPacket


Hmm, this doesn't make sense. If fee was in the stack, it should be above icaControllerKeeper. Futhermore, icaAuthModuleKeeper must call SendTx not SendPacket

I would say that the icaAuthModuleKeeper is still calling SendPacket it's just doing so via SendTx but I agree we should change to SendTx to be more explicit.

colin-axner · 2022-04-21T10:58:18Z

testing/simapp/app.go

+	var icaControllerStack porttypes.IBCModule
+	icaControllerStack = ibcmock.NewIBCModule(&mockModule, ibcmock.NewMockIBCApp("", scopedICAMockKeeper))
+	app.ICAAuthModule = icaControllerStack.(ibcmock.IBCModule)
+	icaControllerStack = ibcfee.NewIBCMiddleware(icaControllerStack, app.IBCFeeKeeper)
+	icaControllerStack = icacontroller.NewIBCMiddleware(icaControllerStack, app.ICAControllerKeeper)


I would prefer this change to be made in a followup pr. Ideally it would come with tests. But if contained in this pr, the fee should be at the top of the stack. Generic middleware cannot be inserted between ICA auth and controller because the SendTx API

Yep, good point. I'll remove it and do it in a follow up PR.

testing/simapp/app.go

colin-axner · 2022-04-21T11:02:29Z

testing/simapp/app.go

+	feeMockModule := ibcmock.NewIBCModule(&mockModule, ibcmock.NewMockIBCApp(MockFeePort, scopedFeeMockKeeper))
+	app.FeeMockModule = feeMockModule
+	feeWithMockModule := ibcfee.NewIBCMiddleware(feeMockModule, app.IBCFeeKeeper)


I think it is possible to combine mockIBCModule and the fee mock stack. Ideally the fee version just wouldn't be set for the isolated mock module usage. I'd be in favor of a followup pr fixing this

crodriguezvega · 2022-04-25T07:29:44Z

testing/simapp/app.go


-	mockIBCModule := ibcmock.NewIBCModule(&mockModule, ibcmock.NewMockIBCApp(ibcmock.ModuleName, scopedIBCMockKeeper))
+	// RecvPacket, message that originates from core IBC and goes down to app, the flow is the otherway


Suggested change

// RecvPacket, message that originates from core IBC and goes down to app, the flow is the otherway

// RecvPacket, message that originates from core IBC and goes bottom to app, the flow is the other way

crodriguezvega · 2022-04-25T07:31:27Z

testing/simapp/app.go

+		AddRoute(ibcmock.ModuleName+icacontrollertypes.SubModuleName, icaControllerStack) // ica with mock auth module stack route to ica (top level of middleware stack)
+
+	// Create Mock IBC Fee module stack for testing
+	// OnAckPacket as this is where fee's are paid out


Same nit as Damian's:

Suggested change

// OnAckPacket as this is where fee's are paid out

// OnAcknowledgementPacket as this is where fee's are paid out

crodriguezvega · 2022-04-25T07:31:40Z

testing/simapp/app.go

+	// OnAckPacket as this is where fee's are paid out
+	// mockModule.OnAcknowledgementPacket -> fee.OnAcknowledgementPacket -> channel.OnAcknowledgementPacket
+
+	// RecvPacket, message that originates from core IBC and goes down to app, the flow is the otherway


Suggested change

// RecvPacket, message that originates from core IBC and goes down to app, the flow is the otherway

// RecvPacket, message that originates from core IBC and goes bottom to app, the flow is the other way

crodriguezvega · 2022-04-25T07:32:59Z

testing/simapp/app.go


+	// SendPacket, since it is originating from the application to core IBC:


Wouldn't it be more clear if you revert the order? So the explanation for SendPacket first, then RecvPacket's, and finally OnAcknowledgementPacket's

AdityaSripal · 2022-04-25T09:58:26Z

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

@@ -163,3 +167,22 @@ func (im IBCModule) OnTimeoutPacket(

 	return im.app.OnTimeoutPacket(ctx, packet, relayer)
 }
+
+// SendPacket implements the ICS4 Wrapper interface
+func (im IBCMiddleware) SendPacket(


Ok I understand now why you've done it this way.

I think it should be documented in this function's go-doc that the packet is going to get constructed directly by ICA controller and sent to IBC rather than being created by ICA-auth.

Reading this file, made me think SendPacket wasn't happening at all when really it is happening through the controller's SendTx function

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

Co-authored-by: Aditya <adityasripal@gmail.com>

seantking · 2022-04-27T16:43:09Z

I think this is good to merge now?

colin-axner

Wahoo!! Great job 🚀

colin-axner · 2022-04-28T14:10:26Z

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

+	"github.com/cosmos/ibc-go/v3/modules/core/exported"
 	ibcexported "github.com/cosmos/ibc-go/v3/modules/core/exported"


duplicate import is still there

AdityaSripal

Nice work! Could you create a new issue to tackle wrapping ICA with fee?

AdityaSripal · 2022-04-28T15:24:09Z

testing/simapp/app.go

+	feeMockModule := ibcmock.NewIBCModule(&mockModule, ibcmock.NewMockIBCApp(MockFeePort, scopedFeeMockKeeper))
+	app.FeeMockModule = feeMockModule
+	feeWithMockModule := ibcfee.NewIBCMiddleware(feeMockModule, app.IBCFeeKeeper)


nit: why do you use different variables here, but with transfer you just reassign?

I needed to satisfy the compiler. The mock module is of type ibcmock.MockModule & the middleware is of type IBCMiddleware.

I think we could do a follow-up to create a mock middleware if it's useful.

## Overview  ## Checklist  - [ ] New and updated code has appropriate documentation - [ ] New and updated code has new and/or updated testing - [ ] Required CI checks are passing - [ ] Visual proof for any user facing features like CLI or documentation updates - [ ] Linked issues closed with keywords --------- Co-authored-by: Manav Aggarwal <manavaggarwal1234@gmail.com>

seantking added 3 commits April 12, 2022 12:02

refactor: UX improvements for middleware

227f47b

fix: move fee keeper

725936f

fix: remove unncessary wrapping

700ad6a

seantking requested review from charleenfei, colin-axner and damiannolan April 12, 2022 12:18

refactor: ica module stack setup

5852bdf

seantking commented Apr 13, 2022

View reviewed changes

testing/simapp/app.go Outdated Show resolved Hide resolved

seantking commented Apr 13, 2022

View reviewed changes

testing/mock/ibc_module.go Outdated Show resolved Hide resolved

seantking commented Apr 13, 2022

View reviewed changes

fix: define icaControllerStack as type porttypes.IBCModule

94f3e23

colin-axner reviewed Apr 13, 2022

View reviewed changes

add back controller routing for port

e0e405e

colin-axner reviewed Apr 13, 2022

View reviewed changes

modules/apps/27-interchain-accounts/controller/ibc_module.go Outdated Show resolved Hide resolved

testing/mock/ibc_module.go Outdated Show resolved Hide resolved

testing/simapp/app.go Show resolved Hide resolved

testing/simapp/app.go Show resolved Hide resolved

damiannolan reviewed Apr 13, 2022

View reviewed changes

seantking and others added 6 commits April 14, 2022 14:34

refactor: remove unncessary SendPacket

fd2360f

refactor: panic instead of return nil

2793d73

refactor: changing structure to init all keepers, then stacks & routing

b419f8f

docs: add comments

4a60ff6

Merge branch 'main' into sean/issue#773-cleanup-middleware-stacks

2aa1f8c

nit: comment

31c3f93

seantking marked this pull request as ready for review April 14, 2022 13:12

seantking requested review from AdityaSripal and crodriguezvega as code owners April 14, 2022 13:12

seantking requested a review from fedekunze April 14, 2022 13:13

fedekunze reviewed Apr 14, 2022

View reviewed changes

colin-axner reviewed Apr 14, 2022

View reviewed changes

AdityaSripal requested changes Apr 19, 2022

View reviewed changes

seantking added 3 commits April 19, 2022 17:22

chore: rename files

40fbdcc

chore: comment + nit

68f4843

refactor: stacks

6624b73

seantking requested review from damiannolan, colin-axner, AdityaSripal and crodriguezvega April 19, 2022 16:28

seantking force-pushed the sean/issue#773-cleanup-middleware-stacks branch from 4254ea7 to 6624b73 Compare April 19, 2022 16:40

damiannolan approved these changes Apr 20, 2022

View reviewed changes

colin-axner reviewed Apr 21, 2022

View reviewed changes

crodriguezvega reviewed Apr 25, 2022

View reviewed changes

AdityaSripal requested changes Apr 25, 2022

View reviewed changes

seantking and others added 4 commits April 26, 2022 19:32

Update modules/apps/27-interchain-accounts/controller/ibc_middleware.go

611c5db

Co-authored-by: Aditya <adityasripal@gmail.com>

nits: nits

5898bfc

chore: comment

80816aa

Merge branch 'main' into sean/issue#773-cleanup-middleware-stacks

112d1c1

seantking requested review from crodriguezvega, AdityaSripal, damiannolan and colin-axner April 27, 2022 16:43

colin-axner approved these changes Apr 28, 2022

View reviewed changes

seantking and others added 2 commits April 28, 2022 16:23

chore: chore

ac937cb

Merge branch 'main' into sean/issue#773-cleanup-middleware-stacks

80de572

AdityaSripal approved these changes Apr 28, 2022

View reviewed changes

seantking merged commit cc4cd7b into main Apr 28, 2022

seantking deleted the sean/issue#773-cleanup-middleware-stacks branch April 28, 2022 16:17

		"github.com/cosmos/ibc-go/v3/modules/core/exported"
		ibcexported "github.com/cosmos/ibc-go/v3/modules/core/exported"

	// NewIBCMiddlware creates a new IBCMiddlware given the keeper and underlying application
	// NewIBCMiddleware creates a new IBCMiddleware given the keeper and underlying application


		icaModule := ica.NewAppModule(&app.ICAControllerKeeper, &app.ICAHostKeeper)
		// OnAckPacket as this is where fee's are paid out

	// OnAckPacket as this is where fee's are paid out
	// OnAcknowledgementPacket as this is where fee's are paid out


		mockIBCModule := ibcmock.NewIBCModule(&mockModule, ibcmock.NewMockIBCApp(ibcmock.ModuleName, scopedIBCMockKeeper))
		// RecvPacket, message that originates from core IBC and goes down to app, the flow is the otherway


		// SendPacket, since it is originating from the application to core IBC:

refactor: cleanup middleware stacks #1248

refactor: cleanup middleware stacks #1248

Conversation

seantking commented Apr 12, 2022 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seantking Apr 13, 2022 • 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

codecov-commenter commented Apr 13, 2022

Codecov Report

Choose a reason for hiding this comment

fedekunze commented Apr 14, 2022

seantking commented Apr 14, 2022

fedekunze left a comment • edited Loading

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan 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

colin-axner 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

seantking commented Apr 27, 2022

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

seantking commented Apr 12, 2022 •

edited

Loading

seantking Apr 13, 2022 •

edited

Loading

fedekunze left a comment •

edited

Loading

colin-axner Apr 21, 2022 •

edited

Loading

colin-axner Apr 25, 2022 •

edited

Loading