Skip to content

Conversation

@0xalexbel
Copy link
Contributor

@0xalexbel 0xalexbel commented Nov 6, 2025

First Draft - OZ Confidential Contracts for FHEVM v9

  • add support for FHEVM v9
  • remove calls to requestDecryption
  • use FHE.checkSignatures
  • sample implementation of unwrap/finalizeUnwrap request mechanism
  • use new of publicDecrypt TS function + decryptionProof.

Zama/FHEVM dependencies (devDependencies + peerDependencies)

  • use FHEVM Hardhat Plugin v0.3.0-0
  • use Zama-fhe/relayer-sdk v0.3.0-5
  • use fhevm/solidity v0.9.0

Missing

  • swapConfidentialToERC20 has not been ported yet. The logic expected was not clear to me. I left it commented.

Important notice.

  • The use of getSepoliaConfig() has to be clarified and improved.

Summary by CodeRabbit

  • Bug Fixes

    • Redesigned unwrap mechanism to use asynchronous request-based flow with two-step finalization instead of immediate processing.
  • Refactor

    • Removed swap workflow functionality for ERC7984 to ERC20 conversions.
    • Removed encrypted amount disclosure functionality.
  • Chores

    • Updated FHEVM dependencies and libraries to latest versions.
    • Extended configuration support to Ethereum network.

@0xalexbel 0xalexbel requested a review from a team as a code owner November 6, 2025 16:39
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 6, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Multiple mock contracts migrate from SepoliaConfig to EthereumConfig. Swap and encrypted disclosure workflows are disabled in ERC7984 and SwapERC7984ToERC20 contracts. The ERC7984ERC20Wrapper refactors from immediate decryption to an asynchronous request-based unwrap mechanism with per-request state tracking. Dev dependencies are pinned to new versions.

Changes

