refactor!: Kerying migration #9222

cyberbono3 · 2021-04-28T16:01:21Z

Description

This is draft PR
closes: #7108

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-04-28T16:26:49Z

This pull request introduces 1 alert when merging bcd9f81 into a2911d0 - view on LGTM.com

new alerts:

1 for Expression has no effect

crypto/keyring/keyring.go

amaury1093

pubkeys should be Any, not bytes, and we shouldn't cast to "crypto/ed25519.PublicKey"
A lot of information is already in pubkey (threshold, algo...), we don't need to duplicate anymore.

I'm also proposing a new model for representing *Info, using oneof.

proto/cosmos/crypto/keyring/types.proto

…rotoUnmarshalKeyringEntry

…hal methods

cyberbono3 · 2021-04-30T19:08:15Z

@robert-zaremba Can you review it, please?

robert-zaremba

We need to update the proto definitions. Amaury has a good idea how to construct the KeyEntry. Moreover:

the migration is not correct
everywhere else (outside migration) we must use proto encoding
we need to add tests

crypto/keyring/info.go

crypto/keyring/keyring.go

proto/cosmos/crypto/keyring/types.proto

crypto/keyring/keyring.go

proto/cosmos/crypto/keyring/types.proto

crypto/keyring/keyring.go

lgtm-com · 2021-07-13T11:24:40Z

This pull request introduces 1 alert when merging b0685fe into c23f386 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-07-13T14:15:18Z

This pull request introduces 1 alert when merging 24f44e8 into c23f386 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-07-13T15:20:48Z

This pull request introduces 1 alert when merging 8f239df into c23f386 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-07-13T16:54:58Z

This pull request introduces 1 alert when merging 8652e00 into 7040387 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

amaury1093

some initial comments

amaury1093 · 2021-07-20T10:53:03Z

.gitignore

+private/*.sh
+test


Suggested change

private/*.sh

test

amaury1093 · 2021-07-20T10:56:19Z

client/keys/add.go

@@ -118,8 +118,7 @@ func runAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *buf
 	}

 	if dryRun, _ := cmd.Flags().GetBool(flags.FlagDryRun); dryRun {
-		// use in memory keybase
-		kb = keyring.NewInMemory()
+		kb = keyring.NewInMemory(ctx.Codec)


Suggested change

kb = keyring.NewInMemory(ctx.Codec)

// use in memory keybase

kb = keyring.NewInMemory(ctx.Codec)

amaury1093 · 2021-07-20T10:59:19Z

crypto/keyring/legacy_info.go

@@ -1,50 +1,51 @@
 package keyring


how about doing like in module, and creating a crypto/keyring/migrations package. Then all migration logic (including this legacyInfo) can go in that package.

amaury1093 · 2021-07-20T10:59:44Z

proto/cosmos/crypto/hd/hd.proto

@@ -0,0 +1,18 @@
+syntax = "proto3";
+package cosmos.crypto.hd;


Suggested change

package cosmos.crypto.hd;

package cosmos.crypto.hd.v1;

amaury1093 · 2021-07-20T11:00:40Z

proto/cosmos/crypto/keyring/types.proto

+  oneof item {
+    Local local = 3;
+    Ledger ledger  = 4;
+    Empty empty  = 5;


Why are we using empty? I think using Multi and Offline would be clearer, even both of them are empty structs

amaury1093 · 2021-07-20T11:31:26Z

superseded by #9695

@robert-zaremba

## Description The draft PR #9222 Closes: #7108  - implement proto definition for `Record` - rename `Info.go` to `legacyInfo.go` within `keyring` package - implement CLI `migrate` command that migrates all keys from legacyInfo to proto according to @robert-zaremba migration [algorithm](#9222) - remove legacy keybase entirely. - add `Migrate` and `MigrateAll` functions in `keyring.go` for single key and all keys migration - add tests - fix tests --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

robert-zaremba and others added 6 commits April 16, 2021 17:53

Keyring migration from amino to proto

c727fa6

adding proto definitions - wip

7505e5a

add rename in the test

9160257

define proto structs for all structs in keyring/info.go

16eebea

generate types.pb.go

6ae658d

WIP keyring migration

bcd9f81

github-actions bot added the T:Docs Changes and features related to documentation. label Apr 28, 2021

cyberbono3 requested a review from robert-zaremba April 28, 2021 16:01

cyberbono3 commented Apr 28, 2021

View reviewed changes

crypto/keyring/keyring.go Outdated Show resolved Hide resolved

amaury1093 suggested changes Apr 29, 2021

View reviewed changes

new proto model,gen types.pb.go and define protoMarshalKeyringEntry,p…

36311a3

…rotoUnmarshalKeyringEntry

cyberbono3 force-pushed the kerying-migration branch from f42ca2b to 36311a3 Compare April 29, 2021 22:04

cyberbono3 added 2 commits April 29, 2021 16:02

KeyringEntry satisfies Info interface

c9ae124

fix KeyringEntry getters

2421d02

cyberbono3 force-pushed the kerying-migration branch from ca7ff77 to 2421d02 Compare April 30, 2021 17:56

WIP add codec to keystore and dbKeybase, fix protoMarshal,protoUnmars…

ea31d04

…hal methods

robert-zaremba reviewed May 2, 2021

View reviewed changes

amaury1093 assigned robert-zaremba and amaury1093 May 3, 2021

cyberbono3 added 3 commits May 4, 2021 07:31

apply some of reviewers changes

23a2316

fix protos

1f0ae05

fix keyring.New

059aa39

cyberbono3 commented May 4, 2021

View reviewed changes

crypto/keyring/keyring.go Outdated Show resolved Hide resolved

amaury1093 reviewed May 4, 2021

View reviewed changes

crypto/keyring/keyring.go Outdated Show resolved Hide resolved

crypto/keyring/keyring.go Outdated Show resolved Hide resolved

pass *codec.AminoCodec as function argument in keyring.New

3a420d2

cyberbono3 force-pushed the kerying-migration branch from d82c38c to 3a420d2 Compare May 4, 2021 15:43

cyberbono3 added 2 commits May 4, 2021 12:35

replace codec.AmintoCodec by codec.Codec

52426d4

merge master to kerying-migration

9a4bd80

cyberbono3 added 3 commits July 13, 2021 13:11

Merge branch 'master' into kerying-migration

c691246

remove dead code

5514769

gofmt

b0685fe

github-actions bot removed C:x/params C:x/mint C:x/crisis C:x/slashing C:x/evidence labels Jul 13, 2021

cyberbono3 force-pushed the kerying-migration branch 2 times, most recently from d3de827 to b0685fe Compare July 13, 2021 12:41

cyberbono3 added 2 commits July 13, 2021 16:32

refactor keyring_test.go

4d8c0e8

add TestMigrateErrEmptyItemData to migration_test.go

24f44e8

cyberbono3 added 3 commits July 13, 2021 17:43

set proper codec in genaccounts.go

fe572c9

set proper codec in testnet.go

dda1851

remove some comments from x/authz/client/testutil/tx.go

8f239df

cyberbono3 added 2 commits July 13, 2021 18:25

fix lint comments

1401291

fix ineffectual assignment to err in 2 suite files

7beebb2

cyberbono3 force-pushed the kerying-migration branch from 35009b3 to 7beebb2 Compare July 13, 2021 15:57

update proto files

8652e00

cyberbono3 force-pushed the kerying-migration branch from a7c3b2e to 8652e00 Compare July 14, 2021 07:09

cyberbono3 mentioned this pull request Jul 14, 2021

refactor!: Keyring migration #9695

Merged

19 tasks

amaury1093 reviewed Jul 20, 2021

View reviewed changes

cyberbono3 closed this Jul 20, 2021

amaury1093 deleted the kerying-migration branch July 20, 2021 11:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: Kerying migration #9222

refactor!: Kerying migration #9222

cyberbono3 commented Apr 28, 2021

lgtm-com bot commented Apr 28, 2021

amaury1093 left a comment

cyberbono3 commented Apr 30, 2021 •

edited

Loading

robert-zaremba left a comment

lgtm-com bot commented Jul 13, 2021

lgtm-com bot commented Jul 13, 2021

lgtm-com bot commented Jul 13, 2021

lgtm-com bot commented Jul 13, 2021

amaury1093 left a comment

amaury1093 Jul 20, 2021

amaury1093 Jul 20, 2021

amaury1093 Jul 20, 2021

amaury1093 Jul 20, 2021

amaury1093 Jul 20, 2021

amaury1093 commented Jul 20, 2021

	kb = keyring.NewInMemory(ctx.Codec)
	// use in memory keybase
	kb = keyring.NewInMemory(ctx.Codec)

refactor!: Kerying migration #9222

refactor!: Kerying migration #9222

Conversation

cyberbono3 commented Apr 28, 2021

Description

lgtm-com bot commented Apr 28, 2021

amaury1093 left a comment

Choose a reason for hiding this comment

cyberbono3 commented Apr 30, 2021 • edited Loading

robert-zaremba left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Jul 13, 2021

lgtm-com bot commented Jul 13, 2021

lgtm-com bot commented Jul 13, 2021

lgtm-com bot commented Jul 13, 2021

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Jul 20, 2021

Choose a reason for hiding this comment

amaury1093 Jul 20, 2021

Choose a reason for hiding this comment

amaury1093 Jul 20, 2021

Choose a reason for hiding this comment

amaury1093 Jul 20, 2021

Choose a reason for hiding this comment

amaury1093 Jul 20, 2021

Choose a reason for hiding this comment

amaury1093 commented Jul 20, 2021

cyberbono3 commented Apr 30, 2021 •

edited

Loading