Conversation
* 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>
* generate versioned docs * publish docs even on pre-release
* 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 * fix lint
* 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` * up
* 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>
* M-11: fix `ERC7984Rwa` docs * add docs * Update contracts/token/ERC7984/extensions/ERC7984Rwa.sol
* L-05: Grant allowances in `confidentialAvailable` * fix doc
…241) * L-01: `tryDecrease` return initialized value if delta is initialized * add comment * Add changeset
* Upgrade to use fhevm contracts v0.9.1 * bump sub package as well
* 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>
* Revert when wrapper full * up * simplify and add tests * update docs * cei * add doc * up * add custom error and fix tests * comment clarification * Rename `_checkTotalSupply` to `_checkConfidentialTotalSupply` * add changeset * Update contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol * docs --------- Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Add missing docs
#284) Rename `totalSupply()` to `inferredTotalSuppy()`
* Release v0.3.1 * update changelog --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com>
✅ Deploy Preview for confidential-tokens ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request releases version 0.3.1 of OpenZeppelin Confidential Contracts, featuring a refactored ERC7984 disclosure mechanism, unwrap flow changes in the ERC20 wrapper, ERC165 interface support, dependency updates to Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ERC7984 as ERC7984<br/>(Smart Contract)
participant FHE as FHE Library
participant Decryptor as Public Decryptor
User->>ERC7984: requestDiscloseEncryptedAmount(encryptedAmount)
ERC7984->>FHE: makePubliclyDecryptable(encryptedAmount)
ERC7984->>ERC7984: emit AmountDiscloseRequested(encryptedAmount, requester)
Note over Decryptor: Off-chain: Decrypts encryptedAmount<br/>and generates decryptionProof
User->>ERC7984: discloseEncryptedAmount(encryptedAmount, cleartextAmount, decryptionProof)
ERC7984->>FHE: checkSignatures(handles, abi.encode(cleartextAmount), decryptionProof)
alt Verification Successful
ERC7984->>ERC7984: emit AmountDisclosed(encryptedAmount, cleartextAmount)
else Verification Failed
ERC7984-->>User: revert
end
sequenceDiagram
actor User
participant ERC7984ERC20Wrapper as ERC7984ERC20Wrapper<br/>(Smart Contract)
participant FHE as FHE Library
participant Decryptor as Public Decryptor
User->>ERC7984ERC20Wrapper: _unwrap(from, to, encryptedAmount)
ERC7984ERC20Wrapper->>ERC7984ERC20Wrapper: _burn(from, encryptedAmount)
ERC7984ERC20Wrapper->>ERC7984ERC20Wrapper: _checkConfidentialTotalSupply()
alt Supply Overflow
ERC7984ERC20Wrapper-->>User: revert ERC7984TotalSupplyOverflow()
end
ERC7984ERC20Wrapper->>FHE: makePubliclyDecryptable(encryptedAmount)
ERC7984ERC20Wrapper->>ERC7984ERC20Wrapper: _receivers[encryptedAmount] = to
ERC7984ERC20Wrapper->>ERC7984ERC20Wrapper: emit UnwrapRequested(to, encryptedAmount)
Note over Decryptor: Off-chain: Decrypts encryptedAmount<br/>and generates decryptionProof
User->>ERC7984ERC20Wrapper: finalizeUnwrap(encryptedAmount, cleartextAmount, decryptionProof)
ERC7984ERC20Wrapper->>ERC7984ERC20Wrapper: lookup recipient from _receivers[encryptedAmount]
ERC7984ERC20Wrapper->>FHE: checkSignatures(handles, abi.encode(cleartextAmount), decryptionProof)
alt Verification Successful
ERC7984ERC20Wrapper->>ERC7984ERC20Wrapper: transfer cleartextAmount \\* rate() underlying tokens to recipient
ERC7984ERC20Wrapper->>ERC7984ERC20Wrapper: emit UnwrapFinalized(recipient, encryptedAmount, cleartextAmount)
else Verification Failed
ERC7984ERC20Wrapper-->>User: revert InvalidUnwrapRequest()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @contracts/utils/FHESafeMath.sol:
- Around line 11-12: Fix the grammar in the FHESafeMath library comment: replace
the phrase "will may return an uninitialized value" with a correct wording such
as "may return an uninitialized value" (or "will return an uninitialized value"
if you mean certainty) in the top-of-file doc comment so the sentence reads
correctly and conveys intended meaning.
In @test/token/ERC7984/ERC7984.test.ts:
- Around line 540-545: The requester variable is typed as HardhatEthersSigner |
undefined but is assigned this.holder.address (a string) and later used as an
address; change requester to a string type (e.g., string | undefined) or
consistently use a signer object instead—update the declaration for requester
and any places that set or pass it (references: requester, this.holder.address,
and any tests that use requester as an address) so the types match and no
undefined/signer-to-string mismatch remains.
🧹 Nitpick comments (4)
CHANGELOG.md (1)
22-22: Optional: Minor style improvements.Static analysis suggests two minor style improvements:
- Line 22: Consider using a synonym for "given" (e.g., "granted" or "provided")
- Line 25: Consider hyphenating "Real-World Assets" as a compound adjective
These are stylistic nitpicks and can be safely deferred.
Also applies to: 25-25
test/token/ERC7984/extensions/ERC7984Wrapper.test.ts (1)
364-370: Helper function assumes singleUnwrapRequestedevent.The helper always takes the first event from
queryFilter. If multiple unwrap requests exist in the same test context (e.g., frombeforeEachsetup or prior test steps), this could retrieve the wrong event. Consider filtering by block number or clearing events between tests.♻️ Suggested improvement
-async function publicDecryptAndFinalizeUnwrap(wrapper: ERC7984ERC20WrapperMock, caller: HardhatEthersSigner) { - const [to, amount] = (await wrapper.queryFilter(wrapper.filters.UnwrapRequested()))[0].args; +async function publicDecryptAndFinalizeUnwrap(wrapper: ERC7984ERC20WrapperMock, caller: HardhatEthersSigner) { + const events = await wrapper.queryFilter(wrapper.filters.UnwrapRequested()); + const [to, amount] = events[events.length - 1].args; // Get the most recent eventcontracts/token/ERC7984/extensions/ERC7984Rwa.sol (1)
243-247: Hardcoded function selectors are fragile and error-prone.The selectors
0x6c9c3c85and0x44fd6e40are computed from function signatures and must be manually updated if function signatures change. Consider using computed selectors for maintainability.♻️ Suggested improvement
function _isForceTransfer() private pure returns (bool) { return - msg.sig == 0x6c9c3c85 || // bytes4(keccak256("forceConfidentialTransferFrom(address,address,bytes32)")) - msg.sig == 0x44fd6e40; // bytes4(keccak256("forceConfidentialTransferFrom(address,address,bytes32,bytes)")) + msg.sig == this.forceConfidentialTransferFrom.selector || + msg.sig == bytes4(keccak256("forceConfidentialTransferFrom(address,address,bytes32,bytes)")); }Note: The first overload's selector can be obtained via
.selector. For the second, you may need to compute it or use a constant.contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol (1)
200-201:assertfor request uniqueness could panic in edge cases.If two burns somehow produce the same
burntAmounthandle (extremely unlikely given FHE guarantees), this assert would panic. Consider usingrequirewith a custom error for better error handling, though this is likely over-cautious given FHE handle uniqueness.♻️ Optional: Use require for clearer error handling
- assert(_unwrapRequests[burntAmount] == address(0)); + require(_unwrapRequests[burntAmount] == address(0), "Duplicate unwrap handle"); _unwrapRequests[burntAmount] = to;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
.changeset/beige-fans-call.md.changeset/fast-ravens-lose.md.changeset/floppy-otters-dance.md.changeset/floppy-wasps-clap.md.changeset/fruity-bananas-smile.md.changeset/new-crews-boil.md.changeset/public-dolls-lose.md.changeset/seven-books-dig.md.changeset/six-maps-follow.md.changeset/sour-ties-warn.md.changeset/stupid-fans-bow.md.github/workflows/docs.ymlCHANGELOG.mdcontracts/finance/ERC7821WithExecutor.solcontracts/finance/VestingWalletCliffConfidential.solcontracts/finance/VestingWalletConfidential.solcontracts/finance/VestingWalletConfidentialFactory.solcontracts/interfaces/IERC7984.solcontracts/interfaces/IERC7984Receiver.solcontracts/interfaces/IERC7984Rwa.solcontracts/mocks/docs/SwapERC7984ToERC20.solcontracts/mocks/finance/VestingWalletCliffConfidentialMock.solcontracts/mocks/finance/VestingWalletConfidentialFactoryMock.solcontracts/mocks/finance/VestingWalletConfidentialMock.solcontracts/mocks/token/ERC7984ERC20WrapperMock.solcontracts/mocks/token/ERC7984FreezableMock.solcontracts/mocks/token/ERC7984Mock.solcontracts/mocks/token/ERC7984ObserverAccessMock.solcontracts/mocks/token/ERC7984ReceiverMock.solcontracts/mocks/token/ERC7984RestrictedMock.solcontracts/mocks/token/ERC7984RwaMock.solcontracts/mocks/utils/FHESafeMathMock.solcontracts/mocks/utils/HandleAccessManagerMock.solcontracts/mocks/utils/HandleAccessManagerUserMock.solcontracts/package.jsoncontracts/token/ERC7984/ERC7984.solcontracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.solcontracts/token/ERC7984/extensions/ERC7984Freezable.solcontracts/token/ERC7984/extensions/ERC7984ObserverAccess.solcontracts/token/ERC7984/extensions/ERC7984Omnibus.solcontracts/token/ERC7984/extensions/ERC7984Restricted.solcontracts/token/ERC7984/extensions/ERC7984Rwa.solcontracts/token/ERC7984/extensions/ERC7984Votes.solcontracts/token/ERC7984/utils/ERC7984Utils.solcontracts/utils/FHESafeMath.solcontracts/utils/structs/temporary-Checkpoints.soldocs/antora.ymlpackage.jsonscripts/update-docs-branch.jstest/helpers/accounts.tstest/token/ERC7984/ERC7984.test.tstest/token/ERC7984/extensions/ERC7984Rwa.test.tstest/token/ERC7984/extensions/ERC7984Wrapper.test.tstest/utils/FHESafeMath.test.ts
💤 Files with no reviewable changes (12)
- .changeset/fast-ravens-lose.md
- .changeset/sour-ties-warn.md
- .changeset/fruity-bananas-smile.md
- .changeset/floppy-wasps-clap.md
- .changeset/floppy-otters-dance.md
- .changeset/six-maps-follow.md
- .changeset/new-crews-boil.md
- .changeset/seven-books-dig.md
- .changeset/beige-fans-call.md
- .changeset/stupid-fans-bow.md
- .changeset/public-dolls-lose.md
- contracts/mocks/token/ERC7984ObserverAccessMock.sol
🧰 Additional context used
🧠 Learnings (3)
📚 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.solcontracts/mocks/token/ERC7984ReceiverMock.solcontracts/token/ERC7984/utils/ERC7984Utils.solcontracts/token/ERC7984/extensions/ERC7984Freezable.solcontracts/token/ERC7984/ERC7984.solcontracts/token/ERC7984/extensions/ERC7984Rwa.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.soltest/token/ERC7984/ERC7984.test.tscontracts/token/ERC7984/extensions/ERC7984Freezable.solcontracts/token/ERC7984/ERC7984.solcontracts/interfaces/IERC7984Rwa.solcontracts/mocks/docs/SwapERC7984ToERC20.solcontracts/mocks/token/ERC7984FreezableMock.solcontracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.solcontracts/token/ERC7984/extensions/ERC7984Rwa.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/token/ERC7984/extensions/ERC7984Freezable.solcontracts/token/ERC7984/ERC7984.solcontracts/mocks/token/ERC7984FreezableMock.solcontracts/token/ERC7984/extensions/ERC7984Rwa.sol
🧬 Code graph analysis (3)
test/helpers/accounts.ts (1)
hardhat/remappings.js (1)
fs(1-1)
test/token/ERC7984/extensions/ERC7984Rwa.test.ts (1)
test/helpers/interface.ts (1)
getFunctions(4-8)
test/token/ERC7984/ERC7984.test.ts (1)
test/helpers/interface.ts (2)
getFunctions(4-8)getInterfaceId(10-15)
🪛 LanguageTool
CHANGELOG.md
[style] ~22-~22: Try using a synonym here to strengthen your writing.
Context: ... each account to add an observer who is given access to their transfer and balance am...
(GIVE_PROVIDE)
[uncategorized] ~25-~25: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...f ERC7984, that supports confidential Real World Assets (RWAs). ([#160](https://github.c...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ 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). (1)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
🔇 Additional comments (68)
contracts/token/ERC7984/utils/ERC7984Utils.sol (2)
2-2: Header comment updated correctly.The version metadata is now aligned with the v0.3.0 release. The full file path is also included for clarity.
19-20: Documentation clarification is helpful.The updated comment now explicitly documents the refund behavior when the callback returns
false. This aligns the documentation with standard callback patterns and makes the expected behavior clear to callers.Please verify that all ERC7984 transfer implementations that use
checkOnTransferReceivedproperly handle thefalsereturn value by attempting to refund thefromaddress as documented.contracts/token/ERC7984/extensions/ERC7984ObserverAccess.sol (2)
2-2: LGTM! Version header updated correctly.The header comment now reflects the v0.3.0 version, aligning with this release.
14-14: LGTM! Named mapping key improves readability.Adding the
accountidentifier to the mapping key is a best practice that enhances code documentation without any functional changes.contracts/token/ERC7984/extensions/ERC7984Restricted.sol (3)
2-2: LGTM: Header comment addition.The addition of the OpenZeppelin Confidential Contracts header with version tracking is good practice for maintaining library version information.
54-54: LGTM: English spelling standardization.The standardization to American English spelling ("behavior" instead of "behaviour") improves consistency in the documentation.
86-86: LGTM: Corrected error reference in documentation.The doc comment now correctly references
{UserRestricted}(defined at line 30) instead of the incorrect{ERC20Restricted}. This improves documentation accuracy and helps developers understand the actual revert error.contracts/mocks/utils/FHESafeMathMock.sol (1)
4-4: LGTM! Configuration migration looks correct.The migration from
SepoliaConfigtoZamaEthereumConfigaligns with the broader configuration update across the codebase and introduces no behavioral changes to the mock contract.Also applies to: 8-8
contracts/utils/FHESafeMath.sol (1)
34-43: Improved edge-case handling for uninitialized values.The updated logic in
tryDecreasenow correctly handles the case whereoldValueis uninitialized butdeltais initialized. When attempting to subtract an initialized value from an uninitialized value (0), the function now:
- Returns success only if
deltais 0 (since 0 - 0 = 0 is valid)- Returns an explicit 0 instead of an uninitialized value
This is more semantically consistent with the operation's intent.
test/utils/FHESafeMath.test.ts (1)
64-67: LGTM! Test expectations updated to match new behavior.The test cases have been correctly updated to expect an explicit
0result instead of an undefined value whenoldValueis uninitialized intryDecrease. This aligns with the implementation changes inFHESafeMath.solwhere the function now returnsFHE.asEuint64(0)instead of leaving the result uninitialized.docs/antora.yml (1)
3-3: LGTM!Version bump to 0.3 aligns with the broader release and is consistent with the contract header updates to v0.3.0.
.github/workflows/docs.yml (1)
5-5: Verify the workflow trigger change is intentional.The docs workflow will no longer trigger on pushes to
master. It will only build docs when pushing torelease-v*branches. Ensure this aligns with the intended documentation release workflow and that docs are still properly published when releases are merged to master.contracts/utils/structs/temporary-Checkpoints.sol (1)
2-2: LGTM!Header version update to v0.3.0 is consistent with the release. No functional changes to the Checkpoints library implementation.
contracts/interfaces/IERC7984Receiver.sol (1)
2-2: LGTM!Header version and filename reference updated appropriately for v0.3.0. The interface signature remains correct for the ERC7984 receiver callback pattern.
contracts/mocks/token/ERC7984FreezableMock.sol (2)
5-5: LGTM!Config import migration from
SepoliaConfigtoZamaEthereumConfigis consistent with the broader mock updates across the repository.
34-38: The use of_confidentialAvailableis correct.The internal method
_confidentialAvailableis properly defined inERC7984Freezablewith the signaturefunction _confidentialAvailable(address account) internal virtual returns (euint64), andERC7984FreezableMockinherits from this contract. The change fromconfidentialAvailable(account)to_confidentialAvailable(account)is valid and aligns with the internal balance calculation pattern.contracts/finance/VestingWalletConfidentialFactory.sol (1)
2-2: LGTM!Header version update to v0.3.0 is consistent with the release. The factory implementation for confidential vesting wallets remains unchanged and correct.
contracts/token/ERC7984/extensions/ERC7984Votes.sol (1)
2-2: LGTM!Header version and path reference updated appropriately for v0.3.0. The ERC7984Votes extension implementation for confidential voting remains correct.
contracts/finance/ERC7821WithExecutor.sol (1)
17-19: LGTM! Storage constant naming aligned with Solidity conventions.The rename to
ERC7821_WITH_EXECUTOR_STORAGE_LOCATIONfollows the idiomatic SCREAMING_SNAKE_CASE convention for constants, eliminating the need for the previous solhint-disable directive. The storage slot value and assembly usage remain correct.Also applies to: 40-44
contracts/package.json (1)
34-37: Version bump and peer dependency update look correct.The
@fhevm/soliditypeer dependency is pinned to exactly0.9.1while other peer dependencies use caret ranges (^5.4.0). This is likely intentional given FHE compatibility requirements, but consumers will need to use exactly this version.scripts/update-docs-branch.js (2)
30-35: Docs branch naming logic looks correct.The conditional logic correctly handles the 0.x series (docs-v0.{minor}.x) separately from major versions (docs-v{major}.x), which aligns with semantic versioning conventions where 0.x versions may have breaking changes between minor versions.
44-46: No changes needed. The Set destructuring at line 45 works correctly in JavaScript—when destructuring a Set via its iterator with[first, ...rest], therestoperator correctly produces an array with the remaining elements. The code properly detects multiple unique matching branches by checking ifothers.length > 0after deduplication. This approach is sound and idiomatic.contracts/token/ERC7984/extensions/ERC7984Freezable.sol (2)
34-49: Good refactoring separating ACL grants from internal balance calculation.The split between
confidentialAvailable(public, grants ACLs) and_confidentialAvailable(internal, no ACLs) is a clean separation of concerns. This avoids unnecessary gas costs from ACL grants during internal gating operations in_update.
68-74: Internal gating correctly uses_confidentialAvailable.The
_updatefunction now uses the internal_confidentialAvailableto avoid granting ACL allowances during balance checks. The gating logic remains correct: ifencryptedAmount > unfrozen, the transfer amount is set to 0.contracts/interfaces/IERC7984.sol (2)
6-9: LGTM! ERC165 inheritance enables standard interface discovery.Extending
IERC165allows callers to usesupportsInterfaceto detect ERC7984 support, following established Ethereum interface discovery patterns. This is a backwards-compatible addition.
36-37: Documentation updated with ERC-7572 reference.The
contractURI()documentation now includes a direct link to the ERC-7572 specification, improving discoverability.test/helpers/accounts.ts (1)
20-22: ACL ABI path requires actual dependency installation to verify.The code references
node_modules/@fhevm/host-contracts/artifacts/contracts/ACL.json, but this path cannot be verified in the sandbox environment where node_modules is not available. While the package.json lists the expected dependencies (@fhevm/hardhat-plugin 0.3.0-1, @fhevm/solidity 0.9.1), actual verification that the ACL.json file exists at this path and exports the correct ABI structure requires runningnpm installin a non-sandbox environment.package.json (1)
48-49: @zama-fhe/relayer-sdk 0.3.0-5 is the officially required minimum version.According to Zama's documentation,
@zama-fhe/relayer-sdk>= 0.3.0-5 is required, so using 0.3.0-5 is not a stability concern but the expected version for this release. However,@fhevm/hardhat-plugin0.3.0-1 remains a prerelease version without a stable 0.3.0 release on npm—confirm this is intentional for the 0.3.1 release.Likely an incorrect or invalid review comment.
contracts/finance/VestingWalletConfidential.sol (2)
2-2: LGTM: Version header updated to v0.3.0.The OpenZeppelin Confidential Contracts version header has been correctly updated to reflect the new release.
37-38: LGTM: Storage constant standardized to UPPER_SNAKE_CASE.The storage location constant has been renamed from
VestingWalletStorageLocationtoVESTING_WALLET_STORAGE_LOCATION, following Solidity naming conventions for constants. The assembly reference has been correctly updated, and the storage slot hash remains unchanged, ensuring no impact on existing storage layout.Also applies to: 138-138
contracts/mocks/token/ERC7984RwaMock.sol (2)
6-6: LGTM: Import added to support interface override.The ERC7984 import is necessary for the
supportsInterfaceoverride declaration.
20-22: LGTM: Standard diamond inheritance resolution.The
supportsInterfaceoverride correctly resolves the diamond problem by explicitly specifying both parent contracts and delegating tosuper. This ensures proper interface detection for both ERC7984 and ERC7984Rwa interfaces.contracts/token/ERC7984/extensions/ERC7984Omnibus.sol (2)
2-2: LGTM: Version header updated to v0.3.0.
192-207: LGTM: ACL ordering correctly handles transfer flow.The ACL setup is properly sequenced:
- Sender/recipient ACLs established before transfer (lines 192-198)
- Transfer executed to get actual transferred amount (line 200)
- Transferred amount ACLs set up post-transfer (lines 202-204)
- Event emitted with all encrypted values properly accessible (line 206)
This ordering ensures ACLs are available when needed while accounting for the fact that the actual transferred amount may differ from the requested amount.
contracts/finance/VestingWalletCliffConfidential.sol (2)
2-2: LGTM: Version header updated to v0.3.0.
19-20: LGTM: Storage constant standardized to UPPER_SNAKE_CASE.The storage location constant follows the same naming convention update as other vesting contracts. The assembly reference is correctly updated, and the storage slot hash remains unchanged, preserving the existing storage layout.
Also applies to: 69-69
contracts/mocks/token/ERC7984ERC20WrapperMock.sol (1)
4-4: Configuration import is correct and compatible with @fhevm/solidity v0.9.1.
ZamaEthereumConfigis the unified configuration export introduced in @fhevm/solidity v0.9.1 to replaceSepoliaConfig. The import path and contract inheritance are correctly implemented.contracts/mocks/finance/VestingWalletConfidentialMock.sol (1)
4-4: LGTM! Consistent config migration.The migration to
ZamaEthereumConfigis consistent with the broader configuration changes in this PR.Also applies to: 7-7
contracts/mocks/token/ERC7984ReceiverMock.sol (1)
4-4: LGTM! Config migration applied correctly.The switch to
ZamaEthereumConfigfollows the same pattern as other mock contracts in this PR.Also applies to: 8-8
contracts/mocks/utils/HandleAccessManagerMock.sol (1)
4-4: LGTM! Config migration consistent.The configuration change aligns with the broader migration to
ZamaEthereumConfigacross all mock contracts.Also applies to: 8-8
contracts/mocks/token/ERC7984Mock.sol (1)
5-5: Migration to ZamaEthereumConfig is consistent and complete.The change from
SepoliaConfigtoZamaEthereumConfigaligns with the @fhevm/solidity v0.9.1 dependency and is applied uniformly across all mock contracts in the repository (11 contracts total). All claims in the review are verified as correct.Recommend running the test suite to ensure ERC7984Mock and its dependent tests pass with the new configuration.
test/token/ERC7984/extensions/ERC7984Rwa.test.ts (1)
25-30: No change needed—interface composition is correct and consistent with contract implementation.The pattern using
[IERC7984__factory, IERC165__factory]is already established in the baseERC7984.test.ts(line 56) and correctly reflects how the contract hierarchy works. BothERC7984andERC7984Rwainherit fromERC165and theirsupportsInterfaceimplementations delegate through the inheritance chain, which means they support the combined interface ID of IERC7984 + IERC165 functions. The test expectations align with the contract's actual behavior wheresupportsInterfacereturnstruefor IERC7984, IERC165, and (for ERC7984Rwa) IERC7984Rwa interface IDs.contracts/mocks/token/ERC7984RestrictedMock.sol (1)
5-5: LGTM: Config migration is consistent.The migration to
ZamaEthereumConfigfollows the same pattern as other mocks in this PR. The implementation logic remains unchanged.Also applies to: 9-9
CHANGELOG.md (2)
4-9: LGTM: Clear bug fix documentation.The v0.3.1 changelog entry appropriately documents the overflow protection fix in
ERC7984ERC20Wrapper.
10-31: Documentation expanded appropriately.The v0.3.0 changelog provides comprehensive coverage of the token and utility changes, including ERC165 support, new extensions, and FHESafeMath enhancements.
contracts/mocks/finance/VestingWalletCliffConfidentialMock.sol (1)
4-4: LGTM: Consistent config migration.The config update is consistent with the project-wide migration pattern.
Also applies to: 7-7
contracts/mocks/finance/VestingWalletConfidentialFactoryMock.sol (2)
4-4: LGTM: Config migration follows established pattern.The import and inheritance updates align with the project-wide migration to
ZamaEthereumConfig.Also applies to: 10-10, 49-53
68-68: No changes needed. The code at line 68 correctly usesZamaConfig.getEthereumCoprocessorConfig(), which aligns with the contract's inheritance ofZamaEthereumConfig. No evidence exists of prior Sepolia configuration usage or any recent change between config types. The implementation is consistent with test usage and operates as intended.Likely an incorrect or invalid review comment.
contracts/mocks/utils/HandleAccessManagerUserMock.sol (1)
4-4: Config migration to ZamaEthereumConfig is complete and consistent across all mock contracts.The migration is clean and fully applied. All 11 mock contracts now use ZamaEthereumConfig imported from "@fhevm/solidity/config/ZamaConfig.sol" with no remaining SepoliaConfig references. Verify that this config change aligns with your deployment targets and doesn't break existing tests.
test/token/ERC7984/extensions/ERC7984Wrapper.test.ts (4)
85-106: Good coverage for max amount wrapping scenario.The test correctly validates that the maximum underlying balance (derived from
maxTotalSupply * rate) wraps successfully and results in the expected confidential balance.
108-124: Overflow protection test looks correct.The test properly verifies that depositing beyond
maxTotalSupplyreverts withERC7984TotalSupplyOverflow. The two-step approach (deposit max, then try to deposit more) is a good way to test boundary conditions.
283-312: Good coverage for invalid signature scenario.The test correctly simulates a corrupted decryption proof by truncating it. Using
.to.be.revertedwithout a specific error is acceptable here since the revert comes from the FHE library.
314-318: LGTM!The test correctly verifies that finalizing a non-existent unwrap request reverts with
InvalidUnwrapRequest.test/token/ERC7984/ERC7984.test.ts (2)
54-67: ERC165 interface support tests look good.The tests correctly verify that the token supports both
IERC7984andIERC165interfaces, and returnsfalsefor an unsupported interface ID. The use of helper functionsgetFunctionsandgetInterfaceIdkeeps the test clean.
596-619:afterEachpattern for disclosure verification is well-structured.The afterEach correctly:
- Skips if no expected values are set
- Validates the
AmountDiscloseRequestedevent args- Performs public decryption and verifies the
AmountDisclosedeventThis pattern ensures consistent verification across all disclosure tests.
contracts/token/ERC7984/ERC7984.sol (3)
69-72: ERC165 implementation is correct.The
supportsInterfacecorrectly returnstrueforIERC7984interface ID and delegates tosuper.supportsInterfaceforIERC165support.
201-216:requestDiscloseEncryptedAmountimplementation looks correct.The function properly:
- Validates caller has access to the encrypted amount
- Makes the value publicly decryptable
- Emits the
AmountDiscloseRequestedevent
218-235:discloseEncryptedAmountis permissionless - consider access control.The function can be called by anyone with valid decryption proof, even without prior
requestDiscloseEncryptedAmount. While the NatSpec notes this, consider whether this is intentional. An attacker could potentially disclose amounts they obtained proofs for through other means.If this is intentional (e.g., allowing off-chain signature collection), the design is acceptable. If disclosure should be tied to a prior request, consider tracking requests and validating them here.
contracts/token/ERC7984/extensions/ERC7984Rwa.sol (3)
22-33: Comprehensive documentation for AGENT_ROLE.The NatSpec clearly documents the capabilities granted by the agent role, which is helpful for auditors and developers.
51-56:supportsInterfaceoverride is correctly simplified.The function now only checks for
IERC7984Rwainterface and delegates to the parent chain, which already includesIERC7984andIERC165support.
94-96: Implementation is correct—no action required.The semantics of
_resetUserproperly unblock the user as expected._resetUsersets the restriction toDEFAULT(the unblocked state in the blocklist model), which is the correct inverse of_blockUser(which setsBLOCKED). Using_allowUserinstead would be semantically incorrect, as it sets the restriction toALLOWED, which is an explicit allowlist state rather than an unblocked state.contracts/mocks/docs/SwapERC7984ToERC20.sol (2)
12-12: Usingeuint64as mapping key relies on handle uniqueness.The mapping
euint64 amount => addressassumes eachamountTransferredhandle is unique. This should be true since_fromToken.confidentialTransferFromreturns a new encrypted handle per call, but this is an implicit dependency on FHE implementation behavior.Confirm that
confidentialTransferFromalways returns a unique handle for the transferred amount, even if the cleartext values are identical across different calls.
30-42:finalizeSwapallows anyone to finalize - verify this is intentional.The function is public and can be called by anyone with a valid proof. This is acceptable if finalization is meant to be permissionless (e.g., relayer-friendly), but the original sender has no control over when finalization occurs.
contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol (3)
177-181: Overflow check timing may allow brief over-supply.
_checkConfidentialTotalSupply()is called in_updatebeforesuper._update, butinferredTotalSupply()reads the underlying balance which is already updated bywrap()before_mint()is called. This ordering is correct.
113-133:finalizeUnwrapimplementation is correct and secure.The function properly:
- Validates the unwrap request exists
- Deletes the request before external calls (CEI pattern)
- Verifies the decryption proof
- Transfers underlying tokens and emits event
161-163: Documentation clearly warns aboutinferredTotalSupplylimitations.The NatSpec correctly notes that the value can be inflated by direct transfers and lags compared to
confidentialTotalSupply. This is important context for consumers.contracts/interfaces/IERC7984Rwa.sol (2)
9-9: Interface inheritance correctly simplified.Removing
IERC165fromIERC7984Rwais correct sinceIERC7984now extendsIERC165. This avoids diamond inheritance issues and keeps the hierarchy clean.
10-62: Interface definition is comprehensive and well-documented.All public functions are properly declared with NatSpec comments. The interface correctly includes both overloads for functions that accept either
euint64orexternalEuint64with proof.
contracts/utils/FHESafeMath.sol
Outdated
| * NOTE: An uninitialized `euint64` value (equivalent to euint64.wrap(bytes32(0))) is evaluated as 0. | ||
| * This library will may return an uninitialized value if all inputs are uninitialized. |
There was a problem hiding this comment.
Fix grammatical error in documentation.
The phrase "will may return" is grammatically incorrect.
📝 Proposed fix
- * This library will may return an uninitialized value if all inputs are uninitialized.
+ * This library may return an uninitialized value if all inputs are uninitialized.📝 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.
| * NOTE: An uninitialized `euint64` value (equivalent to euint64.wrap(bytes32(0))) is evaluated as 0. | |
| * This library will may return an uninitialized value if all inputs are uninitialized. | |
| * NOTE: An uninitialized `euint64` value (equivalent to euint64.wrap(bytes32(0))) is evaluated as 0. | |
| * This library may return an uninitialized value if all inputs are uninitialized. |
🤖 Prompt for AI Agents
In @contracts/utils/FHESafeMath.sol around lines 11 - 12, Fix the grammar in the
FHESafeMath library comment: replace the phrase "will may return an
uninitialized value" with a correct wording such as "may return an uninitialized
value" (or "will return an uninitialized value" if you mean certainty) in the
top-of-file doc comment so the sentence reads correctly and conveys intended
meaning.
| let requester: HardhatEthersSigner | undefined; | ||
|
|
||
| beforeEach(async function () { | ||
| expectedAmount = undefined; | ||
| expectedHandle = undefined; | ||
| requester = undefined; |
There was a problem hiding this comment.
requester variable type allows undefined assignment but is later used as address.
The variable requester is typed as HardhatEthersSigner | undefined but is assigned this.holder.address (a string) on line 553. This type mismatch could cause issues.
🔧 Suggested fix
- let requester: HardhatEthersSigner | undefined;
+ let requester: string | undefined;📝 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.
| let requester: HardhatEthersSigner | undefined; | |
| beforeEach(async function () { | |
| expectedAmount = undefined; | |
| expectedHandle = undefined; | |
| requester = undefined; | |
| let requester: string | undefined; | |
| beforeEach(async function () { | |
| expectedAmount = undefined; | |
| expectedHandle = undefined; | |
| requester = undefined; |
🤖 Prompt for AI Agents
In @test/token/ERC7984/ERC7984.test.ts around lines 540 - 545, The requester
variable is typed as HardhatEthersSigner | undefined but is assigned
this.holder.address (a string) and later used as an address; change requester to
a string type (e.g., string | undefined) or consistently use a signer object
instead—update the declaration for requester and any places that set or pass it
(references: requester, this.holder.address, and any tests that use requester as
an address) so the types match and no undefined/signer-to-string mismatch
remains.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.