Extract ProtobufConvert derive to separate crate #1501

pavel-mukhanov · 2019-10-16T08:42:23Z

Derive macros ProtobufConvert is moved to separate crate.
Added derive macros BinaryValue and ObjectHash to exonum-derive.

Co-Authored-By: Oleksandr Anyshchenko <oleksandr.anyshchenko@xdev.re>

Co-Authored-By: Igor Aleksanov <popzxc@yandex.ru>

Co-Authored-By: Oleksandr Anyshchenko <oleksandr.anyshchenko@xdev.re>

…tract_proto_sep_crate

Co-Authored-By: Oleksandr Anyshchenko <oleksandr.anyshchenko@xdev.re>

popzxc · 2019-10-16T08:51:26Z

exonum/src/proto/mod.rs

@@ -65,8 +65,7 @@ impl ProtobufConvert for ValidatorId {
    fn from_pb(pb: Self::ProtoStruct) -> Result<Self, Error> {
        ensure!(
            pb <= u32::from(u16::max_value()),
-            "{} is out of range for valid ValidatorId",
-            pb
+            "u32 is out of range for valid ValidatorId"


Merge artifact, sorry

codecov · 2019-10-16T09:45:16Z

Codecov Report

Merging #1501 into dynamic_services will decrease coverage by <.01%.
The diff coverage is 95.5%.

@@                 Coverage Diff                  @@
##           dynamic_services    #1501      +/-   ##
====================================================
- Coverage              93.4%   93.39%   -0.01%     
====================================================
  Files                   198      198              
  Lines                 28381    28408      +27     
====================================================
+ Hits                  26510    26533      +23     
- Misses                 1871     1875       +4

Impacted Files	Coverage Δ
testkit/src/lib.rs	`94.27% <ø> (ø)`	⬆️
components/proto/src/lib.rs	`97.01% <ø> (ø)`	⬆️
...ples/cryptocurrency-advanced/backend/src/wallet.rs	`100% <100%> (ø)`	⬆️
testkit/tests/service_hooks/hooks.rs	`100% <100%> (ø)`	⬆️
testkit/src/simple_supervisor/proto/mod.rs	`100% <100%> (ø)`	⬆️
examples/timestamping/backend/src/schema.rs	`97.36% <100%> (ø)`	⬆️
exonum/src/runtime/rust/tests.rs	`95.45% <100%> (ø)`	⬆️
examples/timestamping/backend/src/transactions.rs	`90% <100%> (ø)`	⬆️
services/supervisor/src/proto_structures.rs	`91.66% <100%> (ø)`	⬆️
examples/cryptocurrency/src/lib.rs	`91.34% <100%> (ø)`	⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
P 8000 owered by Codecov. Last update 47114e3...f8629b6. Read the comment docs.

alekseysidorov

So we can replace
[exonum(pb = "")] by the [protobuf_convert(source = "")] or even [protobuf_convert(source = foo::path)]

alekseysidorov · 2019-10-16T12:47:34Z

examples/cryptocurrency-advanced/backend/src/transactions.rs

@@ -44,7 +45,7 @@ pub enum Error {
 }

 /// Transfer `amount` of the currency from one wallet to another.
-#[derive(Clone, Debug, ProtobufConvert)]
+#[derive(Clone, Debug, ProtobufConvert, BinaryValue, ObjectHash)]
 #[exonum(pb = "proto::Transfer", serde_pb_convert)]


I think we should avoid of using exonum for attribute name here, and also is there any need to have serde_pb_convert attribute that looks very strange and unclear.

aleksuss · 2019-10-17T15:09:12Z

exonum/src/lib.rs

@@ -55,6 +57,7 @@ extern crate log;
 extern crate serde_derive;
 #[macro_use]
 extern crate serde_json;
+extern crate exonum_proto;


aleksuss · 2019-10-17T15:10:52Z

CHANGELOG.md

+
+#### exonum-proto-derive
+
+- Introduced new crate `exonum-proto-derive`. Derive macro `ProtobufConvert` is


Suggested change

- Introduced new crate `exonum-proto-derive`. Derive macro `ProtobufConvert` is

- Introduced a new crate `exonum-proto-derive`. Derive macro `ProtobufConvert` is

aleksuss · 2019-10-17T15:11:36Z

components/derive/src/lib.rs

@@ -20,56 +20,58 @@

 extern crate proc_macro;


popzxc · 2019-10-18T06:00:03Z

components/proto-derive/src/pb_convert.rs

@@ -473,14 +429,13 @@ impl ToTokens for ProtobufConvert {
        };

        let expanded = quote! {
-            mod #mod_name {
+                mod #mod_name {


Why this padding was added?

vitvakatu · 2019-10-18T09:01:51Z

CHANGELOG.md

@@ -39,8 +39,17 @@ The project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html)

 #### exonum-proto

- Introduced new crate `exonum-proto`. Trait `ProtobufConvert` is moved to
-  `exonum-proto` crate. (#1496)
+- Introduced a new crate `exonum-proto`. Trait `ProtobufConvert` changed to is moved


slowli

Comments as of cb81870.

slowli · 2019-10-17T09:49:24Z

components/proto-derive/src/lib.rs

+/// Implement `serde::{Serialize, Deserialize}` using structs that were generated with
+/// protobuf.
+/// For example, it should be used if you want json representation of your struct
+/// to be compatible with protobuf representation (including proper nesting of fields).


Nit / off-topic: it's not clear what "compatible" means in this context. E.g., bytes should be base64-encoded according to Protobuf 3 spec, but the example below shows that it is serialized as an array.

components/proto-derive/src/lib.rs

slowli · 2019-10-17T09:50:36Z

components/proto-derive/src/lib.rs

+/// (de)serializer will be represented as
+/// StructName {
+///     "hash": {
+///         data: [1, 2, ...]


Nit: "data" should be quoted?

slowli · 2019-10-18T08:52:30Z

components/proto-derive/src/lib.rs

+
+#[derive(Debug, FromMeta, PartialEq, Eq)]
+#[darling(default)]
+struct CratePath(syn::Path);


Is this struct necessary?

slowli · 2019-10-18T08:58:07Z

components/proto-derive/src/lib.rs

+///     // ...
+/// }
+/// ```
+#[proc_macro_attribute]


Am I correct that protobuf_convert is not a derive macro? Why? It derives a specific trait, so a derive macro would be more appropriate. IMO, a proc macro attribute implies a deeper level of transform of the attached TokenStream.

slowli · 2019-10-18T08:59:14Z

examples/cryptocurrency-advanced/backend/Cargo.toml

@@ -20,6 +20,7 @@ exonum-derive = { version = "0.12.0", path = "../../../components/derive" }
 exonum-merkledb = { version = "0.12.0", path = "../../../components/merkledb" }
 exonum-crypto = { version = "0.12.0", path = "../../../components/crypto"}
 exonum-proto = { version = "0.12.0", path = "../../../components/proto" }
+exonum-proto-derive = { version = "0.12.0", path = "../../../components/proto-derive"}


Could it be possible to bundle exonum-proto-derive with exonum-proto as in structopt and recent versions of serde?

Bumping the request. This is not a serious concern; still, it improves UX.

slowli · 2019-10-18T09:04:43Z

components/proto-derive/src/lib.rs

+use quote::ToTokens;
+use syn::{Attribute, NestedMeta};
+
+/// ProtobufConvert attribute macros.


Nit: why plural (macros)? Also, an example or two would be great, especially for enums.

slowli

Looks mergeable as of ee261f0.

slowli · 2019-10-23T10:04:43Z

components/proto-derive/src/lib.rs

+/// #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, ProtobufConvert)]
+/// #[protobuf_convert(source = "schema::runtime::ConfigChange", oneof_field = "message")]
+/// pub enum ConfigChange {
+///    /// New consensus config.


Nit: bogus indentation (3 instead of 4).

slowli · 2019-10-23T10:04:58Z

components/proto-derive/src/lib.rs

+/// Corresponding proto file:
+/// ```text
+/// message Wallet {
+///  // Public key of the wallet owner.


Nit: bogus indentation (1).

slowli · 2019-10-23T10:05:46Z

components/proto-derive/src/lib.rs

+/// ```
+///
+/// Corresponding proto file:
+/// ```test


text? Also, maybe use proto for syntax highlighting?

slowli · 2019-10-23T10:06:03Z

components/proto-derive/src/lib.rs

+/// Corresponding proto file:
+/// ```test
+/// message ConfigChange {
+///  oneof message {


Nit: Bogus indentation (1).

slowli · 2019-10-23T10:08:11Z

components/proto-derive/src/lib.rs

+/// ```
+///
+/// This macro can also be applied to enums. In proto files enums are represented
+/// by `oneof` field. You can specify `oneof` field name, default is "message".


Do I understand correctly that:

The protobuf message must contain only this oneof field group (additional fields are not allowed)?

The Rust enum should have variants with a single unnamed field? (Or are zero-field variants allowed too?)

It could be worth specifying these details.

slowli · 2019-10-23T10:12:17Z

examples/cryptocurrency-advanced/backend/Cargo.toml

@@ -20,6 +20,7 @@ exonum-derive = { version = "0.12.0", path = "../../../components/derive" }
 exonum-merkledb = { version = "0.12.0", path = "../../../components/merkledb" }
 exonum-crypto = { version = "0.12.0", path = "../../../components/crypto"}
 exonum-proto = { version = "0.12.0", path = "../../../components/proto" }
+exonum-proto-derive = { version = "0.12.0", path = "../../../components/proto-derive"}


Bumping the request. This is not a serious concern; still, it improves UX.

pavel-mukhanov and others added 22 commits October 9, 2019 15:26

Move proto to separate crate.

6001f9a

Fix some comments.

27b9006

Update exonum/src/messages/tests.rs

52cde7a

Co-Authored-By: Oleksandr Anyshchenko <oleksandr.anyshchenko@xdev.re>

Use env variables in common and crypto.

3a98da2

Add protobuf generator.

e66eeb8

Fixes.

6c24b44

Fix build.

a9b57dc

Polish code.

7a33f1c

Fix lints.

f881a1c

Update components/merkledb/src/macros.rs

29d450c

Co-Authored-By: Igor Aleksanov <popzxc@yandex.ru>

Update components/build/src/lib.rs

cfc0143

Co-Authored-By: Oleksandr Anyshchenko <oleksandr.anyshchenko@xdev.re>

Fix review comments and add docs.

6915489

Merge remote-tracking branch 'origin/extract_proto_sep_crate' into ex…

79fef04

…tract_proto_sep_crate

Add changelog.

097808c

Update CHANGELOG.md

dedbb12

Co-Authored-By: Oleksandr Anyshchenko <oleksandr.anyshchenko@xdev.re>

Add BinaryValue and ObjectHash derive macros.

fc9e17b

WIP.

8e10f4f

WIP.

c6f0e6b

Polish code.

442fa16

Remove "crate" attribute from protobuf convert.

397dd63

Fix benches.

9a172a1

Merge upstream.

8102214

pavel-mukhanov requested review from alekseysidorov, aleksuss and popzxc October 16, 2019 08:42

pavel-mukhanov requested a review from slowli as a code owner October 16, 2019 08:42

Polish some code.

0b85f41

popzxc approved these changes Oct 16, 2019

View reviewed changes

Revert merge artifact.

cdaf6bd

Add doc and changelog.

c98dbdf

alekseysidorov reviewed Oct 16, 2019

View reviewed changes

aleksuss reviewed Oct 17, 2019

View reviewed changes

components/derive/src/lib.rs

@@ -20,56 +20,58 @@

extern crate proc_macro;

Copy link

Contributor

aleksuss Oct 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needless ?

popzxc reviewed Oct 18, 2019

View reviewed changes

pavel-mukhanov added 3 commits October 18, 2019 10:44

Change ProtobufConvert to attribure macro.

32b7614

Polish code.

efd8151

Polish code 2.

e9fa68c

popzxc approved these changes Oct 18, 2019

View reviewed changes

Fix typo.

cb81870

aleksuss approved these changes Oct 18, 2019

View reviewed changes

vitvakatu suggested changes Oct 18, 2019

View reviewed changes

slowli reviewed Oct 18, 2019

View reviewed changes

pavel-mukhanov added 3 commits October 18, 2019 15:54

Fix comments.

4d2a612

Make ProtobufConvert derive macro again.

c874497

Polish code.

6cb5df4

aleksuss requested a review from vitvakatu October 18, 2019 14:21

Fix typo.

ee261f0

slowli approved these changes Oct 23, 2019

View reviewed changes

pavel-mukhanov added 4 commits October 23, 2019 14:22

Bundle proto-derive with proto.

7433538

Fix doc in ProtobufConvert derive macro.

6707ebe

Merge branch 'dynamic_services' into extract-proto-derive

98D4 35bbe31

Fix doc tests.

f8629b6

aleksuss merged commit dd87b91 into exonum:dynamic_services Oct 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Extract ProtobufConvert derive to separate crate #1501

Extract ProtobufConvert derive to separate crate #1501

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!


		#### exonum-proto-derive

		- Introduced new crate `exonum-proto-derive`. Derive macro `ProtobufConvert` is

	- Introduced new crate `exonum-proto-derive`. Derive macro `ProtobufConvert` is
	- Introduced a new crate `exonum-proto-derive`. Derive macro `ProtobufConvert` is

Extract ProtobufConvert derive to separate crate #1501

Extract ProtobufConvert derive to separate crate #1501

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!