Check chain data when performing settle by pcppcp · Pull Request #2468 · raiden-network/raiden

pcppcp · 2018-09-13T12:26:20Z

checks on-chain state of the channel balance_proof state in order to
use ensure consistent settle call even if partner used empty hash to
close the channel or if he didn't call updateTransfer
fixes Check if partner submitted balance proof before settling #2410

[ci integration]

coveralls · 2018-09-13T13:23:56Z

Coverage remained the same at 77.852% when pulling 47452d6 on pcppcp:test_balance_proof into 8899541 on raiden-network:master.

palango · 2018-09-13T15:35:51Z


-        if our_balance_proof:
+        our_balance_hash = channel_details.participants_data.our_details.balance_hash
+        if our_balance_proof and our_balance_hash != bytes(32):


Can you use the raiden.constants.EMPTY_HASH constant here.

I agree; fixed

palango · 2018-09-13T15:35:58Z


-        if partner_balance_proof:
+        partner_balance_hash = channel_details.participants_data.partner_details.balance_hash
+        if partner_balance_proof and partner_balance_hash != bytes(32):


palango · 2018-09-13T15:36:53Z

+        app0,
+        app1,
+        token_network_identifier,
+        1,


would be nice to add keywords for all parameters

yeah it would, but there're no kwargs for this call :)

Hu? Why can't you write amount=1?

plz add kwargs as @palango suggested. We want to make it a habit. Helps readability and avoid errors.

Edit: what he means is:

direct_transfer( initiator_app=app0, target_app=app1, ... )

Also please do the same to all the function calls in this test

palango

Last comment is about readability, lets get this merged afterwards.

LefterisJP · 2018-09-13T21:12:32Z

            args=args,
        )

+    def decode_transaction_input(self, transaction_hash):


LefterisJP · 2018-09-13T21:20:36Z

+        app0,
+        app1,
+        token_network_identifier,
+        1,


plz add kwargs as @palango suggested. We want to make it a habit. Helps readability and avoid errors.

Edit: what he means is:

direct_transfer( initiator_app=app0, target_app=app1, ... )

LefterisJP · 2018-09-13T21:25:13Z

+        app0,
+        app1,
+        token_network_identifier,
+        1,


Also please do the same to all the function calls in this test

LefterisJP · 2018-09-13T21:36:02Z

+    )
+    channel_identifier = get_channelstate(app0, app1, token_network_identifier).identifier
+
+    # make a transfer


Add a bit more commentary here to explain what's happening:

-# make a transfer +# make a transfer from app0 to app1 so that app1 is supposed to have a non empty balance hash

LefterisJP · 2018-09-13T21:41:34Z

+    app1.stop()
+    token_network_contract = TokenNetwork(app1.raiden.chain.client, token_network_identifier)
+
+    # app1 closes the channel


Add more info on the comment.

- # app1 closes the channel + # app1 closes the channel with an empty hash instead of the expected hash of the transferred amount from app0

LefterisJP · 2018-09-13T21:42:45Z

+    )
+    channel_identifier = get_channelstate(app0, app1, token_network_identifier).identifier
+
+    # make a transfer


Same comments as in the above test apply.

Add kwargs in the function calls

Expand comments a bit so that the future readers easily understand what's happening

- checks on-chain state of the channel balance_proof state in order to use ensure consistent settle call even if partner used empty hash to `close` the channel or if he didn't call `updateTransfer` - fixes raiden-network#2410 - fix webargs' `handle_request_parsing_error` [ci integration]

LefterisJP · 2018-09-14T13:27:22Z

The stress test fails. But locally when I check out your branch it passes.

LefterisJP · 2018-09-14T14:34:10Z

merging since all pass except a flaky test.

pcppcp force-pushed the test_balance_proof branch 4 times, most recently from dd90a30 to 0a80d37 Compare September 13, 2018 12:37

pcppcp added the dev: Please Review label Sep 13, 2018

palango reviewed Sep 13, 2018

View reviewed changes

pcppcp force-pushed the test_balance_proof branch from 0a80d37 to be185ab Compare September 13, 2018 16:23

palango approved these changes Sep 13, 2018

View reviewed changes

LefterisJP reviewed Sep 13, 2018

View reviewed changes

pcppcp force-pushed the test_balance_proof branch 6 times, most recently from 47452d6 to 11d3131 Compare September 14, 2018 09:41

pcppcp force-pushed the test_balance_proof branch from 11d3131 to 2423540 Compare September 14, 2018 10:45

LefterisJP mentioned this pull request Sep 14, 2018

Stress test is flaky with "exceeds block gas limit" #2492

Closed

LefterisJP merged commit 03ab8d2 into raiden-network:master Sep 14, 2018

Conversation

pcppcp commented Sep 13, 2018

Uh oh!

coveralls commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palango left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LefterisJP commented Sep 14, 2018

Uh oh!

LefterisJP commented Sep 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Sep 13, 2018 •

edited

Loading