Naming of property and variable parts and part is possibly misleading

[iancoleman]

The property KeyGen.parts (code below) is misleading and may benefit from being renamed proposal_states since the type being used is ProposalState.

https://github.com/maidsafe/BLS-DKG/blob/988d9f0c3362d57cfd5bc807a1aad37252e03eb6/src/key_gen/mod.rs#L306-L307

There's a struct Part (code below) which seems like KeyGen.parts would be, but it's not, it's a set of ProposalState

https://github.com/maidsafe/BLS-DKG/blob/988d9f0c3362d57cfd5bc807a1aad37252e03eb6/src/key_gen/mod.rs#L73-L84

KeyGen.parts is used in generate_keys (code below) to get a variable called part which is a ProposalState and not a Part. That's very confusing to me!

https://github.com/maidsafe/BLS-DKG/blob/988d9f0c3362d57cfd5bc807a1aad37252e03eb6/src/key_gen/mod.rs#L876-L880

It's especially confusing since Part and ProposalState both have the property commitment so could easily be using the wrong commitment without knowing it. Is it worth changing those properties to Part.our_commitment and ProposalState.proposed_commitment so they cannot be accidentally used interchangeably?

[iancoleman]

@oetyng I am aware you did some naming work in 18a4ff0 so am tagging you here too.

[oetyng]

Totally agree with the comments here @iancoleman 👍 I haven't been at this one for a while, but seems at minimum this can be made clearer, could be more things. I can have a look later and see what I find.

[iancoleman]

I did some experimental variable renaming and found that the heart of my confusion is captured in the last part of this line:

https://github.com/maidsafe/BLS-DKG/blob/988d9f0c3362d57cfd5bc807a1aad37252e03eb6/src/key_gen/mod.rs#L953-L956

A Part is being converted to a ProposalState (using the same commitment data).

To me this is lacking a necessary chronology in the naming. Perhaps duplicating ProposalState type into something like AckedState or JustifiedState or FinalState or CommitmentState, not sure but the Phase enum (code below) isn't really helping here because the phase I want to use in the name is called Commitment which would give a strange property of commitment_state.commitment.

https://github.com/maidsafe/BLS-DKG/blob/988d9f0c3362d57cfd5bc807a1aad37252e03eb6/src/key_gen/mod.rs#L170-L178

Phases should be I guess verbs or maybe adjectives, but the Commitment phase is a noun. This is also adding some confusion to me.

It seems as a general point this is code for managing commitments-with-metadata. The Part and the ProposalState sort of indicate this with a common property of commitment, so it seems those objects are really just commitment with differing metadata. Same for my suggested duplicate of ProposalState. The types currently seem very Phase oriented when perhaps they are more naturally Commitment oriented?

Hope this clarifies a bit further where my confusion comes from when reading the source. I think I am still a little confused but am making progress on understanding.

Also in 18a4ff0 there is a comment "Aligns with original paper"; is that original paper linked in the readme? I'm not sure what this is referring to but it would be helpful to know so I can appreciate what in the variables is domain language (so fairly fixed) and what is open to negotiation.

edit: I am making suggestions about changes here but they are not coming from a place of full understanding so are not 'real suggestions', just trying to capture my misunderstanding in a tangible way. If you feel these suggestions are misplaced I would accept that decision of course! For example I just realised I made an incorrect assumption that ProposalState would come chronologically before Part, but that is not true (I assumed this because Proposal feels like a thing done early and is pending decision, whereas Part sounds like an accepted thing ready to be formed into a whole thing). So converting a Part into a ProposalState makes sense after better understanding the code, but perhaps the naming does not indicate the progression very clearly without some extra context or domain knowledge.

[iancoleman]

See iancoleman@dadb309 iancoleman@5340661 for my ideas on variable names that I feel make it simpler to read and understand the code.

Still not sure if it's 100% there, some variables are maybe too long.

consumable_commitment and commitment_from_contribution could probably both be just commitment because it's clearer from their parent type Contribution and ContributionTracker what this is. But I thought for now being explicit in their naming better indicated how this data is being used in the code.