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.

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

ensureConcluded fails when channel has been progressed to final #147

Closed
matthiasgeihs opened this issue Jul 22, 2021 · 2 comments · Fixed by #148
Closed

ensureConcluded fails when channel has been progressed to final #147

matthiasgeihs opened this issue Jul 22, 2021 · 2 comments · Fixed by #148

Comments

@matthiasgeihs
Copy link
Contributor

matthiasgeihs commented Jul 22, 2021

Location: backend/eth/conclude, contracts/Adjudicator.sol

Problem: When a channel has been force-update on-chain into a final state, the participants will not have fully signed states. Now, when calling ensureConcluded, it will call concludeFinal, which expects a fully-signed state and thus fails.

Idea: If a channel has been force-updated into final, the client should call conclude instead of concludeFinal, because conclude does not require signatures. The client must be able to detect whether a channel is in a final state on-chain. Usually, we indicate that by setting phase = CONCLUDED. Hence, I suggest that we change the contract such that if a state is force-updated to final, we automatically set phase = CONCLUDED.

Update: We can also apply a quick fix that doesn't require a change in the contracts. We can check whether the current on-chain phase is FORCE_EXECUTION. If this is the case, we know that the state resulted from force execution, which means we do not have the all signatures and need to call conclude instead of concludeFinal.

@ggwpez
Copy link
Contributor

ggwpez commented Jul 22, 2021

How did you detect this? Do you plan on adding a test for this case?
@matthiasgeihs

@matthiasgeihs
Copy link
Contributor Author

While working on the app-channel example.

It would probably make sense to add a test case, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants