-
Notifications
You must be signed in to change notification settings - Fork 111
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
WIP: Universe root deletion RPC #335
Conversation
Missing an entry for REST in the |
8554346
to
53f326a
Compare
Addressed review feedback + tested locally with this mint script (on regtest, not testnet) https://github.com/jharveyb/remintooor/blob/main/mint_assets.sh After minting, I synced the universe tree of 100 assets to a second tapd, deleted the universe root on that second tapd, and re-synced the same universe root without issue. Root deletion on the minting node also succeeded. |
53f326a
to
c19825e
Compare
Diff looking better, I think this is just missing unit tests + an itest (to replicate the scenario we execute live on the instance). |
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 pending some unit and integration tests!
836e041
to
7cabe13
Compare
Added unit tests + itests, no large changes otherwise. Needed one change here to handle trees with missing leaves: Line 301 in 7cabe13
|
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.
Found one potential issue, the rest looks pretty good!
7cabe13
to
0218f09
Compare
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.
Great improvements 👍
One or two nits, and one doc fix.
// Instantiate a compact tree so we can delete the MS-SMT | ||
// backing the universe. | ||
universeTree := mssmt.NewCompactedTree( | ||
newTreeStoreWrapperTx(db, b.smtNamespace), | ||
) |
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'm not proposing a further change in this PR, but I think our language is sub-optimal here.
We're not actually creating a new compact tree or instantiating a compact tree. We're creating a database handle for a compact tree. I think we should rename NewCompactedTree
to NewCompactedTreeHandle
or something like that.
@@ -354,6 +371,29 @@ func TestUniverseTreeIsolation(t *testing.T) { | |||
} | |||
return false | |||
})) | |||
|
|||
// We should be able to delete one Universe with no effect on the other. |
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.
👍
0218f09
to
168772a
Compare
CI failure in postgres itests, but the logs show all itests passing, in ~520s; not sure what the issue may be there? |
Yeah, saw that too, probably just a weird timing issue in the itest framework. |
Interim fix for #320, which should be reworked once we land other DB changes.
Add a command to delete specific universe trees, which is useful for avoiding universe sync failures. Ideally this would be done more automatically with cascading deletes in the DB.
Successfully tested as fixing the root cause of #320, my
tapd
no longer shows an incorrect root for that asset group. Sync with the default universe still fails.