Skip to content
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.

Sign up for GitHub

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

Actually return the error #366

Merged
merged 2 commits into from
Apr 25, 2022
Merged

Actually return the error #366

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb932f6
Actually return the error
tcharding Apr 22, 2022
4d0e8fb
Add unit test SortedMultiVec constructor
tcharding Apr 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion src/descriptor/sortedmulti.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
pub fn new(k: usize, pks: Vec<Pk>) -> Result<Self, Error> {
// A sortedmulti() is only defined for <= 20 keys (it maps to CHECKMULTISIG)
if pks.len() > MAX_PUBKEYS_PER_MULTISIG {
Error::BadDescriptor("Too many public keys".to_string());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

post-merge comment: Does Clippy warn about this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you just need to upgrade to 2018 edition. It's a standard warning now: #[must_use]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Clippy warn about this?

ooo you got me, usually I always mention 'found by clippy' in the commit log but I forgot this time.

I think you just need to upgrade to 2018 edition

The PR is open!

return Err(Error::BadDescriptor("Too many public keys".to_string()));
}

// Check the limits before creating a new SortedMultiVec
Expand Down Expand Up @@ -237,3 +237,34 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> fmt::Display for SortedMultiVec<Pk,
f.write_str(")")
}
}

#[cfg(test)]
mod tests {
use super::*;
use bitcoin::secp256k1::PublicKey;
use miniscript::context::Legacy;

#[test]
fn too_many_pubkeys() {
// Arbitrary pubic key.
let pk = PublicKey::from_str(
"02e6642fd69bd211f93f7f1f36ca51a26a5290eb2dd1b0d8279a87bb0d480c8443",
)
.unwrap();

let over = 1 + MAX_PUBKEYS_PER_MULTISIG;

let mut pks = Vec::new();
for _ in 0..over {
pks.push(pk.clone());
}

let res: Result<SortedMultiVec<PublicKey, Legacy>, Error> = SortedMultiVec::new(0, pks);
let error = res.err().expect("constructor should err");

match error {
Error::BadDescriptor(_) => {} // ok
other => panic!("unexpected error: {:?}", other),
}
}
}