Replace ERC20Blocklist/Allowlist with ERC20Restricted #715
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR replaces ERC20Allowlist and ERC20Blacklist with a unified ERC20Restricted contract across stablecoin implementations. It adds an isUserAllowed() function, updates _update() override signatures, removes _approve() overrides, and changes the disallow flow to use _resetUser(). The changeset marks this as a minor version bump. Changes
Sequence DiagramsequenceDiagram
participant User
participant Stablecoin as Stablecoin Contract
participant Restriction as ERC20Restricted
rect rgb(220, 230, 240)
Note over User,Restriction: User Restriction Check (New Flow)
User->>Stablecoin: transfer/mint/burn
Stablecoin->>Stablecoin: _update()
Stablecoin->>Stablecoin: isUserAllowed(user)
Stablecoin->>Restriction: getRestriction(user)
Restriction-->>Stablecoin: Restriction enum
Stablecoin-->>Stablecoin: compare == Restriction.ALLOWED
end
rect rgb(230, 240, 220)
Note over User,Restriction: User Disallow Flow (Updated)
User->>Stablecoin: disallowUser(address user)
Stablecoin->>Restriction: _resetUser(user)
Restriction-->>Stablecoin: restriction cleared
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/solidity/src/stablecoin.test.ts.md (1)
407-440: Snapshot confirms API inconsistency between allowlist and blocklist modes.This blocklist snapshot confirms that contracts generated with blocklist mode do NOT include the
isUserAllowedfunction, while allowlist contracts do (see lines 380-382 in the allowlist snapshot). This creates an inconsistent public API across the two modes despite both using the sameERC20Restrictedbase contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/core/solidity/src/stablecoin.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
.changeset/cuddly-bats-swim.md(1 hunks)packages/core/solidity/src/stablecoin.test.ts.md(5 hunks)packages/core/solidity/src/stablecoin.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-15T22:49:25.653Z
Learnt from: ernestognw
Repo: OpenZeppelin/contracts-wizard PR: 609
File: .changeset/sour-hats-grow.md:2-6
Timestamp: 2025-08-15T22:49:25.653Z
Learning: In OpenZeppelin contracts-wizard, breaking changes that have concrete migration paths (like dependency migrations from Community Contracts to OpenZeppelin Contracts) can be handled as minor version bumps instead of major bumps, per maintainer ernestognw's versioning policy.
Applied to files:
.changeset/cuddly-bats-swim.md
📚 Learning: 2025-09-12T15:07:08.673Z
Learnt from: ericglau
Repo: OpenZeppelin/contracts-wizard PR: 663
File: packages/core/cairo_alpha/src/custom.test.ts.md:12-12
Timestamp: 2025-09-12T15:07:08.673Z
Learning: In the OpenZeppelin contracts-wizard cairo_alpha package changelog (packages/core/cairo_alpha/CHANGELOG.md), each alpha version gets its own separate entry under the "Unreleased" section rather than updating a single entry. This allows tracking of changes across different alpha releases (e.g., v3.0.0-alpha.0, v3.0.0-alpha.1, v3.0.0-alpha.2 all have separate entries).
Applied to files:
.changeset/cuddly-bats-swim.mdpackages/core/solidity/src/stablecoin.test.ts.md
📚 Learning: 2025-09-12T20:50:43.101Z
Learnt from: CoveMB
Repo: OpenZeppelin/contracts-wizard PR: 644
File: packages/ui/api/services/open-ai.ts:43-56
Timestamp: 2025-09-12T20:50:43.101Z
Learning: In the OpenZeppelin contracts-wizard project, the current streaming implementation in packages/ui/api/services/open-ai.ts intentionally handles text deltas and function_calls with different formats (raw text chunks vs JSON objects). While this can cause JSON.parse issues, harmonizing the response format is considered outside scope of dependency updates as it requires UI refactoring to handle unified streaming responses.
Applied to files:
.changeset/cuddly-bats-swim.md
🧬 Code graph analysis (1)
packages/core/solidity/src/stablecoin.ts (2)
packages/core/solidity/src/erc20.ts (1)
functions(432-486)packages/core/solidity/src/set-access-control.ts (1)
requireAccessControl(48-83)
⏰ 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). (2)
- GitHub Check: build (solidity, default)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (6)
packages/core/solidity/src/stablecoin.ts (3)
67-69: LGTM! Unification to ERC20Restricted.The consolidation of ERC20Allowlist and ERC20Blocklist into a single ERC20Restricted contract simplifies the architecture.
169-174: LGTM! Function definition is well-formed.The
isUserAllowedfunction definition correctly specifies it as a public view function returning bool.
87-87: No issues found._resetUseris the correct removal function.The
_resetUserinternal helper in OpenZeppelin's ERC20Restricted "resets a user account to the default restriction", which correctly handles both allowlist removal (disallowUser) and blocklist removal (unblockUser). The change is semantically sound.packages/core/solidity/src/stablecoin.test.ts.md (2)
370-401: LGTM! Allowlist snapshot correctly reflects ERC20Restricted migration.The allowlist test snapshot correctly shows:
- Import and inheritance of
ERC20Restricted- Addition of
isUserAllowedfunction- Updated
disallowUserto call_resetUser- Correct override signature for
_update
593-679: LGTM! Full stablecoin snapshot correctly updated.The comprehensive stablecoin test case properly reflects the migration to
ERC20Restrictedwith all override signatures and function implementations updated correctly..changeset/cuddly-bats-swim.md (1)
1-5: LGTM! Minor version bump is appropriate.The changeset correctly marks this as a minor version bump. Per project versioning policy, breaking changes with concrete migration paths (like this dependency migration to ERC20Restricted) can be handled as minor rather than major bumps.
Based on learnings.
| if (type) { | ||
| c.addOverride(ERC20Limitation, functions.isUserAllowed); | ||
| c.setFunctionBody([`return getRestriction(user) == Restriction.ALLOWED;`], functions.isUserAllowed); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
API inconsistency: isUserAllowed only available in allowlist mode.
The isUserAllowed function is only added to contracts when type is true (allowlist mode). This creates an inconsistent public API where:
- Allowlist contracts expose
isUserAllowed(address) → bool - Blocklist contracts do NOT have this function
If ERC20Restricted is truly a unified base contract, both modes should expose the same interface. Additionally, the implementation getRestriction(user) == Restriction.ALLOWED appears allowlist-specific. In blocklist mode, non-blocked users might have a different restriction status (e.g., NONE or DEFAULT), causing this logic to return false even for allowed users.
Run this script to verify the ERC20Restricted contract interface and restriction enum values:
🏁 Script executed:
#!/bin/bash
# Description: Check ERC20Restricted interface and Restriction enum to understand the unified contract design
# Search for ERC20Restricted contract definition and public functions
rg -nP -A 20 'abstract contract ERC20Restricted' --type sol
# Search for Restriction enum definition to understand valid states
rg -nP -A 10 'enum Restriction' --type sol
# Check if isUserAllowed is defined in the base ERC20Restricted contract
rg -nP 'function isUserAllowed' --type solLength of output: 83
🏁 Script executed:
#!/bin/bash
# Find all Solidity files and search for contract/enum definitions
# First, find .sol files in the repository
echo "=== Finding Solidity files ==="
fd -e sol
# Search for ERC20Restricted contract (without type flag)
echo -e "\n=== ERC20Restricted contract definition ==="
rg -n -A 20 'abstract contract ERC20Restricted'
# Search for Restriction enum
echo -e "\n=== Restriction enum definition ==="
rg -n -A 10 'enum Restriction'
# Search for isUserAllowed function definition
echo -e "\n=== isUserAllowed function definitions ==="
rg -n 'function isUserAllowed'Length of output: 479
🏁 Script executed:
#!/bin/bash
# Search for stablecoin.ts and examine the context around lines 74-77
# Also search for blocklist mode implementation
echo "=== stablecoin.ts file around lines 74-77 (broader context) ==="
rg -n -B 30 -A 30 'isUserAllowed' packages/core/solidity/src/stablecoin.ts | head -100
echo -e "\n=== Search for blocklist mode handling (type=false) ==="
rg -n -B 5 -A 10 'if.*type' packages/core/solidity/src/stablecoin.ts
echo -e "\n=== Search for ERC20Restricted definition/import ==="
rg -n 'ERC20Restricted' packages/core/solidity/src/stablecoin.ts
echo -e "\n=== Search for Restriction enum in TypeScript files ==="
rg -n 'enum Restriction\|Restriction\.' packages/core/solidity/src/
echo -e "\n=== Search for function isUserAllowed in TypeScript generator ==="
rg -n -B 3 -A 3 'isUserAllowed' packages/core/solidity/src/stablecoin.tsLength of output: 4549
🏁 Script executed:
#!/bin/bash
# Read the ERC20Restricted.sol file to understand the base contract
echo "=== Reading SafetyCheck.sol (only .sol file) ==="
cat packages/core/solidity/contracts/SafetyCheck.sol
echo -e "\n=== Searching for ERC20Restricted in node_modules or vendor ==="
find . -name "ERC20Restricted.sol" 2>/dev/null | head -5
echo -e "\n=== Check if ERC20Restricted is in the codebase or external ==="
rg -n 'abstract contract ERC20Restricted\|function isUserAllowed' --max-count 20Length of output: 668
🏁 Script executed:
#!/bin/bash
# Read the SafetyCheck.sol and look for external contract references
echo "=== Reading SafetyCheck.sol ==="
cat packages/core/solidity/contracts/SafetyCheck.sol
echo -e "\n=== Search in entire repo for ERC20Restricted contract definition ==="
find . -type f -name "*.sol" -o -name "*.ts" 2>/dev/null | xargs grep -l "ERC20Restricted" | head -20
echo -e "\n=== Check stablecoin.test.ts.md for generated contract examples ==="
grep -n -A 15 "function isUserAllowed" packages/core/solidity/src/stablecoin.test.ts.md | head -50Length of output: 1969
🏁 Script executed:
#!/bin/bash
# Search for blocklist mode examples in test file and compare with allowlist
echo "=== Search for blocklist test case ==="
grep -n -B 5 'blocklist\|blockUser\|unblockUser' packages/core/solidity/src/stablecoin.test.ts.md | head -60
echo -e "\n=== Count occurrences of isUserAllowed in test file ==="
grep -c 'isUserAllowed' packages/core/solidity/src/stablecoin.test.ts.md
echo -e "\n=== Find context around blockUser (should be blocklist) ==="
grep -n -B 10 -A 10 'function blockUser' packages/core/solidity/src/stablecoin.test.ts.md | head -40
echo -e "\n=== Check if both allowlist and blocklist examples exist ==="
grep -n 'limitations.*allowlist\|limitations.*blocklist' packages/core/solidity/src/stablecoin.test.ts.md | head -10Length of output: 1588
API inconsistency: isUserAllowed is only added in allowlist mode, not blocklist mode.
Verification confirms the issue. The test file shows:
- Allowlist contract (lines 380, 640): includes
function isUserAllowed(address user) public view override returns (bool) - Blocklist contract (lines 423-428): only includes
blockUserandunblockUser, with noisUserAllowedfunction
This creates different public APIs for the two modes. Both contracts inherit from the same ERC20Restricted base, yet only the allowlist mode overrides isUserAllowed. If ERC20Restricted defines a default implementation of isUserAllowed, the blocklist mode's implementation would not reflect blocklist logic; if it doesn't, the blocklist mode lacks this function entirely.
Either:
- Both modes should override
isUserAllowedwith appropriate logic, or - The function should be removed from allowlist mode if it's not meaningful for blocklist
🤖 Prompt for AI Agents
In packages/core/solidity/src/stablecoin.ts around lines 74–77, the review found
that isUserAllowed is only added/overridden in allowlist mode creating an
inconsistent public API between allowlist and blocklist; to fix, ensure both
modes explicitly override isUserAllowed with the appropriate logic: for
allowlist keep the current override returning getRestriction(user) ==
Restriction.ALLOWED, and for blocklist add a matching override that returns
getRestriction(user) != Restriction.BLOCKED (or equivalent blocklist logic),
using c.addOverride with the correct function signature and c.setFunctionBody to
set the correct return expression so both contract variants expose the same
public function signature and behavior consistent with their restriction model.
There was a problem hiding this comment.
isUserAllowed is overridden only for custom allowlist logic. It is already implemented in ERC20Restricted, so overriding it for a blacklist has no purpose.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ERC20Restricted OpenZeppelin community contracts isUserAllowed default implementation
💡 Result:
Short answer: ERC20Restricted's default is a blocklist — isUserAllowed returns true for all accounts except those with Restriction == BLOCKED. To make it an allowlist you must override isUserAllowed to check for Restriction.ALLOWED (example below). See OpenZeppelin Community Contracts docs. [1][2]
Example override:
function isUserAllowed(address account) public view virtual override returns (bool) {
return getRestriction(account) == Restriction.ALLOWED;
}
Sources:
[1] OpenZeppelin Community Contracts — ERC20Restricted / IERC7943 description.
[2] OpenZeppelin Community Contracts API (ERC20 token extensions: allowlist/blocklist/restricted).
@typicalHuman, thank you for the clarification! You're absolutely right. I've confirmed that ERC20Restricted's default implementation of isUserAllowed returns true for all accounts except those marked as BLOCKED, which provides the blocklist behavior out of the box. The override in allowlist mode is necessary to change this to check for Restriction.ALLOWED instead.
Both modes expose the same public API—only allowlist requires the override to implement its custom logic. My initial concern was unfounded. Thank you for the explanation!
(\__/)
(='.'=)
(")_(") *learned something new!*
✏️ Learnings added
Learnt from: typicalHuman
Repo: OpenZeppelin/contracts-wizard PR: 715
File: packages/core/solidity/src/stablecoin.ts:74-77
Timestamp: 2025-11-03T12:18:33.767Z
Learning: In packages/core/solidity/src/stablecoin.ts, the ERC20Restricted base contract from openzeppelin/community-contracts provides a default implementation of isUserAllowed that returns true for non-BLOCKED users (blocklist logic). Only allowlist mode needs to override isUserAllowed to check for Restriction.ALLOWED instead. Blocklist mode uses the default implementation without override.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
ericglau
changed the title
ericglau
left a comment
ericglau
left a comment
There was a problem hiding this comment.
Thanks! Just a few minor suggestions.
Co-authored-by: Eric Lau <ericglau@outlook.com>
ericglau
left a comment
ericglau
left a comment
There was a problem hiding this comment.
Due to type checking, packages/ui/api/ai-assistant/function-definitions/solidity.ts also needs to be updated from limitations->restrictions.
3 participants
Fixes #713