A few minor cleanups for consistency with the orchard builder API
#114
Conversation
| @@ -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.
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.
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.
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 FnMuts with no context argument, and then later an &mut context argument was added to solve lifetime issues (because the individual FnMuts 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.
Yeah, I changed to use Fn rather than FnMut because it's unnecessary given the mutable context.
nuttycom
force-pushed
the
sapling_builder_cleanup
branch
from
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 <jack@electriccoin.co>
nuttycom
force-pushed
the
sapling_builder_cleanup
branch
from
25bd572
to
93d369f
Compare
|
force-pushed to address review comments. |
No description provided.