Add rust native implementation of product #99

adeebahmed · 2019-06-21T16:31:55Z

This PR contains rust native implementations of the data models and functions associated, in product

sdk/src/protocol/product/payload.rs

agunde406

This does not compile. You need to add pub mod product; to src/protocol/mod.rs and fix the compilation errors.

peterschwarz · 2019-06-27T18:01:54Z

Why didn't that compilation failure get caught by Jenkins?

agunde406 · 2019-06-27T18:03:09Z

Since product is not added the protocol mod.rs file the product protocols are not being included.

pa3ng · 2019-07-11T03:34:37Z

Looks like adding in the rust native implementations for grid product is causing some sections of the product smart contract code to error out; will update the product smart contract in this PR to fix the Jenkins build. Apologies for the large PR - didn't foresee this issue

pa3ng · 2019-07-12T15:07:32Z

This open issue is causing the Jenkins build to break: rust-lang/rust#62562

TL;DR: serde does not build with the latest version of nightly

eloaverona · 2019-07-12T16:17:29Z

@pa3ng It seems the issue was fixed in the newest nightly. I restarted your build in Jenkins and it passed :)

pa3ng · 2019-07-12T21:00:28Z

This does not compile. You need to add pub mod product; to src/protocol/mod.rs and fix the compilation errors.

@agunde406 ✅

contracts/product/src/handler.rs

pa3ng · 2019-07-17T19:51:57Z

After talking with @peterschwarz about not creating a central, shared test package to reduce test code redundancy, I plan on adding the smart contract unit tests in this PR instead of a separate PR. Let me know if this is desired or not.

TODO:

Add rust native implementations to product
Update product smart contract to use rust native implementations of product
Add product protocol unit tests
Incorporating other PR requests

peterschwarz · 2019-07-18T16:20:38Z

sdk/src/protocol/product/state.rs

