Skip to content
This repository has been archived by the owner on Apr 23, 2024. It is now read-only.

sdk/state: refactor validations to check against the channel state #270

Closed
wants to merge 15 commits into from
Closed

sdk/state: refactor validations to check against the channel state #270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
105eed0
first cut
acharb Aug 30, 2021
f04f586
Merge branch 'main' into acharb-refactor-validate
acharb Aug 31, 2021
7da38ad
Merge branch 'main' into acharb-refactor-validate
acharb Aug 31, 2021
809433e
fix unit tests
acharb Aug 31, 2021
4c1636a
add to close functions, and fix test
acharb Aug 31, 2021
860dcf7
add formation tx xdr helper, fix integration tests
acharb Sep 1, 2021
bcd9fc4
check if state is statenone
acharb Sep 1, 2021
3e69491
change back
acharb Sep 1, 2021
9fd645b
fix unit test
acharb Sep 1, 2021
1a33b01
make putting the channel into the Open state in tests consistent
acharb Sep 1, 2021
b1e52be
missed some
acharb Sep 1, 2021
687c88a
cleanup
acharb Sep 1, 2021
9665200
cleanup
acharb Sep 1, 2021
27032bb
cleanup
acharb Sep 1, 2021
25bb784
fix
acharb Sep 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions sdk/state/close.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,11 @@ func (c *Channel) ProposeClose() (CloseAgreement, error) {
}

// If the channel is not open yet, error.
if c.latestAuthorizedCloseAgreement.isEmpty() {
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")
Comment on lines +96 to 101
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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:

Suggested change
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?

Copy link
Contributor Author

@acharb acharb Sep 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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

}

Expand Down Expand Up @@ -122,9 +126,14 @@ func (c *Channel) ProposeClose() (CloseAgreement, error) {

func (c *Channel) validateClose(ca CloseAgreement) error {
// If the channel is not open yet, error.
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")
}

Comment on lines -125 to +136
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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 ca.Details.IterationNumber != c.latestAuthorizedCloseAgreement.Details.IterationNumber {
return fmt.Errorf("close agreement iteration number does not match saved latest authorized close agreement")
}
Expand Down
47 changes: 37 additions & 10 deletions sdk/state/close_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ import (
"testing"
"time"

"github.com/stellar/experimental-payment-channels/sdk/txbuildtest"
"github.com/stellar/go/keypair"
"github.com/stellar/go/network"
"github.com/stellar/go/txnbuild"
"github.com/stellar/go/xdr"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -107,16 +109,41 @@ func TestChannel_ProposeClose(t *testing.T) {
MaxOpenExpiry: 2 * time.Hour,
})

open1, err := localChannel.ProposeOpen(OpenParams{
ObservationPeriodTime: 1,
ObservationPeriodLedgerGap: 1,
ExpiresAt: time.Now().Add(time.Hour),
})
require.NoError(t, err)
open2, err := remoteChannel.ConfirmOpen(open1)
require.NoError(t, err)
_, err = localChannel.ConfirmOpen(open2)
require.NoError(t, err)
// Put channel into the Open state.
{
open1, err := localChannel.ProposeOpen(OpenParams{
ObservationPeriodTime: 1,
ObservationPeriodLedgerGap: 1,
ExpiresAt: time.Now().Add(time.Hour),
})
require.NoError(t, err)
open2, err := remoteChannel.ConfirmOpen(open1)
require.NoError(t, err)
_, err = localChannel.ConfirmOpen(open2)
require.NoError(t, err)

ftx, err := localChannel.OpenTx()
require.NoError(t, err)
ftxXDR, err := ftx.Base64()
require.NoError(t, err)

successResultXDR, err := txbuildtest.BuildResultXDR(true)
require.NoError(t, err)
resultMetaXDR, err := txbuildtest.BuildFormationResultMetaXDR(txbuildtest.FormationResultMetaParams{
InitiatorSigner: localSigner.Address(),
ResponderSigner: remoteSigner.Address(),
InitiatorEscrow: localEscrowAccount.Address.Address(),
ResponderEscrow: remoteEscrowAccount.Address.Address(),
StartSequence: localEscrowAccount.SequenceNumber + 1,
Asset: txnbuild.NativeAsset{},
})
require.NoError(t, err)

err = localChannel.IngestTx(ftxXDR, successResultXDR, resultMetaXDR)
require.NoError(t, err)
err = remoteChannel.IngestTx(ftxXDR, successResultXDR, resultMetaXDR)
require.NoError(t, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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?


// If the local proposes a close, the agreement will have them as the proposer.
closeByLocal, err := localChannel.ProposeClose()
Expand Down
6 changes: 6 additions & 0 deletions sdk/state/ingest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ func TestChannel_IngestTx_latestUnauthorizedDeclTxViaFeeBump(t *testing.T) {

// Mock initiatorChannel ingested formation tx successfully.
initiatorChannel.openExecutedAndValidated = true
initiatorChannel.setInitiatorEscrowAccountSequence(initiatorEscrowAccount.SequenceNumber + 1)
responderChannel.openExecutedAndValidated = true

// To prevent xdr parsing error.
placeholderXDR := "AAAAAgAAAAIAAAADABArWwAAAAAAAAAAWPnYf+6kQN3t44vgesQdWh4JOOPj7aer852I7RJhtzAAAAAWg8TZOwANrPwAAAAKAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAABABArWwAAAAAAAAAAWPnYf+6kQN3t44vgesQdWh4JOOPj7aer852I7RJhtzAAAAAWg8TZOwANrPwAAAALAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAABAAAABAAAAAMAD/39AAAAAAAAAAD49aUpVx7fhJPK6wDdlPJgkA1HkAi85qUL1tii8YSZzQAAABdjSVwcAA/8sgAAAAEAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAEAECtbAAAAAAAAAAD49aUpVx7fhJPK6wDdlPJgkA1HkAi85qUL1tii8YSZzQAAABee5CYcAA/8sgAAAAEAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAMAECtbAAAAAAAAAABY+dh/7qRA3e3ji+B6xB1aHgk44+Ptp6vznYjtEmG3MAAAABaDxNk7AA2s/AAAAAsAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAIAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAEAECtbAAAAAAAAAABY+dh/7qRA3e3ji+B6xB1aHgk44+Ptp6vznYjtEmG3MAAAABZIKg87AA2s/AAAAAsAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAIAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAA="
Expand Down Expand Up @@ -153,6 +155,8 @@ func TestChannel_IngestTx_latestUnauthorizedDeclTx(t *testing.T) {

// Mock initiatorChannel ingested formation tx successfully.
initiatorChannel.openExecutedAndValidated = true
initiatorChannel.setInitiatorEscrowAccountSequence(initiatorEscrowAccount.SequenceNumber + 1)
responderChannel.openExecutedAndValidated = true

// To prevent xdr parsing error.
placeholderXDR := "AAAAAgAAAAIAAAADABArWwAAAAAAAAAAWPnYf+6kQN3t44vgesQdWh4JOOPj7aer852I7RJhtzAAAAAWg8TZOwANrPwAAAAKAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAABABArWwAAAAAAAAAAWPnYf+6kQN3t44vgesQdWh4JOOPj7aer852I7RJhtzAAAAAWg8TZOwANrPwAAAALAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAABAAAABAAAAAMAD/39AAAAAAAAAAD49aUpVx7fhJPK6wDdlPJgkA1HkAi85qUL1tii8YSZzQAAABdjSVwcAA/8sgAAAAEAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAEAECtbAAAAAAAAAAD49aUpVx7fhJPK6wDdlPJgkA1HkAi85qUL1tii8YSZzQAAABee5CYcAA/8sgAAAAEAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAMAECtbAAAAAAAAAABY+dh/7qRA3e3ji+B6xB1aHgk44+Ptp6vznYjtEmG3MAAAABaDxNk7AA2s/AAAAAsAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAIAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAEAECtbAAAAAAAAAABY+dh/7qRA3e3ji+B6xB1aHgk44+Ptp6vznYjtEmG3MAAAABZIKg87AA2s/AAAAAsAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAIAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAA="
Expand Down Expand Up @@ -294,6 +298,8 @@ func TestChannel_IngestTx_oldDeclTx(t *testing.T) {

// Mock initiatorChannel ingested formation tx successfully.
initiatorChannel.openExecutedAndValidated = true
initiatorChannel.setInitiatorEscrowAccountSequence(initiatorEscrowAccount.SequenceNumber + 1)
responderChannel.openExecutedAndValidated = true

// To prevent xdr parsing error.
placeholderXDR := "AAAAAgAAAAIAAAADABArWwAAAAAAAAAAWPnYf+6kQN3t44vgesQdWh4JOOPj7aer852I7RJhtzAAAAAWg8TZOwANrPwAAAAKAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAABABArWwAAAAAAAAAAWPnYf+6kQN3t44vgesQdWh4JOOPj7aer852I7RJhtzAAAAAWg8TZOwANrPwAAAALAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAABAAAABAAAAAMAD/39AAAAAAAAAAD49aUpVx7fhJPK6wDdlPJgkA1HkAi85qUL1tii8YSZzQAAABdjSVwcAA/8sgAAAAEAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAEAECtbAAAAAAAAAAD49aUpVx7fhJPK6wDdlPJgkA1HkAi85qUL1tii8YSZzQAAABee5CYcAA/8sgAAAAEAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAMAECtbAAAAAAAAAABY+dh/7qRA3e3ji+B6xB1aHgk44+Ptp6vznYjtEmG3MAAAABaDxNk7AA2s/AAAAAsAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAIAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAEAECtbAAAAAAAAAABY+dh/7qRA3e3ji+B6xB1aHgk44+Ptp6vznYjtEmG3MAAAABZIKg87AA2s/AAAAAsAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAIAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAA="
Expand Down
126 changes: 126 additions & 0 deletions sdk/state/integrationtests/state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"time"

"github.com/stellar/experimental-payment-channels/sdk/state"
"github.com/stellar/experimental-payment-channels/sdk/txbuildtest"
"github.com/stellar/go/amount"
"github.com/stellar/go/clients/horizonclient"
"github.com/stellar/go/keypair"
Expand Down Expand Up @@ -108,6 +109,31 @@ func TestOpenUpdatesUncoordinatedClose(t *testing.T) {
require.NoError(t, err)
}

{
t.Log("Initiator and Responder channels ingest the formation tx ...")
ftx, err := initiatorChannel.OpenTx()
require.NoError(t, err)
ftxXDR, err := ftx.Base64()
require.NoError(t, err)

successResultXDR, err := txbuildtest.BuildResultXDR(true)
require.NoError(t, err)
resultMetaXDR, err := txbuildtest.BuildFormationResultMetaXDR(txbuildtest.FormationResultMetaParams{
InitiatorSigner: initiator.KP.Address(),
ResponderSigner: responder.KP.Address(),
InitiatorEscrow: initiator.Escrow.Address(),
ResponderEscrow: responder.Escrow.Address(),
StartSequence: s,
Asset: txnbuild.NativeAsset{},
})
require.NoError(t, err)

err = initiatorChannel.IngestTx(ftxXDR, successResultXDR, resultMetaXDR)
require.NoError(t, err)
err = responderChannel.IngestTx(ftxXDR, successResultXDR, resultMetaXDR)
require.NoError(t, err)
}

t.Log("Iteration", i, "Declarations:", txSeqs(declarationTxs))
t.Log("Iteration", i, "Closes:", txSeqs(closeTxs))

Expand Down Expand Up @@ -343,6 +369,31 @@ func TestOpenUpdatesCoordinatedCloseStartCloseThenCoordinate(t *testing.T) {
require.NoError(t, err)
}

{
t.Log("Initiator and Responder channels ingest the formation tx ...")
ftx, err := initiatorChannel.OpenTx()
require.NoError(t, err)
ftxXDR, err := ftx.Base64()
require.NoError(t, err)

successResultXDR, err := txbuildtest.BuildResultXDR(true)
require.NoError(t, err)
resultMetaXDR, err := txbuildtest.BuildFormationResultMetaXDR(txbuildtest.FormationResultMetaParams{
InitiatorSigner: initiator.KP.Address(),
ResponderSigner: responder.KP.Address(),
InitiatorEscrow: initiator.Escrow.Address(),
ResponderEscrow: responder.Escrow.Address(),
StartSequence: s,
Asset: txnbuild.CreditAsset{Code: asset.Code(), Issuer: asset.Issuer()},
})
require.NoError(t, err)

err = initiatorChannel.IngestTx(ftxXDR, successResultXDR, resultMetaXDR)
require.NoError(t, err)
err = responderChannel.IngestTx(ftxXDR, successResultXDR, resultMetaXDR)
require.NoError(t, err)
}

// Update balances known for each other.
initiatorChannel.UpdateRemoteEscrowAccountBalance(responder.Contribution)
responderChannel.UpdateRemoteEscrowAccountBalance(initiator.Contribution)
Expand Down Expand Up @@ -524,6 +575,31 @@ func TestOpenUpdatesCoordinatedCloseCoordinateThenStartClose(t *testing.T) {
require.NoError(t, err)
}

{
t.Log("Initiator and Responder channels ingest the formation tx ...")
ftx, err := initiatorChannel.OpenTx()
require.NoError(t, err)
ftxXDR, err := ftx.Base64()
require.NoError(t, err)

successResultXDR, err := txbuildtest.BuildResultXDR(true)
require.NoError(t, err)
resultMetaXDR, err := txbuildtest.BuildFormationResultMetaXDR(txbuildtest.FormationResultMetaParams{
InitiatorSigner: initiator.KP.Address(),
ResponderSigner: responder.KP.Address(),
InitiatorEscrow: initiator.Escrow.Address(),
ResponderEscrow: responder.Escrow.Address(),
StartSequence: s,
Asset: txnbuild.CreditAsset{Code: asset.Code(), Issuer: asset.Issuer()},
})
require.NoError(t, err)

err = initiatorChannel.IngestTx(ftxXDR, successResultXDR, resultMetaXDR)
require.NoError(t, err)
err = responderChannel.IngestTx(ftxXDR, successResultXDR, resultMetaXDR)
require.NoError(t, err)
}

// Update balances known for each other.
initiatorChannel.UpdateRemoteEscrowAccountBalance(responder.Contribution)
responderChannel.UpdateRemoteEscrowAccountBalance(initiator.Contribution)
Expand Down Expand Up @@ -705,6 +781,31 @@ func TestOpenUpdatesCoordinatedCloseCoordinateThenStartCloseByRemote(t *testing.
require.NoError(t, err)
}

{
t.Log("Initiator and Responder channels ingest the formation tx ...")
ftx, err := initiatorChannel.OpenTx()
require.NoError(t, err)
ftxXDR, err := ftx.Base64()
require.NoError(t, err)

successResultXDR, err := txbuildtest.BuildResultXDR(true)
require.NoError(t, err)
resultMetaXDR, err := txbuildtest.BuildFormationResultMetaXDR(txbuildtest.FormationResultMetaParams{
InitiatorSigner: initiator.KP.Address(),
ResponderSigner: responder.KP.Address(),
InitiatorEscrow: initiator.Escrow.Address(),
ResponderEscrow: responder.Escrow.Address(),
StartSequence: s,
Asset: txnbuild.CreditAsset{Code: asset.Code(), Issuer: asset.Issuer()},
})
require.NoError(t, err)

err = initiatorChannel.IngestTx(ftxXDR, successResultXDR, resultMetaXDR)
require.NoError(t, err)
err = responderChannel.IngestTx(ftxXDR, successResultXDR, resultMetaXDR)
require.NoError(t, err)
}

// Update balances known for each other.
initiatorChannel.UpdateRemoteEscrowAccountBalance(responder.Contribution)
responderChannel.UpdateRemoteEscrowAccountBalance(initiator.Contribution)
Expand Down Expand Up @@ -865,6 +966,31 @@ func TestOpenUpdatesUncoordinatedClose_recieverNotReturningSigs(t *testing.T) {
require.NoError(t, err)
}

{
t.Log("Initiator and Responder channels ingest the formation tx ...")
ftx, err := initiatorChannel.OpenTx()
require.NoError(t, err)
ftxXDR, err := ftx.Base64()
require.NoError(t, err)

successResultXDR, err := txbuildtest.BuildResultXDR(true)
require.NoError(t, err)
resultMetaXDR, err := txbuildtest.BuildFormationResultMetaXDR(txbuildtest.FormationResultMetaParams{
InitiatorSigner: initiator.KP.Address(),
ResponderSigner: responder.KP.Address(),
InitiatorEscrow: initiator.Escrow.Address(),
ResponderEscrow: responder.Escrow.Address(),
StartSequence: s,
Asset: txnbuild.NativeAsset{},
})
require.NoError(t, err)

err = initiatorChannel.IngestTx(ftxXDR, successResultXDR, resultMetaXDR)
require.NoError(t, err)
err = responderChannel.IngestTx(ftxXDR, successResultXDR, resultMetaXDR)
require.NoError(t, err)
}

// Update balances known for each other.
initiatorChannel.UpdateRemoteEscrowAccountBalance(responder.Contribution)
responderChannel.UpdateRemoteEscrowAccountBalance(initiator.Contribution)
Expand Down
15 changes: 12 additions & 3 deletions sdk/state/open.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,9 +192,14 @@ func (c *Channel) OpenTx() (formationTx *txnbuild.Transaction, err error) {
// initiating the channel.
func (c *Channel) ProposeOpen(p OpenParams) (OpenAgreement, error) {
// if the channel is already open, error.
if c.openAgreement.isFull() {
return OpenAgreement{}, fmt.Errorf("cannot propose a new open if channel is already opened")
cs, err := c.State()
if err != nil {
return OpenAgreement{}, fmt.Errorf("getting channel state: %w", err)
}
if cs >= StateOpen {
return OpenAgreement{}, fmt.Errorf("cannot propose a new open if channel has already opened")
Comment on lines +199 to +200
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❗ 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:

Suggested change
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:

  • StateFailed
  • StateOpenProposed = only one signer on the open
  • StateOpenConfirmed = two signers, not ingested
  • StateOpened = ingested, actually opened on network
  • StateClosing
  • StateClosed

❓ 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.

}

c.startingSequence = c.initiatorEscrowAccount().SequenceNumber + 1

d := OpenAgreementDetails{
Expand Down Expand Up @@ -224,7 +229,11 @@ func (c *Channel) ProposeOpen(p OpenParams) (OpenAgreement, error) {

func (c *Channel) validateOpen(m OpenAgreement) error {
// if the channel is already open, error.
if c.openAgreement.isFull() {
cs, err := c.State()
if err != nil {
return fmt.Errorf("getting channel state: %w", err)
}
if cs >= StateOpen {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return fmt.Errorf("cannot confirm a new open if channel is already opened")
}

Expand Down
Loading