From b38b9d5d6271d24c30e78a44967dc635990c05b5 Mon Sep 17 00:00:00 2001
From: Kris Nuttycombe <kris@nutty.land>
Date: Tue, 25 Jun 2024 15:04:12 -0600
Subject: [PATCH] shardtree: Minor cleanups & documentation improvements.

---
 incrementalmerkletree/CHANGELOG.md |  1 +
 incrementalmerkletree/src/lib.rs   |  3 +++
 shardtree/src/lib.rs               | 21 ++++++++-------------
 shardtree/src/prunable.rs          | 12 +-----------
 shardtree/src/tree.rs              |  9 +++++++--
 5 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/incrementalmerkletree/CHANGELOG.md b/incrementalmerkletree/CHANGELOG.md
index efa8906..633e1ac 100644
--- a/incrementalmerkletree/CHANGELOG.md
+++ b/incrementalmerkletree/CHANGELOG.md
@@ -9,6 +9,7 @@ and this project adheres to Rust's notion of
 
 ### Added
 - `incrementalmerkletree::Marking`
+- `incrementalmerkletree::Level::ZERO`
 
 ### Changed
 - `incrementalmerkletree::Retention`
diff --git a/incrementalmerkletree/src/lib.rs b/incrementalmerkletree/src/lib.rs
index ea4d009..6c74a7c 100644
--- a/incrementalmerkletree/src/lib.rs
+++ b/incrementalmerkletree/src/lib.rs
@@ -280,6 +280,9 @@ impl TryFrom<Position> for usize {
 pub struct Level(u8);
 
 impl Level {
+    /// Level 0 corresponds to a leaf of the tree.
+    pub const ZERO: Self = Level(0);
+
     pub const fn new(value: u8) -> Self {
         Self(value)
     }
diff --git a/shardtree/src/lib.rs b/shardtree/src/lib.rs
index 398b24b..011f37b 100644
--- a/shardtree/src/lib.rs
+++ b/shardtree/src/lib.rs
@@ -210,7 +210,8 @@ impl<
         Ok(())
     }
 
-    /// Append a single value at the first available position in the tree.
+    /// Append a single value at the first unfilled position greater than the maximum position of
+    /// any previously inserted leaf.
     ///
     /// Prefer to use [`Self::batch_insert`] when appending multiple values, as these operations
     /// require fewer traversals of the tree than are necessary when performing multiple sequential
@@ -320,17 +321,8 @@ impl<
             .map_err(ShardTreeError::Storage)?
             .unwrap_or_else(|| LocatedTree::empty(subtree_root_addr));
 
-        trace!(
-            max_position = ?current_shard.max_position(),
-            subtree = ?current_shard,
-            "Current shard");
         let (updated_subtree, supertree) =
             current_shard.insert_frontier_nodes(frontier, &leaf_retention)?;
-        trace!(
-            max_position = ?updated_subtree.max_position(),
-            subtree = ?updated_subtree,
-            "Replacement shard"
-        );
         self.store
             .put_shard(updated_subtree)
             .map_err(ShardTreeError::Storage)?;
@@ -1464,7 +1456,8 @@ mod tests {
                     id: 1,
                     marking: frontier_marking,
                 },
-            )?;
+            )
+            .unwrap();
 
             // Insert a few leaves beginning at the subsequent position, so as to cross the shard
             // boundary.
@@ -1473,7 +1466,8 @@ mod tests {
                 ('g'..='j')
                     .into_iter()
                     .map(|c| (c.to_string(), Retention::Ephemeral)),
-            )?;
+            )
+            .unwrap();
 
             // Trigger pruning by adding 5 more checkpoints
             for i in 2..7 {
@@ -1486,7 +1480,8 @@ mod tests {
                 ('e'..='f')
                     .into_iter()
                     .map(|c| (c.to_string(), Retention::Marked)),
-            )?;
+            )
+            .unwrap();
 
             // Compute the witness
             tree.witness_at_checkpoint_id(frontier_end, &6)
diff --git a/shardtree/src/prunable.rs b/shardtree/src/prunable.rs
index 1ec3052..1ce9fee 100644
--- a/shardtree/src/prunable.rs
+++ b/shardtree/src/prunable.rs
@@ -322,11 +322,7 @@ impl<H: Hashable + Clone + PartialEq> PrunableTree<H> {
             {
                 Tree::leaf((H::combine(level, &lv.0, &rv.0), rv.1))
             }
-            (left, right) => Tree::parent(
-                    ann,
-                    left,
-                    right,
-            ),
+            (left, right) => Tree::parent(ann, left, right),
         }
     }
 }
@@ -563,12 +559,6 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
             subtree: LocatedPrunableTree<H>,
             contains_marked: bool,
         ) -> Result<(PrunableTree<H>, Vec<IncompleteAt>), InsertionError> {
-            trace!(
-                root_addr = ?root_addr,
-                max_position = ?LocatedTree::max_position_internal(root_addr, into),
-                to_insert = ?subtree.root_addr(),
-                "Subtree insert"
-            );
             // An empty tree cannot replace any other type of tree.
             if subtree.root().is_nil() {
                 Ok((into.clone(), vec![]))
diff --git a/shardtree/src/tree.rs b/shardtree/src/tree.rs
index 6682c7b..7ce5c78 100644
--- a/shardtree/src/tree.rs
+++ b/shardtree/src/tree.rs
@@ -94,7 +94,9 @@ impl<A, V> Deref for Tree<A, V> {
 
 impl<A, V> Tree<A, V> {
     /// Constructs the empty tree.
-    pub fn empty() -> Self {
+    ///
+    /// This represents a tree for which we have no information.
+    pub const fn empty() -> Self {
         Tree(Node::Nil)
     }
 
@@ -104,6 +106,9 @@ impl<A, V> Tree<A, V> {
     }
 
     /// Constructs a tree containing a single leaf.
+    ///
+    /// This represents either leaf of the tree, or an internal parent node of the
+    /// tree having all [`Node::Pruned`] children.
     pub fn leaf(value: V) -> Self {
         Tree(Node::Leaf { value })
     }
@@ -117,7 +122,7 @@ impl<A, V> Tree<A, V> {
         })
     }
 
-    /// Returns `true` if the tree has no leaves.
+    /// Returns `true` if the tree is the [`Node::Nil`] node.
     pub fn is_empty(&self) -> bool {
         self.0.is_nil()
     }