Skip to content
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

More idiomatic domain types for privval responses #1228

Open
mzabaluev opened this issue Nov 14, 2022 · 1 comment
Open

More idiomatic domain types for privval responses #1228

mzabaluev opened this issue Nov 14, 2022 · 1 comment
Labels
domain-types Anything relating to the creation, modification or removal of domain types enhancement New feature or request technical debt Issues that are important, but not urgent, that should eventually receive attention

Comments

@mzabaluev
Copy link
Contributor

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 mzabaluev added enhancement New feature or request technical debt Issues that are important, but not urgent, that should eventually receive attention domain-types Anything relating to the creation, modification or removal of domain types labels Nov 14, 2022
@tony-iqlusion
Copy link
Collaborator

tony-iqlusion commented Nov 14, 2022

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 tendermint/tendermint#9256, unless you're talking about a prospective tendermint-privval crate? (#1152)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain-types Anything relating to the creation, modification or removal of domain types enhancement New feature or request technical debt Issues that are important, but not urgent, that should eventually receive attention
Projects
None yet
Development

No branches or pull requests

2 participants