tendermint-rs icon indicating copy to clipboard operation
tendermint-rs copied to clipboard

More idiomatic domain types for privval responses

Open mzabaluev opened this issue 2 years ago • 1 comments

Description

The privval gRPC protocol responses have the same structure and convention: they either have the usable value field, or the error field present. This looks very much like the Rust Result type, and the tendermint-rs domain type should also be an enum with a conversion to Result, if not straight Result (unfortunately, it's not possible to define Protobuf conversions for the latter).

Definition of "done"

These types in tendermint are changed to enums to represent either a success value or a RemoteSignerError variant:

  • proposal::SignedProposalResponse
  • public_key::PubKeyResponse
  • vote::SignedVoteResponse

In TryFrom conversions from the tendermint-proto structs, presence of both fields is treated as an error.

mzabaluev avatar Nov 14 '22 11:11 mzabaluev

FWIW here's how I've handled this elsewhere: https://github.com/iqlusioninc/iqkms/blob/2ac999e/iqkms-ethereum/src/error.rs#L58-L62

It defines a conversion from an Error type to tonic::Status, as gRPC responses are modeled as Result<T, Status>. It winds up looking like this in the context of an actual request handler: https://github.com/iqlusioninc/iqkms/blob/2ac999e/iqkms-ethereum/src/signer.rs#L59-L62

There's quite a bit of rich error information you can potentially return as a tonic::Status.

See also: #1147.

Also I take it this whole issue is hypothetical as #1227 is blocked on https://github.com/tendermint/tendermint/issues/9256, unless you're talking about a prospective tendermint-privval crate? (#1152)

tony-iqlusion avatar Nov 14 '22 23:11 tony-iqlusion