feat(deposits): use Berachain deposit event for EIP-6110#226
feat(deposits): use Berachain deposit event for EIP-6110#226
Conversation
Replace the standard EIP-6110 DepositEvent signature with Berachain's custom Deposit event: Deposit(bytes,bytes,uint64,bytes,uint64) → 0x68af751683498a9f9be59fe8b0d52a64dd155255d85cdb29fea30b1e3f891d46. The Berachain event differs in two ways: the event name is Deposit (not DepositEvent) and amount/index are uint64 rather than bytes, requiring little-endian conversion when constructing deposit request bytestrings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a public Changes
Sequence DiagramsequenceDiagram
actor Client
participant Executor as Executor
participant Receipts as "Tx Receipts & Logs"
participant Parser as "deposits::parse_deposits_from_receipts"
participant Buffer as "Deposit Byte Buffer"
Client->>Executor: submit txs / advance block
Executor->>Receipts: collect receipts & logs
Executor->>Parser: parse_deposits_from_receipts(deposit_contract, receipts)
Parser->>Receipts: iterate logs
Parser->>Parser: filter by address & signature hash
alt decode success
Parser->>Buffer: encode deposit fields (pubkey, credentials, amount, signature, index)
Buffer-->>Executor: return concatenated deposit Bytes
else decode failure
Parser-->>Executor: return BlockValidationError
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c694d33a59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR updates Berachain’s EIP-6110 deposit-request construction to parse Berachain’s custom Deposit(bytes,bytes,uint64,bytes,uint64) event (including little-endian encoding for amount/index) instead of the standard DepositEvent signature.
Changes:
- Add a new
src/deposits.rsparser for Berachain’s customDepositevent and wire it into block execution request building. - Update the chain spec deposit contract topic to Berachain’s event signature hash.
- Add E2E coverage ensuring deposit requests appear only when a deposit transaction emits the expected log.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/mod.rs | Registers the new deposit_test E2E module. |
| tests/e2e/deposit_test.rs | Adds E2E tests + a mock log-emitter contract to validate deposit requests. |
| src/node/evm/executor.rs | Switches deposit parsing during Prague to the Berachain-specific parser. |
| src/lib.rs | Exposes the new deposits module. |
| src/deposits.rs | Implements Berachain custom deposit log decoding and request-bytes accumulation. |
| src/chainspec/mod.rs | Updates the configured deposit contract topic hash to Berachain’s Deposit signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/chainspec/mod.rs (1)
675-683:⚠️ Potential issue | 🟡 MinorExtract the deposit topic literal to a named constant and remove stale wording.
Line 683 hardcodes a magic hash, and the nearby note still states “same deposit topic as mainnet,” which is now inaccurate for this PR’s Berachain-specific signature.
♻️ Proposed cleanup
+const BERACHAIN_DEPOSIT_EVENT_TOPIC: B256 = + b256!("0x68af751683498a9f9be59fe8b0d52a64dd155255d85cdb29fea30b1e3f891d46"); + - // NOTE: in full node, we prune all receipts except the deposit contract's. We do not - // have the deployment block in the genesis file, so we use block zero. We use the same - // deposit topic as the mainnet contract if we have the deposit contract address in the - // genesis json. let deposit_contract = genesis.config.deposit_contract_address.map(|address| DepositContract { address, block: 0, - topic: b256!("0x68af751683498a9f9be59fe8b0d52a64dd155255d85cdb29fea30b1e3f891d46"), + topic: BERACHAIN_DEPOSIT_EVENT_TOPIC, });As per coding guidelines, Extract magic numbers into documented constants.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chainspec/mod.rs` around lines 675 - 683, Extract the hardcoded deposit topic hash into a named, documented constant (e.g., DEPOSIT_CONTRACT_TOPIC) and replace the inline b256!("0x68af75...") usage inside the DepositContract construction (where deposit_contract is created from genesis.config.deposit_contract_address) with that constant; also update the surrounding comment/note in mod.rs to remove the phrase "same deposit topic as mainnet" (or reword to reflect the Berachain-specific signature) so the note matches the new constant and avoids stale mainnet wording.
🧹 Nitpick comments (1)
src/deposits.rs (1)
22-22: Add documentation to the constant explaining its composition.As per coding guidelines, documentation should focus on 'why' rather than 'what'. Consider adding a brief doc comment explaining the field breakdown.
📝 Suggested documentation
+/// Deposit request byte layout: pubkey (48) + credentials (32) + amount (8) + signature (96) + index (8) const DEPOSIT_BYTES_SIZE: usize = 48 + 32 + 8 + 96 + 8;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/deposits.rs` at line 22, The constant DEPOSIT_BYTES_SIZE lacks a doc comment explaining why it equals that sum; add a brief doc comment above DEPOSIT_BYTES_SIZE describing the purpose of the constant and the field breakdown (e.g., which fields contribute 48, 32, 8, 96, and 8 bytes) and why those sizes are used so future readers understand the rationale; reference the constant name DEPOSIT_BYTES_SIZE when adding the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/chainspec/mod.rs`:
- Around line 675-683: Extract the hardcoded deposit topic hash into a named,
documented constant (e.g., DEPOSIT_CONTRACT_TOPIC) and replace the inline
b256!("0x68af75...") usage inside the DepositContract construction (where
deposit_contract is created from genesis.config.deposit_contract_address) with
that constant; also update the surrounding comment/note in mod.rs to remove the
phrase "same deposit topic as mainnet" (or reword to reflect the
Berachain-specific signature) so the note matches the new constant and avoids
stale mainnet wording.
---
Nitpick comments:
In `@src/deposits.rs`:
- Line 22: The constant DEPOSIT_BYTES_SIZE lacks a doc comment explaining why it
equals that sum; add a brief doc comment above DEPOSIT_BYTES_SIZE describing the
purpose of the constant and the field breakdown (e.g., which fields contribute
48, 32, 8, 96, and 8 bytes) and why those sizes are used so future readers
understand the rationale; reference the constant name DEPOSIT_BYTES_SIZE when
adding the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4972ffcd-b57a-4dfe-bd66-0f21b70de224
📒 Files selected for processing (15)
src/chainspec/mod.rssrc/consensus/mod.rssrc/deposits.rssrc/engine/validator.rssrc/evm/mod.rssrc/genesis/mod.rssrc/hardforks/mod.rssrc/lib.rssrc/node/evm/executor.rssrc/pool/mod.rssrc/rpc/api.rssrc/rpc/receipt.rssrc/transaction/mod.rstests/e2e/deposit_test.rstests/e2e/mod.rs
calbera
force-pushed
the
claude/update-deposit-event-signature-CnrWq
branch
from
6fd7bf4 to
a447495
Compare
Signed-off-by: Cal Bera <calbera@berachain.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/chainspec/mod.rs (1)
683-685: Consider extracting the deposit event topic hash into a documented constant.The topic hash is a magic number that could be extracted for consistency with the existing pattern (e.g.,
DEFAULT_MIN_BASE_FEE_WEIat line 34). This would also provide a single source of truth if referenced elsewhere.♻️ Suggested refactor
Add near the top of the file with other constants:
/// Berachain deposit event topic hash for `Deposit(bytes,bytes,uint64,bytes,uint64)`. /// This differs from Ethereum mainnet's `DepositEvent` signature. const BERACHAIN_DEPOSIT_EVENT_TOPIC: B256 = b256!("0x68af751683498a9f9be59fe8b0d52a64dd155255d85cdb29fea30b1e3f891d46");Then use it inline:
let deposit_contract = genesis.config.deposit_contract_address.map(|address| DepositContract { address, block: 0, - // This value is unique to Berachain's deposit event in its deposit contract. - // This is different from Eth mainnet's deposit event signature. - topic: b256!("0x68af751683498a9f9be59fe8b0d52a64dd155255d85cdb29fea30b1e3f891d46"), + topic: BERACHAIN_DEPOSIT_EVENT_TOPIC, });Please verify the topic hash is the correct keccak256 of the event signature
Deposit(bytes,bytes,uint64,bytes,uint64):#!/bin/bash # Verify the deposit event topic hash matches the expected keccak256 output python3 -c " from hashlib import sha3_256 # Using sha3_256 (keccak256 equivalent in Python's hashlib for Ethereum) # Actually, Ethereum uses original Keccak, not SHA3. Let's use a different approach. import subprocess # Calculate expected topic hash for Berachain's Deposit event event_sig = 'Deposit(bytes,bytes,uint64,bytes,uint64)' print(f'Event signature: {event_sig}') print(f'Expected topic from PR: 0x68af751683498a9f9be59fe8b0d52a64dd155255d85cdb29fea30b1e3f891d46') " # Use cast (foundry) if available to verify keccak256 if command -v cast &> /dev/null; then echo "Calculating keccak256 using cast:" cast keccak "Deposit(bytes,bytes,uint64,bytes,uint64)" else echo "cast not available, using Python with pysha3 or manual verification needed" pip install pysha3 -q 2>/dev/null || true python3 -c " try: import sha3 event_sig = 'Deposit(bytes,bytes,uint64,bytes,uint64)' k = sha3.keccak_256() k.update(event_sig.encode()) print(f'Calculated keccak256: 0x{k.hexdigest()}') except ImportError: print('pysha3 not available - please verify manually that keccak256(\"Deposit(bytes,bytes,uint64,bytes,uint64)\") = 0x68af751683498a9f9be59fe8b0d52a64dd155255d85cdb29fea30b1e3f891d46') " fiAs per coding guidelines: "Extract magic numbers into documented constants".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chainspec/mod.rs` around lines 683 - 685, Extract the magic topic hash into a documented constant (e.g., const BERACHAIN_DEPOSIT_EVENT_TOPIC: B256) near the other top-of-file constants and replace the inline b256!("0x68af...") usage in the topic field with that constant; include a doc comment stating this is the keccak256 topic for Deposit(bytes,bytes,uint64,bytes,uint64) and verify the value matches the keccak256 of that event signature before committing so the new constant is the single source of truth referenced by the code that previously used the literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/chainspec/mod.rs`:
- Around line 683-685: Extract the magic topic hash into a documented constant
(e.g., const BERACHAIN_DEPOSIT_EVENT_TOPIC: B256) near the other top-of-file
constants and replace the inline b256!("0x68af...") usage in the topic field
with that constant; include a doc comment stating this is the keccak256 topic
for Deposit(bytes,bytes,uint64,bytes,uint64) and verify the value matches the
keccak256 of that event signature before committing so the new constant is the
single source of truth referenced by the code that previously used the literal.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/deposits.rs (2)
30-30: Document the magic numbers in the size constant.The component sizes (48, 32, 8, 96, 8) are not self-documenting. Consider adding a brief comment or extracting named constants to clarify what each represents.
📝 Suggested documentation
-const DEPOSIT_BYTES_SIZE: usize = 48 + 32 + 8 + 96 + 8; +/// Size of a serialized deposit request in bytes. +/// pubkey (48) + credentials (32) + amount (8) + signature (96) + index (8) +const DEPOSIT_BYTES_SIZE: usize = 48 + 32 + 8 + 96 + 8;As per coding guidelines: "Extract magic numbers into documented constants".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/deposits.rs` at line 30, DEPOSIT_BYTES_SIZE currently uses unexplained magic numbers (48, 32, 8, 96, 8); replace them with named constants (e.g., PUBKEY_BYTES = 48, HASH_BYTES = 32, U64_BYTES = 8, SIGNATURE_BYTES = 96, TIMESTAMP_BYTES = 8) or add an inline comment explaining each component and then compute DEPOSIT_BYTES_SIZE by summing those constants so the intent is clear; update the constant definition in deposits.rs and ensure names match the actual struct fields used when serializing/deserializing deposits.
32-39: Consider validating field lengths for defense-in-depth.The function assumes correct field sizes (pubkey=48, credentials=32, signature=96). If a malformed log is processed, the output will have incorrect size, potentially causing issues downstream. Since the deposit contract is trusted infrastructure, this is optional defensive coding.
🛡️ Optional validation
+const PUBKEY_SIZE: usize = 48; +const CREDENTIALS_SIZE: usize = 32; +const SIGNATURE_SIZE: usize = 96; + fn accumulate_deposit_from_log(log: &Log<Deposit>, out: &mut Vec<u8>) { + debug_assert_eq!(log.pubkey.len(), PUBKEY_SIZE); + debug_assert_eq!(log.credentials.len(), CREDENTIALS_SIZE); + debug_assert_eq!(log.signature.len(), SIGNATURE_SIZE); out.reserve(DEPOSIT_BYTES_SIZE); out.extend_from_slice(log.pubkey.as_ref());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/deposits.rs` around lines 32 - 39, The function accumulate_deposit_from_log currently assumes pubkey, credentials, and signature have exact sizes; add defensive validation in accumulate_deposit_from_log to check log.pubkey.as_ref().len()==48, log.credentials.as_ref().len()==32, and log.signature.as_ref().len()==96 before extending out — if any check fails, return an error (change the function signature to return Result<(), DepositError> or similar) or explicitly panic/assert with a clear message; ensure the callers of accumulate_deposit_from_log are updated to handle the Result or potential panic and keep the existing byte-order handling for amount/index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/deposits.rs`:
- Line 30: DEPOSIT_BYTES_SIZE currently uses unexplained magic numbers (48, 32,
8, 96, 8); replace them with named constants (e.g., PUBKEY_BYTES = 48,
HASH_BYTES = 32, U64_BYTES = 8, SIGNATURE_BYTES = 96, TIMESTAMP_BYTES = 8) or
add an inline comment explaining each component and then compute
DEPOSIT_BYTES_SIZE by summing those constants so the intent is clear; update the
constant definition in deposits.rs and ensure names match the actual struct
fields used when serializing/deserializing deposits.
- Around line 32-39: The function accumulate_deposit_from_log currently assumes
pubkey, credentials, and signature have exact sizes; add defensive validation in
accumulate_deposit_from_log to check log.pubkey.as_ref().len()==48,
log.credentials.as_ref().len()==32, and log.signature.as_ref().len()==96 before
extending out — if any check fails, return an error (change the function
signature to return Result<(), DepositError> or similar) or explicitly
panic/assert with a clear message; ensure the callers of
accumulate_deposit_from_log are updated to handle the Result or potential panic
and keep the existing byte-order handling for amount/index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a090bd64-959a-4193-aec6-6d50ebcc4b72
📒 Files selected for processing (2)
src/chainspec/mod.rssrc/deposits.rs
✅ Files skipped from review due to trivial changes (1)
- src/chainspec/mod.rs
Replace the standard EIP-6110 DepositEvent signature with Berachain's custom Deposit event: Deposit(bytes,bytes,uint64,bytes,uint64) → 0x68af751683498a9f9be59fe8b0d52a64dd155255d85cdb29fea30b1e3f891d46.
The Berachain event differs in two ways: the event name is Deposit (not DepositEvent) and amount/index are uint64 rather than bytes, requiring little-endian conversion when constructing deposit request bytestrings.
Enabled in the CL here
Summary by CodeRabbit
New Features
Tests