Comment out cooperativeSettle & setTotalWithdraw temporarily #270

loredanacirstea · 2018-09-17T15:34:01Z

fixes #265

Please do not merge this until I have the contract limits PR functional on top of this one.

Issue: TokenNetworkRegistry bytecode is too big for deployment if we try to implement #262 and #263

We might go with either:

(favorite option until now) remove code that is unimplemented in Raiden: Comment out cooperativeSettle & setTotalWithdraw temporarily #270 (remove cooperativeSettle & setTotalWithdraw)
move some part of the code in a library Move constant functions to TokenNetworkUtils library #264 (implemented just in case)

Changed in this PR, that should be changed back when re-enabling cooperative settle & withdraw

Contracts

uncomment withdraw event

raiden-contracts/raiden_contracts/contracts/TokenNetwork.sol

Lines 148 to 152 in d47aa6b

    
               /* event ChannelWithdraw( 
        
                   uint256 indexed channel_identifier, 
        
                   address indexed participant, 
        
                   uint256 total_withdraw 
        
               ); */

uncomment setTotalWithdraw

raiden-contracts/raiden_contracts/contracts/TokenNetwork.sol

Line 333 in d47aa6b

/* /// @notice Allows `participant` to withdraw tokens from the channel that he
uncomment cooperativeSettle

raiden-contracts/raiden_contracts/contracts/TokenNetwork.sol

Line 814 in d47aa6b

/* /// @notice Cooperatively settles the balances between the two channel
uncomment recoverAddressFromCooperativeSettleSignature

raiden-contracts/raiden_contracts/contracts/TokenNetwork.sol

Line 1441 in d47aa6b

/* function recoverAddressFromCooperativeSettleSignature(
uncomment recoverAddressFromWithdrawMessage

raiden-contracts/raiden_contracts/contracts/TokenNetwork.sol

Line 1472 in d47aa6b

/* function recoverAddressFromWithdrawMessage(

Test contracts

uncomment recoverAddressFromCooperativeSettleSignaturePublic recoverAddressFromCooperativeSettleSignaturePublic
uncomment recoverAddressFromWithdrawMessagePublic

raiden-contracts/raiden_contracts/contracts/test/TokenNetworkInternalsTest.sol

Line 224 in d47aa6b

/* function recoverAddressFromWithdrawMessagePublic(

Tests

raiden-contracts/raiden_contracts/tests/fixtures/channel.py

Lines 118 to 126 in d47aa6b

    
           # TODO uncomment this after setTotalWithdraw is uncommented in the TokenNetwork contract 
        
           txn_hash = b'' 
        
           # txn_hash = token_network.functions.setTotalWithdraw( 
        
           #     channel_identifier, 
        
           #     participant, 
        
           #     withdraw_amount, 
        
           #     signature_participant, 
        
           #     signature_partner 
        
           # ).transact({'from': delegate})

unskip all cooperative settle tests in https://github.com/raiden-network/raiden-contracts/blob/d47aa6b6adbbfc3702e2258387d7695c96101d7b/raiden_contracts/tests/test_channel_cooperative_settle.py
unskip all withdraw tests in https://github.com/raiden-network/raiden-contracts/blob/d47aa6b6adbbfc3702e2258387d7695c96101d7b/raiden_contracts/tests/test_channel_withdraw.py

fix settlement test

raiden-contracts/raiden_contracts/tests/test_channel_settle.py

Lines 146 to 158 in d47aa6b

    
           # FIXME after setTotalWithdraw is implemented again 
        
           post_balance_A = pre_balance_A + 33 
        
           post_balance_B = pre_balance_B + 15 
        
           post_balance_contract = pre_balance_contract - 48 
        
           # FIXME after setTotalWithdraw is implemented again 
        
           # we add the withdrawn amount here, because it was never withdrawn due to the 
        
           # removal of setTotalWithdraw 
        
           assert custom_token.functions.balanceOf(A).call() == post_balance_A + 10 
        
           assert custom_token.functions.balanceOf(B).call() == post_balance_B + 5 
        
           assert custom_token.functions.balanceOf( 
        
               token_network.address, 
        
           ).call() == post_balance_contract - 15

unskip test_recover_address_from_withdraw_message

raiden-contracts/raiden_contracts/tests/unit/test_unit_signatures.py

Lines 6 to 7 in d47aa6b

@pytest.mark.skip(reason='Delayed to another milestone')

def test_recover_address_from_withdraw_message(

unskip test_recover_address_from_cooperative_settle_signature

raiden-contracts/raiden_contracts/tests/unit/test_unit_signatures.py

Lines 173 to 174 in d47aa6b

    
           @pytest.mark.skip(reason='Delayed to another milestone') 
        
           def test_recover_address_from_cooperative_settle_signature(

fix ChannelValues (now withdraw is set to 0 for all tests)

raiden-contracts/raiden_contracts/tests/utils/utils.py

Lines 30 to 31 in d47aa6b

# FIXME after setTotalWithdraw is enabled again

self.withdrawn = 0

LefterisJP · 2018-09-18T11:54:21Z

raiden_contracts/contracts/TokenNetwork.sol

@@ -330,7 +330,7 @@ contract TokenNetwork is Utils {
        require(token.transferFrom(msg.sender, address(this), added_deposit));
    }

-    /// @notice Allows `participant` to withdraw tokens from the channel that he
+    /* /// @notice Allows `participant` to withdraw tokens from the channel that he


nitpick: Why start he comment at the docstring (commenting out comments does not make sense) and not at the actual code like you did at the event above?

The natspec comments as more than normal comments and part of the function that they actually define. This is why I also commented them out.
If you specifically want them to not be commented out, please let me know.

LefterisJP · 2018-09-18T11:54:49Z

raiden_contracts/contracts/TokenNetwork.sol

@@ -811,7 +811,7 @@ contract TokenNetwork is Utils {
        assert(locked_amount >= unlocked_amount);
    }

-    /// @notice Cooperatively settles the balances between the two channel
+    /* /// @notice Cooperatively settles the balances between the two channel


LefterisJP · 2018-09-18T11:56:02Z

raiden_contracts/tests/fixtures/channel.py

-            signature_participant,
-            signature_partner
-        ).transact({'from': delegate})
+        # TODO uncomment this after setTotalWithdraw is uncommented in the TokenNetwork contract


In order to not forget it in the future I suggest making an issue in this repo about reintroducint coopsettle and total withdraw and link it in this and subsequent comments

Yes, I will make an issue with a list of changes after merging.

LefterisJP · 2018-09-18T11:56:56Z

raiden_contracts/tests/test_channel_cooperative_settle.py

@@ -520,6 +528,7 @@ def test_cooperative_settle_channel_wrong_balances(
    ).transact({'from': C})


+@pytest.mark.skip(reason='Delayed to another milestone')


super super nitpick for all of these skips: Delayed until

LefterisJP · 2018-09-18T11:59:41Z

raiden_contracts/tests/test_channel_settle.py

@@ -389,6 +390,14 @@ def test_settlement_with_unauthorized_token_transfer(
        vals_B,
    )

+    # Assign additional tokens to A
+    assign_tokens(A, externally_transferred_amount)


Why are you changing the logic of the the test in this PR? I can't see how this is connected to removing coopsettle and withdraw.

Because the test was completely wrong. A did not have tokens to transfer and the transfer function used does not revert, it just returns false. So A was not transferring anything to the token_network and the last balance checks were constructed wrongly.
I am adding some more checks for the balances in the test, so things are clear.

Added the additional checks in dab0a9a

LefterisJP · 2018-09-18T12:00:42Z

raiden_contracts/tests/utils/utils.py

@@ -25,7 +25,9 @@ def __init__(
            additional_hash=EMPTY_ADDITIONAL_HASH,
    ):
        self.deposit = deposit
-        self.withdrawn = withdrawn
+        # FIXME after setTotalWithdraw is enabled again


if you make that issue I mentioned above you should point to it here too.

raiden-network#270 (comment)

We are scoping out the cooperativeSettle for now, as it is not implemented in the Raiden client.

We are scoping out the setTotalWithdraw for now, as it is not implemented in the Raiden client.

raiden-network#270 (comment)

…ransfer

#270 (comment)

loredanacirstea mentioned this pull request Sep 17, 2018

Remove cooperativeSettle temporarily #269

Closed

loredanacirstea force-pushed the remove-cooperativesettle-withdraw branch from f80e600 to 39929d7 Compare September 18, 2018 09:27

LefterisJP reviewed Sep 18, 2018

View reviewed changes

loredanacirstea added the dev: Work In Progress label Sep 18, 2018

loredanacirstea added a commit to loredanacirstea/raiden-contracts that referenced this pull request Sep 18, 2018

Travis skip message fix (pr review)

6af34c4

raiden-network#270 (comment)

This was referenced Sep 18, 2018

Move constant functions to TokenNetworkUtils library #264

Closed

Deprecation switch for Bug Bounty release #275

Merged

loredanacirstea added a commit to loredanacirstea/raiden-contracts that referenced this pull request Sep 19, 2018

Travis skip message fix (pr review)

2b32366

raiden-network#270 (comment)

loredanacirstea force-pushed the remove-cooperativesettle-withdraw branch from 6af34c4 to d5c398c Compare September 19, 2018 10:02

loredanacirstea added a commit to loredanacirstea/raiden-contracts that referenced this pull request Sep 19, 2018

Travis skip message fix (pr review)

d5c398c

raiden-network#270 (comment)

loredanacirstea added 6 commits September 19, 2018 17:32

Comment out cooperativeSettle & skip tests - to be added later

cd6a34b

We are scoping out the cooperativeSettle for now, as it is not implemented in the Raiden client.

Comment out setTotalWithdraw & skip tests - to be added later

acf3717

We are scoping out the setTotalWithdraw for now, as it is not implemented in the Raiden client.

Skip another withdraw signature test

fa18e45

Travis skip message fix (pr review)

864f1a7

raiden-network#270 (comment)

Fix failing test after setTotalWithdraw removal

efbd0af

Add more balance checks for test_settlement_with_unauthorized_token_t…

d47aa6b

…ransfer

loredanacirstea force-pushed the remove-cooperativesettle-withdraw branch from dab0a9a to d47aa6b Compare September 19, 2018 15:32

loredanacirstea added dev: Please Review and removed dev: Work In Progress labels Sep 19, 2018

LefterisJP approved these changes Sep 20, 2018

View reviewed changes

LefterisJP merged commit 2b4d503 into raiden-network:master Sep 20, 2018

LefterisJP pushed a commit that referenced this pull request Sep 20, 2018

Travis skip message fix (pr review)

14ad055

#270 (comment)

loredanacirstea mentioned this pull request Sep 20, 2018

Reenable setTotalWithdraw #277

Closed

loredanacirstea mentioned this pull request Jan 7, 2019

Reenable cooperativeSettle #399

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comment out cooperativeSettle & setTotalWithdraw temporarily #270

Comment out cooperativeSettle & setTotalWithdraw temporarily #270

loredanacirstea commented Sep 17, 2018 •

edited

Loading

LefterisJP Sep 18, 2018

loredanacirstea Sep 19, 2018

LefterisJP Sep 18, 2018

LefterisJP Sep 18, 2018

loredanacirstea Sep 19, 2018

LefterisJP Sep 18, 2018

LefterisJP Sep 18, 2018

loredanacirstea Sep 19, 2018

loredanacirstea Sep 19, 2018

LefterisJP Sep 18, 2018

	/* event ChannelWithdraw(
	uint256 indexed channel_identifier,
	address indexed participant,
	uint256 total_withdraw
	); */

	# TODO uncomment this after setTotalWithdraw is uncommented in the TokenNetwork contract
	txn_hash = b''
	# txn_hash = token_network.functions.setTotalWithdraw(
	# channel_identifier,
	# participant,
	# withdraw_amount,
	# signature_participant,
	# signature_partner
	# ).transact({'from': delegate})

	# FIXME after setTotalWithdraw is implemented again
	post_balance_A = pre_balance_A + 33
	post_balance_B = pre_balance_B + 15
	post_balance_contract = pre_balance_contract - 48

	# FIXME after setTotalWithdraw is implemented again
	# we add the withdrawn amount here, because it was never withdrawn due to the
	# removal of setTotalWithdraw
	assert custom_token.functions.balanceOf(A).call() == post_balance_A + 10
	assert custom_token.functions.balanceOf(B).call() == post_balance_B + 5
	assert custom_token.functions.balanceOf(
	token_network.address,
	).call() == post_balance_contract - 15

	@pytest.mark.skip(reason='Delayed to another milestone')
	def test_recover_address_from_withdraw_message(

	@pytest.mark.skip(reason='Delayed to another milestone')
	def test_recover_address_from_cooperative_settle_signature(

	# FIXME after setTotalWithdraw is enabled again
	self.withdrawn = 0

		@@ -520,6 +528,7 @@ def test_cooperative_settle_channel_wrong_balances(
		).transact({'from': C})


		@pytest.mark.skip(reason='Delayed to another milestone')

Comment out cooperativeSettle & setTotalWithdraw temporarily #270

Comment out cooperativeSettle & setTotalWithdraw temporarily #270

Conversation

loredanacirstea commented Sep 17, 2018 • edited Loading

Changed in this PR, that should be changed back when re-enabling cooperative settle & withdraw

Contracts

Test contracts

Tests

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

loredanacirstea commented Sep 17, 2018 •

edited

Loading