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.

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

QoL #85

Merged
merged 9 commits into from
Sep 11, 2024
Merged

QoL #85

merged 9 commits into from
Sep 11, 2024

Conversation

uncomputable
Copy link
Collaborator

Some quality-of-life improvements. In particular, hex strings for arbitrary byte arrays, more constructors, and builtin aliases in witness json files.

@apoelstra
Copy link
Contributor

dc60cee fails to build

error[E0599]: no variant or associated item named `ExpressionUnexpectedType` >
   --> src/value.rs:719:48
    |
719 |                         _ => return Err(Error::ExpressionUnexpectedType>
    |                                                ^^^^^^^^^^^^^^^^^^^^^^^^>
    |
   ::: src/error.rs:259:1
    |
259 | pub enum Error {
    | -------------- variant or associated item `ExpressionUnexpectedType` no>

error: aborting due to 1 previous error

but code-review-ack 8d93f95.

This enables hex literals for all [u8; N] Simfony types.

Adapt the examples to write digital signatures in this way, which is
more comfortable than writing Rust-type arrays of hex bytes.
This error is like ExpressionTypeMismatch, but it doesn't know the
type of the observed expression. We use ExpressionTypeMismatch as
often as we can, and we use ExpressionUnexpectedType in cases where
it is hard to determine the observed type.
We no longer need it. An empty JSON file is kind of useless.
It turns out that the parser for ResolvedType that I wrote accepts
builtin aliases. This makes witness json files more readable.
@uncomputable
Copy link
Collaborator Author

I messed up during rebasing when I changed the order of commits. I wonder if we can include this check in CI...

@apoelstra
Copy link
Contributor

We could try this, but it's a bit of a PITA to get the list of commits in CI, and it'd take a long time to test every single commit (especially on 9-commit PRs like this).

@apoelstra
Copy link
Contributor

Alternately you could always run git rebase -x 'cargo test' master before pushing.

@uncomputable
Copy link
Collaborator Author

@apoelstra Is the rest of the PR ok?

@apoelstra
Copy link
Contributor

@uncomputable yes

@uncomputable uncomputable merged commit 8eec967 into BlockstreamResearch:master Sep 11, 2024
5 checks passed
@uncomputable
Copy link
Collaborator Author

I merged the PR without ACK 😛

@uncomputable uncomputable deleted the 2024-09-qol branch September 11, 2024 09:14
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.

2 participants