ERC20 Approval Cap and Expiration Configurables#6521
Conversation
…d file for test. added safeapprovaltoken logic
🦋 Changeset detectedLatest commit: 84932b3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis PR introduces 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
test/token/ERC20/ERC20SafeApproval.t.sol (1)
1-32: ⚡ Quick winRemove commented-out test file or implement the tests.
The entire test file is wrapped in a block comment with no active test logic. Including a fully commented-out placeholder file adds maintenance burden and can confuse future developers. Since comprehensive JavaScript tests already exist in
test/token/ERC20/extensions/ERC20SafeApproval.test.jscovering approval caps, expiration, and combined behaviors, consider:
- Preferred: Remove this file entirely if Foundry tests are not immediately planned.
- Alternative: Implement the TODO test cases if Foundry coverage is required alongside the existing JavaScript tests.
- If keeping as a TODO: Track the work in a GitHub issue rather than committing a skeleton file.
🤖 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 `@test/token/ERC20/ERC20SafeApproval.t.sol` around lines 1 - 32, The ERC20SafeApproval.t.sol file is entirely commented out; either delete the file or implement the Foundry tests: if deleting, remove this file from the repo; if implementing, uncomment and in setUp() deploy ERC20SafeApprovalToken with an initial cap and supply and assign it to the token state variable, then add tests matching the TODOs (e.g., testNormalApprovalWithinCap, testApprovalAboveCapReverts, testApproveWithExpirySetsExpiry, testTransferFromBeforeExpirySucceeds, testTransferFromAfterExpiryReverts, testCombinedCapAndExpiry) that exercise approve/approveWithExpiry and transferFrom behaviors using the owner, spender, and recipient addresses defined in the contract so Foundry verifies the cap and expiry logic.contracts/token/ERC20/extensions/ERC20SafeApprovalToken.sol (1)
11-17: ⚡ Quick winAdd NatSpec for the constructor parameters.
The constructor takes four parameters with non-obvious semantics (
capoverloads withtype(uint256).maxas "disabled",initialSupplyis in raw token units withdecimals()implicit). A short@paramblock per argument prevents misuse, especially since this is presented as a reference / demo contract.🤖 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 `@contracts/token/ERC20/extensions/ERC20SafeApprovalToken.sol` around lines 11 - 17, Add NatSpec comments to the ERC20SafeApprovalToken constructor: place a docblock immediately above the constructor and add an `@param` line for each argument describing semantics — `@param` name the token name, `@param` symbol the token symbol, `@param` cap the maximum supply cap (document that passing type(uint256).max disables the cap), and `@param` initialSupply the initial token amount to mint (state that this is in raw token units and subject to decimals(), e.g., includes implicit decimals()). Ensure the comment references the constructor of ERC20SafeApprovalToken so callers understand the overload/disabled-cap behavior.contracts/token/ERC20/extensions/ERC20SafeApproval.sol (2)
41-52: ⚡ Quick winAdd NatSpec for the new public surface (errors, events, view helpers, and
approveWithExpiration).The errors and
ApprovalWithExpirationevent are documented, butgetApprovalCap,getApprovalExpiry,isApprovalExpired,approveWithExpiration, and the cap-update semantics aren't. Since this is meant to land in OZ-style extension surface, third-party integrators rely on generated docs and IDE hovers. A few lines of NatSpec on each public/external/internal-virtual member (including thecap == type(uint256).max"disabled cap" semantics on_checkCap) would meaningfully improve the API.🤖 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 `@contracts/token/ERC20/extensions/ERC20SafeApproval.sol` around lines 41 - 52, Add NatSpec comments for the newly exposed API: document getApprovalCap(), getApprovalExpiry(address owner, address spender), isApprovalExpired(address owner, address spender), and approveWithExpiration(...) (and mark inherited/override behavior for virtual/external where appropriate); also add short NatSpec on the cap-update semantics used by _checkCap (explicitly state that cap == type(uint256).max disables cap enforcement) and annotate any public errors/events related to approvals so IDEs and generated docs show parameter meanings and behavior (e.g., units for expiry, when expiry==0 means no expiry, and how caps interact with approveWithExpiration).
79-84: ⚡ Quick winDocument the "cap disabled" sentinel behavior.
_checkCapsilently treatstype(uint256).maxas "no cap" — a reasonable convention, but it's invisible to readers ofgetApprovalCap()(which will return the sentinel) and to anyone reading the cap via events. Either add NatSpec stating thattype(uint256).maxdisables enforcement, or just writevalue > capand rely ontype(uint256).maxnaturally not being exceedable; the explicit branch is mostly stylistic.🤖 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 `@contracts/token/ERC20/extensions/ERC20SafeApproval.sol` around lines 79 - 84, The function _checkCap currently treats type(uint256).max as a sentinel meaning "cap disabled" but this is undocumented; add NatSpec comments to _checkCap and the public getter (getApprovalCap) stating that when _approvalCap == type(uint256).max enforcement is disabled (i.e., no approval cap), and mention that events/externally visible values will show the sentinel; alternatively you may remove the explicit branch and rely on the natural impossibility of exceeding type(uint256).max—but be explicit in the NatSpec so callers and auditors understand the sentinel behavior; reference _checkCap, _approvalCap, getApprovalCap(), and the ERC20ApprovalCapExceeded error in the comment.
🤖 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 `@contracts/token/ERC20/extensions/ERC20SafeApproval.sol`:
- Around line 54-59: Remove the stale comment "// TODO: override approve()" that
sits immediately above the implemented approve(address spender, uint256 amount)
function; the override is already implemented in the approve function (which
calls _checkCap, clears _approvalExpiry, and returns super.approve), so delete
that misleading TODO comment to avoid confusing future readers.
- Around line 27-32: Remove the stale TODO comments left alongside the
implemented storage declarations: delete the comments "// TODO: approval cap
storage", "// TODO: expiry storage", and the misplaced "// TODO: override
transferFrom()" so only the actual storage members remain (_approvalCap and
mapping _approvalExpiry) and the existing transferFrom implementation is not
obscured by comments; keep the declarations for uint256 private _approvalCap and
mapping(address => mapping(address => uint256)) private _approvalExpiry as-is.
- Around line 55-77: The extension incorrectly uses msg.sender instead of
_msgSender(), causing mismatches with ERC20's use of _msgSender() (breaks
meta-transactions/trusted forwarder). Update approve(), approveWithExpiration(),
and transferFrom() to use _msgSender() when accessing/deleting/setting
_approvalExpiry[msg.sender][spender] (and when emitting ApprovalWithExpiration)
and when reading expiry in transferFrom() (replace
_approvalExpiry[from][msg.sender] with _approvalExpiry[from][_msgSender()]) so
that the expiry mapping, emitted event and allowance consumption use the same
effective sender as super.approve()/super.transferFrom().
- Around line 55-69: Override the internal 4-arg _approve(owner, spender, value,
emitEvent) in ERC20SafeApproval and enforce the cap only when emitEvent == true
by calling _checkCap(value) in that branch; also ensure user-initiated allowance
clears approvals expiry when value == 0 by deleting
_approvalExpiry[owner][spender] when emitEvent == true and value == 0, then
delegate to super._approve(owner, spender, value, emitEvent); keep the method
signature as internal virtual override to match the base contract.
In `@contracts/token/ERC20/extensions/ERC20SafeApprovalToken.sol`:
- Around line 19-23: Remove the stale "// TODO: add constructor" comment and
either wire ownership or document intent; to satisfy the PR goal, inherit
OpenZeppelin Ownable in ERC20SafeApprovalToken, add an external onlyOwner
function setApprovalCap(uint256 newCap) that calls the internal
_setApprovalCap(newCap), and remove the "// TODO: add owner storage and
onlyOwner modifier" and "// TODO: expose setApprovalCap for owner" comments;
ensure the contract import/constructor changes reflect Ownable inheritance and
that setApprovalCap is the single owner-controlled entry point to update the
cap.
In `@README.md`:
- Around line 74-76: Update the README "Project Structure" and "Run Tests"
sections to point to the maintained JavaScript test suite instead of the
placeholder Foundry path: replace the forge command example with the npm test
invocation that runs the JS test file
(test/token/ERC20/extensions/ERC20SafeApproval.test.js) and update any
references to the old Solidity test path
(test/token/ERC20/ERC20SafeApproval.t.sol) to the new JS path so contributors
run the correct tests; ensure examples show "npm test --
test/token/ERC20/extensions/ERC20SafeApproval.test.js" and adjust nearby
explanatory text under the "Project Structure" and "Run Tests" headings
accordingly.
- Around line 60-63: The README's setup steps change directory to "contracts",
which is incorrect; update the commands so users run them from the repository
root (or from the directory that contains foundry.toml) — e.g., remove or
replace "cd contracts" and ensure the sequence is "git clone <repo> && cd
<repo-root-or-foundry-dir> && forge install" so that forge install runs where
foundry.toml exists.
---
Nitpick comments:
In `@contracts/token/ERC20/extensions/ERC20SafeApproval.sol`:
- Around line 41-52: Add NatSpec comments for the newly exposed API: document
getApprovalCap(), getApprovalExpiry(address owner, address spender),
isApprovalExpired(address owner, address spender), and
approveWithExpiration(...) (and mark inherited/override behavior for
virtual/external where appropriate); also add short NatSpec on the cap-update
semantics used by _checkCap (explicitly state that cap == type(uint256).max
disables cap enforcement) and annotate any public errors/events related to
approvals so IDEs and generated docs show parameter meanings and behavior (e.g.,
units for expiry, when expiry==0 means no expiry, and how caps interact with
approveWithExpiration).
- Around line 79-84: The function _checkCap currently treats type(uint256).max
as a sentinel meaning "cap disabled" but this is undocumented; add NatSpec
comments to _checkCap and the public getter (getApprovalCap) stating that when
_approvalCap == type(uint256).max enforcement is disabled (i.e., no approval
cap), and mention that events/externally visible values will show the sentinel;
alternatively you may remove the explicit branch and rely on the natural
impossibility of exceeding type(uint256).max—but be explicit in the NatSpec so
callers and auditors understand the sentinel behavior; reference _checkCap,
_approvalCap, getApprovalCap(), and the ERC20ApprovalCapExceeded error in the
comment.
In `@contracts/token/ERC20/extensions/ERC20SafeApprovalToken.sol`:
- Around line 11-17: Add NatSpec comments to the ERC20SafeApprovalToken
constructor: place a docblock immediately above the constructor and add an
`@param` line for each argument describing semantics — `@param` name the token name,
`@param` symbol the token symbol, `@param` cap the maximum supply cap (document that
passing type(uint256).max disables the cap), and `@param` initialSupply the
initial token amount to mint (state that this is in raw token units and subject
to decimals(), e.g., includes implicit decimals()). Ensure the comment
references the constructor of ERC20SafeApprovalToken so callers understand the
overload/disabled-cap behavior.
In `@test/token/ERC20/ERC20SafeApproval.t.sol`:
- Around line 1-32: The ERC20SafeApproval.t.sol file is entirely commented out;
either delete the file or implement the Foundry tests: if deleting, remove this
file from the repo; if implementing, uncomment and in setUp() deploy
ERC20SafeApprovalToken with an initial cap and supply and assign it to the token
state variable, then add tests matching the TODOs (e.g.,
testNormalApprovalWithinCap, testApprovalAboveCapReverts,
testApproveWithExpirySetsExpiry, testTransferFromBeforeExpirySucceeds,
testTransferFromAfterExpiryReverts, testCombinedCapAndExpiry) that exercise
approve/approveWithExpiry and transferFrom behaviors using the owner, spender,
and recipient addresses defined in the contract so Foundry verifies the cap and
expiry logic.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3d94251e-8b01-4d01-a539-dc344a7d6ed2
📒 Files selected for processing (7)
.changeset/chubby-heads-film.mdREADME.mdcontracts/mocks/token/ERC20SafeApprovalMock.solcontracts/token/ERC20/extensions/ERC20SafeApproval.solcontracts/token/ERC20/extensions/ERC20SafeApprovalToken.soltest/token/ERC20/ERC20SafeApproval.t.soltest/token/ERC20/extensions/ERC20SafeApproval.test.js
| // TODO: approval cap storage | ||
| uint256 private _approvalCap; | ||
| // TODO: expiry storage | ||
|
|
||
| // TODO: override transferFrom() | ||
| mapping(address owner => mapping(address spender => uint256 expiry)) private _approvalExpiry; |
There was a problem hiding this comment.
Stale TODOs interleaved with implemented storage declarations.
Lines 27, 29, and 31 are leftover scaffolding TODOs (approval cap storage, expiry storage, override transferFrom()) for work that has since been completed in this file. The misplaced // TODO: override transferFrom() between the expiry comment and the mapping is especially confusing for readers. Please remove these before merging.
🧹 Proposed cleanup
- // TODO: approval cap storage
uint256 private _approvalCap;
- // TODO: expiry storage
-
- // TODO: override transferFrom()
mapping(address owner => mapping(address spender => uint256 expiry)) private _approvalExpiry;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: approval cap storage | |
| uint256 private _approvalCap; | |
| // TODO: expiry storage | |
| // TODO: override transferFrom() | |
| mapping(address owner => mapping(address spender => uint256 expiry)) private _approvalExpiry; | |
| uint256 private _approvalCap; | |
| mapping(address owner => mapping(address spender => uint256 expiry)) private _approvalExpiry; |
🤖 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 `@contracts/token/ERC20/extensions/ERC20SafeApproval.sol` around lines 27 - 32,
Remove the stale TODO comments left alongside the implemented storage
declarations: delete the comments "// TODO: approval cap storage", "// TODO:
expiry storage", and the misplaced "// TODO: override transferFrom()" so only
the actual storage members remain (_approvalCap and mapping _approvalExpiry) and
the existing transferFrom implementation is not obscured by comments; keep the
declarations for uint256 private _approvalCap and mapping(address =>
mapping(address => uint256)) private _approvalExpiry as-is.
| // TODO: override approve() | ||
| function approve(address spender, uint256 amount) public virtual override returns (bool) { | ||
| _checkCap(amount); | ||
| delete _approvalExpiry[msg.sender][spender]; | ||
| return super.approve(spender, amount); | ||
| } |
There was a problem hiding this comment.
Remove stale // TODO: override approve() — already implemented below.
Same scaffolding cleanup as above; the override is right under the TODO. Drop the comment so it doesn't mislead future readers into thinking work is pending.
🧹 Proposed cleanup
- // TODO: override approve()
function approve(address spender, uint256 amount) public virtual override returns (bool) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: override approve() | |
| function approve(address spender, uint256 amount) public virtual override returns (bool) { | |
| _checkCap(amount); | |
| delete _approvalExpiry[msg.sender][spender]; | |
| return super.approve(spender, amount); | |
| } | |
| function approve(address spender, uint256 amount) public virtual override returns (bool) { | |
| _checkCap(amount); | |
| delete _approvalExpiry[msg.sender][spender]; | |
| return super.approve(spender, amount); | |
| } |
🤖 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 `@contracts/token/ERC20/extensions/ERC20SafeApproval.sol` around lines 54 - 59,
Remove the stale comment "// TODO: override approve()" that sits immediately
above the implemented approve(address spender, uint256 amount) function; the
override is already implemented in the approve function (which calls _checkCap,
clears _approvalExpiry, and returns super.approve), so delete that misleading
TODO comment to avoid confusing future readers.
| function approve(address spender, uint256 amount) public virtual override returns (bool) { | ||
| _checkCap(amount); | ||
| delete _approvalExpiry[msg.sender][spender]; | ||
| return super.approve(spender, amount); | ||
| } | ||
|
|
||
| function approveWithExpiration(address spender, uint256 amount, uint256 expiryTime) public virtual returns (bool) { | ||
| if (expiryTime <= block.timestamp) { | ||
| revert ERC20InvalidExpiration(expiryTime); | ||
| } | ||
| _checkCap(amount); | ||
| _approvalExpiry[msg.sender][spender] = expiryTime; | ||
| emit ApprovalWithExpiration(msg.sender, spender, amount, expiryTime); | ||
| return super.approve(spender, amount); | ||
| } | ||
|
|
||
| function transferFrom(address from, address to, uint256 amount) public virtual override returns (bool) { | ||
| uint256 expiry = _approvalExpiry[from][msg.sender]; | ||
| if (expiry != 0 && block.timestamp > expiry) { | ||
| revert ERC20ApprovalExpired(from, msg.sender, expiry); | ||
| } | ||
| return super.transferFrom(from, to, amount); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which sender API is used by other ERC20 extensions in OpenZeppelin contracts
rg -nP --type=sol -C2 '\b(msg\.sender|_msgSender\(\))' -g 'contracts/token/ERC20/**'Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Find ERC20 extension files first
find contracts/token/ERC20/extensions -name "*.sol" -type f | head -20Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 914
🏁 Script executed:
#!/bin/bash
# Check which sender API is used in ERC20 extensions
rg -n --glob='contracts/token/ERC20/**/*.sol' '(msg\.sender|_msgSender\(\))' -C 1Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 8849
🏁 Script executed:
#!/bin/bash
# Check the base ERC20 contract's implementation
rg -n --glob='contracts/token/ERC20/ERC20.sol' '(msg\.sender|_msgSender\(\))' -B 1 -A 1Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 914
Use _msgSender() instead of msg.sender for ERC-2771 (meta-transaction) compatibility.
OpenZeppelin's ERC20 uses _msgSender() in approve() (line 121), transferFrom() (line 143), and other public methods. This extension hardcodes msg.sender, creating a divergence with ERC2771Context (trusted forwarder):
- In
approve()(line 57), the expiry is deleted undermsg.sender, butsuper.approve()records the allowance under_msgSender(). With a forwarder, these are different addresses—the wrong expiry entry is cleared and a stale expiring approval survives. - In
approveWithExpiration()(lines 66–67), the expiry and event usemsg.senderwhilesuper.approve()uses_msgSender(), causing off-chain indexers and on-chain checks to diverge for meta-tx users. - In
transferFrom()(lines 72, 74), the spender lookup usesmsg.sender, butsuper.transferFrom()consumes the allowance keyed by_msgSender().
All other ERC20 extensions (ERC20Wrapper, ERC4626, ERC20Burnable, ERC1363, etc.) consistently use _msgSender(). Mirror the pattern and use _msgSender() (already available via Context).
Proposed fix
function approve(address spender, uint256 amount) public virtual override returns (bool) {
_checkCap(amount);
- delete _approvalExpiry[msg.sender][spender];
+ address owner = _msgSender();
+ delete _approvalExpiry[owner][spender];
return super.approve(spender, amount);
}
function approveWithExpiration(address spender, uint256 amount, uint256 expiryTime) public virtual returns (bool) {
if (expiryTime <= block.timestamp) {
revert ERC20InvalidExpiration(expiryTime);
}
_checkCap(amount);
- _approvalExpiry[msg.sender][spender] = expiryTime;
- emit ApprovalWithExpiration(msg.sender, spender, amount, expiryTime);
+ address owner = _msgSender();
+ _approvalExpiry[owner][spender] = expiryTime;
+ emit ApprovalWithExpiration(owner, spender, amount, expiryTime);
return super.approve(spender, amount);
}
function transferFrom(address from, address to, uint256 amount) public virtual override returns (bool) {
- uint256 expiry = _approvalExpiry[from][msg.sender];
+ address spender = _msgSender();
+ uint256 expiry = _approvalExpiry[from][spender];
if (expiry != 0 && block.timestamp > expiry) {
- revert ERC20ApprovalExpired(from, msg.sender, expiry);
+ revert ERC20ApprovalExpired(from, spender, expiry);
}
return super.transferFrom(from, to, amount);
}🤖 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 `@contracts/token/ERC20/extensions/ERC20SafeApproval.sol` around lines 55 - 77,
The extension incorrectly uses msg.sender instead of _msgSender(), causing
mismatches with ERC20's use of _msgSender() (breaks meta-transactions/trusted
forwarder). Update approve(), approveWithExpiration(), and transferFrom() to use
_msgSender() when accessing/deleting/setting
_approvalExpiry[msg.sender][spender] (and when emitting ApprovalWithExpiration)
and when reading expiry in transferFrom() (replace
_approvalExpiry[from][msg.sender] with _approvalExpiry[from][_msgSender()]) so
that the expiry mapping, emitted event and allowance consumption use the same
effective sender as super.approve()/super.transferFrom().
| function approve(address spender, uint256 amount) public virtual override returns (bool) { | ||
| _checkCap(amount); | ||
| delete _approvalExpiry[msg.sender][spender]; | ||
| return super.approve(spender, amount); | ||
| } | ||
|
|
||
| function approveWithExpiration(address spender, uint256 amount, uint256 expiryTime) public virtual returns (bool) { | ||
| if (expiryTime <= block.timestamp) { | ||
| revert ERC20InvalidExpiration(expiryTime); | ||
| } | ||
| _checkCap(amount); | ||
| _approvalExpiry[msg.sender][spender] = expiryTime; | ||
| emit ApprovalWithExpiration(msg.sender, spender, amount, expiryTime); | ||
| return super.approve(spender, amount); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm OZ v5 ERC20 still exposes the 4-arg internal _approve and how _spendAllowance calls it
ast-grep --pattern 'function _approve(address $_, address $_, uint256 $_, bool $_) internal virtual { $$$ }'
rg -nP --type=sol -C3 '_approve\([^)]*\b(true|false)\b\s*\)' -g 'contracts/token/ERC20/**'Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 994
🏁 Script executed:
#!/bin/bash
# Find and read ERC20SafeApproval.sol
find . -name "ERC20SafeApproval.sol" -type fRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 136
🏁 Script executed:
#!/bin/bash
# Check the full ERC20SafeApproval.sol implementation
cat -n contracts/token/ERC20/extensions/ERC20SafeApproval.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 3900
🏁 Script executed:
#!/bin/bash
# Check ERC20Permit.permit() to see how it invokes _approve
rg -n "permit" contracts/token/ERC20/extensions/ERC20Permit.sol -A 20 | head -50Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 1730
🏁 Script executed:
#!/bin/bash
# Check _spendAllowance implementation and how it calls _approve
rg -n "_spendAllowance" contracts/token/ERC20/ERC20.sol -A 15Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 2054
🏁 Script executed:
#!/bin/bash
# Get the complete permit() function including the _approve call
rg -n "function permit" contracts/token/ERC20/extensions/ERC20Permit.sol -A 30Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 1065
🏁 Script executed:
#!/bin/bash
# Also verify if ERC20SafeApproval might be used with ERC20Permit in tests
find . -name "*.sol" -type f -exec grep -l "ERC20SafeApproval" {} \; | head -10Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 291
🏁 Script executed:
#!/bin/bash
# Check if there's a 3-arg _approve method or how it resolves
rg -n "function _approve" contracts/token/ERC20/ERC20.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 381
🏁 Script executed:
#!/bin/bash
# Check the test file to see if ERC20SafeApproval is tested with permit
cat -n test/token/ERC20/ERC20SafeApproval.t.sol | head -100Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 1113
🏁 Script executed:
#!/bin/bash
# Check the implementation of the 3-arg _approve
sed -n '251,265p' contracts/token/ERC20/ERC20.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 813
🏁 Script executed:
#!/bin/bash
# Verify the relationship between 3-arg and 4-arg _approve and how permit uses them
rg -n "function _approve" contracts/token/ERC20/ERC20.sol -A 5Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 928
Cap bypass via ERC20Permit.permit() and other internal _approve callers—override the virtual 4-arg _approve to enforce cap.
_checkCap is invoked only on the public approve / approveWithExpiration entry points. The cap is bypassed by any caller that writes allowance through the inherited _approve(owner, spender, value) method—most notably ERC20Permit.permit() (line 64 of ERC20Permit.sol), which sets allowances directly via signature. The 3-arg _approve delegates to the virtual 4-arg variant with emitEvent=true.
Since the PR aims to provide an approval-cap alternative that complements ERC-2612, this gap should be closed in the extension itself.
Override the internal virtual 4-arg _approve(owner, spender, value, emitEvent) and enforce the cap only when emitEvent == true. This captures user-initiated approvals (including permit() and public approve()) while avoiding interference with _spendAllowance, which calls the 4-arg variant with emitEvent=false when decrementing allowance after a transfer.
🤖 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 `@contracts/token/ERC20/extensions/ERC20SafeApproval.sol` around lines 55 - 69,
Override the internal 4-arg _approve(owner, spender, value, emitEvent) in
ERC20SafeApproval and enforce the cap only when emitEvent == true by calling
_checkCap(value) in that branch; also ensure user-initiated allowance clears
approvals expiry when value == 0 by deleting _approvalExpiry[owner][spender]
when emitEvent == true and value == 0, then delegate to super._approve(owner,
spender, value, emitEvent); keep the method signature as internal virtual
override to match the base contract.
| // TODO: add owner storage and onlyOwner modifier | ||
|
|
||
| // TODO: add constructor | ||
|
|
||
| // TODO: expose setApprovalCap for owner |
There was a problem hiding this comment.
Reconcile/clean up the trailing TODOs.
// TODO: add constructor(line 21) is already done on lines 12–17 — drop it.// TODO: add owner storage and onlyOwner modifier(line 19) and// TODO: expose setApprovalCap for owner(line 23) describe meaningful follow-ups that block delivering on the PR objective of "Allow on-chain configurable caps (updatable internally)" — as shipped,_setApprovalCapis unreachable after deployment, so the concrete token effectively has a fixed cap. Please either (a) wireOwnable+ asetApprovalCap(uint256)onlyOwner entry point into this concrete reference token, or (b) explicitly note in the contract NatSpec and the changeset that this concrete token is a minimal demo and that ownership/admin control is intentionally out of scope.
Either resolution is fine, but the current "TODO list as a comment" leaves the concrete token in an in-between state.
Want me to draft the Ownable + setApprovalCap wiring (option a) so this concrete token actually satisfies the "configurable cap" objective?
🤖 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 `@contracts/token/ERC20/extensions/ERC20SafeApprovalToken.sol` around lines 19
- 23, Remove the stale "// TODO: add constructor" comment and either wire
ownership or document intent; to satisfy the PR goal, inherit OpenZeppelin
Ownable in ERC20SafeApprovalToken, add an external onlyOwner function
setApprovalCap(uint256 newCap) that calls the internal _setApprovalCap(newCap),
and remove the "// TODO: add owner storage and onlyOwner modifier" and "// TODO:
expose setApprovalCap for owner" comments; ensure the contract
import/constructor changes reflect Ownable inheritance and that setApprovalCap
is the single owner-controlled entry point to update the cap.
| ```bash | ||
| git clone https://github.com/kennyb66/CS5833-FinalProject | ||
| cd contracts | ||
| forge install |
There was a problem hiding this comment.
Fix repository setup path in installation steps.
The cd contracts step is likely incorrect for project setup and can break forge install for users following the README. Use the repository root (or the actual directory containing foundry.toml) instead.
Suggested doc fix
git clone https://github.com/kennyb66/CS5833-FinalProject
-cd contracts
+cd CS5833-FinalProject
forge install📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```bash | |
| git clone https://github.com/kennyb66/CS5833-FinalProject | |
| cd contracts | |
| forge install |
🤖 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 `@README.md` around lines 60 - 63, The README's setup steps change directory to
"contracts", which is incorrect; update the commands so users run them from the
repository root (or from the directory that contains foundry.toml) — e.g.,
remove or replace "cd contracts" and ensure the sequence is "git clone <repo> &&
cd <repo-root-or-foundry-dir> && forge install" so that forge install runs where
foundry.toml exists.
| ```bash | ||
| forge test --match-path test/token/ERC20/ERC20SafeApproval.t.sol -v | ||
| ``` |
There was a problem hiding this comment.
Align test instructions with the actual maintained test suite.
The README points to a Foundry test file path that appears to be placeholder-oriented, while this PR’s main coverage is described under the JavaScript suite. Update the “Project Structure” and “Run Tests” sections so contributors execute the real tests.
Suggested doc fix
-└── ERC20SafeApproval.t.sol # Foundry test suite
+└── extensions/
+ └── ERC20SafeApproval.test.js # Hardhat test suite-### Run Tests
+### Run Tests (Hardhat)
```bash
-forge test --match-path test/token/ERC20/ERC20SafeApproval.t.sol -v
+npm test -- test/token/ERC20/extensions/ERC20SafeApproval.test.js</details>
Also applies to: 37-45
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @README.md around lines 74 - 76, Update the README "Project Structure" and
"Run Tests" sections to point to the maintained JavaScript test suite instead of
the placeholder Foundry path: replace the forge command example with the npm
test invocation that runs the JS test file
(test/token/ERC20/extensions/ERC20SafeApproval.test.js) and update any
references to the old Solidity test path
(test/token/ERC20/ERC20SafeApproval.t.sol) to the new JS path so contributors
run the correct tests; ensure examples show "npm test --
test/token/ERC20/extensions/ERC20SafeApproval.test.js" and adjust nearby
explanatory text under the "Project Structure" and "Run Tests" headings
accordingly.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Updated project structure and installation instructions, added new files and deployment scripts, and refined usage examples.
2 participants
Fixes #6500
Added an ERC20 extension that adds two optional and configurable constraints (an approval cap and an approval expiration time).
ERC20SafeApproval extends OpenZeppelin’s ERC20 implementation and introduces a configurable approval cap that restricts the maximum allowance that can be granted. Any approval exceeding the cap is reverted using a custom error (ERC20ApprovalCapExceeded). The cap can be updated by an internal setter and enforced consistently across approve and approveWithExpiration.
The approveWithExpiration function allows setting a timestamp after which the allowance becomes invalid. The transferFrom function is extended to check whether the approval has expired and reverts with ERC20ApprovalExpired if so. Expiration state is stored per (owner, spender) pair.
PR Checklist
npx changeset add)