Cohort / File(s) Summary
Config Migration to EthereumConfig
contracts/mocks/token/ERC7984*.sol, contracts/mocks/utils/*.sol, contracts/mocks/finance/VestingWallet*.sol
Replaced SepoliaConfig with EthereumConfig across 11 mock files (ERC7984Mock, ERC7984ERC20WrapperMock, ERC7984FreezableMock, ERC7984ObserverAccessMock, ERC7984ReceiverMock, ERC7984RestrictedMock, FHESafeMathMock, HandleAccessManagerMock, HandleAccessManagerUserMock, VestingWalletConfidentialMock, VestingWalletCliffConfidentialMock, VestingWalletConfidentialFactoryMock) to inherit updated base configuration.
Workflow Disablement
contracts/mocks/docs/SwapERC7984ToERC20.sol, contracts/token/ERC7984/ERC7984.sol
Removed/disabled swap workflow (swapConfidentialToERC20, finalizeSwap) in SwapERC7984ToERC20.sol; removed encrypted amount disclosure functions (discloseEncryptedAmount, finalizeDiscloseEncryptedAmount) in ERC7984.sol, leaving only commented-out placeholders.
ERC7984ERC20Wrapper Async Refactor
contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol
Replaced immediate decryption flow with asynchronous request-based unwrap mechanism: added UnwrapRequest struct and _unwrapRequests mapping for tracking pending requests, introduced UnwrapRequested event and nextUnwrapRequestId/unwrapFinalized helper functions, restructured _unwrap to emit async requests via FHE.makePubliclyDecryptable, and modified finalizeUnwrap to verify signatures per-request using FHE.verifySignatures instead of checkSignatures.
Dependency Updates
package.json
Pinned @fhevm/hardhat-plugin to 0.3.0-0, @fhevm/solidity to 0.9.0, and @zama-fhe/relayer-sdk to 0.3.0-5 in devDependencies.

Sequence Diagram

sequenceDiagram
    participant User
    participant ERC7984ERC20Wrapper
    participant FHE
    participant OffChain
    participant Finalize

    rect rgb(240, 248, 255)
    Note over User,Finalize: NEW: Async Unwrap Flow
    end

    User->>ERC7984ERC20Wrapper: unwrap(amount)
    activate ERC7984ERC20Wrapper
    ERC7984ERC20Wrapper->>ERC7984ERC20Wrapper: _unwrap(from, to, amount)
    ERC7984ERC20Wrapper->>ERC7984ERC20Wrapper: increment _counterUnwrapRequestId
    ERC7984ERC20Wrapper->>ERC7984ERC20Wrapper: store UnwrapRequest{receiver, amount}
    ERC7984ERC20Wrapper->>FHE: makePubliclyDecryptable(burntAmount)
    FHE-->>ERC7984ERC20Wrapper: ✓
    ERC7984ERC20Wrapper->>ERC7984ERC20Wrapper: emit UnwrapRequested(requestID, receiver, amount)
    deactivate ERC7984ERC20Wrapper

    rect rgb(255, 250, 240)
    Note over OffChain: Off-chain: Decrypt & Sign
    end
    OffChain->>OffChain: Decrypt cleartexts
    OffChain->>OffChain: Generate decryptionProof

    rect rgb(245, 255, 250)
    Note over Finalize: ON-CHAIN: Verification & Settlement
    end
    User->>Finalize: finalizeUnwrap(requestID, cleartexts, decryptionProof)
    activate Finalize
    Finalize->>Finalize: Retrieve UnwrapRequest from storage
    Finalize->>Finalize: Derive handle from amount
    Finalize->>Finalize: Delete request from storage
    Finalize->>FHE: verifySignatures(handlesList, cleartexts, decryptionProof)
    FHE-->>Finalize: ✓ Valid
    Finalize->>User: Transfer decrypted amount
    deactivate Finalize
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • ERC7984ERC20Wrapper.sol requires careful review of the async unwrap refactor: new data structures (UnwrapRequest struct, mapping, counter), modified _unwrap and finalizeUnwrap flows, and signature verification logic changes (verifySignatures vs. checkSignatures). State machine semantics and edge cases (deleted requests, re-entrancy) warrant attention.
  • ERC7984.sol function removals eliminate public API surface; verify all call sites and dependent tests are addressed.
  • SwapERC7984ToERC20.sol disablement of core swap functions; ensure no orphaned references or broken dependencies.
  • Mock config migrations are straightforward and homogeneous but collectively affect many files; spot-check a few to ensure consistency.
  • Package.json pinning to pre-release versions (@fhevm/hardhat-plugin 0.3.0-0, @zama-fhe/relayer-sdk 0.3.0-5) may introduce instability; verify compatibility and stability guarantees.

Possibly related PRs

  • PR #217: Modifies the same finalizeDiscloseEncryptedAmount and finalizeUnwrap implementations to inline amount decoding, directly overlapping with this PR's removal and restructuring of these functions.
  • PR #202: Changes finalize* swap and disclosure function signatures to accept cleartexts+signatures for fhevm v0.8.0, intersecting with this PR's disabling of swap/disclosure workflows.
  • PR #115: Modifies the same ERC7984 disclose workflow (discloseEncryptedAmount/finalizeDiscloseEncryptedAmount) that this PR removes entirely.

Suggested labels

ignore-changeset

Suggested reviewers

  • james-toussaint
  • arr00

Poem

🐰 Hoppity hop, the configs now bloom,
From Sepolia's state to Ethereum's room,
Unwraps now async, requests in flight,
Off-chain decryption brings order to light.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'fhevm-v9' is vague and generic; it refers only to a version number without describing the actual changes or purpose of the pull request. Use a more descriptive title that summarizes the main change, such as 'Update FHEVM dependencies to v9 and refactor decryption flow' or 'Port OpenZeppelin Confidential Contracts to FHEVM v9'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@netlify
Copy link

netlify bot commented Nov 6, 2025

Deploy Preview for confidential-tokens ready!

Name Link
🔨 Latest commit a8abbd1
🔍 Latest deploy log https://app.netlify.com/projects/confidential-tokens/deploys/690cd426c67cbd000878783c
😎 Deploy Preview https://deploy-preview-240--confidential-tokens.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
contracts/mocks/finance/VestingWalletConfidentialFactoryMock.sol (1)

64-64: Use the Ethereum coproccessor config here
The contract now inherits EthereumConfig, but initialize still wires the coproccessor with ZamaConfig.getSepoliaConfig(). This mismatch will leave the vesting wallets talking to the Sepolia keys and break FHEVM v9 flows on Ethereum. Please switch to the Ethereum getter (e.g. ZamaConfig.getEthereumConfig()).

♻️ Duplicate comments (1)
contracts/mocks/finance/VestingWalletCliffConfidentialMock.sol (1)

4-7: Verify config migration implications.

Same SepoliaConfigEthereumConfig migration as other mocks. See comment on VestingWalletConfidentialMock.sol for details on clarifying this change.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b2c6388 and 0547737.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (16)
  • contracts/mocks/docs/SwapERC7984ToERC20.sol (1 hunks)
  • contracts/mocks/finance/VestingWalletCliffConfidentialMock.sol (1 hunks)
  • contracts/mocks/finance/VestingWalletConfidentialFactoryMock.sol (2 hunks)
  • contracts/mocks/finance/VestingWalletConfidentialMock.sol (1 hunks)
  • contracts/mocks/token/ERC7984ERC20WrapperMock.sol (1 hunks)
  • contracts/mocks/token/ERC7984FreezableMock.sol (1 hunks)
  • contracts/mocks/token/ERC7984Mock.sol (1 hunks)
  • contracts/mocks/token/ERC7984ObserverAccessMock.sol (1 hunks)
  • contracts/mocks/token/ERC7984ReceiverMock.sol (1 hunks)
  • contracts/mocks/token/ERC7984RestrictedMock.sol (1 hunks)
  • contracts/mocks/utils/FHESafeMathMock.sol (1 hunks)
  • contracts/mocks/utils/HandleAccessManagerMock.sol (1 hunks)
  • contracts/mocks/utils/HandleAccessManagerUserMock.sol (1 hunks)
  • contracts/token/ERC7984/ERC7984.sol (2 hunks)
  • contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol (4 hunks)
  • package.json (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-15T14:43:25.644Z
Learnt from: arr00
Repo: OpenZeppelin/openzeppelin-confidential-contracts PR: 186
File: contracts/token/ERC7984/extensions/ERC7984Omnibus.sol:140-167
Timestamp: 2025-09-15T14:43:25.644Z
Learning: In ERC7984Omnibus callback functions like confidentialTransferFromAndCallOmnibus, the encrypted sender and recipient addresses are not passed to the callback recipient - only the standard transfer parameters (omnibusFrom, omnibusTo, amount, data) are passed. The ACL grants for the encrypted addresses are for omnibus event emission and future access, not for callback usage.

Applied to files:

  • contracts/mocks/token/ERC7984ReceiverMock.sol
  • contracts/token/ERC7984/ERC7984.sol
📚 Learning: 2025-09-22T09:21:34.470Z
Learnt from: james-toussaint
Repo: OpenZeppelin/openzeppelin-confidential-contracts PR: 160
File: test/token/ERC7984/extensions/ERC7984Rwa.test.ts:474-479
Timestamp: 2025-09-22T09:21:34.470Z
Learning: In ERC7984Freezable force transfers, the frozen balance is reset to the new balance only when the transferred amount exceeds the available balance (balance - frozen). If the transferred amount is within the available balance, the frozen amount remains unchanged. This is implemented via FHE.select(FHE.gt(encryptedAmount, confidentialAvailable(account)), confidentialBalanceOf(account), frozen).

Applied to files:

  • contracts/token/ERC7984/ERC7984.sol
  • contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol
🔇 Additional comments (6)
contracts/mocks/finance/VestingWalletConfidentialMock.sol (1)

4-7: Clarify the SepoliaConfig → EthereumConfig migration intent.

The mock now inherits EthereumConfig instead of SepoliaConfig. This change is consistent across all mocks in the PR, but it represents a significant semantic shift from testnet-specific to mainnet configuration.

The PR description notes: "the use of getSepoliaConfig() requires clarification and improvement." Please clarify:

  1. Is this change intentional for FHEVM v9 compatibility?
  2. Are the mocks still intended for testnet usage or have requirements changed?
  3. What are the implications for network parameters, addresses, and deployment?

Based on learnings

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

5-10: Config migration noted.

The config inheritance change from SepoliaConfig to EthereumConfig is consistent with other mocks. The mock's functional logic remains unchanged. Please ensure this config migration is intentional and verified (see VestingWalletConfidentialMock.sol comment).

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

4-8: Config migration consistent.

The inheritance change to EthereumConfig is consistent with the broader mock migration pattern. Mock functionality is preserved. Verify the config migration intent as noted in VestingWalletConfidentialMock.sol.

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

5-9: Config migration applied consistently.

The EthereumConfig inheritance update is consistent with the broader migration. The mock's helper functions remain unchanged. Ensure the config migration is validated (see VestingWalletConfidentialMock.sol comment).

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

4-8: Config migration finalized.

Final mock file with the SepoliaConfigEthereumConfig migration. The callback implementation is preserved correctly. Verify the config migration intent as noted in the initial mock file comment.

package.json (1)

48-49: Prerelease versions verified, but devnet testing required before production use.

All three package versions exist in the npm registry with no security advisories found:

  • @fhevm/hardhat-plugin@0.3.0-0
  • @fhevm/solidity@0.9.0
  • @zama-fhe/relayer-sdk@0.3.0-5

The 0.9.x versions are intentionally designed for FHEVM v0.9 compatibility. However, the 0.9.0 prerelease should be tested against the v0.9 gateway on a devnet before mainnet use. Pinning these versions without semver ranges is appropriate given their prerelease status. Before merging, ensure devnet testing has been completed and documented.

Comment on lines +21 to +44
// function swapConfidentialToERC20(externalEuint64 encryptedInput, bytes memory inputProof) public {
// euint64 amount = FHE.fromExternal(encryptedInput, inputProof);
// FHE.allowTransient(amount, address(_fromToken));
// euint64 amountTransferred = _fromToken.confidentialTransferFrom(msg.sender, address(this), amount);

// bytes32[] memory cts = new bytes32[](1);
// cts[0] = euint64.unwrap(amountTransferred);
// uint256 requestID = FHE.requestDecryption(cts, this.finalizeSwap.selector);

// // register who is getting the tokens
// _receivers[requestID] = msg.sender;
// }

// function finalizeSwap(uint256 requestID, bytes calldata cleartexts, bytes calldata decryptionProof) public virtual {
// FHE.checkSignatures(requestID, cleartexts, decryptionProof);
// uint64 amount = abi.decode(cleartexts, (uint64));
// address to = _receivers[requestID];
// require(to != address(0), SwapConfidentialToERC20InvalidGatewayRequest(requestID));
// delete _receivers[requestID];

// if (amount != 0) {
// SafeERC20.safeTransfer(_toToken, to, amount);
// }
// }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Non-functional contract: track or implement the missing swap logic.

The entire swap workflow is commented out, leaving the contract with no executable functionality beyond construction. While the PR objectives acknowledge that the logic was unclear, this creates technical debt that should be explicitly tracked.

Concerns:

  • State variables and constructor are initialized but never used
  • Deploying this contract would be non-functional and waste gas
  • No documentation (TODO/FIXME) indicating the migration path or expected timeline
  • The commented code shows a mix of old (requestDecryption at line 28) and new (checkSignatures at line 35) API patterns, suggesting the migration requirements are not yet fully understood

Recommendations:

  1. Add a TODO/FIXME comment at the contract level explaining the missing implementation
  2. Open a tracking issue for porting this functionality to FHEVM v9
  3. Consider removing this contract entirely if it's not needed in this PR, or mark it clearly as a non-functional placeholder
  4. Document the specific clarifications needed about the "expected logic" mentioned in the PR description

Would you like me to help implement the FHEVM v9 migration for these functions based on patterns from other contracts in the PR, or open a tracking issue for this work?

🤖 Prompt for AI Agents
In contracts/mocks/docs/SwapERC7984ToERC20.sol around lines 21-44, the swap
workflow is fully commented out leaving the contract non-functional; add a
top-of-contract TODO/FIXME noting it's a non-functional placeholder and that
swap logic must be migrated to FHEVM v9, open a tracking issue linking this file
and describing the missing functions (swapConfidentialToERC20 and finalizeSwap)
and the API mismatches (requestDecryption vs checkSignatures), and either remove
the contract from the PR if unused or add a clear NOP marker and deploy-blocker
comment indicating it must not be used in production; also add a short checklist
of the clarifications needed (expected cleartext format, receiver handling, and
exact FHEVM v9 API calls) so whoever picks up the issue can implement the
migration.

Comment on lines +4 to +8
import {EthereumConfig} from "@fhevm/solidity/config/ZamaConfig.sol";
import {FHE, ebool, euint64, externalEuint64} from "@fhevm/solidity/lib/FHE.sol";
import {FHESafeMath} from "./../../utils/FHESafeMath.sol";

contract FHESafeMathMock is SepoliaConfig {
contract FHESafeMathMock is EthereumConfig {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

EthereumConfig still wires zeroed coprocessor/oracle addresses

EthereumConfig in the current ZamaConfig release is only a placeholder: getEthereumConfig() and getEthereumOracleAddress() both return address(0), and the docs explicitly state Sepolia is the only supported network config today. Inheriting EthereumConfig therefore sets the FHE coprocessor and oracle to zero, so every downstream FHE.* call (e.g., the FHESafeMath ops exercised here) will revert at runtime. Please keep inheriting SepoliaConfig (or provide non-zero mainnet addresses) before enabling these mocks. Same fix will be needed for the other mocks switched to EthereumConfig in this PR. (docs.zama.ai)

-import {EthereumConfig} from "@fhevm/solidity/config/ZamaConfig.sol";
+import {SepoliaConfig} from "@fhevm/solidity/config/ZamaConfig.sol";-contract FHESafeMathMock is EthereumConfig {
+contract FHESafeMathMock is SepoliaConfig {
📝 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.

Suggested change
import {EthereumConfig} from "@fhevm/solidity/config/ZamaConfig.sol";
import {FHE, ebool, euint64, externalEuint64} from "@fhevm/solidity/lib/FHE.sol";
import {FHESafeMath} from "./../../utils/FHESafeMath.sol";
contract FHESafeMathMock is SepoliaConfig {
contract FHESafeMathMock is EthereumConfig {
import {SepoliaConfig} from "@fhevm/solidity/config/ZamaConfig.sol";
import {FHE, ebool, euint64, externalEuint64} from "@fhevm/solidity/lib/FHE.sol";
import {FHESafeMath} from "./../../utils/FHESafeMath.sol";
contract FHESafeMathMock is SepoliaConfig {
🤖 Prompt for AI Agents
In contracts/mocks/utils/FHESafeMathMock.sol around lines 4 to 8, the contract
inherits EthereumConfig which currently returns zeroed coprocessor/oracle
addresses causing all FHE.* calls to revert; change the base contract to
SepoliaConfig (or otherwise initialize non-zero mainnet coprocessor and oracle
addresses) so FHE operations have valid addresses, and apply the same change to
any other mock files in this PR that were switched to EthereumConfig.

Comment on lines 191 to 207
// /**
// * @dev Discloses an encrypted amount `encryptedAmount` publicly via an {IERC7984-AmountDisclosed}
// * event. The caller and this contract must be authorized to use the encrypted amount on the ACL.
// *
// * NOTE: This is an asynchronous operation where the actual decryption happens off-chain and
// * {finalizeDiscloseEncryptedAmount} is called with the result.
// */
// function discloseEncryptedAmount(euint64 encryptedAmount) public virtual {
// require(
// FHE.isAllowed(encryptedAmount, msg.sender),
// ERC7984UnauthorizedUseOfEncryptedAmount(encryptedAmount, msg.sender)
// );

// bytes32[] memory cts = new bytes32[](1);
// cts[0] = euint64.unwrap(encryptedAmount);
// FHE.requestDecryption(cts, this.finalizeDiscloseEncryptedAmount.selector);
// }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Remove or complete the commented disclosure workflow.

The discloseEncryptedAmount function is commented out rather than removed or replaced. According to the PR objectives, the disclosure flow should be replaced with FHE.checkSignatures and publicDecrypt, but no replacement implementation is visible.

Leaving commented code creates technical debt and confusion. Either:

  1. Remove the commented code entirely if the feature is deprecated, or
  2. Implement the new disclosure mechanism using the FHEVM v9 approach

Verify the intended approach for encrypted amount disclosure in FHEVM v9:


🏁 Script executed:

#!/bin/bash
# Search for new disclosure implementations or FHE.checkSignatures usage

echo "Searching for FHE.checkSignatures usage..."
rg -n "checkSignatures" --type=sol -C3

echo "Searching for publicDecrypt references..."
rg -n "publicDecrypt" --type=sol -C3

echo "Searching for any remaining requestDecryption calls..."
rg -n "requestDecryption" --type=sol -C3

Length of output: 223



🏁 Script executed:

#!/bin/bash
# Search for new disclosure implementations or FHE.checkSignatures usage in Solidity files

echo "Searching for FHE.checkSignatures usage..."
rg -n "checkSignatures" --glob="*.sol" -C3

echo "Searching for publicDecrypt references..."
rg -n "publicDecrypt" --glob="*.sol" -C3

echo "Searching for any remaining requestDecryption calls..."
rg -n "requestDecryption" --glob="*.sol" -C3

Length of output: 2717



🏁 Script executed:

#!/bin/bash
# Check for active implementations in ERC7984.sol after commented code

echo "Checking ERC7984.sol content around and after commented sections..."
sed -n '190,250p' contracts/token/ERC7984/ERC7984.sol

echo -e "\n\nSearching for any active finalizeDiscloseEncryptedAmount implementations..."
rg -n "finalizeDiscloseEncryptedAmount" --glob="*.sol" -A5

echo -e "\n\nSearching for any other active disclosure-related functions..."
rg -n "disclose|Disclose" --glob="*.sol" --type-not java -C2 | head -100

Length of output: 9638


Remove commented disclosure functions or complete the migration to FHEVM v9 pattern.

Both discloseEncryptedAmount and finalizeDiscloseEncryptedAmount are entirely commented out with no active replacement implementation, creating technical debt and a breaking change to the public API. The commented code shows the new pattern using FHE.verifySignatures, but it remains inactive.

Either remove these functions entirely if the feature is deprecated, or uncomment and complete the implementation. Note: the new pattern uses FHE.verifySignatures, not FHE.checkSignatures.

Contracts/token/ERC7984/ERC7984.sol lines 191-230: Uncomment and activate the new disclosure implementation, or remove completely.

🤖 Prompt for AI Agents
In contracts/token/ERC7984/ERC7984.sol around lines 191 to 207 (extend to 230),
the commented-out disclosure functions leave a broken public API; either remove
them or restore the new FHEVM v9 pattern. Reintroduce a working
discloseEncryptedAmount + finalizeDiscloseEncryptedAmount pair matching the new
pattern: check ACL as before, build cts array from euint64.unwrap, call
FHE.requestDecryption and in the callback use FHE.verifySignatures (not
FHE.checkSignatures) to validate decryption responses, emit
IERC7984-AmountDisclosed on success, and revert/emit appropriate errors on
verification failure; alternatively, if deprecated, delete both commented
functions and update the interface/docs accordingly so the public API no longer
references them.

Comment on lines 220 to 230
// function finalizeDiscloseEncryptedAmount(
// euint64 encryptedAmount,
// bytes calldata cleartexts,
// bytes calldata decryptionProof
// ) public virtual {
// //ERC7984UnauthorizedUseOfEncryptedAmount(encryptedAmount, msg.sender)
// bytes32[] memory cts = new bytes32[](1);
// cts[0] = euint64.unwrap(encryptedAmount);
// FHE.verifySignatures(cts, cleartexts, decryptionProof);
// emit AmountDisclosed(encryptedAmount, abi.decode(cleartexts, (uint64)));
// }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Remove or replace commented finalization function.

The finalizeDiscloseEncryptedAmount function is commented out with an incomplete implementation referencing verifySignatures (line 228). The comment block above (lines 209-219) still describes the old workflow.

This creates inconsistency and technical debt. Additionally, note that line 228 references verifySignatures while the PR description mentions checkSignatures - clarify which API is correct for FHEVM v9.

Search the FHEVM v9 documentation to confirm the correct signature verification API:


🌐 Web query:

FHEVM v9 signature verification API checkSignatures vs verifySignatures

💡 Result:

Short answer — they’re different layers of the same flow:

  • FHE.checkSignatures (public API) — a convenience function in the FHE library that you call inside your callback. Signature: checkSignatures(uint256 requestId, bytes cleartexts, bytes[] signatures). It validates the KMS signatures for the given request and reverts if they are invalid. [1]

  • verifySignatures (internal/private helper) — library/internal implementation that extracts the decrypted bytes for the requested handles and forwards them to the on‑chain KMS verifier (IKMSVerifier.verifyDecryptionEIP712KMSSignatures). It operates on handlesList + decryptedResult + signatures and returns the boolean verification result; it’s used by higher‑level checkSignatures logic. [1][2]

Sources:

  • FHE.decryption / FHE.checkSignatures docs. [1]
  • FHE/TFHE implementation showing verifySignatures → IKMSVerifier.verifyDecryptionEIP712KMSSignatures. [2]


---

🏁 Script executed:

```shell
#!/bin/bash
# Check FHE imports and verifySignatures usage in the contract file
rg -nP 'import.*FHE|verifySignatures|checkSignatures' contracts/token/ERC7984/ERC7984.sol

Length of output: 215


🏁 Script executed:

#!/bin/bash
# Search for FHE function definitions or interface definitions in codebase
fd -e sol | head -20 | xargs rg -l 'interface.*FHE|library.*FHE' 2>/dev/null | head -5

Length of output: 33


🏁 Script executed:

#!/bin/bash
# Check for verifySignatures function definitions across codebase
rg -nP 'function\s+verifySignatures|function\s+checkSignatures' --type=sol

Length of output: 27



---

🏁 Script executed:

```shell
#!/bin/bash
# Find package.json to check FHEVM version
fd -name package.json -type f | xargs grep -l "fhevm" 2>/dev/null

Length of output: 230


🏁 Script executed:

#!/bin/bash
# Check FHEVM version in dependencies
cat package.json 2>/dev/null | grep -A5 -B5 "fhevm"

Length of output: 1681


🏁 Script executed:

#!/bin/bash
# Search for FHE.sol in node_modules or lib directories
find . -name "FHE.sol" -type f 2>/dev/null | head -5

Length of output: 0


Remove or properly refactor the commented function to use the correct FHEVM v0.9.0 API.

The commented finalizeDiscloseEncryptedAmount function at lines 220-230 incorrectly references FHE.verifySignatures, which is an internal helper function. The correct public API is FHE.checkSignatures(uint256 requestId, bytes cleartexts, bytes[] signatures).

Additionally, the parameter list at line 228 does not match the public API signature—cts (bytes32[]) and decryptionProof (bytes) do not align with the expected requestId (uint256) and signatures (bytes[]). Remove the commented block or refactor it to use the correct public API with appropriate parameters.

🤖 Prompt for AI Agents
In contracts/token/ERC7984/ERC7984.sol around lines 220 to 230, the commented
finalizeDiscloseEncryptedAmount uses the internal FHE.verifySignatures API and
mismatched parameters; either delete this dead code or refactor it to call the
public FHE.checkSignatures(uint256 requestId, bytes cleartexts, bytes[]
signatures) instead: replace construction of bytes32[] cts with a uint256
requestId passed in, change decryptionProof to bytes[] signatures, call
FHE.checkSignatures(requestId, cleartexts, signatures), and then emit
AmountDisclosed(encryptedAmount, abi.decode(cleartexts, (uint64))); ensure
function visibility and parameter types match the public API if you reintroduce
it.

@arr00
Copy link
Contributor

arr00 commented Nov 12, 2025

closed in favor of #248

@arr00 arr00 closed this Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants