docs: Specification update to clean up and improve documentation for proto message definitions #388

jsync-swirlds · 2024-07-03T22:27:14Z

Massive update to all services proto files to update and correct in-place
specification and documentation.

Note, the rendered markdown is removed from this PR to ease review.
The full rendered markdown documents may be found here.

Added svg diagrams in a branch for most state objects to assist in review.
Cleaned up copyright headers to remove non-printing characters.
Began reworking documents to produce clear documentation via the most
common document processor for protobuf
- There are limitations that encourage a slightly different style.
- Markdown produces the best output (with slight modifications to the template),
  so that is selected.
Added a modified markdown template (to work around the "sanitization" bug in
the docs processor for markdown).
- Now we can add most HTML tags in the markdown, allowing things like line breaks
 in attribute descriptions.
- This allows using a line with only   or  in place of blank
 lines (that break the markdown tables).
Thorough update of many proto files in state.
Committed a fully generated set of docs in a separate branch for review
purposes.
Moved document outputs to documents/api/services to allow for other document sets
Removed unused imports to quiet warnings.
Replicate "service" documentation to the message body entries for clarity
- Message bodies were just "see ..." references, which do not link well,
  and push content specification to the service API, which muddies both.
Rewrite message body documentation to content specification
Rewrite "service" API documentation to API specification.
Adjust common fields to have consistent descriptions.
Updated copyright header to match services standard.
- Corrected incorrect copy-paste of older years in some files.
- Removed hidden zero-width characters.
- Removed unnecessary additions.
Adjusted query header and query response header text to be more uniform.
Thorough update across services proto definitions
- State messages complete
- Consensus service complete
- Smart Contract service complete.
- Crypto Service complete.
- Miscellaneous protos complete.
- File Service complete.
- Freeze Service complete.
- Transaction and records complete.
- Synthetic node_stake_update transaction complete.
- Query and Response complete.
- General Queries complete.
  - Harmonized crypto_get_info and get_account_details
  - Discovered no service interface defines getByKey, we may be able to
    completely remove that query, with it's handler in network admin service,
    and just reserve the field number in query and hedera functionality henceforth.
  - Found a few more unsupported queries and marked them deprecated in the
    query.proto one-of block.
- Network Service complete
- Schedule Service complete
- Token Service complete
  - Fixed the new token_reject file, which was not created according to
    the content in the HIP.
- Utility Service complete.
- Address Book Service complete.
  - This was a separate PR.
- Response Code
  - Separate due to size (over 360 fields) and frequent changes.
    Will be addressed in a separate PR and HIP to minimize conflict.

jsync-swirlds

.

jsync-swirlds · 2024-11-01T00:06:37Z

I believe I've addressed or responded to each comment, thank you all for your diligence.
Please check my changes and/or responses, and resolve or comment further as needed.

services/file_create.proto

services/file_append.proto

thomas-swirlds-labs · 2024-11-04T15:57:50Z

services/state/schedule/schedule.proto

@@ -33,116 +39,167 @@ option java_multiple_files = true;

 /**
 * Representation of a Hedera Schedule entry in the network Merkle tree.
- *
- * As with all network entities, a schedule has a unique entity number, which is usually given along
- * with the network's shard and realm in the form of a shard.realm.number id.
 */


This doesn't seem to conform to the Style Guidelines - "Include a line break ( ) after the first line, then a brief description, if appropriate. Not every field needs additional description, but every message does."

Good point, I will improve this message with some clear description.

thomas-swirlds-labs · 2024-11-04T16:10:27Z

documents/Specification-Format-Style-Guidelines.md

+     comment boxes are limited to roughly 80 characters width. The primary
+     "code" interaction with proto files and markdown files is in review.
+     Also, some markdown processors struggle with longer lines.
+- Use blank lines (with `*` prefix) for paragraph breaks in message or


I think might be a little confusing. When should a message have a blank line and when should it have a  ?

All specification content should be as close to clean markdown as the current processor permits. The use of  in fields is necessitated by limitations of markdown tables; messages should always prefer a blank line over a  tag.

I'll add some clarifying text here.

thomas-swirlds-labs · 2024-11-04T16:17:05Z

services/consensus_service.proto

-     * On success, the resulting TransactionReceipt contains the newly created TopicId.
-     * Request is [ConsensusCreateTopicTransactionBody](#proto.ConsensusCreateTopicTransactionBody)
+     * Create an HCS topic.
+     * <p>


Maybe we could add a section on Service definitions to the Style Guidelines. Seems messages cannot contain  tags but Services can is that true?

It's tricky because gRPC service syntax is somewhat "bolted on" and reuses other proto syntax with different keywords.
Basically, a service is equivalent to a message; a rpc is equivalent to a field.

I'll add something to that effect to the guidelines.

thomas-swirlds-labs · 2024-11-04T16:18:11Z

services/consensus_submit_message.proto

     */
    int32 number = 3;
 }

 /**
- * UNDOCUMENTED
+ * Submit a message for consensus.
+ * <p>


Should messages contain  tags or only blank lines 🤔

Good catch, messages should just be blank lines, in order to remain clean markdown syntax.
Occasional exceptions may be required temporarily until we upgrade to a version of Java (23 or later) with the markdown syntax for Javadoc (JEP 467) available, and update PBJ to translate the documentation accordingly.

thomas-swirlds-labs · 2024-11-04T16:19:47Z

services/consensus_topic_info.proto

+     * The bytes of each uint64 or uint32 encoded for the hash input
+     * MUST be in Big-Endian format.
+     * <p>
+     * <hr/>


Just wondering in general on the guidance of when is it advisable or appropriate to add <hr/> tags ?

In large blocks of documentation, it is appropriate to add hr tags to set apart large groupings of closely related items, or provide visual separation for multiple broad topics.
In general, such usage should be rare, however, as we would prefer not to need such large blocks of documentation in most situations.

thomas-swirlds-labs · 2024-11-04T16:20:37Z

services/consensus_topic_info.proto

+     * <p>
+     * If the `autoRenewAccount` value for this topic is set to a valid account
+     * with sufficient HBAR balance to pay renewal fees when this topic
+     * expires, the system SHALL automatically renew this topic, extending the


Is this a description or a requirement?

Requirement, just a complex one to express.

thomas-swirlds-labs · 2024-11-04T16:27:33Z

services/contract_update.proto

     */
    Timestamp expirationTime = 2;

    /**
-     * The new key to control updates to the contract
+     * If set, modify the key that authorizes updates to the contract.
+     * <p>


Here we have a  tag but in the previous field we have a   ?

This separates summary from requirements. The prior field requires more description than the summary statement, so it includes a descriptive sentence or two; this field does not require that additional descriptive content.

thomas-swirlds-labs · 2024-11-04T16:28:53Z

services/contract_update.proto

+        string memo = 9 [deprecated = true];
+
+        /**
+         * If set, modify the short memo for this smart contract.<br/>


should the   be on the next line ?

The syntax guidelines place the   tag at the end of the line. This generally reads better than a tag before text on a line.
e.g.

text ending in a break. is generally easier to read than text starting with a break

Added this to guidelines.

thomas-swirlds-labs · 2024-11-04T16:31:42Z

services/crypto_add_live_hash.proto

+ * or more signatures.
+ * - The account key of the account associated to the live hash.
+ * - Any one key from the key list in the live hash entry.
+ * - Any combination of keys from the key list in the live hash entry.
 */


This transactionBody might be missing heading ### Block Stream Effects.

This entire transaction is deprecated (see block on line 5), and has no block stream effects.

thomas-swirlds-labs · 2024-11-04T16:37:38Z

services/consensus_submit_message.proto

     */
    int32 number = 3;
 }

 /**
- * UNDOCUMENTED
+ * Submit a message for consensus.
+ * <p>


Is there some heuristic around when messages should have  tags or should we always favour blank lines for breaks in messages?

Always prefer a blank line.
This line is an error that I will fix.

thomas-swirlds-labs · 2024-11-04T17:09:56Z

documents/Specification-Format-Style-Guidelines.md

+     sentences than "normal" prose. Some describe the flow as "staccato".
+- Add another blank line after requirements for `message` documents only.
+- Each message that represents a transaction body **must** document the
+  Block Stream effects with a heading `### Block Stream Effects`.


Could also add here that any heading such as ### Block Stream Effects should not have a   since titles are block elements (like paragraphs)

thomas-swirlds-labs · 2024-11-04T17:11:29Z

documents/Specification-Format-Style-Guidelines.md

+     description or requirements, not here.
+      - A good rule of thumb is that if this first sentence exceeds 60
+        characters, then it is trying to describe too much.
+- Include a line break (`<br/>`) after the first line, then a brief description,


Additional Note: A   should never be alone on a line; it should always appear at end of the line instead.

I'll update these in the HIP that defines this file, and replicate here.

thomas-swirlds-labs · 2024-11-04T17:16:17Z

documents/Specification-Format-Style-Guidelines.md

+     Blank lines break the automated documentation generator in **fields**
+     because fields are output as markdown tables, and markdown tables require
+     each row to be a single long line.
+- Be _prescriptive_ in requirements.  Do not _describe_ the behavior.


In proto specification 2 we had the required tag but that has been removed in v3. Should every field now try to have the equivalent of either an optional or required tag in the comments e.g. "This field SHALL not be empty" or what are the guidelines around that?

optional still exists in proto3 and later versions (2023 edition is latest, I believe); it requests "explicit presence".
Required fields are the only ones we probably should describe, since optional is the default.
Note, PBJ currently does not support explicit presence, so that feature gap will need to be closed before we can enforce such a guideline.

How would this read, as a guideline?

The default state for any field is "optional", and all required fields should be specified as such. If the presence or absence of a field must be clear separate from the default value of that field, the field should use proto3 "explicit presence" and be marked with the optional keyword.

thomas-swirlds-labs · 2024-11-04T18:08:10Z

services/schedulable_transaction_body.proto

+     */
+    string memo = 2;
+
+    // The choices here are arranged by service in roughly lexicographic


