Improved RPC documentation #620
Conversation
There was a problem hiding this comment.
I've left a whole bunch of nits/suggestions. The vast majority are just stylistic/optional - feel free to disregard.
I do think we should get someone with a more technical/user documentation background to standardize these and to ensure they make sense from a more outside perspective.
| message BlockHeader { | ||
| // specifies the version of the protocol. | ||
| // Specifies the version of the protocol. |
There was a problem hiding this comment.
Is this the version of the miden-protocol aka the version of the chain at this block?
There was a problem hiding this comment.
Yes, indeed. We set version as 1 on genesis creation and all blocks for now use just the version from previous block.
| @@ -6,6 +6,7 @@ import "requests.proto"; | |||
| import "responses.proto"; | |||
|
|
|||
| service Api { | |||
There was a problem hiding this comment.
Also side note: should Api be made more specific, e.g. BlockProducer service?
I'm unsure how this code gets generated for other languages, but in rust its usually a bit annoying because you end with impl Api for T which is not very informative.
There was a problem hiding this comment.
In Rust it isn't an issue. We can add more context by just:
use miden_node_proto::generated::rpc::api_server::Api as RpcApi;
...
impl RpcApi for RpcApiImpl {
...
}But I agree, for some languages it might be impossible to introduce such aliases.
| repeated digest.Digest nullifiers = 2; | ||
| // Array of unauthenticated note IDs to be checked for existence in the database. |
There was a problem hiding this comment.
Clumsy attempt below.
We should strongly document unauthenticated notes somewhere; even if its only for internal documentation.
| // Array of unauthenticated note IDs to be checked for existence in the database. | |
| // Set of unauthenticated notes to check for existence onchain. | |
| // | |
| // These are notes which were not onchain at the state the transaction was proven, | |
| // but could by now be present. |
There was a problem hiding this comment.
Looks good to me, thank you!
| message SubmitProvenTransactionRequest { | ||
| // Transaction encoded using miden's native format | ||
| // Transaction encoded using Miden's native format. |
There was a problem hiding this comment.
Link to the format.
| message GetNotesByIdRequest { | ||
| // List of NoteId's to be queried from the database | ||
| // List of NoteId's to be queried from the database. |
There was a problem hiding this comment.
| // List of NoteId's to be queried from the database. | |
| // List of notes to be queried from the database. |
| message GetNoteAuthenticationInfoRequest { | ||
| // List of NoteId's to be queried from the database | ||
| // List of NoteId's to be queried from the database. |
There was a problem hiding this comment.
| // List of NoteId's to be queried from the database. | |
| // List of notes to be queried from the database. |
| @@ -126,9 +136,9 @@ message GetAccountStateDeltaRequest { | |||
| fixed32 to_block_num = 3; | |||
| } | |||
|
|
|||
| // Request message to get account proofs. | |||
| // Returns the latest state proofs of accounts with the specified IDs. | |||
There was a problem hiding this comment.
| // Returns the latest state proofs of accounts with the specified IDs. | |
| // Returns the latest state proofs of the specified accounts. |
Co-authored-by: Mirko <[email protected]>
Co-authored-by: Mirko <[email protected]>
Co-authored-by: Mirko <[email protected]>
Co-authored-by: Mirko <[email protected]>
There was a problem hiding this comment.
Just a couple more suggestions; though I think we should just get these improvements in at this stage.
We could iterate on these for a long time still; and I feel we would be better served treating the RPC docs as semi-complete for now and instead focus on getting proper user facing documentation going. The proto definitions could then easily link to the actual documentation, with only additional details for things like serialization format.
We decided that we need only part with improved doc comments from the PR: #542
This PR contains only needed part of the one behind.