Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(ibc-hooks): Integration Readme seems incorrect #221

Open
puneet2019 opened this issue Sep 5, 2024 · 1 comment
Open

(ibc-hooks): Integration Readme seems incorrect #221

puneet2019 opened this issue Sep 5, 2024 · 1 comment
Assignees
Labels
ibc-hooks Label for items related to Osmosis' ibc-hooks implementation

Comments

@puneet2019
Copy link

Readme mentions the integration of ibc-hooks as ->

...

	// 'ibc-hooks' module - depends on
	// 1. 'auth'
	// 2. 'bank'
	// 3. 'distr'
	app.keys[ibchookstypes.StoreKey] = storetypes.NewKVStoreKey(ibchookstypes.StoreKey)
	app.IBCHooksKeeper = ibchookskeeper.NewKeeper(
		app.keys[ibchookstypes.StoreKey],
	)
	app.Ics20WasmHooks = ibchooks.NewWasmHooks(&app.IBCHooksKeeper, nil, AccountAddressPrefix) // The contract keeper needs to be set later

	// initialize the wasm keeper with
	// wasmKeeper := wasm.NewKeeper( ... )
	app.WasmKeeper = &wasmKeeper

	// Pass the contract keeper to all the structs (generally ICS4Wrappers for ibc middlewares) that need it
	app.ContractKeeper = wasmkeeper.NewDefaultPermissionKeeper(app.WasmKeeper)
	app.Ics20WasmHooks.ContractKeeper = app.ContractKeeper
	app.HooksICS4Wrapper = ibchooks.NewICS4Middleware(
		app.IBCKeeper.ChannelKeeper,
		app.Ics20WasmHooks,
	)
	// Hooks Middleware
	transferIBCModule := ibctransfer.NewIBCModule(app.TransferKeeper)
	app.TransferStack = ibchooks.NewIBCMiddleware(&transferIBCModule, &app.HooksICS4Wrapper)

...

But this integration doesn't make sure that Hooks corresponding to ICS4Wrapper are triggered, only the hooks corresponding to IBCModule are triggered.

// Basiclly Middleware has both IBCModule and ICS4Wrapper
type Middleware interface {
	IBCModule // these hooks are triggered as they are part of the "transferStack"
	ICS4Wrapper // these hooks are not triggered as ICS4Wrapper is not passed into TransferKeeper Or PFM keeper
}
@puneet2019 puneet2019 changed the title (ibc-hooks): Readme (ibc-hooks): Integration Readme seems incorrect Sep 5, 2024
@puneet2019
Copy link
Author

also a small request, can we rename https://github.com/cosmos/ibc-apps/blob/main/modules/ibc-hooks/ics4_middleware.go#L17

type ICS4Middleware struct {
	channel porttypes.ICS4Wrapper // this variable to point to being a ics4 wrapper

	// Hooks
	Hooks Hooks
}

@Reecepbcups Reecepbcups added the ibc-hooks Label for items related to Osmosis' ibc-hooks implementation label Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ibc-hooks Label for items related to Osmosis' ibc-hooks implementation
Projects
None yet
Development

No branches or pull requests

3 participants