Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

swarm/network/stream: remove dead code #19650

Closed
wants to merge 2 commits into from

Conversation

nonsense
Copy link
Member

This PR is removing dead code, which only increases complexity and is not used currently.

The P2P message serialised and sent across the wire is changing, as we no longer have HandoverProof, so I should check if this is backward compatible - not really sure how RLP marshal/unmarshal works in this case.

Apart from that, code used in production is equivalent before/after this PR.

This code has been in the codebase for a year. As a general rule-of-thumb, we should not be adding code to the codebase that is not directly used, or that we won't be using shortly after introduced, because this increases complexity and introduces unnecessary indirection.

@nonsense nonsense requested review from zelig, skylenet and acud May 31, 2019 10:44
@@ -474,7 +474,7 @@ type server struct {
// setNextBatch adjusts passed interval based on session index and whether
Copy link
Member Author

@nonsense nonsense May 31, 2019

Choose a reason for hiding this comment

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

@justelad I think we should extract this logic and have it unit tested. Basically the functionality which adjusts the input intervals and uses the adjusted intervals is not clear and not tested, which is weird, as there are very little input parameters to it:

  1. from
  2. to
  3. sessionIndex
  4. stream type (live or history)

This should have been unit tested, at least for the purpose of documentation.

Copy link
Member

Choose a reason for hiding this comment

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

this function should not even exist IMO. not sure why we would ever want to override a value. e.g. when syncing is live and from is 0... but i think that with the first offered hashes msg the client already gets the actual session index from the server with the from range specified on the message. a lot of the code here is not needed, some of it also should return errors directly (which it doesn't)

Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

kewl

@nonsense nonsense force-pushed the remove-dead-code-again branch from fc6dcb5 to e9f4d1a Compare May 31, 2019 14:00
@nonsense
Copy link
Member Author

nonsense commented Jun 3, 2019

This will be continued at https://github.com/ethersphere/swarm

@nonsense nonsense closed this Jun 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants