docs: generate RPC documentation #542

polydez · 2024-11-05T12:03:27Z

Resolves #512

Added generation of API reference documentation (resulting file is docs/api.md). Regrouped documentation in order to utilize node docs in docs/index.md.

The most challenging here is to automatically generate corresponding messages right after each endpoint and this seems to be impossible to do using protoc-gen-doc or any other tools I had chance to try. So if it is crucial, we will probably need to regroup them manually. But for the first attempt I tried to do fully automated API reference docs generation.

# Conflicts: # CHANGELOG.md

bobbinth

Thank you! Not a full review, but I left some comments inline. These comments are not exhaustive - similar issues apply in other places which I haven't commented on. Basically, in may cases the added comments are not very useful. We should go a layer deeper and explain things more so that even an unfamiliar person can understand them.

Regarding the new doc format. I'm not sure I like it. It combines everything together, and IMO, makes docs more difficult to navigate. Is there a way to generate docs separately for RPC, store, and block producers components?

bobbinth · 2024-11-17T09:28:09Z

proto/transaction.proto

+    // The block number.
    fixed32 block_num = 2;


This comment is not very helpful (i.e., we can see from the name that this is a block number). Sometimes, such things cannot be avoided, but in this case, we should actually provide more detail on what this block number means.

bobbinth · 2024-11-17T09:28:21Z

proto/transaction.proto

+    // The account ID.
    account.AccountId account_id = 3;


Same comment as above.

bobbinth · 2024-11-17T09:32:11Z

proto/account.proto

+    // Merkle path to verify the account's inclusion in the MMR.
    uint32 block_num = 3;


This comment doesn't seem correct.

bobbinth · 2024-11-17T09:33:43Z

proto/account.proto

+// A summary of an account.
 message AccountSummary {


"A summary of an account" comment is not very helpful here. I would expand it to a couple of sentences which describe what it is and what it is used for.

bobbinth · 2024-11-17T09:35:11Z

proto/account.proto

+    // Account details encoded using Miden native format.
    optional bytes details = 2;


I would describe what the details actually contain (i think they contain full account state description, right?). Also, we should say why the are optional.

bobbinth · 2024-11-17T09:36:01Z

proto/mmr.proto

+// Represents an MMR delta.
 message MmrDelta {


Let's add some detail on what is the MMR delta.

bobbinth · 2024-11-17T09:36:50Z

proto/merkle.proto

+// Represents a Merkle path.
 message MerklePath {


nit: let's say "Represents a Merkle authentication path."

polydez · 2024-11-17T14:46:46Z

Regarding the new doc format. I'm not sure I like it. It combines everything together, and IMO, makes docs more difficult to navigate. Is there a way to generate docs separately for RPC, store, and block producers components?

I tried, but didn't find any way to do it by just using tools for automatic docs generation. We still have options, for example, we can write script for post-processing of generated docs in order to regroup their contents. Or regroup them manually.

Mirko-von-Leipzig · 2024-12-03T12:44:35Z

Regarding the new doc format. I'm not sure I like it. It combines everything together, and IMO, makes docs more difficult to navigate. Is there a way to generate docs separately for RPC, store, and block producers components?

I tried, but didn't find any way to do it by just using tools for automatic docs generation. We still have options, for example, we can write script for post-processing of generated docs in order to regroup their contents. Or regroup them manually.

I imagine we would have to more concretely separate the various RPC service files. Which is something we want to do at some point in any case I believe?

Mirko-von-Leipzig

LGTM; some questions, suggestions but nothing major.

Probably we should have an actual documentation person go over the wording of things, but that can be a separate pass once we're happy.

Mirko-von-Leipzig · 2024-12-03T12:36:54Z

proto/block_producer.proto

@@ -6,6 +6,7 @@ import "requests.proto";
 import "responses.proto";

 service Api {
+    // Submits proven transaction to the Miden network


Suggested change

// Submits proven transaction to the Miden network

// Submits proven transaction to the Miden network.

Mirko-von-Leipzig · 2024-12-03T12:40:02Z

crates/rpc/README.md

-**Returns**
-
-This method doesn't return any data.
+Full API documentation located [here](../../docs/api.md).


This link will break once published. Issue of course is that we're sharing documentation between crates within a workspace.

This isn't an immediate issue imo; just something to consider as we flesh out our solution to this.

Mirko-von-Leipzig · 2024-12-03T12:40:20Z

crates/store/README.md

-**Returns**
-
- `notes`: `[Note]` – list of all notes of the current chain.
+Full API documentation located [here](../../docs/api.md).


Similar link breakage on publish.

Mirko-von-Leipzig · 2024-12-03T12:46:02Z

docs/api.tmpl

This gets generated but doesn't actually do anything afterwards iiuc? Should this be checked in?

Or maybe this could be used to specify/separate the various rpc endpoints into separate docs somehow?

Mirko-von-Leipzig · 2024-12-03T13:00:00Z

Makefile

+	docker run --rm \
+      -v $(PWD)/docs:/out \
+      -v $(PWD)/proto:/protos \
+      pseudomuto/protoc-gen-doc --doc_opt=/out/api.tmpl,api.md


We probably need a separate CI check to ensure the generated docs match this?

I'm also unsure if this should be part of make doc like this; though maybe its fine.

I would:

add a README.md with instructions to the docs folder with these instructions

optionally remove these instructions from here (only manual invocation)

add ci check to ensure we stay in sync

bobbinth

Thank you! I left some comments inline - mostly not related to this PR.

For this PR, I think the updates in the .proto files look fine, but I think auto-generated documentation is not great. I think it actually makes things a bit worse as compared to now.

So, maybe we split the PR so that we can commit .proto changes and maybe update the existing documentation to match the current state.

Then, in a separate PR, we can try to refactor how the .proto files are organized as @Mirko-von-Leipzig suggested or maybe do something else that will improve the overall readability of the documentation.

bobbinth · 2024-12-04T00:18:40Z

proto/rpc.proto

+    // Gets a list of proofs for given nullifier hashes, each proof as a sparse Merkle Tree.
    rpc CheckNullifiers(requests.CheckNullifiersRequest) returns (responses.CheckNullifiersResponse) {}


@igamigo - are we using this endpoint anywhere in the client?

bobbinth · 2024-12-04T00:23:44Z

proto/rpc.proto

+    // Returns the latest state of an account with the specified ID.
    rpc GetAccountDetails(requests.GetAccountDetailsRequest) returns (responses.GetAccountDetailsResponse) {}
+
+    // Returns the latest state proofs of accounts with the specified IDs.
    rpc GetAccountProofs(requests.GetAccountProofsRequest) returns (responses.GetAccountProofsResponse) {}
+
+    // Returns delta of the account states in the range from `from_block_num` (exclusive) to
+    // `to_block_num` (inclusive).
    rpc GetAccountStateDelta(requests.GetAccountStateDeltaRequest) returns (responses.GetAccountStateDeltaResponse) {}


Not for this PR, but it feels like we should normalize these endpoints somehow. For example:

I'm not sure if we are actually using GetAccountStateDelta in the client - @igamigo, could you check?

I think the current GetAccountDetails endpoint returns the entire account, which would probably not work for large accounts.

GetAccountProofsRequest is used primarily for FPI, but maybe it can be used for other things as well?

Generally, we should probably list the use case we have for retrieving account data from the node and see if these 3 methods are still the right way to get this data. @polydez - could you create an issue for this?

Responding here for completeness as well

polydez added 2 commits November 5, 2024 16:58

docs: improve protobuf services/messages comments

8ef6713

docs: auto-generated api.md

b943640

polydez changed the base branch from main to next November 5, 2024 12:03

polydez added 11 commits November 7, 2024 16:07

docs: update comments

5722bf1

refactor: fix warning in requests.proto

8878994

feat: generate markdown docs by custom template

ffe14e0

docs: try to fix links

206958b

docs: fix links to files

7d9f685

docs: fix links to files

a4693ce

docs: fix links to fields

895b299

docs: update flags formatting

8796ef5

docs: rename methods to endpoints

aa67874

docs: improve documentation

912edf9

docs: update CHANGELOG.md

0a4b05d

polydez requested review from bobbinth and Dominik1999 November 7, 2024 16:33

Merge branch 'next' into polydez-rpc-docs

b2c5a69

# Conflicts: # CHANGELOG.md

polydez marked this pull request as ready for review November 7, 2024 16:34

docs: move update in CHANGELOG.md to v0.7.0

8b86b6e

bobbinth reviewed Nov 17, 2024

View reviewed changes

bobbinth requested a review from Mirko-von-Leipzig December 2, 2024 17:23

Mirko-von-Leipzig changed the title ~~RPC documentation~~ docs: generate RPC documentation Dec 3, 2024

Mirko-von-Leipzig approved these changes Dec 3, 2024

View reviewed changes

bobbinth reviewed Dec 4, 2024

View reviewed changes

Mirko-von-Leipzig mentioned this pull request Jan 13, 2025

Normalize RPC endpoints #616

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: generate RPC documentation #542

docs: generate RPC documentation #542

polydez commented Nov 5, 2024 •

edited

Loading

bobbinth left a comment

bobbinth Nov 17, 2024

bobbinth Nov 17, 2024

bobbinth Nov 17, 2024

bobbinth Nov 17, 2024

bobbinth Nov 17, 2024

bobbinth Nov 17, 2024

bobbinth Nov 17, 2024

polydez commented Nov 17, 2024

Mirko-von-Leipzig commented Dec 3, 2024

Mirko-von-Leipzig left a comment

Mirko-von-Leipzig Dec 3, 2024

Mirko-von-Leipzig Dec 3, 2024

Mirko-von-Leipzig Dec 3, 2024

Mirko-von-Leipzig Dec 3, 2024

Mirko-von-Leipzig Dec 3, 2024

bobbinth left a comment

bobbinth Dec 4, 2024

bobbinth Dec 4, 2024

igamigo Jan 13, 2025

		// Merkle path to verify the account's inclusion in the MMR.
		uint32 block_num = 3;

		// Account details encoded using Miden native format.
		optional bytes details = 2;

	// Submits proven transaction to the Miden network
	// Submits proven transaction to the Miden network.

		// Gets a list of proofs for given nullifier hashes, each proof as a sparse Merkle Tree.
		rpc CheckNullifiers(requests.CheckNullifiersRequest) returns (responses.CheckNullifiersResponse) {}

docs: generate RPC documentation #542

Are you sure you want to change the base?

docs: generate RPC documentation #542

Conversation

polydez commented Nov 5, 2024 • edited Loading

bobbinth left a 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

polydez commented Nov 17, 2024

Mirko-von-Leipzig commented Dec 3, 2024

Mirko-von-Leipzig left a 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

bobbinth left a 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

polydez commented Nov 5, 2024 •

edited

Loading