chore(proto): refactor messages structure#1045
Conversation
|
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Thank you! Not a full review but I left some comments inline.
Overall, I don't think this is much of an improvement over existing layout (and in some ways more confusing). A few things I've highlighted in the comments:
- I would not use action verbs to for message naming.
- If we are removing
Request/Responsesuffixes, we shouldn't just keep the previous names. For example,ApplyBlockResponseis OK as a message name butApplyBlockis probably not. And I think sometimes it is OK to keepRequestsuffix to make things clear. - I'm not sure we need the
shared.protofile. Most of these messages could either go into other files undertypesor go into individual service files. - I think it should be OK to import messages from other service files. For example, in
rpc.protowe could import messages fromblock_producer.protoandstore.proto.
|
@bobbinth answering your comments:
I'm now working on renaming the Rust generated structs. lmk what do you think of this approach. |
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Thank you! I think this structure works much better. This is not a full review yet - but I left a few comments inline.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left some comments inline - mostly about using namespaces to prefix protobuf types (i.e., instead of ProtoMyType doing something like proto::MyType) - let's try to use this approach consistently throughout.
Also, I think #1045 (comment) still needs to be addressed.
…ith note_index_in_block
| } | ||
|
|
||
| // Represents the result of syncing notes request. | ||
| message SyncNotesResponse { |
There was a problem hiding this comment.
Part of me wants to strive for fully replacing the request/response suffixes. Mainly because that naming tends to be less expressive than the alternative. Understood that sometimes it may be very difficult to think of an appropriate / expressive name in the context.
Taking this as an example. What would we rename SyncNotesResponse to? Perhaps NoteSyncRecordsByTag or ..ByTagAndBlock which gets verbose but you get the idea.
LMK what you think. cc @bobbinth
There was a problem hiding this comment.
We tried to do that where new names were unambiguous - but for a few, I'm not sure there are good alternatives. For example, we could rename SyncNotesRequest into something like BlockNumberAndNoteTagList - but I think this may actually be more confusing then SyncNotesRequest.
Similar with the SyncNotesResponse - though, here maybe something like NoteSyncRecords or maybe BlockNoteSyncRecords would actually be an improvement (though, I don't think by much).
| // ================================================================================================ | ||
|
|
||
| // Request to subscribe to mempool events. | ||
| message MempoolSubscriptionRequest { |
There was a problem hiding this comment.
Even just MempoolSubscription would make sense to me here.
There was a problem hiding this comment.
I'm not sure MempoolSubscription is a better option here - it implies that the message carries a subscription while it is actually a request for a subscription.
| // ================================================================================================ | ||
|
|
||
| // Returns data required to prove the next block. | ||
| message GetBlockInputsRequest { |
There was a problem hiding this comment.
BlockInputRequirements or BlockInputFields or similar?
There was a problem hiding this comment.
Or *Parameters for messages that specify parameters like this?
There was a problem hiding this comment.
would make more sense in the code:
Request<BlockInputsParameters>
rather than
Request<GetBlockInputsRequest>.
There was a problem hiding this comment.
"InputsParameters" sounds somewhat redundant - but I do think we can improve the naming here by at least getting rid of the Get prefix - e.g.:
GetBlockInputsRequest->BlockInputsRequestGetBatchInputsRequest->BatchInputsRequestGetTransactionInputsRequest->TransactionInputsRequest
There was a problem hiding this comment.
I just pushed this changes, and removed an alias that I missed before.
There was a problem hiding this comment.
"InputsParameters" sounds somewhat redundant
The parameters refers to the fields in the request that result the block inputs returned. Although I could be confused about these endpoints I'm not super familiar.
Removing Get prefix is definitely an improvement. I still have the nit of disliking Request<SomethingRequest> in the code (2x "request" in the type). But its not a problem for readability.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I'm sure we can rename/reshuffle things a bit more, but given the footprint of this PR, I'd prefer merge it sooner rather than later. If there are specific things we want to address, we can do this in follow-up PR/issues.
But would be good to get 1 or 2 more people look through it.
Mirko-von-Leipzig
left a comment
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
Some minor comments you can ignore; this looks like a really good step forward imo. Thank you!
| message SmtLeaf { | ||
| oneof leaf { | ||
| // An empty leaf. | ||
| uint64 empty = 1; |
There was a problem hiding this comment.
This could probably be the empty type google.proto.Empty (or whatever it actually is).
There was a problem hiding this comment.
I think that we still want the leaf index (that's what the empty represents here). Probably it is better to just rename it
There was a problem hiding this comment.
I don't think we need to separate the file descriptors by service, especially for the store.
There was a problem hiding this comment.
It was my suggestion to separate them. My thinking was that this file has gotten very big and it was difficult to parse through.
There may be better ways to split it up though (rather than by service) - but maybe that could be a separate task.
|
After I merged #1097, there are some more merge conflicts. Sorry about that! |
4 participants
closes: #993
This PR aims to refactor the structure of the proto files and some messages. I split the common messages into three files: primitives (with the merkle tree and digest messages), accounts (account headers, info, notes), and blockchain (block headers, transactions).
The services remain as top-level "modules" (as Mirko suggested in the issue), and the messages specific to them were placed inside each one of them. Some messages are shared between services, so I introduced a "shared.proto" file with those.
Also, I removed the Request/Response suffixes. In some cases, due to the same name on both messages without the suffix, I used the suffix
Result. I don't really love this last part, any suggestion on this would be appreciated.The next step is to use this new names from the generated code in our domain. I'll do this in this PR, but opened it anyways to start discussing the new message's structure.