Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

Swap Bounced Cheque Verification Process #2134

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

santicomp2014
Copy link
Contributor

@santicomp2014 santicomp2014 commented Mar 12, 2020

This PR intends to satisfy Swap bounced cheque User Story

This should satisfy two criteria

  • On handshake connection/addPeer, If the peer has a bounced cheque in his Smart contract.
  • On handleEmitChequeMsg, this will do a verification on the Smart contract to ensure no bounced cheque has been detected since it's initial handshake.

Test cases should validate these constraints.

@santicomp2014
Copy link
Contributor Author

santicomp2014 commented Mar 12, 2020

The Go Binding will be replaced by @ralph-pichler when they are ready for the codebase.
Updated from [email protected] to [email protected]

@santicomp2014 santicomp2014 marked this pull request as ready for review March 31, 2020 11:49
contracts/swap/swap.go Outdated Show resolved Hide resolved
@@ -91,6 +93,12 @@ func InstanceAt(address common.Address, backend chain.Backend) (Contract, error)
return c, err
}

// Bounced obtains if a cheque has been bounced previously of a contract at a specific address.
// Once it's bounced, it will not change it's state again, unless re deployed.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in its

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

you fixed the one that was correct, it should read:

Once it's bounced, it will not change its state again

swap/peer.go Outdated Show resolved Hide resolved
swap/peer.go Outdated Show resolved Hide resolved
swap/protocol_test.go Outdated Show resolved Hide resolved
swap/swap.go Outdated
@@ -463,6 +463,14 @@ func (s *Swap) processAndVerifyCheque(cheque *Cheque, p *Peer) (*int256.Uint256,
return nil, err
}

bounced, err := p.getBouncedCheque()
Copy link
Contributor

Choose a reason for hiding this comment

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

hasBouncedCheque is probably a more accurate variable name here, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

p.getBouncedCheque() is now p.hasBouncedCheque()
bounced could be rename to bouncedCheque so it's not verboes

Copy link
Contributor

Choose a reason for hiding this comment

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

the bouncedCheque variable is not a cheque, therefore it should be called something boolean e.g. hasBouncedCheque

swap/swap_test.go Outdated Show resolved Hide resolved
swap/swap_test.go Outdated Show resolved Hide resolved
swap/swap_test.go Outdated Show resolved Hide resolved
@@ -91,6 +93,12 @@ func InstanceAt(address common.Address, backend chain.Backend) (Contract, error)
return c, err
}

// Bounced returns the boolean indicating if a cheque attempted to be cashed has been bounced it in the receiver contract
Copy link
Contributor

Choose a reason for hiding this comment

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

you have a spare it in has been bounced it in the receiver

@@ -221,3 +230,20 @@ func (p *Peer) sendCheque() error {
Cheque: cheque,
})
}

// hasBouncedCheque returns the boolean indicating if a cheque attempted to be cashed has been bounced it in the receiver peer
Copy link
Contributor

Choose a reason for hiding this comment

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

spare it here as well

@@ -221,3 +230,20 @@ func (p *Peer) sendCheque() error {
Cheque: cheque,
})
}

// hasBouncedCheque returns the boolean indicating if a cheque attempted to be cashed has been bounced it in the receiver peer
// Once its bounced, it will not change it's state again, unless re deployed.
Copy link
Contributor

Choose a reason for hiding this comment

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

same typo here

swap/protocol_test.go Outdated Show resolved Hide resolved
if err != nil {
// On connection to peer fail if a bounced cheque exists
if err == ErrBouncedCheque {
t.Fatalf("Bounced cheque in chequebook")
Copy link
Contributor

@mortelli mortelli Apr 1, 2020

Choose a reason for hiding this comment

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

ErrBouncedCheque already conveys that there is an error as a result of a bounced cheque. you don't need to call t.Fatalf with a custom string that explains this.

just do:

if err != nil {
    t.Fatal(err)
}

apply this also to the rest of the TestBouncedCheque function when possible

testAmount := DefaultPaymentThreshold + 42

ctx := context.Background()
err := testDeploy(ctx, creditorSwap, int256.Uint256From(0))
Copy link
Contributor

@mortelli mortelli Apr 1, 2020

Choose a reason for hiding this comment

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

since you are only obtaining err here, you can do something like:

if err := testDeploy(ctx, creditorSwap, int256.Uint256From(0)); err != nil{
    t.Fatal(err)
}

apply this also to the rest of the TestBouncedCheque function when possible

}

// Verifies bounced cheque after incorrect cheque sent
// set balances that will cause a bounce cheque
Copy link
Contributor

Choose a reason for hiding this comment

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

bounced *

swap/swap.go Outdated
@@ -463,6 +463,14 @@ func (s *Swap) processAndVerifyCheque(cheque *Cheque, p *Peer) (*int256.Uint256,
return nil, err
}

bounced, err := p.getBouncedCheque()
Copy link
Contributor

Choose a reason for hiding this comment

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

the bouncedCheque variable is not a cheque, therefore it should be called something boolean e.g. hasBouncedCheque

Copy link
Contributor

@mortelli mortelli left a comment

Choose a reason for hiding this comment

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

approved, but please resolve the remaining minor issues (also from my previous review)

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

Successfully merging this pull request may close these issues.

2 participants