Skip to content

Conversation

@arr00
Copy link
Contributor

@arr00 arr00 commented Nov 12, 2025

Summary by CodeRabbit

  • Refactor
    • Optimized internal handling of confidential balance calculations and access control permissions for frozen token operations through improved separation of logic.

@arr00 arr00 requested a review from a team as a code owner November 12, 2025 18:40
@netlify
Copy link

netlify bot commented Nov 12, 2025

Deploy Preview for confidential-tokens ready!

Name Link
🔨 Latest commit 9fbd732
🔍 Latest deploy log https://app.netlify.com/projects/confidential-tokens/deploys/6914d66cb8e56a00088b1d7d
😎 Deploy Preview https://deploy-preview-252--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.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 12, 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

The PR refactors the ERC7984Freezable contract by introducing a new internal helper function _confidentialAvailable() that computes the available unfrozen balance. The public confidentialAvailable() now delegates to this helper and grants FHE ACL allowances. The mock is updated accordingly to call the new helper.

Changes

Cohort / File(s) Summary
Refactor internal available balance computation
contracts/token/ERC7984/extensions/ERC7984Freezable.sol, contracts/mocks/token/ERC7984FreezableMock.sol
Introduced new private internal helper _confidentialAvailable(address) to compute available unfrozen balance. Updated confidentialAvailable(address) to delegate to the helper and grant FHE ACL allowances. Updated _update() to use the new helper. Mock updated to call _confidentialAvailable() instead of confidentialAvailable().

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Verify that _confidentialAvailable() correctly computes the unfrozen balance without side effects
  • Confirm ACL allowances (allowThis and allow(amount, account)) are correctly granted in the delegating public function
  • Ensure all internal callsites consistently use the new helper

Possibly related PRs

Suggested labels

ignore-changeset

Suggested reviewers

  • james-toussaint

Poem

🐰 A helper springs forth from the code,
Unfrozen balances now clearly showed,
Internal secrets, no longer wide,
ACL allowances granted with pride! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically references the main change: granting allowances in the confidentialAvailable function, which aligns with the core modification of adding ACL allowance grants to the function.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

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

🧹 Nitpick comments (1)
contracts/token/ERC7984/extensions/ERC7984Freezable.sol (1)

58-65: Consider clarifying the override guidance.

The comment on line 64 mentions overriding {confidentialAvailable} to change freezing behavior. Since the computation logic is now in _confidentialAvailable, it might be clearer to mention overriding _confidentialAvailable for computation changes, while confidentialAvailable would be overridden for allowance behavior changes.

Consider updating the comment:

- * The default freezing behavior can be changed (for a pass-through for instance) by overriding
- * {confidentialAvailable}.
+ * The default freezing behavior can be changed (for a pass-through for instance) by overriding
+ * {_confidentialAvailable} to change the computation logic or {confidentialAvailable} to change
+ * allowance behavior.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ce46973 and 795ef52.

📒 Files selected for processing (2)
  • contracts/mocks/token/ERC7984FreezableMock.sol (1 hunks)
  • contracts/token/ERC7984/extensions/ERC7984Freezable.sol (2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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).
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: For force transfers in ERC7984Freezable, the frozen balance should be reset to the new balance if the transfer amount exceeded the available balance. If the transfer amount was within the available balance, the frozen amount behavior needs clarification from the user.
📚 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/mocks/token/ERC7984FreezableMock.sol
  • contracts/token/ERC7984/extensions/ERC7984Freezable.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: For force transfers in ERC7984Freezable, the frozen balance should be reset to the new balance if the transfer amount exceeded the available balance. If the transfer amount was within the available balance, the frozen amount behavior needs clarification from the user.

Applied to files:

  • contracts/mocks/token/ERC7984FreezableMock.sol
  • contracts/token/ERC7984/extensions/ERC7984Freezable.sol
📚 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/token/ERC7984/extensions/ERC7984Freezable.sol
⏰ 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). (3)
  • GitHub Check: coverage
  • GitHub Check: slither
  • GitHub Check: tests
🔇 Additional comments (5)
contracts/mocks/token/ERC7984FreezableMock.sol (1)

35-35: LGTM! Correct usage of the new internal helper.

The mock now calls _confidentialAvailable to compute the available balance without automatic allowances, then manually handles allowance granting on lines 36-37. This aligns with the refactored pattern in ERC7984Freezable.sol and gives the mock appropriate control over ACL management.

contracts/token/ERC7984/extensions/ERC7984Freezable.sol (4)

34-39: Well-structured refactor with clear ACL management.

The public function now cleanly delegates computation to _confidentialAvailable and explicitly handles ACL allowances. This maintains the same external behavior while improving code organization.


41-48: Good addition of internal computation helper.

The new internal function cleanly encapsulates the available balance calculation without granting allowances. This enables gas-efficient internal usage (e.g., in _update) and provides a clear extension point for derived contracts.


68-68: Excellent gas optimization using internal helper.

Using _confidentialAvailable here avoids unnecessary ACL allowance grants during internal token transfers, since the computed value is only used within the contract's logic.


41-48: No additional changes needed.

All internal usages already correctly call _confidentialAvailable instead of the public confidentialAvailable. The _update function (line 68) and the public wrapper (line 35) both properly use the internal version, avoiding unnecessary allowance grants where they aren't needed.

@arr00 arr00 merged commit 7f4c759 into OpenZeppelin:master Nov 12, 2025
13 checks passed
@arr00 arr00 deleted the fix/give-allowance-confidentialAvailable branch November 12, 2025 19:09
arr00 added a commit that referenced this pull request Nov 12, 2025
* L-05: Grant allowances in `confidentialAvailable`

* fix doc
arr00 added a commit that referenced this pull request Dec 1, 2025
* Start release candidate

* Release v0.3.0 (rc) (#221)

* Release v0.3.0 (rc)

* Update changelog

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>

* Update `checkOnTransferReceived` doc (#235)

* Versioned Docs (#236)

* generate versioned docs

* publish docs even on pre-release

* N-04: remove unused import in `ERC7984Rwa` (#243)

* N-01: reset user instead of allowing user in `unblockUser` (#244)

* N-05: Named mapping var in `ERC7984ObserverAccess` (#251)

* N-05: Named mapping var in `ERC7984ObserverAccess`

* Update contracts/token/ERC7984/extensions/ERC7984ObserverAccess.sol

Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>

---------

Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>

* N-08: constant names are screaming camel case (#247)

* N-08: constant names are screaming camel case

* fix lint

* N-02: reorder allowances omnibus (#250)

* Support ERC-165 interface detection on ERC-7984 (#246)

* Support ERC-165 interface detection on ERC-7984

* update link format

* Add ERC7984 impl changeset

* Update changeset

---------

Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com>

* M-03: grant allowances to agent in `ERC7984Rwa` (#242)

* M-03: grant allowances to agent in `ERC7984Rwa`

* up

* N-12: update docs in `ERC7984Restricted` (#245)

* Upgrade to use fhevm contracts v0.9.0 (#248)

* chore: fhevm-v9

* chore: port all tests for fhevm v9

* Merge pull request #1 from OpenZeppelin/chore/update-disclose-flow

update disclose flow

* Update wrapper contract (#2)

* Update wrapper contract

* fix tests

* fix mock

* update docs

* add changeset

* request id unnecessary

* Update contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol

Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>

* remove unused params

* Update test/token/ERC7984/ERC7984.test.ts

Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>

* `cts` -> `handles`

* `cleartext` -> `cleartextAmount`

* Update test/token/ERC7984/extensions/ERC7984Wrapper.test.ts

Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>

* nit

---------

Co-authored-by: 0xalexbel <alexandre.belhoste@zama.ai>
Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>

* N-[9,11]: fix `ERC7984Rwa` docs (#249)

* M-11: fix `ERC7984Rwa` docs

* add docs

* Update contracts/token/ERC7984/extensions/ERC7984Rwa.sol

* L-05: Grant allowances in `confidentialAvailable` (#252)

* L-05: Grant allowances in `confidentialAvailable`

* fix doc

* L-01: `tryDecrease` return initialized value if delta is initialized (#241)

* L-01: `tryDecrease` return initialized value if delta is initialized

* add comment

* Add changeset

* Upgrade to use fhevm contracts v0.9.1 (#254)

* Upgrade to use fhevm contracts v0.9.1

* bump sub package as well

* Update `ERC7984Rwa` docs (#255)

* Exit pre-release (#258)

* Release v0.3.0 (#253)

* Release v0.3.0

* Update changelog (#259)

* Update changelog

* Update CHANGELOG.md

Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>

---------

Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>

* remove duplicate entry

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com>
Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>
Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com>
Co-authored-by: 0xalexbel <alexandre.belhoste@zama.ai>
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.

1 participant