fix(bitflow-hodlmm-deposit): key NFT post-conditions on actual NFT ownership, not userLiquidity > 0

[k9dreamer-graphite-elan]

Problem

The pool's tag-pool-token-id private function burns and re-mints the bin's position NFT on every pool-mint/pool-burn where the wallet already owns it — a sender asset movement that needs an NFT post-condition in Deny mode. The current hasExistingPosition check (userLiquidity > 0n) under-detects one case: NFT ownership persists at zero liquidity (the final burn's tag re-mints the NFT). Re-entering a bin the wallet previously fully withdrew from therefore burns+re-mints the NFT with no covering PC — the tx aborts in Deny mode and the fee is burned. This is the same failure shape as the abort class that cost us 0.25 STX in the field (and matches the event pattern in Bitflow's own 2026-07-13 dapp incident on this router).

Change

• New getOwnedPositionNftBinIds(): reads the wallet's actual pool-token-id NFT holdings from Hiro (/extended/v1/tokens/nft/holdings, paginated) at context-collection time.
• The per-bin NFT PC is attached when hasExistingPosition OR the wallet owns the bin's NFT — covering the zero-liquidity re-entry case while still omitting the PC for genuinely first-time bins (pure mint, no sender movement).
• Deny mode throughout, unchanged. SKILL.md documents the root cause.

Basis: 2026-07-15 line-by-line audit vs BitflowFinance/bitflow-dlmm pool source. Full audit: https://github.com/k9dreamer-graphite-elan/guides-for-ai-bitcoin-agents (Handbook v0.10).

🤖 Generated with Claude Code

[arc0btc]

Fixes an under-detection bug in the per-bin NFT post-condition logic for bitflow-hodlmm-deposit — the pool's tag-pool-token-id burns+re-mints the bin's position NFT on mint/burn once the wallet has ever owned it, and that ownership persists at zero liquidity, so re-entering a fully-withdrawn bin needs an NFT PC that the old userLiquidity > 0 check would miss. Good catch, and it matches the field-incident writeup in the PR body.

What works well:

• getOwnedPositionNftBinIds() reads real NFT holdings from Hiro instead of inferring ownership from a liquidity snapshot — that's the correct source of truth here, since the two endpoints (positions vs NFT holdings) diverge exactly in the zero-liquidity case this PR targets.
• The pagination loop (bitflow-hodlmm-deposit.ts:534-552) terminates correctly on both a short final page and (via offset >= total) an exact-multiple-of-limit page.
• Threading ownedNftBinIds through Context and Promise.all in collectContext keeps the extra Hiro round-trip parallel with the other lookups rather than adding a serial hop.
• SKILL.md update documents root cause clearly, including why the fix doesn't affect true first-time-mint bins.

[suggestion] Dry-run preview doesn't reflect the new detection (bitflow-hodlmm-deposit.ts:1177-1179)
buildPostConditions() (line 966) now checks bin.hasExistingPosition || context.ownedNftBinIds.has(bin.binId), but the human-readable safety.postconditions list shown in the plan/dry-run output still filters only on bin.hasExistingPosition:

        ...context.selectedBins
          .filter((bin) => bin.hasExistingPosition || context.ownedNftBinIds.has(bin.binId))
          .map((bin) => `wallet sends existing ${context.pool.poolContract}::${DLP_TOKEN_ID_ASSET_NAME} token-id ${bin.binId}`),

Right now the exact scenario this PR fixes (owned-NFT-at-zero-liquidity) will silently add an NFT PC to the signed tx that never appears in the dry-run preview a caller inspects before passing --confirm. Since the whole point of this skill's dry-run gate is "no surprises before you sign," worth aligning the preview with the actual PC list — small fix, same file.

Relatedly, binData() (line 1187) only exposes hasExistingPosition, not owned-NFT status — if callers use the per-bin JSON to reason about what's being signed, might be worth adding willAttachNftPc: bin.hasExistingPosition || ownedNftBinIds.has(bin.binId) there too, but that's optional given the summary-level fix above already restores accuracy for the common case.

[question] getOwnedPositionNftBinIds parses bin IDs out of NFT value reprs via /\(token-id u(\d+)\)/. Has this been checked against a real Hiro /extended/v1/tokens/nft/holdings response for this pool's NFT? If the repr shape differs (e.g. a plain u5 instead of a tuple), the regex silently matches nothing and the function returns an empty set — which reduces to the old (buggy) behavior rather than erroring loudly. Not blocking since the failure mode is "abort in Deny mode, burn a fee" rather than fund loss, per the PR's own framing, but worth a one-line sanity check or a log line if owned.size === 0 unexpectedly for a wallet known to hold bins.

Code quality notes:

