Conversation
There was a problem hiding this comment.
Top level overview; attaching to an arbitrary file so we can discuss inline.
I think this gives the correct end result, but I worry about the ergonomics of it. Having to specify the field, type and operation is a problem and will be error prone. I'll leaving some thoughts on possible things to explore.
At a higher level, I want to move away from TryFrom for something this specific. We really should use a dedicated GrpcDecode trait. This would be a rather large diff so I would suggest we do this in tandem with #1742. This allows us to have the current implementation, where we can then slowly move over to the #1742 piece by piece without requiring all things to change at once. So as we implement a method for #1742, we also implement the GrpcDecode parts that we need.
| let prev_block_commitment = value | ||
| .prev_block_commitment | ||
| .try_convert_field::<proto::blockchain::BlockHeader>("prev_block_commitment")?; | ||
| let chain_commitment = value | ||
| .chain_commitment | ||
| .try_convert_field::<proto::blockchain::BlockHeader>("chain_commitment")?; | ||
| let account_root = value | ||
| .account_root | ||
| .try_convert_field::<proto::blockchain::BlockHeader>("account_root")?; | ||
| let nullifier_root = value | ||
| .nullifier_root | ||
| .try_convert_field::<proto::blockchain::BlockHeader>("nullifier_root")?; | ||
| let note_root = value | ||
| .note_root | ||
| .try_convert_field::<proto::blockchain::BlockHeader>("note_root")?; | ||
| let tx_commitment = value | ||
| .tx_commitment | ||
| .try_convert_field::<proto::blockchain::BlockHeader>("tx_commitment")?; | ||
| let tx_kernel_commitment = value | ||
| .tx_kernel_commitment | ||
| .try_convert_field::<proto::blockchain::BlockHeader>("tx_kernel_commitment")?; | ||
| let validator_key = value | ||
| .validator_key | ||
| .try_convert_field::<proto::blockchain::BlockHeader>("validator_key")?; | ||
| let fee_parameters = value | ||
| .fee_parameters | ||
| .try_convert_field::<proto::blockchain::BlockHeader>("fee_parameters")?; |
There was a problem hiding this comment.
serde solves the redundancy of specifying the parent type (proto::blockchain::BlockHeader) by having dedicated sub-decoders. Something like this (I forget the exact details):
/// This gives a single place to inject the parent struct name.
struct GrpcStructDecoder<T: GrpcMessage>;
impl GrpcStructDecoder<T: GrpcMessage> {
fn decode_field<U: GrpcDecode, V>(name: &'static str, value: U) -> Result<V> {
value.decode().context(name)?
}
}
let mut decoder = value.decode_struct();
let account_root = decoder.decode_field("account_root", value.account_root)?;and they have similar helper structs for arrays (which inject indices), and enums etc.
This still requires manually specifying the field name which is a bummer, but I wonder if we can't write a proc-macro that injects that intelligently for us to give something along
#[GrpcDecode::struct]
impl GrpcDecode<BlockHeader> for proto::blockchain::BlockHeader {
fn decode(self) -> Result<BlockHeader, GrpcDecodeError> {
let prev_block_commitment = self.prev_block_commitment.decode()?;
let chain_commitment = self.chain_commitment.decode()?;
let account_root = self.account_root.decode()?;
let nullifier_root = self.nullifier_root.decode()?;
...
Ok(BlockHeader::new(...))
}
}| .ok_or(ConversionError::missing_field::<proto::blockchain::FeeParameters>( | ||
| "native_asset_id", | ||
| ))? |
There was a problem hiding this comment.
Maybe we can add a decode_required() which the proc macro can fill in appropriately?
| fn try_from(value: proto::blockchain::BlockBody) -> Result<Self, Self::Error> { | ||
| BlockBody::read_from_bytes(&value.block_body) | ||
| .map_err(|source| ConversionError::deserialization_error("BlockBody", source)) | ||
| .map_err(|source| ConversionError::deserialization("BlockBody", source)) |
There was a problem hiding this comment.
Since this occurs a lot, I wonder if we can't add our own extension trait that does this more trivially somehow. e.g. Vec<u8>::decode_bytes for gRPC specially.
Context
Closes #1528.
ConversionErroris currently factored as an enum whose variants are never matched on.We also wish to add proto field paths to error contents when proto types are being converted to domain types, producing messages like
"header.account_root: value is not in range 0..MODULUS".Changes
ConversionErrorfrom a public enum to an opaque struct with a field path stack and a boxed error source. TheDisplayimpl renders dotted paths likeheader.account_root: value is not in range 0..MODULUS.ConversionResultExttrait onResult<T, E: Into<ConversionError>>to ergonomically inject field context via.context("field_name")at each?site.TryConvertFieldExttrait onOption<T>that combinesok_or(missing_field)+try_into()+.context()into a single.try_convert_field::<ParentProtoMessage>("field_name")?call, eliminating the most common three-line boilerplate pattern.impl_from_for_conversion_error!macro to replace 13 identicalFrom<ExternalError> for ConversionErrorimplementations..context()calls through all proto-to-domain conversions incrates/proto/src/domain/(account, block, batch, note, nullifier, merkle, transaction, mempool, digest)..context()through downstream crates:store/src/server/,block-producer/src/store/,ntx-builder/src/clients/store.rs.missing_fieldtype parameters (e.g.missing_field::<FeeParameters>("fee_parameters")→missing_field::<BlockHeader>("fee_parameters")wherefee_parametersis a field onBlockHeader).read_account_idgeneric over the parent proto message type somissing_fielderrors correctly identify the containing message.stringify!(field_name)with"field_name"string literals inmissing_fieldcalls.prostfrommiden-node-protofor downstream use of theprost::Messagetrait bound.ConversionResultExt(single field, nested paths, deep nesting, external error types) andTryConvertFieldExt(missing field, conversion error).