Thank you! Not a review yet, but I wanted to understand the overall approach. The way I see it is as follows:

• We still keep protobuf files in the proto directory in the root of the repo as the "source of truth".

• These files are then copied to the miden-node-proto-build crate during the build process and are used to set the protobuf files for the tonic builder (which is passed-in externally).

  • I actually didn't see the code that copies the file from the root into the crate - but maybe I missed it.

• Then, in miden-node-proto we import miden-node-proto-build as a dependency and use it to actually generate the bindings.

• If someone wanted to use the protobuf files (e.g., miden-client) they would add miden-node-proto-build as a build dependency and this would allow them to generate the desired bindings.

I was hoping to only have the proto files directly in the miden-node-proto-build. No more copies, only that single set. We would highlight their location in the readme and the docs if we're concerned about discoverability, or we could move this particular crate to the top level (instead of within ./crates) and could even keep the folder named proto.

Assuming this is roughly correct, I think it works (and is probably a cleaner design than what we had before) - but I don't think it addresses the original issue. That is, we still need to either remove the client code from the PRC crate or somehow simplify the support for TLS.

I also think that people will still probably try to use miden-node-rpc to access the RPC component as the client and maybe pointing them to miden-node-proto-build may be too much of a burden.

I agree they may still do this, but I think we can minimize this with a

and a proper example in the <proto-build> crate and readme.

One solution is to add miden-node-rpc-client crate (which would basically replace the old miden-rpc-proto crate). This crate would expose bindings with TLS and other features configured. This crate can then be directly used in miden-client (i.e., no need to build anything).

Another option is to use a single miden-node-rpc crate but have client and server features. The server feature would be used by the node internally, and the client feature would be used externally (e.g., by the miden-client). This may be a bit more complicated to implement though.

My hope is really to push users to the builder only. They may need both client and server for RPC - the latter for testing, or to create a proxy. In this case I think having both exposed by default, along with a solid example, should suffice as a jumping off point for users.

The only suboptimal point for me is the lack of domain types - but tbh they shouldn't be relying on our "internal" types, that just shows a lack of documentation in our gRPC API.

I was hoping to only have the proto files directly in the miden-node-proto-build. No more copies, only that single set. We would highlight their location in the readme and the docs if we're concerned about discoverability, or we could move this particular crate to the top level (instead of within ./crates) and could even keep the folder named proto.

Yeah, I was mostly concerned about discoverability. Adding this to docs could work - but it'll need to be pretty prominent otherwise those searching for proto files won't find them. Moving the crate to the root level may be better.

My hope is really to push users to the builder only. They may need both client and server for RPC - the latter for testing, or to create a proxy. In this case I think having both exposed by default, along with a solid example, should suffice as a jumping off point for users.

For testing, just RPC is not enough - right? They'd have to run the full node.

I think the builders could be for more sophisticated users and if we can provide the required functionality via a simple import (assuming it doesn't complicate things too much for us), that would be better. But maybe this can be a separate issue.

To summarize:

• We move miden-node-proto-build to the root level (into the proto directory). I guess the actual protobuf files will end up in proto/proto directory - right?
  • We add a good example of how to set up a build script using the miden-node-proto crate.
• We remove the client code from miden-node-rpc and add relevant sections to the readme to point to miden-node-proto-build.

My hope is really to push users to the builder only. They may need both client and server for RPC - the latter for testing, or to create a proxy. In this case I think having both exposed by default, along with a solid example, should suffice as a jumping off point for users.

For testing, just RPC is not enough - right? They'd have to run the full node.

It depends on what they want or need to test. I imagine basic testing could be done with server stubs and would be how I do unit testing. Or maybe by using snapshots - create the snapshots by running a node once, then use the RPC server to serve the snapshots for actual testing and CI. Its a pattern I'd like to move towards as well for more complex setups especially if it involved proving or things like proof-of-work.

I think the builders could be for more sophisticated users and if we can provide the required functionality via a simple import (assuming it doesn't complicate things too much for us), that would be better. But maybe this can be a separate issue.

I'm not convinced we want/need this, but let's tackle that separately :)

To summarize:

• We move miden-node-proto-build to the root level (into the proto directory). I guess the actual protobuf files will end up in proto/proto directory - right?

  • We add a good example of how to set up a build script using the miden-node-proto crate.

• We remove the client code from miden-node-rpc and add relevant sections to the readme to point to miden-node-proto-build.

Correct yes.

We remove the client code from miden-node-rpc and add relevant sections to the readme to point to miden-node-proto-build.

@bobbinth I am not sure if I am completely on the same page on this. In the rpc crate there is only a RpcApi server component, that is used on the node binary. Which client code do you mean?

We remove the client code from miden-node-rpc and add relevant sections to the readme to point to miden-node-proto-build.

@bobbinth I am not sure if I am completely on the same page on this. In the rpc crate there is only a RpcApi server component, that is used on the node binary. Which client code do you mean?

He meant the client exposed from proto which is prebuilt. Or rather I guess I did because I wrote rpc in the original issue description, confusing the matter.

The tonic_build compile_protos_with_config function receives both the file descriptors with compiled proto files (I think this is used for server reflexion), and the raw proto files. So we would need to distribute the proto files anyway. For this, I think we could just keep the old way of copying the files and let the users compile them.

There is the https://docs.rs/tonic-build/latest/tonic_build/struct.Builder.html#method.compile_fds_with_config which compiles using the file descriptors alone. I think the file descriptors fully describe the .proto files and are essentially just the parsed version of them used by tonic_build, but I could be wrong.

3. We should then remove the various proto build scripts and the generated code from being checked in ideally.

By this you mean to have the generated code stored in "OUT_DIR", instead of being tracked and commited with the source code on miden-node-proto? In that case, I think I would be a good idea to do that in a separate PR and discuss it there.

This discussion is also about this approach: 0xPolygonMiden/miden-base#994

I'll revive that discussion and we can discuss it there. Let's leave that suggestion out of this PR :)

@bobbinth thanks! I addressed the comments.

One other thing: we should still remove the ApiClient from the rpc crate, right?

About this, I am still not sure if I understand which code you mean. Maybe you mean the rpc ApiClient from the miden-node-proto crate?

@bobbinth There is no ApiClient in the miden-node-rpc crate. Assuming you mean the rpc ApiClient from the proto crate, the problem with deleting it is that it's used by the faucet, as we discussed in the original issue. I understood that Mirko's view on this was to just push users to use the new proto-build crate, and leave the proto crate for internal use only. I added a warning in the crate's readme for this (ac049e3). I should also update this PR's title.

An alternative could be to add again a feature flag to the proto crate ("rpc-client"), that would allow the faucet to access it, but not be included by default. This would mean that we wouldn't be deleting the ApiClient, the generated files will stay as they are now.

To actually delete the ApiClient from the crate we need a different solution. It could be to add bindings generation for the faucet, though I think that would add too much unnecessary boilerplate. Or perhaps to move the rpc client into a separate new crate.