[Sumtree]: Initialization and Insertion for Sumtree #56

crnbarr93 · 2024-03-08T17:05:22Z

Closes: #54 #55

What is the purpose of the change

These changes introduce the concept of a sumtree in to our orderbook implementation. The implementation is separate from the standard linear orderbook as a completely new contract. The implementation of the sumtree revolves around the state mentioned in #54, the only difference being the naming of the Node struct as TreeNode.

Several utility methods were added to the TreeNode struct to ease readability of insertion logic. Primarily getters and setters for all values.

The min and max range defined in these methods can be defined as follows:

For an Internal node this is simply the range values.
For a Leaf node the min is the Effective Total Amount Sold (ETAS) of the node and the max is the value of the node added to the ETAS

The get_value method returns the following:

For an Internal node it returns the accumulator (sum of all value in ancestral leafs)
For a Leaf node it returns the value of the leaf (how much was canceled with the order)

Insertion logic is handled using insert which is a recursive function that traverses the tree and inserts when specified by the conditions outlined in #55 . Each condition is handled on a case by case basis, invoking priority based on code execution order.

Internal node values are updated as the tree is traversed by the insertion logic, with the assumption that if a node is visited then the new node will be an ancestor of that node. Each individual case is labeled via comments and handled in the test_node_insert_valid test in test_node.rs.

There is a potential upgrade in insertion logic (currently insertion leans left) by judging traversal based on closest range. There is also an issue where the range of each initial leaf is judged by the first two nodes added on each side.

Testing and Verifying

Tests can be run using:

cargo unit-test test_node

Added a test for all 6 possible node insertion cases

AlpinYukseloglu

Great job – looks like happy path insertions are working generally correctly! This implementation is still likely vulnerable to being pushed to a linear-time worst case, but we should discuss how to address this offline and do a follow up PR.

Did a first pass with comments on readability (especially on the core insertion function & tests) that we should address before merging. Happy to discuss these and move them to separate issues to not block this as well

contracts/sumtree-orderbook/NOTICE

AlpinYukseloglu · 2024-03-11T04:01:43Z

contracts/sumtree-orderbook/src/sumtree/node.rs

+    /// If the node is a leaf it will be inserted by the following priority:
+    /// 1. New node fits in either left or right range, insert accordingly
+    /// 2. Left is empty, insert left
+    /// 3. Incompatible left, Right is empty, insert right


Can we clarify what incompatible refers to here?

AlpinYukseloglu · 2024-03-11T04:20:11Z

contracts/sumtree-orderbook/src/sumtree/node.rs

+        let maybe_right = self.get_right(storage)?;
+
+        // Check if left node exists
+        if let Some(mut left_node) = maybe_left {


Thoughts on splitting these branches up into higher level conditionals that are only 1 branch deep and have variable names for the conditions (e.g. leftLeaf && rightInternalInRange)? I'm finding these multi layer if/elses somewhat difficult to keep track of mentally while reviewing, and I suspect future reviewers will feel similarly.

This would also help us better understand where there is shared logic or overlap that can help lower the number of cases we need to separately handle

AlpinYukseloglu · 2024-03-11T04:33:52Z

contracts/sumtree-orderbook/src/sumtree/node.rs

+        // New node is being placed below current, update ranges and accumulator as tree is traversed
+        self.add_value(new_node.get_value())?;
+        if self.get_min_range() > new_node.get_min_range() {
+            self.set_min_range(new_node.get_min_range())?;


Is there a risk here that the range of the left child of root gets expanded to overlap with the right child of root? If this is the case, I suspect this would get in the way of prefix sum calculations.

Unfortunately we probably can't completely avoid mutating ranges for internal nodes, since creating a new internal node/layer for every out of range insertion can potentially be spammed.

I guess the specific case we're dealing with here is: what do we do when the gap between left and right child ranges is less than the size of an order at the high end of the left range? My intuition is that this shouldn't be possible since individual orders can't have overlapping ranges, but we should validate & document this well

We should include an edge case test for this as part of rebalancing

Sounds good. Created an issue here: #62

AlpinYukseloglu · 2024-03-11T04:38:06Z

contracts/sumtree-orderbook/src/sumtree/test/test_node.rs

+
+use crate::sumtree::node::{generate_node_id, NodeType, TreeNode, NODES};
+
+pub fn print_tree(storage: &dyn Storage, node: &TreeNode, depth: u8, top: bool) {


Thoughts on having the trees print vertically so we can compare better against hand-solved examples?

5e34b08

I'm not sure how well this algorithm will scale with tree size, it quickly becomes very unreadable. It may even have issues with node string length when printing the nodes.

contracts/sumtree-orderbook/src/sumtree/test/test_node.rs

AlpinYukseloglu

Great work with the fixes and readability for tests. I created issues for non-blocking comments to not hold up this PR since other ones depend on it, but I did have one blocking question about a potential broken invariant for case 3 (and I suspect others as well if we consider deletion)

AlpinYukseloglu · 2024-03-13T20:45:10Z

contracts/sumtree-orderbook/src/sumtree/node.rs

+        // New node is being placed below current, update ranges and accumulator as tree is traversed
+        self.add_value(new_node.get_value())?;
+        if self.get_min_range() > new_node.get_min_range() {
+            self.set_min_range(new_node.get_min_range())?;


Sounds good. Created an issue here: #62

AlpinYukseloglu · 2024-03-13T21:18:46Z

contracts/sumtree-orderbook/src/sumtree/test/test_node.rs

+    s
+}
+
+pub fn print_tree_row(row: Vec<(Option<TreeNode>, Option<TreeNode>)>, top: bool, height: u32) {


nit: I wonder if it would make sense to move these helpers to the end of the file to not distract from the core tests

Agreed, easier to look straight at the test cases. Printing algorithm is less important.

AlpinYukseloglu · 2024-03-13T21:24:42Z

contracts/sumtree-orderbook/src/sumtree/test/test_node.rs

+#[test]
+fn test_node_insert_valid() {
+    let book_id = 1;
+    let tick_id = 1;


Since we expect to have independent trees on each tick for each orderbook, we should probably have another layer of tests that does not hardcode these and builds multiple trees in the same test case.

Moved this into an issue here #64 since this makes sense to implement after rebalancing logic is complete to avoid needing to recompute too many test cases

Should we test this independently? Or as part of order cancellation?

I think it's good to have separate sanity check/unit tests for cancellation & higher level flows

AlpinYukseloglu · 2024-03-13T21:38:38Z

contracts/sumtree-orderbook/src/sumtree/test/test_node.rs

+        //     ┌────────
+        // 2: 1 10
+        TestNodeInsertCase {
+            name: "Case 2: Left Empty Right Empty",


Is this supposed to say Left Leaf, Right Empty?

The case is the precondition, this tree is completely empty with only one insert. Might be a better descriptor.

AlpinYukseloglu · 2024-03-13T21:39:03Z

contracts/sumtree-orderbook/src/sumtree/test/test_node.rs

+        //     ┌────────────────┐
+        // 2: 1 10         3: 12 10
+        TestNodeInsertCase {
+            name: "Case 3: Left Leaf, Right Empty",


And this one Left Leaf, Right Leaf?

5f1151e

Does this make the cases clearer?

Hmm I see. Probably worth having a distinction between the precondition and the action being tested (e.g. [Left Internal, Right Internal] -> Insert right side). Non blocking though

contracts/sumtree-orderbook/src/sumtree/test/test_node.rs

AlpinYukseloglu · 2024-03-13T21:55:05Z

contracts/sumtree-orderbook/src/sumtree/node.rs

+        }
+
+        // Case 3
+        if !is_in_left_range && maybe_right.is_none() {


Err does this make it possible to insert a node with an ETAS that is below the range of the left child into the right child?

e.g. assume you create two orders and cancel (insert) the second. You now have a left leaf (right empty). When you cancel (insert) the first, it extends the minimum range of the root node but places the order as the right child?

For the prefix sum to work, all the leaves need to be stored in increasing ETAS from left to right, so unless I'm missing something here we might need to add some more cases :/

78403e4

Added two more cases:

As describe, node on left is higher than new node, right empty, reorder

Node on right is smaller than new node, left empty, reorder

crnbarr93 · 2024-03-14T13:29:32Z

I'm wondering if min/max range is a sufficient check? Could we consider checking the median value of the node and comparing which is closer when the node is between left and right values?

Could improve balance.

AlpinYukseloglu · 2024-03-15T19:13:26Z

I'm wondering if min/max range is a sufficient check? Could we consider checking the median value of the node and comparing which is closer when the node is between left and right values?

Oh hmm that's an interesting idea – moving this to an offline discussion to dive a bit deeper

AlpinYukseloglu

This LGTM as a happy-path implementation. There were several loose threads/bugs that I either created issues for or committed directly to not block this PR further. Happy to discuss offline & make additional changes in follow up PRs.

Also as a heads up I primarily reviewed the sumtree logic in this PR and only looked at the tick wiring parts at a very cursory level.

AlpinYukseloglu · 2024-03-15T19:55:10Z

contracts/sumtree-orderbook/src/sumtree/node.rs

+
+        // Case 3: reordering
+        let is_lower_than_left_leaf = maybe_left.clone().map_or(false, |l| {
+            !l.is_internal() && new_node.get_max_range() <= l.get_min_range()


We seem to be handling range bounds inconsistently/potentially incorrectly (i.e. this assumes they're fully exclusive while splitting logic assumes they're fully inclusive, and the correct approach is probably neither). Created an issue to track this #68

AlpinYukseloglu · 2024-03-15T20:21:07Z

contracts/sumtree-orderbook/src/sumtree/node.rs

+            (self.key, new_node.key)
+        } else {
+            (new_node.key, self.key)
+        };


This seems technically correct but a somewhat risky implementation. We should probably be much more explicit in the check here (the min of one node being greater than the min of the other feels excessively loose + we should probably have explicit cases & error in else)

Created issue here #70

AlpinYukseloglu · 2024-03-15T20:21:47Z

contracts/sumtree-orderbook/src/sumtree/node.rs

+            new_node.get_max_range().max(self.get_max_range()),
+        );
+
+        let mut new_parent = TreeNode::new(


We should also be setting the new node's parent here right?

Committed this change to not block the PR further – if this was intentional/I made a mistake we can fix in a follow up PR

AlpinYukseloglu · 2024-03-15T20:36:09Z

contracts/sumtree-orderbook/src/sumtree/node.rs

+        // Case 5: Reordering
+        // TODO: Add edge case test for this
+        let is_higher_than_right_leaf = maybe_right.clone().map_or(false, |r| {
+            !r.is_internal() && new_node.get_min_range() >= r.get_min_range()


This should be new_node.get_min_range() >= r.get_max_range() right?

Committed the fix directly for this one as well

AlpinYukseloglu · 2024-03-15T20:49:07Z

contracts/sumtree-orderbook/src/sumtree/node.rs

+        }
+
+        // Case 5: Reordering
+        // TODO: Add edge case test for this


Created an issue to track this so we don't forget #69

crnbarr93 added 13 commits February 27, 2024 15:22

Wired up tick price

cb4c27c

Abstracted method for mapping order direction to tick math

4bc6026

Comment and adjustment

de76856

Added basic tick math tests

cc1a506

Fixed failing place limit tests

20dc792

Fixed erroneous summation of funds to be sent to placed order

cf8f06a

Fix to received funds check, tests broken

ada9f1f

Reverted previous changes

db3bd79

Sumtree insertion

9351622

Moved testing fn to test file

ce443a7

Linting fixes

f8807e1

Added comments

54ac284

Fixed failing test

f725ea3

crnbarr93 requested a review from AlpinYukseloglu March 8, 2024 17:05

crnbarr93 linked an issue Mar 8, 2024 that may be closed by this pull request

[Sumtree]: Sumtree Insertion #55

Closed

AlpinYukseloglu reviewed Mar 11, 2024

View reviewed changes

AlpinYukseloglu mentioned this pull request Mar 11, 2024

EPIC: Sumtree/LOOT #44

Open

73 tasks

crnbarr93 added 5 commits March 12, 2024 11:52

Improve insert readability

791b6c5

Small refactor

e6e5d58

Added vertical tree printing

5e34b08

Small comment adjustment

0983152

Altered copyright

35c20c6

AlpinYukseloglu reviewed Mar 13, 2024

View reviewed changes

Handled node reordering

78403e4

crnbarr93 added 2 commits March 14, 2024 13:38

Added pre-state trees to comments

5f1151e

Terminology consistency

31bb82c

AlpinYukseloglu added 2 commits March 15, 2024 13:31

link new internal nodes back to their parents

e1efca9

fix reordering range check

63a7c0c

AlpinYukseloglu approved these changes Mar 15, 2024

View reviewed changes

AlpinYukseloglu merged commit 623803c into main Mar 15, 2024
2 checks passed

AlpinYukseloglu deleted the connor/sumtree-init branch March 15, 2024 21:29

This was referenced Mar 15, 2024

[Sumtree]: Set up sumtree/LOOT contract & directory #45

Closed

[Sumtree]: High level sumtree spec #46

Closed

[Orderbook]: Wiring up tick to price to fulfillments #49

Closed

[Orderbook]: Wire up tick to price conversion into market filling logic #43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sumtree]: Initialization and Insertion for Sumtree #56

[Sumtree]: Initialization and Insertion for Sumtree #56

crnbarr93 commented Mar 8, 2024

AlpinYukseloglu left a comment

AlpinYukseloglu Mar 11, 2024

crnbarr93 Mar 12, 2024

AlpinYukseloglu Mar 11, 2024

crnbarr93 Mar 12, 2024

AlpinYukseloglu Mar 11, 2024

crnbarr93 Mar 12, 2024

AlpinYukseloglu Mar 13, 2024

AlpinYukseloglu Mar 11, 2024

crnbarr93 Mar 12, 2024

AlpinYukseloglu left a comment

AlpinYukseloglu Mar 13, 2024

AlpinYukseloglu Mar 13, 2024

crnbarr93 Mar 14, 2024

AlpinYukseloglu Mar 13, 2024

crnbarr93 Mar 14, 2024

AlpinYukseloglu Mar 15, 2024

AlpinYukseloglu Mar 13, 2024

crnbarr93 Mar 14, 2024

AlpinYukseloglu Mar 13, 2024

crnbarr93 Mar 14, 2024

AlpinYukseloglu Mar 15, 2024

AlpinYukseloglu Mar 13, 2024

crnbarr93 Mar 14, 2024 •

edited

Loading

crnbarr93 commented Mar 14, 2024

AlpinYukseloglu commented Mar 15, 2024

AlpinYukseloglu left a comment

AlpinYukseloglu Mar 15, 2024 •

edited

Loading

AlpinYukseloglu Mar 15, 2024

AlpinYukseloglu Mar 15, 2024

AlpinYukseloglu Mar 15, 2024

AlpinYukseloglu Mar 15, 2024

AlpinYukseloglu Mar 15, 2024

AlpinYukseloglu Mar 15, 2024


		use crate::sumtree::node::{generate_node_id, NodeType, TreeNode, NODES};

		pub fn print_tree(storage: &dyn Storage, node: &TreeNode, depth: u8, top: bool) {

[Sumtree]: Initialization and Insertion for Sumtree #56

[Sumtree]: Initialization and Insertion for Sumtree #56

Conversation

crnbarr93 commented Mar 8, 2024

What is the purpose of the change

Testing and Verifying

AlpinYukseloglu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlpinYukseloglu 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

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

crnbarr93 Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

crnbarr93 commented Mar 14, 2024

AlpinYukseloglu commented Mar 15, 2024

AlpinYukseloglu left a comment

Choose a reason for hiding this comment

AlpinYukseloglu Mar 15, 2024 • edited Loading

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

crnbarr93 Mar 14, 2024 •

edited

Loading

AlpinYukseloglu Mar 15, 2024 •

edited

Loading