adr-028 address generation #8415

robert-zaremba · 2021-01-22T14:50:09Z

Description

ADR-028 implementation

closes: ADR-028 address generation algorithm #8414
closes: Extensible address format #5694

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

lgtm-com · 2021-01-22T15:16:38Z

This pull request introduces 1 alert when merging 5eb4e97 into 31fdee0 - view on LGTM.com

new alerts:

1 for Expression has no effect

lgtm-com · 2021-01-27T16:47:43Z

This pull request introduces 1 alert when merging 90a0a39 into e2f510a - view on LGTM.com

new alerts:

1 for Expression has no effect

codecov · 2021-01-27T18:05:37Z

Codecov Report

Merging #8415 (0d6325c) into master (a534a96) will increase coverage by 0.02%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master    #8415      +/-   ##
==========================================
+ Coverage   61.46%   61.48%   +0.02%     
==========================================
  Files         656      658       +2     
  Lines       37542    37569      +27     
==========================================
+ Hits        23074    23100      +26     
  Misses      12059    12059              
- Partials     2409     2410       +1

Impacted Files	Coverage Δ
codec/amino.go	`72.22% <ø> (ø)`
crypto/keyring/legacy.go	`44.77% <ø> (ø)`
crypto/keys/ed25519/ed25519.go	`69.38% <ø> (+2.04%)`	⬆️
types/errors/handle.go	`0.00% <0.00%> (ø)`
x/auth/types/account.go	`75.42% <ø> (ø)`
crypto/keys/secp256k1/secp256k1.go	`86.27% <100.00%> (-0.52%)`	⬇️
types/address/hash.go	`100.00% <100.00%> (ø)`
x/staking/types/validator.go	`62.50% <100.00%> (-0.39%)`	⬇️

robert-zaremba · 2021-02-08T23:59:51Z

The issue you described for ed25519 also effects secp25k1. There are networks using secp256k1 as consensus key.

secp256k1 is not a problem, because it's addresses won't change - in ADR-28 defines an exempt for secp256k1.

robert-zaremba · 2021-02-09T00:00:15Z

Let's continue a discussion re ed25519 in #8543

amaury1093

lgtm

amaury1093 · 2021-02-09T10:16:40Z

proto/cosmos/crypto/ed25519/keys.proto

+// It's needed for Any serialization and SDK compatibility.
+// It must not be used in a non Tendermint key context because it doesn't implement
+// ADR-28. Nevertheless, you will like to use ed25519 in app user level
+// then you must create a new proto message and follow ADR-28 for Address construction.


I think it could be useful to explicity say in the comments how the address is formed for these pubkeys.

aaronc · 2021-02-09T16:25:46Z

types/address/hash.go

+	hasher := sha256.New()
+	hasher.Write(unsafeStrToByteArray(typ))
+	th := hasher.Sum(nil)


Let's cache this?

It can be cached in a public key structure, not here. O you have some better idea?

blushi

Looks good overall! Just small nits and tests refactoring request.
I was also wondering about Submodule account addresses, don't we wanna add that too in here?

crypto/keys/ed25519/ed25519.go

types/address/hash_test.go

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

alessio · 2021-02-12T16:59:12Z

types/address/hash_test.go

@@ -0,0 +1,90 @@
+package address


Can we use test packages, please?

Suggested change

package address

package address_test

These are unit tests.

blushi

I was also wondering about Submodule account addresses, don't we wanna add that too in here?

It doesn't seem like this question has been addressed.

Lint command is still failing.

robert-zaremba · 2021-02-15T14:28:10Z

I was also wondering about Submodule account addresses, don't we wanna add that too in here?

It doesn't seem like this question has been addressed.

Sorry, I missed it. You mean the Submodule function from the ADR? We don't have a clear use-case for it. It's left in the ADR as a note for further investigation.

The linter was complaining locally re gocritics, not sure why it's there. I will check it.

robert-zaremba added 3 commits January 22, 2021 15:38

Optimize secp256k1 hashing

78f7d46

Add ADR-028 related functions

d9d677b

Update ed25519

5eb4e97

robert-zaremba added the C:Crypto label Jan 22, 2021

robert-zaremba requested review from aaronc, alessio and alexanderbez as code owners January 22, 2021 14:50

robert-zaremba requested review from amaury1093, blushi, ethanfrey and fdymylja January 22, 2021 14:50

robert-zaremba changed the title ~~Robert/adr 28 poc~~ adr-028 address generation Jan 22, 2021

robert-zaremba mentioned this pull request Jan 22, 2021

Implement ADR 028 and Address Follow-ups #7618

Closed

8 tasks

robert-zaremba marked this pull request as draft January 22, 2021 14:52

robert-zaremba mentioned this pull request Jan 27, 2021

Refactor store keys for variable-length addresses #8363

Merged

9 tasks

fix errors/handle

90a0a39

robert-zaremba added 2 commits January 27, 2021 18:53

fix build

8394e39

fix build

48b584d

robert-zaremba added 7 commits January 28, 2021 00:14

Add tests and update function names

d842642

Merge branch 'master' into robert/adr-28-poc

0c2a983

wip

d4a1d02

Merge branch 'master' into robert/adr-28-poc

9263dbd

Use LengthPrefix for composed addresses

0fe7b12

add tests for NewComposed

32710db

add module hash function

98cdf6d

robert-zaremba force-pushed the robert/adr-28-poc branch from 09592af to 98cdf6d Compare February 2, 2021 22:01

fix append

956edf0

add note to ed25519 about SDK support with regards to ADR-28

65a5105

Update ed25519 TestAddress

4951e95

amaury1093 approved these changes Feb 9, 2021

View reviewed changes

Adding Deprecated notes for ed25519.PrivKey

4e2f3c1

aaronc reviewed Feb 9, 2021

View reviewed changes

robert-zaremba requested review from aaronc and alessio February 9, 2021 21:07

clevinson assigned blushi and unassigned amaury1093 Feb 10, 2021

blushi suggested changes Feb 11, 2021

View reviewed changes

crypto/keys/ed25519/ed25519.go Outdated Show resolved Hide resolved

types/address/hash_test.go Outdated Show resolved Hide resolved

types/address/hash_test.go Show resolved Hide resolved

robert-zaremba and others added 2 commits February 12, 2021 09:38

Update crypto/keys/ed25519/ed25519.go

ff6ff40

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

Update types/address/hash_test.go

0a1773c

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

robert-zaremba requested a review from blushi February 12, 2021 14:31

alessio reviewed Feb 12, 2021

View reviewed changes

solve linter issues

66dcd6d

robert-zaremba requested a review from alessio February 13, 2021 00:17

blushi reviewed Feb 15, 2021

View reviewed changes

linter: remove gocritic

0d6325c

robert-zaremba requested a review from blushi February 15, 2021 14:31

robert-zaremba mentioned this pull request Feb 15, 2021

move all address related functions to /types/address package #8587

Open

4 tasks

blushi approved these changes Feb 15, 2021

View reviewed changes

Merge branch 'master' into robert/adr-28-poc

a2e5f89

robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 15, 2021

mergify bot merged commit adbf5a7 into master Feb 15, 2021

mergify bot deleted the robert/adr-28-poc branch February 15, 2021 15:32

ethanfrey mentioned this pull request Feb 16, 2021

Update Contract Address Derivation CosmWasm/wasmd#410

Closed

ntchjb mentioned this pull request Apr 2, 2021

Implemented request pool keeper bandprotocol/chain#24

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adr-028 address generation #8415

adr-028 address generation #8415

robert-zaremba commented Jan 22, 2021 •

edited

Loading

lgtm-com bot commented Jan 22, 2021

lgtm-com bot commented Jan 27, 2021

codecov bot commented Jan 27, 2021 •

edited

Loading

robert-zaremba commented Feb 8, 2021

robert-zaremba commented Feb 9, 2021

amaury1093 left a comment

amaury1093 Feb 9, 2021

aaronc Feb 9, 2021

robert-zaremba Feb 9, 2021

blushi left a comment

alessio Feb 12, 2021

robert-zaremba Feb 12, 2021

blushi left a comment

robert-zaremba commented Feb 15, 2021

adr-028 address generation #8415

adr-028 address generation #8415

Conversation

robert-zaremba commented Jan 22, 2021 • edited Loading

Description

lgtm-com bot commented Jan 22, 2021

lgtm-com bot commented Jan 27, 2021

codecov bot commented Jan 27, 2021 • edited Loading

Codecov Report

robert-zaremba commented Feb 8, 2021

robert-zaremba commented Feb 9, 2021

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Feb 9, 2021

Choose a reason for hiding this comment

aaronc Feb 9, 2021

Choose a reason for hiding this comment

robert-zaremba Feb 9, 2021

Choose a reason for hiding this comment

blushi left a comment

Choose a reason for hiding this comment

alessio Feb 12, 2021

Choose a reason for hiding this comment

robert-zaremba Feb 12, 2021

Choose a reason for hiding this comment

blushi left a comment

Choose a reason for hiding this comment

robert-zaremba commented Feb 15, 2021

robert-zaremba commented Jan 22, 2021 •

edited

Loading

codecov bot commented Jan 27, 2021 •

edited

Loading