chore: simplify ADR-028 and address.Module

docs/architecture/adr-028-public-key-addresses.md

@@ -97,7 +97,7 @@ As in other parts of the Cosmos SDK, we will use `sha256`.
 
 ### Basic Address
 
-We start with defining a base hash algorithm for generating addresses. Notably, it's used for accounts represented by a single key pair. For each public key schema we have to have an associated `typ` string, which we discuss in a section below. `hash` is the cryptographic hash function defined in the previous section.
+We start with defining a base algorithm for generating addresses which we will call `Hash`. Notably, it's used for accounts represented by a single key pair. For each public key schema we have to have an associated `typ` string, explained in the next section. `hash` is the cryptographic hash function defined in the previous section.
 
 ```go
 const A_LEN = 32
@@ -114,16 +114,13 @@ Motivation: this algorithm keeps the address relatively small (length of the `ty
 and it's more secure than [post-hash-prefix-proposal] (which uses the first 20 bytes of a pubkey hash, significantly reducing the address space).
 Moreover the cryptographer motivated the choice of adding `typ` in the hash to protect against a switch table attack.
 
-We use the `address.Hash` function for generating addresses for all accounts represented by a single key:
+`address.Hash` is a low level function to generate _base_ addresses for new key types. Example:
 
-* simple public keys: `address.Hash(keyType, pubkey)`
-
-* aggregated keys (eg: BLS): `address.Hash(keyType, aggregatedPubKey)`
-* modules: `address.Hash("module", moduleName)`
+* BLS: `address.Hash("bls", pubkey)`
 
 ### Composed Addresses
 
-For simple composed accounts (like new naive multisig), we generalize the `address.Hash`. The address is constructed by recursively creating addresses for the sub accounts, sorting the addresses and composing them into a single address. It ensures that the ordering of keys doesn't impact the resulting address.
+For simple composed accounts (like a new naive multisig) we generalize the `address.Hash`. The address is constructed by recursively creating addresses for the sub accounts, sorting the addresses and composing them into a single address. It ensures that the ordering of keys doesn't impact the resulting address.
 
 ```go
 // We don't need a PubKey interface - we need anything which is addressable.
@@ -140,13 +137,13 @@ func Composed(typ string, subaccounts []Addressable) []byte {
 
 The `typ` parameter should be a schema descriptor, containing all significant attributes with deterministic serialization (eg: utf8 string).
 `LengthPrefix` is a function which prepends 1 byte to the address. The value of that byte is the length of the address bits before prepending. The address must be at most 255 bits long.
-We are using `LengthPrefix` to eliminate conflicts - it assures, that for 2 lists of addresses: `as = {a1, a2, ..., an}` and `bs = {b1, b2, ..., bm}` such that every `bi` and `ai` is at most 255 long, `concatenate(map(as, \a -> LengthPrefix(a))) = map(bs, \b -> LengthPrefix(b))` iff `as = bs`.
+We are using `LengthPrefix` to eliminate conflicts - it assures, that for 2 lists of addresses: `as = {a1, a2, ..., an}` and `bs = {b1, b2, ..., bm}` such that every `bi` and `ai` is at most 255 long, `concatenate(map(as, (a) => LengthPrefix(a))) = map(bs, (b) => LengthPrefix(b))` iff `as = bs`.
 
 Implementation Tip: account implementations should cache addresses.
 
 #### Multisig Addresses
 
-For new multisig public keys, we define the `typ` parameter not based on any encoding scheme (amino or protobuf). This avoids issues with non-determinism in the encoding scheme.
+For a new multisig public keys, we define the `typ` parameter not based on any encoding scheme (amino or protobuf). This avoids issues with non-determinism in the encoding scheme.
 
 Example:
 
@@ -161,67 +158,83 @@ message PubKey {
 
 ```go
 func (multisig PubKey) Address() {
-  // first gather all nested pub keys
-  var keys []address.Addressable  // cryptotypes.PubKey implements Addressable
-  for _, _key := range multisig.Pubkeys {
-    keys = append(keys, key.GetCachedValue().(cryptotypes.PubKey))
-  }
+	// first gather all nested pub keys
+	var keys []address.Addressable  // cryptotypes.PubKey implements Addressable
+	for _, _key := range multisig.Pubkeys {
+		keys = append(keys, key.GetCachedValue().(cryptotypes.PubKey))
+	}
 
-  // form the type from the message name (cosmos.crypto.multisig.PubKey) and the threshold joined together
-  prefix := fmt.Sprintf("%s/%d", proto.MessageName(multisig), multisig.Threshold)
+	// form the type from the message name (cosmos.crypto.multisig.PubKey) and the threshold joined together
+	prefix := fmt.Sprintf("%s/%d", proto.MessageName(multisig), multisig.Threshold)
 
-  // use the Composed function defined above
-  return address.Composed(prefix, keys)
+	// use the Composed function defined above
+	return address.Composed(prefix, keys)
 }
 ```
 
-#### Module Account Addresses
 
-NOTE: this section is not finalize and it's in active discussion.
+### Derived Addresses
 
-In Basic Address section we defined a module account address as:
+We must be able to cryptographically derive one address from another one. The derivation process must guarantee hash properties, hence we use the already defined `Hash` function:
 
 ```go
-address.Hash("module", moduleName)
+func Derive(address, derivationKey []byte) []byte {
+	return Hash(addres, derivationKey)
+}
 ```
 
-We use `"module"` as a schema type for all module derived addresses. Module accounts can have sub accounts. The derivation process has a defined order: module name, submodule key, subsubmodule key.
-Module account addresses are heavily used in the Cosmos SDK so it makes sense to optimize the derivation process: instead of using of using `LengthPrefix` for the module name, we use a null byte (`'\x00'`) as a separator. This works, because null byte is not a part of a valid module name.
+### Module Account Addresses
+
+A module account will have `"module"` type. Module accounts can have sub accounts. The submodule account will be created based on module name, and sequence of derivation keys. Typically, the first derivation key should be a class of the derived accounts. The derivation process has a defined order: module name, submodule key, subsubmodule key... An example module account is created using:
 
 ```go
-func Module(moduleName string, key []byte) []byte{
-	return Hash("module", []byte(moduleName) + 0 + key)
-}
+address.Module(moduleName, key)
 ```
 
-**Example**  A lending BTC pool address would be:
+An example sub-module account is created using:
 
 ```go
-btcPool := address.Module("lending", btc.Address()})
+groupPolicyAddresses := []byte{1}
+address.Module(moduleName, groupPolicyAddresses, policyID)
 ```
 
-If we want to create an address for a module account depending on more than one key, we can concatenate them:
+The `address.Module` function is using `address.Hash` with `"module"` as the type argument, and byte representation of the module name concatenated with submodule key. The two last component must be uniquely separated to avoid potential clashes (example: modulename="ab" & submodulekey="bc" will have the same derivation key as modulename="a" & submodulekey="bbc").
+We use a null byte (`'\x00'`) to separate module name from the submodule key. This works, because null byte is not a part of a valid module name. Finally, the sub-submodule accounts are created by applying the `Derive` function recursively.
+We could use `Derive` function also in the first step (rather than concatenating module name with zero byte and the submodule key). We decided to do concatenation to avoid one level of derivation and speed up computation.
+
+For backward compatibility with the existing `authtypes.NewModuleAddress`, we add a special case in `Module` function: when no derivation key is provided, we fallback to the "legacy" implementation. 
 
 ```go
-btcAtomAMM := address.Module("amm", btc.Address() + atom.Address()})
+func Module(moduleName string, derivationKeys ...[]byte) []byte{
+	if len(derivationKeys) == 0 {
+		return authtypes.NewModuleAddress(modulenName)  // legacy case
+	}
+	submoduleAddress := Hash("module", []byte(moduleName) + 0 + key)
+	return fold((a, k) => Derive(a, k), subsubKeys, submoduleAddress)
+}
 ```
 
-#### Derived Addresses
-
-We must be able to cryptographically derive one address from another one. The derivation process must guarantee hash properties, hence we use the already defined `Hash` function:
+**Example 1**  A lending BTC pool address would be:
 
 ```go
