Skip to content

Conversation

@james-toussaint
Copy link
Contributor

@james-toussaint james-toussaint commented Oct 29, 2025

Summary by CodeRabbit

  • Documentation
    • Updated documentation clarifying fallback behavior when a receiver contract returns false. The transfer function will now attempt to refund the initiating address instead of reversing the transfer.

@james-toussaint james-toussaint requested a review from arr00 October 29, 2025 14:41
@netlify
Copy link

netlify bot commented Oct 29, 2025

Deploy Preview for confidential-tokens ready!

Name Link
🔨 Latest commit 8ff23ad
🔍 Latest deploy log https://app.netlify.com/projects/confidential-tokens/deploys/69022796d5bf690008b662ac
😎 Deploy Preview https://deploy-preview-235--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 Oct 29, 2025

Walkthrough

Documentation comment updated in ERC7984Utils for the checkOnTransferReceived function, clarifying fallback behavior when the receiver contract returns false. The description changed from indicating a transfer reversal to suggesting a refund attempt to the sender address. No functional code modifications were made.

Changes

Cohort / File(s) Change Summary
Documentation Update
contracts/token/ERC7984/utils/ERC7984Utils.sol
Updated NatSpec comment for checkOnTransferReceived to clarify fallback behavior: changed from "transfer would be reversed" to "transfer function should attempt to refund the from address instead" when receiver returns false.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

📜✨ A clarified note, so precise and fine,
Our refund logic now shines in each line,
Words rearranged with intention so clear—
The rabbit approves, no changes to fear! 🐰

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Update checkOnTransferReceived doc" directly and accurately summarizes the main change in the changeset. The raw summary confirms that the pull request updates the documentation comment for the checkOnTransferReceived function to change the described fallback behavior when the receiver contract returns false, with no actual code changes present. The title is concise, specific to the function being modified, and clearly indicates this is a documentation update rather than a functional change. A developer scanning the repository history would immediately understand that this commit pertains to documentation improvements for a specific utility function.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/check-on-transfer-received-note

📜 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 0024cc7 and 8ff23ad.

📒 Files selected for processing (1)
  • contracts/token/ERC7984/utils/ERC7984Utils.sol (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-15T14:43:25.644Z
Learnt from: arr00
PR: OpenZeppelin/openzeppelin-confidential-contracts#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/utils/ERC7984Utils.sol
🔇 Additional comments (1)
contracts/token/ERC7984/utils/ERC7984Utils.sol (1)

19-20: Documentation clarification is accurate and properly implemented.

The documentation update correctly describes the expected behavior. Verification shows that the transfer implementation in ERC7984._transferAndCall() (line 264) properly handles the encrypted boolean return value using FHE.select(success, FHE.asEuint64(0), sent), which conditionally refunds the sender based on the callback's encrypted boolean response.


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

@james-toussaint james-toussaint marked this pull request as ready for review October 29, 2025 15:11
@james-toussaint james-toussaint requested a review from a team as a code owner October 29, 2025 15:11
@james-toussaint james-toussaint merged commit 89b9ba4 into master Oct 29, 2025
15 of 16 checks passed
@james-toussaint james-toussaint deleted the fix/check-on-transfer-received-note branch October 29, 2025 15:25
arr00 pushed 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.

3 participants