Skip to content

Conversation

james-toussaint
Copy link
Contributor

@james-toussaint james-toussaint commented Aug 20, 2025

Summary by CodeRabbit

  • New Features

    • Added explicit owner-only controls to grant access to confidential current and historical total supply.
  • Bug Fixes

    • Improved arithmetic handling for uninitialized values to prevent incorrect decreases.
    • Adjusted confidential-available logic to rely on safer decrease behavior.
  • Tests

    • Introduced a reusable ERC7984 behavior test suite and refactored token tests to use it.
    • Updated voting and freezable tests to invoke new access steps and shared behaviors.
  • Documentation

    • Minor wording clarification in the freezable extension description.

@james-toussaint james-toussaint marked this pull request as ready for review August 20, 2025 14:17
@james-toussaint james-toussaint requested a review from a team as a code owner August 20, 2025 14:17
arr00 and others added 2 commits August 20, 2025 14:36
Base automatically changed from feature/confidential-freezable-token to master August 25, 2025 14:57
Copy link

netlify bot commented Aug 27, 2025

Deploy Preview for confidential-tokens ready!

Name Link
🔨 Latest commit 27c50c6
🔍 Latest deploy log https://app.netlify.com/projects/confidential-tokens/deploys/68b08e23c36d5f0008134501
😎 Deploy Preview https://deploy-preview-181--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 bot commented Aug 27, 2025

Walkthrough

Adjusted multiple mock contracts' inheritance override targets, added owner-only helper access functions, changed FHESafeMath.tryDecrease to handle uninitialized values, modified ERC7984Freezable.confidentialAvailable to use a named return and rely on tryDecrease's second value, and refactored ERC7984 tests into a shared behavior suite with updated fixtures and access calls.

Changes

Cohort / File(s) Change Summary
Mock override targets
contracts/mocks/token/ERC7984FreezableMock.sol, contracts/mocks/token/ERC7984ObserverAccessMock.sol, contracts/mocks/token/ERC7984VotesMock.sol
Changed internal _update override specifiers to target ERC7984 instead of ERC7984Mock (override lists altered); bodies still delegate to super._update.
Mock token access helper
contracts/mocks/token/ERC7984Mock.sol
Removed implicit post-update access grant; added public confidentialTotalSupplyAccess() requiring msg.sender == _OWNER and calling FHE.allow(confidentialTotalSupply(), msg.sender).
Votes mock helper
contracts/mocks/token/ERC7984VotesMock.sol
Added public getPastTotalSupplyAccess(uint256 timepoint) (owner-only) to grant decryption access for past total supply.
Freezable extension
contracts/token/ERC7984/extensions/ERC7984Freezable.sol
Doc wording tweak. confidentialAvailable(address) now declares euint64 unfrozen and returns the second value from FHESafeMath.tryDecrease directly (removed prior success-flag fallback).
FHE safe math
contracts/utils/FHESafeMath.sol
tryDecrease now guards uninitialized inputs: returns (false, 0) if oldValue uninitialized; (true, oldValue) if delta uninitialized; otherwise retains original compare/subtract semantics.
Shared ERC7984 behavior tests
test/token/ERC7984/ERC7984.behaviour.ts
New reusable test suite shouldBehaveLikeERC7984(...) covering constructor, confidential balances, mint/burn, transfers (including callbacks), disclosures, auth/error cases; uses fixtures and FHEVM helpers.
ERC7984 test refactor & fixture
test/token/ERC7984/ERC7984.test.ts
Replaced inline tests with fixture-based deployFixture and exposed deployERC7984Fixture; now delegates behavior verification to shouldBehaveLikeERC7984(contract), updated imports and token address access.
Votes tests access step
test/token/ERC7984/ERC7984Votes.test.ts
Inserted calls to getPastTotalSupplyAccess(blockNumber) after obtaining past-supply handles and before decryption for the two checked blocks.
Freezable tests integration
test/token/ERC7984/extensions/ERC7984Freezable.test.ts
Integrated shared shouldBehaveLikeERC7984 behavior; updated deployment/mint setup to use encrypted input and $_mint and invoked shared tests with freezer role arg.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Owner
  participant Token as ERC7984Mock / ERC7984VotesMock
  participant FHE as FHE.allow

  rect rgba(200,230,255,0.18)
  note over Owner,Token: Owner explicitly grants read access
  Owner->>Token: confidentialTotalSupplyAccess()
  Token->>FHE: allow(confidentialTotalSupply(), Owner)
  FHE-->>Token: granted
  Token-->>Owner: done
  end

  rect rgba(220,255,220,0.18)
  note over Owner,Token: Grant access for historical supply
  Owner->>Token: getPastTotalSupplyAccess(timepoint)
  Token->>FHE: allow(getPastTotalSupply(timepoint), Owner)
  FHE-->>Token: granted
  Token-->>Owner: done
  end
