Skip to content

docs(fee-abstraction): clarify expiration_ledger in collect_fee's Lazy branch - #893

Open
Eras256 wants to merge 1 commit into
OpenZeppelin:mainfrom
Eras256:docs/fee-abstraction-clarify-expiration-ledger
Open

Eras256 wants to merge 1 commit into
OpenZeppelin:mainfrom
Eras256:docs/fee-abstraction-clarify-expiration-ledger

Conversation

@Eras256

@Eras256 Eras256 commented Sep 14, 2026 •

Copy link
Copy Markdown

Summary

collect_fee and collect_fee_and_invoke document expiration_ledger
with identical prose ("the ledger sequence at which the approval
expires"). That's accurate for collect_fee_and_invoke, where the value
is threaded into user_args_for_auth as a freshness bound on the signed
call itself, but it reads as "the allowance's expiration" when
collect_fee is read (or called) in isolation, since it's the only
place that phrasing appears for a function with no require_auth of
its own.

That ambiguity produced a real filed-and-withdrawn finding, #840 (fix
attempt at #844): a reproduction called collect_fee directly with a
genuinely non-expired 100-unit allowance and still hit
InvalidExpirationLedger, read at the time as a bug. @brozorec's
closing explanation on #840 was correct and is not being reopened here:
in the Lazy branch, once an existing allowance already covers
max_fee_amount, expiration_ledger is validated via
validate_expiration_ledger as a freshness bound on the call, wholly
independent of the token's own allowance expiry. This PR is the doc
clarification promised in that thread's closing comment, so the next
reader doesn't hit the same misreading.

Changes

Three lines added to collect_fee's own # Arguments doc comment,
nothing else touched.

Test plan

  • Doc-only change, no behavior touched.
  • cargo doc -p stellar-fee-abstraction --no-deps builds clean locally.

Disclosure: drafted with AI assistance under my direction and reviewed
by hand.

🤖 Generated with Claude Code

…y branch

collect_fee and collect_fee_and_invoke document expiration_ledger with
identical prose ("the ledger sequence at which the approval expires"),
which reads correctly for the latter but is misleading for the former
when called on its own: in the Lazy branch, once an existing allowance
already covers max_fee_amount, the value is validated as a freshness
bound on the call itself (see validate_expiration_ledger), never read
from the token's actual on-chain allowance expiry. This ambiguity led
to a filed-and-withdrawn bug report (OpenZeppelin#840/OpenZeppelin#844); this note is the
follow-up promised in that thread's closing comment.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 3e3e4afb-6968-45ea-8b4e-aad459701e83

📥 Commits

Reviewing files that changed from the base of the PR and between f11f8c0 and 6966c68.

📒 Files selected for processing (1)
  • packages/fee-abstraction/src/storage.rs

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


Walkthrough

The collect_fee documentation now clarifies how expiration_ledger behaves in the Lazy approval branch. No functional code changes were made.

Changes

Fee collection documentation

Layer / File(s) Summary
Expiration ledger clarification
packages/fee-abstraction/src/storage.rs
The expiration_ledger documentation now states that it provides a freshness bound on the call when an existing allowance covers max_fee_amount. It is not read from the token’s on-chain allowance expiry.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Other

Suggested reviewers: brozorec

Merge Risk: ⚪ Minimal · up to 6966c

This documentation clarification does not alter fee collection behavior and is safe to merge.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
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 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely identifies the documentation change to expiration_ledger in collect_fee’s Lazy branch.
Description check ✅ Passed The description is detailed and relevant. It explains the ambiguity, scope, implementation change, and test plan. The issue placeholder remains unfilled, and the template checklist is omitted, but the…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

A rabbit reads each line,
The patch grows clear beneath the moon,
Small changes hop in place,
Tests guard the garden path,
Reviews bloom before the dawn.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant