-
Notifications
You must be signed in to change notification settings - Fork 132
tapdb: add implementation of supplycommit.CommitmentTracker and supplycommit.StateMachineStore #1508
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
base: universe-supply-trees
Are you sure you want to change the base?
tapdb: add implementation of supplycommit.CommitmentTracker and supplycommit.StateMachineStore #1508
Conversation
4778c90
to
d718094
Compare
522c3ee
to
d786104
Compare
d718094
to
1db2104
Compare
d786104
to
af5f8f5
Compare
e67b58f
to
6b99f70
Compare
af5f8f5
to
64516c9
Compare
6b99f70
to
5056daa
Compare
64516c9
to
6e0b6a4
Compare
5056daa
to
5c20025
Compare
6e0b6a4
to
aa782c8
Compare
-- Table storing individual update events associated with a pending transition. | ||
CREATE TABLE supply_update_events ( | ||
event_id INTEGER PRIMARY KEY, | ||
|
||
-- Reference to the state transition this event is part of. | ||
transition_id BIGINT NOT NULL REFERENCES supply_commit_transitions(transition_id) ON DELETE CASCADE, |
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.
an update event can't be persisted unless it references an existing transition, is that correct?
If so, then if we're waiting for tx conf then new update events can't be persisted?
If true, then I don't think it matters a great deal, but from a UX perspective, we will need to return an error to the user along the lines of: unconfirmed supply tx, can't accept new asset ignore right now.
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.
That's a great insight. As of right now, if everything is spun up together and a user attempts to add a new update while we're creating a new commitment on-disk, then this'll fail.
I think we can fix this just by removing the NOT NULL
here. We'll need some additional follow ups in the state machine though:
- Any time an update comes through in a state, attempt to insert it to disk w/ a nil
transition_id
. - Once we circle back to the default state, then read all the "dangling" update from disk, and assign them to a state transition (in a single db txn).
Alternatively, we can create some other table, then re-insert them in the default state.
I can either do this in another PR (as it affects this one, and the next), or do the incremental change here. Which is preferred?
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.
Great stuff! Did a first pass to load some context, will continue with the other PRs in the saga.
) VALUES ( | ||
@group_key, | ||
-- Use the target state ID if found, otherwise use the default state ID. | ||
coalesce((SELECT id FROM target_state), (SELECT id FROM default_state)), |
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.
Hmm, I'm a bit surprised this works. So the actual value returned from a sub query is NULL
if there was no row found?
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.
Hehe, yeah I went through a few versions of this. I had one which used a CTE as above as part of the update (so a CTE that inserts, then returns a value), but sqlite
doesn't allow CTE statements in insert/update like say postgres does.
To answer the question, we basically want either the ID of the target state, or the default if a target isn't set. So on the caller side, we just don't specify the state_name
field if we want the default state to be inserted. If no state is set, then target_state
has no rows, so trying to select on it returns nothing. In that case the default_state
query will always return a value (as we set up the table as such in the migration).
assetSpec asset.Specifier) lfn.Result[supplycommit.PreCommits] { | ||
|
||
groupKey := assetSpec.UnwrapGroupKeyToPtr() | ||
if groupKey == nil { |
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: UnwrapGroupKeyOrErr()
? Not that it shortens the code by any meaningful amount, just a bit more explicit.
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.
Or, I guess since we're only ever using the group key part of the specifier, why don't we change the param to be a public key directly? Just to have some semantic meaning around what the key is?
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.
Switched everything over to use UnwrapGroupKeyOrErr
.
I used asset.Specifier
as most other similar interfaces across the codebase are starting to take that value, to force an author/writer to make sure there's logic to handle the various combos.
// NULL (e.g., initial commit before ApplyStateTransition ran), use the | ||
// empty root. | ||
var rootNode *mssmt.BranchNode | ||
if commit.SupplyRootHash == nil || !commit.SupplyRootSum.Valid { |
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.
Check length instead of nil?
tapdb/supply_commit.go
Outdated
// its associated chain confirmation details. | ||
func fetchCommitment(ctx context.Context, db SupplyCommitStore, | ||
commitID sql.NullInt64, groupKeyBytes []byte, | ||
) (lfn.Option[supplycommit.RootCommitment], lfn.Option[commitmentChainInfo], error) { //nolint:lll |
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.
Does the root commitment here really need to be an option? Doesn't look like it's ever None
in a non-error case.
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.
I have it as an option as in supplycommit.SupplyStateTransition
, it's an option. It's an option here as the very first time we go to create a state transition, OldCommitment
won't yet exist.
We can return a value of None
in the very fist error case: if we query and it doesn't exist (ErrNoRows
), then noneRootCommit
with a nil
error is returned.
// assertPendingTransitionExists asserts that a pending (non-finalized) | ||
// transition exists. | ||
// | ||
//nolint:lll |
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 might as well add this to the top of the file so it counts for its entirety 😅
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.
Heh, I prefer to just sprinkle it as needed. The linter comments don't really both me when reading/reviewing code.
In this instance, I added it as there's no good place to split the line w/o it looking awkward. I'd rather just throw this on and move along instead of spending additional time on the more marginal aspects of our code style.
} | ||
``` | ||
|
||
## State Machine Persistence Logic |
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.
Very nice doc!
aa782c8
to
36e3753
Compare
bf61350
to
16ae70c
Compare
36e3753
to
1c17470
Compare
16ae70c
to
67728da
Compare
1c17470
to
b5f3d5e
Compare
c2c394f
to
b1173b9
Compare
dd1d318
to
87fefbc
Compare
b1173b9
to
c6f9962
Compare
87fefbc
to
570e477
Compare
c6f9962
to
da0df38
Compare
570e477
to
6584d4f
Compare
da0df38
to
ee045a1
Compare
6584d4f
to
d1ae6c4
Compare
ee045a1
to
598067d
Compare
bca0e22
to
faae3fc
Compare
598067d
to
9f03d81
Compare
faae3fc
to
3b3420a
Compare
9f03d81
to
e77d0cf
Compare
3b3420a
to
8fff268
Compare
e77d0cf
to
3044a87
Compare
8fff268
to
c668391
Compare
3044a87
to
9854971
Compare
c668391
to
8b06b69
Compare
… tree state machine In this commit, we add a new migration for the persistence layer of the supply tree state machine. We track a state machine that points to the latest supply commitment. We update the supply commit via a new state transition, which refrerences a series of updates. Once we're ready to apply, we'll apply the updates, mark the transition as finalized, and finally update the relevant pointers. We also make a change to modify mint_anchor_uni_commitments to point to a supply commitment. This lets us keep track of the set of pre commitments that aren't yet spent.
… new spent_by field
…and supplycommit.StateMachineStore In this commit, we add concrete implementation of both CommitmentTracker and StateMachineStore. This implements the persistence layer for the supply commit state machine. The main method to understand is ApplyStateTransition, as it implements the atomic update of all the various components (tree, state transition, etc) on disk.
9854971
to
37bdcfa
Compare
8b06b69
to
b8988d3
Compare
@Roasbeef, remember to re-request review from reviewers when ready |
In this PR, we add a concrete implementation of the two fundamental interfaces that will drive the state machine implemented in #1464.
We start with a state machine on disk for a given group key that also points to the latest supply commitment. When we want to add new elements to the supply tree, we create a new state transition, and then queue pending updates associated with this state transition. Once we've signed and broadcasted the commitment transaction, we then apply the state transition: insert all the updates into the tree, update the state machine to point to that new tree, and finalize the state transition.
See the readme for more details.