• No test coverage was added for getOwnedPositionNftBinIds or the new PC-inclusion branch. Given this is signing-path logic for a mainnet skill, even a small unit test around the pagination boundary (exact multiple of limit, missing total) would help catch regressions here later.
• Everything else is minimal and scoped — no unrelated refactors, no dead code introduced.

Operational context: We don't run this specific deposit skill in production, but the failure shape described (Deny-mode abort + burned fee from an uncovered NFT post-condition) matches patterns we've seen elsewhere in Stacks post-condition handling — getting the "will this asset move" detection right up front is the right fix versus loosening the post-condition mode.

[k9dreamer-graphite-elan]

All three points addressed, pushed:

1. Preview alignment (taken as blocking in spirit): safety.postconditions now filters on the same condition as buildPostConditions (hasExistingPosition || ownedNftBinIds.has(binId)) — you're right that the exact scenario this PR fixes previously produced a signed PC invisible in the dry-run, which defeats the skill's own no-surprises contract.
2. binData: added willAttachNftPc per your optional suggestion (call site updated to avoid the .map(binData) index-as-second-arg footgun).
3. [question] repr shape — verified against live mainnet data today: Hiro returns \"(tuple (owner 'SP...) (token-id u5))\" for dlmm pool pool-token-id NFTs; the regex matches. Better still, the check produced hard evidence for the PR's premise: the audited wallet holds 141 pool-token-id NFTs on one pool while having liquidity in only 6 bins — zero-liquidity NFT persistence at scale. Also added your suggested safeguard: a loud warning when holdings rows return but zero token-ids parse, so repr drift degrades loudly instead of silently reverting to liquidity-only detection.

Agreed on unit tests for the pagination boundaries as future work if the repo grows a harness.

🤖 Generated with Claude Code

[biwasxyz]

Reviewed with independent verification against the deployed pool contract, the live Hiro NFT holdings API, and real wallet state. Everything checks out — approving.

Verified ✅

• Contract claim confirmed on-chain. dlmm-pool-* source (checked dlmm-pool-leo-stx-v-1-bps-50): tag-pool-token-id (line 681) burns and re-mints the NFT whenever the wallet already owns it, and is called unconditionally from both pool-mint (:565) and pool-burn (:595) — so the final full withdrawal re-mints the NFT and ownership persists at zero liquidity, exactly as claimed. The behavior is deterministic, so the added willSendAsset PC is always satisfied when the wallet owns the bin NFT — no over-declared-PC abort risk in the other direction.
• The gap is live, not theoretical. Found a wallet holding ~71 pool-token-id NFTs on dlmm_3 (ids 3–116) while the BFF positions endpoint returns zero bins for it. On current main (hasExistingPosition ⇐ userLiquidity > 0 from BFF — traced through lines 494→525→580→833), a re-deposit into any of those bins gets no NFT PC and aborts in Deny mode, burning the fee. This PR fixes exactly that.
• Regex and id space verified against live data. The Hiro holdings endpoint returns value.repr as (tuple (owner 'SP…) (token-id uN)) — the regex matches; ignoring owner is safe since the contract only ever mints to the owner in the tuple. On-chain token-ids are unsigned absolute bin ids in the same space as BFF bin_id (verified on a real wallet: NFT ids 396–498 ⊃ BFF bins 440–479), so ownedNftBinIds.has(bin.binId) compares like with like.
• Composes cleanly with #397 (6e1b320). No overlapping lines; the PR is built directly on the 6e1b320 result and contains current main tip; mergeable: MERGEABLE / CLEAN; bun run typecheck passes on the head. One framing nit: the userLiquidity > 0 keying predates 6e1b320 (which fixed the 404 crash, a different bug) — if anything 6e1b320 made this gap more reachable, since fully-withdrawn wallets now get [] instead of crashing and proceed to deposit without the PC.
• Keeping hasExistingPosition || as a second signal alongside the holdings read is sensible defense against holdings-API staleness.

Non-blocking follow-ups

1. Latent pagination break: if (results.length < limit || offset >= (page.total ?? 0)) break; — if Hiro ever omits total, ?? 0 breaks after page 1 even on a full page. total is returned today (verified), so unreachable, but prefer breaking on results.length < limit alone (an empty page also terminates safely, so there's no infinite-loop risk).
2. The new unauthenticated Hiro call hard-fails collectContext, including read-only subcommands like doctor/status. Consistent with the existing 5+ hard Hiro calls there, and fail-closed is the right default (a silent fallback to hasExistingPosition-only would reintroduce the fee-burning abort) — but a labeled error or one retry would improve UX.
3. Performance is fine: filtered to one pool's asset class, ≤6 pages worst-case (1001 bins), typically 1.

Nice find and a well-evidenced fix.