-
Notifications
You must be signed in to change notification settings - Fork 66
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
A few minor cleanups for consistency with the orchard
builder API
#114
Conversation
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.
utACK
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.
Reviewed as of 950bcc4.
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.
Reviewed as of caae6c6.
@@ -788,13 +806,21 @@ impl<S: InProgressSignatures, V> Bundle<InProgress<Unproven, S>, V> { | |||
) -> Bundle<InProgress<Proven, S>, V> { | |||
let total_progress = | |||
self.shielded_spends().len() as u32 + self.shielded_outputs().len() as u32; | |||
self.map_authorization(CreateProofs::new( | |||
let mut cp = CreateProofs::new( |
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.
This change was very confusing to me until I realised that you were removing the MapAuth
trait in this commit simultaneously with the commit's documented change. The removal is unnecessary for adding dummy spends AFAICT (instead you'd have defined a new struct like CreateProofs
further down where an RNG was needed), so I'd have preferred that the MapAuth
trait removal happen separately. Not going to block on cleaning up the commits, but I'd prefer at least a mention of it in the commit message, if not an explicitly separate commit (like the subsequent TryMapAuth
removal commit, which was similarly confusing to me until I found where MapAuth
was removed).
I'm also confused because the reason the {Try}MapAuth
traits existed is that you advocated strongly for them over my closure-based design in the orchard
crate. This change appears to switch sapling-crypto
to match the orchard
crate, rather than the reverse (which is what I thought was the plan). What is your (updated) rationale for removing them? The only mention I can find in the PR is "in order to simplify handling of context values" in the changelog (which I agree with).
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.
I was unable to find a way to pass a mutable context through when constructing the MapAuth
in terms of a tuple of functions. When I saw how Orchard did it, it seemed much simpler, so I went with that.
Since TryMapAuth
is unused, we might as well get rid of it.
mut output_proof: impl FnMut(&mut R, A::OutputProof) -> B::OutputProof, | ||
mut auth_sig: impl FnMut(&mut R, A::AuthSig) -> B::AuthSig, | ||
mut auth: impl FnMut(&mut R, A) -> B, | ||
spend_proof: impl Fn(&mut R, A::SpendProof) -> B::SpendProof, |
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.
I think an Fn
might be fine here? I looked at the history of the equivalent APIs in the orchard
crate (where my closure design originated), and they were originally FnMut
s with no context argument, and then later an &mut
context argument was added to solve lifetime issues (because the individual FnMut
s couldn't all capture the same mutable context). So the FnMuts
are probably unnecessary now, as long as callers are happy passing all mutable context through the context field (even if it is only used by one of the closures).
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.
Yeah, I changed to use Fn
rather than FnMut
because it's unnecessary given the mutable context.
e7351c4
to
25bd572
Compare
…duced. This adds a flag to `BundleType` that, when set, requires a dummy outputs to be produced even if no outputs are added to the builder, and when unset results in standard padding.
Co-authored-by: str4d <[email protected]>
25bd572
to
93d369f
Compare
force-pushed to address review comments. |
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.
utACK 93d369f
No description provided.