Add verification method of eth address and requestor singularity in ForcePayment #560

pawelkisielewicz · 2018-06-26T07:34:09Z

Resolves #545

pawelkisielewicz · 2018-06-26T07:36:57Z

concent_api/core/message_handlers.py

@@ -671,6 +676,11 @@ def handle_send_force_payment(client_message: message.concents.ForcePayment) ->
            reason = message.concents.ServiceRefused.REASON.DuplicateRequest
        )

+    if not are_eth_addresses_and_singularities_equal_in_all_subtask_results_accepted_list(client_message):
+        return message.concents.ServiceRefused(
+            reason = message.concents.ServiceRefused.REASON.DuplicateRequest


REASON will be changed. I wait until Golem add new REASON and I will change it. But you can give me your feedback for the rest of code now.

changed according to @lukasz-glen suggestion

cameel · 2018-06-26T09:53:27Z

You have a typo in pull request name.

rwrzesien · 2018-06-26T11:13:24Z

concent_api/core/message_handlers.py

@@ -653,13 +657,14 @@ def get_clients_eth_accounts(task_to_compute: message.tasks.TaskToCompute):
    return (requestor_eth_address, provider_eth_address)


-def handle_send_force_payment(client_message: message.concents.ForcePayment) -> message.concents.ForcePaymentCommitted:  # pylint: disable=inconsistent-return-statements
+def handle_send_force_payment(client_message: message.concents.ForcePayment) \


We don't brake signatures this way, please check coding-style. It should something like

def handle_send_force_payment( client_message: message.concents.ForcePayment ) -> Union[ServiceRefused, ForcePaymentRejected, ForcePaymentCommitted]:

rwrzesien · 2018-06-26T11:18:10Z

concent_api/core/message_handlers.py

+    subtask_results_accepted_list = force_payment.subtask_results_accepted_list
+
+    first_key = subtask_results_accepted_list[0].task_to_compute.requestor_ethereum_public_key
+    for subtask_results_accepted in subtask_results_accepted_list:


Why not performing both checks in single loop ? And with single if.

done in one loop, but i left two if. This variables, like subtask_results_accepted.task_to_compute.requestor_ethereum_public_key are pretty long and it's hard to read it in one if. I think it is the best way.. unless you have better idea :)

You can split if into several lines, something like:

if ( first_key != subtask_results_accepted.task_to_compute.requestor_ethereum_public_key or first_address != subtask_results_accepted.task_to_compute.requestor_ethereum_address ):

I have looked at the code now, please combine it into single if.

ok, done :)

rwrzesien · 2018-06-26T11:21:17Z

concent_api/core/tests/test_unit_validation.py

+
+
+REQUESTOR_PUBLIC_KEY = generate_ecc_key_pair()[1]
+REQUESTOR_ETH_ADDRESS = generate_priv_and_pub_eth_account_key()[1]


You have this available in ConcentIntegrationTestCase in self..

rwrzesien

Some small changes might be applied or at least clarification.

kbeker

Small changes, but please also test it locally with synced geth and on cluster

kbeker · 2018-06-26T13:35:35Z

concent_api/core/message_handlers.py

+            return False
+        if first_address != subtask_results_accepted.task_to_compute.requestor_ethereum_address:
+            return False
+    return True


This function should be placed in core/utils.py not in core/message_handler.py. I remember that, there are more functions like this that can be removed from this file and transferred to utils. It might be another issue and be done in new PR, but this function should be placed there right now.

rwrzesien · 2018-06-26T14:41:33Z

concent_api/core/message_handlers.py

@@ -653,13 +657,15 @@ def get_clients_eth_accounts(task_to_compute: message.tasks.TaskToCompute):
    return (requestor_eth_address, provider_eth_address)


-def handle_send_force_payment(client_message: message.concents.ForcePayment) -> message.concents.ForcePaymentCommitted:  # pylint: disable=inconsistent-return-statements
+def handle_send_force_payment(
+        client_message: message.concents.ForcePayment


Please fix indentation.

dybi

Some changes are needed

dybi · 2018-06-27T07:09:26Z

concent_api/core/utils.py

@@ -84,3 +85,19 @@ def hex_to_bytes_convert(client_public_key: str):
    key_bytes = decode_hex(client_public_key)
    assert len(key_bytes) == GOLEM_PUBLIC_KEY_LENGTH
    return key_bytes
+
+
+def are_eth_addresses_and_singularities_equal_in_all_subtask_results_accepted_list(force_payment: ForcePayment) -> bool:


You don't need to pass whole ForcePayment message; it will suffice if you pass just a subtask_results_accepted_list

as disuccesd, please remove this function and refactor verify_message_subtask_results_accepted and enhance it with check of requestor_ethereum_address

cameel · 2018-06-27T08:39:05Z

concent_api/core/subtask_helpers.py

+    is_ethereum_address_key_equal = len(
+        set(subtask_results_accepted.task_to_compute.requestor_ethereum_address for subtask_results_accepted in subtask_results_accepted_list)) == 1
+    is_ethereum_public_key_equal = len(
+        set(subtask_results_accepted.task_to_compute.requestor_ethereum_public_key for subtask_results_accepted in subtask_results_accepted_list)) == 1


Please fix the style here. This should be:

is_ethereum_public_key_equal = len( set(subtask_results_accepted.task_to_compute.requestor_ethereum_public_key for subtask_results_accepted in subtask_results_accepted_list) ) == 1

But I'd just use helper variables if you want to make it shorter:

unique_public_keys = set(subtask_results_accepted.task_to_compute.requestor_public_key for subtask_results_accepted in subtask_results_accepted_list) unique_ethereum_addresses = set(subtask_results_accepted.task_to_compute.requestor_ethereum_address for subtask_results_accepted in subtask_results_accepted_list) unique_ethereum_public_keys = set(subtask_results_accepted.task_to_compute.requestor_ethereum_public_key for subtask_results_accepted in subtask_results_accepted_list) is_public_key_equal = (len(public_keys) == 1) is_ethereum_address_key_equal = (len(ethereum_addresses) == 1) is_ethereum_public_key_equal = (len(ethereum_public_keys) == 1)

done, and Your suggestion is short and easy to read.

It still does not follow our styie. If you insist on having the parameter on a separate line then the closing parenthesis must be on a separate line too:

unique_public_keys = set( subtask_results_accepted.task_to_compute.requestor_public_key for subtask_results_accepted in subtask_results_accepted_list )

It would be better to just keep it in a single line.

done. It is a long line.. :)

We do not have hard line length limit. Just a suggestion to limit length to 120. The goal is readability. Sometimes it's more readable to just leave a long line as is. Especially when there are several similar long lines next to each other.

ok :) it's done

cameel · 2018-06-27T08:41:34Z

concent_api/core/tests/test_unit_validation.py

+from utils.testing_helpers import generate_ecc_key_pair
+
+
+CONCENT_PRIVATE_KEY, CONCENT_PUBLIC_KEY = generate_ecc_key_pair()


Add parentheses

(CONCENT_PRIVATE_KEY, CONCENT_PUBLIC_KEY) = generate_ecc_key_pair()

cameel · 2018-06-27T08:44:43Z

concent_api/core/tests/test_unit_validation.py

+
+
+@override_settings(CONCENT_PUBLIC_KEY=CONCENT_PUBLIC_KEY)
+class TestAreEthAddressesAndSingularitiesEqual(ConcentIntegrationTestCase):


"singularity" is a property of a value (I think "uniqueness" would be a better word here though). You're not comparing singularities. You're checking if the values are singular (unique). The name of the class does not make sense.

rwrzesien

Looks good after fixes, but please also apply other reviewers comments.

dybi

Some changes are needed

dybi · 2018-06-27T10:14:24Z

