Skip to content

Fixing documentation for BIP 340-related functions#307

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
BP-WG:fix/schorr-docs
Nov 2, 2021
Merged

Fixing documentation for BIP 340-related functions#307
apoelstra merged 1 commit intorust-bitcoin:masterfrom
BP-WG:fix/schorr-docs

Conversation

@dr-orlovsky
Copy link
Copy Markdown
Contributor

No description provided.

@apoelstra
Copy link
Copy Markdown
Member

nit: in one place you wrote si instead of is.

needs rebase.

concept ACK

@dr-orlovsky
Copy link
Copy Markdown
Contributor Author

@apoelstra rebased & fixed

///
/// Panics if internal representation of the provided [`SecretKey`] does not hold correct secret
/// key value obtained from Secp256k1 library previously, for instance when the corresponding
/// public key is not even key.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To be clearer: this will panic on out-of-range keys (0 or in excess of the group order).

/// key value obtained from Secp256k1 library previously, for instance when the corresponding
/// public key is not even key.
// TODO: Consider returning `Result`, since even if the secret key is constructed with Secp256k1
// library, the function still will panic on keys producing odd public key values.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't sound right. AFAIK if the resultant public key were to be odd, it will simply be negated.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, it's even simpler than that, the keypair type simply has no restrictions on key parity.

@dr-orlovsky
Copy link
Copy Markdown
Contributor Author

@apoelstra addressed your comments and rebased

Copy link
Copy Markdown
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Nice! Much prefer referring to errors via their type instead of importing variants directly.

ACK 931b560

Copy link
Copy Markdown
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 931b560

@apoelstra apoelstra merged commit 476ced6 into rust-bitcoin:master Nov 2, 2021
chain-forgexcr45 added a commit to chain-forgexcr45/rust-secp256k1 that referenced this pull request Sep 28, 2025
…40-related functions

931b560dc7a339c7544a1eb719b1e4d450456836 Fixing documentation for BIP 340-related functions (Dr Maxim Orlovsky)

Pull request description:

ACKs for top commit:
  thomaseizinger:
    ACK 931b560dc7a339c7544a1eb719b1e4d450456836
  apoelstra:
    ACK 931b560dc7a339c7544a1eb719b1e4d450456836

Tree-SHA512: d78fbda138e636c04c6f356e2a41fcdd0270eea710a9af6c965ff90acfd6740bd650a8909638c558badd52d307cbdcd914fa25846236b064ca8c38faccec8be8
william2332-limf added a commit to william2332-limf/rust-secp256k1 that referenced this pull request Oct 2, 2025
…40-related functions

931b560dc7a339c7544a1eb719b1e4d450456836 Fixing documentation for BIP 340-related functions (Dr Maxim Orlovsky)

Pull request description:

ACKs for top commit:
  thomaseizinger:
    ACK 931b560dc7a339c7544a1eb719b1e4d450456836
  apoelstra:
    ACK 931b560dc7a339c7544a1eb719b1e4d450456836

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

3 participants