-
Notifications
You must be signed in to change notification settings - Fork 132
universe+tapdb: add new block_height
field to universe_leaves
use that to implement block height based supply tree queries
#1596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
3e7ed65
to
c57d19d
Compare
c2c394f
to
b1173b9
Compare
c57d19d
to
bd5d0b4
Compare
b1173b9
to
c6f9962
Compare
bd5d0b4
to
37f23fa
Compare
c6f9962
to
da0df38
Compare
37f23fa
to
be0a52c
Compare
ee045a1
to
598067d
Compare
be0a52c
to
f2a69aa
Compare
f2a69aa
to
93ed469
Compare
598067d
to
9f03d81
Compare
93ed469
to
633373a
Compare
9f03d81
to
e77d0cf
Compare
633373a
to
3ca1517
Compare
e77d0cf
to
3044a87
Compare
3ca1517
to
988837b
Compare
3044a87
to
9854971
Compare
988837b
to
a9ef139
Compare
9854971
to
37bdcfa
Compare
a9ef139
to
ca7db9a
Compare
16f9f9a
to
059ed3f
Compare
99529ae
to
87ba694
Compare
059ed3f
to
5025c8c
Compare
Rebased. |
5025c8c
to
acafde1
Compare
Pull Request Test Coverage Report for Build 16428331342Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
@ffranr: review reminder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Got a couple of suggestions and the linter isn't fully happy yet. Other than that LGTM 🎉
tapdb/supply_tree.go
Outdated
|
||
readTx := NewBaseUniverseReadTx() | ||
dbErr := s.db.ExecTx(ctx, &readTx, func(db BaseUniverseStore) error { | ||
for _, treeType := range []supplycommit.SupplySubTree{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: feels like this would make a good package-level variable with the list of tree types we often iterate over? Would also help a bit with the formatting/readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty much there. One question on SQL query re-use.
-- name: QuerySupplyLeavesByHeight :many | ||
SELECT | ||
leaves.script_key_bytes, | ||
gen.gen_asset_id, | ||
nodes.value AS supply_leaf_bytes, | ||
nodes.sum AS sum_amt, | ||
gen.asset_id, | ||
leaves.block_height | ||
FROM universe_leaves AS leaves | ||
JOIN mssmt_nodes AS nodes | ||
ON leaves.leaf_node_key = nodes.key | ||
AND leaves.leaf_node_namespace = nodes.namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a where
clause in QueryUniverseSupplyLeaves
. Instead of adding a new query here, can't we just extend QueryUniverseSupplyLeaves
with an optional block height?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good eye. These queries differ slightly though: the above is based on always knowing the root_id
to pass in (hence the join with universe supply roots). Here we query based on just the namespace, so we can skip the extra join on the root.
In this commit, we add a migration to add a block_height to the universe leaf table. We'll use this in the future to be able to allow clients to sync block by block for the new supply universe trees.
acafde1
to
beb58bb
Compare
beb58bb
to
3ff47a4
Compare
In this commit, we add a new method to the SupplyTreeStore that is able to read out the leaves of a supply tree based on a start and end height. This will be useful for writing the new syncing state machine and the sub-system that serves the supply tree syncer.
3ff47a4
to
4e7a385
Compare
This should be ready for a final pass. The unit test failure is being addressed in another PR to improve the set of property based tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
In this PR, we add a new method
FetchSupplyLeavesByHeight
, that can be used to query for the leaves of a supply tree by height. This will be useful for writing the new syncing state machine and the sub-system that serves the supply tree syncer. Such a syncer would start from the initially created pre-commitment, then fetch all the leaves for the future updates, assemble those into the supply tree, then verify that everything matches up.Prompts used during development (aider+Gemini 2.5-pro-06-5)
Initial draft creation
Based on by working memory of this new feature, I scanned the relevant files, then made mental nodes of what needed to be updated. I then drafted this fairly detailed prompt that guided
aider
w.r.t which files to update, and the set of high level changes to make.Subsequent iterations
From here I made an edit or two to get things compiling. Then I executed a series of prompts to refine the code, refactor slightly, then add unit tests.
I messed when committing and dropped a commit entirely (lol), so I had
aider
re-add it: