RWA Modular Compliance

[arr00]

Summary by CodeRabbit

Release Notes

• New Features

  • Introduced a modular compliance framework for confidential Real World Assets, enabling dynamic installation and uninstallment of compliance modules.
  • Added support for two module types: Standard and ForceTransfer, with customizable pre- and post-transfer compliance checks.
  • Implemented admin-controlled module management with comprehensive compliance validation on transfers.

• Tests

  • Added comprehensive test suite for modular compliance functionality.

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@contracts/finance/compliance/ComplianceModuleConfidential.sol`:
- Around line 41-43: onInstall and onUninstall are declared public virtual with
no restrictions allowing any external caller to invoke lifecycle hooks; change
them to enforce access control by adding and using a modifier (e.g., onlyAgent
or onlyRegisteredToken) that checks msg.sender is an authorized contract/agent
(or validates registration in the token registry), then apply that modifier to
both onInstall and onUninstall while keeping them virtual so overrides can call
super; implement the modifier logic (authorization check and revert) in the same
contract (or a shared base) and reference the modifier in the function
signatures for onInstall and onUninstall.

In `@contracts/token/ERC7984/extensions/rwa/ERC7984RwaModularCompliance.sol`:
- Around line 224-230: The override _validateHandleAllowance currently has no
return but the base HandleAccessManager._validateHandleAllowance returns bool;
change the function signature to return bool and return true on success. Keep
the existing require check using
_forceTransferComplianceModules.contains(msg.sender) ||
_complianceModules.contains(msg.sender) with
SenderNotComplianceModule(msg.sender) for the revert, then return true (or
return the boolean expression) so the override matches the base return type.

In `@test/token/ERC7984/extensions/ERC7984RwaModularCompliance.test.ts`:
- Around line 57-67: The negative test "should not support other module types"
is inside the for-loop and runs twice; move that it(...) block outside the for
(const type of [ModuleType.Standard, ModuleType.ForceTransfer]) loop so the loop
only contains the per-type test `it(\`should support module type
${ModuleType[type]}\`, ...)` and the `it('should not support other module
types', ...)` runs once; update the block boundaries around the loop and adjust
indentation accordingly so `this.token.supportsModule(3)` is asserted once.

🧹 Nitpick comments (6)

contracts/mocks/token/ComplianceModuleConfidentialMock.sol (2)

3-3: Pragma version inconsistency with parent contract.

This mock uses ^0.8.24 while the parent ComplianceModuleConfidential uses ^0.8.27. This could lead to compilation issues or unexpected behavior if language features differ between versions. Consider aligning to ^0.8.27.

♻️ Proposed fix

-pragma solidity ^0.8.24;
+pragma solidity ^0.8.27;

---

42-52: assert usage in mock may mask test failures.

Line 47 uses assert(FHE.isAllowed(...)) which consumes all remaining gas on failure. For a mock used in testing, require with a descriptive error message would provide better debugging feedback when handle access isn't properly granted.

♻️ Proposed fix

-            assert(FHE.isAllowed(fromBalance, address(this)));
+            require(FHE.isAllowed(fromBalance, address(this)), "Handle allowance not granted");

contracts/token/ERC7984/extensions/rwa/ERC7984RwaModularCompliance.sol (1)

134-135: Low-level call return value intentionally ignored.

The comment at line 134 explains the intent, but the compiler warning persists. You can explicitly discard the return value to silence the warning while preserving the intentional behavior.

♻️ Proposed fix to silence warning

         // ignore success purposely to avoid modules that revert on uninstall
-        module.call(abi.encodeCall(IERC7984RwaComplianceModule.onUninstall, (deinitData)));
+        (bool success, ) = module.call(abi.encodeCall(IERC7984RwaComplianceModule.onUninstall, (deinitData)));
+        success; // Explicitly ignore - modules may revert on uninstall

contracts/mocks/token/ERC7984RwaModularComplianceMock.sol (1)

3-3: Pragma version inconsistency with parent contract.

Same issue as ComplianceModuleConfidentialMock.sol: this mock uses ^0.8.24 while parent ERC7984RwaModularCompliance uses ^0.8.27.

♻️ Proposed fix

-pragma solidity ^0.8.24;
+pragma solidity ^0.8.27;

test/token/ERC7984/extensions/ERC7984RwaModularCompliance.test.ts (2)

4-4: Unused import.

time from @nomicfoundation/hardhat-network-helpers is imported but never used in this test file.

♻️ Proposed fix

-import { time } from '@nomicfoundation/hardhat-network-helpers';

---

16-34: Unused fixture function.

The fixture function is defined but never used. The beforeEach block (lines 37-55) duplicates the same setup logic. Consider either using the fixture pattern with loadFixture for snapshot-based test isolation, or remove the unused function.

♻️ Option 1: Use loadFixture for better test isolation

+import { loadFixture } from '@nomicfoundation/hardhat-network-helpers';
+
 const fixture = async () => {
   // ... existing fixture code
 };

 describe('ERC7984RwaModularCompliance', function () {
   beforeEach(async function () {
-    const [admin, agent1, agent2, holder, recipient, anyone] = await ethers.getSigners();
-    const token = (
-      await ethers.deployContract('$ERC7984RwaModularComplianceMock', ['name', 'symbol', 'uri', admin])
-    ).connect(anyone) as $ERC7984RwaModularCompliance;
-    await token.connect(admin).addAgent(agent1);
-    const complianceModule = await ethers.deployContract('$ComplianceModuleConfidentialMock');
-
-    Object.assign(this, {
-      token,
-      complianceModule,
-      admin,
-      agent1,
-      agent2,
-      recipient,
-      holder,
-      anyone,
-    });
+    Object.assign(this, await loadFixture(fixture));
   });

♻️ Option 2: Remove unused fixture

-const fixture = async () => {
-  const [admin, agent1, agent2, holder, recipient, anyone] = await ethers.getSigners();
-  const token = (
-    await ethers.deployContract('$ERC7984RwaModularComplianceMock', ['name', 'symbol', 'uri', admin])
-  ).connect(anyone) as $ERC7984RwaModularCompliance;
-  await token.connect(admin).addAgent(agent1);
-  const complianceModule = await ethers.deployContract('$ComplianceModuleConfidentialMock');
-
-  return {
-    token,
-    holder,
-    complianceModule,
-    admin,
-    agent1,
-    agent2,
-    recipient,
-    anyone,
-  };
-};
-

[ernestognw]

Question about the original ERC7984Rwa contract: should the admin role be the DEFAULT_ADMIN_ROLE or another admin dedicated to agents?

Let's say I extend this contract and I add another role called PAUSER, so that I want the pauser to be managed by a PAUSER_ADMIN role. However, by leaving the current admin as DEFAULT_ADMIN_ROLE, it would unexpectedly have authority over PAUSER's and PAUSER_ADMIN. In that scenario I think we want to keep admins for agents separated from admins for pausers.

[ernestognw]

I think we can drop the override since this comes from an interface

Suggested change

	function isModule() public pure override returns (bytes4) {
	function isModule() public pure returns (bytes4) {

[arr00]

I honestly prefer to have an override when possible. Why do we have the policy to not?

[coderabbitai]

🧹 Nitpick comments (4)

contracts/interfaces/IERC7984Rwa.sol (1)

106-122: Consider consolidating duplicate interface definitions.

IERC7984RwaComplianceModule has identical method signatures to IComplianceModuleConfidential defined in contracts/interfaces/IComplianceModuleConfidential.sol. Having two identical interfaces can lead to maintenance burden and potential divergence.

Consider either:

1. Having IERC7984RwaComplianceModule extend IComplianceModuleConfidential
2. Using IComplianceModuleConfidential directly throughout the codebase

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@contracts/interfaces/IERC7984Rwa.sol` around lines 106 - 122, The two
interfaces IERC7984RwaComplianceModule and IComplianceModuleConfidential define
identical signatures (isModule, isCompliantTransfer, postTransfer, onInstall,
onUninstall); consolidate them by replacing the duplicate
IERC7984RwaComplianceModule with either an extension or direct reuse of
IComplianceModuleConfidential: update IERC7984RwaComplianceModule to "extend
IComplianceModuleConfidential" or remove IERC7984RwaComplianceModule and update
all references to use IComplianceModuleConfidential, ensuring method names
isModule, isCompliantTransfer, postTransfer, onInstall, and onUninstall remain
available where referenced.

contracts/token/ERC7984/extensions/ERC7984RwaModularCompliance.sol (1)

91-97: Consider explicit return for unsupported module types.

When moduleType is neither ForceTransfer nor Standard, the function returns an uninitialized installed (implicitly false). While this is safe, an explicit return false or a revert would make the behavior clearer.

♻️ Optional: Make the fallback explicit

     function _isModuleInstalled(
         ComplianceModuleType moduleType,
         address module
     ) internal view virtual returns (bool installed) {
         if (moduleType == ComplianceModuleType.ForceTransfer) return _forceTransferComplianceModules.contains(module);
         if (moduleType == ComplianceModuleType.Standard) return _complianceModules.contains(module);
+        return false;
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@contracts/token/ERC7984/extensions/ERC7984RwaModularCompliance.sol` around
lines 91 - 97, The _isModuleInstalled function currently falls through without
an explicit return for unsupported ComplianceModuleType values; update
_isModuleInstalled to explicitly return false (or revert if you prefer stricter
behavior) when moduleType is not ForceTransfer or Standard so the function never
relies on an uninitialized installed value—locate the _isModuleInstalled method
and add a final explicit `return false` (or a revert with a clear message) after
the existing conditionals.

test/token/ERC7984/extensions/ERC7984RwaModularCompliance.test.ts (2)

16-34: Consolidate setup using the existing fixture helper.

Setup is duplicated between fixture and beforeEach; using fixture() directly in beforeEach will reduce drift and keep tests easier to maintain.

♻️ Proposed refactor

-  beforeEach(async function () {
-    const [admin, agent1, agent2, holder, recipient, anyone] = await ethers.getSigners();
-    const token = (
-      await ethers.deployContract('$ERC7984RwaModularComplianceMock', ['name', 'symbol', 'uri', admin])
-    ).connect(anyone) as $ERC7984RwaModularCompliance;
-    await token.connect(admin).addAgent(agent1);
-    const complianceModule = await ethers.deployContract('$ComplianceModuleConfidentialMock');
-
-    Object.assign(this, {
-      token,
-      complianceModule,
-      admin,
-      agent1,
-      agent2,
-      recipient,
-      holder,
-      anyone,
-    });
-  });
+  beforeEach(async function () {
+    Object.assign(this, await fixture());
+  });

Also applies to: 37-55

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/token/ERC7984/extensions/ERC7984RwaModularCompliance.test.ts` around
lines 16 - 34, The test defines a local async fixture function that duplicates
setup already performed in beforeEach; replace the duplicated setup by calling
the shared fixture helper directly in beforeEach and remove the local fixture
function to avoid drift — update references to the returned objects (token,
holder, complianceModule, admin, agent1, agent2, recipient, anyone) so they are
obtained from the existing fixture call (e.g., const { token, holder,
complianceModule, admin, agent1, agent2, recipient, anyone } = await fixture();
or similar), and ensure uses of addAgent and contract connections are performed
inside the central fixture/home fixture helper instead of the local fixture in
ERC7984RwaModularCompliance.test.ts.

---

57-57: Remove async keyword from describe callbacks.

At Lines 57, 69, 112, and 161, describe uses async function. Mocha executes describe callbacks synchronously during suite loading; async callbacks are ignored, and tests registered after an await may not be registered. Move async setup to hooks (before, beforeEach) instead.

Proposed fix

-  describe('support module', async function () {
+  describe('support module', function () {

-  describe('install module', async function () {
+  describe('install module', function () {

-  describe('uninstall module', async function () {
+  describe('uninstall module', function () {

-  describe('check compliance on transfer', async function () {
+  describe('check compliance on transfer', function () {

Also applies to: 69-69, 112-112, 161-161

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/token/ERC7984/extensions/ERC7984RwaModularCompliance.test.ts` at line
57, Remove the async keyword from the describe callbacks (e.g., the
describe('support module', ...) and the other describe blocks flagged) because
Mocha runs describe blocks synchronously; any async/await inside them prevents
proper test registration. Move any asynchronous setup/await logic into before or
beforeEach hooks (create async functions like before(async () => { ... }) or
beforeEach(async () => { ... })) and keep the describe callback plain function()
so all it does is declare nested it/describe calls synchronously; update any
references to setup variables to be assigned in those hooks instead of inside
the describe body.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@contracts/interfaces/IERC7984Rwa.sol`:
- Around line 106-122: The two interfaces IERC7984RwaComplianceModule and
IComplianceModuleConfidential define identical signatures (isModule,
isCompliantTransfer, postTransfer, onInstall, onUninstall); consolidate them by
replacing the duplicate IERC7984RwaComplianceModule with either an extension or
direct reuse of IComplianceModuleConfidential: update
IERC7984RwaComplianceModule to "extend IComplianceModuleConfidential" or remove
IERC7984RwaComplianceModule and update all references to use
IComplianceModuleConfidential, ensuring method names isModule,
isCompliantTransfer, postTransfer, onInstall, and onUninstall remain available
where referenced.

In `@contracts/token/ERC7984/extensions/ERC7984RwaModularCompliance.sol`:
- Around line 91-97: The _isModuleInstalled function currently falls through
without an explicit return for unsupported ComplianceModuleType values; update
_isModuleInstalled to explicitly return false (or revert if you prefer stricter
behavior) when moduleType is not ForceTransfer or Standard so the function never
relies on an uninitialized installed value—locate the _isModuleInstalled method
and add a final explicit `return false` (or a revert with a clear message) after
the existing conditionals.

In `@test/token/ERC7984/extensions/ERC7984RwaModularCompliance.test.ts`:
- Around line 16-34: The test defines a local async fixture function that
duplicates setup already performed in beforeEach; replace the duplicated setup
by calling the shared fixture helper directly in beforeEach and remove the local
fixture function to avoid drift — update references to the returned objects
(token, holder, complianceModule, admin, agent1, agent2, recipient, anyone) so
they are obtained from the existing fixture call (e.g., const { token, holder,
complianceModule, admin, agent1, agent2, recipient, anyone } = await fixture();
or similar), and ensure uses of addAgent and contract connections are performed
inside the central fixture/home fixture helper instead of the local fixture in
ERC7984RwaModularCompliance.test.ts.
- Line 57: Remove the async keyword from the describe callbacks (e.g., the
describe('support module', ...) and the other describe blocks flagged) because
Mocha runs describe blocks synchronously; any async/await inside them prevents
proper test registration. Move any asynchronous setup/await logic into before or
beforeEach hooks (create async functions like before(async () => { ... }) or
beforeEach(async () => { ... })) and keep the describe callback plain function()
so all it does is declare nested it/describe calls synchronously; update any
references to setup variables to be assigned in those hooks instead of inside
the describe body.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 586eb39 and 1566cab.

📒 Files selected for processing (12)

• .changeset/wet-results-doubt.md
• contracts/finance/compliance/ComplianceModuleConfidential.sol
• contracts/interfaces/IComplianceModuleConfidential.sol
• contracts/interfaces/IERC7984Rwa.sol
• contracts/mocks/token/ComplianceModuleConfidentialMock.sol
• contracts/mocks/token/ERC7984Mock.sol
• contracts/mocks/token/ERC7984RwaModularComplianceMock.sol
• contracts/token/ERC7984/extensions/ERC7984Rwa.sol
• contracts/token/ERC7984/extensions/ERC7984RwaModularCompliance.sol
• test/helpers/interface.ts
• test/token/ERC7984/ERC7984.test.ts
• test/token/ERC7984/extensions/ERC7984RwaModularCompliance.test.ts