Skip to content

Conversation

@arr00
Copy link
Contributor

@arr00 arr00 commented Nov 11, 2025

Summary by CodeRabbit

  • Refactor
    • Internal code reorganization to improve structure and maintainability. No functional changes or new features.

@arr00 arr00 requested a review from a team as a code owner November 11, 2025 20:20
@netlify
Copy link

netlify bot commented Nov 11, 2025

Deploy Preview for confidential-tokens ready!

Name Link
🔨 Latest commit 3a06a24
🔍 Latest deploy log https://app.netlify.com/projects/confidential-tokens/deploys/69139aa06aa04f0008349681
😎 Deploy Preview https://deploy-preview-250--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 11, 2025

Walkthrough

Reorders the declaration of the transferred local variable in _confidentialTransferFromAndCallOmnibus to occur after ACL allowance blocks for sender and recipient instead of before. No behavioral changes; the variable is still emitted and used identically.

Changes

Cohort / File(s) Summary
Variable Declaration Reordering
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol
Moves transferred variable declaration from before ACL allowance blocks to after, maintaining functional equivalence

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • Omnibus Extension #186: Directly related as it introduced the _confidentialTransferFromAndCallOmnibus function that is being modified in this PR.

Suggested reviewers

  • james-toussaint

Poem

🐰 A variable moves, yet logic stays true,
Reordered with care, no harm comes through.
The bits rearrange in the order they go,
Transferred remains where it needs to flow! ✨

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 identifies the specific change: reordering allowance logic in the omnibus contract.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 973820f and 3a06a24.

📒 Files selected for processing (1)
  • contracts/token/ERC7984/extensions/ERC7984Omnibus.sol (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/ERC7984Omnibus.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/extensions/ERC7984Omnibus.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: slither
  • GitHub Check: coverage
  • GitHub Check: tests
🔇 Additional comments (1)
contracts/token/ERC7984/extensions/ERC7984Omnibus.sol (1)

199-200: LGTM! Improved consistency with the sibling function.

Moving the transferred variable declaration to after the ACL allowances for sender and recipient aligns this function with the pattern used in _confidentialTransferFromOmnibus (line 177), improving code consistency and logical ordering of operations.


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

Copy link
Contributor

@james-toussaint james-toussaint left a comment

Choose a reason for hiding this comment

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

Ok for consistency ✅

@arr00 arr00 merged commit 8412a63 into OpenZeppelin:master Nov 12, 2025
13 of 14 checks passed
@arr00 arr00 deleted the fix/reorder-allowances branch November 12, 2025 14:54
arr00 added a commit that referenced this pull request Nov 12, 2025
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.

2 participants