Conversation
✅ 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 ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a confidential token batching system (BatcherConfidential) enabling batch deposit management, swaps, and claims using encrypted balances and confidential transfers. Supporting mock contracts for testing and comprehensive test coverage are included. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant BC as BatcherConfidential
participant Wrapper as ERC7984<br/>Wrapper
participant Exchange as ExchangeMock
participant Route as Route<br/>Execution
User->>BC: join(encryptedAmount)
BC->>Wrapper: confidentialTransfer(fromToken)
Wrapper-->>BC: ✓ deposited
BC->>BC: update deposits & totalDeposits
User->>BC: dispatchBatch()
BC->>BC: compute unwrapAmount
BC->>Wrapper: unwrap(fromToken)
Wrapper-->>BC: ✓ unwrap initiated
BC->>BC: dispatchBatchCallback()
BC->>Route: _executeRoute()
Route->>Exchange: swapAToB()
Exchange-->>Route: swapped toToken
Route->>Wrapper: wrap(toToken)
Wrapper-->>Route: ✓ wrapped
Route->>BC: setExchangeRate()
User->>BC: claim(batchId)
BC->>BC: compute share (deposit × exchangeRate)
BC->>Wrapper: transfer(toToken)
Wrapper-->>User: ✓ claimed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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: 1
🤖 Fix all issues with AI agents
In `@contracts/utils/BatcherConfidential.sol`:
- Around line 64-65: The call to toToken().confidentialTransfer(msg.sender,
amountToSend) must capture and handle its return value instead of ignoring it:
call confidentialTransfer and store its returned euint64 (e.g., sent), then if
sent < amountToSend adjust the stored deposit (restore remaining deposit =
previous deposit - sent) or revert the operation; follow the pattern used in
cancel() where euint64 sent = fromToken().confidentialTransfer(...) is used to
recalculate deposit so users aren't left without tokens or deposit when the
transfer fails.
🧹 Nitpick comments (5)
contracts/mocks/finance/ExchangeMock.sol (1)
26-31: Division by zero risk ifexchangeRateis zero.If
exchangeRateis set to0,swapBToAwill revert with a division by zero error. While this is a mock contract, consider adding a guard or documenting this precondition.💡 Suggested guard
function swapBToA(uint256 amount) public returns (uint256) { + require(exchangeRate != 0, "Exchange rate cannot be zero"); uint256 amountOut = (amount * 1e18) / exchangeRate; require(tokenB.transferFrom(msg.sender, address(this), amount), "Transfer of token B failed"); require(tokenA.transfer(msg.sender, amountOut), "Transfer of token A failed"); return amountOut; }contracts/utils/BatcherConfidential.sol (2)
1-7: Non-standard pragma placement.The
pragma soliditydirective is placed after the import statements. While valid Solidity, convention is to place pragma before imports for better readability and consistency.♻️ Standard ordering
// SPDX-License-Identifier: MIT +pragma solidity ^0.8.27; import {FHE, externalEuint64, euint64, ebool, euint128} from "@fhevm/solidity/lib/FHE.sol"; import {ERC7984ERC20Wrapper} from "../token/ERC7984/extensions/ERC7984ERC20Wrapper.sol"; import {FHESafeMath} from "./../utils/FHESafeMath.sol"; - -pragma solidity ^0.8.27;
198-201: Guard against zero exchange rate could be strengthened.The
_setExchangeRatefunction only checks if the exchange rate was already set but doesn't prevent setting a zero exchange rate, which would cause issues in theclaimfunction's require check on line 52 (making the batch appear unfinalized).💡 Suggested enhancement
function _setExchangeRate(uint256 batchId, uint64 exchangeRate_) internal virtual { require(exchangeRate(batchId) == 0, ExchangeRateAlreadySet(batchId)); + require(exchangeRate_ != 0, "Exchange rate cannot be zero"); _batches[batchId].exchangeRate = exchangeRate_; }contracts/mocks/utils/BatcherConfidentialSwapMock.sol (1)
51-68: Potential division by zero and overflow risks in_executeRoute.
Line 66: If
unwrapAmountis0, division will revert. While this may be prevented by the flow (non-zero deposits required), it's worth documenting.Line 67: The cast
uint64(exchangeRate)can silently truncate ifexchangeRateexceedstype(uint64).max. This could occur with extreme rate ratios.Since this is a mock for testing, these may be acceptable, but consider adding guards or using SafeCast for production-like mocks.
💡 Safer implementation
// Set the exchange rate for the batch based on swapped amount uint256 exchangeRate = (amountOut * 1e18) / unwrapAmount; - _setExchangeRate(batchId, uint64(exchangeRate)); + require(exchangeRate <= type(uint64).max, "Exchange rate overflow"); + _setExchangeRate(batchId, uint64(exchangeRate));test/utils/BatcherConfidential.test.ts (1)
140-148: Inconsistent assertion format.Line 147 uses
0(number) while other decrypt assertions use string format like'1000'or'3000'. This inconsistency could cause test failures depending on howuserDecryptEuintreturns values.♻️ Consistent string format
).to.eventually.eq(0); + // Consider using string format for consistency: + // ).to.eventually.eq('0');
* fix bug in the cancel workflow * use an execution outcome enum
| SafeERC20.forceApprove(IERC20(fromToken().underlying()), address(fromToken()), type(uint256).max); | ||
| SafeERC20.forceApprove(IERC20(toToken().underlying()), address(toToken()), type(uint256).max); |
There was a problem hiding this comment.
could we add a comment somewhere telling what we need this to call wrap in ERC7984ERC20Wrapper (required for when the underlying ERC20 is not ERC1363
There was a problem hiding this comment.
The assumption is that most ERC-20 tokens are not 1363 so for compatibility, we use the approve and wrap flow instead.
There was a problem hiding this comment.
Requesting a comment to state that?
5 participants
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.