taprpc+tapdb: add GenesisInfo to AssetGroupBalance in ListBalances#2130
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the 'ListBalances' RPC functionality by including comprehensive metadata for grouped assets. By adding genesis information and decimal display settings to the 'AssetGroupBalance' response, clients can now display asset details directly without requiring additional API calls. The changes involve updates to the protocol buffers, database query logic, and the RPC server implementation to populate these new fields efficiently. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the ListBalances RPC by including representative genesis information and decimal display metadata for asset groups. The changes involve updating the database queries to select anchor genesis data, extending the AssetGroupBalance structures in both the database and RPC layers, and populating these new fields in the RPC server. Feedback was provided regarding a potential N+1 query performance issue when fetching decimal displays and a requirement to use structured logging for debug messages as per the repository style guide.
kaldun-tech
force-pushed
the
taprpc-add-GenesisInfo-AssetGroupBalance-ListBalancesResponse
branch
2 times, most recently
from
2b0f3f4 to
48f4267
Compare
|
I believe these CI failures are flakes. @claude please code review and investigate whether the failed CI is due to code changes or flakes |
|
@jtobin @darioAnongba Please re-run the failed CI jobs. My investigation is that the failures were due to flakes but let me know if you want me to look into anything on my end |
| if err == nil { | ||
| groupBalance.DecimalDisplay = fn.MapOptionZ( | ||
| decDisplay, | ||
| func(d uint32) *taprpc.DecimalDisplay { | ||
| return &taprpc.DecimalDisplay{ | ||
| DecimalDisplay: d, | ||
| } | ||
| }, | ||
| ) | ||
| } else { | ||
| rpcsLog.DebugS(ctx, "Failed to fetch decimal display", | ||
| btclog.Fmt("asset_id", "%x", balance.ID[:]), | ||
| "error", err) | ||
| } |
There was a problem hiding this comment.
We can avoid silently logging debug and escalate instead.
| if err == nil { | |
| groupBalance.DecimalDisplay = fn.MapOptionZ( | |
| decDisplay, | |
| func(d uint32) *taprpc.DecimalDisplay { | |
| return &taprpc.DecimalDisplay{ | |
| DecimalDisplay: d, | |
| } | |
| }, | |
| ) | |
| } else { | |
| rpcsLog.DebugS(ctx, "Failed to fetch decimal display", | |
| btclog.Fmt("asset_id", "%x", balance.ID[:]), | |
| "error", err) | |
| } | |
| switch { | |
| case errors.Is(err, address.ErrAssetMetaNotFound): | |
| // Asset legitimately has no meta — leave DecimalDisplay unset. | |
| case err != nil: | |
| return nil, fmt.Errorf("unable to fetch decimal display "+ | |
| "for asset %x: %w", balance.ID[:], err) | |
| default: | |
| groupBalance.DecimalDisplay = fn.MapOptionZ( | |
| decDisplay, | |
| func(d uint32) *taprpc.DecimalDisplay { | |
| return &taprpc.DecimalDisplay{DecimalDisplay: d} | |
| }, | |
| ) | |
| } |
| -- works for locally-minted assets where the anchor is inserted first. | ||
| SELECT | ||
| tweaked_group_key, | ||
| MIN(gen_asset_id) as anchor_gen_id |
There was a problem hiding this comment.
gen_asset_id is a local-DB primary key reflecting local insertion order, not protocol-level ordering or any canonical anchor. The canonical anchor is stored explicitly in asset_groups.genesis_point_id
A fix is to join through asset_groups.genesis_point_id to resolve the actual anchor gen_asset_id. The existing FetchGroupByGroupKey pattern (ORDER BY witness_id LIMIT 1) already does this.
github-project-automation
Bot
moved this from 🆕 New
to 👀 In review
in Taproot-Assets Project Board
|
Hey @darioAnongba thanks for the feedback. I'm making a few changes:
|
Added GenesisInfo and DecimalDisplay fields to the AssetGroupBalance message, making the group_by=group_key mode of ListBalances useful for clients that need asset metadata alongside balances. The SQL query was modified to join genesis_info_view and return genesis data for a representative issuance per group (selected via MIN to get the earliest-inserted genesis, which is the anchor for locally-minted assets). Additional tests in asset_store_test for new failure cases. Fixes lightninglabs#2066. Signed-off-by: kaldun-tech <tsmereka@protonmail.com>
kaldun-tech
force-pushed
the
taprpc-add-GenesisInfo-AssetGroupBalance-ListBalancesResponse
branch
from
48f4267 to
44d12a0
Compare
|
@darioAnongba Pushed my changes
|
|
@kaldun-tech, remember to re-request review from reviewers when ready |
3 participants
Add
GenesisInfoandDecimalDisplayfields toAssetGroupBalancein theListBalancesRPC response when usinggroup_by=group_keymode.Previously,
AssetGroupBalanceonly containedgroup_keyandbalance, making it impossible for clients to display asset metadata (name, type, decimal display) alongside grouped balances without additional RPC calls.Changes
asset_genesis(field 3) anddecimal_display(field 4) toAssetGroupBalancemessageQueryAssetBalancesByGroupto joingenesis_info_viewand return genesis data for a representative issuance per groupAssetGroupBalancestruct with genesis fields and updated query result mappinglistBalancesByGroupKey, includingDecimalDisplaylookup viaDecDisplayForAssetIDDesign Notes
MIN(gen_asset_id), which gives the earliest-inserted genesis. For locally-minted assets, this is the group anchor. For fungible assets, all tranches share the same name/type, so any genesis provides correct metadata.QueryAssetBalancesByGroupresults (usegroup_by=asset_idmode for those).Test Coverage Gaps
Error handling paths in RPC layer: The
ErrAssetMetaNotFoundand error escalation paths inlistBalancesByGroupKeyare not directly unit tested. This would require mockingAddrBook.DecDisplayForAssetID.Received grouped assets without local anchor: Edge case where we receive a grouped asset but don't have the anchor locally. The new query should still work (joins through
asset_groups.genesis_point_id), but this isn't explicitly tested.Test Plan
make unit pkg=tapdb- verify DB layer changesmake unit pkg=rpcserver- verify RPC marshalingmake itest icase=minting_multi_asset_groupstapcli assets balance --by-groupreturns genesis infoFixes issue #2066