If we can have code comments -- for example here to clarify the oneof data structure then we could maybe add a section in the style guidelines to illustrate and clarify?

Good idea, I'll work on that.

thomas-swirlds-labs · 2024-11-04T18:15:35Z

services/schedulable_transaction_body.proto

+        ConsensusDeleteTopicTransactionBody consensusDeleteTopic = 20;
+
+        /**
+         * Submit a message to a topic.<br/>


In the Style Guidelines it states "Include a line break ( ) after the first line, then a brief description, if appropriate." Is this applicable to fields as well?

Yes, both messages and fields may have description, if the summary line is not sufficient.

Added some clarification to the guidelines.

…message definitions * Added svg diagrams for most state objects. * Clean up copyright headers to remove non-printing characters. * Began reworking documents to produce clear documentation via the most common document processor for protobuf * There are limitations that encourage a slightly different style. * Markdown produces the best output (with slight modifications to the template), so that is selected. * Added a modified markdown template (to work around the "sanitization" bug in the docs processor for markdown). * Now we can add most HTML tags in the markdown, allowing things like line breaks in attribute descriptions. * The results are much better, might be something worth submitting as a PR upstream (along with some non-trivial examples)... * This allows using a line with only ` ` or `` in place of blank lines (that break the markdown tables). * Ideally the go code would do this, but that's a bigger lift, this works for now. * Thorough update of many proto files in state. * Committed a fully generated set of docs in this branch for review purposes, will remove before completing the task. * Moved document outputs to `documents/api/services` to allow for other document sets * Removed unused imports to quiet warnings. * Replicate "service" documentation to the message body entries for clarity * Message bodies were just "see ..." references, which do not link well, and push content specification to the service API, which muddies both. * Rewrite message body documentation to *content* specification * Rewrite "service" API documentation to *API* specification. * Adjust common fields to have consistent descriptions. Mass update of the copyright header. * Updated copyright header to match services standard. * Corrected incorrect copy-paste of older years in state. * Removed hidden zero-width characters. * Removed unnecessary additions. * Adjusted query header and query response header text to be more uniform. * Thorough update across services proto definitions * State messages complete * Consensus service complete * Smart Contract service complete. * Crypto Service complete. * Miscellaneous protos complete. * File Service complete. * Freeze Service complete. * Transaction and records complete. * Synthetic node_stake_update transaction complete. * Query and Response complete. * General Queries complete. * Harmonized crypto_get_info and get_account_details * Discovered no service interface defines `getByKey`, we may be able to completely remove that query, with it's handler in network admin service, and just reserve the field number in query and hedera functionality henceforth. * Found a few more unsupported queries and marked them deprecated in the query.proto one-of block. * Network Service complete * Schedule Service complete * Token Service complete * Fixed the new `token_reject` file, which was not created according to the content in the HIP. * Utility Service complete. * Address Book Service complete. * Minor updates and clarification only. * HIP 904 additions * Updated all airdrop and pending airdrop entries. * Response Code * Separate due to size (iover 360 fields) and frequent changes. Will be addressed in a separate PR to minimize conflict with ongoing services work. Signed-off-by: Joseph Sinclair <[email protected]>

* Documentation is moved to the hip-1037-documentation branch Signed-off-by: Joseph Sinclair <[email protected]>

* Fixed incorrect reference in block header. * Improvements to style guidelines file. * Small review fixes. Signed-off-by: Joseph Sinclair <[email protected]>

* Many suggestions slightly rewritten to better match style guidelines. * Harmonized use of the `reserved` keyword. * Added several clarifications to style guidelines. Signed-off-by: Joseph Sinclair <[email protected]>

derektriley

Partial review of UtilService LGTM.

jsync-swirlds self-assigned this Jul 3, 2024

jsync-swirlds changed the title ~~docs: Documentation Update to clean up and improve documentation for proto message definitions~~ docs: Specification update to clean up and improve documentation for proto message definitions Jul 11, 2024

jsync-swirlds requested review from jasperpotts and rbair23 July 11, 2024 16:20

jsync-swirlds force-pushed the pbj-storage-spec-review branch 6 times, most recently from fb4bc88 to df235a5 Compare July 19, 2024 00:51

jsync-swirlds force-pushed the pbj-storage-spec-review branch 4 times, most recently from 658ad21 to e4dfc42 Compare July 24, 2024 23:38

jsync-swirlds mentioned this pull request Jul 30, 2024

docs: Update glossary defns of aliases, triplets hashgraph/hedera-services#14372

Merged

1 task

jsync-swirlds force-pushed the pbj-storage-spec-review branch 3 times, most recently from 85a8dc1 to 99947b7 Compare August 1, 2024 18:02

jsync-swirlds force-pushed the pbj-storage-spec-review branch 2 times, most recently from 030315e to 3c26875 Compare August 9, 2024 19:33

jsync-swirlds commented Aug 9, 2024

View reviewed changes

jsync-swirlds force-pushed the pbj-storage-spec-review branch 7 times, most recently from 22a1201 to 5ffdb1c Compare August 14, 2024 23:58

jsync-swirlds force-pushed the pbj-storage-spec-review branch 2 times, most recently from 8abac61 to 243d22f Compare August 22, 2024 21:21

povolev15 reviewed Nov 1, 2024

View reviewed changes

services/file_create.proto Outdated Show resolved Hide resolved

povolev15 reviewed Nov 1, 2024

View reviewed changes

services/file_create.proto Show resolved Hide resolved

povolev15 reviewed Nov 1, 2024

View reviewed changes

services/file_create.proto Show resolved Hide resolved

povolev15 reviewed Nov 1, 2024

View reviewed changes

services/file_append.proto Show resolved Hide resolved

jsync-swirlds force-pushed the pbj-storage-spec-review branch 2 times, most recently from 13d4594 to 238d649 Compare November 1, 2024 20:15

thomas-swirlds-labs reviewed Nov 4, 2024

View reviewed changes

jsync-swirlds force-pushed the pbj-storage-spec-review branch from 238d649 to 99339af Compare November 4, 2024 23:55

jsync-swirlds requested a review from Neeharika-Sompalli November 5, 2024 17:28

jsync-swirlds added 4 commits November 7, 2024 15:08

Removed documentation to simplify code review.

aa255fa

* Documentation is moved to the hip-1037-documentation branch Signed-off-by: Joseph Sinclair <[email protected]>

Revision based on code review

b7a9c28

* Fixed incorrect reference in block header. * Improvements to style guidelines file. * Small review fixes. Signed-off-by: Joseph Sinclair <[email protected]>

Updates to address review comments

329b970

* Many suggestions slightly rewritten to better match style guidelines. * Harmonized use of the `reserved` keyword. * Added several clarifications to style guidelines. Signed-off-by: Joseph Sinclair <[email protected]>

jsync-swirlds force-pushed the pbj-storage-spec-review branch from 99339af to 329b970 Compare November 7, 2024 22:10

derektriley approved these changes Nov 8, 2024

View reviewed changes

docs: Specification update to clean up and improve documentation for proto message definitions #388

Are you sure you want to change the base?

docs: Specification update to clean up and improve documentation for proto message definitions #388

Conversation

jsync-swirlds commented Jul 3, 2024 • edited Loading

jsync-swirlds left a comment • edited Loading

Choose a reason for hiding this comment

jsync-swirlds commented Nov 1, 2024

thomas-swirlds-labs Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomas-swirlds-labs Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomas-swirlds-labs Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomas-swirlds-labs Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

jsync-swirlds Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsync-swirlds Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derektriley left a comment

Choose a reason for hiding this comment

jsync-swirlds commented Jul 3, 2024 •

edited

Loading

jsync-swirlds left a comment •

edited

Loading

thomas-swirlds-labs Nov 4, 2024 •

edited

Loading

thomas-swirlds-labs Nov 4, 2024 •

edited

Loading

thomas-swirlds-labs Nov 4, 2024 •

edited

Loading

thomas-swirlds-labs Nov 4, 2024 •

edited

Loading

jsync-swirlds Nov 4, 2024 •

edited

Loading

jsync-swirlds Nov 4, 2024 •

edited

Loading