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

HIP-1037 - Protocol Buffer Specification #1037

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jsync-swirlds
Copy link
Member

@jsync-swirlds jsync-swirlds commented Aug 31, 2024

This pull request introduces HIP-1037, titled "Protocol Buffer API Specification" The proposal aims to enhance the Hedera API declarations by adding detailed API specification text to every protocol buffer element. This API specification text is added in a manner very similar to the Java API specification by adding properly formatted comments above each API message and field. The standard protoc plugin protoc-gen-doc processes these comments, in combination with an included template, to produce markdown files that fully detail the API.

Copy link

netlify bot commented Aug 31, 2024

Deploy Preview for hedera-hips ready!

Name Link
🔨 Latest commit eea1903
🔍 Latest deploy log https://app.netlify.com/sites/hedera-hips/deploys/672c220390b2840008b66a0b
😎 Deploy Preview https://deploy-preview-1037--hedera-hips.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jsync-swirlds jsync-swirlds self-assigned this Aug 31, 2024
@jsync-swirlds jsync-swirlds changed the title Create HIP for Protocol Buffer Specification HIP-1037 - Protocol Buffer Specification Aug 31, 2024
@jsync-swirlds jsync-swirlds force-pushed the hip-protobuf-specification branch 4 times, most recently from cfbe260 to 87d5e01 Compare September 18, 2024 23:45
Copy link
Member

@david-bakin-sl david-bakin-sl left a comment

Choose a reason for hiding this comment

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

You'll see below a suggestion that the thrust of this HIP be changed; hope you consider it!

HIP/hip-1037.md Outdated Show resolved Hide resolved
HIP/hip-1037.md Outdated Show resolved Hide resolved
HIP/hip-1037.md Outdated Show resolved Hide resolved
HIP/hip-1037.md Show resolved Hide resolved
HIP/hip-1037.md Show resolved Hide resolved
HIP/hip-1037.md Show resolved Hide resolved
Hedera APIs so that I can confidently design my application to call those
APIs.
- As an independent implementor I want to have clear and accurate specification
for all Hedera APIs so that I can design an independent, correct, and
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to rename them to "Hiero APIs" or something similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. Definitely something to consider.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but I don't believe in this PR or in this repo.
Changes will probably be done at once by @mgarbs in the heiro org I imagine

Copy link
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

nice work, some considerations

## Content Guidelines
### General `proto` File Guidelines
- Protocol buffers are compiled for the `hedera-services` codebase using a
custom processor 'PBJ'. While no other entity is required to use `PBJ`, all
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is this essentially enforcing the community to have a comfortable understanding of PBJ outputs. This may be true but do we want to require this?
Should we call out the practical implications here so users know if they are compatible. How can they have confidence, does it require ensuring certain reserved types aren't used or does it require passing some tests on PBJ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The simplest way is to pass the PBJ integration test, or pass compilation in the hedera-services codebase. Probably we need to, when resources permit, split the PBJ integration test out into a useable compatibility test library for general use.

The hedera-services repo is already the primary repository for any proto under the services subfolder (hedera-protobufs is a manually maintained copy), and the current direction is to continue to split every group of proto files out to the various code repositories (so Block Node would have block stream; mirror node would have MN their files, etc...).

Ideally, PBJ should compile anything that protoc would. There are a decent array of open bugs and limitations that prevent this, however. PBJ is also behind protoc standard by a bit. PBJ supports proto3 while protoc (with the new "editions" versioning) is at the 2023 Edition.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added some explanation to describe the recommended current process.

 * Initial proposed HIP text

Signed-off-by: Joseph Sinclair <[email protected]>

Update hip-1037.md to Review

Signed-off-by: Michael Garber <[email protected]>
- Added style guidelines as an asset.
   - Linked these guidelines in the specification section
- Moved the large PR link to the reference implementation section
- Added reference to protoc-gen-doc
- Added two rejected ideas
- Added paragraph breaks in two places
- Updated style guidelines to mention PBJ and Map field concerns.
- Adjusted style guidelines to add section headings and
  clarify the examples.

Signed-off-by: Joseph Sinclair <[email protected]>
 - Added some additional sample content
 - Clarified "equivalent" types with respect to these guidelines
 - Clarified line break usage
 - Clarified when a horizontal rule (`<hr>`) tag is appropriate
 - Added additional clarification for paragraph breaks
 - Added guideline for non-API comments using "line oriented" (`//`)
   style comments.
 - Added a user story related to smart contract requirements.

Signed-off-by: Joseph Sinclair <[email protected]>
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.

4 participants