Middleware Wiring Improvements

[srdtrk]

Summary

The proposed changes aim to simplify the middleware wiring process significantly, making it more intuitive and less error-prone, thereby improving the overall reliability and security of IBC middleware integration. The proposal recommends introducing a new helper called IBCStackBuilder.

Problem

The current mechanism for integrating IBC middleware is complex and prone to errors, leading to difficulties in understanding the middleware stack, which subsequently results in bugs and security vulnerabilities. This complexity stems from the need to intertwine two distinct stacks: the IBC module stack and the ICS4Wrapper stack. The latter is often managed by a module's keeper rather than the module itself, complicating the integration process. Furthermore, the necessity to wire these stacks in reverse order complicates their integration during module initialization.

Proposal

This proposal suggests a syntactic enhancement aimed at simplifying the wiring process of IBC middlewares without altering their underlying architecture. It introduces a new construct, the IBCStackBuilder, designed to streamline the middleware integration process. The proposed API enables a straightforward layering of middlewares, culminating in the base application, as demonstrated below:

transferStack := porttypes.NewIBCStackBuilder().Next(feeMiddleware).Next(callbacksMiddleware).Base(transferApp).Build()

To facilitate this approach, several API modifications are proposed:

1. Removing the obligation for IBCModule and IBCMiddleware interfaces to accept an ICS4Wrapper during initialization, which eliminates circular dependencies. Instead, a SetICS4Wrapper method will be added to the IBCModule interface, allowing for the post-initialization setting of the middleware. (Alternatively, this could be an optional interface that the module can implement to be middleware compatible.)

type IBCModule interface {
	// ... other methods
+
+	// SetICS4Wrapper sets the ICS4Wrapper. This function may be used after
+	// the module's initialization to set the middleware which is above this
+	// module in the IBC application stack.
+	SetICS4Wrapper(wrapper ICS4Wrapper)

This will allow the IBCStackBuilder to wire the ICS4Wrapper after the IBC module has been initialized.

2. Introduce a new SetUnderlyingModule method to the IBCMiddleware interface. This allows for the setting of the underlying application post-initialization, thus supporting middleware initializayion without the underlying application.

type Middleware interface {
	IBCModule
	ICS4Wrapper
+
+	// SetUnderlyingModule sets the underlying IBC module. This function may be used after
+	// the middleware's initialization to set the ibc module which is below this middleware.
+	SetUnderlyingApplication(IBCModule)
}

3. A detailed implementation of the IBCStackBuilder is provided, outlining methods for adding middlewares, setting the base module, and building the final IBC module stack. This builder pattern facilitates a clear and orderly assembly of the middleware stack.

package types

type IBCStackBuilder struct {
	middlewares []Middleware
	baseModule  IBCModule
}

func NewIBCStackBuilder() *IBCStackBuilder {
	return &IBCStackBuilder{}
}

func (b *IBCStackBuilder) Next(middleware Middleware) {
	b.middlewares = append(b.middlewares, middleware)
}

func (b *IBCStackBuilder) Base(baseModule IBCModule) {
	if baseModule == nil {
		panic("base module cannot be nil")
	}
	if b.baseModule != nil {
		panic("base module already set")
	}
	b.baseModule = baseModule
}

func (b *IBCStackBuilder) Build() IBCModule {
	if b.baseModule == nil {
		panic("base module cannot be nil")
	}
	if len(b.middlewares) == 0 {
		panic("middlewares cannot be empty")
	}

	for i := 0; i < len(b.middlewares); i++ {
		var ics4wrapper ICS4Wrapper
		if i == 0 {
			ics4wrapper = b.baseModule.GetICS4Wrapper()
		} else {
			ics4wrapper = b.middlewares[i-1].GetICS4Wrapper()
		}

		b.middlewares[i].SetICS4Wrapper(ics4wrapper)

		var underlyingModule IBCModule
		if i == len(b.middlewares)-1 {
			underlyingModule = b.baseModule
		} else {
			underlyingModule = b.middlewares[i+1]
		}

		b.middlewares[i].SetUnderlyingApplication(underlyingModule)
	}

	return b.middlewares[0]
}

4. The proposal recommends removing the ICS4Wrapper from the constructors of keepers and modules, streamlining their instantiation and enhancing maintainability. For instance, see the proposed changes to the NewKeeper function in the transfer module:

// NewKeeper creates a new IBC transfer Keeper instance
func NewKeeper(
	cdc codec.BinaryCodec,
	key storetypes.StoreKey,
	legacySubspace types.ParamSubspace,
-	ics4Wrapper porttypes.ICS4Wrapper,
	channelKeeper types.ChannelKeeper,
	portKeeper types.PortKeeper,
	authKeeper types.AccountKeeper,
	bankKeeper types.BankKeeper,
	scopedKeeper exported.ScopedKeeper,
	authority string,
) Keeper {
	// ensure ibc transfer module account is set
	if addr := authKeeper.GetModuleAddress(types.ModuleName); addr == nil {
		panic(errors.New("the IBC transfer module account has not been set"))
	}

	if strings.TrimSpace(authority) == "" {
		panic(errors.New("authority must be non-empty"))
	}

	return Keeper{
		cdc:            cdc,
		storeKey:       key,
		legacySubspace: legacySubspace,
-		ics4Wrapper:    ics4Wrapper,
+		ics4Wrapper:    channelKeeper,
		channelKeeper:  channelKeeper,
		portKeeper:     portKeeper,
		authKeeper:     authKeeper,
		bankKeeper:     bankKeeper,
		scopedKeeper:   scopedKeeper,
		authority:      authority,
	}
}

[AdityaSripal]

This proposal looks great. Would love to see it prototyped to ensure that we don't run into unexpected build issues. Perhaps after we get consensus on the approach from rest of engineers?
Cc: @crodriguezvega