ADR-28 derive address functions

[robert-zaremba]

Description

In ADR-28 we left over the Derive function while waiting for first use-cases and usage. Now we are need it for group module.

closes: #9033

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[amaury1093]

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?

[robert-zaremba]

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

[amaury1093]

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.

[robert-zaremba]

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?

[amaury1093]

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

Another option:

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

[robert-zaremba]

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

[robert-zaremba]

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

[alessio]

@robert-zaremba mind resolving the conflicts, please?

[amaury1093]

lgtm, thanks

[codecov]

Codecov Report

Merging #9088 (7ee294b) into master (ef69863) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 7ee294b differs from pull request most recent head ba1b14b. Consider uploading reports for the commit ba1b14b to get more accurate results

@@           Coverage Diff           @@
##           master    #9088   +/-   ##
=======================================
  Coverage   58.79%   58.79%           
=======================================
  Files         583      583           
  Lines       32750    32752    +2     
=======================================
+ Hits        19255    19257    +2     
  Misses      11218    11218           
  Partials     2277     2277           

Impacted Files	Coverage Δ
types/address/hash.go	100.00% <100.00%> (ø)
x/bank/types/key.go	80.00% <0.00%> (ø)