cmd/swarm, swarm, swap, api: configurable payment/disconnect thresholds #1729
Conversation
…ceOracle from peer to swap
Eknir
changed the title
There was a problem hiding this comment.
i know that this PR is a draft still, but while i had some time to review it i though i might as well. so please consider this review as preliminary 😃
my comments are mainly coding style-related, but i do have an important question:
are we adding a disconnect oracle as well as a payment one?
api/config.go
Outdated
| @@ -52,12 +53,14 @@ type Config struct { | |||
| BaseKey []byte | |||
|
|
|||
| // Swap configs | |||
| SwapBackendURL string | |||
| SwapEnabled bool | |||
| SwapBackendURL string | |||
There was a problem hiding this comment.
more explanation in comment needed
There was a problem hiding this comment.
What do you mean here?
There was a problem hiding this comment.
please comment exported constants, explain what they are
swap/swap.go
Outdated
| @@ -350,7 +352,7 @@ func (s *Swap) sendCheque(swapPeer *Peer) error { | |||
|
|
|||
| log.Info("sending cheque", "honey", cheque.Honey, "cumulativePayout", cheque.ChequeParams.CumulativePayout, "beneficiary", cheque.Beneficiary, "contract", cheque.Contract) | |||
| s.setCheque(peer, cheque) | |||
|
|
|||
| fmt.Println(sentChequeKey(peer)) | |||
| @@ -289,6 +292,32 @@ func TestRepeatedBookings(t *testing.T) { | |||
| verifyBookings(t, swap, append(bookings, mixedBookings...)) | |||
| } | |||
|
|
|||
| func TestDisconnectThreshold(t *testing.T) { | |||
There was a problem hiding this comment.
give description of tests in a comment
swarm_test.go
Outdated
| @@ -114,7 +112,7 @@ func TestNewSwarm(t *testing.T) { | |||
| configure: func(config *api.Config) { | |||
| config.SwapBackendURL = ipcEndpoint | |||
| config.SwapEnabled = true | |||
| config.NetworkID = swap.AllowedNetworkID | |||
| config.NetworkID = 2 | |||
There was a problem hiding this comment.
better use constant still, so that the name gives away why we expect that value to behave the way we want
… redundant fmt.Println()
cmd/swarm/flags.go
Outdated
| @@ -65,6 +65,16 @@ var ( | |||
| Usage: "URL of the Ethereum API provider to use to settle SWAP payments", | |||
| EnvVar: SwarmEnvSwapBackendURL, | |||
| } | |||
| SwarmSwapPaymentThresholdFlag = cli.Uint64Flag{ | |||
| Name: "swap-payment-threshold", | |||
| Usage: "honey amount indebted to a peer at which you will initiate payment", | |||
There was a problem hiding this comment.
This is only from one peer's point of view. I would change this to "honey amount at which a payment is triggered"
There was a problem hiding this comment.
I think in anything outside of the test directory, we should make the comments and variable names as if it is from the perspective of the node operator. Especially here, as this is a user-facing comment.
There was a problem hiding this comment.
Maybe, but even then, this node operator can cross the payment threshold in both ways. The threshold doesn't define if you are indebted or not, which is defined only in which direction you cross it.
I would like to insist on this being changed.
There was a problem hiding this comment.
Actually, this threshold does define whether a node is indebted or not, because the threshold which is set here only applies to what this particular node does. Sure, there are other payment thresholds in the network, and the node can cross these from the other direction (and get paid). but this particular threshold is only about how much the node that the user is about to boot up must be indebted to other peers until payment will be initiated.
There was a problem hiding this comment.
What if we would add a log Warn on the creditor node that a peer crossed the threshold and is indebted with him?
Check this exact comment I made on another PR: #1754 (comment)
If we did this (and if we add some other metric or anything to help nodes track that peers got indebted to them), then your comment would not apply.
|
@Eknir @holisticode what is the status of this PR? |
…ts in swarm, remove backend on swarm, correct test TestConfigCmdLineOverrides
mortelli
dismissed
their stale review
changes were applied, but PR is not ready to be approved yet
* 'master' of github.com:ethersphere/swarm: (32 commits) network/stream: refactor cursors tests (ethersphere#1786) network: Add capabilities if peer from store does not have it (ethersphere#1791) Swap logger (ethersphere#1754) network: Add capability filtered depth calculation (ethersphere#1787) travis: remove go1.12 job (ethersphere#1784) cmd/swarm: correct bzznetworkid flag description (ethersphere#1761) network, pss: Capability in pss (ethersphere#1764) network/stream: handle nil peer in TestNodesExchangeCorrectBinIndexes (ethersphere#1779) protocols, retrieval: swap-enabled messages implement Price (ethersphere#1771) cmd/swarm-smoke: fix waitToPushSynced connection closing (ethersphere#1781) cmd/swarm: simplify testCluster.StartNewNodes (ethersphere#1777) build: increase golangci-lint deadline (ethersphere#1778) docker: ignore build/bin when copying files (ethersphere#1780) swap: fix and rename Peer.getLastSentCumulativePayout (ethersphere#1769) network/stream: more resilient TestNodesCorrectBinsDynamic (ethersphere#1776) network: Add Capabilities to Kademlia database (ethersphere#1713) network: add own address to KademliaInfo (ethersphere#1775) pss: Refactor. Step 2. Refactor forward cache (ethersphere#1742) all: configurable payment/disconnect thresholds (ethersphere#1729) network/stream/v2: more resilient TestNodesExchangeCorrectBinIndexes (ethersphere#1760) ...
swarm.go:NewSwarmtoswap/swap.go:Newcloses #1441