x/auth/types: CountSubKeys inexplicably returns 1 when the public key isn't of type multisig.PubKeyMultisigThreshold yet naturally counts otherwise

[odeke-em]

The code in question is

cosmos-sdk/x/auth/types/stdtx.go

Lines 128 to 140 in 72ebafe

	func CountSubKeys(pub crypto.PubKey) int {
	v, ok := pub.(multisig.PubKeyMultisigThreshold)
	if !ok {
	return 1
	}
	numKeys := 0
	for _, subkey := range v.PubKeys {
	numKeys += CountSubKeys(subkey)
	}
	return numKeys
	}

Why does it return the magic number 1 if pub is not of the required yet, yet in the positive case is actually recursively counts subkeys. I don't see a test to affirm this behavior and could perhaps return random behavior. If this logic is intended, at least let's document it.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[alexanderbez]

A non-multisig, i.e. a regular signature, it naturally a count of 1. If it is a multisig, then it recursively calls it on its pubkeys. The number one isn't magic here.

[aaronc]

Is this just missing a test @odeke-em ? The logic looks correct to me.

[odeke-em]

It could use a comment as to why that 1, and a test of sorts indeed would be nice but just the comment that Aleksander explaining that logic would suffice for me.

…

On Thu, Jul 30, 2020 at 7:34 AM Aaron Craelius ***@***.***> wrote: Is this just missing a test @odeke-em <https://github.com/odeke-em> ? The logic looks correct to me. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6889 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3V3T6L6JDLKDNZRKBZDR6GAJBANCNFSM4PMYTG7Q> .

[alexanderbez]

Is recursive function lingo, returning 1 is the base-case -- i.e. when a key has no sub-keys. If tests are lacking, we should certainly add more tests/cases!

[JulianToledano]

@odeke-em can we close this one? there are some test cases for this in here
https://github.com/cosmos/cosmos-sdk/blob/main/x/auth/ante/ante_test.go#L1281-L1313