From ab9c9dc732ad9a396f1930d9de17d9e4c92dbba7 Mon Sep 17 00:00:00 2001 From: Ralph Pichler Date: Sat, 7 Mar 2020 13:16:20 +0100 Subject: [PATCH 1/2] swap: remove honeyOracle --- swap/cheque.go | 8 ++++---- swap/honeyOracle.go | 41 ----------------------------------------- swap/peer.go | 9 +-------- swap/prices.go | 2 -- swap/swap.go | 23 +++++++---------------- swap/swap_test.go | 24 ++++++++---------------- 6 files changed, 20 insertions(+), 87 deletions(-) delete mode 100644 swap/honeyOracle.go diff --git a/swap/cheque.go b/swap/cheque.go index 5b27a435d9..0bd285ba06 100644 --- a/swap/cheque.go +++ b/swap/cheque.go @@ -133,22 +133,22 @@ func (cheque *Cheque) verifyChequeProperties(p *Peer, expectedBeneficiary common // verifyChequeAgainstLast verifies that the amount is higher than in the previous cheque and the increase is as expected // returns the actual amount received in this cheque -func (cheque *Cheque) verifyChequeAgainstLast(lastCheque *Cheque, expectedAmount *uint256.Uint256) (*uint256.Uint256, error) { +func (cheque *Cheque) verifyChequeAgainstLast(lastCheque *Cheque, expectedAmount *uint256.Uint256) error { actualAmount := uint256.New().Copy(cheque.CumulativePayout) if lastCheque != nil { if cheque.CumulativePayout.Cmp(lastCheque.CumulativePayout) < 1 { - return nil, fmt.Errorf("wrong cheque parameters: expected cumulative payout larger than %v, was: %v", lastCheque.CumulativePayout, cheque.CumulativePayout) + return fmt.Errorf("wrong cheque parameters: expected cumulative payout larger than %v, was: %v", lastCheque.CumulativePayout, cheque.CumulativePayout) } actualAmount.Sub(actualAmount, lastCheque.CumulativePayout) } if !expectedAmount.Equals(actualAmount) { - return nil, fmt.Errorf("unexpected amount for honey, expected %v was %v", expectedAmount, actualAmount) + return fmt.Errorf("unexpected amount for honey, expected %v was %v", expectedAmount, actualAmount) } - return actualAmount, nil + return nil } func (cheque *Cheque) String() string { diff --git a/swap/honeyOracle.go b/swap/honeyOracle.go deleted file mode 100644 index 50ae044936..0000000000 --- a/swap/honeyOracle.go +++ /dev/null @@ -1,41 +0,0 @@ -// Copyright 2019 The go-ethereum Authors -// This file is part of the go-ethereum library. -// -// The go-ethereum library is free software: you can redistribute it and/or modify -// it under the terms of the GNU Lesser General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. -// -// The go-ethereum library is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU Lesser General Public License for more details. -// -// You should have received a copy of the GNU Lesser General Public License -// along with the go-ethereum library. If not, see . - -package swap - -// HoneyOracle is the interface through which Oracles will deliver prices -type HoneyOracle interface { - GetPrice(honey uint64) (uint64, error) -} - -// NewHoneyPriceOracle returns the actual oracle to be used for discovering the price -// It will return a default one -func NewHoneyPriceOracle() HoneyOracle { - - return &fixedPriceOracle{ - honeyPrice: defaultHoneyPrice, - } -} - -// fixedPriceOracle is a price oracle which which returns a fixed price -type fixedPriceOracle struct { - honeyPrice uint64 -} - -// GetPrice returns the actual price for honey -func (cpo *fixedPriceOracle) GetPrice(honey uint64) (uint64, error) { - return honey * cpo.honeyPrice, nil -} diff --git a/swap/peer.go b/swap/peer.go index 78e0786006..04d6b1a029 100644 --- a/swap/peer.go +++ b/swap/peer.go @@ -161,15 +161,8 @@ func (p *Peer) createCheque() (*Cheque, error) { } // the balance should be negative here, we take the absolute value: honey := uint64(-p.getBalance()) - - oraclePrice, err := p.swap.honeyPriceOracle.GetPrice(honey) - if err != nil { - return nil, fmt.Errorf("error getting price from oracle: %v", err) - } - price := uint256.FromUint64(oraclePrice) - cumulativePayout := p.getLastSentCumulativePayout() - newCumulativePayout, err := uint256.New().Add(cumulativePayout, price) + newCumulativePayout, err := uint256.New().Add(cumulativePayout, uint256.FromUint64(honey)) if err != nil { return nil, err } diff --git a/swap/prices.go b/swap/prices.go index 60a1ec2240..0b6bf3860d 100644 --- a/swap/prices.go +++ b/swap/prices.go @@ -44,6 +44,4 @@ allowing for a multi-currency design. const ( RetrieveRequestPrice = uint64(8043036262) ChunkDeliveryPrice = uint64(17672687) - // default conversion of honey into output currency - currently ETH in Wei - defaultHoneyPrice = uint64(1) ) diff --git a/swap/swap.go b/swap/swap.go index 0417a7294f..3a4faa5aca 100644 --- a/swap/swap.go +++ b/swap/swap.go @@ -62,7 +62,6 @@ type Swap struct { params *Params // economic and operational parameters contract contract.Contract // reference to the smart contract chequebookFactory contract.SimpleSwapFactory // the chequebook factory used - honeyPriceOracle HoneyOracle // oracle which resolves the price of honey (in Wei) cashoutProcessor *CashoutProcessor // processor for cashing out logger Logger //Swap Logger } @@ -92,7 +91,6 @@ func newSwapInstance(stateStore state.Store, owner *Owner, backend chain.Backend owner: owner, params: params, chequebookFactory: chequebookFactory, - honeyPriceOracle: NewHoneyPriceOracle(), chainID: chainID, cashoutProcessor: newCashoutProcessor(backend, owner.privateKey), logger: logger, @@ -360,7 +358,7 @@ func (s *Swap) handleEmitChequeMsg(ctx context.Context, p *Peer, msg *EmitCheque }) } - _, err := s.processAndVerifyCheque(cheque, p) + err := s.processAndVerifyCheque(cheque, p) if err != nil { return protocols.Break(fmt.Errorf("processing and verifying received cheque: %w", err)) } @@ -458,29 +456,22 @@ func cashCheque(s *Swap, cheque *Cheque) { // processAndVerifyCheque verifies the cheque and compares it with the last received cheque // if the cheque is valid it will also be saved as the new last cheque // the caller is expected to hold p.lock -func (s *Swap) processAndVerifyCheque(cheque *Cheque, p *Peer) (*uint256.Uint256, error) { +func (s *Swap) processAndVerifyCheque(cheque *Cheque, p *Peer) error { if err := cheque.verifyChequeProperties(p, s.owner.address); err != nil { - return nil, err + return err } lastCheque := p.getLastReceivedCheque() - // TODO: there should probably be a lock here? - expectedAmount, err := s.honeyPriceOracle.GetPrice(cheque.Honey) - if err != nil { - return nil, err - } - - actualAmount, err := cheque.verifyChequeAgainstLast(lastCheque, uint256.FromUint64(expectedAmount)) - if err != nil { - return nil, err + if err := cheque.verifyChequeAgainstLast(lastCheque, uint256.FromUint64(cheque.Honey)); err != nil { + return err } // calculate tentative new balance after cheque is processed newBalance := p.getBalance() - int64(cheque.Honey) // check if this new balance would put creditor into debt if newBalance < -int64(ChequeDebtTolerance) { - return nil, fmt.Errorf("received cheque would result in balance %d which exceeds tolerance %d and would cause debt", newBalance, ChequeDebtTolerance) + return fmt.Errorf("received cheque would result in balance %d which exceeds tolerance %d and would cause debt", newBalance, ChequeDebtTolerance) } if err := p.setLastReceivedCheque(cheque); err != nil { @@ -488,7 +479,7 @@ func (s *Swap) processAndVerifyCheque(cheque *Cheque, p *Peer) (*uint256.Uint256 // TODO: what do we do here? Related issue: https://github.com/ethersphere/swarm/issues/1515 } - return actualAmount, nil + return nil } // loadLastReceivedCheque loads the last received cheque for the peer from the store diff --git a/swap/swap_test.go b/swap/swap_test.go index 42f04caebb..fdb6b52f14 100644 --- a/swap/swap_test.go +++ b/swap/swap_test.go @@ -1110,14 +1110,10 @@ func TestPeerVerifyChequeAgainstLast(t *testing.T) { t.Fatal(err) } - actualAmount, err := newCheque.verifyChequeAgainstLast(oldCheque, increase) + err = newCheque.verifyChequeAgainstLast(oldCheque, increase) if err != nil { t.Fatalf("failed to verify cheque compared to old cheque: %v", err) } - - if !actualAmount.Equals(increase) { - t.Fatalf("wrong actual amount, expected: %v, was: %v", increase, actualAmount) - } } // TestPeerVerifyChequeAgainstLastInvalid tests that verifyChequeAgainstLast rejects cheques with lower amount or an unexpected value @@ -1128,7 +1124,7 @@ func TestPeerVerifyChequeAgainstLastInvalid(t *testing.T) { oldCheque := newTestCheque() newCheque := newTestCheque() - if _, err := newCheque.verifyChequeAgainstLast(oldCheque, increase); err == nil { + if err := newCheque.verifyChequeAgainstLast(oldCheque, increase); err == nil { t.Fatal("accepted a cheque with same amount") } @@ -1145,7 +1141,7 @@ func TestPeerVerifyChequeAgainstLastInvalid(t *testing.T) { t.Fatal(err) } - if _, err := newCheque.verifyChequeAgainstLast(oldCheque, increase); err == nil { + if err := newCheque.verifyChequeAgainstLast(oldCheque, increase); err == nil { t.Fatal("accepted a cheque with unexpected amount") } } @@ -1159,15 +1155,11 @@ func TestPeerProcessAndVerifyCheque(t *testing.T) { cheque := newTestCheque() cheque.Signature, _ = cheque.Sign(ownerKey) - actualAmount, err := swap.processAndVerifyCheque(cheque, peer) + err := swap.processAndVerifyCheque(cheque, peer) if err != nil { t.Fatalf("failed to process cheque: %s", err) } - if !actualAmount.Equals(cheque.CumulativePayout) { - t.Fatalf("computed wrong actual amount: was %v, expected: %v", actualAmount, cheque.CumulativePayout) - } - // verify that it was indeed saved if !peer.getLastReceivedCheque().CumulativePayout.Equals(cheque.CumulativePayout) { t.Fatalf("last received cheque has wrong cumulative payout, was: %v, expected: %v", peer.lastReceivedCheque.CumulativePayout, cheque.CumulativePayout) @@ -1182,7 +1174,7 @@ func TestPeerProcessAndVerifyCheque(t *testing.T) { otherCheque.Honey = 10 otherCheque.Signature, _ = otherCheque.Sign(ownerKey) - if _, err := swap.processAndVerifyCheque(otherCheque, peer); err != nil { + if err := swap.processAndVerifyCheque(otherCheque, peer); err != nil { t.Fatalf("failed to process cheque: %s", err) } @@ -1205,7 +1197,7 @@ func TestPeerProcessAndVerifyChequeInvalid(t *testing.T) { cheque.Beneficiary = ownerAddress cheque.Signature, _ = cheque.Sign(ownerKey) - if _, err := swap.processAndVerifyCheque(cheque, peer); err == nil { + if err := swap.processAndVerifyCheque(cheque, peer); err == nil { t.Fatal("accecpted an invalid cheque as first cheque") } @@ -1213,7 +1205,7 @@ func TestPeerProcessAndVerifyChequeInvalid(t *testing.T) { cheque = newTestCheque() cheque.Signature, _ = cheque.Sign(ownerKey) - if _, err := swap.processAndVerifyCheque(cheque, peer); err != nil { + if err := swap.processAndVerifyCheque(cheque, peer); err != nil { t.Fatalf("failed to process cheque: %s", err) } @@ -1230,7 +1222,7 @@ func TestPeerProcessAndVerifyChequeInvalid(t *testing.T) { otherCheque.Honey = 10 otherCheque.Signature, _ = otherCheque.Sign(ownerKey) - if _, err := swap.processAndVerifyCheque(otherCheque, peer); err == nil { + if err := swap.processAndVerifyCheque(otherCheque, peer); err == nil { t.Fatal("accepted a cheque with lower amount") } From 7ad6d8ababfe1bfb2850851b752a5b6a75135ebd Mon Sep 17 00:00:00 2001 From: Ralph Pichler Date: Fri, 13 Mar 2020 12:45:58 +0100 Subject: [PATCH 2/2] swap: remove outdated comment --- swap/cheque.go | 1 - 1 file changed, 1 deletion(-) diff --git a/swap/cheque.go b/swap/cheque.go index 0bd285ba06..849ba4f3fd 100644 --- a/swap/cheque.go +++ b/swap/cheque.go @@ -132,7 +132,6 @@ func (cheque *Cheque) verifyChequeProperties(p *Peer, expectedBeneficiary common } // verifyChequeAgainstLast verifies that the amount is higher than in the previous cheque and the increase is as expected -// returns the actual amount received in this cheque func (cheque *Cheque) verifyChequeAgainstLast(lastCheque *Cheque, expectedAmount *uint256.Uint256) error { actualAmount := uint256.New().Copy(cheque.CumulativePayout)