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

Add RecoveryId getters #774

Open
tcharding opened this issue Jan 27, 2025 · 7 comments
Open

Add RecoveryId getters #774

tcharding opened this issue Jan 27, 2025 · 7 comments

Comments

@tcharding
Copy link
Member

Add two getters to the RecoveryId type

  • to_u8()
  • to_u32()

Original mentioned here: #765 (comment)

For anyone that wants to know why we add getters to int types and not setters, its because the type of From often cannot be inferred so the explicit getter is more ergonomic.

@apoelstra
Copy link
Member

I don't think we need to_u32. If we have to_u8 then this can be converted to every other integer type (except i8) with .into().

If we currently have to_i32 then we can keep that but we should deprecate it.

@Kixunil
Copy link
Collaborator

Kixunil commented Jan 27, 2025

Also as is still possible but TBF some people like to deny as in clippy to avoid problems.

A constructor for consideration though: from_u8_masked that infallibly converts x & 3.

@apoelstra
Copy link
Member

Oh, I like that idea.

@tcharding
Copy link
Member Author

Should it take a u8 or i32? In rust-bitcoin we use

            let recid = RecoveryId::from_i32(((bytes[0] - 27) & 0x03) as i32)?;

@apoelstra
Copy link
Member

Let's say a u8. It's a little tempting to do a T: Into<u64> or something crazy but there's really no known usecase for this except that expression in rust-bitcoin.

@Kixunil
Copy link
Collaborator

Kixunil commented Jan 27, 2025

That expression is also silly. It takes a byte, transforms it to a different byte then converts it to i32, a bigger and signed type, and then back to what's effectively u2. That useless intermediate conversion should just be removed.

@apoelstra
Copy link
Member

Well, given the existing API it's not too silly :P. But yes, we should fix the API so we can fix it.

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

No branches or pull requests

3 participants