modules/core/keeper: NewKeeper blank value detection assumes interface values are all non-pointer receivers and will fail to detect blank pointer receivers

[odeke-em]

Summary of Bug

The code inside

ibc-go/modules/core/keeper/keeper.go

Lines 52 to 60 in a4be561

	// panic if any of the keepers passed in is empty
	if reflect.ValueOf(stakingKeeper).IsZero() {
	panic(fmt.Errorf("cannot initialize IBC keeper: empty staking keeper"))
	}
	if reflect.ValueOf(upgradeKeeper).IsZero() {
	panic(fmt.Errorf("cannot initialize IBC keeper: empty upgrade keeper"))
	}

checks if values are Zero values using reflect.ValueOf. That'll work in cases where value receivers are passed in. However, for a general purpose package that receives either pointer or value receivers, this forces odd behavior in which only value receivers can be checked. For example if we examine https://go.dev/play/p/zfFZRDjqz-X or inlined below

package main

import (
	"io"
	"reflect"
)

type dt struct {
	i int
	s string
}

func (dt) Write(b []byte) (int, error) { return len(b), nil }

func main() {
	println(reflect.ValueOf(dt{}).IsZero())
	println(reflect.ValueOf(&dt{}).IsZero())
	println(reflect.ValueOf((io.Writer)(dt{})).IsZero())
	println(reflect.ValueOf((io.Writer)(&dt{})).IsZero())
}

Which prints out

true
false
true
false

But really if zero value checking were implemented correctly by dereferencing pointers, every line should print true.

Expected Behaviour

Catching blank pointer values

Just an FYI for my colleagues @elias-orijtech @kirbyquerby @willpoint

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[crodriguezvega]

Maybe I am missing something, but the stakingKeeper and upgradeKeeper parameters need to be passed by value (as the signature of NewKeeper indicates), so if in the upstream caller function they were pointers, they would need to be dereferenced before being passed to NewKeeper, right?

[odeke-em]

@crodriguezvega it expects values that conform to an interface. Anything that conforms to that interface can be passed in and they could either be pointers or non-pointers.

[crodriguezvega]

@crodriguezvega it expects values that conform to an interface. Anything that conforms to that interface can be passed in and they could either be pointers or non-pointers.

Ah, yes, I see now. Thanks!

[alizahidraja]

Hey! I have added a switch case to cater to this issue, let me know if that works