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

Add autogenerated protos for v1.0 #1381

Closed
wants to merge 10 commits into from
Closed

Conversation

sergio-mena
Copy link

@sergio-mena sergio-mena commented Nov 19, 2023

This (draft) PR is the first to providing support for CometBFT v1.0.

The PR appears to be massive but the reason is the tons of newly auto-generated code it introduces. For this reason, please don't review it in one shot; rather use the following description of commits:

  • Commit 1: Ignore. It is the result of running the protobuf generation on my machine, which produces a few changes in the auto-generated code.
  • Commit 2: Review. First step: add the logic to handle both Tendermint and CometBFT protobufs. We might decide to deprecate the Tendermint part.
  • Commit 3: Ignore. The effect in auto-generated code of commit 2.
  • Commit 4: Review. Adapted protobuf generation tweaks to CombetBFT protos. Commit 2, just copy-pasted the Tendermint ones. This commit allows to see how I adapted them.
  • Commit 5: Ignore. The effect in auto-generated code of commit 4.
  • Commit 6: Review. As there are several levels of modules now, the preexisting mechanism to generate the modules was too rigid. This commit introduces and extended version
  • Commit 7: Ignore. The effect in auto-generated code of commit 6.
  • Commit 8: Review. Fixes needed in the manual code so that the new proto bufs builds correctly
  • Commit 9: Review. Changelog.

Note: even if this PR is likely to be ready for review, I decided to leave it a draft PR because we need to discuss whether we should be forking tendermint-rs into cometbft-rs and move this PR there.

  • 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/

@sergio-mena sergio-mena self-assigned this Nov 19, 2023
Comment on lines 123 to 156
(".cometbft.libs.bits.BitArray", SERIALIZED),//TODO Revisit
(".cometbft.types.BlockIDFlag", PRIMITIVE_ENUM),
(".cometbft.types.Block", SERIALIZED),
(".cometbft.types.Data", SERIALIZED),
(".cometbft.types.EvidenceParams", SERIALIZED),
(".cometbft.types.Evidence.sum", SERIALIZED),
(".cometbft.types.Evidence.sum", TYPE_TAG),
(".cometbft.types.EvidenceList", SERIALIZED),
(".cometbft.types.DuplicateVoteEvidence", SERIALIZED),
(".cometbft.types.Vote", SERIALIZED),
(".cometbft.types.BlockID", SERIALIZED),
(".cometbft.types.PartSetHeader", SERIALIZED),
(".cometbft.types.LightClientAttackEvidence", SERIALIZED),
(
".cometbft.types.LightClientAttackEvidence",
RENAME_ALL_PASCALCASE,
),
(".cometbft.types.LightBlock", SERIALIZED),
(".cometbft.types.SignedHeader", SERIALIZED),
(".cometbft.types.Header", SERIALIZED),
(".cometbft.version.Consensus", SERIALIZED),
(".cometbft.types.Commit", SERIALIZED),
(".cometbft.types.CommitSig", SERIALIZED),
(".cometbft.types.ValidatorSet", SERIALIZED),
(".cometbft.crypto.PublicKey.sum", SERIALIZED),
(".cometbft.crypto.PublicKey.sum", TYPE_TAG),
(".cometbft.abci.ResponseInfo", SERIALIZED),
(".cometbft.types.CanonicalBlockID", SERIALIZED),
(".cometbft.types.CanonicalPartSetHeader", SERIALIZED),
(".cometbft.types.Validator", SERIALIZED),
(".cometbft.types.CanonicalVote", SERIALIZED),
(".cometbft.types.BlockMeta", SERIALIZED),
(".cometbft.types.TxProof", SERIALIZED),
(".cometbft.crypto.Proof", SERIALIZED),
Copy link
Author

Choose a reason for hiding this comment

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

Please ignore these changes: review them in Commit 4

Comment on lines 279 to 392
QUOTED_WITH_DEFAULT,
),
(
".cometbft.abci.ResponseInfo.last_block_height",
QUOTED_WITH_DEFAULT,
),
(".cometbft.abci.ResponseInfo.last_block_app_hash", DEFAULT),
(
".cometbft.abci.ResponseInfo.last_block_app_hash",
BYTES_SKIP_IF_EMPTY,
),
(".cometbft.types.BlockID.hash", HEXSTRING),
(".cometbft.types.BlockID.part_set_header", RENAME_PARTS),
(
".cometbft.types.PartSetHeader.total",
PART_SET_HEADER_TOTAL,
),
(".cometbft.types.PartSetHeader.hash", HEXSTRING),
(".cometbft.types.Header.height", QUOTED),
(".cometbft.types.Header.time", OPTIONAL),
(".cometbft.types.Header.last_commit_hash", HEXSTRING),
(".cometbft.types.Header.data_hash", HEXSTRING),
(".cometbft.types.Header.validators_hash", HEXSTRING),
(".cometbft.types.Header.next_validators_hash", HEXSTRING),
(".cometbft.types.Header.consensus_hash", HEXSTRING),
(".cometbft.types.Header.app_hash", HEXSTRING),
(".cometbft.types.Header.last_results_hash", HEXSTRING),
(".cometbft.types.Header.evidence_hash", HEXSTRING),
(".cometbft.types.Header.proposer_address", HEXSTRING),
(".cometbft.types.Data.txs", NULLABLEVECARRAY),
(".cometbft.types.EvidenceList.evidence", NULLABLE),
(".cometbft.types.Commit.height", QUOTED),
(".cometbft.types.Commit.signatures", NULLABLE),
(".cometbft.types.CommitSig.validator_address", HEXSTRING),
(".cometbft.types.CommitSig.timestamp", OPTIONAL),
(".cometbft.types.CommitSig.signature", BASE64STRING),
(
".cometbft.types.DuplicateVoteEvidence.total_voting_power",
RENAME_TOTAL_VOTING_POWER_QUOTED,
),
(
".cometbft.types.DuplicateVoteEvidence.validator_power",
RENAME_VALIDATOR_POWER_QUOTED,
),
(
".cometbft.types.DuplicateVoteEvidence.timestamp",
RENAME_TIMESTAMP,
),
(
".cometbft.types.LightClientAttackEvidence.common_height",
QUOTED,
),
(
".cometbft.types.LightClientAttackEvidence.total_voting_power",
QUOTED,
),
(".cometbft.types.Vote.height", QUOTED),
(".cometbft.types.Vote.validator_address", HEXSTRING),
(".cometbft.types.Vote.signature", BASE64STRING),
(".cometbft.types.Vote.timestamp", OPTIONAL),
(".cometbft.types.Validator.address", HEXSTRING),
(
".cometbft.types.Validator.voting_power",
ALIAS_POWER_QUOTED,
), // https://github.com/tendermint/tendermint/issues/5549
(
".cometbft.types.Validator.proposer_priority",
QUOTED_ALLOW_NULL,
), // null occurs in some LightBlock data
(".cometbft.types.Validator.proposer_priority", DEFAULT), // Default is for /genesis deserialization
(
".cometbft.types.ValidatorSet.total_voting_power",
QUOTED_WITH_DEFAULT,
),
(
".cometbft.types.ValidatorSet.total_voting_power",
SKIP_SERIALIZING,
),
(".cometbft.types.BlockMeta.block_size", QUOTED),
(".cometbft.types.BlockMeta.num_txs", QUOTED),
(".cometbft.crypto.PublicKey.sum.ed25519", RENAME_EDPUBKEY),
(
".cometbft.crypto.PublicKey.sum.secp256k1",
RENAME_SECPPUBKEY,
),
(".cometbft.crypto.PublicKey.sum.sr25519", RENAME_SRPUBKEY),
(
".cometbft.types.Evidence.sum.duplicate_vote_evidence",
RENAME_DUPLICATEVOTE,
),
(
".cometbft.types.Evidence.sum.light_client_attack_evidence",
RENAME_LIGHTCLIENTATTACK,
),
(".cometbft.types.TxProof.data", BASE64STRING),
(".cometbft.types.TxProof.root_hash", HEXSTRING),
(".cometbft.crypto.Proof.index", QUOTED),
(".cometbft.crypto.Proof.total", QUOTED),
(".cometbft.crypto.Proof.aunts", VEC_BASE64STRING),
(".cometbft.crypto.Proof.leaf_hash", BASE64STRING),
Copy link
Author

Choose a reason for hiding this comment

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

Please ignore these changes: review them in Commit 4

Copy link
Contributor

@mzabaluev mzabaluev left a comment

Choose a reason for hiding this comment

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

Good automation for the module tree.

I don't think we need to namespace the cometbft generated files under module v1_0.
Remember the idea of ADR-107 is to eventually promote the message definitions to *.v1 packages at the proto level. So abci.v1beta4.Request becomes abci.v1.Request and so on. It's better that we generate the root module under proto/cometbft/lib.rs. Further evolution of the protos will add v2 packages alongside if needed, as designed.

I don't see a need to hold on to proto::tendermint, modules either. We're making a switch for cometbft-rs, the legacy generated files will not be used by the rest of the cometbft-rs crates, so they are pretty much dead code. @thanethomson had the idea that the users who need support for tendermint.* protos could still use the tendermint-rs crates as already released, and we can still support those from this current repository in maintenance mode.

@@ -172,7 +172,7 @@ pub fn copy_files(src_dir: &Path, target_dir: &Path) {
e.file_type().is_file()
&& e.file_name()
.to_str()
.map(|name| name.starts_with("tendermint."))
.map(|name| name.starts_with(&format!("{project}.")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Performance nit: it would be better if the prefix string was formatted once, not with every invocation of the closure.

let file_names = WalkDir::new(prost_dir)
.into_iter()
.filter_map(|e| e.ok())
.filter(|e| {
e.file_type().is_file()
&& e.file_name().to_str().unwrap().starts_with("tendermint.")
&& e.file_name().to_str().unwrap().starts_with(&project_dot)
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, like here.

@sergio-mena
Copy link
Author

sergio-mena commented Dec 7, 2023

This is continued in cometbft/cometbft-rs#2

@sergio-mena sergio-mena deleted the sergio/proto-renaming branch December 15, 2023 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants