fix(nft): ReconcileBurnedNft reclaims the mint and ExtraAccountMetaList rent (#182)

[0x-SquidSol]

Closes #182.

Problem

#102 stopped this leak for BurnPositionNft and EmergencyBurn, which both close the NFT mint and call close_extra_metas. ReconcileBurnedNft was added later (#138) with seven accounts and no slot for either, so every reconciled NFT abandoned 7,676,880 lamports (~0.0077 SOL) — 2,707,440 for the 261-byte metas PDA and 4,969,440 for the 586-byte mint.

The loss is unrecoverable, and earlier than it first looks. BurnPositionNft and EmergencyBurn are the only instructions that close either account, and both require the holder ATA to hold amount == 1 — already 0 after the out-of-band burn that Reconcile exists to handle. RepairExtraMetas only ever pays rent in. So both are dead from the burn onward, and Reconcile is the only instruction that can still legally touch them, exercised at most once.

Fix

extra_metas and the Token-2022 program become accounts 7 and 8, and both are closed to last_holder_ai — the same recipient the PDA rent already goes to, and the one both burn paths already pay these rents to.

They are required, not optional. I originally proposed optional trailing accounts to avoid an ABI break, and changed it: Reconcile is permissionless, irreversible and one-shot, so an opt-in could be lost permanently to whoever called first — a stale client, a helpful third party, or a griefer paying a single 5,000-lamport fee to destroy 7.68M of someone else's rent. The compatibility that would have bought protects an empty set: tag 7 had no account list in instruction.rs, no row in the README instruction table, and a search across this repo and both sibling trees finds no callers.

Verification

51 pre-existing tests unchanged, plus 5 new ones. cargo clippy --all-targets -- -D warnings clean; cargo build-sbf --tools-version v1.52 succeeds.

The load-bearing test is control_emergency_burn_reclaims_both_when_the_pda_is_alive — identical accounts with Reconcile simply not run, showing EmergencyBurn closes the metas PDA and returns its rent. That isolates "Reconcile omitted them" from "these accounts are inherently unclosable". the_short_seven_account_form_is_rejected pins the required-accounts decision and asserts the short call destroys nothing on its way out.

Honest limits of the suite: the mint close is a Token-2022 CPI, which no-ops under host SyscallStubs, so its lamports do not move in tests. What is proven is the metas rent (moved in-program) landing on the recorded last_holder, plus conservation on that address. The mint close rests on the identical pattern already exercised by BurnPositionNft.

Security review

No new authority. Every pin precedes the new code: close_extra_metas re-derives extra_metas from nft_mint, which is itself pinned to nft_state.nft_mint; token2022::close_account hardcodes the Token-2022 program id, so account 8 cannot redirect the CPI; mint_auth is the canonical PDA; and last_holder_ai is pinned to the recorded last_holder. Substituted metas, substituted mint, substituted mint authority, substituted token program and a substituted rent recipient were each attacked and each fails closed.

Caveats, documented in the code

• Not a complete recovery. The holder's own ATA (~0.0021 SOL) is still theirs to close directly against Token-2022. Closing the mint does not block that — Token-2022's mint close checks only the close-authority extension, supply and signature, and never scans for live token accounts.
• Requires MintCloseAuthority on the mint. Every mint this program creates has it, added before ReconcileBurnedNft existed, so all reconcilable mints carry it. A mint predating that would make the CPI — and therefore the whole reconcile, including the escrow release — revert. Worth knowing before any historical deploy is considered.

Also included

• Tag 7's account list in instruction.rs — it was the only tag without one.
• Its missing row in the README instruction table.
• The handler's account doc, which described seven accounts and marked nft_mint read-only while the code now requires it writable.

Adjacent, not fixed here

The + 128 slack in mint_space + metadata_tlv_size + 128 at mint creation is dead weight — nothing ever reallocs the mint, so those bytes are funded and never used, over-funding every NFT by 890,880 lamports. It comes back to the holder on the normal burn paths, so it is a capital-lockup issue rather than a loss, but it is exactly the amount that made my original estimate 13% low. Worth its own issue.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added permissionless recovery for NFTs burned outside the normal process.
  • Reclaims stranded escrow and associated account rent for the recorded last holder.
  • Automatically closes the recovered NFT’s related accounts once the mint supply reaches zero.

• Bug Fixes

  • Prevented rent from remaining locked after an NFT is burned out-of-band.
  • Added validation to reject incomplete or incompatible recovery account configurations.

• Documentation

  • Documented the new NFT reconciliation instruction and its required accounts.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a64d4e32-1e93-4be5-bae3-c639e428f89e

📥 Commits

Reviewing files that changed from the base of the PR and between 215842e and c52c3bb.

📒 Files selected for processing (4)

• README.md
• src/instruction.rs
• src/processor.rs
• tests/poc_reconcile_rent_leak.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Changes

ReconcileBurnedNft rent recovery

Layer / File(s)	Summary
Reconcile account contract README.md, src/instruction.rs, src/processor.rs	The instruction now documents and consumes nine accounts, including the NFT mint, ExtraAccountMetaList PDA, and Token-2022 program.
Mint and metadata closure src/processor.rs	The processor validates the zero-supply mint, closes the NFT mint and ExtraAccountMetaList, and returns their rent to the recorded last holder.
Rent recovery validation tests/poc_reconcile_rent_leak.rs	Tests cover successful extended reconciliation, rejected seven-account and substituted-program calls, rent recovery, account closure, and EmergencyBurn behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: 🔵 Low · up to c52c3

Reconciliation now requires the mint and metadata accounts and returns their rent to the recorded holder. The identities and authorities are tightly constrained; the remaining merge-readiness risk is that failures after portfolio release rely on transaction atomicity, so rollback behavior should be explicitly confirmed.

Suggested reviewers: dcccrypto

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR closes the NFT mint and ExtraAccountMetaList PDA and returns their rent to last_holder. However, it rejects seven-account callers, so it does not preserve the backward compatibility required by…	Make extra_metas and the Token-2022 program optional trailing accounts so existing seven-account callers continue to work. Preserve validation and rent reclamation when the accounts are provided.
Docstring Coverage	⚠️ Warning	Docstring coverage is 46.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 3 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the primary change: reclaiming NFT mint and ExtraAccountMetaList rent in ReconcileBurnedNft.
Out of Scope Changes check	✅ Passed	The README update, account documentation, processor changes, and regression tests all support the ReconcileBurnedNft rent-reclamation objective in issue #182. No unrelated code changes are identified.

Full details: Linked Issues check

Explanation

The PR closes the NFT mint and ExtraAccountMetaList PDA and returns their rent to last_holder. However, it rejects seven-account callers, so it does not preserve the backward compatibility required by issue #182.

Full details: Docstring Coverage

Explanation

Docstring coverage is 46.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 3 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[0x-SquidSol]

Correction: there IS a first-party caller of tag 7 — percolator-sdk — and the required-accounts form in #183 breaks it

I wrote, both here and in #183:

The compatibility it would preserve protects an empty set: tag 7 had no account list in instruction.rs, no row in the README instruction table, and a search across this repo and both sibling trees finds no callers.

That search was scoped to percolator-nft, percolator and percolator-prog — the three repos I had cloned. It missed dcccrypto/percolator-sdk, which is a first-party TypeScript SDK and does call tag 7:

// percolator-sdk/src/abi/nft.ts
export function encodeNftReconcile(): Uint8Array { ... }   // tag 7

/**
 *   0. [writable]  PositionNft PDA (closed)
 *   1. []          NFT mint (Token-2022 — supply must be 0)
 *   ...
 *   6. [writable]  Recorded last-holder wallet (escrow + PDA-rent recipient)
 */
export const ACCOUNTS_NFT_RECONCILE: AccountMeta[] = [
  "w", "r", "w", "r", "r", "r", "w",
];

Exactly seven accounts, with the NFT mint marked read-only. #183 as it stands requires nine and requires the mint writable, so it breaks this on both counts.

What I'd suggest

The argument for required still holds and I don't think it should be abandoned: Reconcile is permissionless, irreversible and one-shot, so an optional form can be permanently defeated by whoever calls first — including a griefer paying a single fee to destroy the last holder's rent. That property is what makes an opt-in the wrong shape here, and it is unaffected by the existence of a caller.

What changes is that this is now a coordinated release rather than a free ABI break. Three options, in the order I'd rank them:

1. Ship both together. Land fix(nft): ReconcileBurnedNft reclaims the mint and ExtraAccountMetaList rent (#182) #183 and a matching percolator-sdk change — extend ACCOUNTS_NFT_RECONCILE to nine, flip account 1 to "w", and update the doc block. The SDK is first-party and in the same org, so lockstep is realistic. I'm happy to prepare that PR if useful.
2. Optional, with the arity hole closed. Accept 7 or 9 and hard-error on 8, so a malformed call fails loudly rather than silently destroying the rent. Weaker, because the 7-account path still wins any race, but it is non-breaking.
3. Defer fix(nft): ReconcileBurnedNft reclaims the mint and ExtraAccountMetaList rent (#182) #183 until the SDK change is scheduled.

I would not merge #183 alone. Note also that any deployed keeper or bot built against the published SDK carries the seven-account list, so the blast radius of merging in isolation is wider than the repo suggests.

One more thing found while looking

percolator-sdk/src/abi/nft.ts's module header documents the PositionNft PDA seeds as ["position_nft", portfolio_account, asset_index_u16_LE]. The code is correct — derivePositionNftPda uses market_id_u64_LE and cites #108 — so this is a stale comment rather than a bug, but it describes the pre-#108 scheme that #108 was filed to remove, and it sits at the top of the file a reader consults first.

[0x-SquidSol]

The SDK companion is ready, and the safe merge order is SDK-first

Following up on my note that this should not merge alone. The companion is up, and checking the deployed handler settled the ordering — in the opposite direction to what I assumed.

dcccrypto/percolator-sdk#378 mirrors this change (ACCOUNTS_NFT_RECONCILE 7 → 9, mint at index 1 writable). It is stacked on dcccrypto/percolator-sdk#377, an unrelated live bug found in the same file — ACCOUNTS_NFT_BURN and ACCOUNTS_NFT_EMERGENCY_BURN mark the holder [signer] rather than [signer, writable], so every burn built from those templates is rejected by require_writable_rent_recipient today.

Merge the SDK first, then this, then release. The reason it is safe in that order and not the other:

• A nine-account call against the currently deployed program is harmless. process_reconcile_burned_nft pulls seven accounts off an iterator, there is no accounts.len() check anywhere in processor.rs, and NotEnoughAccountKeys is the too-few error — so indices 7 and 8 are simply never read. The writable flag on index 1 is inert too: the deployed reconcile path checks writability only on nft_pda, portfolio and last_holder_ai (processor.rs:1243) and never writes the mint.
• A seven-account call against this PR's program fails hard, because the two new next_account_info calls are required by design.

So the SDK change is forward-compatible and this one is not backward-compatible, which fixes the order. The npm publish is the real gate rather than either merge — the SDK's CI publishes only on a release:-prefixed commit, so that commit is what should wait until this is deployed to devnet.

Nothing is urgent: seven repos depend on the SDK, all pinned by exact version or git SHA rather than a floating range, and none of them calls Reconcile at all.