Add extension ERC7984Hooked that calls hooks before and after transfer

[arr00]

Replaces #302

Summary by CodeRabbit

• New Features

  • Added ERC7984Hooked extension for installing and managing hook modules that execute before and after confidential token transfers.
  • Introduced IERC7984HookModule interface for implementing custom transfer hooks and compliance modules.
  • Extended IERC7984Rwa interface with admin/agent role checks and additional confidential mint/burn/transfer operations.

• Tests

  • Added comprehensive test coverage for ERC7984Hooked module lifecycle and hook execution behavior.

[netlify]

✅ Deploy Preview for confidential-tokens ready!

Name	Link
🔨 Latest commit	9d95fa4
🔍 Latest deploy log	https://app.netlify.com/projects/confidential-tokens/deploys/69c33703fe51e400087368f9
😎 Deploy Preview	https://deploy-preview-332--confidential-tokens.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[coderabbitai]

🧹 Nitpick comments (6)

contracts/interfaces/IERC7984Rwa.sol (1)

5-5: Unused import: ebool.

The ebool type is imported but not used in this interface. Consider removing it to keep imports clean.

🧹 Proposed fix

-import {ebool, externalEuint64, euint64} from "@fhevm/solidity/lib/FHE.sol";
+import {externalEuint64, euint64} from "@fhevm/solidity/lib/FHE.sol";

🤖 Prompt for AI Agents

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

In `@contracts/interfaces/IERC7984Rwa.sol` at line 5, The import list in
IERC7984Rwa.sol includes an unused symbol `ebool`; remove `ebool` from the
import statement that currently imports `{ebool, externalEuint64, euint64}` so
the file only imports the used types (`externalEuint64, euint64`) and keep the
rest of the interface untouched.

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

5-6: Remove unused imports.

ZamaEthereumConfig and FHE are imported but not directly used in this contract. ZamaEthereumConfig is already inherited through ERC7984Mock, and euint64 comes through the parent contracts.

🧹 Proposed fix

 pragma solidity ^0.8.24;
 
-import {ZamaEthereumConfig} from "@fhevm/solidity/config/ZamaConfig.sol";
-import {FHE, euint64} from "@fhevm/solidity/lib/FHE.sol";
+import {euint64} from "@fhevm/solidity/lib/FHE.sol";
 import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";

🤖 Prompt for AI Agents

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

In `@contracts/mocks/token/ERC7984HookedMock.sol` around lines 5 - 6, Remove the
unused imports by deleting the import lines for ZamaEthereumConfig and FHE from
ERC7984HookedMock.sol; the contract already gets ZamaEthereumConfig via
ERC7984Mock and euint64 is available through parent contracts, so trim the
imports to eliminate ZamaEthereumConfig and FHE (and any unused symbols) so the
file only imports what it actually uses.

contracts/finance/compliance/ERC7984HookModule.sol (2)

20-30: NatDoc comments describe errors, not modifiers.

The NatDoc comments at lines 20 and 26 state "Thrown when..." but they precede modifier definitions, not error definitions. Consider updating to describe the modifier's purpose instead.

📝 Suggested NatDoc fix

-    /// `@dev` Thrown when the sender is not an admin of the token.
+    /// `@dev` Restricts access to accounts with admin role on the token.
     modifier onlyTokenAdmin(address token) {
         require(IERC7984Rwa(token).isAdmin(msg.sender), NotAuthorized(msg.sender));
         _;
     }

-    /// `@dev` Thrown when the sender is not an agent of the token.
+    /// `@dev` Restricts access to accounts with agent role on the token.
     modifier onlyTokenAgent(address token) {
         require(IERC7984Rwa(token).isAgent(msg.sender), NotAuthorized(msg.sender));
         _;
     }

🤖 Prompt for AI Agents

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

In `@contracts/finance/compliance/ERC7984HookModule.sol` around lines 20 - 30,
Update the NatSpec comments to describe the modifier behavior instead of
implying an error is thrown: replace "Thrown when the sender is not an admin of
the token." and "Thrown when the sender is not an agent of the token." with
brief descriptions like "Restricts function to token admins; reverts with
NotAuthorized if caller is not an admin." and "Restricts function to token
agents; reverts with NotAuthorized if caller is not an agent." for the modifiers
onlyTokenAdmin(address token) and onlyTokenAgent(address token), respectively,
so the comments accurately document the modifier semantics and the NotAuthorized
revert.

---

15-15: Unused error declaration.

UnauthorizedUseOfEncryptedAmount is declared but not used in this contract. If it's intended for subclasses to use, consider documenting this or moving it to a shared errors file.

🤖 Prompt for AI Agents

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

In `@contracts/finance/compliance/ERC7984HookModule.sol` at line 15, The declared
error UnauthorizedUseOfEncryptedAmount is never referenced in
ERC7984HookModule.sol; either remove the unused declaration, or if it’s intended
for reuse by inheritors move it into a shared errors library/contract and import
it where needed, or add a comment documenting that subclasses should use it;
update any inheriting contracts to import and reference the shared error name
instead of declaring their own.

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

42-52: Clarify the handle initialization check.

The condition euint64.unwrap(fromBalance) != 0 checks whether the handle itself is non-zero (i.e., initialized), not whether the encrypted value is zero. This appears intentional to guard against calling _getTokenHandleAllowance on an uninitialized handle, but using FHE.isInitialized(fromBalance) would be more explicit and consistent with the pattern used in ERC7984Hooked.sol (lines 131, 141).

♻️ Suggested improvement for clarity

-        if (euint64.unwrap(fromBalance) != 0) {
+        if (FHE.isInitialized(fromBalance)) {
             _getTokenHandleAllowance(token, fromBalance);
             assert(FHE.isAllowed(fromBalance, address(this)));
         }

🤖 Prompt for AI Agents

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

In `@contracts/mocks/token/ERC7984HookModuleMock.sol` around lines 42 - 52, The
check in _preTransfer currently uses euint64.unwrap(fromBalance) != 0 to detect
an initialized handle; change it to use FHE.isInitialized(fromBalance) (the same
pattern as ERC7984Hooked.sol) so we explicitly test handle initialization before
calling _getTokenHandleAllowance(token, fromBalance) and asserting
FHE.isAllowed(fromBalance, address(this)); keep the rest of _preTransfer
behavior (emit PreTransfer and return FHE.asEbool(isCompliant)).

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

92-99: Clarify the intentional silent failure on onUninstall.

Using LowLevelCall.callNoReturn means that if a module's onUninstall reverts, the failure is silently ignored and the module is still removed. While this prevents malicious modules from blocking their own removal, it could lead to inconsistent state in the module if its cleanup logic fails.

Consider documenting this behavior explicitly in the NatDoc, so integrators understand that modules should not rely on onUninstall being successfully executed.

📝 Suggested documentation improvement

-    /// `@dev` Internal function which uninstalls a module.
+    /// `@dev` Internal function which uninstalls a module.
+    /// NOTE: If the module's `onUninstall` reverts, the failure is ignored and the module is still removed.
+    /// This prevents malicious modules from blocking their own removal.
     function _uninstallModule(address module, bytes memory deinitData) internal virtual {

🤖 Prompt for AI Agents

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

In `@contracts/token/ERC7984/extensions/ERC7984Hooked.sol` around lines 92 - 99,
Add a NatSpec clarification that _uninstallModule intentionally ignores errors
from module cleanup: update the documentation for the _uninstallModule function
(which calls LowLevelCall.callNoReturn and invokes
IERC7984HookModule.onUninstall) to state clearly that onUninstall failures are
deliberately swallowed and the module is removed regardless, so integrators must
not rely on onUninstall executing successfully for critical state cleanup;
include guidance that modules should perform idempotent/defensive cleanup or
expose explicit uninstall verification if they need guaranteed cleanup.

🤖 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/finance/compliance/ERC7984HookModule.sol`:
- Around line 20-30: Update the NatSpec comments to describe the modifier
behavior instead of implying an error is thrown: replace "Thrown when the sender
is not an admin of the token." and "Thrown when the sender is not an agent of
the token." with brief descriptions like "Restricts function to token admins;
reverts with NotAuthorized if caller is not an admin." and "Restricts function
to token agents; reverts with NotAuthorized if caller is not an agent." for the
modifiers onlyTokenAdmin(address token) and onlyTokenAgent(address token),
respectively, so the comments accurately document the modifier semantics and the
NotAuthorized revert.
- Line 15: The declared error UnauthorizedUseOfEncryptedAmount is never
referenced in ERC7984HookModule.sol; either remove the unused declaration, or if
it’s intended for reuse by inheritors move it into a shared errors
library/contract and import it where needed, or add a comment documenting that
subclasses should use it; update any inheriting contracts to import and
reference the shared error name instead of declaring their own.

In `@contracts/interfaces/IERC7984Rwa.sol`:
- Line 5: The import list in IERC7984Rwa.sol includes an unused symbol `ebool`;
remove `ebool` from the import statement that currently imports `{ebool,
externalEuint64, euint64}` so the file only imports the used types
(`externalEuint64, euint64`) and keep the rest of the interface untouched.

In `@contracts/mocks/token/ERC7984HookedMock.sol`:
- Around line 5-6: Remove the unused imports by deleting the import lines for
ZamaEthereumConfig and FHE from ERC7984HookedMock.sol; the contract already gets
ZamaEthereumConfig via ERC7984Mock and euint64 is available through parent
contracts, so trim the imports to eliminate ZamaEthereumConfig and FHE (and any
unused symbols) so the file only imports what it actually uses.

In `@contracts/mocks/token/ERC7984HookModuleMock.sol`:
- Around line 42-52: The check in _preTransfer currently uses
euint64.unwrap(fromBalance) != 0 to detect an initialized handle; change it to
use FHE.isInitialized(fromBalance) (the same pattern as ERC7984Hooked.sol) so we
explicitly test handle initialization before calling
_getTokenHandleAllowance(token, fromBalance) and asserting
FHE.isAllowed(fromBalance, address(this)); keep the rest of _preTransfer
behavior (emit PreTransfer and return FHE.asEbool(isCompliant)).

In `@contracts/token/ERC7984/extensions/ERC7984Hooked.sol`:
- Around line 92-99: Add a NatSpec clarification that _uninstallModule
intentionally ignores errors from module cleanup: update the documentation for
the _uninstallModule function (which calls LowLevelCall.callNoReturn and invokes
IERC7984HookModule.onUninstall) to state clearly that onUninstall failures are
deliberately swallowed and the module is removed regardless, so integrators must
not rely on onUninstall executing successfully for critical state cleanup;
include guidance that modules should perform idempotent/defensive cleanup or
expose explicit uninstall verification if they need guaranteed cleanup.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ab1db676-d37d-4839-94df-431166c5eb8d

📥 Commits

Reviewing files that changed from the base of the PR and between 933f7b1 and dcfcbf4.

📒 Files selected for processing (11)

• .changeset/wet-results-doubt.md
• contracts/finance/compliance/ERC7984HookModule.sol
• contracts/interfaces/IERC7984HookModule.sol
• contracts/interfaces/IERC7984Rwa.sol
• contracts/mocks/token/ERC7984HookModuleMock.sol
• contracts/mocks/token/ERC7984HookedMock.sol
• contracts/mocks/token/ERC7984Mock.sol
• contracts/token/ERC7984/extensions/ERC7984Hooked.sol
• test/helpers/interface.ts
• test/token/ERC7984/ERC7984.test.ts
• test/token/ERC7984/extensions/ERC7984Hooked.test.ts

[ernestognw]

Looking good!

[ernestognw]

Small comment. LGTM

[ernestognw]

Shall we prefix these with ERC7984ModuleInstalled? I wonder if a project that has both this contract and an ERC7579 account will have an issue because the identifier is declared twice (not necessarily inheriting from both, that's very unlikely)

Same for the ModuleUninstalled event