concent_api/core/subtask_helpers.py

@@ -146,11 +146,21 @@ def update_subtask_state(
    subtask.save()


-def verify_message_subtask_results_accepted(subtask_results_accepted_list: dict) -> bool:
+def verify_message_subtask_results_accepted(subtask_results_accepted_list: list) -> bool:


please reanme it -> as it returns bool, the name of the function shpuld be in a form of a question

please change list to 'List[SomeType]'

dybi · 2018-06-27T10:20:39Z

concent_api/core/subtask_helpers.py

+    unique_ethereum_public_keys = set(
+        subtask_results_accepted.task_to_compute.requestor_ethereum_public_key for subtask_results_accepted in subtask_results_accepted_list)
+
+    is_public_key_equal = (len(unique_public_keys) == 1)


rename (one key is equal to what?); it's supposed to be unique not equal; the same for lines below

dybi · 2018-06-27T10:21:00Z

concent_api/core/tests/test_unit_validation.py

+
+
+@override_settings(CONCENT_PUBLIC_KEY=CONCENT_PUBLIC_KEY)
+class TestAreEthAddressesAndKeysAreSingular(ConcentIntegrationTestCase):


please rename

please renametest class

renamed to TestAreEthereumAddressesAndKeysUnique. Is it ok ??

dybi · 2018-06-27T10:23:57Z

concent_api/core/tests/test_unit_validation.py

+
+
+@override_settings(CONCENT_PUBLIC_KEY=CONCENT_PUBLIC_KEY)
+class TestAreEthAddressesAndKeysAreSingular(ConcentIntegrationTestCase):


please refactor - extract repeated values to setUp, etc

dybi · 2018-06-27T10:24:13Z

concent_api/core/tests/test_unit_validation.py

+
+
+@override_settings(CONCENT_PUBLIC_KEY=CONCENT_PUBLIC_KEY)
+class TestAreEthAddressesAndKeysAreSingular(ConcentIntegrationTestCase):


please add test for missing condition

dybi

Pleass address issues; also please write "done" below issue that has been addressed

dybi · 2018-06-27T12:58:39Z

concent_api/core/message_handlers.py


-    if not verify_message_subtask_results_accepted(client_message.subtask_results_accepted_list):
+    if not are_keys_and_addresses_in_message_subtask_results_accepted_unique(client_message.subtask_results_accepted_list):


are_keys_and_addresses_in_message_subtask_results_accepted_unique -> are_keys_and_addresses_unique_in_message_subtask_results_accepted

dybi · 2018-06-27T13:00:09Z

concent_api/core/subtask_helpers.py

@@ -146,11 +146,18 @@ def update_subtask_state(
    subtask.save()


-def verify_message_subtask_results_accepted(subtask_results_accepted_list: dict) -> bool:
+def are_keys_and_addresses_in_message_subtask_results_accepted_unique(subtask_results_accepted_list: list) -> bool:


list -> List[SomeType] (#2 warning -> 👿 )

dybi · 2018-06-27T13:01:32Z

concent_api/core/subtask_helpers.py

+    unique_ethereum_public_keys = set(subtask_results_accepted.task_to_compute.requestor_ethereum_public_key for subtask_results_accepted in subtask_results_accepted_list)
+
+    is_public_key_unique = (len(unique_public_keys) == 1)
+    is_ethereum_address_key_unique = (len(unique_ethereum_addresses) == 1)


is_ethereum_address_key_unique -> is_ethereum_address_unique

dybi · 2018-06-27T13:28:29Z

concent_api/core/tests/test_unit_validation.py

+
+
+@override_settings(CONCENT_PUBLIC_KEY=CONCENT_PUBLIC_KEY)
+class TestAreEthAddressesAndKeysAreSingular(ConcentIntegrationTestCase):


please renametest class

kbeker · 2018-06-27T19:31:10Z

@pawelkisielewicz
Did you test it locally with geth and unmocked sci_backend?
Please ask @bartoszbetka tomorrow to deploy your branch od devel cluster and test it also there.

pawelkisielewicz self-assigned this Jun 26, 2018

pawelkisielewicz requested review from cameel, rwrzesien, bartoszbetka, kbeker, wyspiansky87 and dybi June 26, 2018 07:34

pawelkisielewicz commented Jun 26, 2018

View reviewed changes

pawelkisielewicz changed the title ~~Add verifivation method~~ Add verification method of eth address and requestor singularity in ForcePayment Jun 26, 2018

rwrzesien reviewed Jun 26, 2018

View reviewed changes

rwrzesien suggested changes Jun 26, 2018

View reviewed changes

kbeker suggested changes Jun 26, 2018

View reviewed changes

rwrzesien reviewed Jun 26, 2018

View reviewed changes

dybi suggested changes Jun 27, 2018

View reviewed changes

pawelkisielewicz changed the title ~~Add verification method of eth address and requestor singularity in ForcePayment~~ [WIP] Add verification method of eth address and requestor singularity in ForcePayment Jun 27, 2018

pawelkisielewicz changed the title ~~[WIP] Add verification method of eth address and requestor singularity in ForcePayment~~ Add verification method of eth address and requestor singularity in ForcePayment Jun 27, 2018

cameel reviewed Jun 27, 2018

View reviewed changes

rwrzesien approved these changes Jun 27, 2018

View reviewed changes

dybi suggested changes Jun 27, 2018

View reviewed changes

dybi approved these changes Jun 28, 2018

View reviewed changes

Add verifivation method

028eb25

pawelkisielewicz force-pushed the bug-verify-eth-address-and-requestor-singularity-in-force-payment branch from 1181a91 to 028eb25 Compare June 28, 2018 08:53

kbeker approved these changes Jun 28, 2018

View reviewed changes

pawelkisielewicz merged commit 2848ffd into master Jun 28, 2018

pawelkisielewicz deleted the bug-verify-eth-address-and-requestor-singularity-in-force-payment branch June 28, 2018 12:06

cameel added this to the 0.8.0 milestone Jul 31, 2018



		REQUESTOR_PUBLIC_KEY = generate_ecc_key_pair()[1]
		REQUESTOR_ETH_ADDRESS = generate_priv_and_pub_eth_account_key()[1]

		from utils.testing_helpers import generate_ecc_key_pair


		CONCENT_PRIVATE_KEY, CONCENT_PUBLIC_KEY = generate_ecc_key_pair()



		@override_settings(CONCENT_PUBLIC_KEY=CONCENT_PUBLIC_KEY)
		class TestAreEthAddressesAndSingularitiesEqual(ConcentIntegrationTestCase):



		@override_settings(CONCENT_PUBLIC_KEY=CONCENT_PUBLIC_KEY)
		class TestAreEthAddressesAndKeysAreSingular(ConcentIntegrationTestCase):


		if not verify_message_subtask_results_accepted(client_message.subtask_results_accepted_list):
		if not are_keys_and_addresses_in_message_subtask_results_accepted_unique(client_message.subtask_results_accepted_list):

Add verification method of eth address and requestor singularity in ForcePayment #560

Add verification method of eth address and requestor singularity in ForcePayment #560

Conversation

pawelkisielewicz commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel commented Jun 26, 2018

rwrzesien Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwrzesien Jun 26, 2018 • 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

rwrzesien left a comment

Choose a reason for hiding this comment

kbeker 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

dybi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Jun 27, 2018 • 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

rwrzesien left a comment

Choose a reason for hiding this comment

dybi 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

dybi 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

kbeker commented Jun 27, 2018 • edited Loading

rwrzesien Jun 26, 2018 •

edited

Loading

rwrzesien Jun 26, 2018 •

edited

Loading

kbeker left a comment •

edited

Loading

cameel Jun 27, 2018 •

edited

Loading

kbeker commented Jun 27, 2018 •

edited

Loading