Conversation
* remove properties symlink * add properties.js * lint fix
* Remove backwards compatibility from Batcher * Apply suggestion from @arr00
* remove incorrect docs on `unwrap` function * add finalize unwrap and rate to wrapper interface * update batcher to use unwrap request ids instead of unwrap amounts * add `unwrapAmount` to wrapper interface for unwrap request introspection * additional docs * call getter for `unwrapAmount` in `finalizeUnwrap` * update events and errors appropriately * remove `ERC7984ERC20Wrapper` import * fix import order * Move events to interface from `ERC7984ERC20Wrapper` * add note on request id for `unwrap` * change mapping to be of `bytes32` identifiers * add changeset
* Allow claiming on behalf of another account in batcher * revert on claim if users not part of the batch * add error on quit too
…fidential` (#326) * Return a bytes32 unwrap id from the `_unwrap` function in `BatcherConfidential` * modify warning
* Document possible rounding down of user deposits * add contract level comment as well * up
* Release v0.4.0 (rc) * update changelog * update changelog * Apply suggestion from @james-toussaint Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> --------- 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>
* Release v0.4.0 * update changelog * remove changelog duplicate --------- 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. |
Contributor
WalkthroughThis PR releases version 0.4.0, removing published changeset entries and consolidating release notes in CHANGELOG.md. It introduces significant API changes: the unwrap mechanism transitions from euint64 to bytes32 request IDs, ERC7984ERC20Wrapper gains new public methods, and BatcherConfidential refactors to use wrapper interfaces with updated claim semantics. Multiple contract headers are bumped to v0.4.0, and a new documentation template helper module is added. Node.js CI runtime updated from 20.x to 24.x. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Batcher as BatcherConfidential
participant Wrapper as ERC7984ERC20Wrapper
participant Relay as Relayer/Decryptor
User->>Batcher: claim(batchId, account)
Batcher->>Batcher: Validate batch dispatched & finalized
Batcher->>Wrapper: Transfer toToken to account
Wrapper-->>Batcher: Success
Batcher->>Batcher: Clear account deposit
Batcher-->>User: Claimed event
Relay->>Wrapper: unwrap(from, to, euint64)
Wrapper->>Wrapper: Store encrypted amount under requestId
Wrapper-->>Relay: Return bytes32 unwrapRequestId
Relay->>Wrapper: finalizeUnwrap(unwrapRequestId, cleartext, proof)
Wrapper->>Wrapper: Verify signature & emit UnwrapFinalized
Wrapper-->>Relay: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/finance/BatcherConfidential.test.ts (1)
652-654: Resolve the encrypted amount throughunwrapAmount(requestId)in these assertions.These checks still decrypt
unwrapRequestIddirectly.BatcherConfidentialitself goes throughfromToken().unwrapAmount(requestId), so the test is still coupled to the current wrapper detail thatrequestId == encryptedAmount. Mirroring the contract path here keeps the test valid for other compliantIERC7984ERC20Wrapperimplementations.♻️ Suggested change
- const { abiEncodedClearValues, decryptionProof } = await batcher - .unwrapRequestId(batchId1) - .then(amount => fhevm.publicDecrypt([amount])); + const { abiEncodedClearValues, decryptionProof } = await batcher + .unwrapRequestId(batchId1) + .then(requestId => this.fromToken.unwrapAmount(requestId)) + .then(amount => fhevm.publicDecrypt([amount])); ... batcher .unwrapRequestId(batchId2) - .then(amount => fhevm.publicDecrypt([amount])) + .then(requestId => this.fromToken.unwrapAmount(requestId)) + .then(amount => fhevm.publicDecrypt([amount])) .then(({ abiEncodedClearValues }) => abiEncodedClearValues),Also applies to: 709-713
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/finance/BatcherConfidential.test.ts` around lines 652 - 654, The test is decrypting the raw value from batcher.unwrapRequestId(batchId1) instead of resolving it via the wrapper's unwrapAmount path; change the assertions to call the wrapper path (use batcher.fromToken().unwrapAmount(requestId) or batcher.unwrapAmount(requestId) as appropriate) and then pass that result into fhevm.publicDecrypt to produce decryptionProof/abiEncodedClearValues; update both occurrences (the block using batchId1 and the similar block at 709-713) to use unwrapAmount(requestId) rather than directly decrypting unwrapRequestId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/finance/BatcherConfidential.sol`:
- Around line 208-216: The catch block after calling
IERC7984ERC20Wrapper(fromToken()).finalizeUnwrap(unwrapRequestId_,
unwrapAmountCleartext, decryptionProof) is too broad; only treat reverts that
indicate the unwrap was already finalized as the fallback path that validates
using FHE.checkSignatures and continues (handles,
euint64.unwrap(fromToken().unwrapAmount(unwrapRequestId_)),
unwrapAmountCleartext, decryptionProof). Modify the try/catch to inspect the
revert (use catch Error(string memory reason) or catch (bytes memory lowLevel)
to decode) and if the reason matches the wrapper’s “already finalized” sentinel
only then run the existing fallback signature checks and proceed, otherwise
re-throw / revert so real unwrap failures bubble up instead of falling through
to _executeRoute. Ensure you keep the same symbols: unwrapRequestId_,
finalizeUnwrap, fromToken(), unwrapAmountCleartext, decryptionProof, handles,
euint64.unwrap, fromToken().unwrapAmount, and FHE.checkSignatures.
- Around line 355-362: The code in _claim zeroes
_batches[batchId].deposits[account] whenever amountTransferred != 0, but
amountTransferred is in toToken() units while deposit is stored in fromToken()
units, so partial transfers will permanently drop the untransferred remainder;
change the logic to only zero the deposit when the full requested amount was
transferred (compare amountTransferred to amountToSend in toToken units) and
otherwise subtract the actual transferred amount converted back to fromToken
units from deposit (i.e., compute remaining = deposit -
convertToFromUnits(amountTransferred) or use an existing conversion routine),
then FHE.allow/FHE.allowThis on the computed remaining and store that back into
_batches[batchId].deposits[account]; keep references to confidentialTransfer,
amountToSend, amountTransferred, deposit, toToken(),
_batches[batchId].deposits[account], and quit/_claim semantics when
implementing.
In `@contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol`:
- Around line 183-187: The NatSpec comment for function unwrapRequester
incorrectly references `unwrapAmount`; update the doc block to refer to the
current lookup key `unwrapRequestId` and clarify that it returns the address
with a pending unwrap request for that `unwrapRequestId` (or address(0) if
none). Edit the comment above the `unwrapRequester(bytes32 unwrapRequestId)`
function and mention `unwrapRequestId` and `_unwrapRequests[unwrapRequestId]` so
generated docs reflect the current API.
---
Nitpick comments:
In `@test/finance/BatcherConfidential.test.ts`:
- Around line 652-654: The test is decrypting the raw value from
batcher.unwrapRequestId(batchId1) instead of resolving it via the wrapper's
unwrapAmount path; change the assertions to call the wrapper path (use
batcher.fromToken().unwrapAmount(requestId) or batcher.unwrapAmount(requestId)
as appropriate) and then pass that result into fhevm.publicDecrypt to produce
decryptionProof/abiEncodedClearValues; update both occurrences (the block using
batchId1 and the similar block at 709-713) to use unwrapAmount(requestId) rather
than directly decrypting unwrapRequestId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 448d2ba7-cb48-4aca-8341-5d79e548e40d
📒 Files selected for processing (35)
.changeset/fast-humans-run.md.changeset/full-worlds-rescue.md.changeset/good-flies-win.md.changeset/happy-hands-find.md.changeset/long-items-appear.md.changeset/modern-snakes-return.md.changeset/nasty-walls-taste.md.changeset/pink-areas-double.md.changeset/shaky-pugs-return.md.changeset/sour-dodos-sniff.md.github/actions/setup/action.ymlCHANGELOG.mdcontracts/finance/BatcherConfidential.solcontracts/finance/VestingWalletConfidential.solcontracts/governance/utils/VotesConfidential.solcontracts/interfaces/IERC7984ERC20Wrapper.solcontracts/interfaces/IERC7984Receiver.solcontracts/interfaces/IERC7984Rwa.solcontracts/package.jsoncontracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.solcontracts/token/ERC7984/extensions/ERC7984Freezable.solcontracts/token/ERC7984/extensions/ERC7984ObserverAccess.solcontracts/token/ERC7984/extensions/ERC7984Restricted.solcontracts/token/ERC7984/extensions/ERC7984Rwa.solcontracts/token/ERC7984/extensions/ERC7984Votes.solcontracts/utils/FHESafeMath.solcontracts/utils/HandleAccessManager.solcontracts/utils/structs/CheckpointsConfidential.soldocs/antora.ymldocs/templates/properties.jsdocs/templates/properties.jspackage.jsontest/finance/BatcherConfidential.test.tstest/helpers/interface.tstest/token/ERC7984/extensions/ERC7984ERC20Wrapper.test.ts
💤 Files with no reviewable changes (10)
- .changeset/nasty-walls-taste.md
- .changeset/happy-hands-find.md
- .changeset/sour-dodos-sniff.md
- .changeset/long-items-appear.md
- .changeset/fast-humans-run.md
- .changeset/full-worlds-rescue.md
- .changeset/shaky-pugs-return.md
- .changeset/modern-snakes-return.md
- .changeset/pink-areas-double.md
- .changeset/good-flies-win.md
Comment on lines
+208
to
216
| bytes32 unwrapRequestId_ = unwrapRequestId(batchId); | ||
| // finalize unwrap call will fail if already called by this contract or by anyone else | ||
| try ERC7984ERC20Wrapper(fromToken()).finalizeUnwrap(unwrapAmount_, unwrapAmountCleartext, decryptionProof) { | ||
| try IERC7984ERC20Wrapper(fromToken()).finalizeUnwrap(unwrapRequestId_, unwrapAmountCleartext, decryptionProof) { | ||
| // No need to validate input since `finalizeUnwrap` request succeeded | ||
| } catch { | ||
| // Must validate input since `finalizeUnwrap` request failed | ||
| bytes32[] memory handles = new bytes32[](1); | ||
| handles[0] = euint64.unwrap(unwrapAmount_); | ||
| handles[0] = euint64.unwrap(fromToken().unwrapAmount(unwrapRequestId_)); | ||
| FHE.checkSignatures(handles, abi.encode(unwrapAmountCleartext), decryptionProof); |
Contributor
There was a problem hiding this comment.
Don't treat every finalizeUnwrap revert as “already finalized”.
Now that wrappers are only validated at the interface boundary, this generic catch is too broad. A real unwrap failure can still fall through to _executeRoute after only checking the proof, even though this batch may never have received its underlying. The fallback path should only be used once unwrap finalization is known to have happened for this batch; otherwise the revert should bubble.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/finance/BatcherConfidential.sol` around lines 208 - 216, The catch
block after calling
IERC7984ERC20Wrapper(fromToken()).finalizeUnwrap(unwrapRequestId_,
unwrapAmountCleartext, decryptionProof) is too broad; only treat reverts that
indicate the unwrap was already finalized as the fallback path that validates
using FHE.checkSignatures and continues (handles,
euint64.unwrap(fromToken().unwrapAmount(unwrapRequestId_)),
unwrapAmountCleartext, decryptionProof). Modify the try/catch to inspect the
revert (use catch Error(string memory reason) or catch (bytes memory lowLevel)
to decode) and if the reason matches the wrapper’s “already finalized” sentinel
only then run the existing fallback signature checks and proceed, otherwise
re-throw / revert so real unwrap failures bubble up instead of falling through
to _executeRoute. Ensure you keep the same symbols: unwrapRequestId_,
finalizeUnwrap, fromToken(), unwrapAmountCleartext, decryptionProof, handles,
euint64.unwrap, fromToken().unwrapAmount, and FHE.checkSignatures.
Member
There was a problem hiding this comment.
Made an issue #343 to track this. We may not be able to do further introspection.
Contributor
There was a problem hiding this comment.
@arr00 Understood — thanks for opening #343 to track this. You're right that Solidity's try/catch has limited granularity; decoding custom errors from a catch (bytes memory) block is possible but brittle, and there may not be a reliable sentinel to distinguish "already finalized" from other failure modes depending on how the wrapper implements finalizeUnwrap. Happy to revisit the options when that issue is worked on.
🧠 Learnings used
Learnt from: james-toussaint
Repo: OpenZeppelin/openzeppelin-confidential-contracts PR: 160
File: test/token/ERC7984/extensions/ERC7984Rwa.test.ts:474-479
Timestamp: 2025-09-22T09:21:34.470Z
Learning: In ERC7984Freezable force transfers, the frozen balance is reset to the new balance only when the transferred amount exceeds the available balance (balance - frozen). If the transferred amount is within the available balance, the frozen amount remains unchanged. This is implemented via FHE.select(FHE.gt(encryptedAmount, confidentialAvailable(account)), confidentialBalanceOf(account), frozen).
Learnt from: arr00
Repo: OpenZeppelin/openzeppelin-confidential-contracts PR: 302
File: contracts/finance/compliance/ComplianceModuleConfidential.sol:41-43
Timestamp: 2026-02-04T21:38:19.848Z
Learning: In modules that implement a singleton OpenZeppelin confidential contracts pattern using per-caller accounting (msg.sender-based) for isolation, lifecycle hooks like onInstall/onUninstall intentionally have no access control and each caller only accesses its own accounting on the shared module instance. Apply this guideline to any similar Solidity modules in the repository, ensuring no cross-user access to another's state and that access is scoped to the caller's own accounting within the shared instance.
arr00
approved these changes
Apr 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Version 0.4.0 Release
New Features
Updates