Merge release-v0.3 branch #260
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>
✅ Deploy Preview for confidential-tokens ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis pull request upgrades OpenZeppelin Confidential Contracts from v0.2.0 to v0.3.0, consolidating multiple feature changes: refactoring ERC7984 disclosure flows to use a request-based pattern, adding ERC165 interface support, introducing role-based access controls in ERC7984Rwa, updating FHESafeMath's uninitialized value handling, migrating from SepoliaConfig to ZamaEthereumConfig across mocks, and upgrading Changes
Sequence DiagramsequenceDiagram
actor User
participant Contract as ERC7984
participant FHE as FHE Lib
participant Caller
Note over User,Caller: New ERC7984 Disclosure Request-Finalize Pattern
User->>Contract: requestDiscloseEncryptedAmount(euint64)
activate Contract
Contract->>FHE: makePubliclyDecryptable(amount)
FHE-->>Contract: handle (publicly decryptable)
Contract->>Contract: emit AmountDiscloseRequested(amount, requester)
deactivate Contract
Note over User,Contract: Encrypted amount is now decryptable<br/>Requester is recorded in event
Note over User,Caller: External Decryption Phase
Caller->>Caller: Decrypt amount off-chain<br/>Generate proof
Caller->>Contract: discloseEncryptedAmount(amount, cleartext, proof)
activate Contract
Contract->>FHE: checkSignatures(handles, proof, cleartext)
FHE-->>Contract: validation result
Contract->>Contract: emit AmountDisclosed(amount, cleartext)
deactivate Contract
Note over Caller,Contract: Disclosure finalized with<br/>validated cleartext amount
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/token/ERC7984/extensions/ERC7984Rwa.sol (1)
242-247: Hardcoded function selectors are incorrect and will cause runtime failures.The hardcoded selectors
0x6c9c3c85and0x44fd6e40do not match either the signatures in the comments OR the actual function signatures in the code:
- Claimed (in comments):
forceConfidentialTransferFrom(address,address,bytes32)→0x95efd2e2(not0x6c9c3c85)- Claimed (in comments):
forceConfidentialTransferFrom(address,address,bytes32,bytes)→0xdee2729b(not0x44fd6e40)- Actual (in code):
forceConfidentialTransferFrom(address,address,externalEuint64,bytes)→0xad357416- Actual (in code):
forceConfidentialTransferFrom(address,address,euint64)→0xac49b3e4The actual function parameters are
euint64andexternalEuint64, notbytes32. The selector matching will fail at runtime, breaking the force transfer compliance bypass mechanism.Fix: Use computed selectors for both overloads:
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 == bytes4(keccak256("forceConfidentialTransferFrom(address,address,euint64)")) || + msg.sig == bytes4(keccak256("forceConfidentialTransferFrom(address,address,externalEuint64,bytes)")); }
🧹 Nitpick comments (7)
contracts/token/ERC7984/extensions/ERC7984Freezable.sol (1)
42-49: Internal_confidentialAvailableimplementation is correct; consider marking asviewif compatibleUsing
FHESafeMath.tryDecrease(confidentialBalanceOf(account), confidentialFrozen(account))and falling back to encrypted zero viaFHE.select(success, unfrozen, FHE.asEuint64(0))correctly models “available = max(balance − frozen, 0)” and aligns with the transfer-gating docstring.If the underlying FHE/FHESafeMath operations used here are
view/pure, consider declaring this helper asviewfor clarity and better mutability signaling:-function _confidentialAvailable(address account) internal virtual returns (euint64) { +function _confidentialAvailable(address account) internal view virtual returns (euint64) {Otherwise, current mutability is fine.
contracts/token/ERC7984/extensions/ERC7984Votes.sol (1)
2-2: ERC7984Votes voting integration looks correct; minor doc typo
- The header/path update to
v0.3.0is consistent with the rest of the release.confidentialTotalSupply,_update, and_getVotingUnitsfollow the expected Votes‑style pattern: supply and balances are updated insuper._update, and_transferVotingUnitsis driven by thetransferredamount, with voting units sourced fromconfidentialBalanceOf(account).Minor nit: in the docstring,
"Voing power"should be"Voting power".Also applies to: 9-15, 16-30
CHANGELOG.md (1)
4-25: 0.3.0 changelog entries align with code changesThe new 0.3.0 section correctly documents the ERC7984 renames/extensions and the
@fhevm/solidity0.9.1 migration, and the formatting tweaks in the 0.2.0 block are harmless. As an optional copyedit, you could change “Real World Assets (RWAs)” to “real‑world assets (RWAs)” for standard hyphenation, but this is non‑blocking.Also applies to: 31-31, 39-39, 45-51
scripts/update-docs-branch.js (1)
14-22: Release‑branch detection and docs‑branch naming look correctThe new
release-v<major>.<minor>[.<patch>]branch regex, the “not on a release branch” guard, and thedocs-v0.<minor>.x/docs-v<major>.xselection all match the intended release/docs branching scheme.You may want to parse
minorandpublishedMinoras numbers beforeif (minor < publishedMinor)to avoid future string‑comparison surprises once minor versions reach double digits (e.g.,"10" < "9"lexicographically). This is pre‑existing behavior but easy to harden:-const minor = current?.minor ?? pkgVersion.split('.')[1]; +const minor = Number(current?.minor ?? pkgVersion.split('.')[1]); ... -const publishedMinor = publishedVersion.match(/\d+\.(?<minor>\d+)\.\d+/).groups.minor; -if (minor < publishedMinor) { +const publishedMinor = Number(publishedVersion.match(/\d+\.(?<minor>\d+)\.\d+/).groups.minor); +if (minor < publishedMinor) {Also applies to: 26-35, 55-58
test/token/ERC7984/extensions/ERC7984Wrapper.test.ts (1)
273-277: Verify that passing0for the cleartext amount works correctly with ethers.The test passes
0as the second argument tofinalizeUnwrap. While this should work (ethers handles number-to-uint64 conversion), using0nwould be more explicit for BigInt consistency with other tests in this file.await expect( - this.wrapper.connect(this.holder).finalizeUnwrap(ethers.ZeroHash, 0, '0x'), + this.wrapper.connect(this.holder).finalizeUnwrap(ethers.ZeroHash, 0n, '0x'), ).to.be.revertedWithCustomError(this.wrapper, 'InvalidUnwrapRequest');contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol (1)
160-161: Consider usingrequireinstead ofassertfor the handle collision check.The
assertstatement will consume all remaining gas if it fails. Whileeuint64handles from FHE operations are expected to be unique, usingrequirewould provide a cleaner failure mode with a descriptive error:- assert(_unwrapRequests[burntAmount] == address(0)); + require(_unwrapRequests[burntAmount] == address(0), "Duplicate unwrap request"); _unwrapRequests[burntAmount] = to;That said, if handle uniqueness is a fundamental FHE invariant that should never be violated,
assertis semantically correct for indicating a broken invariant.contracts/token/ERC7984/ERC7984.sol (1)
60-61: Remove unused errorERC7984InvalidGatewayRequestto reduce bytecode size.Verification confirms this error is defined but completely unused throughout the codebase. With the disclosure flow refactored from request-based to amount-based, this error is dead code and can be safely removed.
📜 Review details
Configuration used: CodeRabbit 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(0 hunks).changeset/fast-ravens-lose.md(0 hunks).changeset/floppy-otters-dance.md(0 hunks).changeset/floppy-wasps-clap.md(0 hunks).changeset/fruity-bananas-smile.md(0 hunks).changeset/new-crews-boil.md(0 hunks).changeset/public-dolls-lose.md(0 hunks).changeset/seven-books-dig.md(0 hunks).changeset/six-maps-follow.md(0 hunks).changeset/sour-ties-warn.md(0 hunks).changeset/stupid-fans-bow.md(0 hunks).github/workflows/docs.yml(1 hunks)CHANGELOG.md(1 hunks)contracts/finance/ERC7821WithExecutor.sol(3 hunks)contracts/finance/VestingWalletCliffConfidential.sol(3 hunks)contracts/finance/VestingWalletConfidential.sol(3 hunks)contracts/finance/VestingWalletConfidentialFactory.sol(1 hunks)contracts/interfaces/IERC7984.sol(2 hunks)contracts/interfaces/IERC7984Receiver.sol(1 hunks)contracts/interfaces/IERC7984Rwa.sol(1 hunks)contracts/mocks/docs/SwapERC7984ToERC20.sol(2 hunks)contracts/mocks/finance/VestingWalletCliffConfidentialMock.sol(1 hunks)contracts/mocks/finance/VestingWalletConfidentialFactoryMock.sol(3 hunks)contracts/mocks/finance/VestingWalletConfidentialMock.sol(1 hunks)contracts/mocks/token/ERC7984ERC20WrapperMock.sol(1 hunks)contracts/mocks/token/ERC7984FreezableMock.sol(2 hunks)contracts/mocks/token/ERC7984Mock.sol(1 hunks)contracts/mocks/token/ERC7984ObserverAccessMock.sol(0 hunks)contracts/mocks/token/ERC7984ReceiverMock.sol(1 hunks)contracts/mocks/token/ERC7984RestrictedMock.sol(1 hunks)contracts/mocks/token/ERC7984RwaMock.sol(2 hunks)contracts/mocks/utils/FHESafeMathMock.sol(1 hunks)contracts/mocks/utils/HandleAccessManagerMock.sol(1 hunks)contracts/mocks/utils/HandleAccessManagerUserMock.sol(1 hunks)contracts/package.json(2 hunks)contracts/token/ERC7984/ERC7984.sol(4 hunks)contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol(5 hunks)contracts/token/ERC7984/extensions/ERC7984Freezable.sol(3 hunks)contracts/token/ERC7984/extensions/ERC7984ObserverAccess.sol(2 hunks)contracts/token/ERC7984/extensions/ERC7984Omnibus.sol(2 hunks)contracts/token/ERC7984/extensions/ERC7984Restricted.sol(3 hunks)contracts/token/ERC7984/extensions/ERC7984Rwa.sol(11 hunks)contracts/token/ERC7984/extensions/ERC7984Votes.sol(1 hunks)contracts/token/ERC7984/utils/ERC7984Utils.sol(2 hunks)contracts/utils/FHESafeMath.sol(2 hunks)contracts/utils/structs/temporary-Checkpoints.sol(1 hunks)docs/antora.yml(1 hunks)package.json(3 hunks)scripts/update-docs-branch.js(1 hunks)test/helpers/accounts.ts(1 hunks)test/token/ERC7984/ERC7984.test.ts(4 hunks)test/token/ERC7984/extensions/ERC7984Rwa.test.ts(1 hunks)test/token/ERC7984/extensions/ERC7984Wrapper.test.ts(7 hunks)test/utils/FHESafeMath.test.ts(1 hunks)
💤 Files with no reviewable changes (12)
- .changeset/fruity-bananas-smile.md
- .changeset/new-crews-boil.md
- .changeset/sour-ties-warn.md
- .changeset/six-maps-follow.md
- .changeset/beige-fans-call.md
- .changeset/fast-ravens-lose.md
- .changeset/seven-books-dig.md
- .changeset/stupid-fans-bow.md
- .changeset/public-dolls-lose.md
- contracts/mocks/token/ERC7984ObserverAccessMock.sol
- .changeset/floppy-wasps-clap.md
- .changeset/floppy-otters-dance.md
🧰 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/mocks/token/ERC7984ReceiverMock.solcontracts/token/ERC7984/utils/ERC7984Utils.solcontracts/token/ERC7984/extensions/ERC7984Omnibus.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.solcontracts/mocks/token/ERC7984FreezableMock.solcontracts/mocks/docs/SwapERC7984ToERC20.soltest/token/ERC7984/ERC7984.test.tscontracts/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: 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/token/ERC7984/extensions/ERC7984Rwa.sol
🧬 Code graph analysis (2)
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] ~16-~16: 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] ~19-~19: 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). (2)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: Publish to npm
🔇 Additional comments (57)
test/helpers/accounts.ts (1)
21-21: Clean migration from @fhevm/core-contracts to @fhevm/host-contracts verified; no stale references found.The artifact path migration at line 21 is complete with no remaining references to the old
@fhevm/core-contractspackage. The new@fhevm/host-contractspath is the only reference to this artifact in the codebase, confirming a successful and consistent upgrade transition.contracts/utils/FHESafeMath.sol (1)
34-43: LGTM!The updated logic correctly handles the case where
oldValueis uninitialized anddeltais initialized. Returning(FHE.eq(delta, 0), FHE.asEuint64(0))is semantically correct since:
0 - deltaonly succeeds whendelta == 0- The result is always
0(either0-0=0on success, or the original value0on failure)This change ensures an explicitly initialized zero is returned rather than an uninitialized value, which aligns with the updated documentation.
test/utils/FHESafeMath.test.ts (1)
61-69: LGTM!The test data updates correctly reflect the behavior change in
tryDecrease:
[undefined, 0, 0, true]: When decreasing uninitialized value by 0, result is now explicitly 0[undefined, 1, 0, false]: When decreasing uninitialized value by 1, result is 0 with failureThese align with the contract change that returns
FHE.asEuint64(0)instead of an uninitialized value when delta is initialized.contracts/token/ERC7984/extensions/ERC7984Omnibus.sol (2)
2-2: Header version bump is consistent with v0.3.0 upgradeThe updated “last updated v0.3.0” header matches the PR’s version upgrade and pragma; nothing else to adjust here.
192-208: Call reordering in_confidentialTransferFromAndCallOmnibuspreserves ACL and callback behaviorPlacing
confidentialTransferFromAndCall(omnibusFrom, omnibusTo, amount, data)between the ACL grants forsender/recipientand the ACL grants fortransferredkeeps the effects–interaction–post‑effects structure clear and aligned with_confidentialTransferFromOmnibus. The callback still only receivesomnibusFrom,omnibusTo,amount, anddata, and the ACL grants on encryptedsender/recipientremain for omnibus event emission and future access, not for direct callback parameters, so this reordering does not introduce new exposure or reentrancy surface beyond the underlying ERC7984 call. Based on learnings, this maintains the intended omnibus callback privacy model.Also applies to: 200-201
contracts/token/ERC7984/extensions/ERC7984Freezable.sol (4)
2-2: Header version annotation looks correctThe header now reflects
last updated v0.3.0, which matches the release being merged; no issues here.
34-40:confidentialAvailabledelegation and ACL behavior look soundDelegating to
_confidentialAvailablefor the computation and then granting ACLs viaFHE.allowThis(amount)andFHE.allow(amount, account)cleanly separates calculation from permissioning. The docstring correctly reflects that the allowance is given toaccount, and using the internal helper avoids leaking extra ACLs when the value is used for internal gating.
65-66: Doc update matches the new gating patternThe comment now explicitly calling out
_confidentialAvailableas the hook for customizing freezing behavior, and noting that it is used for actual gating to avoid extra ACL grants, is consistent with the implementation and helps future overriders.
70-71: Using_confidentialAvailablein_updateachieves gating without extra ACLsSwitching
_updateto use_confidentialAvailable(from)ensures the transfer is still limited to the unfrozen portion (encryptedAmount <= unfrozen ? encryptedAmount : 0) while avoiding the ACL side effects of the publicconfidentialAvailable. This matches the documented “sufficient unfrozen balance or 0 transfer” behavior.docs/antora.yml (1)
3-3: Docs component version aligned with releaseBumping the Antora component version to
'0.3'is consistent with the 0.3.0 release and keeps docs versioning in sync..github/workflows/docs.yml (1)
5-5: Docs workflow now only runs onrelease-v*branchesChanging
on.push.branchestorelease-v*means docs will no longer build on pushes tomaster, only on branches likerelease-v0.3. Please confirm this matches the intended release/docs flow and actual branch naming (e.g.,release-v0.3vsrelease/v0.3).contracts/package.json (1)
4-4: Contracts package version and FHEVM peer dependency look consistentThe contracts package version bump to
0.3.0and the@fhevm/soliditypeer dependency pin to0.9.1match the root package updates and the 0.3.0 release scope.Please double‑check that all tooling and contracts using
euint/FHE types are validated against@fhevm/solidity@0.9.1(compile, test, and gas workflows), since the peer dependency is pinned to this exact version.Also applies to: 35-35
package.json (1)
4-4: Release and FHE/Zama tooling versions are alignedThe root package version bump to
0.3.0and the devDependency updates for@fhevm/hardhat-plugin,@fhevm/solidity, and@zama-fhe/relayer-sdkare consistent with the contracts package and the 0.3.0 release.Please verify that:
- The updated Hardhat plugin and relayer SDK versions are compatible with each other and with Node
>=20.19.0.- CI (compile, tests, coverage, docs) passes under this new toolchain matrix.
Also applies to: 48-49, 66-66
contracts/finance/VestingWalletConfidentialFactory.sol (1)
2-2: VestingWalletConfidentialFactory header updated for v0.3.0Header metadata now correctly reflects
v0.3.0and the file path; no behavioral changes in the factory logic.contracts/utils/structs/temporary-Checkpoints.sol (1)
2-2: Temporary Checkpoints header now matches v0.3.0The version header for this procedurally generated temporary file is updated to
v0.3.0; implementation remains unchanged.contracts/finance/ERC7821WithExecutor.sol (1)
2-2: ERC7821WithExecutor storage slot rename is consistent
- Header updated to
v0.3.0matches the release.- Renaming the storage location constant to
ERC7821_WITH_EXECUTOR_STORAGE_LOCATIONand wiring it into_getERC7821WithExecutorStoragevia$.slot := ERC7821_WITH_EXECUTOR_STORAGE_LOCATIONkeeps the erc7201 storage layout pattern intact, assuming the bytes32 literal was not altered.Given this contract is upgradeable, it’s worth double‑checking that the slot value exactly matches the one used in previous releases to avoid storage layout breaks.
Also applies to: 18-19, 40-43
contracts/interfaces/IERC7984Receiver.sol (1)
2-2: Header version bump only; interface remains stableThe v0.3.0 header update here is purely informational; the IERC7984Receiver interface and callback signature are unchanged and consistent with existing usage.
contracts/finance/VestingWalletConfidential.sol (1)
2-2: Storage-slot constant rename is layout‑preservingRenaming the storage slot constant and wiring it through
_getVestingWalletStoragekeeps the keccak‑derived slot and storage layout intact while aligning with the new naming convention. No behavioral impact beyond the v0.3.0 header update.Also applies to: 37-38, 136-139
contracts/token/ERC7984/extensions/ERC7984Restricted.sol (1)
2-2: Restriction docs now match the custom error semanticsThe v0.3.0 header plus the
_update/_checkRestrictioncomments now correctly referenceUserRestrictedand clarify the default behavior; implementation remains unchanged and consistent.Also applies to: 54-55, 86-89
contracts/finance/VestingWalletCliffConfidential.sol (1)
2-2: Cliff wallet storage-slot rename preserves layoutThe uppercase storage location constant and matching use in
_getVestingWalletCliffStoragekeep the storage slot and vesting behavior intact while standardizing naming for v0.3.0.Also applies to: 19-20, 67-70
contracts/mocks/token/ERC7984FreezableMock.sol (1)
5-6: Config migration and internal helper usage are consistentSwitching to
ZamaEthereumConfigmatches the broader config migration, and routingconfidentialAvailableAccessthrough_confidentialAvailablekeeps the “available = balance − frozen” semantics while cleanly exposing the handle for tests viagetHandleAllowance.Also applies to: 34-38
contracts/token/ERC7984/extensions/ERC7984ObserverAccess.sol (1)
2-2: Observer mapping key naming is clearer with no behavior changeThe v0.3.0 header plus the
mapping(address account => address)declaration improve clarity around the key without changing how observers are stored or read.Also applies to: 14-14
contracts/mocks/finance/VestingWalletConfidentialMock.sol (1)
4-7: Configuration migration looks correct.The change from
SepoliaConfigtoZamaEthereumConfigaligns with the broader migration across mock contracts in this release. The inheritance chain remains properly structured.contracts/interfaces/IERC7984.sol (2)
36-37: Documentation formatting improvement.The ERC-7572 reference link format is now consistent with standard EIP documentation style.
6-9: ERC165 interface support is properly implemented and correctly integrated.The verification confirms that
ERC7984correctly extends bothIERC7984andERC165, and thesupportsInterfacemethod (lines 70-72 incontracts/token/ERC7984/ERC7984.sol) properly reports support for theIERC7984interface ID:function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC165) returns (bool) { return interfaceId == type(IERC7984).interfaceId || super.supportsInterface(interfaceId); }The implementation correctly handles the inheritance chain and delegates to parent implementations for other interface checks. This is a well-structured addition that follows ERC165 best practices.
contracts/token/ERC7984/utils/ERC7984Utils.sol (1)
19-20: Documentation clarification accurately reflects FHE transfer semantics.The change from "reversing the transfer" to "refund the
fromaddress" is semantically correct for FHE contexts where encrypted operations cannot be truly reversed—the caller must handle rejection by transferring tokens back.contracts/mocks/utils/HandleAccessManagerUserMock.sol (1)
4-8: Configuration migration consistent with other mocks.The switch to
ZamaEthereumConfigaligns with the broader mock migration in this release. No functional changes to the test utility.contracts/mocks/token/ERC7984RestrictedMock.sol (1)
5-9: Configuration migration applied correctly.The inheritance update to
ZamaEthereumConfigis consistent with the broader mock migration pattern across the codebase. The mock functionality remains unchanged.contracts/mocks/token/ERC7984Mock.sol (1)
5-10: ZamaEthereumConfig migration in ERC7984Mock looks consistentUpdating the import and base class to
ZamaEthereumConfigcleanly aligns this mock with the new FHE config without touching the token/FHE logic. No issues from the inheritance ordering (ERC7984, ZamaEthereumConfig) as the config contract is stateless.contracts/mocks/utils/FHESafeMathMock.sol (1)
4-8: FHESafeMathMock config base swap is safeSwitching
FHESafeMathMockto inheritZamaEthereumConfig(and updating the import) is a mechanical change; the FHESafeMath wrapper logic and emitted events are untouched. This should preserve behavior while aligning with the new config.contracts/mocks/utils/HandleAccessManagerMock.sol (1)
4-8: HandleAccessManagerMock now using ZamaEthereumConfigThe change to import and inherit
ZamaEthereumConfigis consistent with the broader migration and does not affect the mock’s logic (_validateHandleAllowance remains a no‑op as intended).contracts/interfaces/IERC7984Rwa.sol (1)
2-9: IERC7984Rwa inheritance simplification keeps ERC165 via IERC7984Updating
IERC7984Rwato extend onlyIERC7984(withIERC165now provided byIERC7984) simplifies the hierarchy without changing the effective interface surface or ERC165 support. The added header comment correctly documents the v0.3.0 update.contracts/mocks/token/ERC7984RwaMock.sol (1)
6-22: supportsInterface override correctly resolves the multiple‑inheritance diamondImporting
ERC7984and overridingsupportsInterfaceasfunction supportsInterface(bytes4 interfaceId) public view virtual override(ERC7984Rwa, ERC7984) returns (bool) { return super.supportsInterface(interfaceId); }is the right pattern here: it explicitly resolves the two inheritance branches that define
supportsInterfaceand preserves the base contracts’ interface reporting viasuper. This keeps ERC165/IERC7984/IERC7984Rwa support coherent on the mock.contracts/mocks/token/ERC7984ReceiverMock.sol (1)
4-8: ERC7984ReceiverMock now aligned with ZamaEthereumConfigThe switch to importing and inheriting
ZamaEthereumConfigis consistent with the rest of the mocks and does not alter the callback logic ofonConfidentialTransferReceived.contracts/mocks/finance/VestingWalletCliffConfidentialMock.sol (1)
4-7: VestingWalletCliffConfidentialMock config update is purely structuralHooking
ZamaEthereumConfiginto this abstract mock (via import and inheritance) is a straightforward config migration; no vesting or FHE logic is modified.contracts/mocks/token/ERC7984ERC20WrapperMock.sol (1)
4-8: ERC7984ERC20WrapperMock config migration preserves wrapper behaviorThe import and inheritance change to
ZamaEthereumConfigis consistent with the rest of the suite; the ERC20 wrapper and underlyingERC7984constructor wiring remain unchanged, so runtime behavior should be identical aside from using the new config.test/token/ERC7984/extensions/ERC7984Rwa.test.ts (1)
25-34: LGTM! Interface support validation correctly restructured.The separation of
erc7984RwaFunctionsanderc7984Functionsproperly reflects the updated inheritance hierarchy whereIERC7984RwaextendsIERC7984directly. The test now correctly validates all three interface IDs: ERC7984Rwa (combined with ERC7984), ERC7984 (combined with ERC165), and ERC165 standalone.contracts/mocks/finance/VestingWalletConfidentialFactoryMock.sol (1)
4-4: LGTM! Config migration from SepoliaConfig to ZamaEthereumConfig is consistent.The mock contract correctly migrates to the new
ZamaEthereumConfigand usesZamaConfig.getEthereumCoprocessorConfig()for FHE coprocessor initialization. This aligns with the broader configuration changes across the codebase.Also applies to: 10-10, 49-53, 68-68
test/token/ERC7984/extensions/ERC7984Wrapper.test.ts (2)
323-329: LGTM! Helper function cleanly encapsulates the unwrap finalization flow.The
publicDecryptAndFinalizeUnwraphelper correctly:
- Queries the
UnwrapRequestedevent to get the recipient and amount handle- Performs public decryption via
fhevm.publicDecrypt- Calls
finalizeUnwrapwith the decryption results- Asserts the
UnwrapFinalizedevent emission with expected argumentsThis reduces test duplication and improves maintainability.
242-271: Good addition of edge case tests for unwrap finalization.The "finalized with invalid signature" test properly validates that a truncated decryption proof is rejected. The truncation approach (
slice(0, length - 2)) is a reasonable way to corrupt the signature.contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol (1)
130-150: LGTM! The new finalization flow correctly validates decryption proofs.The
finalizeUnwrapfunction properly:
- Retrieves and validates the recipient from
_unwrapRequests- Deletes the request before external calls (CEI pattern)
- Constructs the handles array and encodes cleartexts for signature verification
- Calls
FHE.checkSignaturesto validate the decryption proof- Transfers the underlying tokens using
SafeERC20.safeTransferThis is a clean implementation of the request-based unwrap pattern.
test/token/ERC7984/ERC7984.test.ts (2)
54-67: LGTM! ERC165 interface support tests properly validate interface IDs.The tests correctly:
- Compute interface IDs by combining function selectors from
IERC7984__factoryandIERC165__factory- Verify
supportsInterfacereturns true for valid interfaces- Verify
supportsInterfacereturns false for an invalid selector (0xbadbadba)This follows the same pattern established in the ERC7984Rwa tests.
596-619: LGTM! The disclosure flow test correctly validates the request-then-finalize pattern.The
afterEachhook properly:
- Queries the
AmountDiscloseRequestedevent- Validates the expected handle and requester against the event arguments
- Performs public decryption via
fhevm.publicDecrypt- Calls
discloseEncryptedAmountwith the cleartext values and proof- Asserts the
AmountDisclosedevent emissionThis aligns with the new disclosure API changes in the contract.
contracts/token/ERC7984/ERC7984.sol (4)
2-2: LGTM! ERC165 integration properly implemented.The version bump to v0.3.0, ERC165 import, and updated inheritance are correctly structured. The contract now properly extends both
IERC7984andERC165for interface introspection support.Also applies to: 6-7, 27-27
35-36: LGTM! Event declaration is appropriate.The
AmountDiscloseRequestedevent with indexed parameters allows efficient filtering by encrypted amount and requester address.
69-72: LGTM! Standard ERC165 implementation.The
supportsInterfacefunction correctly advertisesIERC7984support and delegates to the parent for other interface checks.
218-235: LGTM! Disclosure finalization is correctly implemented.The function properly validates the decryption proof using
FHE.checkSignaturesbefore emitting the disclosure event. The permissionless design is appropriate since the cryptographic proof validates the claim.contracts/mocks/docs/SwapERC7984ToERC20.sol (3)
30-42: LGTM! Finalization logic is correctly implemented.The function follows checks-effects-interactions pattern: validates the proof, checks receiver existence, deletes the mapping entry, then performs the external transfer. Using
SafeERC20.safeTransferis appropriate for safe ERC20 interactions.
9-19: LGTM! Contract structure and constructor are appropriate.The mock contract correctly initializes both token references and uses clear naming conventions aligned with the ERC7984 standard.
12-12: I'll help you verify the review comment about the potential key collision risk witheuint64as a mapping key. Let me start by examining the file and understanding the context.
<function_calls>
First, let's read the file to understand the context
cat -n contracts/mocks/docs/SwapERC7984ToERC20.sol
</function_calls>Now let me research euint64 and fhEVM handle uniqueness to verify the core claim:
<function_calls>
fhEVM euint64 handle uniqueness ciphertext collision
</function_calls>contracts/token/ERC7984/extensions/ERC7984Rwa.sol (7)
2-2: LGTM! Version and inheritance structure updated correctly.The contract properly inherits from the required base contracts and the version is updated to v0.3.0.
Also applies to: 22-22
51-56: LGTM! Interface support correctly extends parent chain.The
supportsInterfaceproperly advertisesIERC7984Rwaand delegates to the parent for inherited interface detection includingIERC7984andIERC165.
117-136: LGTM! Mint functions correctly return amount with ACL grant.The
confidentialMintfunctions now return the minted amount and grant the calling agent access viaFHE.allow. This enables agents to track and use the minted amounts for subsequent operations.
138-158: LGTM! Burn functions correctly return amount with ACL grant.Consistent with the mint pattern,
confidentialBurnfunctions return the burnt amount and grant the calling agent access.
160-185: LGTM! Force transfer functions properly documented and implemented.The
forceConfidentialTransferFromvariants correctly bypass sender restrictions while maintaining receiver restrictions, and the agent access pattern is consistent with mint/burn.
93-96: Semantic change confirmed:_resetUsersets restriction to DEFAULT, not ALLOWED.The change from
_allowUser(account)to_resetUser(account)is a material behavioral difference:
_allowUsersets the user's restriction state toRestriction.ALLOWED(value 2)_resetUsersets the user's restriction state toRestriction.DEFAULT(value 0)While tests confirm both states allow transfers, they are distinct restriction levels. This asymmetry means
blockUser→ BLOCKED, butunblockUser→ DEFAULT (not back to ALLOWED), which could break integrations expecting an ALLOWED state after unblocking.
223-230: The review comment is incorrect regarding the pause check bypass.The comment states that
_forceUpdatebypasses the paused state by directly callingsuper._update. However, this is factually incorrect.When
_forceUpdate(defined inERC7984Rwa) callssuper._updateat line 227, the method resolution order resolves toERC7984Rwa._updateat line 214, which includes thewhenNotPausedmodifier. Therefore, the pause check is not bypassed—the function will still revert if the contract is paused.The comment at line 226 is misleading and should be removed or corrected to accurately reflect that the pause check is still enforced through the inherited
_updatemethod.Likely an incorrect or invalid review comment.
| /** | ||
| * @dev Discloses an encrypted amount `encryptedAmount` publicly via an {IERC7984-AmountDisclosed} | ||
| * event. The caller and this contract must be authorized to use the encrypted amount on the ACL. | ||
| * @dev Starts the process to disclose an encrypted amount `encryptedAmount` publicly by making it | ||
| * publicly decryptable. Emits the {AmountDiscloseRequested} event. | ||
| * | ||
| * NOTE: This is an asynchronous operation where the actual decryption happens off-chain and | ||
| * {finalizeDiscloseEncryptedAmount} is called with the result. | ||
| * NOTE: Both `msg.sender` and `address(this)` must have permission to access the encrypted amount | ||
| * `encryptedAmount` to request disclosure of the encrypted amount `encryptedAmount`. | ||
| */ | ||
| function discloseEncryptedAmount(euint64 encryptedAmount) public virtual { | ||
| function requestDiscloseEncryptedAmount(euint64 encryptedAmount) public virtual { | ||
| require( | ||
| FHE.isAllowed(encryptedAmount, msg.sender), | ||
| ERC7984UnauthorizedUseOfEncryptedAmount(encryptedAmount, msg.sender) | ||
| ); | ||
|
|
||
| bytes32[] memory cts = new bytes32[](1); | ||
| cts[0] = euint64.unwrap(encryptedAmount); | ||
| FHE.requestDecryption(cts, this.finalizeDiscloseEncryptedAmount.selector); | ||
| FHE.makePubliclyDecryptable(encryptedAmount); | ||
| emit AmountDiscloseRequested(encryptedAmount, msg.sender); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find FHE-related files in the repository
find . -type f -name "*.sol" | head -20Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 1066
🏁 Script executed:
#!/bin/bash
# Search for FHE imports and the makePubliclyDecryptable function
rg -n "makePubliclyDecryptable" --type solidityRepository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Search for FHE interface or library definitions
rg -n "interface FHE\|library FHE" --type solidity -A 5Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Look for FHE-related documentation or README files
find . -type f \( -name "*.md" -o -name "*.txt" \) | xargs grep -l "FHE\|fhEVM" 2>/dev/null | head -5Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 118
🌐 Web query:
fhEVM FHE.makePubliclyDecryptable ACL permission requirement
💡 Result:
Short answer: the caller must already have ACL permission on the ciphertext (i.e., be an allowed address/owner) to call FHE.makePubliclyDecryptable (which flips the ACL to allow anyone to decrypt). See the fhEVM/FHE docs: only an address with isAllowed on that ciphertext may grant allowances such as makePubliclyDecryptable / allowGlobal. [1][2]
Sources
- FHE/ACL examples (shows FHE.makePubliclyDecryptable / make public decryption and describes allow/isAllowed behavior). [1]
- ACL overview / implementation notes (newly created ciphertexts are only accessible to creator for the transaction; additional allowances require calling allow/allowTransient/allowGlobal and checks of isAllowed). [2]
🏁 Script executed:
#!/bin/bash
# Search for other usages of FHE.makePubliclyDecryptable
rg -n "makePubliclyDecryptable" -A 3 -B 3Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 1908
🏁 Script executed:
#!/bin/bash
# Search for FHE.isAllowed to see how it's typically used
rg -n "FHE\.isAllowed" -A 2 -B 2Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 6869
Missing ACL permission check for the contract itself before calling FHE.makePubliclyDecryptable.
The NatSpec comment correctly states that both msg.sender and address(this) must have permission to access the encrypted amount. However, the implementation only verifies FHE.isAllowed(encryptedAmount, msg.sender). According to fhEVM documentation, only an address with ACL permission on the ciphertext may call FHE.makePubliclyDecryptable. Since this function is invoked from within the contract, address(this) is the actual caller and must have permission—a requirement not enforced by the current code.
To match the docstring, add a check for the contract's ACL permission:
require(
FHE.isAllowed(encryptedAmount, msg.sender) && FHE.isAllowed(encryptedAmount, address(this)),
ERC7984UnauthorizedUseOfEncryptedAmount(encryptedAmount, msg.sender)
);🤖 Prompt for AI Agents
In contracts/token/ERC7984/ERC7984.sol around lines 201 to 216, the function
requestDiscloseEncryptedAmount only checks ACL for msg.sender but not for the
contract itself (address(this) is the effective caller of
FHE.makePubliclyDecryptable); update the require to also verify
FHE.isAllowed(encryptedAmount, address(this)) before calling
FHE.makePubliclyDecryptable so both msg.sender and the contract have permission,
keeping the same revert type/signature.
| * to handle potential overflows in FHE operations. | ||
| * | ||
| * 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.
Typo: "will may" should be "may".
- * 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.
| * 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. |
🤖 Prompt for AI Agents
In contracts/utils/FHESafeMath.sol around line 12, the comment contains a typo
"will may" — update the sentence to read "may return an uninitialized value if
all inputs are uninitialized." to remove the extra word and keep grammar
correct.
| let requester: HardhatEthersSigner | undefined; | ||
|
|
||
| beforeEach(async function () { | ||
| expectedAmount = undefined; | ||
| expectedHandle = undefined; | ||
| requester = undefined; |
There was a problem hiding this comment.
Type mismatch: requester declared as HardhatEthersSigner but assigned string addresses.
The variable is declared as HardhatEthersSigner | undefined but is assigned this.holder.address and this.recipient.address which are strings. This works at runtime because the comparison on line 604 only needs the string value, but the type annotation is incorrect:
- let requester: HardhatEthersSigner | undefined;
+ let requester: string | undefined;Also applies to: 553-554, 575-576
🤖 Prompt for AI Agents
In test/token/ERC7984/ERC7984.test.ts around lines 540-545 (and also at the
related assignments around 553-554 and 575-576), the variable requester is typed
as HardhatEthersSigner | undefined but you assign string addresses
(this.holder.address / this.recipient.address); update the type to string |
undefined if you intend to store addresses, or instead assign actual signer
objects and keep HardhatEthersSigner | undefined — make the declaration and
assignments consistent (prefer changing the type to string | undefined if the
tests only compare addresses).
Summary by CodeRabbit
Release Notes
New Features
Dependencies
Chores
✏️ Tip: You can customize this high-level summary in your review settings.