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 1 commit
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 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

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
4 changes: 0 additions & 4 deletions swap/peer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,20 +97,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 Down