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

ToNullifier to ToInputNoteCommitment #732

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

hackaugusto
Copy link
Contributor

follow up to #726

Instead of removing the ToNullifier trait, this replaces said trait with ToInputNoteCommitment. The change is mainly the additional of the Option<NoteId>, which will be used by #353 .

The decision to keep the trait is due to code reuse. Mainly of the NoteInputs which does the validation for the number of inputs and computes the input notes commitment. The alternative of using a wrapper type ended duplicating validation code, when I tried to remove the duplicated the error handling become worse and introduced unwraps. Keeping the trait eliminates these concerns.

@hackaugusto hackaugusto requested a review from bobbinth June 5, 2024 12:46
@hackaugusto
Copy link
Contributor Author

Note: This doesn't yet change the kernel to use the nullifier, note_id_or_zero commitment. It only introduces the backbone changes to the rust code. This is so it is easier to review.

@hackaugusto hackaugusto force-pushed the hacka-tonullifier-to-toinputnotecommitment branch from a4ff4da to e2ac30b Compare June 5, 2024 12:47
@hackaugusto hackaugusto changed the title Hacka tonullifier to toinputnotecommitment ToNullifier to ToInputNoteCommitment Jun 5, 2024
@hackaugusto hackaugusto force-pushed the hacka-tonullifier-to-toinputnotecommitment branch from e2ac30b to b501b9e Compare June 6, 2024 08:13
Copy link
Contributor

@bobbinth bobbinth 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! Thank you! I left a few comments inline - but all of them are very minor (mostly naming-related).

objects/src/transaction/inputs.rs Outdated Show resolved Hide resolved
objects/src/transaction/inputs.rs Outdated Show resolved Hide resolved
objects/src/transaction/proven_tx.rs Outdated Show resolved Hide resolved
@hackaugusto hackaugusto force-pushed the hacka-tonullifier-to-toinputnotecommitment branch from b501b9e to 4a89424 Compare June 7, 2024 12:47
@hackaugusto hackaugusto force-pushed the hacka-tonullifier-to-toinputnotecommitment branch from 4a89424 to 6d70e27 Compare June 7, 2024 12:52
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

All looks good! Thank you!

@bobbinth bobbinth merged commit f475663 into next Jun 7, 2024
9 checks passed
@bobbinth bobbinth deleted the hacka-tonullifier-to-toinputnotecommitment branch June 7, 2024 18:56
@bobbinth bobbinth linked an issue Jun 8, 2024 that may be closed by this pull request
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.

Delegated note inclusion proofs
2 participants