Support compliance modules for confidential RWAs #197
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
I would follow function naming from the existing ERC3643 module https://github.com/ERC-3643/ERC-3643/blob/main/contracts/compliance/modular/IModularCompliance.sol |
…eature/confidential-rwa-compliance
✅ Deploy Preview for confidential-tokens ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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
🧹 Nitpick comments (8)
contracts/mocks/token/ERC7984RwaComplianceModuleMock.sol (1)
18-24: Consider conventional naming for test helper functions.The
$_prefix is unconventional in Solidity outside of Foundry's internal testing conventions. Consider using more standard names likesetCompliant()andunsetCompliant()for clarity, unless this prefix is an established convention in your codebase.contracts/token/ERC7984/extensions/rwa/ERC7984RwaInvestorCapModule.sol (2)
59-71: Consider adding admin access to investor count for monitoring.The
_postTransferlogic correctly maintains the investor count:
- Increments when a recipient receives their first tokens (balance equals transferred amount)
- Decrements when a sender's balance reaches zero
However, line 69 only calls
FHE.allowThis(_investors), which allows the contract itself to read the encrypted count. Consider also granting access to token admins for monitoring purposes, similar to how the test suite expects admins to decrypt the current investor count (see test line 379).Add after line 69:
_investors = FHE.select(FHE.eq(balance, FHE.asEuint64(0)), FHE.sub(_investors, FHE.asEuint64(1)), _investors); FHE.allowThis(_investors); +FHE.allow(_investors, _token);This would allow the token contract (and by extension, its admins through
getHandleAllowance) to decrypt the investor count.
64-70: Refine investor‐count logic
- Ensure FHE lazy‐initialization here matches other modules so
_investorsis never used uninitialized.- Handle zero‐amount transfers explicitly or document that when
encryptedAmount == 0, both selects fire but net effect is zero.- Prevent potential underflow in
FHE.sub(_investors, FHE.asEuint64(1))by guarding_investors > 0or adding an assertion.contracts/token/ERC7984/extensions/rwa/ERC7984RwaModularCompliance.sol (2)
35-35: Consider consistent error naming.The error
SenderNotComplianceModuledoesn't follow theERC7984Rwaprefix convention used by other errors in this contract (lines 27-33). For consistency, consider renaming toERC7984RwaSenderNotComplianceModule.Apply this diff:
- error SenderNotComplianceModule(address account); + error ERC7984RwaSenderNotComplianceModule(address account);And update line 211:
- SenderNotComplianceModule(msg.sender) + ERC7984RwaSenderNotComplianceModule(msg.sender)
149-187: Clarify and enforce non-mutating compliance checks
The public isCompliantTransfer wrapper intentionally updates the FHE context via FHE.allow. To preserve the intended side-effect isolation, require that each module’s internal _isCompliantTransfer implementation be pure or view with no state mutations. Update the IERC7984RwaComplianceModule interface documentation accordingly and audit existing modules (e.g., BalanceCapModule, InvestorCapModule) to confirm compliance.contracts/token/ERC7984/extensions/rwa/ERC7984RwaBalanceCapModule.sol (1)
6-6: Remove unused import.The
EnumerableSetimport on line 6 is not used in this contract. Consider removing it to keep imports clean.Apply this diff:
-import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";Also remove the
usingdirective on line 15:- using EnumerableSet for *;contracts/interfaces/IERC7984Rwa.sol (1)
86-94: Consider strengthening non-mutating guidance.The interface correctly implements the pre-check (isCompliantTransfer) and post-hook (postTransfer) pattern from ERC-3643. However, the "should be non-mutating" comment on line 90 is advisory only. Module implementations could violate this intent, leading to unexpected state changes during compliance checks.
Consider adding this guidance to the interface documentation:
/// @dev Interface for confidential RWA transfer compliance module. +/// IMPORTANT: isCompliantTransfer should NOT mutate state. State mutations should be +/// performed in postTransfer hooks. This separation ensures predictable compliance checks. interface IERC7984RwaComplianceModule {Based on learnings from ERC-3643 and PR comments.
contracts/token/ERC7984/extensions/rwa/ERC7984RwaComplianceModule.sol (1)
48-50: Add zero address validation.The constructor does not validate that
tokenis not the zero address. This could lead to deployment issues that are difficult to debug.Apply this diff to add validation:
constructor(address token) { + require(token != address(0), "Token address cannot be zero"); _token = token; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.changeset/wet-results-doubt.md(1 hunks)contracts/interfaces/IERC7984Rwa.sol(2 hunks)contracts/mocks/token/ERC7984RwaBalanceCapModuleMock.sol(1 hunks)contracts/mocks/token/ERC7984RwaComplianceModuleMock.sol(1 hunks)contracts/mocks/token/ERC7984RwaInvestorCapModuleMock.sol(1 hunks)contracts/mocks/token/ERC7984RwaModularComplianceMock.sol(1 hunks)contracts/token/ERC7984/extensions/ERC7984Rwa.sol(1 hunks)contracts/token/ERC7984/extensions/rwa/ERC7984RwaBalanceCapModule.sol(1 hunks)contracts/token/ERC7984/extensions/rwa/ERC7984RwaComplianceModule.sol(1 hunks)contracts/token/ERC7984/extensions/rwa/ERC7984RwaInvestorCapModule.sol(1 hunks)contracts/token/ERC7984/extensions/rwa/ERC7984RwaModularCompliance.sol(1 hunks)test/token/ERC7984/extensions/ERC7984RwaModularCompliance.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/token/ERC7984/extensions/ERC7984RwaModularCompliance.test.ts (1)
test/helpers/event.ts (1)
callAndGetResult(5-9)
🔇 Additional comments (46)
.changeset/wet-results-doubt.md (1)
1-5: LGTM!The changeset correctly documents the new modular compliance feature with an appropriate minor version bump.
contracts/token/ERC7984/extensions/ERC7984Rwa.sol (1)
69-72: LGTM!The
isAdminOrAgentfunction provides a clean convenience method for combined role checks, which will be useful for modular compliance module management. The implementation correctly delegates to existing role check functions and is marked virtual to allow overrides.contracts/mocks/token/ERC7984RwaModularComplianceMock.sol (1)
1-17: LGTM!The mock contract correctly combines
ERC7984RwaModularCompliancewithSepoliaConfigfor testing. Constructor delegation to parent contracts is accurate and follows the established pattern used in other mocks in this PR.contracts/mocks/token/ERC7984RwaInvestorCapModuleMock.sol (1)
1-10: LGTM!The mock correctly inherits from
ERC7984RwaInvestorCapModuleandSepoliaConfig, with proper constructor delegation. The implementation is minimal and appropriate for testing purposes.contracts/mocks/token/ERC7984RwaBalanceCapModuleMock.sol (1)
1-19: LGTM!The mock extends
ERC7984RwaBalanceCapModuleappropriately for testing. ThecreateEncryptedAmounthelper function provides a useful test utility that:
- Converts plaintext amounts to encrypted euint64 values
- Properly authorizes the encrypted value for both the contract and the caller
- Emits an event for test verification
The FHE authorization pattern is correct for test scenarios where both the contract and the caller need access to the encrypted value.
contracts/mocks/token/ERC7984RwaComplianceModuleMock.sol (3)
10-10: LGTM! Mock contract structure is appropriate for testing.The contract correctly extends the base compliance module and provides a simple boolean flag to simulate compliance state for testing purposes.
13-16: LGTM! Events and constructor are correctly implemented.The parameterless events are suitable for testing module hook invocation, and the constructor properly delegates to the base module.
26-37: LGTM! Mock correctly implements the two-function compliance pattern.The implementation follows the ERC-3643-inspired pattern suggested in PR comments: a pre-transfer check (
_isCompliantTransfer) and a post-transfer hook (_postTransfer). The event emissions enable test verification of module invocation.test/token/ERC7984/extensions/ERC7984RwaModularCompliance.test.ts (5)
16-52: LGTM! Comprehensive fixture setup.The fixture properly deploys the token, multiple compliance modules, and configures encrypted inputs. Good organization for testing various module types and scenarios.
54-115: LGTM! Thorough module lifecycle testing.Excellent coverage of module support, installation, uninstallation, access control, and edge cases. The parameterized approach ensures both module types are tested consistently.
117-185: LGTM! Comprehensive compliance flow testing.Excellent coverage of the core compliance mechanism with all combinations of force transfer and compliance states. The event assertions correctly verify that
transferOnlymodules are bypassed during force transfers whilealwaysOnmodules are always invoked.
187-358: LGTM! Comprehensive balance cap module testing.Thorough coverage of the balance cap module including:
- Both encrypted input paths (with and without proof)
- Access control validation
- Compliant and non-compliant transfer scenarios
- Edge case handling (burning bypasses cap)
The test suite correctly verifies that transfers exceeding the balance cap result in zero transfer.
360-451: LGTM! Thorough investor cap module testing.Comprehensive coverage of the investor cap module including:
- Transfer compliance when cap is reached (correctly expects zero transfer)
- Current investor tracking via encrypted counter
- Admin functions and access control
- Edge cases (transferring to existing investors)
Note: Fix the
maxInverstortypo on lines 444 and 449 as mentioned in the earlier comment.contracts/token/ERC7984/extensions/rwa/ERC7984RwaInvestorCapModule.sol (3)
1-20: LGTM! Well-structured investor cap module.The contract structure is sound with appropriate state variables: plaintext max investor limit and encrypted current investor count. The constructor correctly initializes the base module and sets the initial cap.
22-36: LGTM! Public interface is well-designed.The functions provide appropriate access:
- Admin-only setter with event emission
- Public getters for both max and current investor counts
- Correctly returns encrypted current count to preserve privacy
38-57: Verify the compliance logic handles edge cases correctly.The investor cap logic appears sound but should be verified:
Burning check (line 49): Uses
FHE.asEbool(to == address(0))to check for zero address in plaintext. This is acceptable since the recipient address is not encrypted, but verify this pattern is consistent with your FHE design.Existing investor check (line 53): The condition
FHE.gt(balance, FHE.asEuint64(0))correctly identifies existing investors. However, consider the edge case where an investor's balance was reduced to zero in a previous transaction but they're still counted in_investors. The current logic would allow them to receive tokens again without incrementing the counter, which may be the intended behavior for "reactivating" an investor slot.Race condition consideration: In a concurrent environment, multiple transactions could potentially increment
_investorssimultaneously if they all pass theFHE.lt(_investors, FHE.asEuint64(_maxInvestor))check before any completes. While FHE operations are atomic within a transaction, verify this behavior aligns with your threat model.Consider documenting the intended behavior for these edge cases, particularly the "reactivation" scenario for investors whose balance returns from zero.
contracts/token/ERC7984/extensions/rwa/ERC7984RwaModularCompliance.sol (8)
1-16: LGTM!The contract structure and imports are well-organized. The extension pattern appropriately builds on ERC7984Rwa, and the ERC-7579 inspiration is documented.
18-19: LGTM!EnumerableSet is the right choice for tracking modules, enabling efficient iteration and membership checks.
37-52: LGTM!The query functions provide clean public interfaces with appropriate visibility and extensibility.
54-66: LGTM!The admin-only module management with gas footprint warning is well-designed. The delegation to internal functions enables safe extension.
78-94: LGTM!The module validation using staticcall to
isModule()and checking the returned magic value is a solid pattern for verifying module compatibility. The use of EnumerableSet's return values to detect duplicate installs is idiomatic.
111-147: LGTM! Design aligns with ERC-3643 pattern.The pre-check and post-hook pattern correctly implements the modular compliance flow. The differentiation between regular transfers (AlwaysOn AND TransferOnly) and force transfers (AlwaysOn only) is well-designed. Using
FHE.selectto zero non-compliant transfers is an elegant FHE-native approach.Based on learnings from ERC-3643 and PR comments.
189-205: LGTM!The post-transfer hooks correctly execute after balance updates, allowing modules to track state (e.g., investor counts). The pattern aligns with ERC-3643's modular compliance approach.
Based on learnings from ERC-3643 and PR comments.
207-213: LGTM!The handle access validation correctly restricts access to installed modules only, ensuring that only authorized compliance modules can query encrypted balances.
contracts/token/ERC7984/extensions/rwa/ERC7984RwaBalanceCapModule.sol (4)
17-19: LGTM!The private encrypted balance cap with corresponding event follows good patterns for confidential state management.
25-36: LGTM!The dual overloads for setting max balance (external with proof vs. internal encrypted) provide good flexibility. The
onlyTokenAdminrestriction andFHE.allowThiscalls are appropriate.
38-41: LGTM!Simple getter for the encrypted max balance.
43-58: LGTM!The balance cap compliance logic is well-implemented:
- Burns are correctly exempted (line 49-51)
- Uses
tryIncreaseto safely compute future balance and detect overflow- Checks both overflow protection and cap enforcement (line 56)
The unused
fromparameter is appropriate since balance caps only concern recipients.contracts/interfaces/IERC7984Rwa.sol (3)
4-4: LGTM!The FHE type imports are necessary for the new compliance module interfaces.
12-17: LGTM!The role query functions provide necessary interfaces for compliance modules to validate permissions. The
isAdminOrAgentconvenience function reduces redundant checks.
71-84: LGTM!The modular compliance interface provides a clean API for module management. The two-tier module system (AlwaysOn for all transfers, TransferOnly for regular transfers) offers good flexibility.
contracts/token/ERC7984/extensions/rwa/ERC7984RwaComplianceModule.sol (15)
1-3: LGTM!License and pragma version are appropriate.
5-7: LGTM!Imports are minimal and necessary for the compliance module implementation.
9-13: LGTM!The contract correctly declares an abstract base with appropriate inheritance and immutable token binding.
15-22: LGTM!Custom errors are well-defined and provide helpful debugging information.
24-28: LGTM!The
onlyTokenmodifier correctly restricts access to the token contract.
30-34: LGTM!The
onlyTokenAdminmodifier correctly delegates admin verification to the token contract.
36-40: LGTM!The
onlyTokenAgentmodifier correctly delegates agent verification to the token contract.
42-46: LGTM!The
onlyTokenAdminOrTokenAgentmodifier efficiently checks both roles with a single call.
52-55: LGTM!The
isModulefunction provides a standard mechanism for module identification via selector introspection.
67-69: LGTM!The
postTransferhook correctly provides a post-transfer mutation point restricted to the token contract, aligning with the two-phase compliance check pattern (pre-check + post-hook) mentioned in the PR objectives.
72-76: LGTM!The internal
_isCompliantTransferhook is correctly defined as an abstract method that derived modules must implement.
79-81: LGTM!The default no-op implementation for
_postTransferis appropriate, allowing derived modules to opt-in to post-transfer state updates.
84-93: LGTM!The handle allowance helpers correctly manage FHE handle access with appropriate initialization checks and flexible persistence options.
95-95: Ignore the empty override—base implementation is abstract. The baseHandleAccessManagerdeclares_validateHandleAllowanceas an unimplemented abstract function, so this override—guarded byonlyTokenAdminOrTokenAgent—is the intended validation; no additional handle checks are required.Likely an incorrect or invalid review comment.
57-64: No renaming—keep ERC-7984’sisCompliantTransferandpostTransfer
The module’s hook names must match the ERC-7984 interface (isCompliantTransfer/postTransfer), not ERC-3643’scanTransfer/transferred. Changing them would break the standard’s spec.Likely an incorrect or invalid review comment.
| constructor(address token) ERC7984RwaComplianceModule(token) { | ||
| _token = token; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify redundant assignment.
Line 22 assigns _token = token, but the parent constructor ERC7984RwaComplianceModule(token) is called on line 21. If the parent constructor already sets _token, this assignment is redundant.
Check the parent contract to confirm:
🏁 Script executed:
#!/bin/bash
# Description: Check if ERC7984RwaComplianceModule constructor already sets _token
# Find the ERC7984RwaComplianceModule constructor
ast-grep --pattern $'contract ERC7984RwaComplianceModule {
$$$
constructor(address $PARAM) {
$$$
}
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Locate the ERC7984RwaComplianceModule contract
rg -n 'contract ERC7984RwaComplianceModule' -g 'contracts/token/ERC7984/extensions/rwa/ERC7984RwaComplianceModule.sol'
# Show lines around its constructor
rg -n 'constructor' -C3 -g 'contracts/token/ERC7984/extensions/rwa/ERC7984RwaComplianceModule.sol'Length of output: 764
Remove redundant assignment in constructor
The parent ERC7984RwaComplianceModule constructor already sets _token, so the child’s _token = token; can be removed.
🤖 Prompt for AI Agents
In contracts/token/ERC7984/extensions/rwa/ERC7984RwaBalanceCapModule.sol around
lines 21 to 23, the constructor redundantly reassigns _token after calling the
parent ERC7984RwaComplianceModule(token) which already sets it; remove the line
"_token = token;" from the child constructor so the parent initialization is
relied upon and no duplicate assignment remains.
| const alwaysOnType = 0; | ||
| const transferOnlyType = 1; | ||
| const moduleTypes = [alwaysOnType, transferOnlyType]; | ||
| const maxInverstor = 2; |
There was a problem hiding this comment.
Fix typo in constant name.
The constant is named maxInverstor but should be maxInvestor. This typo is propagated to lines 30, 444, and 449.
Apply this diff to fix the typo:
-const maxInverstor = 2;
+const maxInvestor = 2;Also update references on lines 30, 444, and 449.
📝 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.
| const maxInverstor = 2; | |
| const maxInvestor = 2; |
🤖 Prompt for AI Agents
In test/token/ERC7984/extensions/ERC7984RwaModularCompliance.test.ts around line
12 (and refs at lines 30, 444, 449), the constant is misspelled as
`maxInverstor`; rename the declaration to `maxInvestor` and update all usages at
lines 30, 444, and 449 to use `maxInvestor` so the identifier is consistent and
correctly spelled throughout the file.
2 participants
Summary by Sourcery
Support pluggable compliance modules for confidential RWAs by defining new module interfaces, extending ERC7984Rwa to install and enforce identity and transfer compliance checks, and unifying the compliance hook naming
New Features:
Enhancements:
Chores:
Summary by CodeRabbit
New Features
Tests
Chores