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)
This PR intends to satisfy Swap bounced cheque User Story
This should satisfy two criteria
Test cases should validate these constraints.