-
Notifications
You must be signed in to change notification settings - Fork 154
Add Descriptor::iter_pk
#823
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
base: master
Are you sure you want to change the base?
Conversation
This commit was -not- AI-generated, though probably it should've been.
d3fab75
to
82f86e1
Compare
This one -was- AI-generated, though I reviewed it.
82f86e1
to
aa42cd3
Compare
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.
On aa42cd3 successfully ran local tests
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.
One question. Rest LGTM
use crate::{miniscript, Miniscript, MiniscriptKey}; | ||
|
||
/// Iterator over all the keys in a descriptor. | ||
pub struct PkIter<'desc, Pk: MiniscriptKey> { |
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 is fine. But Ideally, we want this to be a sum type instead of a product type? But then we get whole loads of matches everywhere?
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 main reason to use a product type was that Taproot descriptors have both single keys and (optionally) a taptree iterator.
And in PkIter::next
I have a construction where the taptree iterator yields new Miniscript pkiters.
I can try to refactor this to use a sum type but I think it would result in more repeated/redundant code.
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 can try to refactor this to use a sum type but I think it would result in more repeated/redundant code.
Yeah, that would be messy. Ideally, we want to stick the mantra of "make invalid states unrepresentable"; but it might be overkill for this small change
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.
ACK aa42cd3
use crate::{miniscript, Miniscript, MiniscriptKey}; | ||
|
||
/// Iterator over all the keys in a descriptor. | ||
pub struct PkIter<'desc, Pk: MiniscriptKey> { |
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 can try to refactor this to use a sum type but I think it would result in more repeated/redundant code.
Yeah, that would be messy. Ideally, we want to stick the mantra of "make invalid states unrepresentable"; but it might be overkill for this small change
Fixes #821.
Should backport to 12.x.