Conversation
|
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. PR Code Suggestions ✨Explore these optional code suggestions:
|
dfb4fe8 to
1c6529c
Compare
PaulLaux
left a comment
There was a problem hiding this comment.
please update the branches (especially Orchard with regard to QED-it/orchard#119)
Cargo.toml
Outdated
| # - Orchard | ||
| nonempty = "0.7" | ||
| orchard = { version = "0.8.0", default-features = false, git = "https://github.com/QED-it/orchard", branch = "zsa1" } | ||
| orchard = { version = "0.8.0", default-features = false, git = "https://github.com/QED-it/orchard", branch = "spendauth_clone" } |
There was a problem hiding this comment.
Done, there is an open PR in Orchard
QED-it/orchard#125
.github/workflows/ci.yml
Outdated
| # strategy: | ||
| # matrix: | ||
| # target: | ||
| # - wasm32-wasi |
There was a problem hiding this comment.
There was a problem hiding this comment.
I've brought it back, let's see
Cargo.toml
Outdated
| zcash_note_encryption = { version = "0.4", git = "https://github.com/QED-it/zcash_note_encryption", branch = "zsa1" } | ||
| sapling = { package = "sapling-crypto", version = "0.1.3", git = "https://github.com/QED-it/sapling-crypto", branch = "zsa1" } | ||
| orchard = { version = "0.8.0", git = "https://github.com/QED-it/orchard", branch = "zsa1" } | ||
| orchard = { version = "0.8.0", default-features = false, git = "https://github.com/QED-it/orchard", branch = "spendauth_clone" } |
There was a problem hiding this comment.
I don't follow, shouldn't we use the branch from QED-it/orchard#119 ?
There was a problem hiding this comment.
It is based on the one you mention
There was a problem hiding this comment.
the one i mentioned is remove_circuit_param. But I prefer zsa1 and a commit hash
| &self.orchard_saks, | ||
| ) | ||
| }) | ||
| .unwrap(), |
There was a problem hiding this comment.
do not ignore the possibility of error here: (x2)
let orchard_bundle: Option<OrchardBundle<_>> = match unauthed_tx.orchard_bundle {
Some(OrchardBundle::OrchardVanilla(b)) => {
Some(OrchardBundle::OrchardVanilla(Box::new(
b.create_proof(
&orchard::circuit::ProvingKey::build::<OrchardVanilla>(),
&mut rng,
)
.and_then(|b| {
b.apply_signatures(
&mut rng,
*shielded_sig_commitment.as_ref(),
&self.orchard_saks,
)
})
.map_err(Error::OrchardBuild)?,
)))
}7bac6f1 to
387c5d8
Compare
PaulLaux
left a comment
There was a problem hiding this comment.
This looks much better! waiting for the test vectors.
This reverts commit f33189f.
205bc9f to
9f947f8
Compare
There was a problem hiding this comment.
in transaction/mod.rs, pub fn vanilla_bundle(&self) should be
pub fn as_vanilla_bundle(&self) -> &Bundle<A, Amount, OrchardVanilla> {
match self {
OrchardBundle::OrchardVanilla(b) => b,
// #[cfg(zcash_unstable = "nu6" /* TODO nu7 */ )]
OrchardBundle::OrchardZSA(_) => panic!("Wrong bundle type"),
}
}and have a as_orchard_zsa() function for symmetry.
| _ => Err(Box::leak( | ||
| format!("Unknown consensus branch ID: {:08x}", value).into_boxed_str(), | ||
| )), |
There was a problem hiding this comment.
wow
try
type Error = String;
fn try_from(value: u32) -> Result<Self, Self::Error> {
match value {
0 => Ok(BranchId::Sprout),
...
_=> Err(format!("Unknown consensus branch ID: {:08x}", value)),
}and update the caller to &e, if you really want this.
Also note and fix the format of the caller: "Unknown consensus branch ID:" is already there (their mistake, but still)
There was a problem hiding this comment.
I removed that, that was my debug code, that leaked to production
PaulLaux
left a comment
There was a problem hiding this comment.
much better, added comments.
Also, for OrchardBundle remove the Boxes, it is not needed now.
| let (bundle, meta) = builder.build(&mut rng).map_err(Error::OrchardBuild)?; | ||
|
|
||
| unproven_orchard_zsa_bundle = Some(bundle); | ||
| unproven_orchard_bundle = Some(OrchardBundle::OrchardZSA(Box::new(bundle))); |
There was a problem hiding this comment.
as mentioned, boxes should be removed
| .and_then(|b| { | ||
| b.apply_signatures( | ||
| &mut rng, | ||
| *shielded_sig_commitment.as_ref(), | ||
| &self.orchard_saks, | ||
| ) | ||
| }) | ||
| .map_err(Error::OrchardBuild)?, | ||
| ))) | ||
| } |
There was a problem hiding this comment.
consider
fn prove_and_sign<B>(
bundle: B,
rng: &mut impl RngCore,
proving_key: &orchard::circuit::ProvingKey<B>,
shielded_sig_commitment: &[u8; 32],
orchard_saks: &Vec<orchard::keys::SpendingKey>,
) -> Result<B, Error<FE>>
where
B: orchard::bundle::Bundle + Sized,
{
bundle
.create_proof(proving_key, rng)
.and_then(|b| b.apply_signatures(rng, *shielded_sig_commitment, orchard_saks))
.map_err(Error::OrchardBuild)
}instead of repeating the same logic twice
| writer.write_all(&<[u8; 64]>::from( | ||
| bundle.authorization().binding_signature(), | ||
| ))?; | ||
| match bundle { |
There was a problem hiding this comment.
if let Some(bundle) = bundle { (bundle is already a ref)
| } | ||
|
|
||
| /// Writes an [`Bundle`] in the appropriate transaction format. | ||
| pub fn write_orchard_vanilla_bundle<W: Write>( |
There was a problem hiding this comment.
please make sure that the diff between upstream and us is just a rename from write_v5_bundle() and nothing more. (not the content of the function)
There was a problem hiding this comment.
I've synced them, there is only small difference in writing 0 if dundle is None, I do it one layer higher. Everything related to actual write of non-empty bundle is identical
| // overwrite the value balance, as we can't guarantee that the | ||
| // value doesn't exceed the MAX_MONEY bounds. | ||
| bundle.try_map_value_balance::<_, (), _>(|_| Ok(orchard_value_balance)).unwrap() | ||
| let _bundle: Bundle<_, _, OrchardZSA> = bundle.try_map_value_balance::<_, (), _>(|_| Ok(orchard_value_balance)).unwrap(); |
There was a problem hiding this comment.
: Bundle<_, _, OrchardZSA> is redundent, so is the _bundle var
| orchard_bundle, | ||
| #[cfg(zcash_unstable = "nu6" /* TODO nu7 */ )] | ||
| orchard_zsa_bundle: None, | ||
| orchard_bundle: orchard_bundle.map(|b| OrchardBundle::OrchardVanilla(Box::new(b))), |
There was a problem hiding this comment.
orchard_bundle: orchard_bundle.map(OrchardBundle::OrchardVanilla),
| sapling_bundle, | ||
| orchard_bundle: None, | ||
| orchard_zsa_bundle, | ||
| orchard_bundle: orchard_zsa_bundle.map(|b| OrchardBundle::OrchardZSA(Box::new(b))), |
There was a problem hiding this comment.
same
orchard_bundle: orchard_zsa_bundle.map(OrchardBundle::OrchardZSA),
| use crate::{ | ||
| consensus::BranchId, legacy::Script, transaction::components::amount::NonNegativeAmount, | ||
| }; |
There was a problem hiding this comment.
we should not move existing unrelated imports
| orchard_bundle.map(|b| b.commitment().0) | ||
| orchard_bundle.map(|b| { | ||
| match b { | ||
| OrchardBundle::OrchardVanilla(vanilla_bundle) => vanilla_bundle.commitment().0, |
There was a problem hiding this comment.
reduce boilerplate
OrchardVanilla(b) => b.commitment().0, (import + rename) (x2)
There was a problem hiding this comment.
Not sure what we reduce here. Just var name length? Then it's shadowing which is also not very nice
There was a problem hiding this comment.
as discussed, import OrchardVanillaand vanilla_bundle -> v ..
| orchard_bundle.map_or_else(bundle::commitments::hash_bundle_auth_empty, |b| { | ||
| b.authorizing_commitment().0 | ||
| match b { | ||
| OrchardBundle::OrchardVanilla(vanilla_bundle) => { |
There was a problem hiding this comment.
same OrchardVanilla(b) => b.authorizing_commitment().0, (x2)
PaulLaux
left a comment
There was a problem hiding this comment.
very nice, two small comments and merge it.
| use crate::{ | ||
| consensus::BranchId, legacy::Script, transaction::components::amount::NonNegativeAmount, | ||
| }; |
There was a problem hiding this comment.
It is done automatically by 'cargo fmt'
| orchard_bundle.map(|b| b.commitment().0) | ||
| orchard_bundle.map(|b| { | ||
| match b { | ||
| OrchardBundle::OrchardVanilla(vanilla_bundle) => vanilla_bundle.commitment().0, |
There was a problem hiding this comment.
as discussed, import OrchardVanillaand vanilla_bundle -> v ..
User description
Replace 2 separate Orchard bundle with a single enum
PR Type
enhancement
Description
OrchardBundleenum to consolidate the handling of Orchard bundles, replacing separate variables for different bundle types.BuildBundletrait for constructingOrchardVanillaandOrchardZSAbundles.OrchardBundleenum, simplifying the code and removing redundancy.orcharddependency branch inCargo.tomltospendauth_clone.Changes walkthrough 📝
builder.rs
Consolidate Orchard bundle handling with `OrchardBundle` enumzcash_primitives/src/transaction/builder.rs
OrchardBundleenum.
OrchardBundleenum types.unproven_orchard_zsa_bundlevariable.orchard.rs
Implement `BuildBundle` trait and update bundle handlingzcash_primitives/src/transaction/components/orchard.rs
BuildBundletrait for constructing bundles.BuildBundleforOrchardVanillaandOrchardZSA.OrchardBundle.mod.rs
Unify Orchard bundle handling with `OrchardBundle` enumzcash_primitives/src/transaction/mod.rs
OrchardBundleenum to unify bundle handling.OrchardBundle.OrchardZSAbundles.txid.rs
Update transaction digest logic for `OrchardBundle`zcash_primitives/src/transaction/txid.rs
OrchardBundle.OrchardZSA.tests.rs
Update tests for unified `OrchardBundle` handlingzcash_primitives/src/transaction/tests.rs
OrchardBundlehandling.OrchardZSA.Cargo.toml
Update `orchard` dependency branchCargo.toml
orcharddependency branch tospendauth_clone.