INDY-2297, INDY-2302: Allow multiple active TAAs, change GET_TAA's reply format #1424

Toktar · 2019-12-05T08:56:12Z

No description provided.

TODO: - Choose versions mechanism in the write_request_manager - Add TAA validations for other requests Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

…nto task-2297-get-taa-digest

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

lgtm-com · 2019-12-05T10:17:18Z

This pull request introduces 2 alerts when merging 142f296 into 956cc45 - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Unused import

lgtm-com · 2019-12-05T10:38:08Z

This pull request introduces 2 alerts when merging 3c6877c into 956cc45 - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Unused import

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

lgtm-com · 2019-12-05T12:29:06Z

This pull request introduces 3 alerts when merging 8e92c92 into 956cc45 - view on LGTM.com

new alerts:

2 for Unused import
1 for Unused local variable

lgtm-com · 2019-12-05T16:29:29Z

This pull request introduces 3 alerts when merging b088222 into 956cc45 - view on LGTM.com

new alerts:

2 for Unused import
1 for Unused local variable

skhoroshavin

I might be missing something, but - is there old TAA request handler, did I miss it somehow?

plenum/common/txn_util.py

plenum/server/request_handlers/txn_author_agreement_handler.py

plenum/common/constants.py

lgtm-com · 2019-12-06T07:39:00Z

This pull request introduces 3 alerts when merging 91b7aae into fc5d8c0 - view on LGTM.com

new alerts:

2 for Unused import
1 for Unused local variable

lgtm-com · 2019-12-06T10:48:46Z

This pull request introduces 3 alerts when merging 1644abe into 8340342 - view on LGTM.com

new alerts:

2 for Unused import
1 for Unused local variable

…nto task-2297-get-taa-digest

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

lgtm-com · 2019-12-16T16:34:48Z

This pull request introduces 20 alerts when merging 01e8f50 into beb45f9 - view on LGTM.com

new alerts:

19 for Unused import
1 for Unused local variable

lgtm-com · 2019-12-17T09:40:13Z

This pull request introduces 22 alerts when merging 115cd81 into beb45f9 - view on LGTM.com

new alerts:

21 for Unused import
1 for Unused local variable

Toktar · 2019-12-17T10:54:38Z

test this please

lgtm-com · 2019-12-17T12:23:16Z

This pull request introduces 22 alerts when merging 9aa1ea5 into beb45f9 - view on LGTM.com

new alerts:

21 for Unused import
1 for Unused local variable

skhoroshavin

Code review part 1

plenum/common/constants.py

plenum/server/batch_handlers/audit_batch_handler.py

plenum/test/audit_ledger/helper.py

plenum/test/input_validation/test_client_taa_disable_op.py

plenum/server/request_handlers/base_taa_handler.py

skhoroshavin · 2019-12-17T12:03:05Z

plenum/server/request_handlers/base_taa_handler.py

+
+class BaseTAAHandler(WriteRequestHandler, metaclass=ABCMeta):
+
+    def _update_txn_author_agreement(self, digest, seq_no, txn_time, text=None, version=None, retired=None):


This function looks a bit overcomplicated due to usage in very different scenarios. Probably it makes sense to split it into:

create_txn_author_agreement (which writes to all keys)

update_txn_author_agreement (which updates just key containing data)

plenum/server/request_handlers/txn_author_agreement_handler.py

plenum/server/request_managers/write_request_manager.py

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-2302] Fix getting the latest taa digest

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

lgtm-com · 2019-12-17T14:09:45Z

This pull request introduces 22 alerts when merging 59f2722 into beb45f9 - view on LGTM.com

new alerts:

21 for Unused import
1 for Unused local variable

plenum/server/request_handlers/txn_author_agreement_handler.py

plenum/test/req_handler/test_request_manager.py

plenum/test/req_handler/test_txn_author_agreement_disable_handler.py

skhoroshavin · 2019-12-17T14:55:22Z

plenum/test/req_handler/test_txn_author_agreement_disable_handler.py

+    state_value[TXN_AUTHOR_AGREEMENT_TIMESTAMP] = None if retired else taa_txn_time
+
+    # Set a TAA
+    txn_author_agreement_handler.update_state(taa_txn, None, taa_request)


I think it is better to:

move preparation stage (writing TAA to state) to helper function

add case when we have more than one TAA in ledger when TAA_DISABLE is processed (and check that all of TAAs are disable)

add case when we send another TAA to ledger after sending TAA_DISABLE and make sure that all previously disabled TAAs stay disabled

plenum/test/txn_author_agreement/acceptance/test_taa_acceptance_validation.py

plenum/test/txn_author_agreement/helper.py

plenum/test/txn_author_agreement/test_get_txn_author_agreement.py

skhoroshavin · 2019-12-17T15:25:38Z

plenum/test/txn_author_agreement/test_txn_author_agreement_disable.py

+            match='Transaction author agreement is already disabled'
+    ):
+        sdk_send_txn_author_agreement_disable(looper, sdk_pool_handle, sdk_wallet_trustee)
+    assert get_txn_author_agreement() is None


Same comment as in similar unit-like tests - having more cases would be very nice

lgtm-com · 2019-12-17T16:09:53Z

This pull request introduces 22 alerts when merging 7e5ef8d into beb45f9 - view on LGTM.com

new alerts:

21 for Unused import
1 for Unused local variable

Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>

lgtm-com · 2019-12-17T18:40:18Z

This pull request introduces 22 alerts when merging d4acabd into beb45f9 - view on LGTM.com

new alerts:

21 for Unused import
1 for Unused local variable

Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>

lgtm-com · 2019-12-18T10:24:18Z

This pull request introduces 22 alerts when merging ef899e3 into beb45f9 - view on LGTM.com

new alerts:

21 for Unused import
1 for Unused local variable

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

lgtm-com · 2019-12-18T14:03:22Z

This pull request introduces 23 alerts when merging d62d67b into beb45f9 - view on LGTM.com

new alerts:

21 for Unused import
2 for Unused local variable

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

lgtm-com · 2019-12-18T15:17:42Z

This pull request introduces 23 alerts when merging cf0527b into beb45f9 - view on LGTM.com

new alerts:

21 for Unused import
2 for Unused local variable

Toktar · 2019-12-18T18:53:41Z

Test this please

Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>

lgtm-com · 2019-12-19T00:01:37Z

This pull request introduces 19 alerts when merging 0d981e6 into beb45f9 - view on LGTM.com

new alerts:

17 for Unused import
2 for Unused local variable

ashcherbakov · 2019-12-19T08:54:33Z

plenum/server/request_managers/write_request_manager.py

