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

Remove NodeImplementation Generics from Network Types #2073

Merged
merged 12 commits into from
Nov 20, 2023

Conversation

bfish713
Copy link
Collaborator

This PR does 2 things primarily.

  1. Makes Membership something that is specified on NodeType. This means that we specify 1 Membership type for everything. Just because a Membership is the same type doesn't mean it describes the same thing. So we can still have a Quorum with 1000s of nodes and a committee of 10 nodes. What this describes is that anything describing membership uses the same types throughout the system. Currently this fits our model completely ,we use the same membership type for every Exchange right now (the type is GeneralStaticCommittee). When we implement Variable stake I can foresee us using just one type as well. If we do need to create a different Membership impl for DA, or VID, or something, I propose we simply add this type to NodeType. That change should be very simple to do after this PR.
  2. Remove NodeImplementation generic parameters for a ton of stuff. We had been passing this all over the code, specifically the newtork stack (Messages, ConnectedNetworks, and CommunicationChannels) mostly just to pull out various Memberships Removing this was mostly a side effect of the first change.

Both of these changes will make removing the Exchanges traits much easier.

This depends on: #2071 so review that one first because this branch includes those changes as well.

For reviewing this I would focus on the changes to in node_implementation.rs, vote2.rs, and simple_vote.rs. Also note the remove of the generic parameters from various structs/traits. The rest is simply finding and removing the parameters everywhere.

@bfish713 bfish713 linked an issue Nov 16, 2023 that may be closed by this pull request
@bfish713 bfish713 marked this pull request as ready for review November 17, 2023 15:22
elliedavidson
elliedavidson previously approved these changes Nov 17, 2023
@bfish713 bfish713 dismissed elliedavidson’s stale review November 17, 2023 21:00

The merge-base changed after approval.

elliedavidson
elliedavidson previously approved these changes Nov 20, 2023
@bfish713 bfish713 dismissed elliedavidson’s stale review November 20, 2023 18:49

The merge-base changed after approval.

@bfish713 bfish713 merged commit 2163584 into develop Nov 20, 2023
4 of 5 checks passed
@bfish713 bfish713 deleted the bf/remove-I-generics branch November 20, 2023 22:32
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.

Remove Exchanges
2 participants