Feat: include depositId in depositAsset and emit single enriched DepositEvent#108
Feat: include depositId in depositAsset and emit single enriched DepositEvent#108JoE11-y merged 1 commit intoExplore-Beyond-Innovations:mainfrom truthixify:feat/single-deposit-event-with-id
Conversation
WalkthroughThe changes update the deposit flow to emit a single enriched Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ZeroXBridgeL1
participant MerkleManager
participant StatelessMmr
User->>ZeroXBridgeL1: depositAsset(depositId, assetType, token, amount, user)
ZeroXBridgeL1->>ZeroXBridgeL1: Compute commitmentHash (incl. depositId)
ZeroXBridgeL1->>MerkleManager: appendDepositHash(commitmentHash)
MerkleManager->>StatelessMmr: appendWithPeaksRetrieval(commitmentHash, lastPeaks, lastElementsCount)
StatelessMmr-->>MerkleManager: (newPeaks, newElementsCount, newRoot)
MerkleManager-->>ZeroXBridgeL1: (newRoot, newLeavesCount, elementCount)
ZeroXBridgeL1-->>User: emit DepositEvent(depositId, ..., newRoot, elementCount)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
contracts/L1/src/MerkleManager.sol (2)
39-39: Remove unusedDepositHashAppendedevent definitionAccording to the PR objectives, the
DepositHashAppendedevent should be completely removed as it's replaced by the enrichedDepositEventin ZeroXBridgeL1. This event definition is no longer used inappendDepositHashand creates inconsistency.- event DepositHashAppended(uint256 index, bytes32 commitmentHash, bytes32 rootHash, uint256 elementsCount);
78-106: AlignmultiAppendDepositHashwith the new append pattern
multiAppendDepositHashstill emitsDepositHashAppendedand doesn’t return anything, whileappendDepositHashnow returns(bytes32 newRoot, uint256 newLeavesCount, uint256 elementCount)and no longer emits events. To avoid this inconsistency, update or removemultiAppendDepositHashas follows:• In
contracts/L1/src/MerkleManager.sol:
– Change signature to
function multiAppendDepositHash(bytes32[] memory commitmentHashes) internal returns (bytes32 newRoot, uint256 newLeavesCount, uint256 elementCount)
– Remove theemit DepositHashAppended(…)call inside the loop
– Accumulate and return the finalnextRoot,leavesCount, andnextElementsCountafter the loop• In
contracts/L1/test/mocks/MerkleManagerMock.sol:
– AdjustmultiAppendDepositHashPublicto capture and return the new values from the internal call• In
contracts/L1/test/MerkleManager.t.sol:
– Update tests to consume the returned(newRoot, newLeavesCount, elementCount)instead of relying on emitted eventsThis ensures batch‐append behaves identically to the single‐append API and defers event emission to
ZeroXBridgeL1.contracts/L1/src/ZeroXBridgeL1.sol (1)
193-251: Add user registration validationThe function uses
userRecord[user]in the commitment hash calculation without verifying the user is registered. If an unregistered user deposits, their starknet public key will be 0, making funds unrecoverable on L2.Add registration check at the beginning of the function:
function depositAsset(uint256 depositId, AssetType assetType, address tokenAddress, uint256 amount, address user) external payable returns (uint256) { require(amount > 0, "ZeroXBridge: Amount must be greater than zero"); require(user != address(0), "ZeroXBridge: Invalid user address"); + require(userRecord[user] != 0, "ZeroXBridge: User not registered"); TokenAssetData memory tokenData = getTokenData(assetType, tokenAddress);
🧹 Nitpick comments (2)
contracts/L1/src/ZeroXBridgeL1.sol (1)
243-243: Remove unused variablecountThe
countvariable is assigned fromappendDepositHashbut never used. Consider removing it to improve code clarity.- (bytes32 newRoot, uint256 count, uint256 elementCount) = appendDepositHash(commitmentHash); + (bytes32 newRoot, , uint256 elementCount) = appendDepositHash(commitmentHash);contracts/L1/test/ZeroXBridgeL1.t.sol (1)
703-725: Use unique depositIds in testsThe test uses
depositId = 1for both ETH deposit (line 703) and DAI deposit (line 713). If depositIds are meant to be unique identifiers, this test should use different values to better reflect production behavior.// 2. Deposit ETH vm.prank(user); uint256 depositId = 1; bridge.depositAsset{value: depositEth}(depositId, ZeroXBridgeL1.AssetType.ETH, address(0), depositEth, user); // Expect TokenReserveUpdated for DAI vm.expectEmit(true, false, false, true); emit TokenReserveUpdated(address(dai), depositDai); // 3. Deposit DAI vm.prank(user); - uint256 depositId1 = 1; + uint256 depositId1 = 2; bridge.depositAsset(depositId1, ZeroXBridgeL1.AssetType.ERC20, address(dai), depositDai, user); // Expect TokenReserveUpdated for USDC vm.expectEmit(true, false, false, true); emit TokenReserveUpdated(address(usdc), depositUsdc); // 4. Deposit USDC vm.prank(user); - uint256 depositId2 = 2; + uint256 depositId2 = 3;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/L1/src/MerkleManager.sol(2 hunks)contracts/L1/src/ZeroXBridgeL1.sol(3 hunks)contracts/L1/test/ZeroXBridgeL1.t.sol(16 hunks)
Thanks, let’s do more of this. |
2 participants
Summary
This PR refactors the deposit flow to emit a single DepositEvent with all necessary data, simplifying off-chain indexing.
Changes
Verification
Closes
Closes #107
Summary by CodeRabbit