Serialization improvements #248
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,78 +1,182 @@ | ||
| //! CommitSig within Commit | ||
|
|
||
| use crate::{account, Signature, Time}; | ||
| use serde::de::{self, Error, MapAccess, Visitor}; | ||
| use serde::{Deserialize, Deserializer, Serialize, Serializer}; | ||
| use std::fmt; | ||
|
|
||
| /// CommitSig represents a signature of a validator. | ||
| /// It's a part of the Commit and can be used to reconstruct the vote set given the validator set. | ||
| #[derive(Clone, Debug, PartialEq)] | ||
| pub enum CommitSig { | ||
| /// no vote was received from a validator. | ||
| BlockIDFlagAbsent { | ||
| /// Validator address | ||
| validator_address: account::Id, | ||
| }, | ||
| /// voted for the Commit.BlockID. | ||
| BlockIDFlagCommit { | ||
| /// Validator address | ||
| validator_address: account::Id, | ||
| /// Timestamp of vote | ||
| timestamp: Time, | ||
| /// Signature of vote | ||
| signature: Signature, | ||
| }, | ||
| /// voted for nil. | ||
| BlockIDFlagNil { | ||
| /// Validator address | ||
| validator_address: account::Id, | ||
| /// Timestamp of vote | ||
| timestamp: Time, | ||
| /// Signature of vote | ||
| signature: Signature, | ||
| }, | ||
| } | ||
|
|
||
| /// CommitSig implementation | ||
| impl CommitSig { | ||
| /// Helper: Extract validator address, since it's always present (according to ADR-025) | ||
|
There was a problem hiding this comment. We really should clarify this first: #246 (comment) I can very well be that adr isn't representing the full decision anymore, or, that it is slightly confusing nil and absent votes. Also: #196 (comment) cc @melekes There was a problem hiding this comment. There was a problem hiding this comment. Created tracking issue #260 about this. |
||
| pub fn validator_address(&self) -> &account::Id { | ||
| match &self { | ||
| CommitSig::BlockIDFlagAbsent { validator_address } => validator_address, | ||
| CommitSig::BlockIDFlagCommit { | ||
| validator_address, .. | ||
| } => validator_address, | ||
| CommitSig::BlockIDFlagNil { | ||
| validator_address, .. | ||
| } => validator_address, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl Serialize for CommitSig { | ||
| fn serialize<S>( | ||
| &self, | ||
| _serializer: S, | ||
| ) -> Result<<S as Serializer>::Ok, <S as Serializer>::Error> | ||
| where | ||
| S: Serializer, | ||
| { | ||
| unimplemented!("CommitSig serialization is not implemented yet") | ||
| } | ||
| } | ||
|
|
||
| impl<'de> Deserialize<'de> for CommitSig { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, <D as Deserializer<'de>>::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| struct CommitSigVisitor; | ||
|
|
||
| impl<'de> Visitor<'de> for CommitSigVisitor { | ||
| type Value = CommitSig; | ||
|
|
||
| fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { | ||
| formatter.write_str("CommitSig") | ||
| } | ||
|
|
||
| /// Deserialize map Visitor implementation | ||
| // Implements decision: https://github.com/tendermint/tendermint/blob/master/docs/architecture/adr-025-commit.md#decision | ||
| fn visit_map<A>(self, map: A) -> Result<Self::Value, <A as MapAccess<'de>>::Error> | ||
| where | ||
| A: MapAccess<'de>, | ||
| { | ||
| // Instead of manually deserializing the whole struct (cumbersome), we use annotations | ||
| fn option_signature<'de, D>(deserializer: D) -> Result<Option<Signature>, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| Deserialize::deserialize(deserializer).map(|x: Option<_>| x.unwrap_or(None)) | ||
| } | ||
| #[derive(Deserialize)] | ||
| struct CommitSigSpec { | ||
| block_id_flag: u8, | ||
| validator_address: account::Id, | ||
| #[serde(default)] | ||
| timestamp: Option<Time>, | ||
| #[serde(default, deserialize_with = "option_signature")] | ||
| signature: Option<Signature>, | ||
| } | ||
|
|
||
| // `MapAccessDeserializer` is a wrapper that turns a `MapAccess` | ||
| // into a `Deserializer`, allowing it to be used as the input to T's | ||
| // `Deserialize` implementation. T then deserializes itself using | ||
| // the entries from the map visitor. | ||
| let incoming = | ||
| CommitSigSpec::deserialize(de::value::MapAccessDeserializer::new(map))?; | ||
|
|
||
| // Validate CommitSig (strict) | ||
| match incoming.block_id_flag { | ||
| // BlockIDFlagAbsent | ||
| 1 => { | ||
| if incoming.timestamp.is_some() { | ||
| Err(A::Error::custom(format!( | ||
| "timestamp is present for BlockIDFlagAbsent CommitSig {}", | ||
| incoming.timestamp.unwrap() | ||
| ))) | ||
| } else { | ||
| if incoming.signature.is_some() { | ||
| Err(A::Error::custom(format!( | ||
| "signature is present for BlockIDFlagAbsent CommitSig {:?}", | ||
| incoming.signature.unwrap() | ||
| ))) | ||
| } else { | ||
| Ok(CommitSig::BlockIDFlagAbsent { | ||
| validator_address: incoming.validator_address, | ||
| }) | ||
| } | ||
| } | ||
greg-szabo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| // BlockIDFlagCommit | ||
| 2 => { | ||
| if incoming.timestamp.is_none() { | ||
| Err(A::Error::custom( | ||
| "timestamp is null for BlockIDFlagCommit CommitSig", | ||
| )) | ||
| } else { | ||
| if incoming.signature.is_none() { | ||
| Err(A::Error::custom( | ||
| "signature is null for BlockIDFlagCommit CommitSig", | ||
| )) | ||
| } else { | ||
| Ok(CommitSig::BlockIDFlagCommit { | ||
| validator_address: incoming.validator_address, | ||
| timestamp: incoming.timestamp.unwrap(), | ||
| signature: incoming.signature.unwrap(), | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| // BlockIDFlagNil | ||
| 3 => { | ||
| if incoming.timestamp.is_none() { | ||
| Err(A::Error::custom( | ||
| "timestamp is null for BlockIDFlagNil CommitSig", | ||
| )) | ||
| } else { | ||
| if incoming.signature.is_none() { | ||
| Err(A::Error::custom( | ||
| "signature is null for BlockIDFlagNil CommitSig", | ||
| )) | ||
| } else { | ||
| Ok(CommitSig::BlockIDFlagNil { | ||
| validator_address: incoming.validator_address, | ||
| timestamp: incoming.timestamp.unwrap(), | ||
| signature: incoming.signature.unwrap(), | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| // Error: unknown CommitSig type | ||
| e => Err(A::Error::custom(format!( | ||
| "unknown BlockIdFlag value in CommitSig {}", | ||
| e | ||
| ))), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| deserializer.deserialize_map(CommitSigVisitor) | ||
|
There was a problem hiding this comment. While the enum idea is cool, it seems to come with the cost of a rather complicated There was a problem hiding this comment. Yes and that's the tradeoff @brapse was going to go for in an earlier discussion. (Feel free to engage Sean.) The idea is that serialization is not actually the core product here and during the usual development it's just annoying to do something some particular way just because serialization says so. There was a problem hiding this comment. I have to admit there is one not-completely-Rust-native-best-practice feature in the code and it annoys me slightly. I deserialize the incoming CommitSig into an internal CommitSigSpec structure that is defined in ADR-025. I could have gone hard-core and implement complete deserialization while parsing the incoming text. But that would be even more convoluted and harder to change down the road. I feel this is a good compromise that allows us to easily update the deserialization code, if a new ADR comes by. There was a problem hiding this comment.
That is absolutely reasonable. Serialization isn't the core product and we shouldn't build our core types around it. All in favour of decoupling serialization concerns and core types (I like this approach for this: #197 (comment) by @tarcieri ). What I was referring to was that the seemingly beautiful change made the part where the serialization actually happens difficult to read and quite complex. A simpler way might be to have a serialization type and a Also, it would be cool if the There was a problem hiding this comment. Done and done: Today's commit moved things into serde's "from" annotation. So now, the code looks a bit cleaner, although it's not less code: So the deserializer does type-check and the From trait does validation. My only grief is that type-checking happens in the serializers library while validation happens in the struct's library (in this case at CommitSig). Interestingly, this is similar to how it used to be, so I'm guessing the developers will be fine with it. There was a problem hiding this comment. If the code looks cleaner that is already worth it 👍 And I think it is OK for the core type to defines when it's valid (s.t. you can only construct a valid type). |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Representing this as an enum is a nice idea 👍