Implement Serialize/Deserialize #71

vldm · 2017-04-26T16:02:55Z

In this request merged next commits:

Updating serde to "1.0"
Implemented exonum json serialization/deserialization aspects
For now there three new traits:

ExonumJsonSerialize implemented for all types that should be serializable in json.
ExonumJsonDeserialize for types that can be created from json Value
ExonumJsonDeserializeField for types that can deserialize and write Value as Field

Additionally there some other objects that should be mentioned there:

WriteBufferWrapper - Trait for all types that can write field in their body
ExonumJsonSerializeWrapper and fn wrap() implemented for ExonumJsonSerialize. This types allows use serde serializer for our purposes.

TODO: This is temporary Serialization/Deserialization implementation for our packed structures.
It should be rewritten after storage_value refactor.

defuz · 2017-04-27T21:15:47Z

exonum/src/blockchain/spec.rs

@@ -1,5 +1,9 @@
 #[macro_export]
 macro_rules! storage_value {
+    (@count ) => {0};


Please, implement this as separate macros:

macro_rules! counter { () => { 0 }; ($head:ident $($tail:ident)*) => { 1 + counter!($($tail)*) } }

and than just call counter!($($field_name ))

@defuz if I implement it in separate macros, then I should export this macros too, and waste global namespace, is it ok?

stanislav-tkach · 2017-05-05T10:43:32Z

exonum/src/blockchain/spec.rs

+                    use ::serde::ser::SerializeStruct;
+                    let mut strukt = serializer.serialize_struct(stringify!($name), counter!($($field_name)*))?;
+                    $(strukt.serialize_field(stringify!($field_name), &$crate::serialize::json::wrap(&self.$field_name()))?;)*
+                    strukt.end()               


strukt is a funny name. I understand that you cannot use struct name, but maybe a simple val would be better? Nothing better comes to my mind right now.

stanislav-tkach · 2017-05-05T10:44:07Z

exonum/src/config/mod.rs


        Ok(())
    }
-}
+}


Please add a new line at the end of the file.

stanislav-tkach · 2017-05-05T10:47:06Z

blockchain_explorer/src/api.rs

@@ -133,12 +134,16 @@ struct HexVisitor<T>
    _p: PhantomData<T>,
 }

-impl<T> Visitor for HexVisitor<T>
+impl<'a, T> Visitor<'a> for HexVisitor<T>
    where T: AsRef<[u8]> + HexValue + Clone
 {


Just a nitpick: you used 'v lifetime for Visitor in other places. It would be consistent to use it everywhere.

stanislav-tkach · 2017-05-05T10:49:27Z

exonum/src/messages/fields.rs

+impl<'a> Field<'a> for &'a Signature {
+    fn field_size() -> usize {
+        32
+    }


It would be better to use std::mem::size_of::<Signature>() instead of the hard-coded value.

Yes, but I will done this with all fields after refactor Fields

stanislav-tkach · 2017-05-05T10:51:48Z

exonum/src/messages/protocol.rs

-}
-
-// Message with current state.
+// сообщение о текущем состоянии


Please leave the English version of the comment.

stanislav-tkach · 2017-05-05T10:59:48Z

exonum/src/serialize/json.rs

+}
+
+impl WriteBufferWrapper for ::messages::MessageWriter {
+    fn write<'a, T: Field<'a> >(&'a mut self, from: usize, to: usize, val:T){


Is there a specific reason for importing messages::Field and not messages::MessageWriter?

stanislav-tkach · 2017-05-05T11:00:12Z

exonum/src/serialize/json.rs

+pub fn from_str<T: ExonumJsonDeserialize>(value: &str) -> Result<T, Box<Error>> {    
+    let value: Value = ::serde_json::from_str(value)?;
+    from_value(&value)
+}


New line. 😄

stanislav-tkach · 2017-05-05T11:01:35Z

exonum/src/storage/merkle_patricia_table/proofpathtokey.rs

@@ -533,4 +523,4 @@ mod tests {
        assert_eq!(&bit_vec.repr(), "0111111101");
    }

-}
+}


stanislav-tkach · 2017-05-05T11:01:53Z

exonum/src/serialize/utils.rs

+macro_rules! counter {
+    () => (0usize);
+    ($head:ident $($tail:ident)*) => (1usize + counter!($($tail)*))
+}


stanislav-tkach · 2017-05-05T11:01:59Z

exonum/src/serialize/mod.rs

+}
+
+#[macro_use]
+mod utils;


alekseysidorov · 2017-05-05T14:08:46Z

exonum/src/config/mod.rs

