-
Notifications
You must be signed in to change notification settings - Fork 707
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
Refactor to prepare whitelist peers #2297
Conversation
A few points before you take a look:
edit: the latter points have been resolved, but you could still use some luck :) |
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.
I'm considedring having more of a hashmap peertype -> peerConnectionCount what do you think ? @massalabs/core-team
But first making things as generic as possible
And I'm really wondering what to do with the banned variant, I'd happily take any suggestions |
I'm working on fixing the tests, but let me know if you have any suggestions, especially on code readability |
If you have any scenario ideas with whitelist peers, I'd take them. To do next (maybe in some follow ups, maybe here)
|
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.
My personal belief is we should merge that PR as it since a lot of good refactorings was already done here, and all the items of the checklist you gave should be done as followups issues/PRs!
One last tweak/code change before merging could be to annotate function that share the same pattern but only differ by their types as "mostly duplicated" since they could be refactored in a second pass using generics :)
I'll move them around yet again to make more sense |
@AureliaDolo I have made a proposition to resolve some problems and improve genericity here : #2379 it's based on your PR. I have made a new PR instead of review because the changes suggested are too big. |
Proposition: Refactor whitelist peers
@AureliaDolo It's possible that my refactor broke some tests. Do you want me to fix this or you will do it ? |
…atch the enum-map behaviour.
…oved it. Fix default bootstrap peer connection type.
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.
As I have work partially on that I'm agree on this new design. Well tested. Nice refactor :)
most points were corrected but I didn't do a whole new pass
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! That's always a nice thing to use an Enum
rather than several conflicting boolean fields, nice refactoring here ✨
bors merge |
Merge conflict. |
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.
The merge don't change anything related to this PR.
bors merge |
Build succeeded: |
To prepare for #1946
I mostly created the PeerType variant and refactored a lot accordingly.
In a nutshell, instead of having many not so related indexes, we have now one index and one config per peer type. I tried my best match on peer types and avoid if as much as possible (if you see something other than stuff == PeerType::default() , there may be an underlying issue), and hide complexity is smaller methods.