Improve docs in taproot module #912

tcharding · 2022-03-28T03:49:13Z

I should have done this PR a month ago, my bad. This one is kind of important IMO because we are going to have so many people looking at this part of the code soon as we release.

As has been done in other places in the codebase; improve the docs in the taproot module by doing:

Use full sentences (capital letters + full stops)
Use back ticks and links for types where appropriate
Fix grammar
Fix stale docs
Use third person for describing functions
Use 100 character line width
Use markdown sections (# Examples, # Returns) where appropriate
Separate brief heading from extended description when appropriate
Use /// for all functions/types (both private and public)

I also did:

Build the docs and check all the links
Read all the built docs, check for sanity and pretty-ness

Its all in one patch, I couldn't really tease it apart. I can try a bit harder if it proves too annoying to review.

tcharding · 2022-03-28T03:50:34Z

CC @sanket1729 since this is taproot stuff.

sanket1729

Thank you so much for doing this.
Left one comment that needs to be changed. I need to set my editor to make use of 100 characters whenever available while commenting. I promise I will be more conscious while writing docs for my subsequent PRs.

src/util/taproot.rs

tcharding · 2022-03-28T20:45:36Z

I promise I will be more conscious while writing docs for my subsequent PRs.

I'm a firm believer that folk should work on what stimulates them, if writing clean docs is not your thing then just stick to doing you thing well :) I do these docs fixes at the end of the days or when I'm brain fried, no stress.

As has been done in other places in the codebase; improve the docs in the `taproot` module by doing: - Use full sentences (capital letters + full stops) - Use back ticks and links for types where appropriate - Fix grammar - Fix stale docs - Use third person for describing functions - Use 100 character line width - Use markdown sections (`# Examples`, `# Returns`) where appropriate - Separate brief heading from extended description when appropriate - Use `///` for all functions/types (both private and public) I also did: - Build the docs and check all the links - Read all the built docs, check for sanity and pretty-ness

We have some text quoted directly from BIP341, this text is on the net if folk wish to read it, we don't need it in the source code.

tcharding · 2022-03-28T23:32:50Z

Force push includes:

the minor fixes suggested in review as part of patch 1
the removal of bip341 docs as a separate patch
I've rudely kept in my 'improved' version of the docs on with_huffman_tree even though @dr-orlovsky already fixed the stale types thing (there has to be some perks to being the monkey that does all these docs fixes :)

tcharding · 2022-03-29T00:59:46Z

This PR is a candidate for either the 'RC-fix' label or the 0.28 milestone, leaving for another maintainer to add so as not to be too pushy with my own changes.

sanket1729

ACK c25eddd

sanket1729 · 2022-03-29T01:25:40Z

If we get two reviews in time, sure. I don't mind merging any changes to documentation before release.

dr-orlovsky

ACK c25eddd

Will wait for @apoelstra review before merge, since he had done previous review.

All my suggestions are nits, which may be addressed in the follow up PR. Not sure about the articles, but I assume that all function arg references should be prefixed with the.

dr-orlovsky · 2022-03-30T08:55:24Z

src/util/taproot.rs

+    /// - `TaprootError::InvalidControlBlockSize` if `sl` is not of size 1 + 32 + 32N for any N >= 0.
+    /// - `TaprootError::InvalidParity` if first byte of `sl` is not a valid output key parity.
+    /// - `TaprootError::InvalidTaprootLeafVersion` if first byte of `sl` is not a valid leaf version.
+    /// - `TaprootError::InvalidInternalKey` if internal key is invalid (first 32 bytes after the parity byte).
+    /// - `TaprootError::InvalidMerkleTreeDepth` if merkle tree is too deep (more than 128 levels).


nit: Why not [`..`] for error types? This will allow users to jump to the description of specific error

dr-orlovsky · 2022-03-30T08:57:43Z

src/util/taproot.rs

@@ -513,15 +519,16 @@ impl TaprootBuilder {
    }
 }

-/// Data structure used to represent node information in taproot tree.
+/// Represents the node information in taproot tree.
+///


nit:

Suggested change

///

///

/// The data type is a helper type used in merkle tree construction allowing to build sparse merkle trees.

/// The node represents part of the tree that has information about all of its descendants. See how

/// [`TaprootBuilder`] works for more details.

Used with some minor tweaks.

dr-orlovsky · 2022-03-30T08:57:59Z

src/util/taproot.rs

@@ -466,7 +472,7 @@ impl TaprootBuilder {
        &self.branch
    }

-    // Helper function to insert a leaf at a depth
+    /// Inserts a leaf at `depth`.


nit:

Suggested change

/// Inserts a leaf at `depth`.

/// Inserts a leaf at the `depth`.

I think the existing grammar is correct.

dr-orlovsky · 2022-03-30T08:58:25Z

src/util/taproot.rs

    pub fn add_hidden(self, depth: usize, hash: sha256::Hash) -> Result<Self, TaprootBuilderError> {
        let node = NodeInfo::new_hidden(hash);
        self.insert(node, depth)
    }

-    /// Check if the builder is a complete tree
+    /// Checks if the builder is a complete tree.


nit

Suggested change

/// Checks if the builder is a complete tree.

/// Checks if the builder has a complete tree.

I could go either way on this one.

dr-orlovsky · 2022-03-30T08:58:52Z

src/util/taproot.rs

-    /// root node is 0 and it's immediate child would be at depth 1.
-    /// See [`TaprootBuilder::add_leaf_with_ver`] for adding a leaf with specific version
-    /// See [Wikipedia](https://en.wikipedia.org/wiki/Depth-first_search) for more details
+    /// Adds a leaf script at `depth` to the builder with default script version. Errors if the


nit

Suggested change

/// Adds a leaf script at `depth` to the builder with default script version. Errors if the

/// Adds a leaf script at the `depth` to the builder with default script version. Errors if the

I'll try to explain why the original is, IMO, correct. 'depth' is not a noun in this sentence, conceptually its a location in the tree, in other words 'depth' could be replaced by 'the layer of the tree represented by usize' - hence we use 'depth' without using 'the'. Does that make sense?

dr-orlovsky · 2022-03-30T08:59:05Z

src/util/taproot.rs

@@ -412,7 +419,8 @@ impl TaprootBuilder {
        Ok(TaprootBuilder{branch: vec![Some(node)]})
    }

-    /// Just like [`TaprootBuilder::add_leaf`] but allows to specify script version
+    /// Adds a leaf script at `depth` to the builder with script version `ver`. Errors if the leaves


nit

Suggested change

/// Adds a leaf script at `depth` to the builder with script version `ver`. Errors if the leaves

/// Adds a leaf script at the `depth` to the builder with script version `ver`. Errors if the leaves

dr-orlovsky · 2022-03-30T08:59:55Z

src/util/taproot.rs

-    /// Compute [`TaprootSpendInfo`] from [`NodeInfo`], and internal key.
-    /// This is useful when you want to manually build a taproot tree wihtout
-    /// using [`TaprootBuilder`].
+    /// Computes the [`TaprootSpendInfo`] from `internal_key` and `node`.


nit

Suggested change

/// Computes the [`TaprootSpendInfo`] from `internal_key` and `node`.

/// Computes the [`TaprootSpendInfo`] from the `internal_key` and the `node`.

I believe, in technical writing, both of these are correct. I tend to favour the more terse version. Sometimes I think we do drop too many instances of 'the' though :)

dr-orlovsky · 2022-03-30T09:00:20Z

src/util/taproot.rs

-    /// Create a new key spend with internal key and proided merkle root.
-    /// Provide [`None`] for merkle_root if there is no script path.
+    /// Creates a new key spend with `internal_key` and `merkle_root`. Provide [`None`] for
+    /// `merkle_root` if there is no script path.


nit:

Suggested change

/// `merkle_root` if there is no script path.

/// the `merkle_root` if there is no script path.

For this one however, I like your suggestion :)

dr-orlovsky · 2022-03-30T09:00:58Z

src/util/taproot.rs

@@ -131,70 +131,73 @@ impl TapLeafHash {
    }
 }

-/// Maximum depth of a Taproot Tree Script spend path
+/// Maximum depth of a taproot tree script spend path.


nit, but not sure:

Suggested change

/// Maximum depth of a taproot tree script spend path.

/// Maximum depth of a taproot tree script spending path.

