Skip to content

Commit 4137a59

Browse files
apoelstraapoelstra
committed
expression: encapsulate threshold parsing
Our current expression API has a `to_null_threshold` method, which works reasonably well but requires the caller translate the returned threshold by accessing individual children. We will later want to change the Tree represntation to make individual child access inefficient. To do this, encapsulate the threshold construction into a new verify_threshold. This previously was not done because our messy error structures made it very difficult to manage. But our error structures are less messy so it's possible now, though a bit of a hack. (We map everything to the global Error structure. Later we will do pretty-much the same thing but with ParseError in place of Error, which will be more elegant.)
1 parent 733bedd commit 4137a59

File tree

7 files changed

+52
-59
lines changed

7 files changed

+52
-59
lines changed

‎src/descriptor/sortedmulti.rs‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
6969

7070
let ret = Self {
7171
inner: tree
72-
.to_null_threshold()
73-
.map_err(Error::ParseThreshold)?
74-
.translate_by_index(|i| tree.args[i + 1].verify_terminal("public key"))
75-
.map_err(Error::Parse)?,
72+
.verify_threshold(|sub| sub.verify_terminal("public_key").map_err(Error::Parse))?,
7673
phantom: PhantomData,
7774
};
7875
ret.constructor_check()

‎src/expression/mod.rs‎

Lines changed: 37 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,43 @@ impl<'a> Tree<'a> {
239239
Ok((&self.args[0], &self.args[1]))
240240
}
241241

