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

fix: zero sequence check for from_height #748

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

Conversation

ChrisCho-H
Copy link
Contributor

This is following #740.
I missed from_height also needs to check if zero.
I'm afraid as it's API breaking change.

@apoelstra
Copy link
Member

CI failure is unrelated.

Concept ACK. I'm tempted to also suggest we provide infallible conversions to/from NonZeroU16, but the API for that type is super annoying, and there's also no equivalent type that can express the allowable range of AbsLockTimes so it'd make our API asymmetric.

@ChrisCho-H can you add a from_height_unchecked method which has the original behavior? You don't need to use it anywhere (it's longer to type than from_height(x).unwrap() after all) but I think some users want to grep for unwrap and have the output be clean.

@ChrisCho-H
Copy link
Contributor Author

ChrisCho-H commented Sep 15, 2024

I think NonZeroU16 could be better option for the explicit and infallible usage, but as you mention it would be very inconvenient to use. The only failing case is when zero is given(which is unlikely to happen for the most cases), so we might better not to tighten interface that much.

Copy link
Member

@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.

ACK d030b8f 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.

2 participants