privval: migrate to protobuf#4813
Conversation
|
👋 Thanks for creating a PR! Before we can merge this PR, please make sure that all the following items have been
Thank you for your contribution to Tendermint! 🚀 |
privval/file_test.go
Outdated
| "type": "tendermint/PrivKeyEd25519", | ||
| "value": "%s" | ||
| } | ||
| "pubKey": {"ed25519":"%s"}, |
There was a problem hiding this comment.
what is the preferred way of handling this transition for validators? should we maintain backwards compatibility or have a migration path.
There was a problem hiding this comment.
I'm curious about this too - it has implications for the RPC API as well, and a few other places. If we're going to break this, we might as well do it now since we're breaking everything else with Protobuf, but it's a lot of breakage to deal with.
There was a problem hiding this comment.
Having given this some thought, I think the new scheme makes more sense (using a key per variant instead of a type field), since it can represent an actual well-defined schema instead of having a random assortment of fields depending on the type field. This also simplifies decoding, since we don't have to do it in two passes (first to get the type field, then to decode the contents). My vote is 👍.
There was a problem hiding this comment.
should be fine as long as there's an upgrade path
…rmint into marko/canonical
|
lint is failing because of things on master. working on fixing those now |
* change voteSignBytes & proposalSignBytes to accept a proto type
libs/encode/json.go
Outdated
| // MarshalJSONIndent provides an auxiliary function to return Proto3 indented | ||
| // JSON encoded bytes of a message. | ||
| func MarshalJSONIndent(msg proto.Message) ([]byte, error) { | ||
| jm := &jsonpb.Marshaler{EmitDefaults: false, OrigName: false, Indent: " "} |
There was a problem hiding this comment.
can we make this a global variable to avoid recreating it every time we want to encode something?
privval/file.go
Outdated
| err = cdc.UnmarshalJSON(keyJSONBytes, &pvKey) | ||
| if err != nil { | ||
| pvK := privvalproto.FilePVKey{} | ||
| if err := jsonpb.Unmarshal(strings.NewReader(string(keyJSONBytes)), &pvK); err != nil { |
There was a problem hiding this comment.
| if err := jsonpb.Unmarshal(strings.NewReader(string(keyJSONBytes)), &pvK); err != nil { | |
| if err := jsonpb.Unmarshal(bytes.NewReader(keyJSONBytes), &pvK); err != nil { |
privval/file.go
Outdated
| } | ||
| err = cdc.UnmarshalJSON(stateJSONBytes, &pvState) | ||
| if err != nil { | ||
| if err := jsonpb.Unmarshal(strings.NewReader(string(stateJSONBytes)), &pvS); err != nil { |
| return | ||
| } | ||
|
|
||
| const maxRemoteSignerMsgSize = 1024 * 10 |
There was a problem hiding this comment.
please leave the const https://en.wikipedia.org/wiki/Magic_number_(programming)#Unnamed_numerical_constants
| } | ||
|
|
||
| enum Errors { | ||
| ERRORS_UNKNOWN = 0; |
There was a problem hiding this comment.
following the protobuf style guide, this will have to be done else where as well (ABCI).
rpc/client/evidence_test.go
Outdated
| var err error | ||
| vote.Signature, err = val.Key.PrivKey.Sign(vote.SignBytes(chainID)) | ||
|
|
||
| fmt.Println(vote, vote2) |
There was a problem hiding this comment.
| fmt.Println(vote, vote2) | |
| t.Log(vote, vote2) |
types/evidence.go
Outdated
| _ = voteVal.SignVote("mock-chain-id", &vote) | ||
|
|
||
| v := vote.ToProto() | ||
| _ = voteVal.SignVote("mock-chain-id", v) |
Codecov Report
@@ Coverage Diff @@
## proto-breakage #4813 +/- ##
==================================================
- Coverage 62.16% 62.09% -0.07%
==================================================
Files 165 165
Lines 15238 15242 +4
==================================================
- Hits 9473 9465 -8
- Misses 4931 4940 +9
- Partials 834 837 +3
|
Description
define canonical types in protobuf
Closes: #XXX