PoC: gRPC server standardization

[Mirko-von-Leipzig]

Proof of concept for gRPC server implementations.

Each gRPC method gets its own trait (generated by build.rs):

trait <Method> {
    type Input;
    type Output;

    fn decode(request: proto::RequestType) -> tonic::Result<Self::Input>;

    fn encode(output: Self::Output) -> tonic::Result<proto::ResponseType>;

    async fn handle(&self, input: Self::Input) -> tonic::Result<Self::Output>;

    // Implementers can override the behaviour e.g. if its just a proxy shim, then we
    // can just do proxy_client.request(request).await
    async fn full(
        &self,
        request: proto::RequestType,
    ) -> tonic::Result<proto::ResponseType> {
        let input = Self::decode(request)?;
        let output = self.handle(input).await?;
        Self::encode(output)
    }
}

The implementation is not fully complete; I don't support the mempool subscription stream here for example. The comments in the build.rs are also outdated and reference an initial implementation I had.

Some benefits of doing this, over implementing the tonic generated traits:

• We can easily add instrumentation to encode and decode and handle in one location.
• This lays the foundation for GrpcDecode and GrpcEncode traits as a follow-on, which I think will let us be more consistent with our errors here.
• This will also let us do something like Server::public_facing and Server::internal, which changes how errors are encoded (i.e. hide internal errors for public facing).
• Because each method is its own trait, we can implement one method per file.
• Standard implementation of each method.

I've implemented the validator api as an example.

Looking for feedback if this is worth pursuing and cleaning up :) (imo yes, but maybe its not worth it).

[drahnr]

we should try to avoid calling external funbi aries, particularly when it's impossible to pass on flags (i.e. which version / edition does it expect?)

I suggest to use something like prettyplease for generated code https://github.com/drahnr/expander/blob/master/src/lib.rs#L239-L246

[Mirko-von-Leipzig]

I used prettyplease originally, and it gave notably worse output - and also worse dx because if you enter the generated file manually and hit save, your ide may autofmt it again.

I figured using rustfmt would be much better? I don't really care what version or edition it uses - its just to make it legible. It is actually possible to pass edition and other flags in; I removed it because I thought it added no value.

[drahnr]

Minor stylistic proposal: I find it a bit awkward to have many traits, I'd rather have a single, parameterized trait that uses a concrete type as parameter:

impl WireCodec for MySuperDuperInteraction {
type Input = ..;
type Output = ..;
fn decode_input(grpc::input::Type) -> Result<Self::Input> {
..
}
fn encode_output(Self::Output) ->  Result<grpc::output::Type> {
..
}
}


impl GrpcInteraction<MySuperDuperInteraction> for ValidatorServer {
    async fn handle(&self, input: MySuperDuperInteraction::Input) -> tonic::Result<Self::Output> {
..
}

[SantiagoPittella]

Agree, we can achieve the same separation of concerns with fewer generated types

[Mirko-von-Leipzig]

I'm not quite understanding why you think it will result in less types? You can either use more traits, and slightly less traits and many structs and impls. The trait variant is much less code, and less indirection imo.

But maybe I've missed an approach? @SantiagoPittella I don't quite follow your proc macro suggestion -- on what do you place the proc macro?

@drahnr I actually had your suggestion implemented here, in commit d9fd24.

[Mirko-von-Leipzig]

But I agree simplifying would be great. Create a new thread in one of the files with a sketch of the code you want to generate, and we can hash it out

[drahnr]

There should be Module::new("we_are_here").get_or_new_module("foo")

[SantiagoPittella]

I like how methods can be added and implemented with this approach, but the code generation part is difficult to follow. I have some doubts about maintainability of this, though on the other hand I assume that this is not supposed to be changed frequently. This changes + an extensive explanation of the generation should do the trick IMO.

Cab probably achieve similar results without the codegen part, defining a single trait like GrpcMethod<M> + a proc macro:

  /// Marker trait for gRPC method descriptors.
  trait MethodDescriptor {
      type Request;
      type Response;
  }

  /// The standard decode -> handle -> encode pipeline.
  #[tonic::async_trait]
  trait GrpcMethod<M: MethodDescriptor>: Send + Sync {
      type Input;
      type Output;

      fn decode(request: M::Request) -> tonic::Result<Self::Input>;
      fn encode(output: Self::Output) -> tonic::Result<M::Response>;
      async fn handle(&self, input: Self::Input) -> tonic::Result<Self::Output>;

      async fn execute(&self, request: M::Request) -> tonic::Result<M::Response> {
          let input = Self::decode(request)?;
          let output = self.handle(input).await?;
          Self::encode(output)
      }
  }

Also, I'm not sure how streams will be accommodated into this pipeline.

[sergerad]

@Mirko-von-Leipzig I suppose this relates to this issue #1528 (comment)

Have you thought about how it might impact the ConversionError? Should we hold off on the ConversionError conversation while this poc is going on?

[Mirko-von-Leipzig]

@sergerad yeah this overlaps -- but I think we can begin working on that in parallel if we want. I've given some more thought to the API I'd like -- but I don't actually know how to achieve it. I'll write that down in the issue.

[Mirko-von-Leipzig]

Also, I'm not sure how streams will be accommodated into this pipeline.

We have a couple of options; it's not much more complex than the normal method - we just need to decide what to do with the stream type.

The tonic adds an associated stream type for each streaming method e.g.

trait tonic::BlockProducerApi {
    type MempoolSubscriptionStream: tonic::codegen::tokio_stream::Stream<
         Item = tonic::Result<MempoolEvent>
     > + Send + Sync + 'static;

     async fn mempool_subscription(request: ...) -> tonic::Result<Self::MempoolSubscriptonStream>);
}

Some options:

1. Hardcode the associated type to Pin<Box<dyn Stream>> and let the method return any stream
2. Have the method trait also contain an additional associated stream type which gets passed along