Add ERC-20 Encumber Extension (ERC-7246) #6250
Conversation
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
WalkthroughA new ERC7246 standard interface and implementation are introduced, adding encumbrance functionality to ERC20 tokens. The interface defines methods to track and manage encumbered balances alongside available balances, along with encumber, encumberFrom, and release operations. The implementation extends ERC20 with internal mappings to track encumbrances per owner-spender pair and validates availability before transfers. Documentation is updated across interface and extension READMEs, and a comprehensive test suite validates the encumbrance mechanics and interactions with standard ERC20 operations. A minor dependency version bump is also included. Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 3
🧹 Nitpick comments (3)
test/token/ERC20/extensions/draft-ERC7246.test.js (3)
5-9: Remove unused imports or enable the behavior tests.The static analysis correctly identifies that
shouldBehaveLikeERC20,shouldBehaveLikeERC20Transfer, andshouldBehaveLikeERC20Approveare imported but never used. This aligns with the commented-out line 193.Either remove these imports or uncomment the behavior test to verify ERC20 compatibility is preserved.
🔎 Option 1: Remove unused imports
-const { - shouldBehaveLikeERC20, - shouldBehaveLikeERC20Transfer, - shouldBehaveLikeERC20Approve, -} = require('../ERC20.behavior.js');🔎 Option 2: Enable behavior tests
- // shouldBehaveLikeERC20(value); + shouldBehaveLikeERC20(value);
88-92: Usethis.encumberAmountinstead of hardcoded400n.For consistency and maintainability, reference the variable defined in the
beforeEachblock.🔎 Proposed fix
it('should allow transfer within available balance', async function () { - await expect(this.token.connect(this.holder).transfer(this.other, value - 400n)) + await expect(this.token.connect(this.holder).transfer(this.other, value - this.encumberAmount)) .to.emit(this.token, 'Transfer') - .withArgs(this.holder.address, this.other.address, value - 400n); + .withArgs(this.holder.address, this.other.address, value - this.encumberAmount); });
37-93: Consider adding a test forEncumberevent emission in theencumberdescribe block.The
encumberFromtests verify theEncumberevent is emitted (line 107), but theencumbertests don't have an equivalent assertion.🔎 Suggested test to add
it('should emit Encumber event', async function () { await expect(this.token.connect(this.holder).encumber(this.other, 100n)) .to.emit(this.token, 'Encumber') .withArgs(this.holder.address, this.other.address, 100n); });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
contracts/interfaces/README.adoccontracts/interfaces/draft-IERC7246.solcontracts/token/ERC20/ERC20.solcontracts/token/ERC20/IERC20.solcontracts/token/ERC20/README.adoccontracts/token/ERC20/extensions/draft-ERC7246.solpackage.jsontest/token/ERC20/extensions/draft-ERC7246.test.js
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-04T09:13:21.278Z
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5904
File: contracts/crosschain/ERC7786Recipient.sol:3-3
Timestamp: 2025-09-04T09:13:21.278Z
Learning: In OpenZeppelin contracts, hardhat.config.js uses a sophisticated yargs-based configuration where the Solidity compiler version is set via argv.compiler (line 77) with a default of '0.8.27' defined in the yargs options (line 21), allowing flexible command-line overrides while maintaining a consistent default.
Applied to files:
package.json
📚 Learning: 2025-09-04T09:13:21.278Z
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5904
File: contracts/crosschain/ERC7786Recipient.sol:3-3
Timestamp: 2025-09-04T09:13:21.278Z
Learning: In OpenZeppelin contracts, hardhat.config.js uses yargs to configure the Solidity compiler version dynamically via command line arguments, with a default of '0.8.27' set on line 21 and the solidity configuration using argv.compile around lines 76-87, rather than hardcoded version strings.
Applied to files:
package.json
🧬 Code graph analysis (1)
test/token/ERC20/extensions/draft-ERC7246.test.js (1)
test/utils/Address.test.js (1)
recipient(12-12)
🪛 GitHub Check: lint
test/token/ERC20/extensions/draft-ERC7246.test.js
[failure] 8-8:
'shouldBehaveLikeERC20Approve' is assigned a value but never used
[failure] 7-7:
'shouldBehaveLikeERC20Transfer' is assigned a value but never used
[failure] 6-6:
'shouldBehaveLikeERC20' is assigned a value but never used
🪛 Gitleaks (8.30.0)
test/token/ERC20/extensions/draft-ERC7246.test.js
[high] 69-69: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 75-75: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 84-84: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 160-160: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Redirect rules - solidity-contracts
- GitHub Check: Header rules - solidity-contracts
- GitHub Check: Pages changed - solidity-contracts
- GitHub Check: halmos
- GitHub Check: tests-foundry
- GitHub Check: tests-upgradeable
- GitHub Check: tests
- GitHub Check: coverage
- GitHub Check: slither
🔇 Additional comments (11)
contracts/token/ERC20/IERC20.sol (1)
39-39: LGTM: Documentation standardization.The explicit event placeholders
{IERC20-Transfer}and{IERC20-Approval}improve cross-referencing clarity in the documentation without affecting contract functionality.Also applies to: 65-65, 76-76
contracts/token/ERC20/README.adoc (1)
30-30: LGTM: ERC7246 documentation added.The ERC7246 references are properly placed in the extensions list and documentation sections, consistent with the formatting of other extensions.
Also applies to: 78-79
contracts/interfaces/README.adoc (1)
49-49: LGTM: IERC7246 interface documented.The interface is correctly positioned (numerically before IERC7579) in both the list and Detailed ABI sections, following the established documentation pattern.
Also applies to: 117-118
contracts/token/ERC20/ERC20.sol (1)
155-155: LGTM: Enhanced event documentation placeholders.The explicit event placeholders improve documentation generation quality by providing fully qualified references (
{Transfer-address-address-uint256},{IERC20-Transfer},{IERC20-Approval}), consistent with the standardization applied to IERC20.sol.Also applies to: 174-174, 210-210, 242-242
package.json (1)
64-64: No action required — dependency version is valid and secure.The version
@openzeppelin/[email protected]exists on npm, is the latest available, carries an MIT license, and has no known security vulnerabilities or deprecation warnings.contracts/interfaces/draft-IERC7246.sol (1)
7-63: LGTM: Interface structure is well-designed.The interface correctly extends IERC20, defines appropriate events, and provides clear separation between view functions and state-modifying operations. The encumbrance semantics are well-documented.
contracts/token/ERC20/extensions/draft-ERC7246.sol (5)
70-76: LGTM: Unchecked block is safe.The comment correctly justifies safety: since
availableBalanceOfis checked first and equalsbalanceOf(owner) - encumberedBalanceOf(owner), the additions cannot overflow. The invariant_encumbrances[owner][spender] <= _encumberedBalances[owner] <= balanceOf(owner)holds.
119-127: Resolve TODO before merging.The TODO raises a valid design question: when a user has insufficient balance (not just insufficient available balance), should the error be
ERC7246InsufficientAvailableBalanceor the standard ERC20 insufficient balance error?Current behavior: A user with 100 tokens (0 encumbered) trying to transfer 200 will get
ERC7246InsufficientAvailableBalance(100, 200)instead of the standard ERC20 error.Consider whether this is the intended UX, and remove the TODO once decided.
103-116: LGTM: Encumbrance-first consumption is correctly implemented.The logic correctly consumes encumbrances before allowances. The
Math.minensures we don't release more than available, and the unchecked subtraction is safe becauseencumberedToUse <= amount.
50-53: LGTM: Forward encumbrance pattern enabled.The implementation correctly allows encumbrance forwarding by consuming the caller's encumbrance (via
_spendAllowance) before creating a new encumbrance tospender. This is well-tested in the test file's "can forward encumbrance" case.
87-97: LGTM: Release logic is correct and safe.The require check on line 89 ensures the unchecked subtractions cannot underflow. The invariant that
_encumbrances[owner][spender] <= _encumberedBalances[owner]is maintained.
2 participants
This PR introduces the ERC-20 encumber extension (ERC-7246). ERC-7246 specifies a way to create encumbrances on tokens, which is a stronger version of an approval. If an encumbrance is granted to a given account, only that account can transfer the tokens. The balance of the account granting the encumbrance does not decrease until the tokens are actually transferred out by the account granted the encumbrance.
PR Checklist
npx changeset add)