@@ -11,11 +11,13 @@ use std::fs::File;
 pub struct ConfigFile {}

 impl ConfigFile {
-    pub fn load<T: Deserialize>(path: &Path) -> Result<T, Box<Error>> {
+    pub fn load<T>(path: &Path) -> Result<T, Box<Error>> 
+        where T: for<'r> Deserialize<'r>


You can also update toml crate and simplify this code.

Does toml has new api for working with files?

alekseysidorov · 2017-05-05T14:14:47Z

exonum/src/serialize/json.rs

+}
+
+/*
+macro_rules! impl_deserialize_float {


Why this code is commented?

because we currently didn't support float fields

alekseysidorov · 2017-05-05T14:17:22Z

blockchain_explorer/Cargo.toml

-serde = "0.8"
-serde_derive = "0.8"
-serde_json = "0.8"
+serde = "1.0"


Racer does not like version strings without patch. It is better to use "1.0.0" instead of "1.0".

stanislav-tkach

Still I don't like strukt variable name. If you have no idea for a more specific name, even struct_ will be better, as for me.

vldm · 2017-05-15T08:02:03Z

Rebased again, review pls @defuz @alekseysidorov @DarkEld3r @gisochre

dj8yfo

Are we supposed to use only Exonum-J... traits and available helpers in exonum::serialize::json::{to_value, from_value, to_string, from_str}? https://travis-ci.com/exonum/cryptocurrency/builds/46367602.
A minor issue is this:

error[E0277]: the trait bound `&str: exonum::serialize::json::ExonumJsonSerialize` is not satisfied
  --> src/lib.rs:71:1
   |
71 | message! {
   | ^ the trait `exonum::serialize::json::ExonumJsonSerialize` is not implemented for `&str`
   |
   = note: required because of the requirements on the impl of `serde::Serialize` for `exonum::serialize::json::ExonumJsonSerializeWrapper<'_, &str>`
   = note: this error originates in a macro outside of the current crate

error[E0277]: the trait bound `&str: exonum::serialize::json::ExonumJsonDeserializeField` is not satisfied
  --> src/lib.rs:71:1
   |
71 | message! {
   | ^ the trait `exonum::serialize::json::ExonumJsonDeserializeField` is not implemented for `&str`
   |
   = note: required by `exonum::serialize::json::ExonumJsonDeserializeField::deserialize`
   = note: this error originates in a macro outside of the current crate

The major issue for me is this: serde::Serialize, Deserialize isn't implemented for storage! and message!.

This would be inconvenient, when composing structs with #[derive(Ser/De)]. But we can live without it, probably, or do the following manually:

impl<'de> Deserialize<'de> for Wallet {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
        where D: Deserializer<'de>
    {
        let value = <Value>::deserialize(deserializer)?;
        from_value(&value).map_err(|_| D::Error::custom(""))
    }
}
impl Serialize for Wallet {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
        where S: Serializer
        {
            wrap(self).serialize(serializer)
        }
}

@defuz. What approach do we pursue? Why not add default Serialize/Deserialize generation in macros? maybe there's a good reason not to add it now or ever.

dj8yfo · 2017-05-19T01:38:17Z

exonum/src/messages/spec.rs

+                use $crate::messages::{RawMessage, MessageWriter};
+                let mut writer = MessageWriter::new($extension, $id, $body);
+                // if we could deserialize values, try append signature
+                <Self as ExonumJsonDeserializeField>::deserialize(value, &mut writer, from, to)?;


and checking service_idand message_id against the expected $extension and $id.

dj8yfo · 2017-05-19T01:40:16Z

exonum/src/messages/spec.rs

+                fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> where S: $crate::serialize::json::reexport::Serializer {
+                    use $crate::serialize::json::reexport::SerializeStruct;
+                    let mut structure = serializer.serialize_struct(stringify!($name), counter!($($field_name)*) + 1)?;
+                    $(structure.serialize_field(stringify!($field_name), &$crate::serialize::json::wrap(&self.$field_name()))?;)*


no service_id, message_id and network_id here. I guess, they should be added, maybe, binary message length too, not sure about this last one.

dj8yfo

#[derive(Debug, Clone, PartialEq)]
pub struct BlockProof {
    pub block: blockchain::Block,
    pub precommits: Vec<Precommit>,
}

dj8yfo · 2017-05-22T16:27:57Z

exonum/src/messages/spec.rs

+                let body = obj.get("body").ok_or("Can't get body from json.")?;
+
+                let signature = from_value(obj.get("signature").ok_or("Can't get signature from json")?.clone())?;
+                let message_type = from_value(obj.get("message_id").ok_or("Can't get message_type from json")?.clone())?;


ok , so now neither exonum::serialize::json::reexport::from_value nor exonum::serialize::json::from_value can be used on our macrogenerated types, because it results in stack overflow:

running 15 tests test tests::test_tx_create_wallet ... ok test tests::generate_simple_scenario_transactions ... ok test tests::test_tx_issue_serde ... FAILED test tests::test_tx_transfer_serde ... FAILED test tests::test_wallet_history_true_status ... ok test tests::test_wallet_history_txcreate_false_status ... ok test tests::test_tx_create_wallet_serde ... FAILED test tests::test_wallet_prefix ... ok test tx_metarecord::test_tx_meta_record ... ok test tests::test_wallet_history_txtransfer_false_status_insufficient_balance ... ok test tests::test_wallet_history_txtransfer_false_status_absent_receiver_wallet ... ok test wallet::test_amount_transfer ... ok test wallet::test_wallet ... ok thread 'tx_metarecord::test_tx_meta_record_serde' has overflowed its stack fatal runtime error: stack overflow error: process didn't exit successfully: `/Users/sysmanj/Exonum/cryptocurrency/target/debug/deps/cryptocurrency-99db04e147220907` (signal: 6, SIGABRT: process abort signal) To learn more, run the command again with --verbose.

I'll try to workaround this, shouldn't be a problem, because i don't need explicit calls of from_value, because you implemented service_id and message_id checks by default.

https://travis-ci.com/exonum/cryptocurrency/builds/46532155?utm_source=github_status&utm_medium=notification

dj8yfo · 2017-05-22T16:37:24Z

exonum/src/messages/spec.rs

+            {
+                use $crate::serialize::json::reexport::Error;
+                let value = <$crate::serialize::json::reexport::Value>::deserialize(deserializer)?;
+                $crate::serialize::json::reexport::from_value(value)


Look above. I assume it should be ExomunJsonDeserialize::deserialize_owned here instead of from_value, otherwise you're calling Deserialize till death.

dj8yfo · 2017-05-22T16:58:48Z

exonum/cryptocurrency#18

dj8yfo · 2017-05-22T16:59:11Z

exonum/exonum-configuration#24

Move comments inside macro declaration

dj8yfo · 2017-05-24T01:50:49Z

exonum/src/serialize/json.rs

+pub struct ExonumJsonSerializeWrapper<'a, T: ExonumJsonSerialize + 'a>( &'a T);
+
+
+#[derive(Serialize, Deserialize)]


This results in

"round": 4, "time": { "nanos": 804000000, "secs": 1486720350 }, "validator": 0 }, "message_id": 4, "network_id": 0,

this should be
"secs": "1486720350" for consistency.

defuz · 2017-05-24T13:10:15Z

@vldm Let's rename the methods deserialize and deserialize_owned. I propose to do so:

ExonumJsonDeserializeField::deserialize_field
ExonumJsonDeserialize::deserialize

Rationale:

This fits perfectly with the names of the traits.
ExonumJsonDeserialize method have a common signature for deserialization and ExonumJsonDeserializeField method signature are specific. It is logical that the specific method should be prefixed/suffixed and not vice versa.
Suffix _owned looks confusing, at least for me.

defuz · 2017-05-24T13:13:01Z

@vldm If this does not cause conflicts, run rustfmt before the merge.

dj8yfo

@defuz exonum/cryptocurrency#18
exonum/exonum-configuration#24.
May need some considerable time to parse it line by line for review.
@vldm took care of all my objections.
I may review later, after merge) At least it works, somehow.

defuz · 2017-05-25T00:16:10Z

🎉 🎉 🎉

@gisochre Anyway this is much better than it was before. Refactoring and line-by-line reviews can be done later. Especially since we are now waiting for #103.

Implement Serialize/Deserialize Former-commit-id: 1ab7034c61572e9b7fd2ae4dde67c6b0277ba43e

* Update clippy version (0.0.169) * Fix Clippy warnings

update exonum-client version

vldm mentioned this pull request Apr 26, 2017

Refactor message! and storage_value! #17

Closed

3 tasks

defuz reviewed Apr 27, 2017

View reviewed changes

vldm added API labels May 4, 2017

vldm requested review from dj8yfo, alekseysidorov and stanislav-tkach May 4, 2017 10:53

vldm changed the title ~~WIP: Implement Serialize/Deserialize~~ Implement Serialize/Deserialize May 4, 2017

stanislav-tkach previously approved these changes May 5, 2017

View reviewed changes

alekseysidorov reviewed May 5, 2017

View reviewed changes

vldm dismissed stanislav-tkach’s stale review via 58d65af May 8, 2017 10:32

vldm self-assigned this May 9, 2017

stanislav-tkach previously approved these changes May 14, 2017

View reviewed changes

vldm dismissed stanislav-tkach’s stale review via 4da0b39 May 15, 2017 07:45

stanislav-tkach previously approved these changes May 15, 2017

View reviewed changes

vldm dismissed stanislav-tkach’s stale review via 485342b May 15, 2017 10:21

stanislav-tkach previously approved these changes May 15, 2017

View reviewed changes

vldm mentioned this pull request May 15, 2017

Refactor storage_values! and message! #103

Merged

7 tasks

alekseysidorov self-requested a review May 18, 2017 14:40

alekseysidorov previously approved these changes May 18, 2017

View reviewed changes

dj8yfo added the priority high label May 18, 2017

dj8yfo mentioned this pull request May 18, 2017

Documenting consensus messages #48

Closed

dj8yfo suggested changes May 19, 2017

View reviewed changes

dj8yfo reviewed May 19, 2017

View reviewed changes

vldm added the breaking change ⚠️ API or ABI is changed. label May 19, 2017

vldm added this to the Release 0.1 milestone May 22, 2017

update to serde 0.9

07781ff

vldm dismissed stale reviews from alekseysidorov and stanislav-tkach via 388bfae May 22, 2017 08:42

vldm added 2 commits May 22, 2017 12:07

Add default commmon Ser\De realisation, fix message serialization.

a7c476a

Add network_id and version.

2277c14

stanislav-tkach mentioned this pull request May 22, 2017

Update code for new serialization/deserialization exonum/exonum-configuration#24

Merged

dj8yfo suggested changes May 22, 2017

View reviewed changes

dj8yfo reviewed May 22, 2017

View reviewed changes

dj8yfo suggested changes May 22, 2017

View reviewed changes

vldm and others added 6 commits May 23, 2017 11:37

Remove json api, use reexport one.

125395c

add doc attr to message, and storage_value

28c0adb

Merge branch 'master' into 17-refactor-message-1

eb025ed

Move comments inside macro declaration

89ecc7e

Merge pull request #1 from DarkEld3r/fix-comments-for-macro-messages

928ac14

Move comments inside macro declaration

fix sandbox

6e92574

dj8yfo suggested changes May 24, 2017

View reviewed changes

vldm added 3 commits May 24, 2017 23:10

refactor methods

ac10383

Merge branch 'master' into 17-refactor-message-1

60545be

fix SystemTime serialize, implement some tests.

1801a38

dj8yfo reviewed May 25, 2017

View reviewed changes

dj8yfo approved these changes May 25, 2017

View reviewed changes

defuz merged commit 8a11f36 into exonum:master May 25, 2017

defuz deleted the 17-refactor-message-1 branch May 25, 2017 00:16

vldm pushed a commit that referenced this pull request Jul 13, 2017

Merge pull request #71 from vldm/17-refactor-message-1

83fe69c

Implement Serialize/Deserialize Former-commit-id: 1ab7034c61572e9b7fd2ae4dde67c6b0277ba43e

stanislav-tkach added a commit to stanislav-tkach/exonum that referenced this pull request Feb 10, 2018

Update clippy version (0.0.169) (exonum#71)

4e815a3

* Update clippy version (0.0.169) * Fix Clippy warnings

stanislav-tkach pushed a commit to stanislav-tkach/exonum that referenced this pull request Apr 4, 2018

Merge pull request exonum#71 from boguslavsky/exonum-client-version

4f2604b

update exonum-client version

		pub struct ExonumJsonSerializeWrapper<'a, T: ExonumJsonSerialize + 'a>( &'a T);


		#[derive(Serialize, Deserialize)]

Implement Serialize/Deserialize #71

Implement Serialize/Deserialize #71

Conversation

vldm commented Apr 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vldm May 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vldm May 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vldm May 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stanislav-tkach left a comment

Choose a reason for hiding this comment

vldm commented May 15, 2017

dj8yfo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vldm May 22, 2017 • edited

Choose a reason for hiding this comment

dj8yfo left a comment

Choose a reason for hiding this comment

dj8yfo May 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dj8yfo commented May 22, 2017

dj8yfo commented May 22, 2017

Choose a reason for hiding this comment

defuz commented May 24, 2017 • edited

defuz commented May 24, 2017

dj8yfo left a comment

Choose a reason for hiding this comment

defuz commented May 25, 2017

vldm commented Apr 26, 2017 •

edited

vldm May 8, 2017 •

edited

vldm May 8, 2017 •

edited

vldm May 8, 2017 •

edited

vldm May 22, 2017 •

edited

dj8yfo May 22, 2017 •

edited

defuz commented May 24, 2017 •

edited