Skip to content

Conversation

@liamaharon
Copy link
Contributor

@liamaharon liamaharon commented Sep 14, 2024

Closes #727

  • Refactors RecoveryId into an enum.
  • Replaces custom type methods from_i32 and to_i32 with TryFrom<i32> and Into<i32> (via From<RecoveryId> for i32) implementations.
  • Removes derive Ord PartialOrd and Hash, they don't appear to be used. I can implement on the enum if we want to keep them.

@apoelstra
Copy link
Member

In f6a76af:

There is an as i32 in your diff that can be converted to a i32::from. (The old code is at fault, but you're changing this line anyway so you should fix it.)

@liamaharon
Copy link
Contributor Author

Good catch @apoelstra, fixed it

@liamaharon
Copy link
Contributor Author

Force pushed a lint fix

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 fa0c086 successfully ran local tests

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK fa0c086

@apoelstra apoelstra merged commit 2a80731 into rust-bitcoin:master Sep 16, 2024
tcharding added a commit to tcharding/rust-secp256k1 that referenced this pull request Jun 12, 2025
The `RecoveryId` is an enum but is typically serialized as an integer.
We used to provide a `to_i32` function before rust-bitcoin#743.

Add a `to_u8` getter to the `RecoveryId`
tcharding added a commit to tcharding/rust-secp256k1 that referenced this pull request Jun 12, 2025
The `i32` is a hangover from before rust-bitcoin#743, now we have a nice enum and a
`from_u8_masked` constructor (along with a `to_u8` getter) lets make the
`From<RecoveryId>` impl be on `u8` instead of on `i32`.
tcharding added a commit to tcharding/rust-secp256k1 that referenced this pull request Jun 12, 2025
The `i32` is a hangover from before rust-bitcoin#743, now we have a nice enum and a
`from_u8_masked` constructor (along with a `to_u8` getter) lets make the
`From<RecoveryId>` impl be on `u8` instead of on `i32`.
tcharding added a commit to tcharding/rust-secp256k1 that referenced this pull request Jun 12, 2025
The `i32` is a hangover from before rust-bitcoin#743, now we have a nice enum and a
`from_u8_masked` constructor (along with a `to_u8` getter) lets make the
`From<RecoveryId>` impl be on `u8` instead of on `i32`.
chain-forgexcr45 added a commit to chain-forgexcr45/rust-secp256k1 that referenced this pull request Sep 28, 2025
fa0c086431c9fc6ee872d6a14feccee6eb73dfb5 refactor: recoveryid into enum (Liam Aharon)

Pull request description:

  Closes #727

  - Refactors `RecoveryId` into an enum.
  - Replaces custom type methods `from_i32` and `to_i32` with `TryFrom<i32>` and `Into<i32>` (via `From<RecoveryId> for i32`) implementations.
  - Removes derive `Ord` `PartialOrd` and `Hash`, they don't appear to be used. I can implement on the enum if we want to keep them.

ACKs for top commit:
  apoelstra:
    ACK fa0c086431c9fc6ee872d6a14feccee6eb73dfb5 successfully ran local tests
  tcharding:
    ACK fa0c086431c9fc6ee872d6a14feccee6eb73dfb5

Tree-SHA512: 2b4f448c69d51ca8bf66110a46aa3a846cc47dc137b67f04643ae01a181f7208508c6af27429e26b3ee5d625c37923adc7fd20ccca701b5f5433b5a62d41a802
william2332-limf added a commit to william2332-limf/rust-secp256k1 that referenced this pull request Oct 2, 2025
fa0c086431c9fc6ee872d6a14feccee6eb73dfb5 refactor: recoveryid into enum (Liam Aharon)

Pull request description:

  Closes #727

  - Refactors `RecoveryId` into an enum.
  - Replaces custom type methods `from_i32` and `to_i32` with `TryFrom<i32>` and `Into<i32>` (via `From<RecoveryId> for i32`) implementations.
  - Removes derive `Ord` `PartialOrd` and `Hash`, they don't appear to be used. I can implement on the enum if we want to keep them.

ACKs for top commit:
  apoelstra:
    ACK fa0c086431c9fc6ee872d6a14feccee6eb73dfb5 successfully ran local tests
  tcharding:
    ACK fa0c086431c9fc6ee872d6a14feccee6eb73dfb5

Tree-SHA512: 2b4f448c69d51ca8bf66110a46aa3a846cc47dc137b67f04643ae01a181f7208508c6af27429e26b3ee5d625c37923adc7fd20ccca701b5f5433b5a62d41a802
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.

RecoveryId should be an enum

3 participants