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

Move peer.PrepareURL to net.SubstituteGenesisID #1796

Merged

Conversation

tsachiherman
Copy link
Contributor

@tsachiherman tsachiherman commented Dec 29, 2020

Summary

The wsPeer doesn't provide any value in performing the GenesisID substitution; it's just reading the current value from the wsNetwork and calling the strings.Replace function. Moving this function to wsNetwork makes it much more encapsulated and avoid unneeded referencing.

The long-run plan is to try and de-couple the wsPeer from wsNetwork so that wsPeer would have a wsNetwork interface to work with ( not the GossipNode, but something more suitable for that particular "link" ). Once this is achieved, we should be able to write better testing for wsPeer without relying on the massive wsNetwork implementation.

Test Plan

Existing unit tests were updated.

@tsachiherman tsachiherman self-assigned this Dec 29, 2020
@tsachiherman tsachiherman marked this pull request as ready for review December 29, 2020 18:18
@tsachiherman tsachiherman merged commit 9a0b439 into algorand:master Jan 11, 2021
@tsachiherman tsachiherman deleted the tsachi/refactor_prepare_url branch January 11, 2021 18:40
tsachiherman added a commit to tsachiherman/go-algorand that referenced this pull request Jul 7, 2021
The `wsPeer` doesn't provide any value in performing the GenesisID substitution; it's just reading the current value from the `wsNetwork` and calling the `strings.Replace` function. Moving this function to `wsNetwork` makes it much more encapsulated and avoid unneeded referencing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants