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 1 commit
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
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
105 changes: 62 additions & 43 deletions sdk/state/open_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,57 +373,76 @@ func TestChannel_OpenAgreementIsFull(t *testing.T) {
}

func TestChannel_ProposeAndConfirmOpen_rejectIfChannelAlreadyOpen(t *testing.T) {
localSigner := keypair.MustRandom()
remoteSigner := keypair.MustRandom()
localEscrowAccount := &EscrowAccount{
Address: keypair.MustRandom().FromAddress(),
SequenceNumber: int64(101),
}
remoteEscrowAccount := &EscrowAccount{
Address: keypair.MustRandom().FromAddress(),
SequenceNumber: int64(202),
initiatorSigner, err := keypair.ParseFull("SCBMAMOPWKL2YHWELK63VLAY2R74A6GTLLD4ON223B7K5KZ37MUR6IDF")
require.NoError(t, err)
responderSigner, err := keypair.ParseFull("SBM7D2IIDSRX5Y3VMTMTXXPB6AIB4WYGZBC2M64U742BNOK32X6SW4NF")
require.NoError(t, err)

initiatorEscrow, err := keypair.ParseAddress("GAU4CFXQI6HLK5PPY2JWU3GMRJIIQNLF24XRAHX235F7QTG6BEKLGQ36")
require.NoError(t, err)
responderEscrow, err := keypair.ParseAddress("GBQNGSEHTFC4YGQ3EXHIL7JQBA6265LFANKFFAYKHM7JFGU5CORROEGO")
require.NoError(t, err)

initiatorEscrowAccount := &EscrowAccount{
Address: initiatorEscrow.FromAddress(),
SequenceNumber: int64(28037546508288),
}

channel := NewChannel(Config{
responderEscrowAccount := &EscrowAccount{
Address: responderEscrow.FromAddress(),
SequenceNumber: int64(28054726377472),
}
initiatorChannel := NewChannel(Config{
NetworkPassphrase: network.TestNetworkPassphrase,
MaxOpenExpiry: time.Hour,
Initiator: true,
LocalSigner: localSigner,
RemoteSigner: remoteSigner.FromAddress(),
LocalEscrowAccount: localEscrowAccount,
RemoteEscrowAccount: remoteEscrowAccount,
LocalSigner: initiatorSigner,
RemoteSigner: responderSigner.FromAddress(),
LocalEscrowAccount: initiatorEscrowAccount,
RemoteEscrowAccount: responderEscrowAccount,
})
responderChannel := NewChannel(Config{
NetworkPassphrase: network.TestNetworkPassphrase,
MaxOpenExpiry: time.Hour,
Initiator: false,
LocalSigner: responderSigner,
RemoteSigner: initiatorSigner.FromAddress(),
LocalEscrowAccount: responderEscrowAccount,
RemoteEscrowAccount: initiatorEscrowAccount,
})
channel.openAgreement = OpenAgreement{
Details: OpenAgreementDetails{
ObservationPeriodTime: 1,
ObservationPeriodLedgerGap: 1,
Asset: NativeAsset,
ExpiresAt: time.Now(),
ProposingSigner: localSigner.FromAddress(),
ConfirmingSigner: remoteSigner.FromAddress(),
},
ProposerSignatures: OpenAgreementSignatures{
Declaration: xdr.Signature{0},
Close: xdr.Signature{1},
Formation: xdr.Signature{2},
},
ConfirmerSignatures: OpenAgreementSignatures{
Declaration: xdr.Signature{3},
Close: xdr.Signature{4},
Formation: xdr.Signature{5},
},
}

_, err := channel.ProposeOpen(OpenParams{})
require.EqualError(t, err, "cannot propose a new open if channel is already opened")
open, err := initiatorChannel.ProposeOpen((OpenParams{
Asset: NativeAsset,
ExpiresAt: time.Now().Add(5 * time.Minute),
}))
require.NoError(t, err)
open, err = responderChannel.ConfirmOpen(open)
require.NoError(t, err)
_, err = initiatorChannel.ConfirmOpen(open)
require.NoError(t, err)

_, err = channel.ConfirmOpen(OpenAgreement{})
require.EqualError(t, err, "validating open agreement: cannot confirm a new open if channel is already opened")
formationTx, err := initiatorChannel.OpenTx()
require.NoError(t, err)
formationTxXDR, err := formationTx.Base64()
require.NoError(t, err)

validResultXDR := "AAAAAAAAAGQAAAAAAAAAAQAAAAAAAAABAAAAAAAAAAA="
resultMetaXDR := "AAAAAgAAAAQAAAADAAAZhgAAAAAAAAAABeZHnomROFPTnzMq/2f/9ovCt8AFYg93Lgs47x8JEksAAAAXSHbglAAAGX4AAAACAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAAMAAAAAAAAAAwAAGYEAAAAAYSSM5wAAAAAAAAABAAAZhgAAAAAAAAAABeZHnomROFPTnzMq/2f/9ovCt8AFYg93Lgs47x8JEksAAAAXSHbglAAAGX4AAAACAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAAMAAAAAAAAAAwAAGYEAAAAAYSSM5wAAAAAAAAADAAAZgQAAAAAAAAAAKcEW8EeOtXXvxpNqbMyKUIg1ZdcvEB7630v4TN4JFLMAAAACVAvkAAAAGYAAAAAAAAAAAQAAAAAAAAAAAAAAAAABAQEAAAABAAAAAAXmR56JkThT058zKv9n//aLwrfABWIPdy4LOO8fCRJLAAAAAQAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAMAAAAAAAAAAQAAAAEAAAAABeZHnomROFPTnzMq/2f/9ovCt8AFYg93Lgs47x8JEksAAAAAAAAAAQAAAAEAAAAABeZHnomROFPTnzMq/2f/9ovCt8AFYg93Lgs47x8JEksAAAAAAAAAAQAAGYYAAAAAAAAAACnBFvBHjrV178aTamzMilCINWXXLxAe+t9L+EzeCRSzAAAAAlQL5AAAABmAAAAAAQAAAAEAAAAAAAAAAAAAAAAAAQEBAAAAAQAAAAAF5keeiZE4U9OfMyr/Z//2i8K3wAViD3cuCzjvHwkSSwAAAAEAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAIAAAADAAAAAAAAAAEAAAABAAAAAAXmR56JkThT058zKv9n//aLwrfABWIPdy4LOO8fCRJLAAAAAwAAGYYAAAAAYSSM7AAAAAEAAAABAAAAAAXmR56JkThT058zKv9n//aLwrfABWIPdy4LOO8fCRJLAAAAAAAAAAwAAAAAAAAAAgAAAAMAABmGAAAAAAAAAAApwRbwR461de/Gk2pszIpQiDVl1y8QHvrfS/hM3gkUswAAAAJUC+QAAAAZgAAAAAEAAAABAAAAAAAAAAAAAAAAAAEBAQAAAAEAAAAABeZHnomROFPTnzMq/2f/9ovCt8AFYg93Lgs47x8JEksAAAABAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAwAAAAAAAAABAAAAAQAAAAAF5keeiZE4U9OfMyr/Z//2i8K3wAViD3cuCzjvHwkSSwAAAAMAABmGAAAAAGEkjOwAAAABAAAAAQAAAAAF5keeiZE4U9OfMyr/Z//2i8K3wAViD3cuCzjvHwkSSwAAAAAAAAABAAAZhgAAAAAAAAAAKcEW8EeOtXXvxpNqbMyKUIg1ZdcvEB7630v4TN4JFLMAAAACVAvkAAAAGYAAAAABAAAAAQAAAAAAAAAAAAAAAAACAgIAAAABAAAAAAXmR56JkThT058zKv9n//aLwrfABWIPdy4LOO8fCRJLAAAAAQAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAMAAAAAAAAAAQAAAAEAAAAABeZHnomROFPTnzMq/2f/9ovCt8AFYg93Lgs47x8JEksAAAADAAAZhgAAAABhJIzsAAAAAQAAAAEAAAAABeZHnomROFPTnzMq/2f/9ovCt8AFYg93Lgs47x8JEksAAAAAAAAAAAAAAAAAAAAEAAAAAwAAGYUAAAAAAAAAAGDTSIeZRcwaGyXOhf0wCD2vdWUDVFKDCjs+kpqdE6MXAAAAAlQL5AAAABmEAAAAAAAAAAEAAAAAAAAAAAAAAAAAAQEBAAAAAQAAAABm4nRhJ/SD0DxRgmOmEmtOAkpljFHmB5ymmMM/Ro5dCgAAAAEAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAIAAAADAAAAAAAAAAEAAAABAAAAAGbidGEn9IPQPFGCY6YSa04CSmWMUeYHnKaYwz9Gjl0KAAAAAAAAAAEAAAABAAAAAGbidGEn9IPQPFGCY6YSa04CSmWMUeYHnKaYwz9Gjl0KAAAAAAAAAAEAABmGAAAAAAAAAABg00iHmUXMGhslzoX9MAg9r3VlA1RSgwo7PpKanROjFwAAAAJUC+QAAAAZhAAAAAAAAAACAAAAAAAAAAAAAAAAAAEBAQAAAAIAAAAABeZHnomROFPTnzMq/2f/9ovCt8AFYg93Lgs47x8JEksAAAABAAAAAGbidGEn9IPQPFGCY6YSa04CSmWMUeYHnKaYwz9Gjl0KAAAAAQAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAQAAAAAAAAAAgAAAAEAAAAABeZHnomROFPTnzMq/2f/9ovCt8AFYg93Lgs47x8JEksAAAABAAAAAGbidGEn9IPQPFGCY6YSa04CSmWMUeYHnKaYwz9Gjl0KAAAAAAAAAAEAAAABAAAAAGbidGEn9IPQPFGCY6YSa04CSmWMUeYHnKaYwz9Gjl0KAAAAAAAAAAMAABmGAAAAAAAAAAAF5keeiZE4U9OfMyr/Z//2i8K3wAViD3cuCzjvHwkSSwAAABdIduCUAAAZfgAAAAIAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAIAAAAAAAAAAwAAAAAAAAADAAAZgQAAAABhJIznAAAAAAAAAAEAABmGAAAAAAAAAAAF5keeiZE4U9OfMyr/Z//2i8K3wAViD3cuCzjvHwkSSwAAABdIduCUAAAZfgAAAAIAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAIAAAAAAAAABAAAAAAAAAADAAAZgQAAAABhJIznAAAAAAAAAAAAAAAAAAAAAgAAAAMAABmGAAAAAAAAAABg00iHmUXMGhslzoX9MAg9r3VlA1RSgwo7PpKanROjFwAAAAJUC+QAAAAZhAAAAAAAAAACAAAAAAAAAAAAAAAAAAEBAQAAAAIAAAAABeZHnomROFPTnzMq/2f/9ovCt8AFYg93Lgs47x8JEksAAAABAAAAAGbidGEn9IPQPFGCY6YSa04CSmWMUeYHnKaYwz9Gjl0KAAAAAQAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAQAAAAAAAAAAgAAAAEAAAAABeZHnomROFPTnzMq/2f/9ovCt8AFYg93Lgs47x8JEksAAAABAAAAAGbidGEn9IPQPFGCY6YSa04CSmWMUeYHnKaYwz9Gjl0KAAAAAAAAAAEAAAABAAAAAGbidGEn9IPQPFGCY6YSa04CSmWMUeYHnKaYwz9Gjl0KAAAAAAAAAAEAABmGAAAAAAAAAABg00iHmUXMGhslzoX9MAg9r3VlA1RSgwo7PpKanROjFwAAAAJUC+QAAAAZhAAAAAAAAAACAAAAAAAAAAAAAAAAAAICAgAAAAIAAAAABeZHnomROFPTnzMq/2f/9ovCt8AFYg93Lgs47x8JEksAAAABAAAAAGbidGEn9IPQPFGCY6YSa04CSmWMUeYHnKaYwz9Gjl0KAAAAAQAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAQAAAAAAAAAAgAAAAEAAAAABeZHnomROFPTnzMq/2f/9ovCt8AFYg93Lgs47x8JEksAAAABAAAAAGbidGEn9IPQPFGCY6YSa04CSmWMUeYHnKaYwz9Gjl0KAAAAAAAAAAEAAAABAAAAAGbidGEn9IPQPFGCY6YSa04CSmWMUeYHnKaYwz9Gjl0KAAAAAAAAAAAAAAAAAAAABAAAAAMAABmGAAAAAAAAAAApwRbwR461de/Gk2pszIpQiDVl1y8QHvrfS/hM3gkUswAAAAJUC+QAAAAZgAAAAAEAAAABAAAAAAAAAAAAAAAAAAICAgAAAAEAAAAABeZHnomROFPTnzMq/2f/9ovCt8AFYg93Lgs47x8JEksAAAABAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAwAAAAAAAAABAAAAAQAAAAAF5keeiZE4U9OfMyr/Z//2i8K3wAViD3cuCzjvHwkSSwAAAAMAABmGAAAAAGEkjOwAAAABAAAAAQAAAAAF5keeiZE4U9OfMyr/Z//2i8K3wAViD3cuCzjvHwkSSwAAAAAAAAABAAAZhgAAAAAAAAAAKcEW8EeOtXXvxpNqbMyKUIg1ZdcvEB7630v4TN4JFLMAAAACVAvkAAAAGYAAAAABAAAAAgAAAAAAAAAAAAAAAAACAgIAAAACAAAAAAXmR56JkThT058zKv9n//aLwrfABWIPdy4LOO8fCRJLAAAAAQAAAABm4nRhJ/SD0DxRgmOmEmtOAkpljFHmB5ymmMM/Ro5dCgAAAAEAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAIAAAAEAAAAAAAAAAIAAAABAAAAAAXmR56JkThT058zKv9n//aLwrfABWIPdy4LOO8fCRJLAAAAAQAAAABm4nRhJ/SD0DxRgmOmEmtOAkpljFHmB5ymmMM/Ro5dCgAAAAMAABmGAAAAAGEkjOwAAAABAAAAAQAAAAAF5keeiZE4U9OfMyr/Z//2i8K3wAViD3cuCzjvHwkSSwAAAAAAAAADAAAZhQAAAAAAAAAAZuJ0YSf0g9A8UYJjphJrTgJKZYxR5gecppjDP0aOXQoAAAAXSHblqAAAGYIAAAACAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAAMAAAAAAAAAAwAAGYUAAAAAYSSM6wAAAAAAAAABAAAZhgAAAAAAAAAAZuJ0YSf0g9A8UYJjphJrTgJKZYxR5gecppjDP0aOXQoAAAAXSHblqAAAGYIAAAACAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAAQAAAAAAAAAAwAAGYUAAAAAYSSM6wAAAAAAAAAAAAAAAA=="
err = initiatorChannel.IngestTx(formationTxXDR, validResultXDR, resultMetaXDR)
require.NoError(t, err)

cs, err := initiatorChannel.State()
require.NoError(t, err)
assert.Equal(t, StateOpen, cs)

// A channel without a full open agreement should be able to propose an open
channel.openAgreement.ConfirmerSignatures = OpenAgreementSignatures{}
_, err = channel.ProposeOpen(OpenParams{
// local channel trying to open channel again should error.
_, err = initiatorChannel.ProposeOpen((OpenParams{
Asset: NativeAsset,
ExpiresAt: time.Now().Add(5 * time.Minute),
})
require.NoError(t, err)
}))
require.EqualError(t, err, "cannot propose a new open if channel has already opened")

// local channel trying to confirm an open again should error.
_, err = initiatorChannel.ConfirmOpen(open)
require.EqualError(t, err, "validating open agreement: cannot confirm a new open if channel is already opened")
}
12 changes: 10 additions & 2 deletions sdk/state/payment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,11 @@ func (c *Channel) ProposePayment(amount int64) (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 {
acharb marked this conversation as resolved.
Show resolved Hide resolved
return CloseAgreement{}, fmt.Errorf("cannot propose a payment before channel is opened")
}

Expand Down Expand Up @@ -158,7 +162,11 @@ var ErrUnderfunded = fmt.Errorf("account is underfunded to make payment")
// on the state of the close agreement signatures.
func (c *Channel) validatePayment(ca CloseAgreement) (err 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 {
Comment on lines -161 to +169
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 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.

return fmt.Errorf("cannot confirm a payment before channel is opened")
}

Expand Down
1 change: 1 addition & 0 deletions sdk/state/payment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,7 @@ func TestChannel_ConfirmPayment_initiatorCannotProposePaymentThatIsUnderfunded(t
ObservationPeriodLedgerGap: 10,
},
}

_, err := channel.ProposePayment(110)
assert.EqualError(t, err, "amount over commits: account is underfunded to make payment")
assert.ErrorIs(t, err, ErrUnderfunded)
Expand Down