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.

Sign up for GitHub

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

Jump to bottom

Update cometbft-proto to protobufs in CometBFT v1.0 (pre-release) #7

Merged
merged 14 commits into from
Jan 24, 2024
Merged

Update cometbft-proto to protobufs in CometBFT v1.0 (pre-release) #7

merged 14 commits into from
Jan 24, 2024

Conversation

mzabaluev
Copy link
Contributor

@mzabaluev mzabaluev commented Jan 12, 2024 •
edited
Loading

Resolves #9

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Added entry in .changelog/

@mzabaluev
proto-compiler: use single CometBFT checkout
228f09e 
Generate from the new structure with versioned proto files.
@mzabaluev
proto: remove tendermint modules
aede9f3 
These are no longer going to be generated with the switch to versioned
cometbft.* protos.
@mzabaluev
proto: re-generate cometbft.*, remove tendermint.*
b5227fd 
Bulk change committing output of the recently changed proto-compiler.
@mzabaluev
proto: update serde support for Evidence, PublicKey
8a8ffd5 
The generated struct types only come from cometbft::* modules
generated from versioned protobufs.
Update the imports and manually written implementations.
@mzabaluev
cometbft: update proto conversions
abd7e87 
Convert imports and the provided conversions to/from the protobuf
structs to the version structure of cometbft_proto.
@mzabaluev
cometbft: restructure versioned Request and Response in abci
9880b2d 
Move the versions under the abci module and make the names match
the proto package names.
@mzabaluev
proto: update imports in tests
859e7e3
@mzabaluev
rpc: rework dialects and CompatMode for v1
c985b8d 
The dialect of 0.37 is now known as 1.0.
@mzabaluev
proto: generate serde for abci::v1::InfoResponse
ae3acd3 
Add a flag for proto-compiler that was not copied from abci.v1beta1.ResponseInfo
@mzabaluev mzabaluev mentioned this pull request Jan 12, 2024
Closed
5 tasks
@mzabaluev
proto: restore serialization on Info response fields
cfd6429 
The custom serialization attributes were not copied over from
v1beta1.ResponseInfo to v1.InfoResponse.
@mzabaluev
proto: serde fallbacks for Vote extension fields
9f1f7b6 
When decoding pre-0.38 messages, the "extension" and "extension_signature" fields are not present. Previously, this worked
because the v0_37 version of the generated struct was used; this,
on the other hand, resulted in the extension fields not being properly decoded in RPC with 0.38+ nodes.
@mzabaluev mzabaluev marked this pull request as ready for review January 15, 2024 12:58
@mzabaluev
Copy link
Contributor Author

mzabaluev commented Jan 15, 2024 •
edited
Loading

Notes for reviewers

As previously, changes under proto/src/prost (mostly contained in b5227fd) are generated code and can be largely skipped through.

The important changes are in proto-compiler which is also made quite simpler; don't spend too much time on code quality there as we'll likely replace the whole thing with buf in short order. Some compiler settings made in #2 had to be further adjusted to get the serde to work as expected.

cometbft has a lot of manual changes; the most tedious repetitive changes are in abd7e87.

cometbft-rpc has API changes to reflect 1.0.

@adizere adizere requested a review from romac January 15, 2024 15:42
romac
romac reviewed Jan 16, 2024
View reviewed changes
Copy link
Collaborator

@romac romac left a comment

Choose a reason for hiding this comment

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

Changes to the proto-compiler look good to me, need more time to review the actual changes to the generated protos.

tools/proto-compiler/src/main.rs Show resolved Hide resolved
@romac
Copy link
Collaborator

romac commented Jan 19, 2024

Don't we want to keep the v0_37 and v0_38 modules in the proto crate, alongside v1 and v0_34?

@mzabaluev
Copy link
Contributor Author

Don't we want to keep the v0_37 and v0_38 modules in the proto crate, alongside v1 and v0_34?

proto no longer has v0_34 either, it's all generated from the versioned proto packages with suffixes like v1, v1beta1, etc.

v0_34 is retained in rpc, where basically we have two incompatible encodings to deal with, one used in 0.34 and the newer one used since 0.37.

In fact, 1.0 adds versioning in RPC request paths, so it may be useful to distinguish three modes: 0.34, 0.37/0.38, and 1.0 which will mean requests prefixed with /v1. This PR is not there yet, though. Perhaps it's better to revert the latest defined mode to v0_37 and add v1 in another PR.

@romac
Copy link
Collaborator

romac commented Jan 23, 2024

In fact, 1.0 adds versioning in RPC request paths, so it may be useful to distinguish three modes: 0.34, 0.37/0.38, and 1.0 which will mean requests prefixed with /v1.

Yeah that sounds like the way to go!

This PR is not there yet, though. Perhaps it's better to revert the latest defined mode to v0_37 and add v1 in another PR.

Sounds good, let's proceed like that then.

@mzabaluev
rpc: revert the latest CompatMode to V0_37
515dbd8 
Will do V1 as version-prefixed requests.
@mzabaluev
Copy link
Contributor Author

I have restored CompatMode::V0_37 (renamed from V1) in 515dbd8
Will do another mode that will prefix request paths with /v1 when the client is configured with CompatMode::V1.

Note that the dialect module still has a v1 because we only have two encoding dialects.

@romac
Copy link
Collaborator

romac commented Jan 23, 2024

Awesome 🚀

@mzabaluev
Updated #7 changelog notes for rpc
c1e73b7 
CompatMode::V1 is not there yet, but dialect::v1 deserves a mention.
@mzabaluev mzabaluev merged commit ee5d2d1 into main Jan 24, 2024
21 checks passed
@mzabaluev
Copy link
Contributor Author

Will do another mode that will prefix request paths with /v1 when the client is configured with CompatMode::V1.

This turned out to be not so simple, and not consistent with the Go API, so I essentially went back to the rename in #11

@mzabaluev mzabaluev mentioned this pull request Jan 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romac romac romac approved these changes

@sergio-mena sergio-mena Awaiting requested review from sergio-mena

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adapt domain types to support v1.0 protos
2 participants
@mzabaluev @romac