Channel Withdraw#4130
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4130 +/- ##
===========================================
+ Coverage 80.92% 81.16% +0.24%
===========================================
Files 101 102 +1
Lines 12923 13366 +443
Branches 1983 2055 +72
===========================================
+ Hits 10458 10849 +391
- Misses 1869 1913 +44
- Partials 596 604 +8
Continue to review full report at Codecov.
|
This is done because we have introduced the WithdrawRequest and Withdraw messages which need to be processed deterministicly and in the order in which they were sent. This provides guarantees that a certain withdraw request / confirmation will be validated according to the latest balance proof previous to the withdraw message nonce
Also fixed typing and lint issues after rebase
konradkonrad
left a comment
konradkonrad
left a comment
There was a problem hiding this comment.
not fully through, but posting for visibility of comments. will continue from raiden/transfer/node.py
Removed the part which mentions exception propagation which doesn't really happen in the state machine.
konradkonrad
left a comment
konradkonrad
left a comment
There was a problem hiding this comment.
some docstrings still need fixes, other than that LGTM!
| block_hash = data["block_hash"] | ||
| channel_identifier = args["channel_identifier"] | ||
| token_network_address = event.originating_contract | ||
| participant = to_canonical_address(args["participant"]) |
There was a problem hiding this comment.
please, implement the sandwich encoding, values should be converted from external representation to internal at the boundaries. In this case the address should be decode at decode_event_to_internal.
| ) | ||
| raiden.handle_and_track_state_change(withdraw_statechange) | ||
|
|
||
| chain_state = views.state_from_raiden(raiden) |
There was a problem hiding this comment.
There is a race condition here, there is no synchronization among handle_and_track_state_change and state_from_raiden.
There was a problem hiding this comment.
Looking at the other blockchain event handlers, they all look pretty much the same so am not sure what i missed and what you mean by synchronisation? if i think i know what you're thinking, then that's something for another PR :)
| channel_identifier: ChannelID | ||
| participant: Address | ||
| total_withdraw: WithdrawAmount | ||
| nonce: Nonce |
There was a problem hiding this comment.
I think we should have an ADR explaining why withdraw messages have a nonce.
| if not merkletree: | ||
| return None, None | ||
|
|
||
| nonce = get_next_nonce(sender_end_state) |
There was a problem hiding this comment.
this would be better at events_for_expired_lock (Which would be better named as send_expired_lock)
There was a problem hiding this comment.
I will change the name of the function. But the other part of your comment is to generate a new nonce and pass it as a parameter? Thats contradicts what we're doing in create_sendlockedtransfer so i am not sure i want to do that.
| lock = transfer.lock | ||
| channel_state.our_state.balance_proof = transfer.balance_proof | ||
| channel_state.our_state.merkletree = merkletree | ||
| channel_state.our_state.nonce = transfer.balance_proof.nonce |
There was a problem hiding this comment.
This makes the update in create unnecessary.
There was a problem hiding this comment.
There's no update. create_sendlockedtransfer only generates a new nonce but doesn't set it to the state. We only set the nonce here.
| assert merkletree, "create_sendexpiredlock should return both message and merkle tree" | ||
| channel_state.our_state.merkletree = merkletree | ||
| channel_state.our_state.balance_proof = send_lock_expired.balance_proof | ||
| channel_state.our_state.nonce = send_lock_expired.balance_proof.nonce |
There was a problem hiding this comment.
This makes the update in create unnecessary.
There was a problem hiding this comment.
There's no update, only generate
| return TransitionResult(channel_state, events) | ||
|
|
||
|
|
||
| def handle_receive_withdraw( |
There was a problem hiding this comment.
maybe this should also have the _confirmation postfix?
| else: | ||
| end_state = channel_state.partner_state | ||
|
|
||
| end_state.total_withdraw = state_change.total_withdraw |
There was a problem hiding this comment.
This is not correct. E.g.:
- A requests withdraw from B with
total_withdraw=10 - B confirms withdraw for A with
total_withdraw=10 - A sends withdraw to the chain
- A requests withdraw from B with
total_withdraw=20
- The transaction for the first withdraw is confirmed, this will overwrite the
total_withdraw
- B confirms the
total_withdraw=20, A will wrongly reject
There was a problem hiding this comment.
The value confirmed on-chain and the value request off-chain must be separated.
There was a problem hiding this comment.
Also, the value has to increase, if the value is lower it should be ignored.
3 participants
Resolves #1498
TODO
GAS_REQUIRED_FOR_WITHDRAWWithdrawa confirmation forWithdrawRequestinstead of relying onProcessedtest_channel_withdraw