fix(nft): M-1 docs + entry [6] read-only, keeping entry [5] writable and narrowing L-2 (supersedes #163) - #173
Merged
Merged
Conversation
…EmergencyBurn decode fallback M-1 (docs): remove false claim that LiteSVM integration tests are the runtime ground truth for the vendored portfolio layout. The const_assert! macros verify the mirror struct's internal consistency only; alignment with the live engine layout requires runtime validation. Corrected in cpi_v16.rs, slab_types_v16.rs, and README.md. L-1 (writable flags): flip PositionNft PDA (entry 5) and Portfolio (entry 6) from writable to read-only in the ExtraAccountMetaList for both MintPositionNft and RepairExtraMetas. Post-#105 the transfer hook is validation-only and never writes either account; the writable flags imposed unnecessary write-locks on portfolio accounts during transfers. RepairExtraMetas rationale updated to reflect the new correct flags. L-2 (#110B): EmergencyBurn no longer hard-reverts when a wrapper-owned portfolio is present but fails decode (e.g. future layout migration). Decode failure now skips the eligibility check and falls through to the unwrap CPI, which the wrapper handles on its own terms. The existing portfolio_gone path (#131) is unchanged.
…allback Follow-up to the M-1/L-1/L-2 commit. M-1 (doc corrections) is accurate and kept as-is. The other two needed work. L-1 — entry [5] must stay WRITABLE. The PR flipped both hook accounts to read-only on the stated basis that "#105: hook is validation-only". That was true when the PR was opened, but main has since gained #152/#153, which added a NEW write: process_transfer_hook does `nft_state.last_holder = new_owner` at transfer_hook.rs:542-545 on every genuine Token-2022 transfer. A read-only meta for the PositionNft PDA therefore makes every direct TransferChecked fail — and since RepairExtraMetas is PERMISSIONLESS, that is a free brick-any-NFT vector. Both sites are restored to writable (the repair path had the same flip, which is the dangerous one). Entry [6] (Portfolio) read-only is KEPT, argued separately as the review asked: verified against this build that the hook performs no invoke/invoke_signed at all and never mutably borrows `portfolio` — it only reads it to check the NFT PDA binding. The old "B-3 CPI mutates portfolio.owner" rationale moved to mint/burn in #105. Read-only is correct here and narrows the write lock. To stop the two entry tables drifting apart again, the flags are extracted into a single `EXTRA_META_ENTRY_FLAGS` table used by both the mint path and RepairExtraMetas, so divergence is now structurally impossible rather than a convention. Four tests pin the flags; the 34 pre-existing tests all passed with entry [5] read-only, so nothing in the repo would have caught this. Negative control: flipping entry 5 back to read-only fails `position_nft_pda_meta_entry_is_writable`. L-2 — narrowed. Skipping the eligibility gate on ANY PortfolioDecodeError falls through to UnwrapEscrowedPortfolio, which is not itself leg-gated, so it could emergency-burn a position never proven flat. The fallback now admits only BadVersion / BadAccountVersion / BadLayoutDiscriminator — the "future layout migration" case the PR justified it with, on an account already verified wrapper-owned. TooShort / BadMagic / BadKind / Cast / OwnerMismatch now propagate as errors; OwnerMismatch is a violated engine invariant and must never widen burn eligibility. Conflict resolution note: main's #110C provenance check is preserved on the Ok arm — the PR's version dropped it. cargo build clean; 48 lib tests pass (44 before, +4 new). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
…ta count
Second-pass review findings. All of these were pre-existing text that THIS change
turned into a lie — the failure mode the PR itself set out to fix.
1. ON-CHAIN LOG. RepairExtraMetas emitted
`msg!("... (portfolio now writable)")`. That was accurate on main; this PR
makes portfolio read-only, so the deployed program would log a false statement
on every permissionless repair. Now reports the actual shape
("nft_pda writable, portfolio read-only").
2. instruction.rs doc claimed "Post-#105 the hook is validation-only and no longer
CPIs; both PositionNft PDA and Portfolio are now read-only". The first half is
the exact reasoning that made L-1 dangerous — #152/#153 re-added a write, so
entry [5] is writable. Rewritten to state the current flags and WHY each is
what it is.
3. transfer_hook.rs:284 still labelled the portfolio account "(writable)".
4. README M-1 cited "#110B" for deferred runtime layout validation. Per the issue,
item B is the EmergencyBurn wedge (what L-2 addresses) and item H is the layout
cross-check. Corrected to #110H.
5. EXTRA_META_COUNT was re-declared as a literal `7` in BOTH call sites with
nothing tying it to EXTRA_META_ENTRY_FLAGS — the table that actually decides the
entries. Extracting the flags removed one drift risk and introduced another.
Both are now `EXTRA_META_ENTRY_FLAGS.len()`. Verified: shrinking the table to 6
entries is a compile error ("expected an array with a size of 6, found one with
a size of 7") rather than a silent mismatch.
cargo build clean; 48 lib tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #163 by @v1ktorrr0x. Their commit is preserved as-is; my corrections are a follow-up commit on top.
M-1 (doc corrections) is accurate and kept unchanged. The other two needed work.
L-1 — entry [5] must stay WRITABLE. As filed this would have bricked live transfers.
The PR flipped both hook accounts to read-only on the stated basis that "#105: hook is validation-only". That was true when the PR was opened, but
mainhas since gained #152/#153, which added a new write:process_transfer_hookdoesnft_state.last_holder = new_ownerattransfer_hook.rs:542-545on every genuine Token-2022 transfer. A read-only meta for the PositionNft PDA therefore makes every directTransferCheckedfail — and sinceRepairExtraMetasis permissionless, that is a free brick-any-NFT vector. Both sites are restored to writable (the repair path had the same flip, which is the dangerous one).Entry [6] (Portfolio) read-only is KEPT, argued separately as it should have been: verified against this build that the hook performs no
invoke/invoke_signedat all and never mutably borrowsportfolio— it only reads it to check the NFT PDA binding. The old "B-3 CPI mutates portfolio.owner" rationale moved to mint/burn in #105. Read-only is correct here and narrows the write lock.To stop the two entry tables drifting apart again, the flags are extracted into a single
EXTRA_META_ENTRY_FLAGStable used by both the mint path andRepairExtraMetas, so divergence is now structurally impossible rather than a convention. Four tests pin the flags; the 34 pre-existing tests all passed with entry [5] read-only, so nothing in the repo would have caught this. Negative control: flipping entry 5 back failsposition_nft_pda_meta_entry_is_writable.L-2 — narrowed. Skipping the eligibility gate on any
PortfolioDecodeErrorfalls through toUnwrapEscrowedPortfolio, which is not itself leg-gated, so it could emergency-burn a position never proven flat. The fallback now admits onlyBadVersion/BadAccountVersion/BadLayoutDiscriminator— the "future layout migration" case the PR justified it with, on an account already verified wrapper-owned.TooShort/BadMagic/BadKind/Cast/OwnerMismatchnow propagate as errors;OwnerMismatchis a violated engine invariant and must never widen burn eligibility.Conflict resolution note: main's #110C provenance check is preserved on the
Okarm — the PR's version dropped it.cargo buildclean; 48 lib tests pass (44 before, +4 new).