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

ADR-28 derive address functions #9088

Merged
merged 9 commits into from
Apr 15, 2021
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [\#8786](https://github.com/cosmos/cosmos-sdk/pull/8786) Enabled secp256r1 in x/auth.
* (rosetta) [\#8729](https://github.com/cosmos/cosmos-sdk/pull/8729) Data API fully supports balance tracking. Construction API can now construct any message supported by the application.
* [\#8754](https://github.com/cosmos/cosmos-sdk/pull/8875) Added support for reverse iteration to pagination.
* [#9088](https://github.com/cosmos/cosmos-sdk/pull/9088) Added implementation to ADR-28 Derived Addresses.

### Client Breaking Changes

Expand Down Expand Up @@ -78,6 +79,9 @@ Ref: https://keepachangelog.com/en/1.0.0/
* `MsgCreateValidator.Pubkey` type changed from `string` to `codectypes.Any`.
* (client) [\#8926](https://github.com/cosmos/cosmos-sdk/pull/8926) `client/tx.PrepareFactory` has been converted to a private function, as it's only used internally.
* (auth/tx) [\#8926](https://github.com/cosmos/cosmos-sdk/pull/8926) The `ProtoTxProvider` interface used as a workaround for transaction simulation has been removed.
* [#9088](https://github.com/cosmos/cosmos-sdk/pull/9088) Unify naming of the ADR-28 related functions: Renames: `address.NewCompose` -> `address.Compose`, `address.NewCompose`
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved



### State Machine Breaking

Expand Down
27 changes: 18 additions & 9 deletions docs/architecture/adr-028-public-key-addresses.md
Original file line number Diff line number Diff line change
Expand Up @@ -185,14 +185,14 @@ NOTE: this section is not finalize and it's in active discussion.

In Basic Address section we defined a module account address as:

```
```go
address.Hash("module", moduleName)
```

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 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.

```
```go
func Module(moduleName string, key []byte) []byte{
return Hash("module", []byte(moduleName) + 0 + key)
}
Expand All @@ -208,20 +208,29 @@ If we want to create an address for a module account depending on more than one
btcAtomAMM := address.Module("amm", btc.Addrress() + atom.Address()})
```

We can continue the derivation process and can create an address for a submodule account.
#### Derived Addresses

```
func Submodule(address []byte, derivationKey []byte) {
return Hash("module", address + derivationKey)
Module address is a special case of more general _derived_ address defined by the `Derive` function:
blushi marked this conversation as resolved.
Show resolved Hide resolved

```go
func Derive(address []byte, derivationKey []byte) {
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
return Hash(addres, derivationKey)
}
```

NOTE: if `address` is not a hash based address (with `LEN` length) then we should use `LengthPrefix`. An alternative would be to use one `Module` function, which takes a slice of keys and mapped with `LengthPrefix`. For final version we need to validate what's the most common use.


**Example** For a cosmwasm smart-contract address we could use the following construction:
```
smartContractAddr := Submodule(Module("cosmwasm", smartContractsNamespace), smartContractKey)
smartContractAddr := Derived(Module("cosmwasm", smartContractsNamespace), []{smartContractKey})
```

We can also define a function which will derive an address based on multiple keys (path). The function is similar to the `NewComposed`, however it doesn't sort the derivation keys:

```go
func DeriveMulti(address []byte, derivationKeys [][]byte) {
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
keys = map(derivationKeys, \k -> LengthPrefix(k))
return Hash(LengthPrefix(address), keys[0] + ... + keys[n])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit confusing for me that Derive doesn't length-prefix derivationKey, and DeriveMulti length-prefixes.

I understand Derive doesn't need legnth-prefixing. But isn't it just simpler to always use the same algorithm (i.e. always lenght-prefix), so that Derive == DeriveMulti with 1 derivationKey?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, we use length prefixing only if we want to glue a sequence of keys. So, for Derive it's unnecessary operation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, for Derive it's unnecessary operation.

Yes, but does it create more confusion? We could just:

func Derive(address []byte, derivationKey []byte) []byte {
    return DeriveMulti(address, []byte{derivationKey})
}

so that we have 1 algorithm only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the confusion will be there anyway: DeriveMulti doesn't decompose into multiple Derive calls (even with LengthPrfixing) - what's in the function comment:

// NOTE: DeriveMulti(addr, [k1, k2]) != Derive(Derive(addr, k1), k2)
//                                   != Derive(Derive(addr, k2), k1)

The motivation here was to use a composed derivation path, Normally we would put slashes into the segments. But here, the key can be anything, so we can't have any special separator.

So to summarize, we have 3 options:

  1. leave as is
  2. remove DeriveMulti, and not adding it later in the form described above.
  3. use @AmauryM suggestion to use

Maybe we don't need DeriveMuli - it seams that we will always have the parent key and will never derive into subusub-account without having a sub-account. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// NOTE: DeriveMulti(addr, [k1, k2]) != Derive(Derive(addr, k1), k2)
// != Derive(Derive(addr, k2), k1)

Another option:

  1. DeriveMulti is composition of Derive
func DeriveMulti(address []byte, derivationKeys [][]byte) {
  result := address
  for _, derivKey := range derivationKeys {
    result = Derive(result, derivKey)
  }
  return result
}

The 1st line of your NOTE is then an equality (less confusion), and we never use length-prefixing (also less confusion). The 2nd line stays an inequality, but in general compositions are never commutative.

Maybe we don't need DeriveMuli - it seams that we will always have the parent key and will never derive into subusub-account without having a sub-account. What do you think?

True, that also makes sense. Overall I'm actually in favor of Option 2: remove DeriveMulti for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's why I was more thinking about the Option 2. Again, the motivation for DeriveMulti was not to use composition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean - the motivation was to have a sequence of parameters, which don't compose, but use the whole sequence as a single parameter (as we have in the hardware wallets now).

}
```


Expand Down
25 changes: 23 additions & 2 deletions types/address/hash.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ func Hash(typ string, key []byte) []byte {
return hasher.Sum(nil)
}

// NewComposed creates a new address based on sub addresses.
func NewComposed(typ string, subAddresses []Addressable) ([]byte, error) {
// Compose creates a new address based on sub addresses.
func Compose(typ string, subAddresses []Addressable) ([]byte, error) {
blushi marked this conversation as resolved.
Show resolved Hide resolved
as := make([][]byte, len(subAddresses))
totalLen := 0
var err error
Expand Down Expand Up @@ -62,3 +62,24 @@ func Module(moduleName string, key []byte) []byte {
mKey := append([]byte(moduleName), 0)
return Hash("module", append(mKey, key...))
}

// Derive derives a new address from the main `address` and a derivation `key`.
func Derive(address []byte, key []byte) []byte {
return Hash(conv.UnsafeBytesToStr(address), key)
}

// DeriveMulti generalizes `Derive` function for multiple keys - `path`. The keys are order
// sensitive. Changing an order of the elements in the path will create a different key.
// NOTE: DeriveMulti(addr, [k1, k2]) != Derive(Derive(addr, k1), k2)
// != Derive(Derive(addr, k2), k1)
func DeriveMulti(address []byte, path [][]byte) ([]byte, error) {
key := []byte{}
for i, p := range path {
a, err := LengthPrefix(p)
if err != nil {
return nil, fmt.Errorf("a path key=%v at index=%d is not compatible [%w]", p, i, err)
}
key = append(key, a...)
}
return Hash(conv.UnsafeBytesToStr(address), key), nil
}
38 changes: 34 additions & 4 deletions types/address/hash_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func (suite *AddressSuite) TestComposed() {
a2 := addrMock{[]byte{21, 22}}

typ := "multisig"
ac, err := NewComposed(typ, []Addressable{a1, a2})
ac, err := Compose(typ, []Addressable{a1, a2})
assert.NoError(err)
assert.Len(ac, Len)

Expand All @@ -45,18 +45,18 @@ func (suite *AddressSuite) TestComposed() {
assert.Equal(ac, ac2, "NewComposed works correctly")

// changing order of addresses shouldn't impact a composed address
ac2, err = NewComposed(typ, []Addressable{a2, a1})
ac2, err = Compose(typ, []Addressable{a2, a1})
assert.NoError(err)
assert.Len(ac2, Len)
assert.Equal(ac, ac2, "NewComposed is not sensitive for order")

// changing a type should change composed address
ac2, err = NewComposed(typ+"other", []Addressable{a2, a1})
ac2, err = Compose(typ+"other", []Addressable{a2, a1})
assert.NoError(err)
assert.NotEqual(ac, ac2, "NewComposed must be sensitive to type")

// changing order of addresses shouldn't impact a composed address
ac2, err = NewComposed(typ, []Addressable{a1, addrMock{make([]byte, 300, 300)}})
ac2, err = Compose(typ, []Addressable{a1, addrMock{make([]byte, 300, 300)}})
assert.Error(err)
assert.Contains(err.Error(), "should be max 255 bytes, got 300")
}
Expand All @@ -75,6 +75,36 @@ func (suite *AddressSuite) TestModule() {
assert.NotEqual(addr2, addr3, "changing key must change address")
}

func (suite *AddressSuite) TestDerive() {
assert := suite.Assert()
var addr, key1, key2 = []byte{1, 2}, []byte{3, 4}, []byte{1, 2}
d1 := Derive(addr, key1)
d2 := Derive(addr, key2)
d3 := Derive(key1, key2)
assert.Len(d1, Len)
assert.Len(d2, Len)
assert.Len(d3, Len)

assert.NotEqual(d1, d2)
assert.NotEqual(d1, d3)
assert.NotEqual(d2, d3)
}

func (suite *AddressSuite) TestDeriveMulti() {
assert := suite.Assert()
var addr, key1, key2 = []byte{1, 2}, []byte{3, 4}, []byte{1, 2}
d1, err := DeriveMulti(addr, [][]byte{key1, key2})
assert.NoError(err)
d2, err := DeriveMulti(addr, [][]byte{key2, key1})
assert.NoError(err)

assert.NotEqual(d1, d2)
d3 := Derive(Derive(addr, key1), key2)
assert.NotEqual(d1, d3)
d3 = Derive(Derive(addr, key2), key1)
assert.NotEqual(d1, d3)
}

type addrMock struct {
Addr []byte
}
Expand Down