Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate x/staking to Protobuf #5600

Merged
merged 26 commits into from
Feb 6, 2020
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
dec703b
Migrate staking types to proto
alexanderbez Feb 1, 2020
31311cb
Use int32 for validator status
alexanderbez Feb 3, 2020
0d2938b
Update staking types
alexanderbez Feb 3, 2020
60dc367
Add benchmarks for bech32 pubkey funcs
alexanderbez Feb 3, 2020
f0f6f50
Update BondStatus type alias to int32
alexanderbez Feb 3, 2020
c268a9e
Remove PubKey proto type
alexanderbez Feb 3, 2020
95edc79
Fix unit test
alexanderbez Feb 3, 2020
46be4b4
Update staking keeper
alexanderbez Feb 3, 2020
7503396
Fix staking keeper tests
alexanderbez Feb 3, 2020
97ac0b5
Update query client
alexanderbez Feb 3, 2020
8e84801
Update staking genesis and app_test
alexanderbez Feb 3, 2020
c35939a
Update staking handler
alexanderbez Feb 4, 2020
febaae8
Use Int64Value instead of IntProto
alexanderbez Feb 4, 2020
5b83d9b
Updates tests and APIs
alexanderbez Feb 4, 2020
0a904f1
Fix iteration
alexanderbez Feb 4, 2020
2450409
Linting
alexanderbez Feb 4, 2020
97148e2
Fix linting
alexanderbez Feb 4, 2020
da9d51f
Merge branch 'master' into bez/5444-staking-proto-enc
alexanderbez Feb 4, 2020
11a0d8b
Add changelog entry
alexanderbez Feb 4, 2020
db45cfc
Update changelog entry
alexanderbez Feb 4, 2020
c5a7baa
Merge branch 'master' into bez/5444-staking-proto-enc
alexanderbez Feb 4, 2020
3e1b445
Merge branch 'master' into bez/5444-staking-proto-enc
alexanderbez Feb 5, 2020
d31577c
Start boilerplate for app-level codec
alexanderbez Feb 5, 2020
3dcfea6
Merge branch 'master' into bez/5444-staking-proto-enc
alexanderbez Feb 6, 2020
05ceef7
Merge branch 'master' into bez/5444-staking-proto-enc
alexanderbez Feb 6, 2020
c84d960
Lint proto and update codecov settings to ignore proto codegen
alexanderbez Feb 6, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ issues:
- text: "ST1003:"
linters:
- stylecheck
- text: "ST1016:"
linters:
- stylecheck

linters-settings:
dogsled:
Expand Down
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ balances or a single balance by denom when the `denom` query parameter is presen
* Callers to `NewBaseVestingAccount` are responsible for verifying account balance in relation to
the original vesting amount.
* The `SendKeeper` and `ViewKeeper` interfaces in `x/bank` have been modified to account for changes.
* (staking) [\#5600](https://github.com/cosmos/cosmos-sdk/pull/5600) Migrate the `x/staking` module to use Protocol Buffer for state
serialization instead of Amino. The exact codec used is `codec.HybridCodec` which utilizes Protobuf for binary encoding and Amino
for JSON encoding.
* `BondStatus` is now of type `int32` instead of `byte`.
* Types of `int16` in the `Params` type are now of type `int32`.
* Every reference of `crypto.Pubkey` in context of a `Validator` is now of type string. `GetPubKeyFromBech32` must be used to get the `crypto.Pubkey`.
* The `Keeper` constructor now takes a `codec.Marshaler` instead of a concrete Amino codec. This exact type
provided is specified by `ModuleCdc`.

### Improvements

Expand Down
8 changes: 4 additions & 4 deletions codec/amino.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ func RegisterEvidences(cdc *Codec) {
// MarshalJSONIndent provides a utility for indented JSON encoding of an object
// via an Amino codec. It returns an error if it cannot serialize or indent as
// JSON.
func MarshalJSONIndent(cdc *Codec, obj interface{}) ([]byte, error) {
bz, err := cdc.MarshalJSON(obj)
func MarshalJSONIndent(m JSONMarshaler, obj interface{}) ([]byte, error) {
bz, err := m.MarshalJSON(obj)
if err != nil {
return nil, err
}
Expand All @@ -60,8 +60,8 @@ func MarshalJSONIndent(cdc *Codec, obj interface{}) ([]byte, error) {
}

// MustMarshalJSONIndent executes MarshalJSONIndent except it panics upon failure.
func MustMarshalJSONIndent(cdc *Codec, obj interface{}) []byte {
bz, err := MarshalJSONIndent(cdc, obj)
func MustMarshalJSONIndent(m JSONMarshaler, obj interface{}) []byte {
bz, err := MarshalJSONIndent(m, obj)
if err != nil {
panic(fmt.Sprintf("failed to marshal JSON: %s", err))
}
Expand Down
4 changes: 4 additions & 0 deletions codec/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ type (
UnmarshalBinaryLengthPrefixed(bz []byte, ptr ProtoMarshaler) error
MustUnmarshalBinaryLengthPrefixed(bz []byte, ptr ProtoMarshaler)

JSONMarshaler
}

JSONMarshaler interface {
MarshalJSON(o interface{}) ([]byte, error) // nolint: stdmethods
MustMarshalJSON(o interface{}) []byte

Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@aaronc aaronc Feb 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@alexanderbez alexanderbez Feb 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

github.com/gorilla/mux v1.7.3
github.com/hashicorp/golang-lru v0.5.4
github.com/mattn/go-isatty v0.0.12
Expand Down
2 changes: 1 addition & 1 deletion simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)
app.MintKeeper = mint.NewKeeper(
app.cdc, keys[mint.StoreKey], app.subspaces[mint.ModuleName], &stakingKeeper,
Expand Down
Loading