242+
/// Parses an expression tree as a threshold (a term with at least one child,
243+
/// the first of which is a positive integer k).
244+
///
245+
/// This sanity-checks that the threshold is well-formed (begins with a valid
246+
/// threshold value, etc.) but does not parse the children of the threshold.
247+
/// Instead it returns a threshold holding the empty type `()`, which is
248+
/// constructed without any allocations, and expects the caller to convert
249+
/// this to the "real" threshold type by calling [`Threshold::translate`].
250+
///
251+
/// (An alternate API which does the conversion inline turned out to be
252+
/// too messy; it needs to take a closure, have multiple generic parameters,
253+
/// and be able to return multiple error types.)
254+
pub fn verify_threshold<
255+
const MAX: usize,
256+
F: FnMut(&Self) -> Result<T, E>,
257+
T,
258+
E: From<ParseThresholdError>,
259+
>(
260+
&self,
261+
mut map_child: F,
262+
) -> Result<Threshold<T, MAX>, E> {
263+
// First, special case "no arguments" so we can index the first argument without panics.
264+
if self.args.is_empty() {
265+
return Err(ParseThresholdError::NoChildren.into());
266+
}
267+
268+
if !self.args[0].args.is_empty() {
269+
return Err(ParseThresholdError::KNotTerminal.into());
270+
}
271+
272+
let k = parse_num(self.args[0].name).map_err(ParseThresholdError::ParseK)? as usize;
273+
Threshold::new(k, vec![(); self.args.len() - 1])
274+
.map_err(ParseThresholdError::Threshold)
275+
.map_err(From::from)
276+
.and_then(|thresh| thresh.translate_by_index(|i| map_child(&self.args[1 + i])))
277+
}
278+
242279
/// Check that a tree has no curly-brace children in it.
243280
pub fn verify_no_curly_braces(&self) -> Result<(), ParseTreeError> {
244281
for tree in self.pre_order_iter() {
@@ -403,34 +440,6 @@ impl<'a> Tree<'a> {
403440
args.reverse();
404441
Ok(Tree { name: &s[..node_name_end], children_pos, parens, args })
405442
}
406-
407-
/// Parses an expression tree as a threshold (a term with at least one child,
408-
/// the first of which is a positive integer k).
409-
///
410-
/// This sanity-checks that the threshold is well-formed (begins with a valid
411-
/// threshold value, etc.) but does not parse the children of the threshold.
412-
/// Instead it returns a threshold holding the empty type `()`, which is
413-
/// constructed without any allocations, and expects the caller to convert
414-
/// this to the "real" threshold type by calling [`Threshold::translate`].
415-
///
416-
/// (An alternate API which does the conversion inline turned out to be
417-
/// too messy; it needs to take a closure, have multiple generic parameters,
418-
/// and be able to return multiple error types.)
419-
pub fn to_null_threshold<const MAX: usize>(
420-
&self,
421-
) -> Result<Threshold<(), MAX>, ParseThresholdError> {
422-
// First, special case "no arguments" so we can index the first argument without panics.
423-
if self.args.is_empty() {
424-
return Err(ParseThresholdError::NoChildren);
425-
}
426-
427-
if !self.args[0].args.is_empty() {
428-
return Err(ParseThresholdError::KNotTerminal);
429-
}
430-
431-
let k = parse_num(self.args[0].name).map_err(ParseThresholdError::ParseK)? as usize;
432-
Threshold::new(k, vec![(); self.args.len() - 1]).map_err(ParseThresholdError::Threshold)
433-
}
434443
}
435444

436445
/// Parse a string as a u32, for timelocks or thresholds

‎src/lib.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -480,6 +480,11 @@ pub enum Error {
480480
Parse(ParseError),
481481
}
482482

483+
#[doc(hidden)] // will be removed when we remove Error
484+
impl From<ParseThresholdError> for Error {
485+
fn from(e: ParseThresholdError) -> Self { Self::ParseThreshold(e) }
486+
}
487+
483488
// https://github.com/sipa/miniscript/pull/5 for discussion on this number
484489
const MAX_RECURSION_DEPTH: u32 = 402;
485490

‎src/miniscript/astelem.rs‎

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -122,27 +122,13 @@ impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Termina
122122
"or_c" => binary(top, "or_c", Terminal::OrC),
123123
"or_i" => binary(top, "or_i", Terminal::OrI),
124124
"thresh" => top
125-
.to_null_threshold()
126-
.map_err(Error::ParseThreshold)?
127-
.translate_by_index(|i| Miniscript::from_tree(&top.args[1 + i]).map(Arc::new))
125+
.verify_threshold(|sub| Miniscript::from_tree(sub).map(Arc::new))
128126
.map(Terminal::Thresh),
129127
"multi" => top
130-
.to_null_threshold()
131-
.map_err(Error::ParseThreshold)?
132-
.translate_by_index(|i| {
133-
top.args[1 + i]
134-
.verify_terminal("public key")
135-
.map_err(Error::Parse)
136-
})
128+
.verify_threshold(|sub| sub.verify_terminal("public_key").map_err(Error::Parse))
137129
.map(Terminal::Multi),
138130
"multi_a" => top
139-
.to_null_threshold()
140-
.map_err(Error::ParseThreshold)?
141-
.translate_by_index(|i| {
142-
top.args[1 + i]
143-
.verify_terminal("public key")
144-
.map_err(Error::Parse)
145-
})
131+
.verify_threshold(|sub| sub.verify_terminal("public_key").map_err(Error::Parse))
146132
.map(Terminal::MultiA),
147133
x => Err(Error::Parse(crate::ParseError::Tree(crate::ParseTreeError::UnknownName {
148134
name: x.to_owned(),

‎src/policy/concrete.rs‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -926,10 +926,8 @@ impl<Pk: FromStrKey> Policy<Pk> {
926926
Ok(Policy::Or(subs))
927927
}
928928
"thresh" => top
929-
.to_null_threshold()
930-
.map_err(Error::ParseThreshold)?
931-
.translate_by_index(|i| Policy::from_tree(&top.args[1 + i]).map(Arc::new))
932-
.map(Policy::Thresh),
929+
.verify_threshold(|sub| Self::from_tree(sub).map(Arc::new))
930+
.map(Self::Thresh),
933931
x => Err(Error::Parse(crate::ParseError::Tree(crate::ParseTreeError::UnknownName {
934932
name: x.to_owned(),
935933
}))),

‎src/policy/semantic.rs‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ impl<Pk: FromStrKey> expression::FromTree for Policy<Pk> {
343343
Ok(Policy::Thresh(Threshold::new(1, subs).map_err(Error::Threshold)?))
344344
}
345345
"thresh" => {
346-
let thresh = top.to_null_threshold().map_err(Error::ParseThreshold)?;
346+
let thresh = top.verify_threshold(|sub| Self::from_tree(sub).map(Arc::new))?;
347347

348348
// thresh(1) and thresh(n) are disallowed in semantic policies
349349
if thresh.is_or() {
@@ -353,9 +353,7 @@ impl<Pk: FromStrKey> expression::FromTree for Policy<Pk> {
353353
return Err(Error::ParseThreshold(crate::ParseThresholdError::IllegalAnd));
354354
}
355355

356-
thresh
357-
.translate_by_index(|i| Policy::from_tree(&top.args[1 + i]).map(Arc::new))
358-
.map(Policy::Thresh)
356+
Ok(Policy::Thresh(thresh))
359357
}
360358
x => Err(Error::Parse(crate::ParseError::Tree(crate::ParseTreeError::UnknownName {
361359
name: x.to_owned(),

‎src/primitives/threshold.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -151,8 +151,8 @@ impl<T, const MAX: usize> Threshold<T, MAX> {
151151
/// Like [`Self::translate_ref`] but passes indices to the closure rather than internal data.
152152
///
153153
/// This is useful in situations where the data to be translated exists outside of the
154-
/// threshold itself, and the threshold data is irrelevant. In particular it is commonly
155-
/// paired with [`crate::expression::Tree::to_null_threshold`].
154+
/// threshold itself, and the threshold data is irrelevant. In particular it is used
155+
/// within the `verify_threshold` method for expression trees.
156156
///
157157
/// If the data to be translated comes from a post-order iterator, you may instead want
158158
/// [`Self::map_from_post_order_iter`].

0 commit comments

Comments
 (0)