sdk/state: refactor validations to check against the channel state #270
Conversation
acharb
changed the title
| if err != nil { | ||
| return fmt.Errorf("getting channel state: %w", err) | ||
| } | ||
| if cs >= StateOpen { |
There was a problem hiding this comment.
I think this is better than if cs != StateNone, in the case the channel errored on the first attempt to open, so they should be allowed to try again
There was a problem hiding this comment.
in the case the channel errored on the first attempt to open, so they should be allowed to try again
❗ I think there could be cases where StateFailed is the state returned but the current openAgreement could still be valid. So I'm don't think we should allow trying again and consider the state.Channel one shot. It could be functionality up the stack in the Agent or higher that allows retrying, by destroying the state.Channel and creating a new one.
There was a problem hiding this comment.
I left some ideas (💡), concerns (🤔), notes about things that need addressing (❗). Because I raised some concerns about using the state check in some places, I explicitly marked the places where the state looks good (👍🏻).
The improvements to the tests look great. 🎉
| if cs >= StateOpen { | ||
| return OpenAgreement{}, fmt.Errorf("cannot propose a new open if channel has already opened") |
There was a problem hiding this comment.
❗ I think we're missing catching StateFailed? StateFailed is < StateOpen so will not be caught here and so the open will be allowed. I think we should err on the side of allowing only valid scenarios, so:
| if cs >= StateOpen { | |
| return OpenAgreement{}, fmt.Errorf("cannot propose a new open if channel has already opened") | |
| if cs != StateNone { | |
| return OpenAgreement{}, fmt.Errorf("cannot propose a new open if channel has already opened") |
🤔 Similar to my other two comments above I think this changes the condition to be a little more disconnected from the needs of the function. In this case the function is protecting against replacing an c.openAgreement that already exists. We're replacing that protective guard with one about the state, that indirectly protects against replacing the openAgreement.
❗ Note that there's actually a bug in this function in the original code. The original condition should have been !c.openAgreement.isEmpty() because we should never allow proposing two opens because that could result in us losing control of the channel. If we make that fix I don't think we can reproduce that condition with the state check anymore, because the state would be StateNone, so I'm thinking we need to keep the open agreement condition instead, or maybe 💡 we need more states, such as StateOpenProposed, StateOpenConfirmed. That might not be a bad idea.
For example:
StateFailedStateOpenProposed= only one signer on the openStateOpenConfirmed= two signers, not ingestedStateOpened= ingested, actually opened on networkStateClosingStateClosed
❓ Wdyt about keeping the original condition and adding the state condition with new states? The original condition is defensive to make sure we don't replace data we need, or that data exists before we use it. The state condition is more informative.
| cs, err := c.State() | ||
| if err != nil { | ||
| return CloseAgreement{}, fmt.Errorf("getting channel state: %w", err) | ||
| } | ||
| if cs < StateOpen { | ||
| return CloseAgreement{}, fmt.Errorf("cannot propose a coordinated close before channel is opened") |
There was a problem hiding this comment.
🤔 Looking back on this guard, the comment says to check the channel is open, and the same with the error, but I think the condition that was there before was more valuable and more precise than the new condition on the state. This function needs a latestAuthorizedCloseAgreement, and so the guard that was here was to make sure that existed. In some ways the guard has become less specific, and a little disconnected from what the function needs.
This makes me think we should keep the existing guard but change the error. For example:
| cs, err := c.State() | |
| if err != nil { | |
| return CloseAgreement{}, fmt.Errorf("getting channel state: %w", err) | |
| } | |
| if cs < StateOpen { | |
| return CloseAgreement{}, fmt.Errorf("cannot propose a coordinated close before channel is opened") | |
| if c.latestAuthorizedCloseAgreement.isEmpty() { | |
| return CloseAgreement{}, fmt.Errorf("cannot propose a coordinated close without an authorized close agreement to amend") |
This makes the requirements of the function clear, albeit leaks some details to the user that aren't helpful since it would be more helpful to tell the user that the channel isn't open, in which case maybe the existing error is fine.
Wdyt?
Wdyt of my suggestion in the comment above?
There was a problem hiding this comment.
🤔 in my head I was thinking that, by checking the channel is in the correct state for proposing/confirming a close, then we're checking the prerequisites for doing the action are completed. So in this case checking the state means we're also checking the channel went through the open steps, latestAuthorizedAgreement looks good (or at least it's not empty), and sequence number is in the right place. I liked that better than explicitly checking the data for these propose/confirm methods.
I guess it's the difference of checking the data explicitly vs an abstracted method here, which is more disconnected 🤔 . wdyt, I might not be thinking of something so if you think the former is better I can change back here
| if c.latestAuthorizedCloseAgreement.isEmpty() { | ||
| cs, err := c.State() | ||
| if err != nil { | ||
| return fmt.Errorf("getting channel state: %w", err) | ||
| } | ||
| if cs < StateOpen { | ||
| return fmt.Errorf("cannot confirm a coordinated close before channel is opened") | ||
| } | ||
|
|
There was a problem hiding this comment.
🤔 The same thing here too. This guard on c.latestAuthorizedCloseAgreement protects against our use of that value immediately following, but that direct relation is lost with a change to a guard on the state.
| if err != nil { | ||
| return fmt.Errorf("getting channel state: %w", err) | ||
| } | ||
| if cs >= StateOpen { |
There was a problem hiding this comment.
in the case the channel errored on the first attempt to open, so they should be allowed to try again
❗ I think there could be cases where StateFailed is the state returned but the current openAgreement could still be valid. So I'm don't think we should allow trying again and consider the state.Channel one shot. It could be functionality up the stack in the Agent or higher that allows retrying, by destroying the state.Channel and creating a new one.
| if c.latestAuthorizedCloseAgreement.isEmpty() { | ||
| cs, err := c.State() | ||
| if err != nil { | ||
| return CloseAgreement{}, fmt.Errorf("getting channel state: %w", err) | ||
| } | ||
| if cs != StateOpen { |
There was a problem hiding this comment.
👍🏻 I think this change to using state is good.
| if c.latestAuthorizedCloseAgreement.isEmpty() { | ||
| cs, err := c.State() | ||
| if err != nil { | ||
| return fmt.Errorf("getting channel state: %w", err) | ||
| } | ||
| if cs < StateOpen { |
There was a problem hiding this comment.
👍🏻 I think using the state here is good, however I think the condition needs to be cs != StateOpen, because any non-open state should not be supported here.
| ResponderSigner: remoteSigner.Address(), | ||
| InitiatorEscrow: localEscrowAccount.Address.Address(), | ||
| ResponderEscrow: remoteEscrowAccount.Address.Address(), | ||
| StartSequence: localEscrowAccount.SequenceNumber + 1, |
There was a problem hiding this comment.
💡
| StartSequence: localEscrowAccount.SequenceNumber + 1, | |
| StartSequence: ftx.SequenceNumber(), |
| require.NoError(t, err) | ||
| err = remoteChannel.IngestTx(ftxXDR, successResultXDR, resultMetaXDR) | ||
| require.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
💡 The improvements to the tests look great. The fact that so many places in the tests needed updating for the state to be accurate, I think it would be worth adding into every test that was changed assertions on the result of calling channel.State(), that way these tests will ensure that the state value returned from the function is consistent over time. Wdyt?
|
note: moved the test improvements to this PR: #277 to use the channel.State for validations it looks like we'll need to add more state values, to ensure we're validating at a granular enough level. Closing this PR for now as that will take some additional thought and possible refactoring |
WHAT
changes the validations for checking if the channel is open to use the Channel State instead of checking the openAgreement is empty or not
WHY
the channel state better reflects if the channel is actually open based off of ingested transactions. And more accurately gives the state of the channel.
closes #261