fix(l2): keep newest withdrawal messages in monitor widget

[avilagaston9]

Motivation

The L2 integration test (test_erc20_roundtrip) panics with unwrap() on a None value at integration_tests.rs:705 after ~8 consecutive test runs against the same L1/L2 instance. The find_withdrawal_with_widget helper creates a fresh L2ToL1MessagesTable (starting from block 0), fetches all withdrawal logs, and searches for the latest withdrawal — but on_tick uses truncate(50) which keeps the oldest 50 items. After enough runs accumulate >50 withdrawal events, the newest withdrawal falls outside the window.

Description

Replace truncate(50) with drain(..len - 50) in L2ToL1MessagesTable::on_tick so that the newest 50 messages are kept instead of the oldest. This is also the correct behavior for the monitor UI, which should show recent activity.

Checklist

• Updated STORE_SCHEMA_VERSION (crates/storage/lib.rs) if the PR includes breaking changes to the Store requiring a re-sync.

[github-actions]

🤖 Kimi Code Review

Review of PR #6197

File: tooling/monitor/src/widget/l2_to_l1_messages.rs

Issues Found:

1. Logic Bug in Truncation Logic (Lines 173-179):

   • The current implementation uses drain(..len - 50) which removes the oldest messages, but the original truncate(50) was keeping the first 50 (oldest) messages. This changes the behavior significantly.
   • The variable name new_l1_to_l2_messages is misleading - this appears to actually contain L2 to L1 messages based on the struct name and context.

2. Off-by-One Error (Line 178):

   • self.items.truncate(50 - n_new_latest_batches) could underflow if n_new_latest_batches > 50, though this is unlikely given the previous truncation.

3. Performance:

   • Using drain(..len - 50) is less efficient than truncate(50) for keeping the most recent items. Consider using split_off instead:

   if new_l1_to_l2_messages.len() > 50 {
       new_l1_to_l2_messages = new_l1_to_l2_messages.split_off(new_l1_to_l2_messages.len() - 50);
   }

4. Code Clarity:

   • The variable naming is confusing. If these are L2 to L1 messages, the variable should be renamed from new_l1_to_l2_messages to new_l2_to_l1_messages.

Suggested Fix:

// Keep only the 50 most recent messages
let mut new_l2_to_l1_messages = fetch_l2_to_l1_messages(
    self.last_indexed_block,
    current_block,
    rollup_client,
)
.await?;

// Truncate to keep most recent 50
new_l2_to_l1_messages.truncate(50);

let n_new_messages = new_l2_to_l1_messages.len();
self.items.truncate(50.saturating_sub(n_new_messages));
self.items.extend(new_l2_to_l1_messages);

This maintains the original behavior while fixing the performance and clarity issues.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Looks good overall. This change fixes a correctness bug and is idiomatic.

Findings

1. tooling/monitor/src/widget/l2_to_l1_messages.rs:170
   The prior truncate(50) kept the oldest 50 entries, which is incorrect if you want the most recent 50. Using drain(..len - 50) correctly removes the oldest and keeps the newest. This also avoids underflow with len <= 50.

No security, performance, or memory-safety issues found in this diff.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Now I have a complete understanding of the PR. Let me prepare the review.

---

PR Review: Fix L2 Monitor Widget to Keep Newest Withdrawal Messages

Summary

This PR fixes a critical bug in the L2ToL1MessagesTable widget where repeated test runs would fail because the widget kept the oldest 50 messages instead of the newest 50 messages. The fix correctly changes the behavior to retain recent messages.

Code Correctness

The Fix is Correct ✓

The change from truncate(50) to drain(..len - 50) properly keeps the newest messages:

Before: truncate(50) keeps indices [0..50] (oldest messages)
After: drain(..len - 50) removes indices [0..len-50] and keeps [len-50..len] (newest messages)

This aligns with line 182 (self.items.rotate_right(n_new_latest_batches)), which expects new items to be prepended, making the widget correctly display recent activity.

Potential Issue: Logic Consistency

File: tooling/monitor/src/widget/l2_to_l1_messages.rs:179

There's an inconsistency in how limits are applied:

1. Line 173-176: Limits new_l1_to_l2_messages to 50 items (keeps newest)
2. Line 179: self.items.truncate(50 - n_new_latest_batches) keeps the oldest items from the existing list

This means:

• New messages: newest 50 kept ✓
• Old messages in self.items: oldest kept ✗

Example scenario:

• self.items has 50 old messages (A1-A50)
• Fetch 10 new messages (B1-B10)
• Line 179 truncates self.items to 40, keeping A1-A40 (oldest)
• After rotation, final order: B1-B10, A1-A40

This creates a mixed window where the 10 newest and 40 oldest messages are shown, missing the 10 messages between them (A41-A50). For a monitoring UI showing recent activity, line 179 should likely keep the newest 40 items, not the oldest.

Suggested fix:

let n_new_latest_batches = new_l1_to_l2_messages.len();
let items_to_keep = 50usize.saturating_sub(n_new_latest_batches);
if self.items.len() > items_to_keep {
    self.items.drain(..self.items.len() - items_to_keep);
}

Style & Best Practices

Variable Naming Issue

Line 166: Variable is named new_l1_to_l2_messages but this is the L2ToL1Messages table (as shown in the struct name and the widget title on line 366). This is confusing and should be renamed to new_l2_to_l1_messages for clarity.

Edge Case Handling ✓

The check if len > 50 prevents underflow in the subtraction len - 50 and avoids draining when there are ≤50 items. This is correct.

Performance & Safety

Memory Safety ✓

Using drain(..len - 50) is safe and efficient:

• Removes elements from the front of the vector
• Deallocates removed elements
• No risk of overflow (protected by the if len > 50 check)

Performance Note

drain(..n) on a Vec is O(m) where m is the number of remaining elements (they must be shifted). For this use case (capped at 50 items), the performance impact is negligible.

Testing

The PR description clearly identifies the root cause of the test failure. The fix directly addresses the bug where find_withdrawal_with_widget (integration_tests.rs:524) creates a fresh widget and expects to find the newest withdrawal, which now works correctly after this change.

Verdict

Approve with suggestion: The core fix is correct and solves the reported bug. However, line 179 has a similar logic issue that should ideally be addressed to ensure the monitor UI consistently displays the most recent messages.

Required Changes

None (the PR fixes the immediate bug)

Recommended Changes

1. Fix line 179 to keep newest items instead of oldest (for consistency)
2. Rename new_l1_to_l2_messages to new_l2_to_l1_messages throughout the function

---

Automated review by Claude (Anthropic) · custom prompt