-func Derive(address []byte, derivationKey []byte) []byte {
-    return Hash(addres, derivationKey)
-}
+btcPool := address.Module("lending", btc.Address()})
 ```
 
-Note: `Module` is a special case of the more general _derived_ address, where we set the `"module"` string for the _from address_.
+If we want to create an address for a module account depending on more than one key, we can concatenate them:
+
+```go
+btcAtomAMM := address.Module("amm", btc.Address() + atom.Address()})
+```
 
-**Example**  For a cosmwasm smart-contract address we could use the following construction:
+**Example 2**  a smart-contract address could be constructed by:
 
 ```go
-smartContractAddr := Derived(Module("cosmwasm", smartContractsNamespace), []{smartContractKey})
+smartContractAddr = Module("mySmartContractVM", smartContractsNamespace, smartContractKey})
+
+// which equals to:
+smartContractAddr = Derived(
+    Module("mySmartContractVM", smartContractsNamespace), 
+    []{smartContractKey})
 ```
 
 ### Schema Types
@@ -235,7 +248,7 @@ Example: all public key types have a unique protobuf message type similar to:
 package cosmos.crypto.sr25519;
 
 message PubKey {
-  bytes key = 1;
+	bytes key = 1;
 }
 ```
 

types/address/hash.go

@@ -6,6 +6,8 @@ import (
 	"fmt"
 	"sort"
 
+	"github.com/tendermint/tendermint/crypto"
+
 	"github.com/cosmos/cosmos-sdk/internal/conv"
 	"github.com/cosmos/cosmos-sdk/types/errors"
 )
@@ -18,7 +20,9 @@ type Addressable interface {
 	Address() []byte
 }
 
-// Hash creates a new address from address type and key
+// Hash creates a new address from address type and key.
+// The functions should only be used by new types defining their own address function
+// (eg public keys).
 func Hash(typ string, key []byte) []byte {
 	hasher := sha256.New()
 	_, err := hasher.Write(conv.UnsafeStrToBytes(typ))
@@ -59,13 +63,30 @@ func Compose(typ string, subAddresses []Addressable) ([]byte, error) {
 }
 
 // Module is a specialized version of a composed address for modules. Each module account
-// is constructed from a module name and module account key.
-func Module(moduleName string, key []byte) []byte {
-	mKey := append([]byte(moduleName), 0)
-	return Hash("module", append(mKey, key...))
+// is constructed from a module name and a sequence of derivation keys (at least one
+// derivation key must be provided). The derivation keys must be unique
+// in the module scope, and is usually constructed from some object id. Example, let's
+// a x/dao module, and a new DAO object, it's address would be:
+//
+//	address.Module(dao.ModuleName, newDAO.ID)
+func Module(moduleName string, derivationKeys ...[]byte) []byte {
+	mKey := []byte(moduleName)
+	if len(derivationKeys) == 0 { // fallback to the "traditional" ModuleAddress
+		return crypto.AddressHash(mKey)
+	}
+	// need to append zero byte to avoid potential clash between the module name and the first
+	// derivation key
+	mKey = append(mKey, 0)
+	addr := Hash("module", append(mKey, derivationKeys[0]...))
+	for _, k := range derivationKeys[1:] {
+		addr = Derive(addr, k)
+	}
+	return addr
 }
 
 // Derive derives a new address from the main `address` and a derivation `key`.
+// This function is used to create a sub accounts. To create a module accounts use the
+// `Module` function.
 func Derive(address []byte, key []byte) []byte {
 	return Hash(conv.UnsafeBytesToStr(address), key)
 }

types/address/hash_test.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/suite"
+	"github.com/tendermint/tendermint/crypto/tmhash"
 )
 
 func TestAddressSuite(t *testing.T) {
@@ -64,15 +65,30 @@ func (suite *AddressSuite) TestComposed() {
 func (suite *AddressSuite) TestModule() {
 	assert := suite.Assert()
 	modName, key := "myModule", []byte{1, 2}
+
+	addrLegacy := Module(modName)
+	assert.Equal(tmhash.SumTruncated([]byte(modName)), addrLegacy,
+		"when no derivation keys, we fall back to the legacy module address using sha256 of the module name")
+
 	addr := Module(modName, key)
-	assert.Len(addr, Len, "must have address length")
+	assert.Len(addr, Len, "must have correct address length")
+	assert.NotEqual(addrLegacy, addr,
+		"when derivation key is specified, it must generate non legacy module address")
 
 	addr2 := Module("myModule2", key)
 	assert.NotEqual(addr, addr2, "changing module name must change address")
 
-	addr3 := Module(modName, []byte{1, 2, 3})
+	k1 := []byte{1, 2, 3}
+	addr3 := Module(modName, k1)
 	assert.NotEqual(addr, addr3, "changing key must change address")
 	assert.NotEqual(addr2, addr3, "changing key must change address")
+
+	addr4 := Module(modName, k1, k1)
+	assert.Equal(Derive(addr3, k1), addr4)
+
+	k2 := []byte{0, 0, 7}
+	addr5 := Module(modName, k1, k1, k2)
+	assert.Equal(Derive(addr4, k2), addr5)
 }
 
 func (suite *AddressSuite) TestDerive() {

x/auth/types/account.go

@@ -12,6 +12,7 @@ import (
 	codectypes "github.com/cosmos/cosmos-sdk/codec/types"
 	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/types/address"
 )
 
 var (
@@ -156,7 +157,7 @@ func NewModuleAddressOrBech32Address(input string) sdk.AccAddress {
 
 // NewModuleAddress creates an AccAddress from the hash of the module's name
 func NewModuleAddress(name string) sdk.AccAddress {
-	return sdk.AccAddress(crypto.AddressHash([]byte(name)))
+	return address.Module(name)
 }
 
 // NewEmptyModuleAccount creates a empty ModuleAccount from a string

x/auth/types/credentials.go

@@ -32,25 +32,22 @@ const ModuleCredentialType = "ModuleCredential"
 
 var _ cryptotypes.PubKey = &ModuleCredential{}
 
-func NewModuleCredential(moduleName string, derivationKeys [][]byte) *ModuleCredential {
+// NewModuleCredential creates new module credential key.
+// At least one derivation key must be provided and all derivation keys must be not empty.
+func NewModuleCredential(moduleName string, derivationKeys ...[]byte) (*ModuleCredential, error) {
+	for i := range derivationKeys {
+		if len(derivationKeys[i]) == 0 {
+			return nil, fmt.Errorf("module credential derivation keys at index %d is empty", i)
+		}
+	}
 	return &ModuleCredential{
 		ModuleName:     moduleName,
 		DerivationKeys: derivationKeys,
-	}
+	}, nil
 }
 
 func (m *ModuleCredential) Address() cryptotypes.Address {
-	var addr []byte
-	for i, dk := range m.DerivationKeys {
-		if i == 0 {
-			addr = address.Module(m.ModuleName, dk)
-			continue
-		}
-
-		addr = address.Derive(addr, dk)
-	}
-
-	return addr
+	return address.Module(m.ModuleName, m.DerivationKeys...)
 }
 
 func (m *ModuleCredential) Bytes() []byte {

x/auth/types/credentials_test.go

@@ -10,23 +10,43 @@ import (
 )
 
 func TestNewModuleCrendentials(t *testing.T) {
+	// wrong derivation keys
+	_, err := authtypes.NewModuleCredential("group", []byte{})
+	require.Error(t, err, "derivation keys must be non empty")
+	_, err = authtypes.NewModuleCredential("group", [][]byte{{0x0, 0x30}, {}}...)
+	require.Error(t, err)
+
 	expected := sdk.MustAccAddressFromBech32("cosmos1fpn0w0yf4x300llf5r66jnfhgj4ul6cfahrvqsskwkhsw6sv84wsmz359y")
 
-	credential := authtypes.NewModuleCredential("group", [][]byte{{0x20}, {0x0}})
-	require.NoError(t, sdk.VerifyAddressFormat(credential.Address().Bytes()))
+	credential, err := authtypes.NewModuleCredential("group")
+	require.NoError(t, err, "must be able to create a Root Module credential (see ADR-33)")
+	require.NoError(t, sdk.VerifyAddressFormat(credential.Address()))
+
+	credential, err = authtypes.NewModuleCredential("group", [][]byte{{0x20}, {0x0}}...)
+	require.NoError(t, err)
+	require.NoError(t, sdk.VerifyAddressFormat(credential.Address()))
 	addr, err := sdk.AccAddressFromHexUnsafe(credential.Address().String())
 	require.NoError(t, err)
 	require.Equal(t, expected.String(), addr.String())
 
-	require.True(t, credential.Equals(authtypes.NewModuleCredential("group", [][]byte{{0x20}, {0x0}})))
-	require.False(t, credential.Equals(authtypes.NewModuleCredential("group", [][]byte{{0x20}, {0x1}})))
-	require.False(t, credential.Equals(authtypes.NewModuleCredential("group", [][]byte{{0x20}})))
+	c, err := authtypes.NewModuleCredential("group", [][]byte{{0x20}, {0x0}}...)
+	require.NoError(t, err)
+	require.True(t, credential.Equals(c))
+
+	c, err = authtypes.NewModuleCredential("group", [][]byte{{0x20}, {0x1}}...)
+	require.NoError(t, err)
+	require.False(t, credential.Equals(c))
+
+	c, err = authtypes.NewModuleCredential("group", []byte{0x20})
+	require.NoError(t, err)
+	require.False(t, credential.Equals(c))
 }
 
 func TestNewBaseAccountWithPubKey(t *testing.T) {
 	expected := sdk.MustAccAddressFromBech32("cosmos1fpn0w0yf4x300llf5r66jnfhgj4ul6cfahrvqsskwkhsw6sv84wsmz359y")
 
-	credential := authtypes.NewModuleCredential("group", [][]byte{{0x20}, {0x0}})
+	credential, err := authtypes.NewModuleCredential("group", [][]byte{{0x20}, {0x0}}...)
+	require.NoError(t, err)
 	account, err := authtypes.NewBaseAccountWithPubKey(credential)
 	require.NoError(t, err)
 	require.Equal(t, expected, account.GetAddress())

x/group/keeper/keeper_test.go

@@ -127,16 +127,17 @@ func (s TestSuite) setNextAccount() { //nolint:govet // this is a test and we're
 	derivationKey := make([]byte, 8)
 	binary.BigEndian.PutUint64(derivationKey, nextAccVal)
 
-	accountCredentials := authtypes.NewModuleCredential(group.ModuleName, [][]byte{{keeper.GroupPolicyTablePrefix}, derivationKey})
+	ac, err := authtypes.NewModuleCredential(group.ModuleName, []byte{keeper.GroupPolicyTablePrefix}, derivationKey)
+	s.Require().NoError(err)
 
-	groupPolicyAcc, err := authtypes.NewBaseAccountWithPubKey(accountCredentials)
+	groupPolicyAcc, err := authtypes.NewBaseAccountWithPubKey(ac)
 	s.Require().NoError(err)
 
-	groupPolicyAccBumpAccountNumber, err := authtypes.NewBaseAccountWithPubKey(accountCredentials)
+	groupPolicyAccBumpAccountNumber, err := authtypes.NewBaseAccountWithPubKey(ac)
 	s.Require().NoError(err)
 	groupPolicyAccBumpAccountNumber.SetAccountNumber(nextAccVal)
 
-	s.accountKeeper.EXPECT().GetAccount(gomock.Any(), sdk.AccAddress(accountCredentials.Address())).Return(nil).AnyTimes()
+	s.accountKeeper.EXPECT().GetAccount(gomock.Any(), sdk.AccAddress(ac.Address())).Return(nil).AnyTimes()
 	s.accountKeeper.EXPECT().NewAccount(gomock.Any(), groupPolicyAcc).Return(groupPolicyAccBumpAccountNumber).AnyTimes()
 	s.accountKeeper.EXPECT().SetAccount(gomock.Any(), sdk.AccountI(groupPolicyAccBumpAccountNumber)).Return().AnyTimes()
 }