Add examples to `key` module #376

tcharding · 2022-01-13T08:51:11Z

This PR is an initial attempt to more thoroughly test our public API.

Add examples to various types/methods/functions in the key module.

I'm not entirely sure when is enough, do we want an example on every single public method, function, and type or is this overkill. In this PR I tried to find a balance by doing ever method/function that took an argument that is a custom type from this lib. I think this should be extended to include return values too though ...

Thanks to @thomaseizinger for the idea!

First 2 patches are docs improvements to lib.rs.

src/key.rs

elichai · 2022-01-13T10:05:56Z

src/key.rs

+    /// # #[cfg(feature="rand")] {
+    /// use secp256k1::{rand, SecretKey};


rand isn't really used here

Oh cheers, I've not worked out how to lint the examples yet.

I haven't tried it but I'd expect --all-targets on cargo clippy to include rustdoc.

I only learned during this PR that -all-targets does not include docs (I have my editor configured to run clippy with --all-targets, thanks Lucas :)

src/key.rs

elichai · 2022-01-13T10:14:14Z

src/key.rs

+    /// let mut public_key = key_pair.public_key();
+    /// let original = public_key;
+    /// let parity = public_key.tweak_add_assign(&secp, &tweak).expect("Tweak error");
+    /// assert!(original.tweak_add_check(&secp, &public_key, parity, tweak));


Too bad we don't have access to a hash function to showcase this properly :(

What did you have in mind? We can access bitcoin_hashes here.

elichai · 2022-01-13T10:14:51Z

Sorry for the long review, it's easy to bikeshed over documentation

tcharding · 2022-01-13T21:39:31Z

Sorry for the long review, it's easy to bikeshed over documentation

No apology needed, I appreciate the comments. Documentation is definitely a funny thing, its easy to agree we need it not so easy to agree how it should be done. That's why I tried to stick meticulously to Rust conventions. (I've not written that much thorough Rust documentation in the past though so I'm still learning.)

tcharding · 2022-01-13T21:48:15Z

@elichai I fixed all the 'should have been found by the linter' issues and squashed them into the original commit (4fc759a Add examples to types and methods in key module). The OsRng suggestion is done as a separate patch along with changing how rand::thread_rng is called, please say if you disagree with that patch. Thanks.

thomaseizinger

Nice work.

thomaseizinger · 2022-01-13T23:22:00Z

src/key.rs

+/// # #[cfg(feature="rand")] {
+/// use secp256k1::{rand, Secp256k1, SecretKey};
+///
+/// let secp = Secp256k1::new();
+/// let secret_key = SecretKey::new(&mut rand::thread_rng());
+/// # }


Nice idea with the scope. Untested idea that might also work (note the #!):

Suggested change

/// # #[cfg(feature="rand")] {

/// use secp256k1::{rand, Secp256k1, SecretKey};

///

/// let secp = Secp256k1::new();

/// let secret_key = SecretKey::new(&mut rand::thread_rng());

/// # }

/// # #![cfg(feature="rand")]

/// use secp256k1::{rand, Secp256k1, SecretKey};

///

/// let secp = Secp256k1::new();

/// let secret_key = SecretKey::new(&mut rand::thread_rng());

This doesn't work with Rust 1.29 unfortunately. I think, IIRC, it starts working at 1.34. I'll add a TODO to remove them after bump of MSRV.

Fail! I did not 'note the !#' even when you told me too. It is hard to get good help :)
Updating this PR to include this suggestion.

Bother, and I thought the saying was 'if it builds ship it' ... turns out the !# suggestion builds just fine but doesn't run.

Oh that is weird. Should go back to testing my suggestions before making them!

Not at all, open suggestions lead to leaning stuff. And since they are cheaper to make than tested suggestions it leads to more information being shared, IMO.

src/key.rs

tcharding · 2022-01-17T21:53:32Z

After re-visiting this I've squashed all the suggested changes into the one commit 7e3af61 Add examples to types and methods in key module.

Improve method docs by doing: - Remove 'batch' comment - Remove mention of required features, docs already show this - Use links to types as well as ticks

Improve the main docs by doing: - Remove unneeded `self` from use statement - Add code ticks to `bitcoin_hashes`

Use 'standard' stlye, standard is defined as - No markdown heading - Full sentence (capital first letter and full stop) - Trailing empty comment line

Done in an effort to better test our public API. Add tests in the `Examples` section as is idiomatic in the Rust ecosystem. Make other minor improvements to any rusdocs we touch: - Use full stops - Use 100 character column width - Use plural third person tense - Use plural for section headings

We recently patched much of the docs in the `key` module, lets attempt to attain perfection. Improve docs by doing: - Use full stops - Use 100 character column width - Use plural third person tense - Use plural for section headings - Fix any grammar mistakes - Use code ticks and links as appropriate

apoelstra

ACK aa828f0

This is amazing, thanks!

tcharding · 2022-01-24T20:48:04Z

Cheers man!

aa51638 update changelog for 0.22.0 (Andrew Poelstra) d06dd20 update fuzzdummy API to match normal API (Andrew Poelstra) f3d48a2 update "should terminate abnormally" test to trigger a different ARG_CHECK (Andrew Poelstra) 8294ea3 secp256k1-sys: update upstream library (Andrew Poelstra) 2932179 secp256k1-sys: update secp256k1.h.patch (Andrew Poelstra) Pull request description: Should wait on merging until we get a minor release out with #382 and #376. May also want to bundle #380 with this? ACKs for top commit: real-or-random: ACK aa51638 I can't judge if the feature set is meaningful but this release PR is fine Tree-SHA512: e7f48b402378e280a034127f2de58d3127e04303a114f07f294fa3d00c0a083ae0d43375a8a74d226b13ea45fb3fde07d8450790e602bbf9581adc5fd8bc7d29

tcharding marked this pull request as draft January 13, 2022 08:56

tcharding force-pushed the add-examples-key branch from 7c6de93 to f003b08 Compare January 13, 2022 09:08

tcharding marked this pull request as ready for review January 13, 2022 09:08

elichai reviewed Jan 13, 2022

View reviewed changes

tcharding force-pushed the add-examples-key branch from f003b08 to 6d3919a Compare January 13, 2022 21:46

tcharding force-pushed the add-examples-key branch from 6d3919a to 5e660fe Compare January 13, 2022 21:53

thomaseizinger reviewed Jan 13, 2022

View reviewed changes

tcharding mentioned this pull request Jan 14, 2022

Add integration tests #371

Closed

tcharding force-pushed the add-examples-key branch from e81c6ef to 4a40a6a Compare January 17, 2022 21:52

tcharding force-pushed the add-examples-key branch from 4a40a6a to 694e46f Compare January 17, 2022 22:01

tcharding added 5 commits January 24, 2022 14:43

Improve docs on method generate_keypair

4c4268f

Improve method docs by doing: - Remove 'batch' comment - Remove mention of required features, docs already show this - Use links to types as well as ticks

Improve lib.rs rustdocs

6d23614

Improve the main docs by doing: - Remove unneeded `self` from use statement - Add code ticks to `bitcoin_hashes`

Improve key module docs

a7f3d9b

Use 'standard' stlye, standard is defined as - No markdown heading - Full sentence (capital first letter and full stop) - Trailing empty comment line

tcharding force-pushed the add-examples-key branch from 694e46f to aa828f0 Compare January 24, 2022 03:48

apoelstra approved these changes Jan 24, 2022

View reviewed changes

apoelstra mentioned this pull request Jan 24, 2022

update libsecp to latest ersion; bump major version number #384

Merged

apoelstra merged commit f7d637e into rust-bitcoin:master Jan 25, 2022

tcharding deleted the add-examples-key branch January 26, 2022 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add examples to `key` module #376

Add examples to `key` module #376

tcharding commented Jan 13, 2022

elichai Jan 13, 2022

tcharding Jan 13, 2022

thomaseizinger Jan 13, 2022

tcharding Jan 17, 2022

elichai Jan 13, 2022

tcharding Jan 13, 2022

elichai commented Jan 13, 2022

tcharding commented Jan 13, 2022

tcharding commented Jan 13, 2022

thomaseizinger left a comment

thomaseizinger Jan 13, 2022

tcharding Jan 14, 2022

tcharding Jan 17, 2022

tcharding Jan 17, 2022

thomaseizinger Jan 17, 2022

tcharding Jan 17, 2022

tcharding commented Jan 17, 2022 •

edited

Loading

apoelstra left a comment

tcharding commented Jan 24, 2022

		/// # #[cfg(feature="rand")] {
		/// use secp256k1::{rand, SecretKey};

Add examples to key module #376

Add examples to key module #376

Conversation

tcharding commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai commented Jan 13, 2022

tcharding commented Jan 13, 2022

tcharding commented Jan 13, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Jan 17, 2022 • edited Loading

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Jan 24, 2022

Add examples to `key` module #376

Add examples to `key` module #376

tcharding commented Jan 17, 2022 •

edited

Loading