From 00465367470a579a9073239840bd16323cd341f3 Mon Sep 17 00:00:00 2001 From: eknir Date: Fri, 2 Aug 2019 11:03:17 +0200 Subject: [PATCH 1/8] removed TODOs which probably should not be fixed before merge --- swap/defaults.go | 2 -- swap/oracle.go | 1 - swap/peer.go | 4 ---- 3 files changed, 7 deletions(-) diff --git a/swap/defaults.go b/swap/defaults.go index 2ed14f1bf3..75ed5df81c 100644 --- a/swap/defaults.go +++ b/swap/defaults.go @@ -25,13 +25,11 @@ const ( DefaultPaymentThreshold = 1000000 DefaultDisconnectThreshold = 1500000 // DefaultInitialDepositAmount is the default amount to send to the contract when initially deploying - // TODO: deliberate value for now; needs experimentation DefaultInitialDepositAmount = 0 deployRetries = 5 // delay between retries deployDelay = 1 * time.Second - // Default timeout until cashing in cheques is possible - TODO: deliberate value, experiment // Should be non-zero once we implement waivers defaultCashInDelay = uint64(0) // This is the amount of time in seconds which an issuer has to wait to decrease the harddeposit of a beneficiary. diff --git a/swap/oracle.go b/swap/oracle.go index 38b5250663..d60e35cb9f 100644 --- a/swap/oracle.go +++ b/swap/oracle.go @@ -22,7 +22,6 @@ type PriceOracle interface { } // NewPriceOracle returns the actual oracle to be used for discovering the price -// TODO: Add a config flag so that this can be configured via command line // For now it will return a default one func NewPriceOracle() PriceOracle { return &FixedPriceOracle{ diff --git a/swap/peer.go b/swap/peer.go index 484f6d216c..c48b04abc3 100644 --- a/swap/peer.go +++ b/swap/peer.go @@ -97,7 +97,6 @@ func (sp *Peer) handleEmitChequeMsg(ctx context.Context, msg *EmitChequeMsg) err receipt, err := otherSwap.SubmitChequeBeneficiary(opts, sp.backend, big.NewInt(int64(cheque.Serial)), big.NewInt(int64(cheque.Amount)), big.NewInt(int64(cheque.Timeout)), cheque.Signature) if err != nil { log.Error("error calling submitChequeBeneficiary", "error", err) - //TODO: do something with the error return } log.Info("submit tx mined", "receipt", receipt) @@ -105,12 +104,9 @@ func (sp *Peer) handleEmitChequeMsg(ctx context.Context, msg *EmitChequeMsg) err receipt, err = otherSwap.CashChequeBeneficiary(opts, sp.backend, sp.swap.owner.Contract, big.NewInt(int64(actualAmount))) if err != nil { log.Error("Got error when calling cashChequeBeneficiary", "err", err) - //TODO: do something with the error return } log.Info("cash tx mined", "receipt", receipt) - //TODO: after the cashCheque is done, we have to watch the blockchain for x amount (25) blocks for reorgs - //TODO: make sure we make a case where we listen to the possibiliyt of the peer shutting down. }() return err } From 2e7363e7a2f780556477a226566b3e730815cadf Mon Sep 17 00:00:00 2001 From: eknir Date: Fri, 2 Aug 2019 11:56:07 +0200 Subject: [PATCH 2/8] more TODOs removed --- swap/swap.go | 1 - swap/types.go | 3 --- 2 files changed, 4 deletions(-) diff --git a/swap/swap.go b/swap/swap.go index a5da19c9c6..09634d8ba3 100644 --- a/swap/swap.go +++ b/swap/swap.go @@ -408,7 +408,6 @@ func (s *Swap) GetParams() *swap.Params { // Deploy deploys a new swap contract func (s *Swap) Deploy(ctx context.Context, backend swap.Backend, path string) error { - // TODO: What to do if the contract is already deployed? return s.deploy(ctx, backend, path) } diff --git a/swap/types.go b/swap/types.go index 64a1bc3f07..fc403d7b93 100644 --- a/swap/types.go +++ b/swap/types.go @@ -20,9 +20,6 @@ import ( "github.com/ethereum/go-ethereum/common" ) -// TODO: add handshake protocol where we exchange last cheque (this is useful if one node disconnects) -// FIXME: Check the contract bytecode of the counterparty - // ChequeParams encapsulate all cheque parameters type ChequeParams struct { Contract common.Address // address of chequebook, needed to avoid cross-contract submission From 7011e9e8246bc3dfbbba4e5e7a7f73177be4c537 Mon Sep 17 00:00:00 2001 From: eknir Date: Fri, 2 Aug 2019 13:57:14 +0200 Subject: [PATCH 3/8] removed one more TODO --- swap/peer.go | 1 - 1 file changed, 1 deletion(-) diff --git a/swap/peer.go b/swap/peer.go index c48b04abc3..3794580d54 100644 --- a/swap/peer.go +++ b/swap/peer.go @@ -65,7 +65,6 @@ func (sp *Peer) handleMsg(ctx context.Context, msg interface{}) error { // handleEmitChequeMsg should be handled by the creditor when it receives // a cheque from a debitor -// TODO: validate the contract address in the cheque to match the address given at handshake // TODO: this should not be blocking func (sp *Peer) handleEmitChequeMsg(ctx context.Context, msg *EmitChequeMsg) error { cheque := msg.Cheque From 759c978e5ee79d521cb222ce236f53b1bb3ba9f6 Mon Sep 17 00:00:00 2001 From: eknir Date: Fri, 2 Aug 2019 17:39:26 +0200 Subject: [PATCH 4/8] removed more TODOs --- contracts/swap/swap.go | 2 -- swap/peer.go | 3 --- swap/protocol_test.go | 11 ----------- swap/swap.go | 2 -- swap/types.go | 1 - 5 files changed, 19 deletions(-) diff --git a/contracts/swap/swap.go b/contracts/swap/swap.go index e4abdbd33c..0d4589fc37 100644 --- a/contracts/swap/swap.go +++ b/contracts/swap/swap.go @@ -43,7 +43,6 @@ var ( type Backend interface { bind.ContractBackend TransactionReceipt(ctx context.Context, txHash common.Hash) (*types.Receipt, error) - //TODO: needed? BalanceAt(ctx context.Context, address common.Address, blockNum *big.Int) (*big.Int, error) } // Deploy deploys an instance of the underlying contract and returns its `Contract` abstraction @@ -91,7 +90,6 @@ type Params struct { // ValidateCode checks that the on-chain code at address matches the expected swap // contract code. -// TODO: have this as a package level function and pass the SimpleSwapBin as argument func ValidateCode(ctx context.Context, b bind.ContractBackend, address common.Address) error { codeReadFromAddress, err := b.CodeAt(ctx, address, nil) if err != nil { diff --git a/swap/peer.go b/swap/peer.go index 3794580d54..4e7d5db7f0 100644 --- a/swap/peer.go +++ b/swap/peer.go @@ -65,7 +65,6 @@ func (sp *Peer) handleMsg(ctx context.Context, msg interface{}) error { // handleEmitChequeMsg should be handled by the creditor when it receives // a cheque from a debitor -// TODO: this should not be blocking func (sp *Peer) handleEmitChequeMsg(ctx context.Context, msg *EmitChequeMsg) error { cheque := msg.Cheque log.Debug("received emit cheque message from peer", "peer", sp.ID().String()) @@ -132,7 +131,6 @@ func (sp *Peer) processAndVerifyCheque(cheque *Cheque) (uint64, error) { if err := sp.saveLastReceivedCheque(cheque); err != nil { log.Error("error while saving last received cheque", "peer", sp.ID().String(), "err", err.Error()) - // TODO: what do we do here? } return actualAmount, nil @@ -179,7 +177,6 @@ func verifyChequeAgainstLast(cheque *Cheque, lastCheque *Cheque, expectedAmount actualAmount -= lastCheque.Amount } - // TODO: maybe allow some range around expectedAmount? if expectedAmount != actualAmount { return 0, fmt.Errorf("unexpected amount for honey, expected %d was %d", expectedAmount, actualAmount) } diff --git a/swap/protocol_test.go b/swap/protocol_test.go index 87526564e3..44dda9606c 100644 --- a/swap/protocol_test.go +++ b/swap/protocol_test.go @@ -210,17 +210,6 @@ func TestEmitCheque(t *testing.T) { if creditorSwap.balances[debitor.ID()] != 0 { t.Fatalf("Expected debitor balance to have been reset to %d, but it is %d", 0, creditorSwap.balances[debitor.ID()]) } - /* - TODO: This test actually fails now, because the two Swaps create independent backends, - thus when handling the cheque, it will actually complain (check ERROR log output) - with `error="no contract code at given address"`. - Therefore, the `lastReceivedCheque` is not being saved, and this check would fail. - So TODO is to find out how to address this (should be by having same backend when creating the Swap) - if creditorSwap.loadLastReceivedCheque(debitor.ID()) != cheque { - t.Fatalf("Expected exactly one cheque at creditor, but there are %d:", len(creditorSwap.cheques)) - } - */ - } // TestTriggerPaymentThreshold is to test that the whole cheque protocol is triggered diff --git a/swap/swap.go b/swap/swap.go index 09634d8ba3..ed40f764d6 100644 --- a/swap/swap.go +++ b/swap/swap.go @@ -219,7 +219,6 @@ func (s *Swap) sendCheque(peer enode.ID) error { s.cheques[peer] = cheque err = s.stateStore.Put(sentChequeKey(peer), &cheque) - // TODO: error handling might be quite more complex if err != nil { return fmt.Errorf("error while storing the last cheque: %s", err.Error()) } @@ -229,7 +228,6 @@ func (s *Swap) sendCheque(peer enode.ID) error { } // reset balance; - // TODO: if sending fails it should actually be roll backed... s.resetBalance(peer, int64(cheque.Amount)) return swapPeer.Send(context.Background(), emit) diff --git a/swap/types.go b/swap/types.go index fc403d7b93..199ea1177b 100644 --- a/swap/types.go +++ b/swap/types.go @@ -31,7 +31,6 @@ type ChequeParams struct { } // Cheque encapsulates the parameters and the signature -// TODO: There should be a request cheque struct that only gives the Serial type Cheque struct { ChequeParams Signature []byte // signature Sign(Keccak256(contract, beneficiary, amount), prvKey) From becc05fc8b8c7313a7690eca33243be8f8c841ac Mon Sep 17 00:00:00 2001 From: eknir Date: Fri, 2 Aug 2019 18:41:42 +0200 Subject: [PATCH 5/8] after review holisticode --- swap/defaults.go | 2 +- swap/peer.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/swap/defaults.go b/swap/defaults.go index 75ed5df81c..b0cdae4bc6 100644 --- a/swap/defaults.go +++ b/swap/defaults.go @@ -26,7 +26,7 @@ const ( DefaultDisconnectThreshold = 1500000 // DefaultInitialDepositAmount is the default amount to send to the contract when initially deploying DefaultInitialDepositAmount = 0 - + // NOTE: deliberate value for now; needs experimentation deployRetries = 5 // delay between retries deployDelay = 1 * time.Second diff --git a/swap/peer.go b/swap/peer.go index 4e7d5db7f0..55f6c62a35 100644 --- a/swap/peer.go +++ b/swap/peer.go @@ -131,6 +131,7 @@ func (sp *Peer) processAndVerifyCheque(cheque *Cheque) (uint64, error) { if err := sp.saveLastReceivedCheque(cheque); err != nil { log.Error("error while saving last received cheque", "peer", sp.ID().String(), "err", err.Error()) + // TODO: what do we do here? Related issue: https://github.com/ethersphere/swarm/issues/1515 } return actualAmount, nil From b8d95b64ea9b2b1ec752fd426e413080547e6356 Mon Sep 17 00:00:00 2001 From: eknir Date: Fri, 2 Aug 2019 18:56:10 +0200 Subject: [PATCH 6/8] referenced issue at put back TODO --- swap/protocol_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/swap/protocol_test.go b/swap/protocol_test.go index 44dda9606c..cc2abb0c66 100644 --- a/swap/protocol_test.go +++ b/swap/protocol_test.go @@ -210,6 +210,7 @@ func TestEmitCheque(t *testing.T) { if creditorSwap.balances[debitor.ID()] != 0 { t.Fatalf("Expected debitor balance to have been reset to %d, but it is %d", 0, creditorSwap.balances[debitor.ID()]) } + //TODO: https://github.com/etherspherif crede/swarm/issues/1642 } // TestTriggerPaymentThreshold is to test that the whole cheque protocol is triggered From 35ea5f5a53c724515eec2a3cfb86f3c7f3736bf0 Mon Sep 17 00:00:00 2001 From: eknir Date: Mon, 5 Aug 2019 09:19:41 +0200 Subject: [PATCH 7/8] swap: put in-code TODO back and changed position of NOTE --- swap/defaults.go | 4 ++-- swap/protocol_test.go | 11 ++++++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/swap/defaults.go b/swap/defaults.go index b0cdae4bc6..1647b7b283 100644 --- a/swap/defaults.go +++ b/swap/defaults.go @@ -25,9 +25,9 @@ const ( DefaultPaymentThreshold = 1000000 DefaultDisconnectThreshold = 1500000 // DefaultInitialDepositAmount is the default amount to send to the contract when initially deploying - DefaultInitialDepositAmount = 0 // NOTE: deliberate value for now; needs experimentation - deployRetries = 5 + DefaultInitialDepositAmount = 0 + deployRetries = 5 // delay between retries deployDelay = 1 * time.Second // Should be non-zero once we implement waivers diff --git a/swap/protocol_test.go b/swap/protocol_test.go index cc2abb0c66..0baa06bfb1 100644 --- a/swap/protocol_test.go +++ b/swap/protocol_test.go @@ -210,7 +210,16 @@ func TestEmitCheque(t *testing.T) { if creditorSwap.balances[debitor.ID()] != 0 { t.Fatalf("Expected debitor balance to have been reset to %d, but it is %d", 0, creditorSwap.balances[debitor.ID()]) } - //TODO: https://github.com/etherspherif crede/swarm/issues/1642 + /* + TODO: This test actually fails now, because the two Swaps create independent backends, + thus when handling the cheque, it will actually complain (check ERROR log output) + with `error="no contract code at given address"`. + Therefore, the `lastReceivedCheque` is not being saved, and this check would fail. + So TODO is to find out how to address this (should be by having same backend when creating the Swap) + if creditorSwap.loadLastReceivedCheque(debitor.ID()) != cheque { + t.Fatalf("Expected exactly one cheque at creditor, but there are %d:", len(creditorSwap.cheques)) + } + */ } // TestTriggerPaymentThreshold is to test that the whole cheque protocol is triggered From c5526b9e6611155342d7dfdd56895cbffef0f59e Mon Sep 17 00:00:00 2001 From: mortelli Date: Mon, 5 Aug 2019 11:57:58 -0300 Subject: [PATCH 8/8] swap: restore 2 comments in handleEmitChequeMsg function --- swap/peer.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/swap/peer.go b/swap/peer.go index d122a0eb9f..1d847e8145 100644 --- a/swap/peer.go +++ b/swap/peer.go @@ -97,6 +97,8 @@ func (sp *Peer) handleEmitChequeMsg(ctx context.Context, msg *EmitChequeMsg) err // blocks here, as we are waiting for the transaction to be mined receipt, err := otherSwap.SubmitChequeBeneficiary(opts, sp.backend, big.NewInt(int64(cheque.Serial)), big.NewInt(int64(cheque.Amount)), big.NewInt(int64(cheque.Timeout)), cheque.Signature) if err != nil { + // TODO: do something with the error + // and we actually need to log this error as we are in an async routine; nobody is handling this error for now log.Error("error submitting cheque", "err", err) return } @@ -104,6 +106,8 @@ func (sp *Peer) handleEmitChequeMsg(ctx context.Context, msg *EmitChequeMsg) err receipt, err = otherSwap.CashChequeBeneficiary(opts, sp.backend, sp.swap.owner.Contract, big.NewInt(int64(actualAmount))) if err != nil { + // TODO: do something with the error + // and we actually need to log this error as we are in an async routine; nobody is handling this error for now log.Error("error cashing cheque", "err", err) return }