fix(nft): the burn holder is the rent recipient — mark it writable (#376)

[0x-SquidSol]

Closes #376.

Problem

ACCOUNTS_NFT_BURN and ACCOUNTS_NFT_EMERGENCY_BURN marked account 0 — the NFT holder — as "s": signer but not writable. The program requires signer and writable, because the holder receives the rent from every account those instructions close: the ATA, the mint, the PositionNft PDA and the ExtraAccountMetaList.

percolator-nft rejects a read-only holder outright:

fn require_writable_rent_recipient(holder: &AccountInfo) -> ProgramResult {  // processor.rs:765
    if !holder.is_writable { return Err(ProgramError::InvalidAccountData); }
    Ok(())
}

called from BurnPositionNft (processor.rs:825) and EmergencyBurn (:1000). The program's own ABI table documents account 0 as [signer, writable] (instruction.rs:44 and :99).

So every burn built from these templates via buildNftAccountMetas went on the wire with isWritable: false and was rejected with InvalidAccountData. This is a live break against the deployed programs, not a latent one.

Why nothing caught it

Two reasons, and the second is the more useful one:

1. Nothing in this repo consumes the templates. There is no in-repo call site of buildNftAccountMetas, and no consumer of any ACCOUNTS_NFT_* array. Only external callers exercise them.
2. The existing drift tests assert the shorthand string codes. The defect is invisible at that level — it only appears once the codes become {isSigner, isWritable} booleans, which is the object that actually goes into the transaction.

That second point is the same failure mode the long comment above buildNftAccountMetas already records: the earlier bug there was passing these arrays to the wrong builder, silently yielding isSigner: undefined / isWritable: undefined, described as failing with "InvalidAccountData at ~2.4k CU, before any CPI". Same symptom, different cause — and the same blind spot in the tests.

Fix

"s" → "sw" in both templates, plus the doc lines above them.

Tests

New percolator-nft account-list ABI block in test/drift-check.test.ts, which already tracks this surface. The assertions round-trip each template through the real builder and check the booleans rather than the codes:

const flagsOf = (spec) =>
  buildNftAccountMetas(spec, Array.from({length: spec.length}, () => PublicKey.unique()))
    .map((m) => [m.isSigner, m.isWritable] as const);

Before this change they fail exactly where the bug is:

× BurnPositionNft: the holder is the rent recipient, so signer AND writable
    AssertionError: expected [ true, false ] to deeply equal [ true, true ]
× EmergencyBurn: same holder requirement (processor.rs:1000)
    AssertionError: expected [ true, false ] to deeply equal [ true, true ]

The block also covers MintPositionNft's two signers, extra_metas writability on both burn paths (#102), that Reconcile is permissionless (no account may be a signer), and that buildNftAccountMetas throws on a key-count mismatch rather than truncating.

npx tsc --noEmit clean; full suite 1013 passing across 29 files.

Scope

Deliberately narrow — this is a live break and should be able to merge on its own.

A second, separate change is coming for ACCOUNTS_NFT_RECONCILE, which must grow from 7 to 9 accounts to match dcccrypto/percolator-nft#183. That one is not mergeable yet: both the mainnet and devnet programs still expect seven accounts, so it has to wait until the program change is merged and deployed. Keeping them apart so this fix is not held behind that gate.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • NFT burn and emergency burn operations now correctly identify the NFT holder as a signer and writable account, supporting rent refunds.

• Tests

  • Added validation for NFT instruction account counts and signer/writable permissions.
  • Added coverage to ensure invalid account lists are rejected.

[coderabbitai]

Warning

Review limit reached

Next included review available in 56 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7e399e4d-e946-411d-9455-892d7ef1211d

📥 Commits

Reviewing files that changed from the base of the PR and between 796464a and 8b50f11.

⛔ Files ignored due to path filters (3)

• dist/abi/nft.d.ts is excluded by !**/dist/**
• dist/index.js is excluded by !**/dist/**
• dist/index.js.map is excluded by !**/dist/**, !**/*.map

📒 Files selected for processing (1)

• CHANGELOG.md

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: 418e693f-043d-4ea9-b632-c9375818c930

📥 Commits

Reviewing files that changed from the base of the PR and between 5b0aa77 and 796464a.

📒 Files selected for processing (2)

• src/abi/nft.ts
• test/drift-check.test.ts

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

---

📝 Walkthrough

Walkthrough

The NFT burn and emergency burn templates now mark the holder as signer and writable. Tests validate account counts, account permissions, permissionless reconciliation, and rejection of mismatched key counts.

Changes

NFT ABI validation

Layer / File(s)	Summary
Writable holder metadata src/abi/nft.ts	Burn and emergency burn account templates mark the NFT holder as signer and writable. Their documentation identifies the holder as the rent recipient.
Account metadata tests test/drift-check.test.ts	Tests validate NFT account counts, signer and writable flags, reconciliation metadata, and key-count mismatch errors.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 79646

The burn account metadata now correctly marks the rent recipient as both signer and writable, preventing the affected burn transactions from being rejected. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: dcccrypto

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 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 NFT burn holder writability fix and matches the primary source change.
Linked Issues check	✅ Passed	The pull request changes account 0 to signer and writable in both burn templates. The tests verify the resulting signer and writable flags, which satisfies issue #376.
Out of Scope Changes check	✅ Passed	The changes remain in scope. The pull request adds related ABI coverage, but it does not change the out-of-scope reconcile account template.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files.

✨ 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.}