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

serde: implementation for proven transaction #337

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

hackaugusto
Copy link
Contributor

@hackaugusto hackaugusto commented Dec 4, 2023

as per the pr title.

fixes: #6

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 some comments inline - many of them are related to the question whether we want to serialize all field or skip computed fields. The former is more performant, but the latter guarantees data integrity and should reduce attack surface a bit.

objects/src/notes/metadata.rs Show resolved Hide resolved
objects/src/notes/vault.rs Outdated Show resolved Hide resolved
objects/src/notes/vault.rs Outdated Show resolved Hide resolved
objects/src/transaction/proven_tx.rs Show resolved Hide resolved
objects/src/transaction/proven_tx.rs Show resolved Hide resolved
objects/src/transaction/proven_tx.rs Outdated Show resolved Hide resolved
objects/src/notes/inputs.rs Outdated Show resolved Hide resolved
objects/src/transaction/proven_tx.rs Outdated Show resolved Hide resolved
objects/src/transaction/consumed_notes.rs Outdated Show resolved Hide resolved
objects/src/transaction/consumed_notes.rs Outdated Show resolved Hide resolved
@bobbinth
Copy link
Contributor

bobbinth commented Dec 5, 2023

Btw, I think this will also close #6.

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! Let's wait until we merge the relevant PR in Miden VM so that we can roll back changes in Cargo.toml.

@bobbinth bobbinth force-pushed the hacka-serde-prove-transaction branch from afde61c to c38f083 Compare December 7, 2023 07:37
@bobbinth
Copy link
Contributor

bobbinth commented Dec 7, 2023

I've updated NoteScript serialization to work with the latest updates in Winterfell/Miden VM.

@bobbinth bobbinth merged commit a95cdb8 into main Dec 7, 2023
6 checks passed
@bobbinth bobbinth deleted the hacka-serde-prove-transaction branch December 7, 2023 07:53
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.

Implement note serialization
2 participants