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

refactor: selective disclosure api #380

Merged
merged 13 commits into from
Feb 8, 2024
Merged

refactor: selective disclosure api #380

merged 13 commits into from
Feb 8, 2024

Conversation

sinui0
Copy link
Member

@sinui0 sinui0 commented Oct 31, 2023

This PR redesigns our selective disclosure API, inverting control over the parsed data structures, passing them out to the user instead.

All previous data format structures have been ripped out in favor of re-exporting types from spansy, and then leveraging the ToRangeSet trait from tlsn-utils in the tlsn-core API. Users who want to implement their own format types can if they wish, their types only need to implement ToRangeSet to integrate with tlsn-core.

It introduces two new traits, HttpCommit and JsonCommit which a user can either implement themselves, or just use our default committer. Both traits provide blanket impls for all the methods, so a user can override something without having to reimplement everything.

This API is by no means final. It will continue to evolve as we introduce new commitment types. There is also room for new abstractions for generating proofs similar to how we do commitments via the *Commit traits. For now I've kept that out of scope of this PR and users will have to use the SubstringsProofBuilder API directly (which isn't too bad because at least now they can use typed data instead of manually specifying ranges).

@sinui0 sinui0 requested review from themighty1 and th4s October 31, 2023 03:48
@themighty1
Copy link
Member

ack on the concept

@rozag
Copy link

rozag commented Oct 31, 2023

Nice! Looking forward to switching to this new API 👍

Copy link
Member

@th4s th4s left a comment

Choose a reason for hiding this comment

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

Yes I think this is a good approach 👍

@sinui0 sinui0 force-pushed the refactor/commit-prove-api branch from 676a6c4 to 5839d72 Compare February 6, 2024 20:28
@sinui0 sinui0 marked this pull request as ready for review February 6, 2024 20:30
@sinui0
Copy link
Member Author

sinui0 commented Feb 6, 2024

At last, this is ready for review.

We'll want to wait until outstanding PRs in tlsn-utils have been merged.

@sinui0 sinui0 requested review from th4s and themighty1 and removed request for themighty1 February 6, 2024 20:31
Copy link
Member

@themighty1 themighty1 left a comment

Choose a reason for hiding this comment

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

looks good,

can we use the same formatting for method args everywhere in this PR as per https://github.com/tlsnotary/tlsn/blob/dev/CONTRIBUTING.md#function-arguments

/// * arg1 - The first argument.
(The first letter capitalized and a period at the end)

tlsn/tlsn-core/src/proof/substrings.rs Outdated Show resolved Hide resolved
tlsn/tlsn-formats/src/http/mod.rs Outdated Show resolved Hide resolved
tlsn/tlsn-formats/src/http/commit.rs Outdated Show resolved Hide resolved
tlsn/tlsn-formats/src/http/commit.rs Show resolved Hide resolved
tlsn/tlsn-formats/src/http/commit.rs Outdated Show resolved Hide resolved
tlsn/tlsn-formats/src/http/commit.rs Show resolved Hide resolved
tlsn/tlsn-formats/src/json/commit.rs Outdated Show resolved Hide resolved
tlsn/tlsn-formats/src/json/commit.rs Outdated Show resolved Hide resolved
tlsn/tlsn-formats/src/json/commit.rs Outdated Show resolved Hide resolved
tlsn/tlsn-formats/src/json/commit.rs Show resolved Hide resolved
@themighty1 themighty1 self-requested a review February 8, 2024 08:14
Copy link
Member

@themighty1 themighty1 left a comment

Choose a reason for hiding this comment

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

gw

Ok(())
}

/// Commits to a JSON string.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Commits to a JSON string.
/// Commits to an unquoted JSON string.

tlsn/tlsn-formats/src/json/commit.rs Show resolved Hide resolved
Copy link
Member

@th4s th4s left a comment

Choose a reason for hiding this comment

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

🚀 gw. Very nice API.

tlsn/tlsn-core/src/commitment/builder.rs Show resolved Hide resolved
tlsn/tlsn-formats/src/http/commit.rs Show resolved Hide resolved
tlsn/tlsn-formats/src/http/commit.rs Show resolved Hide resolved
tlsn/tlsn-formats/src/json/commit.rs Show resolved Hide resolved
@sinui0 sinui0 merged commit bb50e3d into dev Feb 8, 2024
15 checks passed
@sinui0 sinui0 deleted the refactor/commit-prove-api branch February 8, 2024 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants