-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Minor refactor to improve code health #2389
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
Conversation
This commit provides a minor fix on the format.
A minor refactor on method `OnGetData`.
Pull Request Test Coverage Report for Build 15872035018Details
💛 - Coveralls |
server.go
Outdated
| sp.server.syncManager.QueueHeaders(msg, sp.Peer) | ||
| } | ||
|
|
||
| // handleGetData is invoked when a peer receives a getdata bitcoin message and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit in commit message: s/prune/prone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
|
|
||
| // handleGetData is invoked when a peer receives a getdata bitcoin message and | ||
| // is used to deliver block and transaction information. | ||
| func (sp *serverPeer) OnGetData(_ *peer.Peer, msg *wire.MsgGetData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pre-existing but the method name is incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool now fixed
The preivous usage of two channels - by piping them together to create a semaphore effect, is difficult to follow and prone to bugs. This commit now refactors the method to explicitly implement a semaphore. Prior to this change, we would allow at max 3 concurrent goroutines - this is now bumped to 5.
ellemouton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
GustavoStingelin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! tACK!
Roasbeef
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 💃🏾
This PR introduces some refactoring to methods like
OnGetDataandBtcEncode, aiming to improve readability and code health.