-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Distribute protobuf files through a new crate #391
Conversation
crates/miden-rpc-proto/build.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This build script might be overkill. It goes through the protobuf files and builds the list automatically.
Initially I thought not all protobuf files would be needed to be distributed, but when prost
compiled rpc.proto
it also ended up compiling non-related files as well (like smt.proto
) so I added this. I might have missed some config to avoid this from happening.
PR implementing this on the client: 0xPolygonMiden/miden-client#395 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I was able to build this without default features. I wasn't able to do so for wasm32_unknown_unknown
target though, but it shouldn't be relevant since we're only using the crate in build scripts.
I like the idea of exposing the plain protobuf files, but I also think clients will end up copy-pasting the code to generate the files from the .proto
files. Couldn't we expose both the proto files as well as the generated files?
Another thing that came up to mind is whether we'd be able to, from miden-node-proto, generate the files without the ApiClient
stuff (I think just commenting:
// Compile the proto file for all servers APIs
let protos = &[
proto_dir.join("block_producer.proto"),
proto_dir.join("store.proto"),
proto_dir.join("rpc.proto"),
];
from proto/build.rs
based on a flag might work), and if those files end up being no_std compatible.
We can iterate on the approach though and I think exposing the proto files is something we just want anyways.
Wouldn't generated files need to assume the framework used for generation (e.g., |
Hmmm, I'm not sure. Looking at the generated files, the only ones that depend on having tonic are |
I'm not sure that works, I did a quick test and didn't seem to. Additionally I'm not sure how valuable those created types would be without being wired to some sort of an API client (or similar) and as Bobbin mentioned that probably makes or breaks the possibility of consuming the API/crate properly. I think eventually we could want to make the API client and independent component (both from the node and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left one last comment inline. After this, we can merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good! Thank you!
oh - i forgot: let's also update the changelog in a small follow-up PR. |
Addresses #388