get approved existence override for consecutive

[ozgunozerk]

Fixes #934

PR Checklist

• Tests
• Documentation

Summary by CodeRabbit

• Bug Fixes
  • Approval lookups now correctly distinguish existing tokens from nonexistent or unminted tokens.
  • Transfers involving nonexistent or unminted tokens now return the appropriate token-not-found error, including when initiated by an unapproved spender.
  • Consecutive-token collections now correctly recognize token-specific approvals and operator approvals during transfers.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Essentials
• Run ID: 6223c4f7-b3db-4e6c-b1f1-31260c54a851

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Essentials
• Run ID: 507b59fa-1afd-4b78-939b-2206e7d2f234

📥 Commits

Reviewing files that changed from the base of the PR and between 6b2f034 and 00da419.

📒 Files selected for processing (6)

• packages/tokens/src/non_fungible/extensions/combinations/storage.rs
• packages/tokens/src/non_fungible/extensions/consecutive/storage.rs
• packages/tokens/src/non_fungible/extensions/consecutive/test.rs
• packages/tokens/src/non_fungible/overrides.rs
• packages/tokens/src/non_fungible/storage.rs
• packages/tokens/src/non_fungible/test.rs

💤 Files with no reviewable changes (1)

• packages/tokens/src/non_fungible/overrides.rs

Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

Walkthrough

Base and Consecutive approval lookups now check whether a token exists before returning its approval. Consecutive transfer and burn authorization use a Consecutive-specific spender check. Regression tests cover transfer attempts for nonexistent tokens.

Changes

NFT approval checks

Layer / File(s)	Summary
Base approval lookup packages/tokens/src/non_fungible/storage.rs, packages/tokens/src/non_fungible/overrides.rs, packages/tokens/src/non_fungible/test.rs	Base::get_approved checks token existence, while get_approval_entry reads the stored approval without that check. The default override delegates to the Base lookup. A regression test checks the error for a transfer of a nonexistent token by a non-owner.
Consecutive approval and spender checks packages/tokens/src/non_fungible/extensions/consecutive/storage.rs, packages/tokens/src/non_fungible/extensions/combinations/storage.rs, packages/tokens/src/non_fungible/extensions/consecutive/test.rs	Consecutive::get_approved checks token existence using the Consecutive token model and reads the approval entry. Consecutive transfer and burn operations use Consecutive::check_spender_approval. ConsecutiveVotes delegates approval lookup to Consecutive. A regression test checks the error for transferring a token ID outside the minted range.

Priority: ⬇️ Low

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

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 00da4

The approval changes are mergeable after normal checks; no outstanding failure was established for the supported token lifecycles.

Pre-merge checks · 2 · 3

Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Description check		The description links issue #934 and marks tests and documentation complete, but it does not describe the implemented changes or provide context for the fix.	Add a concise summary of the changes, including the existence checks for Base and Consecutive approvals, the approval lookup behavior, and the regression tests added.
Linked Issues check		Issue #934 requires get_approved to raise NonExistentToken and to perform the existence check through the contract type's Self::owner_of. The PR adds the error path for Base and Consecutive,…	Route the contract override existence check through Self::owner_of, then read the approval entry without a second base ownership check. In the Consecutive override, call Consecutive::owner_of before reading the approval entry. Add dir…
Docstring Coverage		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 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks (2 passed)

Check name	Status	Explanation
Title check		The title clearly identifies the main change: adding an existence-aware approval override for Consecutive tokens.
Out of Scope Changes check		The added Base::get_approval_entry helper and the Consecutive approval and spender-check overrides support issue #934. Updating burn_from and transfer_from prevents the new base existence chec…

Full details: Linked Issues check

Explanation

Issue #934 requires get_approved to raise NonExistentToken and to perform the existence check through the contract type's Self::owner_of. The PR adds the error path for Base and Consecutive, and it preserves batch-token approval lookup. However, Base::get_approved calls Base::owner_of, the default ContractOverrides::get_approved delegates to it, and Consecutive::get_approved duplicates the burned/range checks instead of calling Consecutive::owner_of. The added regression test covers transfer_from, not the required direct get_approved cases.

Resolution

Route the contract override existence check through Self::owner_of, then read the approval entry without a second base ownership check. In the Consecutive override, call Consecutive::owner_of before reading the approval entry. Add direct tests for never-minted and burned IDs, including a live batch-minted ID.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{I’m a rabbit with a ledger to tend,
I check each token from start to end.
A live one may show its approval,
A missing one gets no false retrieval.
I hop through the checks, then nibble clover,
And stamp each test: “That’s error two hundred!”}

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[brozorec]

Review — stellar-tokens

• packages/tokens/src/non_fungible/overrides.rs:67 — the default get_approved now checks that the token exists through Base::owner_of (called inside Base::get_approved) instead of Self::owner_of. #934, which this PR closes, requires that check to go through the contract type's own owner_of. RoyaltySupport in the same file already does it that way. Now a contract type that overrides owner_of but not get_approved still compiles, and get_approved fails with #200 on live tokens. Consecutive and ConsecutiveVotes work only because each got a hand-written override. Make the default let _ = Self::owner_of(e, token_id); Base::get_approval_entry(e, token_id), and keep Consecutive::get_approved as the cheaper path.
• packages/tokens/src/non_fungible/extensions/combinations/storage.rs:114 — no test reaches the new ConsecutiveVotes::get_approved override. The consecutive_votes_* tests only approve and move last_id, which has its own Owner entry, so Base::owner_of succeeds there anyway. With the override deleted, all 125 NFT tests still pass, but get_approved on id 0 of a three-token ConsecutiveVotes batch panics with #200 (reproduced). Add a test that calls get_approved, and an approved transfer_from, on an id other than the last one in a ConsecutiveVotes batch.
• PR description — the body has only Fixes #934 and the CodeRabbit summary, but the template asks for a description of the changes. The PR changes an error code for existing callers: on every contract type, transfer_from and burn_from by a non-owner on a token that doesn't exist now fail with #200 instead of #202. State that, and explain why the Self::owner_of default from #935 was replaced. The CodeRabbit line saying consecutive collections "now correctly recognize" approvals during transfers is misleading. They already did on main. The new Consecutive::check_spender_approval only keeps them working now that Base::get_approved checks existence.
• packages/tokens/src/non_fungible/extensions/consecutive/storage.rs:201 — this doc comment was copied from Base::check_spender_approval but dropped its caveat: the function checks neither who owns token_id nor authorization. As written, it reads as a complete check. In fact, spender == owner passes for whatever owner is passed in, so a caller that doesn't also run Consecutive::update lets any account through. Restore the "Low-level function … without checking ownership of token_id and without handling authorization" wording.
• packages/tokens/src/non_fungible/test.rs:478 — the comment records the error code from before this PR (#202), which means nothing once the PR is merged. Remove the history and keep only the note that spender is neither owner nor operator.