Integration tests for GetPendingTransactions() #1661

Akvinikym · 2018-08-21T07:47:54Z

Description of the Change

This PR does two things:

Fixes MST - before it did not send batches, which were inserted or updated with new signatures, to the pending storage, which lead to inability to retrieve them
Implements a couple of tests for getPendingTransactions query

Benefits

Tests for a new query.

Possible Drawbacks

None.

Usage of tests

All tests, which are added, are located inside test/integration/pipeline/multisig_tx_pipeline_test.cpp. They can be executed by building and running the corresponding target, multisig_tx_pipeline_test

Signed-off-by: Akvinikym <[email protected]>

igor-egorov

the build is broken on both platforms

igor-egorov

Since there are notable changes in MST part, I request the review from @muratovv .

igor-egorov · 2018-09-01T09:49:09Z

irohad/multi_sig_transactions/impl/mst_processor_impl.cpp

@@ -32,6 +32,13 @@ namespace iroha {
                    });
    }
  }
+  void shareState(
+      ConstRefState state,
+      rxcpp::subjects::subject<std::shared_ptr<MstState>> &subject) {


const ... & subject?

Compile error, if you try to do it :(

igor-egorov · 2018-09-01T09:52:07Z

test/integration/pipeline/multisig_tx_pipeline_test.cpp

@@ -120,3 +140,102 @@ TEST_F(MstPipelineTest, OnePeerSendsTest) {
        ASSERT_EQ(proposal->transactions().size(), 1);
      });
 }
+
+/**
+ * @given ledger with pending transactions


not clear the ledger can be given with pending transactions

igor-egorov · 2018-09-01T09:52:43Z

test/integration/pipeline/multisig_tx_pipeline_test.cpp

+
+/**
+ * @given ledger with pending transactions
+ * @when get pending transactions is executed by peer, which is to sign those


a user is not a peer

igor-egorov · 2018-09-01T09:56:03Z

test/integration/pipeline/multisig_tx_pipeline_test.cpp

+ * @when get pending transactions is executed by peer, which is to sign those
+ * transactions
+ * @then those transactions is returned
+ */


given a user that has sent a semi-signed transaction to a ledger
when the user requests pending transactions
then user's semi-signed transaction is returned

igor-egorov · 2018-09-01T09:58:41Z

test/integration/pipeline/multisig_tx_pipeline_test.cpp

+                        .setAccountDetail(kUserId, "fav_meme", "doge")
+                        .quorum(kSignatories + 1);
+
+  // prepare itf


Maybe move preparation steps to some fixture method that returns a reference to an initialized instance of ITF?
An ITF can be stored inside boost::optional fixture field.

Signed-off-by: Akvinikym <[email protected]>

muratovv · 2018-09-06T07:44:45Z

irohad/multi_sig_transactions/impl/mst_processor_impl.cpp

@@ -32,6 +32,13 @@ namespace iroha {
                    });
    }
  }
+  void shareState(


Add documentation for the function and maybe rename it or previous shareState. Now it isn't obvious that they are doing the not same job.

muratovv · 2018-09-06T07:45:27Z

irohad/multi_sig_transactions/impl/mst_processor_impl.cpp

@@ -32,6 +32,13 @@ namespace iroha {
                    });
    }
  }
+  void shareState(
+      ConstRefState state,
+      rxcpp::subjects::subject<std::shared_ptr<MstState>> &subject) {


Use alias for DataType instead

muratovv · 2018-09-06T07:52:45Z

irohad/multi_sig_transactions/impl/mst_processor_impl.cpp

@@ -56,7 +63,12 @@ namespace iroha {

  auto FairMstProcessor::propagateBatchImpl(const iroha::DataType &batch)
      -> decltype(propagateBatch(batch)) {
-    shareState(storage_->updateOwnState(batch), batches_subject_);
+    auto state_update = storage_->updateOwnState(batch);


I don't see a reason to introduce pair with bool response here. Method isEmpty is already implemented in MstState.
Also, the current interface of updateOwnState is totally misleading - for one situation it returns one value, for another situation - another value.

UpdateOwnState possibly may return pair of two states: state with completed transactions and state with new signatures.

AFAIK the second parameter reflects the state of a batch - completed or just updated.
Nevertheless, the current implementation with std::pair is unclear and misleading.
My vote is for the simplicity.

igor-egorov

I am approving, but the mentioned unclarities should be fixed. Also, the type StateAndCompleteStatus looks redundant. Please consider removing it and making the code simpler.

igor-egorov · 2018-09-06T08:05:20Z

irohad/multi_sig_transactions/state/mst_state.hpp

@@ -92,7 +92,7 @@ namespace iroha {
     * @param rhs - batch for insertion
     * @return State with completed batches
     */
-    MstState operator+=(const DataType &rhs);
+    StateAndCompleteStatus operator+=(const DataType &rhs);


Method's docstring (at least return part) is to be updated.

igor-egorov · 2018-09-06T08:07:07Z

irohad/multi_sig_transactions/state/mst_state.hpp

@@ -166,8 +166,9 @@ namespace iroha {
     * Insert batch in own state and push it in out_state if required
     * @param out_state - state for inserting completed batches
     * @param rhs_tx - batch for insert
+     * @return if batch is complete after that insertion


Since the method returns bool instead of void, it cannot just "return if". Probably, true or false is missing.

igor-egorov · 2018-09-06T08:09:58Z

test/module/irohad/multi_sig_transactions/state_test.cpp

@@ -13,6 +13,9 @@ using namespace std;
 using namespace iroha;
 using namespace iroha::model;

+// state with tx with A sig, receive with B, only B should be shared among
+// others


Seems that this floating comment was forgotten to be removed.
Otherwise, it is not clear what is the thing it relates to.

igor-egorov

Please fix my previous review comments.
Please edit pr description and fill in the Usage examples and Tests section. At least the name of test target should be mentioned. Thanks.

Signed-off-by: Akvinikym <[email protected]>

Fixed

Signed-off-by: Akvinikym <[email protected]>

muratovv · 2018-09-10T08:18:21Z

irohad/multi_sig_transactions/state/mst_state.hpp

     */
-    void insertOne(MstState &out_state, const DataType &rhs_tx);
+    bool insertOne(MstState &out_state, const DataType &rhs_tx);


Seems, that we can avoid this bool with two passed states

muratovv · 2018-09-10T08:19:58Z

irohad/multi_sig_transactions/storage/mst_storage.hpp

        const std::shared_ptr<shared_model::interface::Peer> &target_peer,
        const MstState &new_state);

    /**
     * Provide updating state of current peer with new transaction
     * @param tx - new transaction for insertion in state
-     * @return State with completed transaction
+     * @return completed or updated mst state


muratovv · 2018-09-10T08:22:42Z

test/integration/pipeline/multisig_tx_pipeline_test.cpp

+}
+
+/**
+ * @given a ledger with pending transactions


Seems that ledger initially empty or at least doesn't contains pending transactions.

Signed-off-by: Akvinikym <[email protected]>

Fixed

lebdron · 2018-09-10T07:05:35Z

irohad/multi_sig_transactions/impl/mst_processor_impl.cpp

@@ -77,6 +66,35 @@ namespace iroha {
    return expired_subject_.get_observable();
  }

+  void FairMstProcessor::completedBatchesNotify(ConstRefState state) const {


completedBatchesNotify and expiredBatchesNotify look identical, the only difference is the subject used. Maybe make a single method with two parameters?

lebdron · 2018-09-10T07:06:07Z

irohad/multi_sig_transactions/impl/mst_processor_impl.cpp


    // expired batches
-    auto expired_batches = storage_->getDiffState(from, current_time);
-    shareState(expired_batches, this->expired_subject_);
+    completedBatchesNotify(storage_->getDiffState(from, current_time));


-> expiredBatchesNotify
Please verify that this is covered by tests.

lebdron · 2018-09-10T07:11:28Z

irohad/multi_sig_transactions/impl/mst_processor_impl.cpp

-        std::make_shared<MstState>(storage_->whatsNew(new_state));
-    state_subject_.get_subscriber().on_next(new_batches);
+    auto new_batches = storage_->whatsNew(new_state);
+    updatedBatchesNotify(new_batches);


It does not look consistent with https://github.com/hyperledger/iroha/blob/7ebc34ed3379a1a99253d453ac18f823b5701402/irohad/multi_sig_transactions/impl/mst_processor_impl.cpp#L48-L49
Maybe refactor it, so that updated_state_ is used?

lebdron · 2018-09-10T11:42:19Z

irohad/multi_sig_transactions/state/impl/mst_state.cpp

+  StateUpdateResult MstState::operator+=(const DataType &rhs) {
+    auto completed_state = MstState::empty(completer_);
+    auto updated_state = MstState::empty(completer_);
+    insertOne(completed_state, updated_state, rhs);


It is quite easy to accidently swap completed state and updated state when calling insertOne. Maybe pass StateUpdateResult instead?

test/module/irohad/multi_sig_transactions/mst_processor_test.cpp

Signed-off-by: Akvinikym <[email protected]>

sorabot · 2018-09-10T18:25:41Z

SonarQube analysis reported 3 issues

mst_processor_impl.hpp#L84: Unused private function: 'FairMstProcessor::completedBatchesNotify'
mst_processor_impl.hpp#L91: Unused private function: 'FairMstProcessor::updatedBatchesNotify'
mst_processor_impl.hpp#L97: Unused private function: 'FairMstProcessor::expiredBatchesNotify'

Signed-off-by: Akvinikym <[email protected]>

Akvinikym added 3 commits August 17, 2018 11:31

Tests descriptions are added

f74ad93

Signed-off-by: Akvinikym <[email protected]>

Merge branch 'develop' into feature/itf-get-pending-txs

26f8cc8

Created tests

f146a18

Signed-off-by: Akvinikym <[email protected]>

Akvinikym requested review from lebdron and igor-egorov August 21, 2018 07:47

l4l added needs-review pr awaits review from maintainers tests pr aimed at the code coverage increase or test refactorings query All that relates to the iroha querying labels Aug 27, 2018

Akvinikym added 6 commits August 29, 2018 16:15

Merge branch 'dev' into feature/itf-get-pending-txs

276a800

Merge branch 'dev' into feature/itf-get-pending-txs

caf1720

Merge branch 'dev' into feature/itf-get-pending-txs

fcb3d45

Continue

416e471

Signed-off-by: Akvinikym <[email protected]>

Comments for updates are added

bc9de97

Signed-off-by: Akvinikym <[email protected]>

Fixed more bugs and completed tests

eb2436e

Signed-off-by: Akvinikym <[email protected]>

Akvinikym changed the base branch from develop to dev August 31, 2018 12:31

igor-egorov reviewed Sep 1, 2018

View reviewed changes

igor-egorov force-pushed the feature/itf-get-pending-txs branch from 719d6d4 to eb2436e Compare September 1, 2018 09:43

igor-egorov requested a review from muratovv September 1, 2018 09:48

igor-egorov reviewed Sep 1, 2018

View reviewed changes

Akvinikym added 4 commits September 3, 2018 13:23

Fixed more

68922ee

Signed-off-by: Akvinikym <[email protected]>

Fixed build and review issues

e8afaa3

Signed-off-by: Akvinikym <[email protected]>

Fixed

db5cc74

Signed-off-by: Akvinikym <[email protected]>

Fixed everything I hope

1916e8c

Signed-off-by: Akvinikym <[email protected]>

Akvinikym requested a review from igor-egorov September 5, 2018 14:41

muratovv previously requested changes Sep 6, 2018

View reviewed changes

igor-egorov approved these changes Sep 6, 2018

View reviewed changes

igor-egorov self-requested a review September 6, 2018 08:22

igor-egorov previously requested changes Sep 6, 2018

View reviewed changes

Akvinikym added 2 commits September 7, 2018 11:03

Introduced new type, refactored a bit and fixed new tests

7f2db2b

Signed-off-by: Akvinikym <[email protected]>

Fixed other review issues

f830bbd

Signed-off-by: Akvinikym <[email protected]>

Akvinikym requested review from igor-egorov and muratovv September 7, 2018 10:18

igor-egorov approved these changes Sep 7, 2018

View reviewed changes

Akvinikym added 2 commits September 8, 2018 18:57

Fixed build

314010e

Signed-off-by: Akvinikym <[email protected]>

Fixed little bug

7ebc34e

Signed-off-by: Akvinikym <[email protected]>

muratovv previously requested changes Sep 10, 2018

View reviewed changes

Akvinikym added 3 commits September 10, 2018 11:46

Review issues

2fd5086

Signed-off-by: Akvinikym <[email protected]>

Merged with dev

ad697c1

Signed-off-by: Akvinikym <[email protected]>

Fixed one thing

4925148

Signed-off-by: Akvinikym <[email protected]>

Akvinikym requested a review from muratovv September 10, 2018 09:01

muratovv approved these changes Sep 10, 2018

View reviewed changes

lebdron approved these changes Sep 10, 2018

View reviewed changes

Akvinikym added 3 commits September 10, 2018 15:55

Fixed issues

cc91ac1

Signed-off-by: Akvinikym <[email protected]>

Minor things

aa258a0

Signed-off-by: Akvinikym <[email protected]>

Minor things

c37e2aa

Signed-off-by: Akvinikym <[email protected]>

Akvinikym force-pushed the feature/itf-get-pending-txs branch from 6ac4763 to 2e21944 Compare September 10, 2018 15:34

Another

dcf06c3

Signed-off-by: Akvinikym <[email protected]>

Akvinikym force-pushed the feature/itf-get-pending-txs branch from 2e21944 to dcf06c3 Compare September 10, 2018 15:46

I hope last commit

2fa86e1

Signed-off-by: Akvinikym <[email protected]>

Akvinikym merged commit be1dc33 into dev Sep 11, 2018

Akvinikym deleted the feature/itf-get-pending-txs branch September 11, 2018 05:07

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

MST fix & integration tests for GetPendingTransactions() (#1661)

a9e0b57

Signed-off-by: Akvinikym <[email protected]>

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

Integration tests for GetPendingTransactions() #1661

Integration tests for GetPendingTransactions() #1661

Akvinikym commented Aug 21, 2018 •

edited

Loading

igor-egorov left a comment

igor-egorov left a comment

igor-egorov Sep 1, 2018

Akvinikym Sep 5, 2018

igor-egorov Sep 1, 2018

igor-egorov Sep 1, 2018

igor-egorov Sep 1, 2018

igor-egorov Sep 1, 2018

igor-egorov Sep 1, 2018

muratovv Sep 6, 2018

muratovv Sep 6, 2018

muratovv Sep 6, 2018

igor-egorov Sep 6, 2018 •

edited

Loading

igor-egorov left a comment

igor-egorov Sep 6, 2018

igor-egorov Sep 6, 2018

igor-egorov Sep 6, 2018

igor-egorov left a comment

muratovv Sep 10, 2018

muratovv Sep 10, 2018

muratovv Sep 10, 2018

lebdron Sep 10, 2018

lebdron Sep 10, 2018

lebdron Sep 10, 2018

lebdron Sep 10, 2018

sorabot commented Sep 10, 2018

Integration tests for GetPendingTransactions() #1661

Integration tests for GetPendingTransactions() #1661

Conversation

Akvinikym commented Aug 21, 2018 • edited Loading

Description of the Change

Benefits

Possible Drawbacks

Usage of tests

igor-egorov left a comment

Choose a reason for hiding this comment

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

igor-egorov Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

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

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

sorabot commented Sep 10, 2018

Akvinikym commented Aug 21, 2018 •

edited

Loading

igor-egorov Sep 6, 2018 •

edited

Loading