+        }
+    }
+
+    fn cause(&self) -> Option<&StdError> {


cause is deprecated - you should use source for these: https://doc.rust-lang.org/std/error/trait.Error.html#method.source

peterschwarz · 2019-07-18T16:22:28Z

sdk/src/protocol/product/state.rs

+impl std::fmt::Display for ProductBuildError {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        match *self {
+            ProductBuildError::MissingField(ref s) => write!(f, "MissingField: {}", s),


Display message should be Human-readable, this is no better than just Debug. Something like "missing field \"{}\"" would be appropriate here. Or with the EmptyVec error: "\"{}\" must not be empty".

agunde406

Note the unit tests in the protocol files are still missing. There are two set of unit tests that still need to be added the protocol unit tests and the smart contract unit tests. The protocol unit tests should be add in this PR, while the smart contract unit tests could be added in a future PR.

sdk/protos/product_payload.proto

agunde406 · 2019-07-18T16:13:35Z

sdk/src/protocol/product/payload.rs

+            ),
+            ProductPayload_Action::UNSET_ACTION => {
+                return Err(ProtoConversionError::InvalidTypeError(
+                    "Cannot convert ProductPayload_Action with type unset.".to_string(),


Remove period at the end of the error

agunde406 · 2019-07-18T16:19:17Z

sdk/src/protocol/product/payload.rs

+    }
+}
+
+/// Builder used to create a ProductPayload


ProductPaylaod -> ProductCreateBuilder

agunde406 · 2019-07-18T16:20:44Z

sdk/src/protocol/product/payload.rs

+    pub fn build(self) -> Result<ProductCreateAction, BuilderError> {
+        let product_type = self
+            .product_type
+            .ok_or_else(|| BuilderError::MissingField("product_type".into()))?;


can you make these match the other errors " 'product_type' field is required" etc

agunde406 · 2019-07-18T16:21:43Z

sdk/src/protocol/product/payload.rs

+
+    pub fn build(self) -> Result<ProductUpdateAction, ProductUpdateBuildError> {
+        let product_type = self.product_type.ok_or_else(|| {
+            ProductUpdateBuildError::MissingField("'product_type field is required".to_string())


missing second ' after product_type

same in identifier

agunde406 · 2019-07-18T16:23:33Z

sdk/src/protocol/product/payload.rs

+
+    pub fn build(self) -> Result<ProductDeleteAction, ProductDeleteBuildError> {
+        let product_type = self.product_type.ok_or_else(|| {
+            ProductDeleteBuildError::MissingField("'product_type field is required".to_string())


Missing second ' after product type

same for identifier

agunde406 · 2019-07-18T16:24:49Z

sdk/protos/product_state.proto

@@ -26,7 +26,7 @@ message Product {
  string identifier = 1;

  // What type of product is this (GS1)
-  ProductType type = 2;
+  ProductType product_type = 2;


Can you leave a comment on the associated RFC mentioning this change?

contracts/product/src/handler.rs

agunde406 · 2019-07-23T17:53:07Z

sdk/src/protocol/product/payload.rs

+impl IntoProto<protos::product_payload::ProductUpdateAction> for ProductUpdateAction {}
+impl IntoNative<ProductUpdateAction> for protos::product_payload::ProductUpdateAction {}
+
+/// Builder used to create a ProductUpdateActionBuilder


Change to "Builder used to create a ProductUpdateAction"

agunde406 · 2019-07-23T17:53:36Z

sdk/src/protocol/product/payload.rs

+impl IntoProto<protos::product_payload::ProductDeleteAction> for ProductDeleteAction {}
+impl IntoNative<ProductDeleteAction> for protos::product_payload::ProductDeleteAction {}
+
+/// Builder used to create a ProductDeleteActionBuilder


Change to "Builder used to create a ProductDeleteAction"

sdk/src/protocol/product/payload.rs

sdk/src/protocol/product/state.rs

Other changes - Removed enum ProductType declarations in each Product action message (see protos/product_payload.proto) - Renamed 'product_values' field to 'properties' field for coherency (see protos/product_state.proto) - Renamed 'type' field to 'product_type' because 'type' is a reserved key word - Renamed 'identifier field to 'product_id' field because 'identifier' is a rust error descriptor - Replaced deprecated StdError 'cause' function with new 'source' function (see protocol/product/state.rs) Signed-off-by: Raphael Santo Domingo <raphael.santodomingo@target.com>

Other changes - Moved the 'check_permission' function call before checking for an agent's associated org - Renamed 'identifier field to 'product_id' field because 'identifier' is a rust error descriptor Signed-off-by: Raphael Santo Domingo <raphael.santodomingo@target.com>

adeebahmed · 2019-07-25T21:18:20Z

sdk/protos/product_state.proto

@@ -26,13 +26,13 @@ message Product {
  string identifier = 1;


We should refactor this to "product_id" -- Some error messages in rust use the word "identifier" to convey the meaning of a variables name i.e.

error: expected identifier, found keyword `type` --> src/actions/products.rs:31:9 | 31 | pub type: String, | ^^^^ expected identifier, found keyword help: you can escape reserved keywords to use them as identifiers | 31 | pub r#type: String, | ^^^^^^

It's not a reserved word, but best to avoid any chance of confusion.

✅ This change has already been made (see sdk/protos/product_state.proto)

adeebahmed requested review from agunde406, boydjohnson, annechenette, davececchi, dcmiddle, dplumb94, jsmitchell, peterschwarz, rbuysse, RyanLassigBanks and vaporos as code owners June 21, 2019 16:31

adeebahmed force-pushed the aahmed-product-protocol branch from 5b16eb1 to 0d8376e Compare June 21, 2019 16:33

adeebahmed changed the title ~~Add rust native implementation of protobufs~~ Add rust native implementation of product Jun 21, 2019

agunde406 suggested changes Jun 22, 2019

View reviewed changes

adeebahmed force-pushed the aahmed-product-protocol branch from 0d8376e to be60569 Compare June 24, 2019 20:43

agunde406 self-requested a review June 27, 2019 17:54

agunde406 suggested changes Jun 27, 2019

View reviewed changes

pa3ng deleted the aahmed-product-protocol branch July 1, 2019 17:21

pa3ng force-pushed the aahmed-product-protocol branch 6 times, most recently from beb928e to 7b0e197 Compare July 10, 2019 20:57

pa3ng force-pushed the aahmed-product-protocol branch 3 times, most recently from e7ccc8c to 8305ab4 Compare July 12, 2019 04:50

pa3ng force-pushed the aahmed-product-protocol branch from 8305ab4 to 6870729 Compare July 15, 2019 19:03

adeebahmed commented Jul 17, 2019

View reviewed changes

contracts/product/src/handler.rs Show resolved Hide resolved

adeebahmed commented Jul 17, 2019

View reviewed changes

contracts/product/src/handler.rs Show resolved Hide resolved

adeebahmed commented Jul 17, 2019

View reviewed changes

contracts/product/src/handler.rs Show resolved Hide resolved

pa3ng force-pushed the aahmed-product-protocol branch 2 times, most recently from 78b56cb to c128cb7 Compare July 17, 2019 19:47

agunde406 self-requested a review July 18, 2019 16:10

peterschwarz suggested changes Jul 18, 2019

View reviewed changes

agunde406 suggested changes Jul 18, 2019

View reviewed changes

agunde406 self-assigned this Jul 18, 2019

pa3ng force-pushed the aahmed-product-protocol branch 3 times, most recently from e5d96de to 7b6aa62 Compare July 19, 2019 20:46

peterschwarz requested a review from agunde406 July 19, 2019 21:18

pa3ng force-pushed the aahmed-product-protocol branch from 7b6aa62 to b85691d Compare July 19, 2019 21:30

agunde406 requested a review from peterschwarz July 23, 2019 17:48

agunde406 suggested changes Jul 23, 2019

View reviewed changes

pa3ng force-pushed the aahmed-product-protocol branch from b85691d to a639567 Compare July 23, 2019 19:13

agunde406 approved these changes Jul 23, 2019

View reviewed changes

Raphael Santo Domingo added 2 commits July 25, 2019 16:30

pa3ng force-pushed the aahmed-product-protocol branch from a639567 to d612486 Compare July 25, 2019 21:41

adeebahmed requested a review from rberg2 as a code owner July 25, 2019 21:41

adeebahmed commented Jul 26, 2019

View reviewed changes

peterschwarz approved these changes Jul 30, 2019

View reviewed changes

peterschwarz merged commit 10a6152 into hyperledger-archives:master Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rust native implementation of product #99

Add rust native implementation of product #99

adeebahmed commented Jun 21, 2019 •

edited

Loading

agunde406 left a comment

peterschwarz commented Jun 27, 2019

agunde406 commented Jun 27, 2019

pa3ng commented Jul 11, 2019 •

edited

Loading

pa3ng commented Jul 12, 2019 •

edited

Loading

eloaverona commented Jul 12, 2019

pa3ng commented Jul 12, 2019

pa3ng commented Jul 17, 2019 •

edited

Loading

peterschwarz Jul 18, 2019

pa3ng Jul 19, 2019

peterschwarz Jul 18, 2019

pa3ng Jul 19, 2019

agunde406 left a comment •

edited

Loading

agunde406 Jul 18, 2019

pa3ng Jul 19, 2019

agunde406 Jul 18, 2019

pa3ng Jul 19, 2019

agunde406 Jul 18, 2019

pa3ng Jul 19, 2019

agunde406 Jul 18, 2019

agunde406 Jul 18, 2019

pa3ng Jul 19, 2019

agunde406 Jul 18, 2019

agunde406 Jul 18, 2019

pa3ng Jul 19, 2019

agunde406 Jul 18, 2019

pa3ng Jul 19, 2019

agunde406 Jul 23, 2019

pa3ng Jul 23, 2019

agunde406 Jul 23, 2019

adeebahmed Jul 25, 2019

pa3ng Jul 27, 2019 •

edited

Loading

Add rust native implementation of product #99

Add rust native implementation of product #99

Conversation

adeebahmed commented Jun 21, 2019 • edited Loading

agunde406 left a comment

Choose a reason for hiding this comment

peterschwarz commented Jun 27, 2019

agunde406 commented Jun 27, 2019

pa3ng commented Jul 11, 2019 • edited Loading

pa3ng commented Jul 12, 2019 • edited Loading

eloaverona commented Jul 12, 2019

pa3ng commented Jul 12, 2019

pa3ng commented Jul 17, 2019 • edited Loading

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

agunde406 left a comment • edited Loading

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

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

Choose a reason for hiding this comment

pa3ng Jul 27, 2019 • edited Loading

Choose a reason for hiding this comment

adeebahmed commented Jun 21, 2019 •

edited

Loading

pa3ng commented Jul 11, 2019 •

edited

Loading

pa3ng commented Jul 12, 2019 •

edited

Loading

pa3ng commented Jul 17, 2019 •

edited

Loading

agunde406 left a comment •

edited

Loading

pa3ng Jul 27, 2019 •

edited

Loading