Swap Bounced Cheque Verification Process#2134
Conversation
…with actual go sc binding
|
The Go Binding will be replaced by @ralph-pichler when they are ready for the codebase. |
…@v0.2.3 This update contains new bounced cheque functionality
santicomp2014
force-pushed
the
swap-bounced-cheque
branch
from
4374ba4 to
c57f5cb
Compare
contracts/swap/swap.go
Outdated
| } | ||
|
|
||
| // 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. |
There was a problem hiding this comment.
you fixed the one that was correct, it should read:
Once it's bounced, it will not change its state again
swap/swap.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| bounced, err := p.getBouncedCheque() |
There was a problem hiding this comment.
hasBouncedCheque is probably a more accurate variable name here, what do you think?
There was a problem hiding this comment.
p.getBouncedCheque() is now p.hasBouncedCheque()
bounced could be rename to bouncedCheque so it's not verboes
There was a problem hiding this comment.
the bouncedCheque variable is not a cheque, therefore it should be called something boolean e.g. hasBouncedCheque
| return c, err | ||
| } | ||
|
|
||
| // Bounced returns the boolean indicating if a cheque attempted to be cashed has been bounced it in the receiver contract |
There was a problem hiding this comment.
you have a spare it in has been bounced it in the receiver
| }) | ||
| } | ||
|
|
||
| // hasBouncedCheque returns the boolean indicating if a cheque attempted to be cashed has been bounced it in the receiver peer |
| } | ||
|
|
||
| // 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. |
| if err != nil { | ||
| // On connection to peer fail if a bounced cheque exists | ||
| if err == ErrBouncedCheque { | ||
| t.Fatalf("Bounced cheque in chequebook") |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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 |
swap/swap.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| bounced, err := p.getBouncedCheque() |
There was a problem hiding this comment.
the bouncedCheque variable is not a cheque, therefore it should be called something boolean e.g. hasBouncedCheque
mortelli
left a comment
mortelli
left a comment
There was a problem hiding this comment.
approved, but please resolve the remaining minor issues (also from my previous review)
2 participants
This PR intends to satisfy Swap bounced cheque User Story
This should satisfy two criteria
Test cases should validate these constraints.