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

feat(nifs): Foldable wrapper for trace #357

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

cyphersnake
Copy link
Collaborator

Motivation
During the implementation of #316 we added a condition that consistency tokens started to be collapsed differently than all other instances, which in fact added a constraint to nifs::FS that now only traces with two elements in the first instance column can be collapsed. Otherwise, the behavior is undefined. This PR solves the problem of checking this restriction

Overview
The newtype pattern was used to solve the problem in this way. An example of using such a pattern is the NonZero<T> wrapper from the standard library. Now, instead of checking in all functions that involve consistency tokens, we accept vanilla::FoldablePlonkTrace instead of PlonkTrace. This type can only be created with a single constructor, which performs the trace validity check at the point of creation.

For instance:

// Old way with PlonkTrace
fn old_function(trace: &PlonkTrace) -> Result<(), Error> {
    // Every use site needed to check preconditions
    if trace.instances.first().map(|x| x.len()) != Some(2) {
        return Err(Error::InvalidTrace);
    }
    // function body...
}

// New way with FoldablePlonkTrace
fn new_function(foldable_trace: &FoldablePlonkTrace) -> Result<(), Error> {
    // No need for precondition checks here
    // function body...
}

This pattern encapsulates the validation logic within the FoldablePlonkTrace type, ensuring that all instances of FoldablePlonkTrace are valid by construction. This approach significantly reduces verbosity and potential for error, improving both safety and readability of the code. Moreover, it ensures that the invariant (traces having two elements in the first instance column) is enforced in a single place, thus avoiding scattered checks throughout the codebase.

impl<C: CurveAffine> FoldablePlonkInstance<C> {
    pub fn new(pl: PlonkInstance<C>) -> Option<Self> {
        matches!(pl.instances.first(), Some(instance) if instance.len() == 2).then_some(Self(pl))
    }
}

Here, the new method of FoldablePlonkInstance ensures that it can only be instantiated if the PlonkInstance meets the specific requirement. This is less verbose than returning an error through multiple layers of modules and provides a more accurate check compared to panicking if an implicit invariant is violated.

@cyphersnake cyphersnake requested a review from chaosma September 30, 2024 13:02
@cyphersnake cyphersnake self-assigned this Sep 30, 2024
Base automatically changed from 316-refactoring-permutation to main September 30, 2024 13:23
**Motivation**
During the implementation of #316 we added a condition that consistency
tokens started to be collapsed differently than all other instances,
which in fact added a constraint to `nifs::FS` that now only traces with
two elements in the first instance column can be collapsed. Otherwise,
the behavior is undefined. This PR solves the problem of checking this
restriction

**Overview**
The newtype pattern was used to solve the problem in this way. An example of using such a pattern is the `NonZero<T>` wrapper from the standard library. Now, instead of checking in all functions that involve consistency tokens, we accept `vanilla::FoldablePlonkTrace` instead of `PlonkTrace`. This type can only be created with a single constructor, which performs the trace validity check at the point of creation.

For instance:

```rust
// Old way with PlonkTrace
fn old_function(trace: &PlonkTrace) -> Result<(), Error> {
    // Every use site needed to check preconditions
    if trace.instances.first().map(|x| x.len()) != Some(2) {
        return Err(Error::InvalidTrace);
    }
    // function body...
}

// New way with FoldablePlonkTrace
fn new_function(foldable_trace: &FoldablePlonkTrace) -> Result<(), Error> {
    // No need for precondition checks here
    // function body...
}
```

This pattern encapsulates the validation logic within the `FoldablePlonkTrace` type, ensuring that all instances of `FoldablePlonkTrace` are valid by construction. This approach significantly reduces verbosity and potential for error, improving both safety and readability of the code. Moreover, it ensures that the invariant (traces having two elements in the first instance column) is enforced in a single place, thus avoiding scattered checks throughout the codebase.

```rust
impl<C: CurveAffine> FoldablePlonkInstance<C> {
    pub fn new(pl: PlonkInstance<C>) -> Option<Self> {
        matches!(pl.instances.first(), Some(instance) if instance.len() == 2).then_some(Self(pl))
    }
}
```

Here, the `new` method of `FoldablePlonkInstance` ensures that it can only be instantiated if the `PlonkInstance` meets the specific requirement. This is less verbose than returning an error through multiple layers of modules and provides a more accurate check compared to panicking if an implicit invariant is violated.
@cyphersnake cyphersnake force-pushed the 316-feat-foldable-wrapper branch from 5a66928 to c41c422 Compare September 30, 2024 13:26
@cyphersnake cyphersnake requested a review from chaosma October 2, 2024 10:26
@cyphersnake cyphersnake merged commit a52e179 into main Oct 2, 2024
1 check passed
@cyphersnake cyphersnake deleted the 316-feat-foldable-wrapper branch October 2, 2024 15: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.

2 participants