fee-abstraction: clear residual allowance after eager fee collection - #837
fee-abstraction: clear residual allowance after eager fee collection#837knQzx wants to merge 2 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughEager fee collection now refunds the unused portion of ChangesEager fee allowance refund
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/fee-abstraction/src/storage.rs`:
- Around line 211-215: Update the eager-approval cleanup in
collect_fee_and_invoke to clear the remaining allowance without transferring
tokens from the user or requiring additional user balance, using the
allowance-only authorization available in the eager authorization tree. Preserve
cleanup when fee_amount is below max_fee_amount, and add a regression test where
the user balance equals fee_amount that verifies the target call succeeds.
In `@packages/fee-abstraction/src/test.rs`:
- Around line 74-79: Extend the assertions after the event-count check in the
fee abstraction test to compare events.events().get(2) with the eager refund
Transfer payload, using user.clone() for both from and to and max_fee_amount -
fee_amount as the amount, serialized with to_xdr(&e, &token_address). Preserve
the existing zero-allowance assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8e9f087-5fbe-49de-905d-49bc4f562196
📒 Files selected for processing (2)
packages/fee-abstraction/src/storage.rspackages/fee-abstraction/src/test.rs
| if let FeeAbstractionApproval::Eager = approval { | ||
| let remaining = max_fee_amount - fee_amount; | ||
| if remaining > 0 { | ||
| token_client.transfer_from(&e.current_contract_address(), user, user, &remaining); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 12 \
'fn transfer_from|spend_allowance|spend_balance|receive_balance' \
packages || true
rg -n -C 12 \
'MockToken|mint|balance|collect_fee_with_eager_approval_overwrites_allowance' \
packages/fee-abstraction/src/test.rs || trueRepository: OpenZeppelin/stellar-contracts
Length of output: 50386
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- fee-abstraction candidates ---'
fd -i 'fee|test|storage' packages/fee-abstraction
printf '%s\n' '--- relevant symbols ---'
rg -n -C 10 \
'FeeAbstractionApproval|validate_fee_bounds|transfer_from|approve|eager|allowance|fee_amount|max_fee_amount' \
packages/fee-abstraction
printf '%s\n' '--- fungible balance update implementation ---'
sed -n '250,390p' packages/tokens/src/fungible/storage.rs
printf '%s\n' '--- fee-abstraction source outline ---'
ast-grep outline packages/fee-abstraction/src/storage.rsRepository: OpenZeppelin/stellar-contracts
Length of output: 50388
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- exact token transfer implementation ---'
sed -n '285,385p' packages/tokens/src/fungible/storage.rs
printf '%s\n' '--- exact fee tests and mock token setup ---'
sed -n '15,85p' packages/fee-abstraction/src/test.rs
printf '%s\n' '--- all eager call sites ---'
rg -n -C 8 'FeeAbstractionApproval::Eager|collect_fee_and_invoke' \
packages examplesRepository: OpenZeppelin/stellar-contracts
Length of output: 24839
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Base::update definition ---'
rg -n -A 90 -B 10 'pub fn update|fn update\(' packages/tokens/src/fungible/storage.rs
printf '%s\n' '--- balance storage helpers ---'
rg -n -A 45 -B 10 'pub fn balance|fn balance_data|InsufficientBalance|Base::update' \
packages/tokens/src/fungible/storage.rsRepository: OpenZeppelin/stellar-contracts
Length of output: 14358
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
def update(balance, from_amount, to_amount):
if balance < from_amount:
raise ValueError("InsufficientBalance")
balance -= from_amount
balance += to_amount
return balance
fee_amount = 20
max_fee_amount = 50
remaining = max_fee_amount - fee_amount
for initial_balance in (fee_amount, max_fee_amount):
after_fee = update(initial_balance, fee_amount, fee_amount)
try:
after_cleanup = update(after_fee, remaining, remaining)
except ValueError as exc:
print({
"initial_balance": initial_balance,
"after_fee": after_fee,
"cleanup": "fails",
"error": str(exc),
})
else:
print({
"initial_balance": initial_balance,
"after_fee": after_fee,
"after_self_transfer": after_cleanup,
})
PYRepository: OpenZeppelin/stellar-contracts
Length of output: 329
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
def transfer(balance, amount):
if balance < amount:
raise ValueError("InsufficientBalance")
return balance - amount
fee_amount = 20
max_fee_amount = 50
remaining = max_fee_amount - fee_amount
for initial_balance in (fee_amount, max_fee_amount):
after_fee = transfer(initial_balance, fee_amount)
try:
after_cleanup = transfer(after_fee, remaining)
except ValueError as exc:
print({
"initial_balance": initial_balance,
"after_fee": after_fee,
"cleanup": "fails",
"error": str(exc),
})
else:
print({
"initial_balance": initial_balance,
"after_fee": after_fee,
"cleanup": "succeeds",
"after_debit": after_cleanup,
"after_credit_to_same_user": after_cleanup + remaining,
})
PYRepository: OpenZeppelin/stellar-contracts
Length of output: 375
Avoid debiting the user to clear the eager allowance.
When fee_amount < max_fee_amount, the self-transfer still requires the user balance to cover max_fee_amount - fee_amount. If the user balance equals fee_amount, cleanup fails with InsufficientBalance, and collect_fee_and_invoke does not reach the target call. Use an allowance-only cleanup authorized in the eager authorization tree. Add a regression test for this balance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/fee-abstraction/src/storage.rs` around lines 211 - 215, Update the
eager-approval cleanup in collect_fee_and_invoke to clear the remaining
allowance without transferring tokens from the user or requiring additional user
balance, using the allowance-only authorization available in the eager
authorization tree. Preserve cleanup when fee_amount is below max_fee_amount,
and add a regression test where the user balance equals fee_amount that verifies
the target call succeeds.
There was a problem hiding this comment.
Hey @knQzx the coderabbit finding seems a valid one: if the user's balance is below max_fee_amount - fee_amount the whole invocation would fail. One option is using try_transfer_from but then we might have to deal with residuals that are smaller than remaining. Another option is checking the user's balance and transferring only min(balance, remaining).
| // approval, fee transfer, residual refund and collect fee | ||
| assert_eq!(events.events().len(), 4); | ||
|
|
||
| // the unspent allowance is consumed back to the user in eager mode | ||
| let allowance = token_client.allowance(&user, &contract_address); | ||
| assert_eq!(allowance, 30); | ||
| assert_eq!(allowance, 0); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files 'packages/fee-abstraction/**' | sed -n '1,120p'
printf '%s\n' '--- test structure ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline packages/fee-abstraction/src/test.rs --match '$_' --view compact 2>/dev/null | sed -n '1,160p' || true
fi
printf '%s\n' '--- relevant test lines ---'
sed -n '1,130p' packages/fee-abstraction/src/test.rs
printf '%s\n' '--- event definitions and uses ---'
rg -n -C 4 'Transfer|contractevent|events\(\)|to_xdr|fee_amount|max_fee_amount|allowance' packages/fee-abstraction packages --glob '*.rs' | sed -n '1,260p'Repository: OpenZeppelin/stellar-contracts
Length of output: 23172
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- fee-abstraction imports and declarations ---'
sed -n '1,120p' packages/fee-abstraction/src/lib.rs
sed -n '1,90p' packages/fee-abstraction/src/storage.rs
cat packages/fee-abstraction/Cargo.toml
printf '%s\n' '--- all typed transfer-event references ---'
rg -n -C 5 'fungible::.*Transfer|Transfer \{|TransferEvent|FeeCollected|contractevent' packages --glob '*.rs' | sed -n '1,300p'
printf '%s\n' '--- workspace dependency declarations ---'
rg -n -C 3 'stellar_tokens|stellar-tokens|soroban-sdk' Cargo.toml packages --glob 'Cargo.toml' | sed -n '1,220p'
printf '%s\n' '--- relevant test section ---'
sed -n '1,110p' packages/fee-abstraction/src/test.rsRepository: OpenZeppelin/stellar-contracts
Length of output: 37678
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- token package structure ---'
git ls-files packages/tokens/src | sed -n '1,160p'
printf '%s\n' '--- fungible module outline and event definitions ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline packages/tokens/src/fungible.rs --match '$_' --view compact 2>/dev/null | sed -n '1,220p' || true
fi
rg -n -C 8 'struct Transfer|enum Transfer|emit_transfer|transfer_from|contractevent' packages/tokens/src --glob '*.rs' | sed -n '1,320p'
printf '%s\n' '--- existing typed event assertions in tests ---'
rg -n -C 8 'to_xdr\(|events\(\)\.get|events\(\)\.first|events\(\)\.events\(\)' packages --glob 'test.rs' --glob '*.rs' | sed -n '1,320p'Repository: OpenZeppelin/stellar-contracts
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- fungible event declarations ---'
rg -n -C 10 'struct Transfer|emit_transfer|contractevent' packages/tokens/src/fungible/mod.rs packages/tokens/src/fungible/storage.rs packages/tokens/src/fungible/overrides.rs
printf '%s\n' '--- fungible public exports ---'
sed -n '1,180p' packages/tokens/src/fungible/mod.rs
printf '%s\n' '--- nearby fungible event implementation ---'
python3 - <<'PY'
from pathlib import Path
p = Path("packages/tokens/src/fungible/mod.rs")
lines = p.read_text().splitlines()
for i, line in enumerate(lines):
if "struct Transfer" in line or "emit_transfer" in line or "contractevent" in line:
lo, hi = max(0, i - 8), min(len(lines), i + 35)
print(f"--- lines {lo+1}-{hi} ---")
for n in range(lo, hi):
print(f"{n+1}:{lines[n]}")
PY
printf '%s\n' '--- typed event assertion conventions around indexed events ---'
rg -l 'events\(\)\.get|events\(\)\.first|to_xdr\(' packages --glob '*.rs' |
while IFS= read -r f; do
case "$f" in
packages/tokens/*|packages/fee-abstraction/*)
rg -n -C 4 'events\(\)\.(get|first)|to_xdr\(' "$f"
;;
esac
done | sed -n '1,260p'Repository: OpenZeppelin/stellar-contracts
Length of output: 39261
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
test = Path("packages/fee-abstraction/src/test.rs").read_text()
storage = Path("packages/fee-abstraction/src/storage.rs").read_text()
fungible = Path("packages/tokens/src/fungible/mod.rs").read_text()
# Confirm the test currently checks only the event count and not typed payloads.
assert "assert_eq!(events.events().len(), 4);" in test
assert "to_xdr" not in test
# Confirm the eager path emits approval, fee transfer, refund, then FeeCollected.
body = storage[storage.index("pub fn collect_fee("):storage.index("// ################## FEE TOKEN ALLOWLIST")]
sequence = [
"token_client.approve(",
"token_client.transfer_from(&e.current_contract_address(), user, fee_recipient, &fee_amount);",
"token_client.transfer_from(&e.current_contract_address(), user, user, &remaining);",
"emit_fee_collected(e, user, fee_recipient, fee_token, fee_amount);",
]
positions = [body.index(item) for item in sequence]
assert positions == sorted(positions), positions
# Confirm the refund is a non-muxed fungible Transfer event with the required fields.
assert re.search(
r"Transfer\s*\{.*?from: from\.clone\(\),.*?to: to\.clone\(\),.*?amount",
fungible,
re.S,
)
assert "Transfer { from: from.clone(), to: to.clone(), amount }.publish(e);" in fungible
assert "let remaining = max_fee_amount - fee_amount;" in body
print("The count-only assertion omits the eager refund payload.")
print("The refund is event index 2 and is serialized by stellar_tokens::fungible::Transfer.")
print("Expected refund payload: from=user, to=user, amount=max_fee_amount-fee_amount.")
PYRepository: OpenZeppelin/stellar-contracts
Length of output: 389
Assert the eager refund event payload.
Compare events.events().get(2) with stellar_tokens::fungible::Transfer { from: user.clone(), to: user.clone(), amount: max_fee_amount - fee_amount }.to_xdr(&e, &token_address). Keep the zero-allowance assertion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/fee-abstraction/src/test.rs` around lines 74 - 79, Extend the
assertions after the event-count check in the fee abstraction test to compare
events.events().get(2) with the eager refund Transfer payload, using
user.clone() for both from and to and max_fee_amount - fee_amount as the amount,
serialized with to_xdr(&e, &token_address). Preserve the existing zero-allowance
assertion.
Source: Coding guidelines
The self transfer moves the residual out of and back into the same balance, so the token rejects it when the balance is short and the whole invocation fails. Consume the affordable part instead.
|
good catch, went with the failure is real: the self transfer still goes through i also looked at just calling what is left over is an allowance the user cannot spend anyway at the current balance, which felt better than reverting. two tests added: balance below the residual (5 of 30 consumed, 25 allowance left) and balance exactly the fee (no refund transfer at all). |
2 participants
in
Eagermodecollect_feeapprovesmax_fee_amountbut only spendsfee_amount, so the unspent allowance from the user to the forwarder stays alive after collection. since the forwarder goes on to invoke an arbitrary target, that leftover allowance can be spent by a later callthis consumes the unspent remainder back to the user with a net-zero transfer right after the fee is taken, leaving no residual allowance.
Lazymode is untouched because its remaining allowance is intentional for batchingcargo test for the crate passes and both forwarder examples still forward correctly
Summary by CodeRabbit