Skip to content
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

Universe events stats handle universe identity proof type #561

Merged
merged 6 commits into from
Oct 11, 2023

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Oct 11, 2023

Depends on #540 . Do not merge this PR before merging #540 I've converted this PR to draft until all parent PRs have been merged.

This PR adds universe identity proof type support to universe events stats database SQL statements.

@ffranr ffranr requested review from Roasbeef and guggero October 11, 2023 01:09
@ffranr ffranr self-assigned this Oct 11, 2023
@ffranr ffranr force-pushed the uni-events-proof-type branch from 646b3d1 to e90f314 Compare October 11, 2023 01:35
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM for the split approach.

I think we still need to decide if we go with split or unified on the RPC layer.

tapdb/universe_stats.go Show resolved Hide resolved
tapdb/sqlc/queries/universe.sql Show resolved Hide resolved
tapdb/sqlc/queries/universe.sql Show resolved Hide resolved
tapdb/sqlc/migrations/000007_universe.up.sql Show resolved Hide resolved
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good as far as I can tell. Will do some manual testing once the base PR has been merged.

tapdb/sqlc/migrations/000007_universe.up.sql Show resolved Hide resolved
@ffranr ffranr force-pushed the uni-namespace-proof-type branch from 6bb6a33 to b2d27d6 Compare October 11, 2023 13:42
@ffranr ffranr force-pushed the uni-events-proof-type branch from e90f314 to fbab2ec Compare October 11, 2023 13:43
@ffranr
Copy link
Contributor Author

ffranr commented Oct 11, 2023

@Roasbeef stats are still aggregated as before. This PR adds proof type resolution to how we store stats, but stats are still queried and aggregated as before.

@ffranr ffranr marked this pull request as draft October 11, 2023 14:19
@ffranr ffranr requested review from Roasbeef and guggero October 11, 2023 14:19
@Roasbeef Roasbeef marked this pull request as ready for review October 11, 2023 17:43
@Roasbeef Roasbeef changed the base branch from uni-namespace-proof-type to main October 11, 2023 18:08
The universe_stats view represents per asset/asset group aggregated
stats. However, the universe namespace field is now specific to a proof
type. This commit removes the namespace field from the stats group by.
@ffranr ffranr force-pushed the uni-events-proof-type branch from fbab2ec to 91186e6 Compare October 11, 2023 18:20
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a bunch of manual testing and found a bunch of small things that should be fixed.
Summarized them in this commit (feel free to just take over/split into fixup commits): https://github.com/lightninglabs/taproot-assets/tree/561-fixups

With those fixes applied, I think this is ready to go.

rpcserver.go Outdated Show resolved Hide resolved
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ffranr ffranr force-pushed the uni-events-proof-type branch from 111d885 to dec1676 Compare October 11, 2023 21:06
@ffranr ffranr requested a review from guggero October 11, 2023 21:07
@guggero guggero enabled auto-merge October 11, 2023 21:09
@guggero guggero added this pull request to the merge queue Oct 11, 2023
Merged via the queue into main with commit d7c9a25 Oct 11, 2023
@guggero guggero deleted the uni-events-proof-type branch October 11, 2023 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants