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

Conversation

Eknir
Copy link
Contributor

@Eknir Eknir commented Aug 2, 2019

I removed the TODOs which should probably not be fixed before the merge into master. Created issues where needed, to not to forget about the TODOs. See comments to map the created issues to the removed TODOs

Not all TODO's are removed. Some I left because I think we can/should fix them before the merge. Some I left because I don't know how to make an issue out of them.

Kind request for the reviewers: Look at TODO's which are left and if you think an issue should be made out of it mention me (and I remove it) or do it yourself (permission hereby granted ;))

Copy link
Contributor

@mortelli mortelli left a comment

Choose a reason for hiding this comment

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

@Eknir, would it be too much trouble to add a comment to the removed TODOs linking the issues where they exist?

@Eknir
Copy link
Contributor Author

Eknir commented Aug 2, 2019

@mortelli , I did that. Please look at files changed. There you find the comments to all TODOs which I deleted

swap/defaults.go Show resolved Hide resolved
@@ -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.

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

@@ -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.

swap/peer.go Outdated Show resolved Hide resolved
swap/peer.go Show resolved Hide resolved
swap/peer.go Show resolved Hide resolved
swap/swap.go Show resolved Hide resolved
swap/types.go Show resolved Hide resolved
swap/types.go Show resolved Hide resolved
swap/peer.go Show resolved Hide resolved
Copy link
Contributor

@mortelli mortelli left a comment

Choose a reason for hiding this comment

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

A few more TODOs:

  • /contracts/swap/swap.go:46: I would remove this one. It appears the function is indeed not needed.
  • /contracts/swap/swap.go:94: not sure about this one.
  • swap/types.go:34: remove, we no longer request cheques.
  • swap/swap.go:222: remove, covered by Review error and exception handling #1601.
  • swap/swap.go:232: remove, covered by Review error and exception handling #1601.
  • swap/protocol_test.go:214: unsure about this one, not familiar enough with the tests here. Seems like suitable for a new issue, though.
  • swap/peer.go:68: leaning towards removing this one, where is the block?
  • swap/peer.go:122: not sure about this one.
  • swap/peer.go:135: remove, covered by Review error and exception handling #1601.
  • swap/peer.go:182: create a new issue for this one, then remove it.

Comments are appreciated 👍

@Eknir
Copy link
Contributor Author

Eknir commented Aug 2, 2019

/contracts/swap/swap.go:46: removed
/contracts/swap/swap.go:94: Removed although not sure about this one too. Does somebody object? @holisticode ?
swap/types.go:34: removed
swap/swap.go:222: remove, covered by #1601.
swap/swap.go:232: remove, covered by #1601.
swap/protocol_test.go:214: Removed, I will create an issue copying this code.
swap/peer.go:68:removed, the function calling this function (swap/peer.go handleMsg) is concurrent (it probably was not like this previously)
swap/peer.go:122: I leave it in, asking @holisticode for his advice
swap/peer.go:135: removed
swap/peer.go:182:removed, will create an issue

swap/peer.go Show resolved Hide resolved
swap/peer.go Show resolved Hide resolved
swap/peer.go Show resolved Hide resolved
swap/swap.go Show resolved Hide resolved
swap/swap.go Show resolved Hide resolved
Copy link
Contributor

@holisticode holisticode left a comment

Choose a reason for hiding this comment

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

Thanks for this tedious work!

There are a couple of things I don't agree with, please address them.

It is not necessarily a bad practice to leave TODOs in the code. Not everyone who actually looks at the code is in our team and knows all open issues. It is an open source project thus it may be a hint for a third party that things need to be done still.

contracts/swap/swap.go Show resolved Hide resolved
@@ -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

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:

swap/defaults.go Show resolved Hide resolved
@@ -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

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.

swap/peer.go Show resolved Hide resolved
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

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

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.

@@ -229,7 +228,6 @@ func (s *Swap) sendCheque(peer enode.ID) error {
}

// reset balance;
// 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.

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.

swap/types.go Show resolved Hide resolved
@@ -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

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

@mortelli
Copy link
Contributor

mortelli commented Aug 5, 2019

Even though we have issue #1635, I added 2 comments back to the handleEmitChequeMsg function, lest we forget about them: they are particularly tricky problems.

Unless there are objections, let's remove them in the PR for issue #1635.

@Eknir Eknir merged commit 36ec246 into incentives Aug 5, 2019
holisticode pushed a commit that referenced this pull request Aug 9, 2019
* removed TODOs which probably should not be fixed before merge

* more TODOs removed

* removed one more TODO

* removed more TODOs

* after review holisticode

* referenced issue at put back TODO

* swap: put in-code TODO back and changed position of NOTE

* swap: restore 2 comments in handleEmitChequeMsg function
holisticode pushed a commit that referenced this pull request Aug 15, 2019
* removed TODOs which probably should not be fixed before merge

* more TODOs removed

* removed one more TODO

* removed more TODOs

* after review holisticode

* referenced issue at put back TODO

* swap: put in-code TODO back and changed position of NOTE

* swap: restore 2 comments in handleEmitChequeMsg function
holisticode pushed a commit that referenced this pull request Aug 20, 2019
* removed TODOs which probably should not be fixed before merge

* more TODOs removed

* removed one more TODO

* removed more TODOs

* after review holisticode

* referenced issue at put back TODO

* swap: put in-code TODO back and changed position of NOTE

* swap: restore 2 comments in handleEmitChequeMsg function
holisticode pushed a commit that referenced this pull request Aug 26, 2019
* removed TODOs which probably should not be fixed before merge

* more TODOs removed

* removed one more TODO

* removed more TODOs

* after review holisticode

* referenced issue at put back TODO

* swap: put in-code TODO back and changed position of NOTE

* swap: restore 2 comments in handleEmitChequeMsg function
holisticode pushed a commit that referenced this pull request Aug 27, 2019
* removed TODOs which probably should not be fixed before merge

* more TODOs removed

* removed one more TODO

* removed more TODOs

* after review holisticode

* referenced issue at put back TODO

* swap: put in-code TODO back and changed position of NOTE

* swap: restore 2 comments in handleEmitChequeMsg function
@holisticode holisticode deleted the incentives_removeTODO branch September 2, 2019 15:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants