swap: message price #1827

Eknir · 2019-09-27T08:25:34Z

This PR is desirable to run the Devcon workshop in Osaka. Without it, participants won't be able to see their balance of Ether increase as a consequence of SWAP: they are paying more in tx costs than they receive from cashing.

Sets a more realistic message price for chunkDelivery / chunkRequest
Updates the payment threshold to send payment when a peer has a balance equivalent of 10 mb of data
Updates the disconnect threshold to be 20 times the payment threshold.
Implements a very basic cash-out strategy of cashing out when you earn 2 times the transaction costs.

swap/prices.go

swap/swap.go

zelig · 2019-09-30T08:44:50Z

https://ci.appveyor.com/project/ethersphere/swarm/builds/27715383/job/uvgws92j5au5fd82#L777

Eknir · 2019-10-01T14:51:02Z

swap/swap.go

-	// cash cheque in async, otherwise this blocks here until the TX is mined
-	go defaultCashCheque(s, otherSwap, opts, cheque)
+	gasPrice, err := s.backend.SuggestGasPrice(context.TODO())
+	transactionCosts := gasPrice.Uint64() * 50000 // cashing a cheque is approximately 50000 gas


An issue was created to make this non-hardcoded: #1845

holisticode

This is close to ready to go.

A couple of minor things. I think we should use an average cloud storage price as a base for calculation.
And I prefer we do the type conversion for thresholds only once, instead of at every Add

holisticode · 2019-10-01T17:04:01Z

swap/prices.go

@@ -33,9 +33,15 @@ allowing for a multi-currency design.
 */

 // Placeholder prices
+// Based on a very crude calculation: average monthly cost for bandwidth in the US / average monthly bill for bandwidth in the US


I think we should use an average cloud storage price instead of internet provider cost

Based on the conversation in the chat, I leave it as-is. This is not because I don't think you are right, but because I think that both presented options have flaws which we must consider. Changing it now to a different price will possibly bring up other issues and for the workshop the chosen price is sufficient.

How would you go about defining this in a more proper manner? In my opinion, we must first agree on a methodology on how to get the price, after which we can apply this methodology. Shall we plan a workshop in Japan with the incentives team and research?

holisticode · 2019-10-01T17:04:16Z

swap/prices.go

+// 0.35 / (10^9 * 4096) = $0.0000014336 / chunk
+// 0.0000014336 / 166 * 10^18 = 8636144578.31 Wei / chunk, where 166 is the current Ether price in Dollar
+// RetrieveRequestPrice = 0.1 *  8636144578.31 = 863614458
+// ChunkDeliveryPrice = 0.9 * 8636144578.31 = 7772530120


Just clarify that those factors are currently bogus

holisticode · 2019-10-01T17:13:02Z

swap/protocol_test.go

+	// cashCheque cashes a cheque when we get twice the transaction costs.
+	// gasPrice on testBackend == 1
+	// estimated gas costs == 50000
+	// cheque should be send if the accumulated uncashed cheques is worth more than 100000


cheque should be sent (typo)

also are worth instead of is worth

cheques should be sent updated
are worth not updated, as the accumulated amount is worth and not accumulated amount are worth. Added amount, to make this clear.

holisticode · 2019-10-01T17:15:19Z

swap/swap.go

@@ -76,8 +76,8 @@ type Owner struct {
 // Params encapsulates param
 type Params struct {
 	LogPath             string // optional audit log path
-	PaymentThreshold    int64  // honey amount at which a payment is triggered
-	DisconnectThreshold int64  // honey amount at which a peer disconnects
+	PaymentThreshold    uint64 // honey amount at which a payment is triggered


I don't agree to changing the data type here.

While I understand why you want to do it, the way you propose now has the consequence that now on every comparison we need to do a conversion, while as it was before (if I interpret correctly) the conversion is only done once, when we set these thresholds in the params.

Changed back. Fully agree with your reasoning, thanks

holisticode · 2019-10-01T17:15:58Z

swap/swap.go

@@ -251,7 +251,7 @@ func (s *Swap) Add(amount int64, peer *protocols.Peer) (err error) {

 	// Check if balance with peer is over the disconnect threshold
 	balance := swapPeer.getBalance()
-	if balance >= s.params.DisconnectThreshold {
+	if balance >= int64(s.params.DisconnectThreshold) {


This is what I mean, now we have to do the conversion on every Add, which is unnecessary, especially because it's always the same value.

holisticode · 2019-10-01T17:16:19Z

swap/swap.go

@@ -262,7 +262,7 @@ func (s *Swap) Add(amount int64, peer *protocols.Peer) (err error) {
 	// Check if balance with peer crosses the payment threshold
 	// It is the peer with a negative balance who sends a cheque, thus we check
 	// that the balance is *below* the threshold
-	if swapPeer.getBalance() <= -s.params.PaymentThreshold {
+	if swapPeer.getBalance() <= -int64(s.params.PaymentThreshold) {


swap/swap.go

mortelli

i see no major issues, just have a couple of questions

cmd/swarm/config_test.go

swap/config.go

mortelli · 2019-10-01T18:50:18Z

swap/protocol_test.go

+	// cashCheque cashes a cheque when we get twice the transaction costs.
+	// gasPrice on testBackend == 1
+	// estimated gas costs == 50000
+	// cheque should be send if the accumulated uncashed cheques is worth more than 100000


also are worth instead of is worth

swap/protocol_test.go

santicomp2014

There are some minor things @holisticode pointed out about error handling, that should be addressed before merging.
The change of int64 to uint64 is justified but is a bit verbose now, but it's ok.

The discussion of prices, in my opinion, should be resolved may be in a different PR, because this more about solving this issue for the workshop rather than a definite solution.

Other than that LGTM.

…per byte/chunk

…error checking

santicomp2014 · 2019-10-02T12:41:40Z

The linter is complaining of unnecessary conversion, because of the switch back to int64.

swap/swap_test.go:779:25: unnecessary conversion (unconvert)
		if peerBalance < int64(swap.params.DisconnectThreshold) {

mortelli · 2019-10-02T13:15:37Z

master has new commits, please rebase/merge

Eknir added 3 commits September 27, 2019 09:47

swap: change prices for RetrieveRequest and Delivery

4bf55f9

swap: simple cash strategy

585fa9b

swap: change paymentThreshold

aaeef0b

vojtechsimetka reviewed Sep 27, 2019

View reviewed changes

swap/prices.go Outdated Show resolved Hide resolved

vojtechsimetka reviewed Sep 27, 2019

View reviewed changes

swap/swap.go Outdated Show resolved Hide resolved

Eknir added 3 commits September 27, 2019 11:21

swap: make tests run again

3afa058

swap: cashout strategy based on transaction costs instead of gasPrice

05e0579

swap: made calculation for prices more readable

06b3692

Eknir self-assigned this Sep 27, 2019

Eknir added incentives ready for review labels Sep 27, 2019

Eknir added 3 commits September 27, 2019 13:26

swarm: remove int64 cast from payment/disconnect thresholds)

f342cde

swarm: update number to string in TestConfigCmdLineOverrides

106487d

swap: update auditLog

84a27b8

zelig added this to Backlog in Swarm Core - Sprint planning via automation Sep 30, 2019

zelig added this to the 0.5.1 milestone Sep 30, 2019

zelig approved these changes Sep 30, 2019

View reviewed changes

Eknir added 2 commits October 1, 2019 16:09

Merge branch 'master' into incentives-message_price

b80ed9c

change DisconnectThreshold to 20xpaymentThreshold

e900fdb

Eknir mentioned this pull request Oct 1, 2019

Non-hardcoded expected gas for transaction (e.g. cashCheque) #1845

Open

Eknir commented Oct 1, 2019

View reviewed changes

Eknir added 2 commits October 1, 2019 16:59

swap: updated comment in TestEmitCheque

ce1a512

swap: updated comment to chunkDeliveryPrice calculation

4f6c003

holisticode suggested changes Oct 1, 2019

View reviewed changes

mortelli reviewed Oct 1, 2019

View reviewed changes

santicomp2014 approved these changes Oct 1, 2019

View reviewed changes

Eknir added 3 commits October 2, 2019 11:13

swap: updated the price and thresholds to reflect messages accounted …

5580440

…per byte/chunk

swap: made disconnect and payment threshold int64, updated comments, …

4d895b2

…error checking

swap: updated comment

264f7c6

Eknir added 2 commits October 2, 2019 12:00

swap: updated type conversion in swap_test.go

3fcbaf4

swap: fix misspelling

01ed7cc

Eknir added 2 commits October 2, 2019 14:55

swap: remove unneeded type conversion

3312003

swap: updated comment to threshold, 10^7 instead of 10^6 for threshold

c2b8988

mortelli approved these changes Oct 2, 2019

View reviewed changes

Merge branch 'master' into incentives-message_price

2a99a0a

holisticode approved these changes Oct 2, 2019

View reviewed changes

holisticode merged commit fa28dad into master Oct 2, 2019

Swarm Core - Sprint planning automation moved this from Backlog to Done Oct 2, 2019

mortelli deleted the incentives-message_price branch November 14, 2019 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swap: message price #1827

swap: message price #1827

Eknir commented Sep 27, 2019 •

edited by mortelli

Loading

zelig commented Sep 30, 2019

Eknir Oct 1, 2019

holisticode left a comment

holisticode Oct 1, 2019

Eknir Oct 2, 2019 •

edited

Loading

holisticode Oct 1, 2019

Eknir Oct 2, 2019

holisticode Oct 1, 2019

mortelli Oct 1, 2019

Eknir Oct 2, 2019 •

edited

Loading

holisticode Oct 1, 2019

Eknir Oct 2, 2019

holisticode Oct 1, 2019

holisticode Oct 1, 2019

mortelli left a comment

mortelli Oct 1, 2019

santicomp2014 left a comment

santicomp2014 commented Oct 2, 2019

mortelli commented Oct 2, 2019

swap: message price #1827

swap: message price #1827

Conversation

Eknir commented Sep 27, 2019 • edited by mortelli Loading

zelig commented Sep 30, 2019

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

Eknir Oct 2, 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

Eknir Oct 2, 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

mortelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santicomp2014 left a comment

Choose a reason for hiding this comment

santicomp2014 commented Oct 2, 2019

mortelli commented Oct 2, 2019

Eknir commented Sep 27, 2019 •

edited by mortelli

Loading

Eknir Oct 2, 2019 •

edited

Loading

Eknir Oct 2, 2019 •

edited

Loading