Add serde for Signature types (#4109)

* refactor: Added `from_slice()` method to ECDSA signatures

* doc: Modified ECDSA signature docstring to note Recovery ID

* feat: Implemented serde for Signature types

Note: using hexstring encoding

* feat: Automatically derive serde for MultiSignature

* refactor: Convert hex bytes using try_from instead of from_slice

Avoids panicking in critical code

Implemented from Peer Review

* clean: spaces -> tabs

* test: Added tests for Signature serialization

Added dependency on serde_json for testing purposes

substrate/Cargo.lock

@@ -5985,6 +5985,7 @@ dependencies = [
  "rustc-hex 2.0.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "schnorrkel 0.8.5 (registry+https://github.com/rust-lang/crates.io-index)",
  "serde 1.0.102 (registry+https://github.com/rust-lang/crates.io-index)",
+ "serde_json 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)",
  "sha2 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "sr-std 2.0.0",
  "substrate-bip39 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)",

substrate/primitives/core/Cargo.toml

@@ -44,6 +44,7 @@ pretty_assertions = "0.6.1"
 hex-literal = "0.2.1"
 rand = "0.7.2"
 criterion = "0.2.11"
+serde_json = "1.0"
 
 [[bench]]
 name = "bench"

substrate/primitives/core/src/ecdsa.rs

@@ -172,7 +172,7 @@ impl rstd::hash::Hash for Public {
 	}
 }
 
-/// A signature (a 512-bit value).
+/// A signature (a 512-bit value, plus 8 bits for recovery ID).
 #[derive(Encode, Decode)]
 pub struct Signature([u8; 65]);
 
@@ -190,6 +190,23 @@ impl rstd::convert::TryFrom<&[u8]> for Signature {
 	}
 }
 
+#[cfg(feature = "std")]
+impl Serialize for Signature {
+	fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> where S: Serializer {
+		serializer.serialize_str(&hex::encode(self))
+	}
+}
+
+#[cfg(feature = "std")]
+impl<'de> Deserialize<'de> for Signature {
+	fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> where D: Deserializer<'de> {
+		let signature_hex = hex::decode(&String::deserialize(deserializer)?)
+			.map_err(|e| de::Error::custom(format!("{:?}", e)))?;
+		Ok(Signature::try_from(signature_hex.as_ref())
+			.map_err(|e| de::Error::custom(format!("{:?}", e)))?)
+	}
+}
+
 impl Clone for Signature {
 	fn clone(&self) -> Self {
 		let mut r = [0u8; 65];
@@ -259,6 +276,16 @@ impl Signature {
 		Signature(data)
 	}
 
+	/// A new instance from the given slice that should be 65 bytes long.
+	///
+	/// NOTE: No checking goes on to ensure this is a real signature. Only use it if
+	/// you are certain that the array actually is a signature. GIGO!
+	pub fn from_slice(data: &[u8]) -> Self {
+		let mut r = [0u8; 65];
+		r.copy_from_slice(data);
+		Signature(r)
+	}
+
 	/// Recover the public key from this signature and a message.
 	#[cfg(feature = "full_crypto")]
 	pub fn recover<M: AsRef<[u8]>>(&self, message: M) -> Option<Public> {
@@ -501,6 +528,7 @@ mod test {
 	use super::*;
 	use hex_literal::hex;
 	use crate::crypto::DEV_PHRASE;
+	use serde_json;
 
 	#[test]
 	fn default_phrase_should_be_used() {
@@ -613,4 +641,27 @@ mod test {
 		let cmp = Public::from_ss58check(&s).unwrap();
 		assert_eq!(cmp, public);
 	}
+
+	#[test]
+	fn signature_serialization_works() {
+		let pair = Pair::from_seed(b"12345678901234567890123456789012");
+		let message = b"Something important";
+		let signature = pair.sign(&message[..]);
+		let serialized_signature = serde_json::to_string(&signature).unwrap();
+		// Signature is 65 bytes, so 130 chars + 2 quote chars
+		assert_eq!(serialized_signature.len(), 132);
+		let signature = serde_json::from_str(&serialized_signature).unwrap();
+		assert!(Pair::verify(&signature, &message[..], &pair.public()));
+	}
+
+	#[test]
+	fn signature_serialization_doesnt_panic() {
+		fn deserialize_signature(text: &str) -> Result<Signature, serde_json::error::Error> {
+			Ok(serde_json::from_str(text)?)
+		}
+		assert!(deserialize_signature("Not valid json.").is_err());
+		assert!(deserialize_signature("\"Not an actual signature.\"").is_err());
+		// Poorly-sized
+		assert!(deserialize_signature("\"abc123\"").is_err());
+	}
 }

substrate/primitives/core/src/ed25519.rs

@@ -26,6 +26,8 @@ use codec::{Encode, Decode};
 
 #[cfg(feature = "full_crypto")]
 use blake2_rfc;
+#[cfg(feature = "full_crypto")]
+use core::convert::TryFrom;
 #[cfg(feature = "std")]
 use substrate_bip39::seed_from_entropy;
 #[cfg(feature = "std")]
@@ -85,11 +87,11 @@ impl AsMut<[u8]> for Public {
 }
 
 impl Deref for Public {
-    type Target = [u8];
+	type Target = [u8];
 
-    fn deref(&self) -> &Self::Target {
-        &self.0
-    }
+	fn deref(&self) -> &Self::Target {
+		&self.0
+	}
 }
 
 impl rstd::convert::TryFrom<&[u8]> for Public {
@@ -127,11 +129,11 @@ impl From<Public> for H256 {
 
 #[cfg(feature = "std")]
 impl std::str::FromStr for Public {
-    type Err = crate::crypto::PublicError;
+	type Err = crate::crypto::PublicError;
 
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        Self::from_ss58check(s)
-    }
+	fn from_str(s: &str) -> Result<Self, Self::Err> {
+		Self::from_ss58check(s)
+	}
 }
 
 impl UncheckedFrom<[u8; 32]> for Public {
@@ -199,6 +201,23 @@ impl rstd::convert::TryFrom<&[u8]> for Signature {
 	}
 }
 
+#[cfg(feature = "std")]
+impl Serialize for Signature {
+	fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> where S: Serializer {
+		serializer.serialize_str(&hex::encode(self))
+	}
+}
+
+#[cfg(feature = "std")]
+impl<'de> Deserialize<'de> for Signature {
+	fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> where D: Deserializer<'de> {
+		let signature_hex = hex::decode(&String::deserialize(deserializer)?)
+			.map_err(|e| de::Error::custom(format!("{:?}", e)))?;
+		Ok(Signature::try_from(signature_hex.as_ref())
+			.map_err(|e| de::Error::custom(format!("{:?}", e)))?)
+	}
+}
+
 impl Clone for Signature {
 	fn clone(&self) -> Self {
 		let mut r = [0u8; 64];
@@ -531,6 +550,7 @@ mod test {
 	use super::*;
 	use hex_literal::hex;
 	use crate::crypto::DEV_PHRASE;
+	use serde_json;
 
 	#[test]
 	fn default_phrase_should_be_used() {
@@ -643,4 +663,27 @@ mod test {
 		let cmp = Public::from_ss58check(&s).unwrap();
 		assert_eq!(cmp, public);
 	}
+
+	#[test]
+	fn signature_serialization_works() {
+		let pair = Pair::from_seed(b"12345678901234567890123456789012");
+		let message = b"Something important";
+		let signature = pair.sign(&message[..]);
+		let serialized_signature = serde_json::to_string(&signature).unwrap();
+		// Signature is 64 bytes, so 128 chars + 2 quote chars
+		assert_eq!(serialized_signature.len(), 130);
+		let signature = serde_json::from_str(&serialized_signature).unwrap();
+		assert!(Pair::verify(&signature, &message[..], &pair.public()));
+	}
+
+	#[test]
+	fn signature_serialization_doesnt_panic() {
+		fn deserialize_signature(text: &str) -> Result<Signature, serde_json::error::Error> {
+			Ok(serde_json::from_str(text)?)
+		}
+		assert!(deserialize_signature("Not valid json.").is_err());
+		assert!(deserialize_signature("\"Not an actual signature.\"").is_err());
+		// Poorly-sized
+		assert!(deserialize_signature("\"abc123\"").is_err());
+	}
 }

substrate/primitives/core/src/sr25519.rs

@@ -26,6 +26,8 @@ use rstd::vec::Vec;
 use schnorrkel::{signing_context, ExpansionMode, Keypair, SecretKey, MiniSecretKey, PublicKey,
 	derive::{Derivation, ChainCode, CHAIN_CODE_LENGTH}
 };
+#[cfg(feature = "full_crypto")]
+use core::convert::TryFrom;
 #[cfg(feature = "std")]
 use substrate_bip39::mini_secret_from_entropy;
 #[cfg(feature = "std")]
@@ -91,11 +93,11 @@ impl AsMut<[u8]> for Public {
 }
 
 impl Deref for Public {
-    type Target = [u8];
+	type Target = [u8];
 
-    fn deref(&self) -> &Self::Target {
-        &self.0
-    }
+	fn deref(&self) -> &Self::Target {
+		&self.0
+	}
 }
 
 impl From<Public> for [u8; 32] {
@@ -112,11 +114,11 @@ impl From<Public> for H256 {
 
 #[cfg(feature = "std")]
 impl std::str::FromStr for Public {
-    type Err = crate::crypto::PublicError;
+	type Err = crate::crypto::PublicError;
 
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        Self::from_ss58check(s)
-    }
+	fn from_str(s: &str) -> Result<Self, Self::Err> {
+		Self::from_ss58check(s)
+	}
 }
 
 impl rstd::convert::TryFrom<&[u8]> for Public {
@@ -200,6 +202,23 @@ impl rstd::convert::TryFrom<&[u8]> for Signature {
 	}
 }
 
+#[cfg(feature = "std")]
+impl Serialize for Signature {
+	fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> where S: Serializer {
+		serializer.serialize_str(&hex::encode(self))
+	}
+}
+
+#[cfg(feature = "std")]
+impl<'de> Deserialize<'de> for Signature {
+	fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> where D: Deserializer<'de> {
+		let signature_hex = hex::decode(&String::deserialize(deserializer)?)
+			.map_err(|e| de::Error::custom(format!("{:?}", e)))?;
+		Ok(Signature::try_from(signature_hex.as_ref())
+			.map_err(|e| de::Error::custom(format!("{:?}", e)))?)
+	}
+}
+
 impl Clone for Signature {
 	fn clone(&self) -> Self {
 		let mut r = [0u8; 64];
@@ -606,6 +625,7 @@ mod test {
 	use super::*;
 	use crate::crypto::{Ss58Codec, DEV_PHRASE, DEV_ADDRESS};
 	use hex_literal::hex;
+	use serde_json;
 
 	#[test]
 	fn default_phrase_should_be_used() {
@@ -748,4 +768,27 @@ mod test {
 		));
 		assert!(Pair::verify(&js_signature, b"SUBSTRATE", &public));
 	}
+
+	#[test]
+	fn signature_serialization_works() {
+		let pair = Pair::from_seed(b"12345678901234567890123456789012");
+		let message = b"Something important";
+		let signature = pair.sign(&message[..]);
+		let serialized_signature = serde_json::to_string(&signature).unwrap();
+		// Signature is 64 bytes, so 128 chars + 2 quote chars
+		assert_eq!(serialized_signature.len(), 130);
+		let signature = serde_json::from_str(&serialized_signature).unwrap();
+		assert!(Pair::verify(&signature, &message[..], &pair.public()));
+	}
+
+	#[test]
+	fn signature_serialization_doesnt_panic() {
+		fn deserialize_signature(text: &str) -> Result<Signature, serde_json::error::Error> {
+			Ok(serde_json::from_str(text)?)
+		}
+		assert!(deserialize_signature("Not valid json.").is_err());
+		assert!(deserialize_signature("\"Not an actual signature.\"").is_err());
+		// Poorly-sized
+		assert!(deserialize_signature("\"abc123\"").is_err());
+	}
 }

substrate/primitives/sr-primitives/src/lib.rs

@@ -164,6 +164,7 @@ impl BuildStorage for (StorageOverlay, ChildrenStorageOverlay) {
 pub type ConsensusEngineId = [u8; 4];
 
 /// Signature verify that can work with any known signature types..
+#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
 #[derive(Eq, PartialEq, Clone, Encode, Decode, RuntimeDebug)]
 pub enum MultiSignature {
 	/// An Ed25519 signature.