Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

removed TODOs which probably should not be fixed before merge #1637

Merged
merged 9 commits into from
Aug 5, 2019
Merged

removed TODOs which probably should not be fixed before merge #1637

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0046536
removed TODOs which probably should not be fixed before merge
Eknir Aug 2, 2019
2e7363e
more TODOs removed
Eknir Aug 2, 2019
7011e9e
removed one more TODO
Eknir Aug 2, 2019
759c978
removed more TODOs
Eknir Aug 2, 2019
becc05f
after review holisticode
Eknir Aug 2, 2019
b8d95b6
referenced issue at put back TODO
Eknir Aug 2, 2019
35ea5f5
swap: put in-code TODO back and changed position of NOTE
Eknir Aug 5, 2019
9565343
Merge branch 'incentives' into incentives_removeTODO
Eknir Aug 5, 2019
c5526b9
swap: restore 2 comments in handleEmitChequeMsg function
mortelli Aug 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions contracts/swap/swap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
holisticode marked this conversation as resolved.
Show resolved Hide resolved
func ValidateCode(ctx context.Context, b bind.ContractBackend, address common.Address) error {
codeReadFromAddress, err := b.CodeAt(ctx, address, nil)
if err != nil {
Expand Down
4 changes: 1 addition & 3 deletions swap/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1638

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is fine to relate this point to that issue, but really this is about experimenting with the values, which I feel that issue is not about. And this is not about actually having to do an issue with it, but for readers of the code to be aware of that these values are totally arbitrary for now.

I would suggest to keep it but to rename it to NOTE: instead of TODO:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done that! The issue which I created is still relevant though

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but you did not put the note at the correct spot. It is now above deployRetries which funny enough is not a deliberate value

DefaultInitialDepositAmount = 0

// NOTE: deliberate value for now; needs experimentation
deployRetries = 5
// delay between retries
deployDelay = 1 * time.Second
// Default timeout until cashing in cheques is possible - TODO: deliberate value, experiment
holisticode marked this conversation as resolved.
Show resolved Hide resolved
// 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.
Expand Down
1 change: 0 additions & 1 deletion swap/oracle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is part of the SWIP: payment module preferences. I chose not to make an issue for this one, as it is already kind-of in our backlog due to the SWIP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1632
This one is somehow related and should (I think) be implemented first

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although does the SWIP explicitly describe that the oracle be configurable via config flag?
If not, a separate issue might well be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not explicitly mention this and I think this is also still up to debate. My personal preference would be to set it in a config file (as it is not just mentioning the oracle, but actually a preference list for oracles). Anyways, the SWIP ensures we won't forget this and implementation details will be discussed for sure!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Fyi, Swarm supports different levels of configuration. It is possible to config via a config file with a define structure, and then this can be overridden by command line parameters and/or environment variables.

Thus whatever will be done for this will need to comply to this structure, don't invent a separate config file or something.

// For now it will return a default one
func NewPriceOracle() PriceOracle {
return &FixedPriceOracle{
Expand Down
9 changes: 1 addition & 8 deletions swap/peer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +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
holisticode marked this conversation as resolved.
Show resolved Hide resolved
// TODO: this should not be blocking
holisticode marked this conversation as resolved.
Show resolved Hide resolved
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())
Expand Down Expand Up @@ -97,20 +95,16 @@ 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
holisticode marked this conversation as resolved.
Show resolved Hide resolved
return
}
log.Info("submit tx mined", "receipt", receipt)

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
holisticode marked this conversation as resolved.
Show resolved Hide resolved
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
holisticode marked this conversation as resolved.
Show resolved Hide resolved
//TODO: make sure we make a case where we listen to the possibiliyt of the peer shutting down.
holisticode marked this conversation as resolved.
Show resolved Hide resolved
}()
holisticode marked this conversation as resolved.
Show resolved Hide resolved
return err
}
Expand All @@ -137,7 +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?
holisticode marked this conversation as resolved.
Show resolved Hide resolved
holisticode marked this conversation as resolved.
Show resolved Hide resolved
// TODO: what do we do here? Related issue: https://github.com/ethersphere/swarm/issues/1515
}

return actualAmount, nil
Expand Down Expand Up @@ -184,7 +178,6 @@ func verifyChequeAgainstLast(cheque *Cheque, lastCheque *Cheque, expectedAmount
actualAmount -= lastCheque.Amount
}

// TODO: maybe allow some range around expectedAmount?
holisticode marked this conversation as resolved.
Show resolved Hide resolved
if expectedAmount != actualAmount {
return 0, fmt.Errorf("unexpected amount for honey, expected %d was %d", expectedAmount, actualAmount)
}
Expand Down
12 changes: 1 addition & 11 deletions swap/protocol_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,17 +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: 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))
}
*/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to remain there. I added it, the test actually only checks that the balance is reset, but not that the cheque arrived.

Copy link
Contributor Author

@Eknir Eknir Aug 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@holisticode , I removed it and created a issue for it instead. Isn't it clearer when we keep all TODOs as issues? Obviously, if you want it back I will do so!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created an issue AND put the TODO back in the code with a reference to the issue

Copy link
Contributor

@holisticode holisticode Aug 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You did not put it back into the code though.

I personally don't think it's a good idea to put links to issues into the code. I preferred to have the code back instead of the issue number.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put it back.

holisticode marked this conversation as resolved.
Show resolved Hide resolved
//TODO: https://github.com/etherspherif crede/swarm/issues/1642
}

// TestTriggerPaymentThreshold is to test that the whole cheque protocol is triggered
Expand Down
3 changes: 0 additions & 3 deletions swap/swap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
holisticode marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return fmt.Errorf("error while storing the last cheque: %s", err.Error())
}
Expand All @@ -229,7 +228,6 @@ func (s *Swap) sendCheque(peer enode.ID) error {
}

// reset balance;
holisticode marked this conversation as resolved.
Show resolved Hide resolved
// TODO: if sending fails it should actually be roll backed...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created an issue for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1643

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the issue but I had requested it to be left in the code. Nevermind.

s.resetBalance(peer, int64(cheque.Amount))

return swapPeer.Send(context.Background(), emit)
Expand Down Expand Up @@ -408,7 +406,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?
holisticode marked this conversation as resolved.
Show resolved Hide resolved
return s.deploy(ctx, backend, path)
}

Expand Down
4 changes: 0 additions & 4 deletions swap/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
holisticode marked this conversation as resolved.
Show resolved Hide resolved
// FIXME: Check the contract bytecode of the counterparty
holisticode marked this conversation as resolved.
Show resolved Hide resolved

// ChequeParams encapsulate all cheque parameters
type ChequeParams struct {
Contract common.Address // address of chequebook, needed to avoid cross-contract submission
Expand All @@ -34,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
holisticode marked this conversation as resolved.
Show resolved Hide resolved
type Cheque struct {
ChequeParams
Signature []byte // signature Sign(Keccak256(contract, beneficiary, amount), prvKey)
Expand Down