Skip to content

various improvements/refactors of the decoder and its errors #845

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

apoelstra
Copy link
Member

The last commit, which renames parse_with_ext to decode_with_ext, I think we should backport with deprecation to 12.x. This is a pretty uncommonly used function and is unlikely to bother anybody, but the current name is inconsistent with our general use of "parse" for strings and "decode" for scripts.

This is a little less space efficient but it gets rid of a lifetime
parameter on the lexer token, which is annoying because it prevents us
from putting it into error types. (Currently we convert it to a string
before putting it into an error.)

It also eliminates a whole bunch of panics where we convert slices to
hashes and .expect() on the length being right, when we know it's right.
Also clean up the names and format messages for them.
…onstructors

We have infallible constructors for all the terminals except
multi/multia. We should use them and eliminate a bunch of error paths.
The word "parse" is redundant at best, and wrong at worst (we use
"decode" when converting a script and "parse" when converting a string).
The variants were also overly verbose and the formatting text lost
information. Clean all this up.
We should backport this one to 12.x (with a deprecation). Would also be
open to doing a deprecation here in master. But this has been bugging me
for years, and also it's much less common to decode miniscript from
script than we imagined back in 2018.

Provides symmetry with encode() and consistency with the docs (though
this PR does not fix up the docs).
Copy link
Member Author

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

On cdb36c1 successfully ran local tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant