Add ERC7984Restricted extension
#182
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for confidential-tokens ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| @@ -0,0 +1,94 @@ | |||
| // SPDX-License-Identifier: MIT | |||
Contributor
There was a problem hiding this comment.
Why not something like:
- https://github.com/OpenZeppelin/openzeppelin-community-contracts or https://github.com/OpenZeppelin/openzeppelin-contracts
abstract contract Restricted {
enum Restriction {
DEFAULT, // User has no explicit restriction
BLOCKED, // User is explicitly blocked
ALLOWED // User is explicitly allowed
}
// [...]
}
abstract contract ERC20Restricted is ERC20, Restricted {
function _update(address from, address to, uint256 value) internal virtual override {
if (from != address(0)) _checkRestriction(from); // Not minting
if (to != address(0)) _checkRestriction(to); // Not burning
super._update(from, to, value);
}
}
abstract contract ERC7984Restricted is ERC7984, Restricted {
function _update(address from, address to, euint64 value) internal virtual override returns (euint64) {
if (from != address(0)) _checkRestriction(from); // Not minting
if (to != address(0)) _checkRestriction(to); // Not burning
return super._update(from, to, value);
}
}
considering core restriction behavior is no related here to confidentiality
?
Contributor
Author
There was a problem hiding this comment.
I suggested this to @ernestognw, and he preferred to keep them separate for now. But agree that would be a good reuse of code.
arr00
commented
Aug 26, 2025
Contributor
Author
|
@sourcery-ai review |
Reviewer's GuideThis PR introduces the ERC7984Restricted extension to add per-account transfer restrictions (DEFAULT, BLOCKED, ALLOWED) into ERC7984 tokens, integrates restriction checks into core operations, provides a mock for testing, includes comprehensive E2E tests for various restriction scenarios, and updates documentation and release notes. Sequence diagram for restricted transfer operationsequenceDiagram
participant Sender
participant ERC7984Restricted
participant Receiver
Sender->>ERC7984Restricted: transfer(to, amount)
ERC7984Restricted->>ERC7984Restricted: _checkRestriction(Sender)
ERC7984Restricted->>ERC7984Restricted: _checkRestriction(Receiver)
ERC7984Restricted->>ERC7984Restricted: _update(Sender, Receiver, amount)
ERC7984Restricted-->>Sender: Transfer success or revert (UserRestricted)
Entity relationship diagram for user restriction stateserDiagram
USER {
address id
enum restriction
}
RESTRICTION {
string state
}
USER ||--|{ RESTRICTION : has
RESTRICTION {
string DEFAULT
string BLOCKED
string ALLOWED
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
SourceryAI
reviewed
Aug 26, 2025
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `contracts/token/ERC7984/extensions/ERC7984Restricted.sol:91` </location>
<code_context>
+ }
+
+ /// @dev Checks if a user account is restricted. Reverts with {ERC20Restricted} if so.
+ function _checkRestriction(address account) internal view virtual {
+ require(isUserAllowed(account), UserRestricted(account));
+ }
+}
</code_context>
<issue_to_address>
Custom error is used in a require statement, which may not provide optimal revert data.
Use 'revert UserRestricted(account);' instead to ensure the error selector and arguments are returned, improving error handling and reducing gas costs.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
function _checkRestriction(address account) internal view virtual {
require(isUserAllowed(account), UserRestricted(account));
}
=======
function _checkRestriction(address account) internal view virtual {
if (!isUserAllowed(account)) {
revert UserRestricted(account);
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `test/token/ERC7984/extensions/ERC7984Restricted.test.ts:70` </location>
<code_context>
+ });
+
+ describe('restricted token operations', function () {
+ describe('transfer', function () {
+ it('allows transfer when sender and recipient have DEFAULT restriction', async function () {
+ await this.token.connect(this.holder).transfer(this.recipient, initialSupply);
+ });
</code_context>
<issue_to_address>
Consider adding a test for transferring to self.
Please add a test where the sender and recipient are the same address, covering various restriction states, to verify correct contract behavior.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
describe('restricted token operations', function () {
=======
describe('restricted token operations', function () {
describe('transfer to self', function () {
it('allows transfer to self when restriction is DEFAULT', async function () {
// Ensure holder has DEFAULT restriction
await this.token.$_setUserRestriction(this.holder.address, 0); // 0 = DEFAULT
await expect(
this.token.connect(this.holder).transfer(this.holder.address, initialSupply)
).to.not.be.reverted;
});
it('reverts transfer to self when restriction is BLOCKED', async function () {
// Set holder to BLOCKED restriction
await this.token.$_blockUser(this.holder.address); // BLOCKED
await expect(
this.token.connect(this.holder).transfer(this.holder.address, initialSupply)
).to.be.revertedWith('ERC7984: sender is blocked');
});
});
>>>>>>> REPLACE
</suggested_fix>Hi @arr00! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.| import {FHE, euint64, externalEuint64} from "@fhevm/solidity/lib/FHE.sol"; | ||
| import {ERC7984Restricted} from "../../token/ERC7984/extensions/ERC7984Restricted.sol"; | ||
|
|
||
| abstract contract ERC7984RestrictedMock is ERC7984Restricted, SepoliaConfig { |
Contributor
There was a problem hiding this comment.
How about adding another mock/tests for testing allowlist.
Contributor
Author
There was a problem hiding this comment.
As in override isUserAllowed and test with a different config? I don't think that's necessary.
Contributor
There was a problem hiding this comment.
Some ways to test what suggests our doc yes:
* function isUserAllowed(address account) public view virtual override returns (bool) {
* return getRestriction(account) == Restriction.ALLOWED;
* }
but ok
Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces ERC7984Restricted extension adding per-account transfer restrictions to ERC7984; adds an abstract mock for testing, updates token README to list the extension, includes a changeset entry, and adds tests covering restriction states, events, transfers, transferFrom, mint, and burn. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Caller
participant T as ERC7984Restricted
participant S as ERC7984 (super)
rect rgba(220,240,255,0.5)
note over User,T: Transfer flow
User->>T: transfer(from, to, amount)
T->>T: _update(from, to, euint64)
T->>T: _checkRestriction(from) if not mint
T->>T: _checkRestriction(to) if not burn
alt Allowed (DEFAULT/ALLOWED)
T->>S: super._update(from, to, euint64)
S-->>T: balances updated
T-->>User: success
else Blocked
T-->>User: revert UserRestricted(account)
end
end
rect rgba(235,255,235,0.5)
note over User,T: Mint flow
User->>T: _mint(to, euint64)
T->>T: _checkRestriction(to)
alt Allowed
T->>S: super._update(0x0, to, euint64)
S-->>T: minted
else Blocked
T-->>User: revert UserRestricted(to)
end
end
rect rgba(255,235,235,0.5)
note over User,T: Burn flow
User->>T: _burn(from, euint64)
T->>T: _checkRestriction(from)
alt Allowed
T->>S: super._update(from, 0x0, euint64)
S-->>T: burned
else Blocked
T-->>User: revert UserRestricted(from)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (7)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
test/token/ERC7984/extensions/ERC7984Restricted.test.ts (2)
110-111: Minor: prefer destructuring for readability.Matches prior suggestion; keeps code concise.
- const timestamp = (await ethers.provider.getBlock('latest')).timestamp; + const { timestamp } = await ethers.provider.getBlock('latest');
71-81: Optionally assert post-conditions (event or state).Currently success is inferred by lack of revert. Consider asserting an expected event or observable state to tighten tests.
contracts/token/ERC7984/extensions/ERC7984Restricted.sol (1)
91-93: Compile error:requirecannot use a custom error. Userevert UserRestricted(account).This currently won’t compile;
requireonly accepts a string for the second arg.- /// @dev Checks if a user account is restricted. Reverts with {ERC20Restricted} if so. + /// @dev Checks if a user account is restricted. Reverts with {UserRestricted} if so. function _checkRestriction(address account) internal view virtual { - require(isUserAllowed(account), UserRestricted(account)); + if (!isUserAllowed(account)) { + revert UserRestricted(account); + } }
🧹 Nitpick comments (6)
.changeset/fruity-bananas-smile.md (1)
5-5: Clarify release note with event and error names.Add concrete identifiers to aid consumers scanning changelogs.
-`ERC7984Restricted`: An extension of `ERC7984` that implements user account transfer restrictions. +`ERC7984Restricted`: An extension of `ERC7984` that implements user account transfer restrictions (emits `UserRestrictionUpdated`, reverts with `UserRestricted`).contracts/token/README.adoc (1)
9-12: Standardize phrasing: “An extension of {ERC7984} …”.Line 9 still uses “Extension of” while the rest use “An extension of”. Align for consistency.
-- {ERC7984ERC20Wrapper}: Extension of {ERC7984} which wraps an `ERC20` into a confidential token. The wrapper allows for free conversion in both directions at a fixed rate. +- {ERC7984ERC20Wrapper}: An extension of {ERC7984} which wraps an `ERC20` into a confidential token. The wrapper allows for free conversion in both directions at a fixed rate.contracts/mocks/token/ERC7984RestrictedMock.sol (1)
6-7: Remove unused imports.
euint64andexternalEuint64are not referenced in this mock.-import {FHE, euint64, externalEuint64} from "@fhevm/solidity/lib/FHE.sol"; +import {FHE} from "@fhevm/solidity/lib/FHE.sol";test/token/ERC7984/extensions/ERC7984Restricted.test.ts (2)
10-14: Be explicit passing addresses to contract methods.Passing
Signercan work in ethers v6, but using.addressavoids ambiguity with different toolchains.- const token = await ethers.deployContract('$ERC7984RestrictedMock', ['token', 'tk', 'uri']); - await token['$_mint(address,uint64)'](holder, initialSupply); + const token = await ethers.deployContract('$ERC7984RestrictedMock', ['token', 'tk', 'uri']); + await token['$_mint(address,uint64)'](holder.address, initialSupply);Apply similarly where
this.holder/this.recipientare passed to address params.
69-104: Add a self-transfer test case.Covers an edge path and ensures restrictions apply consistently when
from == to.I can draft a test under “transfer” asserting:
- DEFAULT: self-transfer allowed
- BLOCKED: self-transfer reverts with
UserRestricted(holder)
Want me to add a patch?contracts/token/ERC7984/extensions/ERC7984Restricted.sol (1)
53-60: Docstring contradicts implementation (mint/burn are checked).The function enforces restrictions for both mint (checks
to) and burn (checksfrom). Fix the comment.- * @dev See {ERC7984-_update}. Enforces transfer restrictions (excluding minting and burning). + * @dev See {ERC7984-_update}. Enforces restrictions on transfers, minting (checks `to`) and burning (checks `from`).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.changeset/fruity-bananas-smile.md(1 hunks)contracts/mocks/token/ERC7984RestrictedMock.sol(1 hunks)contracts/token/ERC7984/extensions/ERC7984Restricted.sol(1 hunks)contracts/token/README.adoc(2 hunks)test/token/ERC7984/extensions/ERC7984Restricted.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/token/ERC7984/extensions/ERC7984Restricted.test.ts (1)
test/utils/structs/Checkpoints.test.js (1)
fixture(10-35)
🔇 Additional comments (5)
contracts/token/README.adoc (1)
12-12: Docs entry looks good.Accurate, succinct description of the new extension.
contracts/mocks/token/ERC7984RestrictedMock.sol (1)
18-20: Public transfer wrapper is fine.Simple helper for tests; no reentrancy or value transfers here. LGTM.
contracts/token/ERC7984/extensions/ERC7984Restricted.sol (3)
61-65: Update hook logic looks correct.Checks both ends except the zero-address side, then delegates to parent. Matches tests.
25-27: Good event design (indexed account, enum payload).Efficient to filter and audit.
49-51: Reasonable default policy.Blocklist semantics by default; clearly overridable to allowlist.
james-toussaint
previously approved these changes
Aug 28, 2025
| import {FHE, euint64, externalEuint64} from "@fhevm/solidity/lib/FHE.sol"; | ||
| import {ERC7984Restricted} from "../../token/ERC7984/extensions/ERC7984Restricted.sol"; | ||
|
|
||
| abstract contract ERC7984RestrictedMock is ERC7984Restricted, SepoliaConfig { |
Contributor
There was a problem hiding this comment.
Some ways to test what suggests our doc yes:
* function isUserAllowed(address account) public view virtual override returns (bool) {
* return getRestriction(account) == Restriction.ALLOWED;
* }
but ok
james-toussaint
approved these changes
Aug 28, 2025
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #184
Summary by Sourcery
Add ERC7984Restricted extension for managing user transfer restrictions in ERC7984 tokens, accompanied by a mock implementation, comprehensive tests, and a changeset entry.
New Features:
Enhancements:
Tests:
Chores:
Summary by CodeRabbit