Migrate x/staking to Protobuf #5600

alexanderbez · 2020-02-01T19:36:03Z

Migrate x/staking to protocol buffer encoding:

Update third_party/ to include Tendermint dependencies
Update BondStatus from byte to int32
Update params to use int32 instead of int16
Use string to represent validator public key
Remove a lot of JSON/YAML encoding cruft for the Validator type

Components that still use Amino:

Client (CLI/REST) for JSON
Genesis state for JSON
Message signing
Querier

ref: #5444

For contributor use:

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

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

x/staking/types/types.proto

x/staking/keeper/query_utils.go

x/staking/types/delegation.go

types/staking.go

x/staking/types/delegation.go

tac0turtle

aaronc · 2020-02-05T17:21:11Z

go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/cosmos/ledger-cosmos-go v0.11.1
 	github.com/gogo/protobuf v1.3.1
 	github.com/golang/mock v1.3.1-0.20190508161146-9fa652df1129
+	github.com/golang/protobuf v1.3.2


Why is golang/protobuf required? This likely means there's a directive needed to make a golang/protobuf type point to a gogo/protobuf type.

Because of our dependency on

import "google/protobuf/timestamp.proto"; import "google/protobuf/duration.proto";

e.g.

message Commission { option (gogoproto.equal) = true; option (gogoproto.goproto_stringer) = false; // ... google.protobuf.Timestamp update_time = 2 [ (gogoproto.nullable) = false, (gogoproto.stdtime) = true, (gogoproto.moretags) = "yaml:\"update_time\"" ]; }

In addition, gogo proto itself depends on google/protobuf. Does gogo proto itself provide these? If so, what advantage is there with one over the other?

You need this in protocgen.sh:

protoc -I=. -I=$GOPATH/src -I=$GOPATH/src/github.com/gogo/protobuf/protobuf --{binary}_out=\ Mgoogle/protobuf/any.proto=github.com/gogo/protobuf/types,\ Mgoogle/protobuf/duration.proto=github.com/gogo/protobuf/types,\ Mgoogle/protobuf/struct.proto=github.com/gogo/protobuf/types,\ Mgoogle/protobuf/timestamp.proto=github.com/gogo/protobuf/types,\ Mgoogle/protobuf/wrappers.proto=github.com/gogo/protobuf/types:. \ myproto.proto

See https://github.com/gogo/protobuf#more-speed-and-more-generated-code

aaronc · 2020-02-05T17:55:34Z

simapp/app.go

@@ -180,7 +180,7 @@ func NewSimApp(
 		app.cdc, keys[supply.StoreKey], app.AccountKeeper, app.BankKeeper, maccPerms,
 	)
 	stakingKeeper := staking.NewKeeper(
-		app.cdc, keys[staking.StoreKey], app.BankKeeper, app.SupplyKeeper, app.subspaces[staking.ModuleName],
+		staking.ModuleCdc, keys[staking.StoreKey], app.BankKeeper, app.SupplyKeeper, app.subspaces[staking.ModuleName],


Why is the ModuleCdc being used here? I thought we were trying to deprecate those anyway, and wouldn't the app codec necessarily include everything ModuleCdc has?

Yes, eventually in the app constructor we will pass a singular codec that fulfills all the module interfaces. However, we can't really do this until we wrap up all the modules.

I suppose I can start that groundwork here (singular codec) 👍 .

I've created AppCodec. Please review. Is this in line with what you had in mind?

Yes, makes sense from what I've seen so far.

aaronc · 2020-02-05T17:58:04Z

x/staking/client/cli/query.go

@@ -77,7 +77,12 @@ $ %s query staking validator cosmosvaloper1gghjut3ccd8ay0zduzj64hwre2fxs9ldmqhff
 				return fmt.Errorf("no validator found with address %s", addr)
 			}

-			return cliCtx.PrintOutput(types.MustUnmarshalValidator(cdc, res))
+			validator, err := types.UnmarshalValidator(types.ModuleCdc, res)


Is the idea here to more generally move away from panicking? I think that makes sense esp for CLI

Absolutely, we should never panic in client code. There are remnants of Must* and I've been migrating that as I go.

I would totally appreciate it if you do the same in your PRs 👍

aaronc · 2020-02-05T22:06:10Z

simapp/app.go

@@ -180,7 +172,7 @@ func NewSimApp(
 		app.cdc, keys[supply.StoreKey], app.AccountKeeper, app.BankKeeper, maccPerms,
 	)
 	stakingKeeper := staking.NewKeeper(
-		app.cdc, keys[staking.StoreKey], app.BankKeeper, app.SupplyKeeper, app.subspaces[staking.ModuleName],
+		appCodec.Staking, keys[staking.StoreKey], app.BankKeeper, app.SupplyKeeper, app.subspaces[staking.ModuleName],


Okay, I get it now. Would keepers also receive a Marshaler instance? Would AppCodec include Marshaler?

Would keepers also receive a Marshaler instance?

Yes! Unless they need a custom interface like x/auth that needs to know how to decode its custom types.

codecov · 2020-02-06T16:21:16Z

Codecov Report

Merging #5600 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #5600      +/-   ##
==========================================
- Coverage   44.54%   44.54%   -0.01%     
==========================================
  Files         324      324              
  Lines       24670    24672       +2     
==========================================
  Hits        10990    10990              
- Misses      12623    12625       +2     
  Partials     1057     1057

Impacted Files	Coverage Δ
client/context/query.go	`1.86% <0%> (-0.04%)`	⬇️

tac0turtle · 2020-02-06T16:46:35Z

do we want to generate the .pb_test.go files when we generate the files, would help with not killing codecov

aaronc · 2020-02-06T17:10:42Z

do we want to generate the .pb_test.go files when we generate the files, would help with not killing codecov

Sounds like a good idea. I can enable it globally in the cosmos-proto shim. For now we can use testgen_all.

tac0turtle · 2020-02-06T17:17:55Z

we would have to enable populate_all along side it. might be better to just ignore *.pb.go files. unless people want it

Migrate staking types to proto

dec703b

alexanderbez added C:x/staking WIP T: State Machine Breaking State machine breaking changes (impacts consensus). labels Feb 1, 2020

tac0turtle reviewed Feb 2, 2020

View reviewed changes

x/staking/types/types.proto Outdated Show resolved Hide resolved

alexanderbez added 14 commits February 3, 2020 09:35

Use int32 for validator status

31311cb

Update staking types

0d2938b

Add benchmarks for bech32 pubkey funcs

60dc367

Update BondStatus type alias to int32

f0f6f50

Remove PubKey proto type

c268a9e

Fix unit test

95edc79

Update staking keeper

46be4b4

Fix staking keeper tests

7503396

Update query client

97ac0b5

Update staking genesis and app_test

8e84801

Update staking handler

c35939a

Use Int64Value instead of IntProto

febaae8

Updates tests and APIs

5b83d9b

Fix iteration

0a904f1

golangcibot reviewed Feb 4, 2020

View reviewed changes

x/staking/keeper/query_utils.go Outdated Show resolved Hide resolved

x/staking/types/delegation.go Outdated Show resolved Hide resolved

tac0turtle reviewed Feb 4, 2020

View reviewed changes

types/staking.go Show resolved Hide resolved

Linting

2450409

golangcibot reviewed Feb 4, 2020

View reviewed changes

x/staking/types/delegation.go Outdated Show resolved Hide resolved

alexanderbez and others added 4 commits February 4, 2020 12:03

Fix linting

97148e2

Merge branch 'master' into bez/5444-staking-proto-enc

da9d51f

Add changelog entry

11a0d8b

Update changelog entry

db45cfc

alexanderbez added R4R and removed WIP labels Feb 4, 2020

Merge branch 'master' into bez/5444-staking-proto-enc

c5a7baa

alexanderbez marked this pull request as ready for review February 4, 2020 19:00

alexanderbez requested review from alessio, fedekunze, jackzampolin and rigelrozanski as code owners February 4, 2020 19:00

alexanderbez requested review from AdityaSripal and tac0turtle February 4, 2020 19:01

tac0turtle approved these changes Feb 4, 2020

View reviewed changes

fedekunze approved these changes Feb 4, 2020

View reviewed changes

Merge branch 'master' into bez/5444-staking-proto-enc

3e1b445

aaronc reviewed Feb 5, 2020

View reviewed changes

Start boilerplate for app-level codec

d31577c

aaronc reviewed Feb 5, 2020

View reviewed changes

alexanderbez and others added 2 commits February 6, 2020 10:11

Merge branch 'master' into bez/5444-staking-proto-enc

3dcfea6

Merge branch 'master' into bez/5444-staking-proto-enc

05ceef7

Lint proto and update codecov settings to ignore proto codegen

c84d960

alexanderbez merged commit 53bf227 into master Feb 6, 2020

alexanderbez deleted the bez/5444-staking-proto-enc branch February 6, 2020 19:21

alexanderbez mentioned this pull request Feb 11, 2020

Full Proto Encoding #5444

Closed

28 tasks

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

Merge PR cosmos#5600: Migrate x/staking to Protobuf

ca483bf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate x/staking to Protobuf #5600

Migrate x/staking to Protobuf #5600

alexanderbez commented Feb 1, 2020 •

edited

Loading

tac0turtle left a comment

aaronc Feb 5, 2020 •

edited

Loading

alexanderbez Feb 5, 2020 •

edited

Loading

aaronc Feb 5, 2020

aaronc Feb 5, 2020

alexanderbez Feb 5, 2020

alexanderbez Feb 5, 2020

aaronc Feb 6, 2020

aaronc Feb 5, 2020

alexanderbez Feb 5, 2020

aaronc Feb 6, 2020

aaronc Feb 5, 2020

alexanderbez Feb 6, 2020

codecov bot commented Feb 6, 2020 •

edited

Loading

tac0turtle commented Feb 6, 2020

aaronc commented Feb 6, 2020

tac0turtle commented Feb 6, 2020 •

edited

Loading

Migrate x/staking to Protobuf #5600

Migrate x/staking to Protobuf #5600

Conversation

alexanderbez commented Feb 1, 2020 • edited Loading

tac0turtle left a comment

Choose a reason for hiding this comment

aaronc Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

alexanderbez Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2020 • edited Loading

Codecov Report

tac0turtle commented Feb 6, 2020

aaronc commented Feb 6, 2020

tac0turtle commented Feb 6, 2020 • edited Loading

alexanderbez commented Feb 1, 2020 •

edited

Loading

aaronc Feb 5, 2020 •

edited

Loading

alexanderbez Feb 5, 2020 •

edited

Loading

codecov bot commented Feb 6, 2020 •

edited

Loading

tac0turtle commented Feb 6, 2020 •

edited

Loading