Conversation
Cargo.toml
Outdated
| harness = false | ||
|
|
||
| [patch.crates-io] | ||
| zcash_note_encryption = { version = "0.4", git = "https://github.com/QED-it/zcash_note_encryption", branch = "resolve_zcash_pr2_issues" } |
There was a problem hiding this comment.
Are we going to merge zcash_note_encryption first and use zsa1 here?
src/note_encryption.rs
Outdated
| )) | ||
| } | ||
|
|
||
| fn parse_note_plaintext_bytes(plaintext: &[u8]) -> Option<Self::NotePlaintextBytes> { |
There was a problem hiding this comment.
I don't like the word "parse" here, but that's super minor concern
There was a problem hiding this comment.
The idea of having these functions with such names comes from str4d's review comment. Because our initial approach using the From trait did not provide information about errors (and a possible TryFrom replacement did not align with their approach of using Option instead of Error).
There's also an interesting question from @PaulLaux in the corresponding orchard PR review: QED-it/orchard#111 (comment). Now, I am reconsidering whether we really need to implement these functions in bothorchard and sapling-crypto. We could implement them directly in the zcash_note_encryption crate using the new from_slice and from_slice_with_tag methods of the NoteBytes crate (I have described this approach in response to @PaulLaux's review in that orchard PR).
PaulLaux
left a comment
PaulLaux
left a comment
There was a problem hiding this comment.
- remove both
FIXMEcomments. - Let's merge
zcash_note_encryptionand change tozsa1branch.
src/bundle.rs
Outdated
| } | ||
|
|
||
| fn enc_ciphertext_compact(&self) -> <SaplingDomain as Domain>::CompactNoteCiphertextBytes { | ||
| todo!() |
There was a problem hiding this comment.
unimplemented!("This function is not required for sapling")
seems like a better option
PaulLaux
left a comment
PaulLaux
left a comment
There was a problem hiding this comment.
some minor comments and same question for parse_note_..() functions.
src/note_encryption.rs
Outdated
| input[COMPACT_NOTE_SIZE..NOTE_PLAINTEXT_SIZE].copy_from_slice(&memo[..]); | ||
|
|
||
| NotePlaintextBytes(input) | ||
| // FIXME: avoid unwrap usage |
src/note_encryption.rs
Outdated
| } | ||
|
|
||
| fn enc_ciphertext_compact(&self) -> <SaplingDomain as Domain>::CompactNoteCiphertextBytes { | ||
| // FIXME: avoid unwrap usage |
Replied about |
PaulLaux
changed the title
3 participants
This PR synchronizes the
sapling-cryptorepository with the updates from PR #2 in thezcash_note_encryptionrepository. The changes include:parse_note_plaintext_bytes,parse_note_ciphertext_bytes, andparse_compact_note_plaintext_bytesmethods of theDomaintrait fromzcash_note_encryption.Domaintrait:NotePlaintextBytes,NoteCiphertextBytes,CompactNotePlaintextBytes,CompactNoteCiphertextBytesNoteBytestrait andNoteBytesDatastructure fromzcash_note_encryption.Note
This PR uses the
resolve_zcash_pr2_issuesbranch ofzcash_note_encryption. Before merging this PR, PR #10 needs to be merged into thezsa1branch ofzcash_note_encryption. Additionally, thissapling-cryptoPR branch should be updated to use thezsa1branch ofzcash_note_encryption.