fix: debug trace bridge call matching

[arnaubennassar]

🔄 Changes Summary

• Filter debug trace call matching to actual CALL frames in bridgesync and claimsync, avoiding calldata extraction from DELEGATECALL, STATICCALL, CALLCODE, or missing-type frames.
• Add call tracer frame type constants and include the type field in decoded trace calls.
• Make claim calldata extraction reject ambiguous distinct matching bridge calls while accepting duplicate identical matches.
• Extend bridge and claim sync tests for call-frame filtering, ambiguous calldata, and updated mocked debug traces.

⚠️ Breaking Changes

• 🛠️ Config: None
• 🔌 API/CLI: None
• 🗑️ Deprecated Features: None

📋 Config Updates

• 🧾 Diff/Config snippet: None

✅ Testing

• 🤖 Automatic: go test ./bridgesync ./claimsync
• 🖱️ Manual: Not run

🐞 Issues

• None

🔗 Related PRs

• None

📝 Notes

• The fix keeps debug trace traversal behavior but only treats canonical CALL frames to the bridge as extraction candidates, preventing delegate-call calldata from being selected as if it were a direct bridge call.

[claude]

Claude finished @arnaubennassar's task in 1m 38s —— View job

---

Code Review Complete ✓

---

🚀 What's New

This PR fixes debug trace call matching in bridgesync and claimsync by filtering only CALL frame types and rejecting ambiguous claim calldata extraction when multiple distinct matching bridge calls exist.

🐛 Bug Fixes

• Call frame filtering: Only CALL frames to the bridge are now considered for calldata extraction, preventing DELEGATECALL, STATICCALL, CALLCODE, or missing-type frames from being incorrectly treated as direct bridge calls
• Ambiguous calldata rejection: Claims with multiple distinct matching bridge calls now properly error out instead of silently using the wrong calldata

📋 Config Updates

None

⚠️ Breaking Changes

None

---

Review Feedback

🟢 Overall Assessment

This is a well-implemented security fix with excellent test coverage. The changes correctly address the core issue of distinguishing between different call frame types in debug traces.

✅ Strengths

1. Security Fix - Properly filters call frame types to prevent delegate call exploitation
2. Comprehensive Testing - Excellent test coverage including edge cases for:
   • Non-CALL frame types (DELEGATECALL, STATICCALL, CALLCODE)
   • Reverted calls
   • Ambiguous calldata scenarios
   • Duplicate identical calls vs distinct calls
3. Clear Constants - Well-defined call type constants improve code readability
4. Consistent Implementation - Same pattern applied to both bridgesync and claimsync
5. DeepEqual for Duplicates - Using reflect.DeepEqual is appropriate for detecting true duplicates

🟡 Suggestions (Code Quality)

1. Code Duplication: Call Type Constants

Location: bridgesync/downloader.go:67-81, claimsync/downloader.go:69-83

The call type constants are duplicated across both packages. Consider extracting to a shared package.

Suggestion:

// In aggkit/types/calltracer.go or similar
package types

const (
    CallTypeCall = "CALL"
    CallTypeDelegateCall = "DELEGATECALL"
    CallTypeStaticCall = "STATICCALL"
    CallTypeCallCode = "CALLCODE"
    CallTypeCreate = "CREATE"
    CallTypeCreate2 = "CREATE2"
    CallTypeSelfDestruct = "SELFDESTRUCT"
)

Then import in both packages. This follows DRY principle and ensures consistency.

2. Error Message Clarity

Location: claimsync/downloader.go:566-567

The error message "ambiguous claim calldata" could be more descriptive about why this is problematic.

Current:

return fmt.Errorf("ambiguous claim calldata for globalIndex %s: found %d matching bridge calls",
    c.GlobalIndex.String(), len(candidates))

Suggested:

return fmt.Errorf("ambiguous claim calldata for globalIndex %s: found %d distinct matching bridge calls (expected 1 unique match)",
    c.GlobalIndex.String(), len(candidates))

3. Performance: DeepEqual Usage

Location: claimsync/downloader.go:575-581

Using reflect.DeepEqual in a loop is acceptable for this use case (small candidate lists), but worth noting:

• reflect.DeepEqual compares all fields including private ones
• For Claim structs with pointers (*big.Int), this correctly handles value comparison
• Performance is fine since candidate lists are typically small (1-3 items)

No change needed, but consider documenting this design choice if candidate lists could grow large in the future.

4. Missing Type Check Test in bridgesync

Location: bridgesync/downloader_test.go

The claimsync package has a dedicated test TestFindCallSkipsNonCallFrameTypes (lines 80-106 in claimsync/downloader_test.go), but bridgesync doesn't have an equivalent isolated test for this filtering behavior.

Suggestion: Add a similar test to bridgesync/downloader_test.go:

func TestFindCallSkipsNonCallFrameTypes(t *testing.T) {
    // Test that DELEGATECALL, STATICCALL, etc. are skipped
    // Similar to claimsync/downloader_test.go:80-106
}

🟢 What's Done Well

1. Test Organization - The test file structure is excellent:

   • TestFindCall - Basic functionality
   • TestFindCallSkipsNonCallFrameTypes - Frame type filtering
   • TestFindCallWithMixedMethods - Method filtering
   • TestSetClaimCalldataFromRootFiltersDelegateCall - Integration testing

2. Edge Case Coverage - Tests cover:

   • Reverted calls (properly skipped)
   • Empty candidate lists (error returned)
   • Duplicate identical calls (accepted)
   • Distinct ambiguous calls (rejected)

3. Security Awareness - The PR correctly identifies the security risk of accepting delegate call calldata as if it were a direct bridge call

4. Code Style - Follows Go best practices:

   • Proper error wrapping with context
   • Clear constant naming
   • Descriptive test names
   • Uses require (not assert) in tests ✓

🔴 Critical Issues

None found. This PR is production-ready.

---

Additional Notes

Why This Fix Matters

When a transaction uses DELEGATECALL to invoke the bridge contract, the calldata might differ from what would be used in a direct CALL. By only considering CALL frames, this PR ensures:

1. Correct calldata extraction - Only direct bridge calls provide authoritative calldata
2. Security - Prevents malicious delegate calls from injecting misleading calldata
3. Correctness - Ambiguous scenarios (multiple distinct calls) are now detected and rejected

Test Coverage Analysis

The PR adds 181 new lines to claimsync/downloader_test.go, covering:

• Frame type filtering (lines 80-106)
• Mixed method calls (lines 108-160)
• Unrecognized methods (lines 162-204)
• Delegate call filtering (lines 293-428)
• Ambiguous vs duplicate scenarios (lines 413-427)

This represents excellent test coverage for the fix.

---

Summary

✅ Approve with suggestions - The core fix is correct and well-tested. The suggestions above are minor code quality improvements that don't block merging.

Priority of suggestions:

• 🟡 Suggestion Unify L1 syncers #1 (Extract constants): Low priority, nice-to-have
• 🟡 Suggestion Split L1 / L2 responsibilities for lastgersync #2 (Error message): Low priority, improves clarity
• 🟡 Suggestion Add UTs for lastgersync #3 (Performance note): Informational only
• 🟡 Suggestion Dev Docs #4 (Test coverage): Low priority, bridgesync changes are well-covered by e2e tests

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
95.5% Coverage on New Code
0.6% Duplication on New Code

See analysis details on SonarQube Cloud