tapgarden: hardening phase one (extraction)#2190
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 initiates a large-scale architectural hardening of the tapgarden package. By decoupling node-side interfaces and specific logic components into their own packages, the codebase is now more modular and easier to maintain. Furthermore, the PR resolves a latent state-related issue in the minting process by enforcing purity in key derivation, preventing potential commitment errors. 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 refactors the codebase by splitting several components out of the monolithic tapgarden package into more focused packages: tapnode (for node-side abstractions and mocks), tapreorg (for the re-org watcher), and tapcustody (for the asset custodian). Additionally, it makes MintingOutputKey a pure function by removing its internal cache to ensure it correctly respects different sibling arguments. Feedback on the changes suggests renaming tapcustody/mock.go to mock_test.go to prevent test-only mock code from being compiled into production binaries, and adding missing documentation comments to its exported functions to comply with the repository style guide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
jtobin
force-pushed
the
tapgarden-extraction
branch
2 times, most recently
from
4c687b7 to
a59d6ec
Compare
Coverage Report for CI Build 28966962901Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.2%) to 36.065%Details
Uncovered Changes
Coverage Regressions75 previously-covered lines in 13 files lost coverage.
Coverage Stats
💛 - Coveralls |
github-project-automation
Bot
moved this from 🆕 New
to 👀 In review
in Taproot-Assets Project Board
ChainBridge, WalletAnchor, KeyRing, and GroupFetcher describe tapd's view of its node-side dependencies (chain backend, wallet, key ring, group lookups), but they lived in tapgarden/interface.go. They were consumed by tapfreighter, tapchannel, tapconfig, lndservices, universe/supplycommit, universe/supplyverifier, and backup -- which made the tapgarden home a documented accident of authorship rather than a property of what the package is. Introduce a tapnode package that houses the four interfaces alone. Concrete implementations remain in lndservices; the rest of the tree imports tapnode for the interface only. tapgarden retains the minting-specific symbols (MintingStore, MintSupplyCommitter, Planter, BatchState, FundBatchResp, etc.). No behavior change.
The re-org watcher guards proof integrity in the face of chain re-orgs, which is unrelated to growing seedlings into assets. It lived in tapgarden only because that was the package that first happened to need it. Move tapgarden/re-org_watcher.go to tapreorg/watcher.go, renaming ReOrgWatcher -> Watcher, ReOrgWatcherConfig -> Config, and NewReOrgWatcher -> NewWatcher to drop the package-name stutter. The new package owns its own DefaultTimeout and logger (subsystem "RORG"). Drop a dead pre-overwrite assignment in DefaultUpdateCallback while we're here. tapconfig, tapcfg/server.go, and the root-level log.go are updated for the new symbols.
The Custodian receives assets transferred to this node; the Planter mints new ones. Keeping them in the same package forced every consumer that only cared about receipt to import the minting code, and conflated the two responsibilities at the package boundary. Hoist the Custodian, its config (renamed Config, dropping the type stutter), its event surface (AssetReceiveEvent, AddrImportErrEvent, AddrImportCompleteEvent), its helpers (AddrMatchesAsset, EventMatchesProof, WaitForAddrImport, AddrImportStatus), and MockAssetSyncer into a new tapcustody package. Subsystem CSTD. GenHeaderVerifier, needed by caretaker, planter, and custodian plus tapfreighter, supplyverifier, rpcserver, and tapcfg, moves to tapnode alongside the ChainBridge interface it wraps -- a node-level concern, not a tapgarden one.
MintingOutputKey memoized its result on first call; subsequent calls silently ignored the sibling argument. The caretaker's BatchStateCommitted branch was the only caller that exploited this, passing nil and relying on the Frozen branch having cached the real sibling. Any path that reached Committed without going through Frozen first -- a future RBF retry, a partial restart, a refactor -- would have silently computed and cached the wrong output key. Drop the cache. MintingBatch loses its memoized mintingPubKey and taprootAssetScriptRoot fields; MintingOutputKey recomputes from (BatchKey, RootAssetCommitment, sibling) on every call. The caretaker's Committed branch now loads the sibling preimage before calling MintingOutputKey and passes it explicitly; the same preimage is already used a few lines below for siblingBytes, so this is a reorder rather than a new I/O cost. The work is a single tapscript hash plus an ECC tweak -- not expensive enough to justify a memoizing getter that introduces correctness footguns.
jtobin
force-pushed
the
tapgarden-extraction
branch
from
a59d6ec to
3768622
Compare
343c0d9
github-project-automation
Bot
moved this from 👀 In review
to ✅ Done
in Taproot-Assets Project Board
3 participants
This is an initial chunk of the large-scale tapgarden hardening work originally contained in #2153. It handles the following bullet from the original PR description:
It contains the following changes (summary ctsy Opus):
Surface reduction.
tapgardenhad accumulated three concerns that didn't belong to it: node-side interfaces (chain bridge, key ring, wallet anchor, group fetcher), the re-org watcher, and the custodian. They move to new packagestapnode(withtapnodemockfor tests),tapreorg, andtapcustody. Pure moves with no behavior change; what remains intapgardenis more legibly about minting.MintingOutputKey purity.
MintingOutputKeyread like a function of(batch, sibling)but was secretly stateful — it cached its first answer and returned that on subsequent calls regardless of the sibling argument. Only latent because in practice the call sites happened to agree; any restart or refactor that changed their order would have made the batch anchor to an address the tapscript witness couldn't prove against. The cache is dropped and the function is now actually pure.