fix(nft): make GetPositionValue actually fail-CLOSED on stale state (#180)

[0x-SquidSol]

Closes #180.

Problem

GetPositionValue documented itself as fail-CLOSED on stale state but read no staleness field. no_active_leg and slot_reuse returned Err; nothing checked leg.stale, leg.b_stale, stale_state, b_stale_state, liquidation_lock, resolved_payout_receipt.present or close_progress — none were emitted either, and the success path emitted no status line, so a consumer had neither an error to catch nor a field to inspect.

The program contradicted itself on the same portfolio: the transfer hook refused to move an NFT that this instruction reported to marketplaces and lending protocols as a healthy active leg with full economics. Before this change the hook's gate and the valuation disagreed on exactly seven portfolio states.

Fix

Route the decision through leg_transfer_gate — documented at slab_types_v16.rs:559-561 as the "single consolidated gate" for the hook and the wrapper's B-3 — rather than reimplementing one of its five checks. Each verdict emits its own status=<reason> and returns the matching error. no_active_leg keeps LegNotActive (22), so existing consumers of that path are unaffected.

Ordering follows the hook (verify_bound_leg then transfer_gate_check): the slot-reuse check runs before the gate verdict is applied. This matters — a portfolio that is both slot-reused and transiently stale must report the slot reuse, because MarketIdMismatch is terminal and routes the holder to EmergencyBurn, while b_stale is transient and merely says "retry later". Reporting the transient one would send a holder into an indefinite wait. slot_reuse_outranks_a_transient_stale_flag pins this.

Blocked positions still report their economics — under a separate POSITION_BLOCKED_V16: prefix. This is the part I'd most like scrutinised, so the reasoning in full:

• Emitting them under the existing POSITION_VALUE_V16: prefix would let a parser that ignores err keep reading a real number off a liquidation-locked position, which is precisely the defect being closed. A flag next to the data does not help a reader who was never reading flags.
• Withholding them entirely is worse than it sounds. The engine sets leg.b_stale on any multi-chunk backing settlement (percolator/src/v16.rs:10221), so that state is ordinary crank-paced operation, not an exception — going dark would blank pricing across a market every time it takes losses. And resolved is terminal and carries the position's final settled value, which is exactly what a lender needs to close out.
• The wrapper records the same principle for this same gate: UnwrapEscrowedPortfolio is "deliberately NOT gated on active-leg / resolved_payout_receipt / liquidation_lock / stale / close-progress" because "gating on those would strand funds" (v16_program.rs:16548).

The split gives both: a parser scanning the well-known prefix fails closed by construction, and distressed pricing is available only to a caller who opts in deliberately. slot_reuse_detected is the sole case emitting no economics — its fields would describe a different position instance entirely.

Verification

• 51 pre-existing tests unchanged and passing, plus 3 new unit tests on the extracted gate_status mapping and 6 integration tests.
• cargo clippy --all-targets -- -D warnings clean; cargo build-sbf --tools-version v1.52 succeeds.

On testing the log vocabulary. A reviewer showed my first version's status strings were mutation-provably untested — replacing all four with "WRONG", or deleting the emissions outright, left every test green. msg! output cannot be captured off-chain on this pin: solana-msg's non-BPF sol_log is a bare println! that bypasses program_stubs, so SyscallStubs never sees it. I therefore extracted the mapping as a pure pub(crate) fn gate_status and unit-tested it exhaustively — the vocabulary is pinned, every string is asserted distinct, and only Transferable is allowed a non-error. That closes the gap the mutation exposed; a rename or copy-paste now fails a test.

Also included

• status=ok on the success path. Previously health could only be inferred from the absence of a key — indistinguishable from a truncated log or a parser bug.
• slot_reuse_detected split into one key per line; it previously packed market_id_at_mint= and current_market_id= onto the status line, so status parsed as a compound value.
• The doc claim is corrected in all three places that enumerated an incomplete set: valuation.rs, instruction.rs (the tag-3 doc comment, which is what an integrator reads first), and README.md.
• A README log-contract section. The status= vocabulary was documented nowhere, and a search confirms there is no first-party SDK or TS client — the README plus the log prefix is the entire integration surface. The section lists every status with its meaning, recommended action and error code, plus the prefix split, the simulateTransaction logs-survive-err behaviour and its exceptions, the 10 kB log cap, and the batching caveat below.

Judgement calls left to you

Three reviewers disagreed on the consumer-facing shape; these are genuinely your call, not mine to assume:

• Batching. A failing instruction aborts the transaction, so packing many GetPositionValue calls into one simulation means one blocked position suppresses every call after it. The distinct-prefix design does not fix that. A backward-compatible alternative exists: tag 3 currently rejects trailing bytes, so an optional mode byte could offer an explicitly-requested advisory read that returns Ok with a mandatory status=. That is an ABI extension, so I did not take it unilaterally.
• Error codes. The four blocked states share TransferBlocked (24). Two reviewers wanted distinct codes (the enum is documented append-only, with room); one argued against, since diverging from transfer_gate_check's mapping would reintroduce the two-implementations problem this change removes. I kept them shared and put the distinction in the status string, which is this instruction's real API.
• Status naming. leg_stale and portfolio_locked_or_stale are mechanical LegTransferGate variant names. A reviewer argued settlement_pending and a split into liquidation_locked / account_settlement_pending would read better to an outside integrator. Splitting the latter means reading the underlying flags directly rather than through the gate, which is the coupling this change removes — and liquidation_lock is never set true anywhere in the engine today, so the split has little practical value yet.
• Release. No CHANGELOG exists and the crate is still 0.1.0. resolved and close_in_progress are genuinely new fail-closed conditions rather than the code catching up to documentation, so a version bump and a devnet-first rollout look warranted.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Position valuation now fails safely for all non-transferable states, including stale, locked, resolved, and closing positions.
  • Blocked-position results now clearly separate valuation details from status information.
  • Slot-reuse cases report consistent error information.

• Documentation

  • Expanded GetPositionValue documentation with status values, error behavior, simulation details, batching caveats, and log limitations.

• Tests

  • Added coverage confirming blocked states consistently prevent valuation while valid positions continue to work.

[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: b23474e1-9db3-4186-b255-c8c4ca728226

📥 Commits

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

📒 Files selected for processing (4)

• README.md
• src/instruction.rs
• src/valuation.rs
• tests/poc_valuation_ignores_staleness.rs

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

---

📝 Walkthrough

Walkthrough

GetPositionValue now applies the shared transfer gate, fails closed for all blocked portfolio states, emits distinct status and economics prefixes, preserves healthy valuation behavior, and adds unit and integration coverage.

Changes

Fail-closed valuation

Layer / File(s)	Summary
Transfer gate and log contract src/instruction.rs, src/valuation.rs, README.md	GetPositionValue now maps all transfer-gate states to explicit statuses and errors. Blocked economics use POSITION_BLOCKED_V16:, while healthy economics use POSITION_VALUE_V16:. Documentation describes the status and log contract.
Valuation behavior validation src/valuation.rs, tests/poc_valuation_ignores_staleness.rs	Unit and integration tests cover blocked portfolio states, successful valuation, error precedence, status vocabulary, and the relationship between valuation and the transfer gate.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 9e56c

The change makes valuation fail closed for blocked or stale positions while preserving read-only behavior and existing ownership checks. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: dcccrypto

Sequence Diagram(s)

sequenceDiagram
  participant GetPositionValue
  participant leg_transfer_gate
  participant Portfolio
  participant PositionLogs
  GetPositionValue->>Portfolio: read portfolio and NFT state
  GetPositionValue->>leg_transfer_gate: evaluate transferability
  leg_transfer_gate-->>GetPositionValue: return status and error
  GetPositionValue->>PositionLogs: emit blocked or healthy valuation fields

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 3 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the primary change: making GetPositionValue fail closed for stale state. It is related to the broader gate-alignment work, although it does not list every newly blocked con…
Linked Issues check	✅ Passed	The changes satisfy issue #180. GetPositionValue reuses leg_transfer_gate, fails closed for all listed blocked states, preserves no_active_leg and slot-reuse protections, maintains slot-reuse preceden…
Out of Scope Changes check	✅ Passed	The implementation, documentation, log-contract updates, helper extraction, and tests directly support issue #180 and the stated pull request objectives. No unrelated code changes are identified.

Full details: Title check

Explanation

The title clearly describes the primary change: making GetPositionValue fail closed for stale state. It is related to the broader gate-alignment work, although it does not list every newly blocked condition.

Full details: Linked Issues check

Explanation

The changes satisfy issue #180. GetPositionValue reuses leg_transfer_gate, fails closed for all listed blocked states, preserves no_active_leg and slot-reuse protections, maintains slot-reuse precedence, emits specific status reasons, and preserves successful valuation behavior. Documentation and tests support the required contract.

Full details: Docstring Coverage

Explanation

Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 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.}