Move generated protos into oak_abi #834
Conversation
|
I'm in two minds as to whether to keep the last two commits. On the one hand, it feels more complete to include the storage channel messages in |
| pub mod oak { | ||
| use oak_abi::proto::oak::label; |
There was a problem hiding this comment.
Maybe add a comment to clarify why this re-export is needed? Presumably we would like to make it so that developers only need to depend on oak and not oak_abi directly? I guess that makes sense, though I'm always a bit hesitant to introduce re-exports, as having multiple names for the same things can be confusing.
There was a problem hiding this comment.
Added a comment – the storage message definitions rely on the Label message, which is now (only) generated under the oak_abi crate.
There was a problem hiding this comment.
I see, may as well leave them out of the ABI then I guess, there is not much point in having them there. I don't really mind either way though.
oak/proto/storage_channel.proto
Outdated
|
|
||
| message StorageChannelRollbackResponse { | ||
| } | ||
| import "oak/proto/storage_messages.proto"; |
There was a problem hiding this comment.
Not sure I follow what's going on here. Why is the service definition not part of the ABI? It seems it should be.
There was a problem hiding this comment.
The generated code for the service definition assumes the use of the Oak SDK code, and so cannot live in the oak_abi crate. That leads to the separation of the message definitions (which only depend on prost and so can live in oak_abi) from the service definition generated code (still in oak, but now with a parallel copy of the message definitions), which leads to my earlier question – is it worth bothering with the storage protos?
| @@ -16,73 +16,9 @@ | |||
|
|
|||
There was a problem hiding this comment.
perhaps this file should be renamed to storage_node or storage_service at some point? (Or just deleted -- hint hint)
There was a problem hiding this comment.
Seems reasonable, but for another day.
There was a problem hiding this comment.
Actually, done: it's now StorageService in oak/proto/storage_service.proto
Protocol buffer messages that are exchanged in serialized form between Wasm Nodes and Oak-provided pseudo-Nodes are effectively part of the Oak ABI, so move the generated code for them into the oak_abi crate. This allows us to remove the oak_runtime->oak dependency that was introduced by #772. Along the way, put the gRPC encapsulation protobuf messages into an `encap` submodule/namespace. Fixes #764.
The storage.proto file holds the gRPC service definition for the external storage processor; this is only needed by the C++ storage pseudo-Node code.
|
Dropped the last two commits (which split the storage proto and copied the message definitions to |
Also move into a submodule/namespace oak.storage, and rename the StorageNode service to StorageService.
Protocol buffer messages that are exchanged in serialized form between
Wasm Nodes and Oak-provided pseudo-Nodes are effectively part of the
Oak ABI, so move the generated code for them into the oak_abi crate.
This allows us to remove the oak_runtime->oak dependency that was
introduced by #772.
Along the way, put the gRPC encapsulation protobuf messages into an
encapsubmodule/namespace.Fixes #764.
Checklist
cover any TODOs and/or unfinished work.
construction.