Loading
sequenceDiagram
  participant Caller as ERC7984Freezable/others
  participant Lib as FHESafeMath

  Caller->>Lib: tryDecrease(oldValue, delta)
  alt oldValue uninitialized
    Lib-->>Caller: (success=false, updated=0)
  else delta uninitialized
    Lib-->>Caller: (success=true, updated=oldValue)
  else both initialized
    alt oldValue >= delta
      Lib-->>Caller: (success=true, updated=oldValue - delta)
    else
      Lib-->>Caller: (success=false, updated=oldValue)
    end
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

I twitch my whiskers at tidy code,
Owner grants access down a clearer road.
Math checks seeds both lost and found,
Freezes tally, unfrozen amounts.
Tests hop together, shared and bright —
A rabbit cheers for passing night. 🐇✨

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/confidential-freezable-token-behave-like-erc7984

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbit in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbit in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbit gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbit read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbit help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbit ignore or @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbit summary or @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbit or @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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: 3

🧹 Nitpick comments (8)
contracts/utils/FHESafeMath.sol (1)

33-43: Clarify failure semantics and align NatSpec with behavior

When oldValue is uninitialized, you now return (false, 0). This intentionally returns an initialized zero handle (useful for downstream decryption/allowance flows), but it contradicts the function comment that says “updated will be the original value.” Recommend documenting this special-case to prevent misuse at call sites that rely on the “original value” invariant.

Proposed NatSpec/doc tweak:

- * `success` will be true and `updated` will be the new value. Otherwise, `success` will be false
- * and `updated` will be the original value.
+ * `success` will be true and `updated` will be the new value. Otherwise, `success` will be false.
+ * Failure cases:
+ * - If `oldValue` is initialized and `oldValue < delta`, `updated` equals `oldValue` (preserve).
+ * - If `oldValue` is NOT initialized, `updated` is an initialized zero (to provide a decryptable handle).
test/token/ERC7984/ERC7984Votes.test.ts (2)

264-266: Make the owner context explicit for access helper

getPastTotalSupplyAccess is owner-gated in the mock. This call currently relies on the deployer being the default signer of the contract instance. Make it explicit to avoid future brittleness if the test refactors signer usage.

-      await this.token.getPastTotalSupplyAccess(afterFirstMintBlock);
+      await this.token.connect(this.holder).getPastTotalSupplyAccess(afterFirstMintBlock);

271-275: Repeat explicit owner context for consistency

Same rationale as above.

-      await this.token.getPastTotalSupplyAccess(afterSecondMintBlock);
+      await this.token.connect(this.holder).getPastTotalSupplyAccess(afterSecondMintBlock);
test/token/ERC7984/extensions/ERC7984Freezable.test.ts (1)

61-63: Avoid creating an unused encrypted amount (no-proof path)

The first transaction call to createEncryptedAmount is unused; only the staticCall result is passed to setConfidentialFrozen. Drop the transaction to reduce noise and test time.

-        await token.connect(freezer).createEncryptedAmount(amount);
-        params.push(await token.connect(freezer).createEncryptedAmount.staticCall(amount));
+        params.push(await token.connect(freezer).createEncryptedAmount.staticCall(amount));
contracts/mocks/token/ERC7984VotesMock.sol (1)

33-36: Use an explicit revert reason or a custom error for owner-gate

The bare require hampers debugging. Prefer a custom error (cheaper) or a revert string.

+    error UnauthorizedCaller();
     function getPastTotalSupplyAccess(uint256 timepoint) public {
-        require(msg.sender == _OWNER);
+        if (msg.sender != _OWNER) revert UnauthorizedCaller();
         FHE.allow(getPastTotalSupply(timepoint), msg.sender);
     }
contracts/mocks/token/ERC7984Mock.sol (1)

20-22: Use a custom error (and/or modifier) for owner check; avoid bare require.

Saves gas and improves diagnostics; behavior unchanged.

Apply:

 function confidentialTotalSupplyAccess() public {
-        require(msg.sender == _OWNER);
+        if (msg.sender != _OWNER) revert ERC7984MockUnauthorized();
         FHE.allow(confidentialTotalSupply(), msg.sender);
 }

Add outside this hunk (near the contract header):

error ERC7984MockUnauthorized();
// optionally:
// modifier onlyOwner() { if (msg.sender != _OWNER) revert ERC7984MockUnauthorized(); _; }
contracts/token/ERC7984/extensions/ERC7984Freezable.sol (2)

10-10: Nit: docstring grammar.

“allows implementing” reads better than “allows to implement”.

- * @dev Extension of {ERC7984} that allows to implement a confidential
+ * @dev Extension of {ERC7984} that allows implementing a confidential

34-36: Mark confidentialAvailable as view (if FHE ops permit) and confirm ignoring success is intended.

Returning the second value from tryDecrease makes sense with the new three-branch logic; please confirm that the boolean is not needed anywhere. Also consider declaring the function view for clarity/perf if compatible.

-    function confidentialAvailable(address account) public virtual returns (euint64 unfrozen) {
+    function confidentialAvailable(address account) public view virtual returns (euint64 unfrozen) {
         (, unfrozen) = FHESafeMath.tryDecrease(confidentialBalanceOf(account), confidentialFrozen(account));
     }
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 3a86890 and 5ab8888.

📒 Files selected for processing (10)
  • contracts/mocks/token/ERC7984FreezableMock.sol (1 hunks)
  • contracts/mocks/token/ERC7984Mock.sol (1 hunks)
  • contracts/mocks/token/ERC7984ObserverAccessMock.sol (2 hunks)
  • contracts/mocks/token/ERC7984VotesMock.sol (1 hunks)
  • contracts/token/ERC7984/extensions/ERC7984Freezable.sol (2 hunks)
  • contracts/utils/FHESafeMath.sol (1 hunks)
  • test/token/ERC7984/ERC7984.behaviour.ts (1 hunks)
  • test/token/ERC7984/ERC7984.test.ts (1 hunks)
  • test/token/ERC7984/ERC7984Votes.test.ts (1 hunks)
  • test/token/ERC7984/extensions/ERC7984Freezable.test.ts (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/token/ERC7984/ERC7984.behaviour.ts (2)
test/token/ERC7984/ERC7984.test.ts (1)
  • deployFixture (32-32)
test/helpers/accounts.ts (1)
  • allowHandle (17-24)
test/token/ERC7984/extensions/ERC7984Freezable.test.ts (2)
test/token/ERC7984/ERC7984.test.ts (1)
  • deployFixture (32-32)
test/token/ERC7984/ERC7984.behaviour.ts (1)
  • shouldBehaveLikeERC7984 (596-596)
test/token/ERC7984/ERC7984.test.ts (1)
test/token/ERC7984/ERC7984.behaviour.ts (1)
  • shouldBehaveLikeERC7984 (596-596)
🪛 Gitleaks (8.27.2)
test/token/ERC7984/ERC7984.behaviour.ts

100-100: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


247-247: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


484-484: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

🪛 Biome (2.1.2)
test/token/ERC7984/ERC7984.test.ts

[error] 31-32: Do not export from a test file.

(lint/suspicious/noExportsInTest)

⏰ 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). (4)
  • GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
  • GitHub Check: tests
  • GitHub Check: coverage
  • GitHub Check: slither
🔇 Additional comments (11)
contracts/utils/FHESafeMath.sol (1)

37-41: Delta-uninitialized branch is sensible

Treating an uninitialized delta as a no-op (success = true, updated = oldValue) matches expected “subtract zero” semantics and avoids handle churn. LGTM.

test/token/ERC7984/ERC7984Votes.test.ts (1)

262-270: Assumption on handle stability across calls

The pattern “read handle, then grant access by recomputing the handle internally” assumes getPastTotalSupply returns the same handle on each call. If implementation ever returns a fresh handle per call, access won’t apply to the originally fetched handle.

Two safer options:

  • Call the access helper first, then fetch the handle.
  • Or have the helper accept a handle and call FHE.allow on that handle directly.
test/token/ERC7984/extensions/ERC7984Freezable.test.ts (2)

242-245: Good call: reuse shared ERC7984 behavior suite

Pulling in shouldBehaveLikeERC7984 for the Freezable mock increases coverage and keeps behavior parity. LGTM.


18-31: Fixture setup is consistent and helpful

Pre-minting to the holder simplifies later availability/freeze scenarios and doesn’t interfere with the shared suite (which deploys its own fixture). Looks good.

contracts/mocks/token/ERC7984FreezableMock.sol (3)

31-33: Override resolution updated correctly

Using override(ERC7984, ERC7984Freezable) matches the current inheritance and preserves the linearization. LGTM.


35-38: Helper is fine for tests

Returning an allowed euint64 via createEncryptedAmount and granting both this and msg.sender access is appropriate for test utilities.


40-44: Access helper aligns with new available semantics

confidentialAvailableAccess correctly grants contract and recipient access to the returned handle, which pairs with tryDecrease’s “initialized zero on missing” behavior to ensure decryptability. LGTM.

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

42-44: Override target updated correctly

override(ERC7984, ERC7984Votes) matches the new base resolution. No issues spotted.

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

7-7: Importing ERC7984 directly is correct for override resolution.

Good change; aligns this mock with the real base implementing _update.


22-22: Override specifier is correct

ERC7984ObserverAccessMock properly overrides _update and lists only the two bases—ERC7984ObserverAccess and ERC7984—that define _update. ERC7984Mock does not override _update, so no changes to the override specifier are needed.

test/token/ERC7984/ERC7984.test.ts (1)

18-25: Initial mint via encrypted input looks solid.

The explicit selector usage and input proof handling are correct.

import { ERC7984ReceiverMock } from '../../../types';
import { $ERC7984Mock } from '../../../types/contracts-exposed/mocks/token/ERC7984Mock.sol/$ERC7984Mock';
import { allowHandle } from '../../helpers/accounts';
import { deployERC7984Fixture } from './ERC7984.test';
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

Avoid importing fixtures from a test file; import from a helper instead.

Update import once the fixture is moved out of the test file.

-import { deployERC7984Fixture } from './ERC7984.test';
+import { deployERC7984Fixture } from './fixtures';
🤖 Prompt for AI Agents
In test/token/ERC7984/ERC7984.behaviour.ts around line 4 the test currently
imports deployERC7984Fixture from './ERC7984.test'; move the fixture out of the
test file into a shared helper/fixtures module (e.g. test/helpers/fixtures.ts or
test/fixtures/index.ts) and update this import to point at that helper path;
ensure the fixture is exported with the same name and adjust the relative import
path accordingly.

Comment on lines +452 to +456
// Verify event contents
expect(tx).to.emit(recipientContract, 'ConfidentialTransferCallback').withArgs(callbackSuccess);
const transferEvents = (await tx.wait()).logs.filter((log: any) => log.address === token.target);

const outboundTransferEvent = transferEvents[0];
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

Await the event assertion.

Without await, the assertion may not execute reliably.

-          expect(tx).to.emit(recipientContract, 'ConfidentialTransferCallback').withArgs(callbackSuccess);
+          await expect(tx).to.emit(recipientContract, 'ConfidentialTransferCallback').withArgs(callbackSuccess);
📝 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
// Verify event contents
expect(tx).to.emit(recipientContract, 'ConfidentialTransferCallback').withArgs(callbackSuccess);
const transferEvents = (await tx.wait()).logs.filter((log: any) => log.address === token.target);
const outboundTransferEvent = transferEvents[0];
// Verify event contents
await expect(tx).to.emit(recipientContract, 'ConfidentialTransferCallback').withArgs(callbackSuccess);
const transferEvents = (await tx.wait()).logs.filter((log: any) => log.address === token.target);
const outboundTransferEvent = transferEvents[0];
🤖 Prompt for AI Agents
In test/token/ERC7984/ERC7984.behaviour.ts around lines 452 to 456, the event
assertion using expect(tx).to.emit(...) is not awaited which can make the
assertion flaky; change it to await the assertion (e.g. await
expect(tx).to.emit(recipientContract,
'ConfidentialTransferCallback').withArgs(callbackSuccess)) so the test waits for
the event check to complete before proceeding.

function generateReencryptionErrorMessage(handle: string, account: string): string {
return `User ${account} is not authorized to user decrypt handle ${handle}`;
}
export { deployFixture as deployERC7984Fixture };
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

Fix: remove export from test file to satisfy linter and avoid circular test deps.

Biome flags noExportsInTest. Move the fixture to a dedicated helper and import it where needed.

Minimal change here:

-export { deployFixture as deployERC7984Fixture };

Create a new helper (outside this file), e.g., test/token/ERC7984/fixtures.ts:

import { $ERC7984Mock } from '../../../types/contracts-exposed/mocks/token/ERC7984Mock.sol/$ERC7984Mock';
import { ethers, fhevm } from 'hardhat';

const contract = '$ERC7984Mock';
const name = 'ConfidentialFungibleToken';
const symbol = 'CFT';
const uri = 'https://example.com/metadata';

export async function deployERC7984Fixture(_contract?: string, extraDeploymentArgs: any[] = []) {
  const [holder, recipient, operator, anyone] = await ethers.getSigners();
  const token = (await ethers.deployContract(_contract ?? contract, [name, symbol, uri, ...extraDeploymentArgs])) as any as $ERC7984Mock;
  const encryptedInput = await fhevm.createEncryptedInput(await token.getAddress(), holder.address).add64(1000).encrypt();
  await token.connect(holder)['$_mint(address,bytes32,bytes)'](holder, encryptedInput.handles[0], encryptedInput.inputProof);
  return { token, holder, recipient, operator, anyone };
}
🧰 Tools
🪛 Biome (2.1.2)

[error] 31-32: Do not export from a test file.

(lint/suspicious/noExportsInTest)

🤖 Prompt for AI Agents
In test/token/ERC7984/ERC7984.test.ts around line 32, remove the exported
fixture to satisfy the linter and avoid circular test deps; delete the line
"export { deployFixture as deployERC7984Fixture }" from this test file, create a
new helper file test/token/ERC7984/fixtures.ts that defines and exports
deployERC7984Fixture (implementing the deployment and initial mint logic shown
in the review comment), and update any tests that imported the fixture from the
test file to instead import it from test/token/ERC7984/fixtures.ts.

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: 2

♻️ Duplicate comments (3)
test/token/ERC7984/ERC7984.behaviour.ts (2)

4-4: Fix circular test dependency: import fixture from a helper, not a test.

This import creates a cycle with ERC7984.test.ts and violates linter rules. Move the fixture to a helper (e.g., test/token/ERC7984/fixtures.ts) and import from there.

-import { deployERC7984Fixture } from './ERC7984.test';
+import { deployERC7984Fixture } from './fixtures';

Outside this hunk, add a fixtures helper that exports deployERC7984Fixture, and remove the export from ERC7984.test.ts (see comments in that file).


360-360: Await the event assertion to avoid flakiness.

Not awaiting can skip the assertion under race.

-          expect(tx).to.emit(recipientContract, 'ConfidentialTransferCallback').withArgs(callbackSuccess);
+          await expect(tx).to.emit(recipientContract, 'ConfidentialTransferCallback').withArgs(callbackSuccess);
test/token/ERC7984/ERC7984.test.ts (1)

196-196: Remove export from test file (Biome: noExportsInTest).

Exporting from a test violates the linter and worsens the cycle with behaviour.ts. Export the fixture from a helper instead.

-export { deployFixture as deployERC7984Fixture };
+// Moved to test/token/ERC7984/fixtures.ts:
+// export async function deployERC7984Fixture(...) { ... }

Helper (new file) example:

// test/token/ERC7984/fixtures.ts
import { $ERC7984Mock } from '../../../types/contracts-exposed/mocks/token/ERC7984Mock.sol/$ERC7984Mock';
import { ethers, fhevm } from 'hardhat';

const contract = '$ERC7984Mock';
const name = 'ConfidentialFungibleToken';
const symbol = 'CFT';
const uri = 'https://example.com/metadata';

export async function deployERC7984Fixture(_contract?: string, extraDeploymentArgs: any[] = []) {
  const [holder, recipient, operator, anyone] = await ethers.getSigners();
  const token = (await ethers.deployContract(_contract ?? contract, [name, symbol, uri, ...extraDeploymentArgs])) as any as $ERC7984Mock;
  const encryptedInput = await fhevm.createEncryptedInput(await token.getAddress(), holder.address).add64(1000).encrypt();
  await token.connect(holder)['$_mint(address,bytes32,bytes)'](holder.address, encryptedInput.handles[0], encryptedInput.inputProof);
  return { token, holder, recipient, operator, anyone };
}
🧹 Nitpick comments (4)
test/token/ERC7984/ERC7984.behaviour.ts (2)

187-194: Prefer typed event parsing over index-based selection.

Selecting the first log by address is brittle. Decode with the contract interface for clarity and resilience.

Example (outside this hunk):

const receipt = await tx.wait();
const parsed = receipt.logs
  .map(l => {
    try { return token.interface.parseLog(l); } catch { return undefined; }
  })
  .filter(Boolean);

429-431: Nit: fix error message grammar.

“authorized to user decrypt” → “authorized to user-decrypt” or “authorized to decrypt”.

-  return `User ${account} is not authorized to user decrypt handle ${handle}`;
+  return `User ${account} is not authorized to user-decrypt handle ${handle}`;
test/token/ERC7984/ERC7984.test.ts (2)

25-28: Pass addresses to contract methods expecting address.

Be consistent and explicit; Signer objects are not AddressLike in all contexts.

-  await token
-    .connect(holder)
-    ['$_mint(address,bytes32,bytes)'](holder, encryptedInput.handles[0], encryptedInput.inputProof);
+  await token
+    .connect(holder)
+    ['$_mint(address,bytes32,bytes)'](holder.address, encryptedInput.handles[0], encryptedInput.inputProof);

150-154: Pass recipient address, not Signer, for clarity and type-safety.

-      const tx = await token['confidentialTransfer(address,bytes32,bytes)'](
-        recipient,
+      const tx = await token['confidentialTransfer(address,bytes32,bytes)'](
+        recipient.address,
         encryptedInput.handles[0],
         encryptedInput.inputProof,
       );
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 5ab8888 and 27c50c6.

📒 Files selected for processing (2)
  • test/token/ERC7984/ERC7984.behaviour.ts (1 hunks)
  • test/token/ERC7984/ERC7984.test.ts (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/token/ERC7984/ERC7984.behaviour.ts (2)
test/token/ERC7984/ERC7984.test.ts (1)
  • deployFixture (196-196)
test/helpers/accounts.ts (1)
  • allowHandle (17-24)
🪛 Gitleaks (8.27.2)
test/token/ERC7984/ERC7984.test.ts

73-73: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

test/token/ERC7984/ERC7984.behaviour.ts

154-154: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


391-391: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

🪛 Biome (2.1.2)
test/token/ERC7984/ERC7984.test.ts

[error] 195-196: Do not export from a test file.

(lint/suspicious/noExportsInTest)

⏰ 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). (4)
  • GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
  • GitHub Check: slither
  • GitHub Check: tests
  • GitHub Check: coverage
🔇 Additional comments (1)
test/token/ERC7984/ERC7984.test.ts (1)

193-194: OK: behaviour delegation is clear.

Delegating to the shared behaviour keeps tests DRY.

Comment on lines +89 to +93
it('without operator approval should fail', async function () {
await token.$_setOperator(holder, operator, 0);

await expect(
token
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

Type mismatch: pass addresses to $_setOperator.

Passing Signer objects will mis-encode params; the function expects addresses.

-                  await token.$_setOperator(holder, operator, 0);
+                  await token.$_setOperator(holder.address, operator.address, 0);
📝 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
it('without operator approval should fail', async function () {
await token.$_setOperator(holder, operator, 0);
await expect(
token
it('without operator approval should fail', async function () {
await token.$_setOperator(holder.address, operator.address, 0);
await expect(
token
🤖 Prompt for AI Agents
In test/token/ERC7984/ERC7984.behaviour.ts around lines 89 to 93, the call to
token.$_setOperator is being passed Signer objects which causes ABI mis-encoding
because the function expects address strings; change the arguments to address
values by calling await holder.getAddress() and await operator.getAddress() (or
use the signer.address field if available) so the function receives plain
addresses, and update any other test calls similarly to ensure addresses — not
Signer instances — are passed.

Comment on lines +1 to +2
import { $ERC7984Mock } from '../../../types/contracts-exposed/mocks/token/ERC7984Mock.sol/$ERC7984Mock';
import { shouldBehaveLikeERC7984 } from './ERC7984.behaviour';
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

⚠️ Potential issue

Circular dependency with behaviour suite.

This file imports the behaviour, while the behaviour imports a fixture from this file, causing a cycle and undefined exports at runtime. Move the fixture to a helper module and update both imports.

Planned changes:

  • Create test/token/ERC7984/fixtures.ts exporting deployERC7984Fixture.
  • Update behaviour.ts to import from './fixtures'.
  • Keep a local deployFixture here (non-export) or import from the helper.
🤖 Prompt for AI Agents
In test/token/ERC7984/ERC7984.test.ts around lines 1 to 2, there is a circular
dependency because this test imports the behaviour which in turn imports a
fixture from this file; extract the shared fixture into a new module
test/token/ERC7984/fixtures.ts that exports deployERC7984Fixture, update
test/token/ERC7984/ERC7984.behaviour.ts to import deployERC7984Fixture from
'./fixtures', and update this test file to either import the fixture from
'./fixtures' or keep a local (non-exported) deployFixture helper — ensure no
exports from this test file that the behaviour relies on so the cycle is broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants