feat: Negotiate cheques #1924

metacertain · 2021-05-31T02:27:43Z

The pr consists of three main parts:

the priceoracle, polling the current values of the exchange rate and initial deduction at node start and later in every unix timestamp divisible by 300 (meaning every 5 minutes at approximately the same time on all nodes).
the exchange rate and applicable deduction negotiation happening before issuing a cheque to a peer.
converting between the amount of accounting credits attempted to be settled, and the amount of plur sent to cover for this settlement using the exchange rate learned from the price oracle / negotiated in swap protocol

The negotiation process consists of:

the peer willing to issue a cheque begins opening a stream sending empty headers
the accepting peer returns it's current known exchange rate, and the applicable deduction in headers
- the applicable deduction is the current known deduction, or 0 if the peer attempting to settle has already sent a cheque with negotiated deduction previously
the peer compares the received exchange rate and deduction headers to his version of known truth, if both match, issues a cheque, otherwise cancels the cheque issuing and restrains from further attempts to send cheques to this peer for 10 seconds, to limit rate of errors in negative scenarios
if the deduction was negotiated non zero, both peers save the other as already having a deduction sent to / received from, so that in the future they are able to agree on zero deduction based on this registry.

Negative scenarios can be:

if there was an update of the oracle exchange rate or deduction between polling point A and A+1, any node started after the the point of update but before A+1 can be ahead of the network in his version of the truth until A+1.
if there was an update of the rate or deduction, near the polling point when one of the peers might be ahead by getting or using the new version of truth sooner / faster

The conversion logic between accounting credits and plur:
Cheque value is ( accounting amount * exchange rate ) + deduction
Thereby received/sent cheque is calculated as changing the balance with ( cheque amount - deduction ) / exchange rate

This change is

zelig

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @metacertain)

pkg/settlement/swap/swapprotocol/swapprotocol.go, line 208 at r1 (raw file):

	checkPeer := false

	if checkPeer {

always false

metacertain · 2021-05-31T10:41:56Z

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @metacertain)

pkg/settlement/swap/swapprotocol/swapprotocol.go, line 208 at r1 (raw file):
	checkPeer := false

	if checkPeer {
always false

it is a work in progress, yet to introduce the component for registering whether a peer already had an initial deduction

acud

Reviewed 1 of 7 files at r1, 29 of 30 files at r2.
Reviewable status: 30 of 31 files reviewed, 15 unresolved discussions (waiting on @metacertain and @zelig)

.github/workflows/beekeeper.yml, line 17 at r2 (raw file):

      RUN_TYPE: "PR RUN"
      SETUP_CONTRACT_IMAGE_TAG: "0.1.0"
      BEEKEEPER_BRANCH: "thresholds"

needed?

pkg/accounting/accounting.go, line 32 at r2 (raw file):

	// this value is chosen so that tiny payments are prevented while still allowing small payments in environments with lower payment thresholds
	minimumPaymentDivisor    = int64(5)
	failedSettlementInterval = int64(10)

ten what? s/m/d/m/y?
EDIT: seconds.
comment would be nice

pkg/accounting/accounting.go, line 333 at r2 (raw file):

				// add settled amount to shadow reserve before sending it
				balance.shadowReservedBalance.Add(balance.shadowReservedBalance, paymentAmount)
				go a.payFunction(context.Background(), peer, paymentAmount)

there is no goroutine tracking here for shutdown, which means that payments made during shutdown might be lost

pkg/accounting/accounting_test.go, line 1100 at r2 (raw file):

	}

	acc.Release(peer1Addr, 1)

is this needed at the end of the test?

pkg/settlement/swap/swap.go, line 101 at r2 (raw file):

	if deduction.Cmp(big.NewInt(0)) > 0 {
		err = s.addressbook.AddDeductionFor(peer)

is this operation made under a peer lock? there might be a data race hiding here

pkg/settlement/swap/chequebook/chequestore_test.go, line 447 at r2 (raw file):

	sig := make([]byte, 65)
	chainID := int64(1)
	exchangeRate := big.NewInt(100)

can we have a short comment explaining the arithmetics and why the error should be returned?

pkg/settlement/swap/headers/utilities.go, line 24 at r2 (raw file):

	// ErrNoExchangeHeader denotes p2p.Header lacking specified field
	ErrNoExchangeHeader = errors.New("no exchange header")
	// ErrNoTargetHeader denotes p2p.Header lacking specified field

ErrWrongComment

pkg/settlement/swap/headers/utilities.go, line 45 at r2 (raw file):

	deduction, err = ParseDeductionHeader(receivedHeaders)
	if err != nil {
		return exchangeRate, nil, err

Nit, but since the only error that can be returned from ParseDeductionHeader is ErrNoDeductionHeader, and since the calling contexts always fall back to deduction value 0, then maybe it is not needed to return the error and just return a 0 from ParseDeductionHeader. However having it explicit as it is might be a nice option too, since it makes things a bit clearer in the calling contexts

pkg/settlement/swap/priceoracle/priceoracle.go, line 38 at r2 (raw file):

type Service interface {
	io.Closer
	// Deposit starts depositing erc20 token into the chequebook. This returns once the transactions has been broadcast.

wrong comment, missing comments on other methods in the interface

pkg/settlement/swap/priceoracle/priceoracle.go, line 45 at r2 (raw file):

var (
	priceOracleABI = transaction.ParseABIUnchecked(priceoracleabi.PriceOracleABIv0_1_0)

I think there's a v0_2_0

pkg/settlement/swap/priceoracle/priceoracle.go, line 81 at r2 (raw file):

			ts := time.Now().Unix()

			timeUntilNextPoll := time.Duration(s.timeDivisor-ts%s.timeDivisor) * time.Second

why not use a const?

pkg/settlement/swap/priceoracle/priceoracle.go, line 92 at r2 (raw file):

}

func (s *service) GetPrice(ctx context.Context) (*big.Int, *big.Int, error) {

since this wrapping method does not provide any further functionality, it can be removed and instead the unexported one can be exported

pkg/settlement/swap/priceoracle/priceoracle_test.go, line 22 at r2 (raw file):

var (
	priceOracleABI = transaction.ParseABIUnchecked(priceoracleabi.PriceOracleABIv0_1_0)

here too, v2?

pkg/settlement/swap/swapprotocol/swapprotocol_test.go, line 31 at r2 (raw file):

)

func TestInit(t *testing.T) {

it would be good to document these tests also with a godoc style comment and also inline comments within the tests as they are not self describing and I find them hard to review

ralph-pichler · 2021-06-10T14:02:23Z

pkg/settlement/swap/priceoracle/priceoracle.go, line 45 at r2 (raw file):

Previously, acud (acud) wrote…

I think there's a v0_2_0

that's a different price oracle, for this one there is only one version.

metacertain · 2021-06-10T14:28:18Z

  BEEKEEPER_BRANCH: "thresholds"

needed?

yes, this reduces thresholds by the magnitude of the newly introduced exchange rates
the beekeeper PR containing this should be merged at the same time as this to avoid breaks

failedSettlementInterval = int64(10)
ten what? s/m/d/m/y?
EDIT: seconds.

seconds indeed, added comment

pkg/accounting/accounting.go, line 333 at r2 (raw file):
				// add settled amount to shadow reserve before sending it
				balance.shadowReservedBalance.Add(balance.shadowReservedBalance, paymentAmount)
				go a.payFunction(context.Background(), peer, paymentAmount)
there is no goroutine tracking here for shutdown, which means that payments made during shutdown might be lost

still to be fixed

acc.Release(peer1Addr, 1)
is this needed at the end of the test?

we always do it consistently, not strictly needed

	err = s.addressbook.AddDeductionFor(peer)
is this operation made under a peer lock? there might be a data race hiding here

no need for lock in this case. this adds an empty struct to the specified address-index
if it is overwritten by another empty struct, wouldn't cause a problem
this is to remember deduction happened once towards a specific peer.
only one payment is ongoing at a time so no other payment is attempted without this saved yet

exchangeRate := big.NewInt(100)
can we have a short comment explaining the arithmetics and why the error should be returned?

added comment to test

ErrWrongComment

:)

Nit, but since the only error that can be returned from ParseDeductionHeader is ErrNoDeductionHeader, and since the calling contexts always fall back to deduction value 0, then maybe it is not needed to return the error and just return a 0 from ParseDeductionHeader. However having it explicit as it is might be a nice option too, since it makes things a bit clearer in the calling contexts

this should be fine the way it is, than?

// Deposit starts depositing erc20 token into the chequebook. This returns once the transactions has been broadcast.
wrong comment, missing comments on other methods in the interface

changed comment

var (
priceOracleABI = transaction.ParseABIUnchecked(priceoracleabi.PriceOracleABIv0_1_0)
I think there's a v0_2_0

thats the other ABI for postage stamps, v0_1_0 is good

		timeUntilNextPoll := time.Duration(s.timeDivisor-ts%s.timeDivisor) * time.Second

why not use a const?

added comment explaining logic

func (s *service) GetPrice(ctx context.Context) (*big.Int, *big.Int, error) {
since this wrapping method does not provide any further functionality, it can be removed and instead the unexported one can be exported

done

priceOracleABI = transaction.ParseABIUnchecked(priceoracleabi.PriceOracleABIv0_1_0)

here too, v2?

v1 is ok

func TestInit(t *testing.T) {
it would be good to document these tests also with a godoc style comment and also inline comments within the tests as they are not self describing and I find them hard to review

added comments

acud

LGTM abel. Thanks. 👍

Reviewed 2 of 2 files at r3, 4 of 5 files at r4, 2 of 2 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @zelig)

pkg/accounting/accounting.go, line 333 at r2 (raw file):

Previously, metacertain wrote…

				// add settled amount to shadow reserve before sending it
				balance.shadowReservedBalance.Add(balance.shadowReservedBalance, paymentAmount)
				go a.payFunction(context.Background(), peer, paymentAmount)
there is no goroutine tracking here for shutdown, which means that payments made during shutdown might be lost

still to be fixed
acc.Release(peer1Addr, 1)
is this needed at the end of the test?

we always do it consistently, not strictly needed
	err = s.addressbook.AddDeductionFor(peer)
is this operation made under a peer lock? there might be a data race hiding here

no need for lock in this case. this adds an empty struct to the specified address-index
if it is overwritten by another empty struct, wouldn't cause a problem
this is to remember deduction happened once towards a specific peer.
only one payment is ongoing at a time so no other payment is attempted without this saved yet
exchangeRate := big.NewInt(100)
can we have a short comment explaining the arithmetics and why the error should be returned?

added comment to test

ErrWrongComment

:)

Nit, but since the only error that can be returned from ParseDeductionHeader is ErrNoDeductionHeader, and since the calling contexts always fall back to deduction value 0, then maybe it is not needed to return the error and just return a 0 from ParseDeductionHeader. However having it explicit as it is might be a nice option too, since it makes things a bit clearer in the calling contexts

this should be fine the way it is, than?
// Deposit starts depositing erc20 token into the chequebook. This returns once the transactions has been broadcast.
wrong comment, missing comments on other methods in the interface

changed comment
var (
priceOracleABI = transaction.ParseABIUnchecked(priceoracleabi.PriceOracleABIv0_1_0)
I think there's a v0_2_0

thats the other ABI for postage stamps, v0_1_0 is good
		timeUntilNextPoll := time.Duration(s.timeDivisor-ts%s.timeDivisor) * time.Second
why not use a const?

added comment explaining logic
func (s *service) GetPrice(ctx context.Context) (*big.Int, *big.Int, error) {
since this wrapping method does not provide any further functionality, it can be removed and instead the unexported one can be exported

done
priceOracleABI = transaction.ParseABIUnchecked(priceoracleabi.PriceOracleABIv0_1_0)
here too, v2?

v1 is ok
func TestInit(t *testing.T) {
it would be good to document these tests also with a godoc style comment and also inline comments within the tests as they are not self describing and I find them hard to review

added comments

thanks.

zelig

Reviewed 20 of 30 files at r2, 2 of 2 files at r3, 4 of 5 files at r4, 2 of 2 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @zelig)

ldeffenb · 2021-06-11T19:07:44Z

Is this a breaking change due to the new protocol streams? In other words, if I update to the current master, can I still participate in the 0.6.2 swarm?

bee-runner bot added the pull-request label May 31, 2021

zelig reviewed May 31, 2021

View reviewed changes

ralph-pichler force-pushed the negotiate_cheques branch 2 times, most recently from 3f90ceb to 242a712 Compare May 31, 2021 14:59

metacertain force-pushed the negotiate_cheques branch 4 times, most recently from b8c604a to ef69f8c Compare June 3, 2021 09:26

metacertain added the ready for review The PR is ready to be reviewed label Jun 3, 2021

metacertain force-pushed the negotiate_cheques branch 4 times, most recently from 6db14e0 to 6281c66 Compare June 7, 2021 15:04

acud suggested changes Jun 10, 2021

View reviewed changes

metacertain force-pushed the negotiate_cheques branch from 6281c66 to a61ac90 Compare June 10, 2021 13:19

metacertain requested a review from acud June 11, 2021 11:23

acud approved these changes Jun 11, 2021

View reviewed changes

metacertain requested a review from zelig June 11, 2021 14:09

zelig approved these changes Jun 11, 2021

View reviewed changes

metacertain force-pushed the negotiate_cheques branch from d12fadb to 6458638 Compare June 11, 2021 14:37

metacertain and others added 7 commits June 11, 2021 17:32

feat: start

801dbb2

fix: errors

e26fce0

fix: add missing tests

51e0f4a

feat: add deduction value check

e493abb

feat: price oracle flag

5574243

feat: introduce update function

39e4748

feat: add peer registry

d6a2d94

metacertain and others added 21 commits June 11, 2021 17:36

fix: line endings

2878f6c

fix: format and unused code

2184078

fix: test add accounting

a7900d9

fix: test add swap

8537135

fix: accounting test

69def0f

fix: test add deduction for

cd8692b

fix: accounting test remove wait

34432e1

fix: refactor priceoracle naming

cec57e3

fix: readd priceoracle

de34d1f

feat: add utilities tests

55b201d

fix: priceoracle start

0db431e

fix: readd comment

9bb888c

fix: add export test

9abd58e

fix: lower thresholds, rates and prices

e343bea

fix: beekeeper branch for current thresholds

3f77ce7

fix: review comments I

fad302a

fix: add closer

e0898c7

fix: review

f8b198e

feat: reduce refresh rate

a1b7369

fix: revert reduce refresh rate in tests

8ae73c2

feat: reduce refresh rate

c1acbba

metacertain force-pushed the negotiate_cheques branch from eb66b63 to c1acbba Compare June 11, 2021 15:37

metacertain added 5 commits June 11, 2021 17:42

fix: credit argument in test

1e85b0c

fix: tests

b19d11a

fix: tests

965f8d7

fix: tests

a694007

fix: integration tests

304bda7

metacertain merged commit 1742618 into master Jun 11, 2021

metacertain deleted the negotiate_cheques branch June 11, 2021 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Negotiate cheques #1924

feat: Negotiate cheques #1924

metacertain commented May 31, 2021 •

edited

Loading

zelig left a comment

metacertain commented May 31, 2021

acud left a comment

ralph-pichler commented Jun 10, 2021

metacertain commented Jun 10, 2021

acud left a comment •

edited

Loading

zelig left a comment

ldeffenb commented Jun 11, 2021

feat: Negotiate cheques #1924

feat: Negotiate cheques #1924

Conversation

metacertain commented May 31, 2021 • edited Loading

zelig left a comment

Choose a reason for hiding this comment

metacertain commented May 31, 2021

acud left a comment

Choose a reason for hiding this comment

ralph-pichler commented Jun 10, 2021

metacertain commented Jun 10, 2021

acud left a comment • edited Loading

Choose a reason for hiding this comment

zelig left a comment

Choose a reason for hiding this comment

ldeffenb commented Jun 11, 2021

metacertain commented May 31, 2021 •

edited

Loading

acud left a comment •

edited

Loading