Remove bech32 PubKey support #7477

robert-zaremba · 2020-10-07T16:12:12Z

Description

removing bech32 pubkeys functions
remove bech32 from keyring
removing code duplication

Closes: #7447

This task essentially removes bech32 for public keys in the storage and user layer (clients, outputs etc..).
We are NOT breaking the legacy REST API because it deprecated anyway and will be removed.

Summary

removed bech32 from user facing code
updated the keyring display structure
changed user/client pubkey serialization
added new tests for ed25519 as an example on how to properly serialize interfaces (both binary and JSON).
few API breaking changes:
Created NewJSONAnyMarshaler - a helper method to use codec.(Un)marsjalJSON withing a client context:

am := codec.NewJSONAnyMarshaler(ctx.JSONMarshaler, ctx.InterfaceRegistry)
err := codec.UnmarshalIfcJSON(am, &pk, []byte(pubKey))

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

x/staking/teststaking/handler.go

tac0turtle · 2020-10-08T10:22:03Z

It may be good to keep the removal of bech32 and removing code duplication/test helper package as two separate PRs. They are orthogonal changes.

If someone needs to come back to this PR to view the changes having it only be the removal of bech32, like the title describes, will make the review quicker

aaronc · 2020-10-08T13:41:31Z

It may be good to keep the removal of bech32 and removing code duplication/test helper package as two separate PRs. They are orthogonal changes.

If someone needs to come back to this PR to view the changes having it only be the removal of bech32, like the title describes, will make the review quicker

I tend to agree with this perspective. I know it's annoying and I've had to regretfully split up many of my PRs for reasons like this. In the end though, I think this approach is the best for the SDK.

robert-zaremba · 2020-10-08T14:12:33Z

It may be good to keep the removal of bech32 and removing code duplication/test helper package as two separate PRs. They are orthogonal changes.
If someone needs to come back to this PR to view the changes having it only be the removal of bech32, like the title describes, will make the review quicker

I tend to agree with this perspective. I know it's annoying and I've had to regretfully split up many of my PRs for reasons like this. In the end though, I think this approach is the best for the SDK.

Yes. I also agree, however when going through the code it was already in my head to refactor. I won't add more things. All this changes are related - all this edits would require doing extra error check. Instead, I moved it to a function.

robert-zaremba · 2020-10-13T18:30:09Z

I'm breaking this PR into 2, for the moment I've extracted a code for test refactor and will submit another PR for it.

clevinson · 2020-10-16T14:44:47Z

This probably also needs to include removing this from the keyring (cc @alexanderbez )

…ible

lgtm-com · 2020-11-03T16:22:16Z

This pull request introduces 2 alerts when merging faa822a into 30efa5a - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

server/tm_cmds.go

codec/json.go

orijbot · 2021-03-24T14:07:26Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=559fdfd75d03470f985ba43e1c0b3b1c

client/keys/codec.go

orijbot · 2021-03-24T14:19:41Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=b0b8f59e07d02cda98f00c7f4f40268f9794f586

tac0turtle

Generally looks good. The PR has a bit of scope creep but should be fine. Docs will follow in a different PR right?

blushi

Tested ACK!

This has enough reviews and my change request was addressed

orijbot · 2021-03-24T17:43:45Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=cf64a08b681247d3b7ce743819f0fbad

orijbot · 2021-03-24T17:43:51Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=0436894abe27cbf8a3afde4c29f54033a6c493f3

robert-zaremba · 2021-03-25T00:22:48Z

Any idea why the following test is failing so often?

 --- FAIL: TestPaginatedVotesQuery (0.00s)
    querier_test.go:336: 
        	Error Trace:	querier_test.go:336
        	Error:      	Not equal: 
        	            	expected: 19
        	            	actual  : 20
        	Test:       	TestPaginatedVotesQuery
FAIL

orijbot · 2021-03-25T10:21:11Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=75c36fc1e7b3ce70d1b32af80a5cda0b353b57ac

orijbot · 2021-03-25T10:41:20Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=c73c25fb449649a786df7adb819b2426

orijbot · 2021-03-25T11:04:05Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=1279ac69bc338d87f645720fb8621e493b57b598

orijbot · 2021-03-25T11:23:54Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=8bcd3afb5f4d42f8b3fd14ffe70b6b8a

alessio · 2021-03-25T12:06:12Z

Any idea why the following test is failing so often?

 --- FAIL: TestPaginatedVotesQuery (0.00s)
    querier_test.go:336: 
        	Error Trace:	querier_test.go:336
        	Error:      	Not equal: 
        	            	expected: 19
        	            	actual  : 20
        	Test:       	TestPaginatedVotesQuery
FAIL

Just a flaky test. But it's annoying and no one can really explain why this happens in the first. We definitely need to fix it.

orijbot · 2021-03-25T13:52:09Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=152ad45e85264f72c784f31146d38fa4068d1421

orijbot · 2021-03-25T14:53:36Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=7568b6680a58a5e974b9126734dd62f95eb43b1b

robert-zaremba · 2021-03-25T15:13:50Z

yeah, finally merged after 20 attempts to get the CI go through random errors.

orijbot · 2021-03-25T17:01:37Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=80b76a43a44a41bcb07add86cd78c8bf

robert-zaremba added C:Types C:Encoding labels Oct 7, 2020

robert-zaremba requested review from aaronc, alessio and alexanderbez as code owners October 7, 2020 16:12

robert-zaremba marked this pull request as draft October 7, 2020 16:12

aaronc reviewed Oct 7, 2020

View reviewed changes

x/staking/teststaking/handler.go Outdated Show resolved Hide resolved

robert-zaremba force-pushed the robert/rm-bech32-pk branch 2 times, most recently from c1b2f9a to 88e04f7 Compare October 7, 2020 21:27

robert-zaremba force-pushed the robert/rm-bech32-pk branch from 3eaa1f8 to 73c846b Compare October 13, 2020 18:27

robert-zaremba mentioned this pull request Oct 13, 2020

Refactor x/staking Validation and Delegation tests based on MsgCreateValidator.Pubkey type change. #7526

Merged

9 tasks

clevinson added this to the v0.40 [Stargate] milestone Oct 13, 2020

clevinson added the backport/0.40.x (Stargate) label Oct 13, 2020

clevinson mentioned this pull request Oct 19, 2020

[backports] v0.40.0-rc1 #7590

Merged

31 tasks

robert-zaremba mentioned this pull request Oct 20, 2020

Remove bech32 PubKey support #7447

Closed

9 tasks

Move PubKey bech32 to legacy package and migrate the usage where poss…

faa822a

…ible

robert-zaremba force-pushed the robert/rm-bech32-pk branch from 73c846b to faa822a Compare November 3, 2020 15:55

update /server

2324c4e

robert-zaremba force-pushed the robert/rm-bech32-pk branch from 3a9d4b0 to ea2c091 Compare November 3, 2020 23:00

wip

7df2182

robert-zaremba force-pushed the robert/rm-bech32-pk branch from ea2c091 to 7df2182 Compare November 3, 2020 23:04

robert-zaremba commented Nov 3, 2020

View reviewed changes

server/tm_cmds.go Outdated Show resolved Hide resolved

aaronc reviewed Nov 4, 2020

View reviewed changes

codec/json.go Outdated Show resolved Hide resolved

move proto json encoding helper functions to internal

9198817

tac0turtle reviewed Mar 24, 2021

View reviewed changes

client/keys/codec.go Outdated Show resolved Hide resolved

add link to github issue

b0b8f59

tac0turtle approved these changes Mar 24, 2021

View reviewed changes

blushi approved these changes Mar 24, 2021

View reviewed changes

aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 24, 2021

Merge branch 'master' into robert/rm-bech32-pk

0436894

Merge branch 'master' into robert/rm-bech32-pk

75c36fc

amaury1093 mentioned this pull request Mar 25, 2021

Update relevant docs to remove usage of bech32 pubkeys, and document new Any usage #8053

Closed

7 tasks

Merge branch 'master' into robert/rm-bech32-pk

1279ac6

Merge branch 'master' into robert/rm-bech32-pk

152ad45

mergify bot merged commit 7568b66 into master Mar 25, 2021

mergify bot deleted the robert/rm-bech32-pk branch March 25, 2021 14:53

amaury1093 mentioned this pull request Mar 26, 2021

Fix flaky CI tests #9010

Closed

17 tasks

adizere mentioned this pull request May 12, 2021

Add support for Protobuf-based Keyring informalsystems/hermes#925

Closed

7 tasks

hleb-albau mentioned this pull request Sep 29, 2021

decode multisig acc cosmos/cosmjs#882

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove bech32 PubKey support #7477

Remove bech32 PubKey support #7477

robert-zaremba commented Oct 7, 2020 •

edited

Loading

tac0turtle commented Oct 8, 2020 •

edited

Loading

aaronc commented Oct 8, 2020

robert-zaremba commented Oct 8, 2020

robert-zaremba commented Oct 13, 2020

clevinson commented Oct 16, 2020

lgtm-com bot commented Nov 3, 2020

orijbot commented Mar 24, 2021

orijbot commented Mar 24, 2021

tac0turtle left a comment •

edited

Loading

blushi left a comment

orijbot commented Mar 24, 2021

orijbot commented Mar 24, 2021

robert-zaremba commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

alessio commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

robert-zaremba commented Mar 25, 2021

orijbot commented Mar 25, 2021

Remove bech32 PubKey support #7477

Remove bech32 PubKey support #7477

Conversation

robert-zaremba commented Oct 7, 2020 • edited Loading

Description

Summary

tac0turtle commented Oct 8, 2020 • edited Loading

aaronc commented Oct 8, 2020

robert-zaremba commented Oct 8, 2020

robert-zaremba commented Oct 13, 2020

clevinson commented Oct 16, 2020

lgtm-com bot commented Nov 3, 2020

orijbot commented Mar 24, 2021

orijbot commented Mar 24, 2021

tac0turtle left a comment • edited Loading

Choose a reason for hiding this comment

blushi left a comment

Choose a reason for hiding this comment

orijbot commented Mar 24, 2021

orijbot commented Mar 24, 2021

robert-zaremba commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

alessio commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

robert-zaremba commented Mar 25, 2021

orijbot commented Mar 25, 2021

robert-zaremba commented Oct 7, 2020 •

edited

Loading

tac0turtle commented Oct 8, 2020 •

edited

Loading

tac0turtle left a comment •

edited

Loading