feat(nft)!: ACCOUNTS_NFT_RECONCILE 7 -> 9 accounts (percolator-nft#182) — stacked on #377 - #378
Conversation
`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 {
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 instruction 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.
It went unnoticed because nothing in this repo consumes the templates — there is
no in-repo call site of buildNftAccountMetas or of any ACCOUNTS_NFT_* array, so
only external callers ever exercised them. The existing drift tests could not
have caught it either: they assert the shorthand string codes, and the defect is
only visible once those become {isSigner, isWritable} booleans.
Add account-list tests that round-trip through the real builder and assert those
booleans. That is the assertion that makes this class visible, and it also covers
the historical wrong-builder bug documented above buildNftAccountMetas, where
every flag silently became `undefined`.
Closes dcccrypto#376
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mirrors dcccrypto/percolator-nft#183, which gives ReconcileBurnedNft the rent reclamation the two burn paths have had since dcccrypto#102: extra_metas (writable) at index 7 and the Token-2022 program at index 8. The NFT mint at index 1 becomes writable because the program now closes it. Reconcile previously abandoned the NFT mint and the ExtraAccountMetaList PDA — 7,676,880 lamports per NFT, unrecoverable, because it closes the PositionNft PDA and every path that could later reclaim those two requires it to still be live. Ahead of chain, but forward-compatible, so this can ship before the program rather than after it. The deployed handlers pull seven accounts off an iterator, have no `accounts.len()` check anywhere in processor.rs, and never check `nft_mint.is_writable` in the reconcile path — so the two extra metas are unread and a nine-account call behaves identically on the deployed programs. The reverse order is the one that breaks: deploying dcccrypto#183 while the SDK still says seven fails every Reconcile with NotEnoughAccountKeys. Marked `!` because `buildNftAccountMetas` hard-throws on a count mismatch, so a caller passing seven keys now gets `account count mismatch: expected 9, got 7` at the call site. That is the intended failure — loud and local. Also corrects two module-header defects found alongside: the instruction list omitted tags 6 and 7, and the PositionNft PDA seed was documented as `asset_index_u16_LE`, the pre-dcccrypto#108 scheme dcccrypto#108 existed to remove. The code was always correct. Refs dcccrypto/percolator-nft#182 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 49 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
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. Comment |
Bytes [167..199] were documented as `_reserved`. percolator-nft#138 claimed them for `last_holder: [u8; 32]`, the final field of PositionNftV16 and the one the transfer hook rewrites on every transfer. It matters here because ReconcileBurnedNft reads it as the sole authorisation: the program releases the escrowed portfolio and all rent to whichever account matches, and refuses any other. It is account 6 of ACCOUNTS_NFT_RECONCILE and cannot be derived — only read from the PositionNft account. So without this the SDK shipped the Reconcile account template while giving a caller no way to obtain the one key in it that is not a PDA or a program id. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Added
|
1 participant
Companion to dcccrypto/percolator-nft#183 (issue: dcccrypto/percolator-nft#182).
Stacked on #377 — review that one first; this branch contains its commit.
What changes
ACCOUNTS_NFT_RECONCILEgoes from 7 to 9 accounts:ExtraAccountMetaListPDA, writable (closed)percolator-nft#182: Reconcile abandoned the NFT mint and the ExtraAccountMetaList PDA — 7,676,880 lamports per NFT, unrecoverable, because it closes the PositionNft PDA and every path that could later reclaim those two requires it to still be live. The program-side accounts are required rather than optional: Reconcile is permissionless, irreversible and runs at most once, so an opt-in could be defeated permanently by whoever called first.
Sequencing — this can ship before the program, and should
I initially assumed the reverse and want to correct that explicitly, because it determines the merge order.
A nine-account call is safe against the currently deployed programs. Their
process_reconcile_burned_nftpulls seven accounts off an iterator; there is noaccounts.len()check anywhere inprocessor.rs, andNotEnoughAccountKeysis the too-few error, so surplus accounts cannot trigger it — indices 7 and 8 are simply never read. The writable flag on index 1 is equally inert: the deployed reconcile path checks writability only onnft_pda,portfolioandlast_holder_ai(processor.rs:1243), never on the mint, and never writes it. So this behaves identically onFqhKJT9gtScjrmfUuRMjeg7cXNpif1fqsy5Jh65tJmTS(mainnet) andCNGBPZRALk9Xu8BdgWNyrLJ7daQ9eJYFf1GnEEC7YCU3(devnet).The reverse order is what breaks. Deploy percolator-nft#183 while the SDK still says seven, and every Reconcile fails with
NotEnoughAccountKeys— the program's two newnext_account_infocalls are explicitly required.So SDK-first is strictly safer, and the gate is not the merge:
.github/workflows/ci.yml:26-28publishes only on arelease:-prefixed commit onmain. Merging an ordinary commit runs lint/build/test and publishes nothing.release:commit until percolator-nft#183 is at least devnet-deployed. Until then the docstring promises rent reclamation the chain does not yet perform — which costs nothing, since that rent is already lost on every Reconcile today.I deliberately did not add a legacy 7-account export. It would solve a compatibility problem that does not exist, become a permanent trap once #183 deploys, and — worse — hand every stale client and griefer a ready-made short form to defeat the reclamation, which is precisely the opt-in the program PR made impossible.
Breaking surface
Marked
!becausebuildNftAccountMetashard-throws on a count mismatch: a caller passing seven keys now getsaccount count mismatch: expected 9, got 7, loudly and at the call site.deriveExtraAccountMetasalready exists to supply key 7.Blast radius today is nil: seven repos depend on this package, all by exact version or git SHA rather than a floating
^5, and grepping their sources forreconcile/encodeNftReconcile/ACCOUNTS_NFT_RECONCILEreturns zero hits. No consumer calls this instruction.Also included
dist/rebuilt. It is committed deliberately (.gitignore:20— "so that GitHub git-dep consumers get pre-built files") and there is noprepare/prepackscript, sopercolator-keeper'sgithub:dependency installs the committed tree. Without a rebuild the next SHA re-pin would silently ship the 7-entry array.[5.0.0].package.jsonwas bumped to 5.0.0 by3704dfdwith no changelog section and appears never released; this folds in, following the4.0.0precedent for an ahead-of-chain release including its "NOT YET DEPLOYED" banner.asset_index_u16_LE— the pre-fix(critical): comprehensive security hardening - buffer bounds, price validation, fee split #108 scheme that fix(critical): comprehensive security hardening - buffer bounds, price validation, fee split #108 existed to remove. The code was always correct.Verification
npx tsc --noEmitclean; full suite 1013 passing across 29 files. New drift assertions round-trip the template throughbuildNftAccountMetasand check the{isSigner, isWritable}booleans the runtime actually sees, including that Reconcile remains permissionless (no account may be a signer).🤖 Generated with Claude Code