swap, contracts, vendor: move to waiver-free simplestswap #1683

ralph-pichler · 2019-08-21T10:10:20Z

This PR changes swap to use the new waiver-free SimpleSwap contract.

More precisely it

uses the new autogenerated go bindings repository as a dependency
uses the new SimpleSwap contracts which has different functions and encodings
removes serial and timeout (as well as the related checks)
renames amount to cumulativePayout (to be consistent with naming in new SimpleSwap)
verifies that payment was actually successful using the ChequeBounced event
move Honey away from ChequeParams (which is now only for fields that are actually signed)

holisticode

Very welcomed PR! Great simplification.

A couple of comments though, mostly related to comments and minor stuff. But also a couple of potentially important things.

Should we not add a test for Bounced contracts?

Also, there are conflicts, which appear to have to do with the new vendoring stuff. If you need help resolving, give a shout please.

Finally, AppVeyor is failing, but at first look it looks unrelated to the PR. But maybe we should report it if it is indeed coming from stream.

contracts/swap/swap.go

holisticode · 2019-08-29T17:17:44Z

contracts/swap/swap.go

+	Beneficiary      common.Address
+	Recipient        common.Address
+	Caller           common.Address
+	TotalPayout      *big.Int


Please comment the different payouts. Why is CumulativePayout different than TotalPayout? I guess CallerPayout was the amount of the cheque.

added comments.

CumulativePayout is the value in the cheque. TotalPayout is what was paid out in this call (e.g. if fully liquid then it would be CumulativePayout - paidOut). CallerPayout is the portion of the payout that goes to the caller of the function, it can be used to incentivise third parties to submit the transaction but we don't use that functionality here. For CashChequeBeneficiary calls this will always be 0.

swap/cheque.go

swap/swap.go

holisticode · 2019-08-29T17:34:16Z

swap/swap.go

@@ -364,7 +362,7 @@ func (s *Swap) sendCheque(swapPeer *Peer) error {
 	}

 	// reset balance;
-	err = s.resetBalance(peer, int64(cheque.Amount))
+	err = s.resetBalance(peer, int64(cheque.CumulativePayout))


Does this still work correctly with the CumulativePayout? (I did not go and check, just asking, please confirm). We can not just subtract the whole CumulativePayout from the current balance, or it will overflow?

I don't think that was correct before (as Amount also was cumulative, also balance is denominated in honey whereas the payout is in eth) and it's still wrong now. Should we not increase the balance by cheque.Honey here instead?

If it was incorrect, how come tests don't fail? My comment is actually bad though, this is in sendCheques, which would be from the debitor/issuer, which would have a negative balance and thus the amount needs to be added, not subtracted.

My question here is this one. Let's assume I have -100 of balance, I send a cheque for 100, then the balance gets back to 0. The cumulative payout is 100 for now. Then I get into a new debt, I get to maybe -100 balance again, issue a new cheque for again 100, now the cumulative payout is 200, but my balance should be reset only by 100. If it is reset with CumulativePayout, my balance suddenly gets to +100, no?

I think the tests don't fail because the only tests that call this are the ones in protocol_test.go and all of them only send at most one cheque at which point the chequeactualAmount and the cumulativePayout will be the same. Regardless it is definitely the wrong thing because accounting needs to be done in honey. As far as I see it the only correct approach is to increase the balance by cheque.Honey (actualAmount would also work for now but only because of the hardcode honey price of 1).

I extended the TestTriggerPaymentThreshold test to trigger a second cheque and as expected it did the wrong thing. Increasing by cheque.Honey resulted in the expected behaviour.

There is definitely another major issue with accounting. If two separate nodes run their balance tracking already diverges significantly after the first cheque.

Never mind. It only looked this way due to further swap accounting happening in the meantime. Both nodes remain in sync now.

I think this is the only actual in-process simulation we should write: That we spawn up a number of nodes, exchange some data and then check that all balances are symmetrical.

holisticode

Only a very minor thing and a major question left.
Please check the comment about resetBalance

contracts/swap/swap.go

swap/swap.go

holisticode · 2019-08-29T20:47:34Z

swap/swap.go

@@ -364,7 +362,7 @@ func (s *Swap) sendCheque(swapPeer *Peer) error {
 	}

 	// reset balance;
-	err = s.resetBalance(peer, int64(cheque.Amount))
+	err = s.resetBalance(peer, int64(cheque.CumulativePayout))


If it was incorrect, how come tests don't fail? My comment is actually bad though, this is in sendCheques, which would be from the debitor/issuer, which would have a negative balance and thus the amount needs to be added, not subtracted.

My question here is this one. Let's assume I have -100 of balance, I send a cheque for 100, then the balance gets back to 0. The cumulative payout is 100 for now. Then I get into a new debt, I get to maybe -100 balance again, issue a new cheque for again 100, now the cumulative payout is 200, but my balance should be reset only by 100. If it is reset with CumulativePayout, my balance suddenly gets to +100, no?

janos

I have focused on dependency changes and only technical aspects of this pr. From that point everything is fine including @holisticode comments.

holisticode

Thanks for adding the test with the 2nd cheque, that makes me feel more comfortable that it is working correctly :)

* 'master' of github.com:ethersphere/swarm: swap: fix TestHandshake and TestEmitCheque (ethersphere#1721) cmd/swarm-smoke: prevent smoke test from executing trackChunks twice when we debug (ethersphere#1717) swap, contracts, vendor: move to waiver-free simplestswap (ethersphere#1683)

ralph-pichler force-pushed the incentives-simplestswap branch 2 times, most recently from 65e5e5d to 156b074 Compare August 26, 2019 12:26

holisticode force-pushed the incentives branch 2 times, most recently from 6915f76 to 564f53d Compare August 27, 2019 17:45

ralph-pichler force-pushed the incentives-simplestswap branch 3 times, most recently from 363ea00 to c2bb3fd Compare August 27, 2019 19:07

ralph-pichler changed the base branch from incentives to master August 27, 2019 19:09

ralph-pichler marked this pull request as ready for review August 28, 2019 11:17

ralph-pichler force-pushed the incentives-simplestswap branch from c2bb3fd to 26dbb54 Compare August 28, 2019 12:35

ralph-pichler requested a review from holisticode August 28, 2019 13:19

holisticode added the incentives label Aug 29, 2019

holisticode suggested changes Aug 29, 2019

View reviewed changes

ralph-pichler force-pushed the incentives-simplestswap branch from 24ad566 to 47153ec Compare August 29, 2019 19:53

ralph-pichler requested review from holisticode and janos August 29, 2019 19:58

holisticode reviewed Aug 29, 2019

View reviewed changes

janos approved these changes Aug 30, 2019

View reviewed changes

ralph-pichler force-pushed the incentives-simplestswap branch from 3f15d73 to 935e353 Compare August 30, 2019 12:59

holisticode self-requested a review August 30, 2019 17:09

holisticode approved these changes Aug 30, 2019

View reviewed changes

ralph-pichler added 9 commits September 2, 2019 19:49

swap: use newer simple swap contract

be419a7

swap: remove timeout and serial from cheque

5caf056

swap: rename cheque.Amount to cheque.CumulativePayout

3bc60e1

swap: check if cheque bounced

7499670

swap: cleaner separation of Cheque and ChequeParam

c983ae2

contracts: adjust comments for SimplestSwap

912467a

swap, contract: remove references to submit, serial or timeout

87c76b5

swap: test bouncing cheque

bf03560

contract: fix comment

1cbaaba

ralph-pichler added 2 commits September 2, 2019 19:49

swap: increase balance by honey on sendCheque

d4a8547

swap: add honey to cheque related logs

d6013bd

ralph-pichler force-pushed the incentives-simplestswap branch from 935e353 to d6013bd Compare September 2, 2019 18:00

mortelli mentioned this pull request Sep 2, 2019

all: first working SWAP version #1554

Merged

6 tasks

ralph-pichler changed the title ~~swap: move to waiver-free simplestswap~~ swap, contracts, vendor: move to waiver-free simplestswap Sep 2, 2019

ralph-pichler merged commit 08cd528 into master Sep 2, 2019

ralph-pichler deleted the incentives-simplestswap branch September 2, 2019 21:01

skylenet added this to the 0.5.0 milestone Sep 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swap, contracts, vendor: move to waiver-free simplestswap #1683

swap, contracts, vendor: move to waiver-free simplestswap #1683

ralph-pichler commented Aug 21, 2019 •

edited

Loading

holisticode left a comment •

edited

Loading

holisticode Aug 29, 2019

ralph-pichler Aug 29, 2019

holisticode Aug 29, 2019

ralph-pichler Aug 29, 2019

holisticode Aug 29, 2019

ralph-pichler Aug 30, 2019 •

edited

Loading

ralph-pichler Aug 30, 2019

ralph-pichler Aug 30, 2019

ralph-pichler Aug 30, 2019

holisticode Aug 30, 2019

holisticode left a comment

holisticode Aug 29, 2019

janos left a comment

holisticode left a comment

swap, contracts, vendor: move to waiver-free simplestswap #1683

swap, contracts, vendor: move to waiver-free simplestswap #1683

Conversation

ralph-pichler commented Aug 21, 2019 • edited Loading

holisticode left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ralph-pichler Aug 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holisticode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janos left a comment

Choose a reason for hiding this comment

holisticode left a comment

Choose a reason for hiding this comment

ralph-pichler commented Aug 21, 2019 •

edited

Loading

holisticode left a comment •

edited

Loading

ralph-pichler Aug 30, 2019 •

edited

Loading