-
Notifications
You must be signed in to change notification settings - Fork 0
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
Update Orchard version #103
base: zsa1
Are you sure you want to change the base?
Conversation
alexeykoren
commented
Mar 4, 2025
- Use new Orchard version
- Use new lifecycle of IssueBundle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added questions
.github/workflows/ci.yml
Outdated
- name: Generate coverage report | ||
run: > | ||
cargo tarpaulin | ||
cargo +1.71.0 tarpaulin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain the changes to this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that GitHub action runners have been updated and no longer have 1.71.0 toolchain by default. So we need to install it manually. But here specifically we probably don't need it because cargo will pick it up from toolchain.toml
@@ -972,6 +975,7 @@ impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder< | |||
#[cfg(zcash_unstable = "nu6" /* TODO nu7 */ )] | |||
let issue_bundle = unauthed_tx | |||
.issue_bundle | |||
.map(|b| b.update_rho(first_nullifier(&orchard_bundle).unwrap())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the point of Result<>
if we don't check it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not so much indeed, Error scenario is extremely unlikely here, so we can simplify the flow to just panic inside the function in few extremely rare situation
match orchard_bundle { | ||
Some(OrchardBundle::OrchardVanilla(_)) => Err(io::Error::new( | ||
io::ErrorKind::Other, | ||
"Incorrect bundle type", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the function is named first_nullifier
, why block it for Vanilla?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is only relevant for ZSA/Swap, in case of Vanilla you never call this method and if you managed somehow - it will just panic