-
Notifications
You must be signed in to change notification settings - Fork 44
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.
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
Comment out cooperativeSettle & setTotalWithdraw temporarily #270
Changes from all commits
cd6a34b
acf3717
fa18e45
864f1a7
efbd0af
d47aa6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -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 | ||
/// 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 | ||
|
@@ -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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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. | ||
|
@@ -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, | ||
|
@@ -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, | ||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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( | ||
|
@@ -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, | ||
|
@@ -333,11 +343,24 @@ def test_settlement_with_unauthorized_token_transfer( | |
vals_B, | ||
) | ||
|
||
# Assign additional tokens to A | ||
assign_tokens(A, externally_transferred_amount) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the test was completely wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
||
|
@@ -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( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,9 @@ def __init__( | |
additional_hash=EMPTY_ADDITIONAL_HASH, | ||
): | ||
self.deposit = deposit | ||
self.withdrawn = withdrawn | ||
# FIXME after setTotalWithdraw is enabled again | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.