Conversation
WalkthroughAdds a burn_id: u256 parameter to burn_xzb_for_unlock and includes it in BurnEvent. Updates the L2 bridge contract, its public interface, and tests accordingly. Minor stylistic change in storage pop. Adjusts xZBERC20 test trait impl generics without changing behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Bridge as ZeroXBridgeL2
participant Token as xZB ERC20
participant Events as L2 Event Log
User->>Bridge: burn_xzb_for_unlock(burn_id, amount)
Bridge->>Token: burn(user, amount)
Token-->>Bridge: Burn OK
Note right of Bridge: Prepare BurnEvent payload<br/>including burn_id
Bridge->>Events: Emit BurnEvent(burn_id, user, amount, nonce, commitment_hash)
Events-->>User: Event recorded
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
🧹 Nitpick comments (5)
contracts/L2/src/core/ZeroXBridgeL2.cairo (3)
136-142: Adding burn_id to BurnEvent improves traceabilityGood addition; it gives indexers and off-chain consumers a stable handle for burns. Consider marking burn_id as a keyed field to make it indexable in event logs.
Minimal diff:
- pub struct BurnEvent { - pub burn_id: u256, + pub struct BurnEvent { + #[key] + pub burn_id: u256,Note: this changes the event ABI (keys vs data). If you adopt it, verify any event parsing code/tests.
245-253: Tweak panic message for accuracyThe check is correct, but the message says “less than zero,” which can’t happen for u256. Make the message precise.
- assert(burn_amount_usd > 0, 'Burn amount less than zero'); + assert(burn_amount_usd > 0, 'Burn amount must be greater than zero');
250-252: Potential u256 overflow in mul/div sequence(amount * PRECISION) / mint_rate can overflow u256 for large amounts before division. Same pattern exists in mint_and_claim_xzb ((usd_amount * mint_rate) / PRECISION). Not urgent for this PR, but worth addressing.
- Prefer a mul_div-style helper that does 512-bit intermediate math (mul then exact division).
- If that’s not available, guard inputs (assert amount <= MAX_SAFE) or refactor to reduce overflow risk with acceptable rounding trade-offs.
- Document limits in interface comments if you rely on bounded ranges.
contracts/L2/src/interfaces/IZeroXBridgeL2.cairo (1)
18-19: Interface synced with implementation — LGTMSignature now takes burn_id before amount and matches the core contract.
Add a short doc comment describing burn_id semantics (e.g., client-provided vs contract-generated, uniqueness expectations).
contracts/L2/tests/test_ZeroXBridgeL2.cairo (1)
547-552: Avoid repeated re-declarations of burn_id for readabilityRebinding burn_id five times is legal but a bit noisy. Consider using distinct identifiers (burn_id1..burn_id5) or an array and iterate.
Example approach:
- let burn_id = 123124_u256; + let burn_id1 = 123124_u256; ... - let burn_id = 1223124_u256; + let burn_id2 = 1223124_u256; # …and so on, or store in an array and loop.Also applies to: 556-560, 564-568, 572-576, 580-584
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
contracts/L2/src/core/ZeroXBridgeL2.cairo(4 hunks)contracts/L2/src/core/xZBERC20.cairo(1 hunks)contracts/L2/src/interfaces/IZeroXBridgeL2.cairo(1 hunks)contracts/L2/tests/test_ZeroXBridgeL2.cairo(7 hunks)
🔇 Additional comments (6)
contracts/L2/src/core/xZBERC20.cairo (1)
97-102: Confirm generated trait signature for IxZbTestPlease manually verify that the trait generated by
#[generate_trait]for your test interface matches the impl you’ve written:
- Inspect the macro-expanded definition of
IxZbTest(e.g., via your IDE’s Cairo stub file or by running the Cairo compiler with expansion output).- If the generated trait is declared as
trait IxZbTest<ContractState> { … }, update your impl to:impl TestMint of IxZbTest<ContractState> { #[external(v0)] fn test_mint(ref self: ContractState, recipient: ContractAddress, amount: u256) { self.erc20.mint(recipient, amount); } }- If the trait is non-generic (
trait IxZbTest { … }), your current impl is correct.Also consider isolating this test-only external method—either gate it behind a test feature flag or move it into a dedicated test helper module—to avoid exposing it in your production ABI.
contracts/L2/src/core/ZeroXBridgeL2.cairo (2)
281-287: Event emission now includes burn_id — LGTMEmission payload aligns with the updated struct and interface. No concerns.
484-485: Storage pop style change — LGTMUsing
let _ = …pop()silences the unused value warning and keeps intent clear.contracts/L2/tests/test_ZeroXBridgeL2.cairo (3)
155-160: Expected BurnEvent updated correctlyAssertions include burn_id and preserve the original hash computation (without burn_id), which matches the contract’s commitment logic. Looks good.
627-631: Repeated burn_id inside loop — confirm intentUsing a constant burn_id = 1 for all 8 burns is fine since the contract doesn’t enforce uniqueness, but double-check this reflects intended semantics (some indexers may assume burn_id distinguishes burns).
If uniqueness is desired, compute burn_id from the loop counter.
133-137: All burn_xzb_for_unlock Call Sites Updated and Verified — Approving Changes
- Verified every occurrence of
.burn_xzb_for_unlock(incontracts/L2/tests/test_ZeroXBridgeL2.cairo(lines 133–137, 209–212, 244–249, 505–509, 550–552, 558–560, 566–568, 574–576, 582–584, 629–631) now uses(burn_id, burn_amount)in the correct order.- Confirmed the interface definition in
contracts/L2/src/interfaces/IZeroXBridgeL2.cairois updated to
fn burn_xzb_for_unlock(ref self: TContractState, burn_id: u256, amount: core::integer::u256);- Confirmed the implementation in
contracts/L2/src/core/ZeroXBridgeL2.cairomatches the new signature:
fn burn_xzb_for_unlock(ref self: ContractState, burn_id: u256, amount: u256) { … }LGTM – no further changes needed.
2 participants
Summary by CodeRabbit
New Features
Tests