-
Notifications
You must be signed in to change notification settings - Fork 1
[WIP] Withdraw function #17
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
base: master
Are you sure you want to change the base?
Conversation
| _updateLCstate(_lcID, updateParams, _VCroot, _sigA, _sigI) | ||
| } | ||
|
|
||
| uint256 senderIdx = ( |
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.
This limits the ability of watchers to initiate withdrawals for financial or network hostages
| // certain that's either necessary or desierable. Possibly | ||
| // (probaby?) the `emit DidLCUpdateState(...)` should be moved to | ||
| // the `updateLCstate` function instead of being done here. | ||
| _updateLCstate(_lcID, updateParams, _VCroot, _sigA, _sigI) |
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.
updateLCState was an entry point to start a challenge to settle to consensus. I am not sure how making it internal and wrapping it in a non-consensus reaching function affects security.
| // channel's current sequence number. | ||
| function withdraw( | ||
| bytes32 _lcID, | ||
| uint256 _balance, |
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.
this balance is ambiguous, what does it represent, (EDIT) reading the code implies it is the amount you wish to remove, which i think, as you note later, will cause on-chain/off-chain sync issues
| because it will have `channel.state = N`, but the balances in the | ||
| channel will not match balances shown in signed state N. | ||
| Proposal: when there's a withdrawal, assume that there's a |
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.
This is a large assumption
| 1. Alice wants to withdraw 1 ETH | ||
| 2. She sends state `lc-state(N+1)` to Ingrid, where the amount she | ||
| wants to withdraw has been deducted from the state | ||
| 3. She calls `LedgerChannel.withdraw(...)` with `lc-state(N)` |
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.
how can you be sure LedgerChannel.withdraw(...) is called with exactly the latest two states? What would this do to byzantine situations?
| `channel.sequence += 1` so its internal sequence number will be | ||
| `N + 1`, and its internal state will match `lc-state(N+1)` | ||
| This would also imply that the `deposit(...)` funciton should be |
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 only reason we have considered a deposit function safe to my knowledge is because you can only lose track off money off-chain (additive updates will only hurt abusers or glitches), rather than allow someone to possibly steal it as is with withdrawals
See comments in the code for proposal and questions