I think the existing phrasing is clearer.

I believe 'key spend' and 'script spend' are the terms being used across the eccosystem, I could be wrong.

apoelstra

ACK c25eddd

tcharding · 2022-03-30T20:52:18Z

Thanks for the review @dr-orlovsky, I'll see to your nits.

dr-orlovsky · 2022-03-31T07:48:29Z

@apoelstra @tcharding sorry for the mess with the articles in my suggestions, will not do that in a (the?) future. As a non-native speaker the whole concept of articles for me (with native Slavic language background) is ungraspable. I thought I did understood the logic of using them, but in fact not :)

tcharding · 2022-03-31T20:55:00Z

the articles in my suggestions, will not do that in a (the?) future

LMAO at the '(the?)' :)

English ... what can I say, its not that easy to write well, I hope to achieve it before I die.

@dr-orlovsky

da731c4 Add further description to the NodeInfo struct (Tobin Harding) 492cceb Use links for error types (Tobin Harding) 3e05887 Use 'the' to improve sentence (Tobin Harding) Pull request description: See to nits from review of #912 Three minor patches to the `taproot` module docs. CC @dr-orlovsky ACKs for top commit: dr-orlovsky: ACK da731c4 sanket1729: ACK da731c4 Tree-SHA512: 17a27a19c88f9baa8127023b2ee30fc2259cb0058a92dc9d8ae595e9e02ccb047fefcba7548ff7900fffa7bc6853447183e80660b8756d90d055ab8aa96ae938

apoelstra · 2022-04-01T17:01:16Z

No worries @dr-orlovsky. I am a native English speaker and I do not know any real rules for how articles are used. (If it helps I have the same difficulty in French, e.g. I was surprised that this book title uses both les and des as articles in places where the English title has no articles at all.)

sanket1729 reviewed Mar 28, 2022

View reviewed changes

src/util/taproot.rs Outdated Show resolved Hide resolved

apoelstra reviewed Mar 28, 2022

View reviewed changes

src/util/taproot.rs Outdated Show resolved Hide resolved

apoelstra reviewed Mar 28, 2022

View reviewed changes

src/util/taproot.rs Outdated Show resolved Hide resolved

apoelstra reviewed Mar 28, 2022

View reviewed changes

src/util/taproot.rs Outdated Show resolved Hide resolved

tcharding added 2 commits March 29, 2022 10:27

Remove unnecessary documentation

c25eddd

We have some text quoted directly from BIP341, this text is on the net if folk wish to read it, we don't need it in the source code.

tcharding force-pushed the taproot-docs branch from f488093 to c25eddd Compare March 28, 2022 23:29

sanket1729 approved these changes Mar 29, 2022

View reviewed changes

sanket1729 added the one ack PRs that have one ACK, so one more can progress them label Mar 29, 2022

dr-orlovsky approved these changes Mar 30, 2022

View reviewed changes

apoelstra approved these changes Mar 30, 2022

View reviewed changes

dr-orlovsky merged commit 58a958e into rust-bitcoin:master Mar 30, 2022

tcharding deleted the taproot-docs branch March 30, 2022 23:41

tcharding mentioned this pull request Mar 31, 2022

Taproot docs nits #923

Merged

-///
+///
+/// The data type is a helper type used in merkle tree construction allowing to build sparse merkle trees.
+/// The node represents part of the tree that has information about all of its descendants. See how
+/// [`TaprootBuilder`] works for more details.

	/// Inserts a leaf at `depth`.
	/// Inserts a leaf at the `depth`.

	/// Checks if the builder is a complete tree.
	/// Checks if the builder has a complete tree.

	/// Adds a leaf script at `depth` to the builder with default script version. Errors if the
	/// Adds a leaf script at the `depth` to the builder with default script version. Errors if the

	/// Computes the [`TaprootSpendInfo`] from `internal_key` and `node`.
	/// Computes the [`TaprootSpendInfo`] from the `internal_key` and the `node`.

	/// `merkle_root` if there is no script path.
	/// the `merkle_root` if there is no script path.

	/// Maximum depth of a taproot tree script spend path.
	/// Maximum depth of a taproot tree script spending path.

Improve docs in taproot module #912

Improve docs in taproot module #912

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!