Add batch parser and transport factory #1750

lebdron · 2018-10-01T17:30:58Z

Description of the Change

Remove transaction sequence usage from irohad, introduce

TransactionBatchParser, which splits a list of transactions by possible adjacent batches
TransportFactory interface and protobuf implementation, which allows to mock the deserialization step in tests
One boost header was copied with slight change to allow moving non-copyable parameters in ranges:

Before:

    template<typename Arg>
    R operator()(const Arg& arg) const
    {
        BOOST_ASSERT(m_impl);
        return (*m_impl)(arg);
    }
    template<typename Arg>
    R operator()(Arg& arg) const
    {
        BOOST_ASSERT(m_impl);
        return (*m_impl)(arg);
    }

After:

      template <typename Arg>
      R operator()(Arg &&arg) const {
        BOOST_ASSERT(m_impl);
        return (*m_impl)(std::forward<Arg>(arg));
      }

Benefits

Less hardcoded dependencies

Possible Drawbacks

Code duplication in command service and mst transport, which will hopefully be resolved in another task

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

igor-egorov

Thanks!

kamilsa · 2018-10-02T13:40:12Z

irohad/torii/impl/command_service_transport_grpc.cpp

+                  return false;
+                });
+          })
+        | boost::adaptors::transformed([&](auto result) {


Why not const ref?

Because result contains a unique pointer, which should be moved.

kamilsa · 2018-10-02T13:56:18Z

irohad/multi_sig_transactions/transport/impl/mst_transport_grpc.cpp

+
+  for (auto &batch : batches) {
+    shared_model::interface::TransactionBatchFactory::createTransactionBatch(
+        batch, shared_model::validation::DefaultUnsignedTransactionsValidator())


Is it true, that we validate transactions twice? At first on line 38 and then on this line insude TransactionsValidator?

That will be fixed when hyperledger/iroha#1743 is introduced. Batch factory will only validate batch fields and the order.

kamilsa · 2018-10-02T13:58:22Z

irohad/torii/impl/command_service_transport_grpc.cpp

+    auto transactions = boost::copy_range<
+        shared_model::interface::types::SharedTxsCollectionType>(
+        request->transactions()
+        | boost::adaptors::transformed(


Consider putting this into separate function. Method Torii becomes very large

Same for mst_transport then

kamilsa · 2018-10-02T14:08:58Z

shared_model/interfaces/iroha_internal/abstract_transport_factory.hpp

+namespace shared_model {
+  namespace interface {
+
+    template <typename Interface, typename Transport>


Please drop few comments describing this interface and template parameters

kamilsa · 2018-10-02T14:09:34Z

shared_model/interfaces/iroha_internal/abstract_transport_factory.hpp

+      };
+
+      virtual iroha::expected::Result<std::unique_ptr<Interface>, Error> build(
+          Transport transport) const = 0;


Why not to take const ref of transport?

That will require to copy the argument regardless of the context. Value allows moving the argument if required.

kamilsa · 2018-10-02T14:17:25Z

test/module/shared_model/validators/validators.hpp

 #include "validators/abstract_validator.hpp"
-#include "validators/answer.hpp"
+
+#include <gmock/gmock.h>


Third party libraries should be above ours

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

Use standard order for readability and to avoid hidden dependencies: Related header, C library, C++ library, other libraries' .h, your project's .h.

abstract_validator.hpp is the related header for this file.

kamilsa · 2018-10-02T14:27:36Z

shared_model/interfaces/iroha_internal/transaction_batch_parser_impl.cpp

+
+namespace {
+  template <typename InRange, typename OutRange>
+  auto parseBatchesImpl(InRange in_range, const OutRange &out_range) {


Hard to read... What are these parameters? Maybe leave some comments here as well?

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

Issues addressed

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

Akvinikym · 2018-10-05T13:33:50Z

irohad/main/application.cpp

@@ -149,8 +152,10 @@ void Irohad::initCryptoProvider() {
 void Irohad::initValidators() {
  auto factory = std::make_unique<shared_model::proto::ProtoProposalFactory<
      shared_model::validation::DefaultProposalValidator>>();
+  batch_parser =
+      std::make_shared<shared_model::interface::TransactionBatchParserImpl>();


Are you sure this initValidators is a good place to initialize a parser?

Akvinikym · 2018-10-05T13:36:04Z

irohad/main/application.cpp

+  std::unique_ptr<shared_model::validation::AbstractValidator<
+      shared_model::interface::Transaction>>
+      transaction_validator = std::make_unique<
+          shared_model::validation::DefaultUnsignedTransactionValidator>();


Is that clang-format-ted?

Akvinikym · 2018-10-05T13:54:59Z

irohad/multi_sig_transactions/transport/impl/mst_transport_grpc.cpp

@@ -3,70 +3,87 @@
 * SPDX-License-Identifier: Apache-2.0
 */

+#include "common/default_constructible_unary_fn.hpp"  // non-copyable value workaround


According to what part of codestyle this header is at the top? Just asking

Because it has to be included before any boost includes, so that it does not include the header from boost library. This is not a part of codestyle.

Akvinikym · 2018-10-05T13:56:47Z

irohad/multi_sig_transactions/transport/impl/mst_transport_grpc.cpp

+
+shared_model::interface::types::SharedTxsCollectionType
+MstTransportGrpc::deserializeTransactions(
+    const ::iroha::network::transport::MstState *request) {


Is the extra scope resolution :: necessary here?

Akvinikym · 2018-10-05T13:59:01Z

irohad/multi_sig_transactions/transport/impl/mst_transport_grpc.cpp

+            [&](const auto &tx) { return transaction_factory_->build(tx); })
+      | boost::adaptors::filtered([&](const auto &result) {
+          return result.match(
+              [](const iroha::expected::Value<


Won't const auto & work here?

It will, but is it necessary? When a reader will see auto here, their though will be "OK, this branch might be either value or error", and only later they will see the second branch and deduce the previous branch.

Akvinikym · 2018-10-05T14:25:17Z

libs/common/default_constructible_unary_fn.hpp

+//  http://www.boost.org/LICENSE_1_0.txt)
+//
+// For more information, see http://www.boost.org/libs/range/
+//


Are all copyright issues kept? According to that license, is it permitted to use the file in commercial goals?

Since we include other boost headers which contain the same licence, it should be fine.

Akvinikym · 2018-10-05T14:29:30Z

shared_model/interfaces/iroha_internal/transaction_batch_parser.hpp

+
+      virtual ~TransactionBatchParser() = default;
+    };
+  }  // namespace interface


New line, please

https://github.com/hyperledger/iroha/blob/6e49d70c2e17f20a612bce1b334a91d83f5be4b3/shared_model/interfaces/iroha_internal/transaction_batch_parser.hpp#L30-L33

Akvinikym · 2018-10-05T14:29:51Z

shared_model/interfaces/iroha_internal/transaction_batch_parser.hpp

+  namespace interface {
+
+    /**
+     * Parses a transaction list and return a list of possible batches


...and returns a list...

Akvinikym · 2018-10-05T14:31:05Z

shared_model/interfaces/iroha_internal/transaction_batch_parser.hpp

+     */
+    class TransactionBatchParser {
+     public:
+      virtual std::vector<types::TransactionsForwardCollectionType>


Why have you decided not to put any docs about hose methods?

They all match the description of the class, and argument types describe the differences.

Akvinikym · 2018-10-05T14:33:37Z

shared_model/interfaces/iroha_internal/transaction_batch_parser_impl.cpp

+  auto parseBatchesImpl(InRange in_range, const OutRange &out_range) {
+    std::vector<OutRange> result;
+    auto meta = [](const auto &tx) { return boost::get<0>(tx).batchMeta(); };
+    auto has_meta = [&](const auto &tx) { return static_cast<bool>(meta(tx)); };


Maybe [&](const auto &tx) -> bool { will work? More readable than a static_case

This error was fixed previously hyperledger/iroha@3aad956

error: no viable conversion from 'boost::optional<std::shared_ptr<BatchMeta> >' to 'bool' begin_has_meta = boost::get<0>(*begin).batchMeta(); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

Issues addressed

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

lebdron added 2 commits October 1, 2018 20:23

Add batch parser and transport factory

d3f2d11

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

Merge branch 'dev' into refactor/tx-factory-batch-parser

5ea1832

lebdron requested review from muratovv and kamilsa October 1, 2018 17:30

lebdron added 3 commits October 2, 2018 10:08

Explicit bool cast for optional

3aad956

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

Fix types

4ffc645

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

Fix codacy issues

9ae6681

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

lebdron requested review from igor-egorov and removed request for muratovv October 2, 2018 13:15

igor-egorov approved these changes Oct 2, 2018

View reviewed changes

kamilsa previously requested changes Oct 2, 2018

View reviewed changes

l4l added the needs-review pr awaits review from maintainers label Oct 3, 2018

lebdron added 2 commits October 4, 2018 08:38

Fix review issues

521c14d

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

Merge branch 'dev' into refactor/tx-factory-batch-parser

cdcf202

lebdron requested a review from kamilsa October 4, 2018 05:42

Fix include

2ecdd67

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

Akvinikym previously requested changes Oct 5, 2018

View reviewed changes

Merge branch 'dev' into refactor/tx-factory-batch-parser

954bee3

kamilsa approved these changes Oct 6, 2018

View reviewed changes

Integrate batch factory

6e49d70

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

lebdron requested a review from Akvinikym October 8, 2018 07:59

Akvinikym approved these changes Oct 8, 2018

View reviewed changes

lebdron merged commit 5ac40fa into dev Oct 8, 2018

lebdron deleted the refactor/tx-factory-batch-parser branch October 8, 2018 09:01

kamilsa mentioned this pull request Nov 1, 2018

Hyperledger Iroha v1.0 beta-5 version #1825

Closed

bakhtin pushed a commit that referenced this pull request Nov 2, 2018

Add batch parser and transport factory (#1750)

d6f6a9d

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

neewy mentioned this pull request Nov 2, 2018

Hyperledger Iroha v1.0 beta-5 version #1832

Closed

kamilsa mentioned this pull request Dec 12, 2018

Hyperledger Iroha v1.0 Release Candidate 1 #1941

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add batch parser and transport factory #1750

Add batch parser and transport factory #1750

lebdron commented Oct 1, 2018 •

edited

Loading

igor-egorov left a comment

kamilsa Oct 2, 2018

lebdron Oct 2, 2018

kamilsa Oct 2, 2018

lebdron Oct 2, 2018

kamilsa Oct 2, 2018

lebdron Oct 2, 2018

kamilsa Oct 2, 2018

kamilsa Oct 2, 2018

lebdron Oct 2, 2018

kamilsa Oct 2, 2018

lebdron Oct 2, 2018

kamilsa Oct 2, 2018

Akvinikym Oct 5, 2018

Akvinikym Oct 5, 2018

lebdron Oct 5, 2018

Akvinikym Oct 5, 2018

lebdron Oct 5, 2018

Akvinikym Oct 5, 2018

Akvinikym Oct 5, 2018

lebdron Oct 5, 2018

Akvinikym Oct 5, 2018

lebdron Oct 5, 2018

Akvinikym Oct 5, 2018

lebdron Oct 8, 2018

Akvinikym Oct 5, 2018

Akvinikym Oct 5, 2018

lebdron Oct 5, 2018

Akvinikym Oct 5, 2018

lebdron Oct 5, 2018

Add batch parser and transport factory #1750

Add batch parser and transport factory #1750

Conversation

lebdron commented Oct 1, 2018 • edited Loading

Description of the Change

Benefits

Possible Drawbacks

igor-egorov 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebdron commented Oct 1, 2018 •

edited

Loading