Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Comment out cooperativeSettle & setTotalWithdraw temporarily #270

Merged
merged 6 commits into from
Sep 20, 2018
Merged

Comment out cooperativeSettle & setTotalWithdraw temporarily #270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cd6a34b
Comment out cooperativeSettle & skip tests - to be added later
loredanacirstea Sep 17, 2018
acf3717
Comment out setTotalWithdraw & skip tests - to be added later
loredanacirstea Sep 17, 2018
fa18e45
Skip another withdraw signature test
loredanacirstea Sep 18, 2018
864f1a7
Travis skip message fix (pr review)
loredanacirstea Sep 18, 2018
efbd0af
Fix failing test after setTotalWithdraw removal
loredanacirstea Sep 19, 2018
d47aa6b
Add more balance checks for test_settlement_with_unauthorized_token_t…
loredanacirstea Sep 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions raiden_contracts/contracts/TokenNetwork.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,11 @@ contract TokenNetwork is Utils {
// total_withdraw is how much the participant has withdrawn during the
// lifetime of the channel. The actual amount which the participant withdrew
// is `total_withdraw - total_withdraw_from_previous_event_or_zero`
event ChannelWithdraw(
/* event ChannelWithdraw(
uint256 indexed channel_identifier,
address indexed participant,
uint256 total_withdraw
);
); */

event ChannelClosed(
uint256 indexed channel_identifier,
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// has with `partner`, without closing it. Can be called by anyone. Can
/// only be called once per each signed withdraw message.
/// @param channel_identifier Identifier for the channel on which this
Expand Down Expand Up @@ -422,7 +422,7 @@ contract TokenNetwork is Utils {
assert(participant_state.nonce == 0);
assert(partner_state.nonce == 0);

}
} */

/// @notice Close the channel defined by the two participant addresses. Only
/// a participant may close the channel, providing a balance proof signed by
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

/// participants and transfers the agreed upon token amounts to the
/// participants. After this the channel lifecycle has ended and no more
/// operations can be done on it.
Expand Down Expand Up @@ -908,7 +908,7 @@ contract TokenNetwork is Utils {
if (participant2_balance > 0) {
require(token.transfer(participant2, participant2_balance));
}
}
} */

/// @notice Returns the unique identifier for the channel given by the
/// contract.
Expand Down Expand Up @@ -1438,7 +1438,7 @@ contract TokenNetwork is Utils {
signature_address = ECVerify.ecverify(message_hash, non_closing_signature);
}

function recoverAddressFromCooperativeSettleSignature(
/* function recoverAddressFromCooperativeSettleSignature(
uint256 channel_identifier,
address participant1,
uint256 participant1_balance,
Expand Down Expand Up @@ -1467,9 +1467,9 @@ contract TokenNetwork is Utils {
));

signature_address = ECVerify.ecverify(message_hash, signature);
}
} */

function recoverAddressFromWithdrawMessage(
/* function recoverAddressFromWithdrawMessage(
uint256 channel_identifier,
address participant,
uint256 total_withdraw,
Expand All @@ -1494,7 +1494,7 @@ contract TokenNetwork is Utils {
));

signature_address = ECVerify.ecverify(message_hash, signature);
}
} */

/// @dev Calculates the merkle root for the pending transfers data and
//calculates the amount / of tokens that can be unlocked because the secret
Expand Down
8 changes: 4 additions & 4 deletions raiden_contracts/contracts/test/TokenNetworkInternalsTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ contract TokenNetworkSignatureTest is TokenNetwork {
);
}

function recoverAddressFromCooperativeSettleSignaturePublic(
/* function recoverAddressFromCooperativeSettleSignaturePublic(
uint256 channel_identifier,
address participant1,
uint256 participant1_balance,
Expand All @@ -219,9 +219,9 @@ contract TokenNetworkSignatureTest is TokenNetwork {
participant2_balance,
signature
);
}
} */

function recoverAddressFromWithdrawMessagePublic(
/* function recoverAddressFromWithdrawMessagePublic(
uint256 channel_identifier,
address participant,
uint256 amount_to_withdraw,
Expand All @@ -237,7 +237,7 @@ contract TokenNetworkSignatureTest is TokenNetwork {
amount_to_withdraw,
signature
);
}
} */
}

contract TokenNetworkUtilsTest is TokenNetwork {
Expand Down
16 changes: 9 additions & 7 deletions raiden_contracts/tests/fixtures/channel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,15 @@ def get(channel_identifier, participant, withdraw_amount, partner, delegate=None
channel_identifier,
participant, withdraw_amount
)
txn_hash = token_network.functions.setTotalWithdraw(
channel_identifier,
participant,
withdraw_amount,
signature_participant,
signature_partner
).transact({'from': delegate})
# TODO uncomment this after setTotalWithdraw is uncommented in the TokenNetwork contract
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

txn_hash = b''
# txn_hash = token_network.functions.setTotalWithdraw(
# channel_identifier,
# participant,
# withdraw_amount,
# signature_participant,
# signature_partner
# ).transact({'from': delegate})
return txn_hash
return get

Expand Down
10 changes: 10 additions & 0 deletions raiden_contracts/tests/test_channel_cooperative_settle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from web3.exceptions import ValidationError


@pytest.mark.skip(reason='Delayed until another milestone')
def test_cooperative_settle_channel_call(
token_network,
create_channel_and_deposit,
Expand Down Expand Up @@ -123,6 +124,7 @@ def test_cooperative_settle_channel_call(
).transact({'from': C})


@pytest.mark.skip(reason='Delayed until another milestone')
def test_cooperative_settle_channel_signatures(
token_network,
create_channel_and_deposit,
Expand Down Expand Up @@ -188,6 +190,7 @@ def test_cooperative_settle_channel_signatures(
).transact({'from': C})


@pytest.mark.skip(reason='Delayed until another milestone')
def test_cooperative_settle_channel_0(
custom_token,
token_network,
Expand Down Expand Up @@ -239,6 +242,7 @@ def test_cooperative_settle_channel_0(
)


@pytest.mark.skip(reason='Delayed until another milestone')
def test_cooperative_settle_channel_00(
custom_token,
token_network,
Expand Down Expand Up @@ -290,6 +294,7 @@ def test_cooperative_settle_channel_00(
)


@pytest.mark.skip(reason='Delayed until another milestone')
def test_cooperative_settle_channel_state(
custom_token,
token_network,
Expand Down Expand Up @@ -342,6 +347,7 @@ def test_cooperative_settle_channel_state(
)


@pytest.mark.skip(reason='Delayed until another milestone')
def test_cooperative_settle_channel_state_withdraw(
custom_token,
token_network,
Expand Down Expand Up @@ -399,6 +405,7 @@ def test_cooperative_settle_channel_state_withdraw(
)


@pytest.mark.skip(reason='Delayed until another milestone')
def test_cooperative_settle_channel_bigger_withdraw(
custom_token,
token_network,
Expand Down Expand Up @@ -442,6 +449,7 @@ def test_cooperative_settle_channel_bigger_withdraw(
).transact({'from': C})


@pytest.mark.skip(reason='Delayed until another milestone')
def test_cooperative_settle_channel_wrong_balances(
custom_token,
token_network,
Expand Down Expand Up @@ -520,6 +528,7 @@ def test_cooperative_settle_channel_wrong_balances(
).transact({'from': C})


@pytest.mark.skip(reason='Delayed until another milestone')
def test_cooperative_close_replay_reopened_channel(
get_accounts,
token_network,
Expand Down Expand Up @@ -593,6 +602,7 @@ def test_cooperative_close_replay_reopened_channel(
).transact({'from': B})


@pytest.mark.skip(reason='Delayed until another milestone')
def test_cooperative_settle_channel_event(
get_accounts,
token_network,
Expand Down
45 changes: 37 additions & 8 deletions raiden_contracts/tests/test_channel_settle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,20 @@ def test_settle_channel_state(

# Some manual checks for the final balances, in case the settlement algorithms
# used in `settle_state_tests` are incorrect
assert custom_token.functions.balanceOf(A).call() == pre_balance_A + 33
assert custom_token.functions.balanceOf(B).call() == pre_balance_B + 15

# 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() == pre_balance_contract - 48
).call() == post_balance_contract - 15


def test_settle_single_direct_transfer_for_closing_party(
Expand Down Expand Up @@ -307,6 +316,7 @@ def test_settlement_with_unauthorized_token_transfer(
get_accounts,
custom_token,
token_network,
assign_tokens,
create_channel_and_deposit,
withdraw_channel,
close_and_update_channel,
Expand All @@ -333,11 +343,24 @@ def test_settlement_with_unauthorized_token_transfer(
vals_B,
)

# Assign additional tokens to A
assign_tokens(A, externally_transferred_amount)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the additional checks in dab0a9a

assert custom_token.functions.balanceOf(A).call() >= externally_transferred_amount

# Fetch onchain balances after settlement
pre_balance_A = custom_token.functions.balanceOf(A).call()
pre_balance_B = custom_token.functions.balanceOf(B).call()
pre_balance_contract = custom_token.functions.balanceOf(token_network.address).call()

# A does a transfer to the token_network without appropriate function call - tokens are lost
custom_token.functions.transfer(
token_network.address,
externally_transferred_amount,
).transact({'from': A})
assert custom_token.functions.balanceOf(token_network.address).call() == (
pre_balance_contract +
externally_transferred_amount
)

web3.testing.mine(TEST_SETTLE_TIMEOUT_MIN)

Expand All @@ -354,15 +377,21 @@ def test_settlement_with_unauthorized_token_transfer(

# A has lost the externally_transferred_amount
assert (
vals_A.withdrawn + settlement.participant1_balance - externally_transferred_amount
== post_balance_A
)
pre_balance_A +
settlement.participant1_balance -
externally_transferred_amount
) == post_balance_A

# B's settlement works correctly
assert (settlement.participant2_balance + vals_B.withdrawn == post_balance_B)
assert pre_balance_B + settlement.participant2_balance == post_balance_B

# The externally_transferred_amount stays in the contract
assert (post_balance_contract == externally_transferred_amount)
assert (
pre_balance_contract -
settlement.participant1_balance -
settlement.participant2_balance +
externally_transferred_amount
) == post_balance_contract


def test_settle_with_locked_but_unregistered(
Expand Down
8 changes: 8 additions & 0 deletions raiden_contracts/tests/test_channel_withdraw.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
)


@pytest.mark.skip(reason='Delayed until another milestone')
def test_withdraw_call(
token_network,
create_channel_and_deposit,
Expand Down Expand Up @@ -109,6 +110,7 @@ def test_withdraw_call(
).transact({'from': A})


@pytest.mark.skip(reason='Delayed until another milestone')
def test_withdraw_wrong_state(
web3,
token_network,
Expand Down Expand Up @@ -161,6 +163,7 @@ def test_withdraw_wrong_state(
withdraw_channel(channel_identifier, A, withdraw_A, B)


@pytest.mark.skip(reason='Delayed until another milestone')
def test_withdraw_bigger(
web3,
token_network,
Expand Down Expand Up @@ -189,6 +192,7 @@ def test_withdraw_bigger(
withdraw_channel(channel_identifier, A, deposit_A + deposit_B - 7, B)


@pytest.mark.skip(reason='Delayed until another milestone')
def test_withdraw_wrong_signers(
web3,
token_network,
Expand Down Expand Up @@ -235,6 +239,7 @@ def test_withdraw_wrong_signers(
).transact({'from': C})


@pytest.mark.skip(reason='Delayed until another milestone')
def test_withdraw_wrong_signature_content(
web3,
token_network,
Expand Down Expand Up @@ -333,6 +338,7 @@ def test_withdraw_wrong_signature_content(
).transact({'from': A})


@pytest.mark.skip(reason='Delayed until another milestone')
def test_withdraw_channel_state(
get_accounts,
token_network,
Expand Down Expand Up @@ -421,6 +427,7 @@ def test_withdraw_channel_state(
)


@pytest.mark.skip(reason='Delayed until another milestone')
def test_withdraw_replay_reopened_channel(
web3,
token_network,
Expand Down Expand Up @@ -500,6 +507,7 @@ def test_withdraw_replay_reopened_channel(
).transact({'from': A})


@pytest.mark.skip(reason='Delayed until another milestone')
def test_withdraw_event(
token_network,
create_channel_and_deposit,
Expand Down
2 changes: 2 additions & 0 deletions raiden_contracts/tests/unit/test_unit_signatures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from raiden_contracts.tests.fixtures import fake_bytes


@pytest.mark.skip(reason='Delayed to another milestone')
def test_recover_address_from_withdraw_message(
token_network_test_signatures,
create_withdraw_signatures,
Expand Down Expand Up @@ -169,6 +170,7 @@ def test_recover_address_from_balance_proof_update(
).call()


@pytest.mark.skip(reason='Delayed to another milestone')
def test_recover_address_from_cooperative_settle_signature(
token_network_test_signatures,
create_cooperative_settle_signatures,
Expand Down
4 changes: 3 additions & 1 deletion raiden_contracts/tests/utils/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ def __init__(
additional_hash=EMPTY_ADDITIONAL_HASH,
):
self.deposit = deposit
self.withdrawn = withdrawn
# FIXME after setTotalWithdraw is enabled again
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.withdrawn = 0
# self.withdrawn = withdrawn
self.nonce = nonce
self.transferred = transferred
self.claimable_locked = claimable_locked or locked
Expand Down