Common objects factory #1556

nickaleks · 2018-07-12T08:20:51Z

Description of the Change

This Pull Requests implements abstract factory for common objects in shared model.

There is an abstract factory as well as implementation in protobuf

Benefits

Can remove builders for common objects in the future

Possible Drawbacks

None

Alternate Designs [optional]

Builders are not needed since no fields are optional

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

This allows addition of validator in the future Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Akvinikym · 2018-07-12T09:24:07Z

shared_model/interfaces/common_objects/common_objects_factory.hpp

+namespace shared_model {
+  namespace interface {
+    /**
+     * CommonObjectsFactory provides methods to construct simple objects


What is "simple object"? Can you, please, define it in more specific way?

I guess "common_objects" is a better name here since they are in the folder with this name

Akvinikym · 2018-07-12T09:27:12Z

shared_model/interfaces/common_objects/common_objects_factory.hpp

+      /**
+       * Create amount instance from string
+       *
+       * @param value integer will be divided by 10 * precision,


Aren't this implementation issues, which should not be located in interface?

This is the description of the contract which factory provides.
It basically says that if you pass this precision, this is the final result you should get.

Akvinikym · 2018-07-12T09:29:30Z

shared_model/interfaces/common_objects/common_objects_factory.hpp

+       * For example: "1.23", "10" etc.
+       */
+      virtual FactoryResult<std::unique_ptr<Amount>> createAmount(
+          std::string amount) = 0;


Why not const reference?

because we modify this parameter in the code below. This logic was not written by me, and will be thrown out in the future, so it is simpler to just pass by value for now.

Akvinikym · 2018-07-12T09:30:25Z

shared_model/interfaces/common_objects/common_objects_factory.hpp

+  }  // namespace interface
+}  // namespace shared_model
+
+#endif  // IROHA_COMMONOBJECTSFACTORY_HPP


No empty line?

It is present

Akvinikym · 2018-07-12T09:30:59Z

test/module/shared_model/backend_proto/CMakeLists.txt

+target_link_libraries(proto_common_objects_factory_test
+    shared_model_cryptography
+    shared_model_stateless_validation
+    schema)


Bracket on a new line?

Akvinikym · 2018-07-12T09:33:47Z

shared_model/backend/protobuf/common_objects/proto_common_objects_factory.hpp

+      }
+
+      Validator validator_;
+    };


There is an empty space between curly braces in the beginning of the file, but not here

Akvinikym · 2018-07-12T09:41:04Z

shared_model/backend/protobuf/common_objects/proto_common_objects_factory.hpp

+              validator_.validatePeer(reasons, peer);
+            });
+
+        if (answer) {


Doesn't look very readable. Maybe, you should rename the variable to errors? It represents something wrong happened only, so I think it's a reasonable name

If you choose to follow the advice, change names of all variables in below functions as well

Answer can be empty, which indicates lack of errors. But i agree that errors is a better name here

Akvinikym · 2018-07-12T09:50:13Z

shared_model/backend/protobuf/common_objects/proto_common_objects_factory.hpp

+        // create uint256 value from obtained string
+        boost::multiprecision::uint256_t value = 0;
+        if (begin <= amount.size()) {
+          value = boost::multiprecision::uint256_t(amount.substr(begin));


Isn't overflow possible anywhere? Of course, I don't think someone's going to provide more than uint256, but still, for safety reasons, is it possible to handle such kind of error?

This code is taken from lib/amount. It has worked so far, and I do not want to touch it :). It will be changed in the future, then we can replace it here

Akvinikym · 2018-07-12T09:52:22Z

shared_model/backend/protobuf/common_objects/proto_common_objects_factory.hpp

@@ -0,0 +1,259 @@
+/**


The whole file is implantation inside an .hpp, won't is cause troubles with recompiling etc? Isn't it better to split it?

It will only be used in setup of storage, so its fine.

Akvinikym · 2018-07-12T09:56:35Z

test/module/shared_model/backend_proto/common_objects/proto_common_objects_factory_test.cpp

+TEST_F(PeerTest, InvalidPeerInitialization) {
+  auto peer = factory.createPeer(invalid_address, valid_pubkey);
+
+  peer.match(


Can't it be substituted with just ASSERT_TRUE(err(peer));?

Again, if you decide to follow, change it everywhere downwards as well

Issue with err() is that it copies return result, but we have unique_ptr here.

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

l4l · 2018-07-12T15:33:14Z

shared_model/backend/protobuf/common_objects/proto_common_objects_factory.hpp

+
+        auto errors = validate(*proto_amount, [](const auto &, auto &) {
+          // no validation needed,
+          // since any amount is valid in general context


There's still validateAmount function, so please use it

l4l · 2018-07-12T15:50:19Z

test/module/shared_model/backend_proto/common_objects/proto_common_objects_factory_test.cpp

+ */
+
+#include <gtest/gtest.h>
+#include <ed25519/ed25519.h>


I don't think that the header should be here

l4l · 2018-07-12T15:51:59Z

test/module/shared_model/backend_proto/common_objects/proto_common_objects_factory_test.cpp

+#include <gtest/gtest.h>
+#include <ed25519/ed25519.h>
+
+#include "backend/protobuf/common_objects/amount.hpp"


That header should probably be in common_objects/common_objects_factory.hpp instead

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

l4l

lgtm

…ects_factory Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/module/shared_model/backend_proto/CMakeLists.txt

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/module/shared_model/backend_proto/CMakeLists.txt

…ects_factory

Allows to polymorphically create common objects Signed-off-by: Nikita Alekseev <[email protected]>

nickaleks added 9 commits July 9, 2018 18:41

Implement Abstract factory for peer

43d7902

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Factory returns Result instead of just pointer

f7eabe8

This allows addition of validator in the future Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add validation logic to factory

e904964

This allows addition of validator in the future Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add account creation to factory

781cb39

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add account asset creation to factory

961fe11

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add amount creation to factory

a75f7fc

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add asset and domain creation to factory

48b7605

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add signature creation to factory

34a11ab

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add comments

2f30e41

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

nickaleks requested review from l4l and Akvinikym July 12, 2018 08:21

Akvinikym suggested changes Jul 12, 2018

View reviewed changes

Fix review issues

e40cb10

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

nickaleks force-pushed the feature/common_objects_factory branch from e21faae to e40cb10 Compare July 12, 2018 10:29

Akvinikym approved these changes Jul 12, 2018

View reviewed changes

l4l reviewed Jul 12, 2018

View reviewed changes

Fix review issues

2770bf5

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

l4l approved these changes Jul 16, 2018

View reviewed changes

nickaleks added 2 commits July 17, 2018 12:52

Merge remote-tracking branch 'origin/develop' into feature/common_obj…

d0ee0c6

…ects_factory Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/module/shared_model/backend_proto/CMakeLists.txt

Remove amount from factories

653b982

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # test/module/shared_model/backend_proto/CMakeLists.txt

nickaleks force-pushed the feature/common_objects_factory branch from 55742bb to 653b982 Compare July 17, 2018 10:51

Merge remote-tracking branch 'origin/develop' into feature/common_obj…

80f08a0

…ects_factory

nickaleks merged commit 359c8a7 into develop Jul 17, 2018

l4l deleted the feature/common_objects_factory branch July 17, 2018 20:34

neewy mentioned this pull request Jul 25, 2018

Hyperledger Iroha v1.0 beta-4 version #1597

Merged

l4l pushed a commit that referenced this pull request Jul 25, 2018

Common objects factory (#1556)

0550c28

Allows to polymorphically create common objects Signed-off-by: Nikita Alekseev <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Common objects factory (#1556)

362c502

Allows to polymorphically create common objects Signed-off-by: Nikita Alekseev <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Common objects factory (#1556)

2af32a9

Allows to polymorphically create common objects Signed-off-by: Nikita Alekseev <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Common objects factory (#1556)

e6c31d1

Allows to polymorphically create common objects Signed-off-by: Nikita Alekseev <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Common objects factory (#1556)

63e45b0

Allows to polymorphically create common objects Signed-off-by: Nikita Alekseev <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Common objects factory (#1556)

3867faa

Allows to polymorphically create common objects Signed-off-by: Nikita Alekseev <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Common objects factory (#1556)

a4950d4

Allows to polymorphically create common objects Signed-off-by: Nikita Alekseev <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common objects factory #1556

Common objects factory #1556

nickaleks commented Jul 12, 2018

Akvinikym Jul 12, 2018

nickaleks Jul 12, 2018

Akvinikym Jul 12, 2018

nickaleks Jul 12, 2018

Akvinikym Jul 12, 2018

nickaleks Jul 12, 2018

Akvinikym Jul 12, 2018

nickaleks Jul 12, 2018

Akvinikym Jul 12, 2018

Akvinikym Jul 12, 2018

Akvinikym Jul 12, 2018

Akvinikym Jul 12, 2018

nickaleks Jul 12, 2018

Akvinikym Jul 12, 2018

nickaleks Jul 12, 2018

Akvinikym Jul 12, 2018

nickaleks Jul 12, 2018

Akvinikym Jul 12, 2018

Akvinikym Jul 12, 2018

nickaleks Jul 12, 2018

l4l Jul 12, 2018

l4l Jul 12, 2018

l4l Jul 12, 2018

l4l left a comment

Common objects factory #1556

Common objects factory #1556

Conversation

nickaleks commented Jul 12, 2018

Description of the Change

Benefits

Possible Drawbacks

Alternate Designs [optional]

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

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

l4l left a comment

Choose a reason for hiding this comment