-            raise LogicError(
-                "Txn Author Agreement digest is not defined: version {}, seq_no {}, txn_time {}"
-                .format(taa[TXN_AUTHOR_AGREEMENT_VERSION], taa_seq_no, taa_txn_time)
+        if not self.get_taa_data():


I think we need to get uncommitted TAA here, as committed is not guaranteed to be the same on all the nodes. So, if there is a request with accepted TAA sent right after this TAA is added to the ledger, there is a chance that it may be handled differently by nodes.

Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>

lgtm-com · 2019-12-19T10:38:54Z

This pull request introduces 19 alerts when merging 2d83217 into beb45f9 - view on LGTM.com

new alerts:

17 for Unused import
2 for Unused local variable

ashcherbakov · 2019-12-19T12:08:40Z

plenum/server/request_managers/write_request_manager.py

+        else:
+            raise InvalidClientTaaAcceptanceError(
+                request.identifier, request.reqId,
+                "Incorrect Txn Author Agreement(digest={}) in the request".format(r_taa_a_digest)


I think it's better to say:
Accepted Txn Author Agreement (digest={}) is not found on the ledger. Please make sure that an existing Agreement is accepted.".format(r_taa_a_digest)

ashcherbakov · 2019-12-19T12:11:15Z

plenum/server/request_managers/write_request_manager.py

-                "Txn Author Agreement acceptance digest is invalid or non-latest:"
-                " provided {}, expected {}"
-                .format(r_taa_a_digest, taa_digest)
+                "Txn Author Agreement is retired: version {}, seq_no {}, txn_time {}"


I think it's better to say:
Accepted Txn Author Agreement with version={} and digest={} is retired on {}. Please accept an active Agreement.

ashcherbakov · 2019-12-19T12:11:40Z

plenum/server/request_managers/write_request_manager.py

+            )
+
+        retired = taa.get(TXN_AUTHOR_AGREEMENT_RETIRED)
+        if retired and retired < req_pp_time:


Should it be <=?

Toktar added 9 commits December 2, 2019 00:25

INDY-2297, INDY-2302: add versioning handlers for the TAA

a6d2e1a

TODO: - Choose versions mechanism in the write_request_manager - Add TAA validations for other requests Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-2297, INDY-2302: update TAA handler and tests

9fdbf1c

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-2297, INDY-2302: update TAA handler, move TAA handler_v1

28c9a24

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Merge branch 'master' of https://github.com/hyperledger/indy-plenum i…

51bcdd8

…nto task-2297-get-taa-digest

INDY-2302: change logic of version choosing for _get_handlers_by_version

142f296

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-2302: move test_txn_author_agreement_handler_v1 to indy-node

3c6877c

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-2302: bugfix in write_request_manager

8e92c92

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-2302: update do_taa_validation, fix tests

b088222

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-2302: change the constant TXN_AUTHOR_AGREEMENT_TIMESTAMP

91b7aae

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Toktar requested review from lampkin-diet, ashcherbakov and skhoroshavin as code owners December 5, 2019 08:56

INDY-2297: update tests

1644abe

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

skhoroshavin reviewed Dec 5, 2019

View reviewed changes

plenum/common/txn_util.py Outdated Show resolved Hide resolved

plenum/server/request_handlers/txn_author_agreement_handler.py Outdated Show resolved Hide resolved

plenum/common/constants.py Outdated Show resolved Hide resolved

Toktar added 5 commits December 9, 2019 15:58

Merge branch 'master' of https://github.com/hyperledger/indy-plenum i…

fdc3d8f

…nto task-2297-get-taa-digest

INDY-2302: Add a new txn TXN_AUTHOR_AGREEMENT_DISABLE

01e8f50

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-2302: update taa_signature validation

115cd81

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-2302: update tests

70a4aa4

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-2302: flake8

9aa1ea5

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Toktar changed the title ~~INDY-2297, INDY-2302: Allow multiple active TAAs, change GET_TAA's reply format~~ [ci skip][INDY-2297, INDY-2302]: Allow multiple active TAAs, change GET_TAA's reply format Dec 16, 2019

Toktar changed the title ~~[ci skip][INDY-2297, INDY-2302]: Allow multiple active TAAs, change GET_TAA's reply format~~ INDY-2297, INDY-2302: Allow multiple active TAAs, change GET_TAA's reply format Dec 17, 2019

skhoroshavin reviewed Dec 17, 2019

View reviewed changes

Andrew Nikitin and others added 3 commits December 17, 2019 16:48

[INDY-2302] Fix getting the latest taa digest

e4ae94a

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

Merge pull request #6 from anikitinDSR/public/indy-2302

59f2722

[INDY-2302] Fix getting the latest taa digest

INDY-2302: fix tests

7e5ef8d

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

skhoroshavin reviewed Dec 17, 2019

View reviewed changes

INDY-2302: Fix more tests (and potential crash when rotating keys)

d4acabd

Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>

INDY-2302: Fix more tests

ef899e3

Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>

INDY-2302: update TAA handlers

d62d67b

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-2302: fix tests

cf0527b

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-2302: Attempt to fix test_taa_acceptance_retired

0d981e6

Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>

ashcherbakov reviewed Dec 19, 2019

View reviewed changes

INDY-2302: Skip failing test

2d83217

Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>

skhoroshavin approved these changes Dec 19, 2019

View reviewed changes

Toktar merged commit 9df777f into hyperledger:master Dec 19, 2019

ashcherbakov reviewed Dec 19, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INDY-2297, INDY-2302: Allow multiple active TAAs, change GET_TAA's reply format #1424

INDY-2297, INDY-2302: Allow multiple active TAAs, change GET_TAA's reply format #1424

Toktar commented Dec 5, 2019

lgtm-com bot commented Dec 5, 2019

lgtm-com bot commented Dec 5, 2019

lgtm-com bot commented Dec 5, 2019

lgtm-com bot commented Dec 5, 2019

skhoroshavin left a comment

lgtm-com bot commented Dec 6, 2019

lgtm-com bot commented Dec 6, 2019

lgtm-com bot commented Dec 16, 2019

lgtm-com bot commented Dec 17, 2019

Toktar commented Dec 17, 2019

lgtm-com bot commented Dec 17, 2019

skhoroshavin left a comment

skhoroshavin Dec 17, 2019

lgtm-com bot commented Dec 17, 2019

skhoroshavin Dec 17, 2019

skhoroshavin Dec 17, 2019

lgtm-com bot commented Dec 17, 2019

lgtm-com bot commented Dec 17, 2019

lgtm-com bot commented Dec 18, 2019

lgtm-com bot commented Dec 18, 2019

lgtm-com bot commented Dec 18, 2019

Toktar commented Dec 18, 2019

lgtm-com bot commented Dec 19, 2019

ashcherbakov Dec 19, 2019

lgtm-com bot commented Dec 19, 2019

ashcherbakov Dec 19, 2019

ashcherbakov Dec 19, 2019

ashcherbakov Dec 19, 2019


		class BaseTAAHandler(WriteRequestHandler, metaclass=ABCMeta):

		def _update_txn_author_agreement(self, digest, seq_no, txn_time, text=None, version=None, retired=None):

INDY-2297, INDY-2302: Allow multiple active TAAs, change GET_TAA's reply format #1424

INDY-2297, INDY-2302: Allow multiple active TAAs, change GET_TAA's reply format #1424

Conversation

Toktar commented Dec 5, 2019

lgtm-com bot commented Dec 5, 2019

lgtm-com bot commented Dec 5, 2019

lgtm-com bot commented Dec 5, 2019

lgtm-com bot commented Dec 5, 2019

skhoroshavin left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Dec 6, 2019

lgtm-com bot commented Dec 6, 2019

lgtm-com bot commented Dec 16, 2019

lgtm-com bot commented Dec 17, 2019

Toktar commented Dec 17, 2019

lgtm-com bot commented Dec 17, 2019

skhoroshavin left a comment

Choose a reason for hiding this comment

skhoroshavin Dec 17, 2019

Choose a reason for hiding this comment

lgtm-com bot commented Dec 17, 2019

skhoroshavin Dec 17, 2019

Choose a reason for hiding this comment

skhoroshavin Dec 17, 2019

Choose a reason for hiding this comment

lgtm-com bot commented Dec 17, 2019

lgtm-com bot commented Dec 17, 2019

lgtm-com bot commented Dec 18, 2019

lgtm-com bot commented Dec 18, 2019

lgtm-com bot commented Dec 18, 2019

Toktar commented Dec 18, 2019

lgtm-com bot commented Dec 19, 2019

ashcherbakov Dec 19, 2019

Choose a reason for hiding this comment

lgtm-com bot commented Dec 19, 2019

ashcherbakov Dec 19, 2019

Choose a reason for hiding this comment

ashcherbakov Dec 19, 2019

Choose a reason for hiding this comment

ashcherbakov Dec 19, 2019

Choose a reason for hiding this comment