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

Improve type safeness of InitializedReport #729

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

mendess
Copy link
Collaborator

@mendess mendess commented Nov 28, 2024

InitializedReport has a Option<Vec<u8>> field which is only set by the helper, but this fact is known at compile time, it doesn't need to be encoded in a runtime option.

By replacing Option<Vec<u8>> with a generic that can either be () or WithPeerPrepShare we make it explicit which parts of the code are dealing with reports from a client and which parts are dealing with reports from a peer (leader).

In other words, we lift an invariant that was held by a comment

        // Set by the Helper.
        peer_prep_share: Option<Vec<u8>>

into one upheld by the compiler.

And finally instead of having on new constructor we have from_client and from_leader to make it obvious what the intent is when constructing, which in turn results in different sets of arguments being required (the former doesn't require a peer_prep_share where the latter does)

Other advantages of this change are:

  • we no longer make clippy angry that our function has too many arguments
  • we no longer need to ignore the Some case in leader code
  • we no longer need to handle the None case in helper code
  • we can no longer write tests that give the wrong idea of what the correct set of arguments are since it was incorrect to call new with the helper role and pass None to the peer_prep_share argument, but this test did it anyway

fn no_duplicates<I>(iterator: I) -> Result<(), I::Item>
where
I: Iterator,
I::Item: Eq + std::hash::Hash + Copy,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to remove the Copy constraint?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@mendess mendess force-pushed the mendess/initialize-report-builder branch from 11b958a to 6c68916 Compare December 3, 2024 10:36
@mendess mendess merged commit 69970f0 into main Dec 3, 2024
3 checks passed
@mendess mendess deleted the mendess/initialize-report-builder branch December 3, 2024 12:11
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.

2 participants