feat(exit-certificate): Step B2/B3 - ERC-20 detection and extra contract holder decomposition

[joanestebanr]

🔄 Changes Summary

• Step B2: probes each contract address collected in Step A for the ERC-20 interface (totalSupply/balanceOf). Classifies contracts as DetectedERC20 (holds ≥1 tracked wrapped token) or DiscardedERC20. Outputs step-b2-detected-erc20s.json and step-b2-discarded-erc20s.json.
• Step B3: iterates over options.extraErc20Contracts. Reuses B2 holder data when available; otherwise calls balanceOf for every EOA from Step A. Outputs step-b3-erc20-holders.json.
• Step C: extended to incorporate SC-locked values from B2 detected ERC-20s.
• Step D: generates BridgeExit entries for ERC-20-locked balances from B2/B3.
• config: added extraErc20Contracts option; increased defaultStepAWindowSize from 5 000 to 150 000.
• Pipeline (run.go) updated to execute B2 and B3 between B and C.
• New types, RPC helpers, unit tests, docs and example config for the new steps.

⚠️ Breaking Changes

• 🛠️ Config: new optional field options.extraErc20Contracts (array of addresses). No breaking change — defaults to empty.

📋 Config Updates

"options": {
  "stepAWindowSize": 150000,
  "extraErc20Contracts": ["0xTokenAddress1", "0xTokenAddress2"]
}

✅ Testing

• 🤖 Automatic: unit tests added for Step B2 (step_b2_test.go) and Step B3 (step_b3_test.go); Step C tests extended.
• 🖱️ Manual: run full pipeline with extraErc20Contracts populated and verify step-b2-detected-erc20s.json, step-b3-erc20-holders.json outputs.

🐞 Issues

• Closes https://github.com/agglayer/pm/issues/341

📝 Notes

• Step B3 short-circuits when extraErc20Contracts is empty — no RPC calls made.
• defaultStepAWindowSize raised to 150 000 to reduce RPC round-trips on chains with large block ranges.

[claude]

Claude finished @joanestebanr's task in 1m 41s —— View job

---

✅ Code Review Complete

---

🚀 What's New

This PR implements Steps B2 and B3 for the exit certificate tool, enabling:

• Step B2: Automatic ERC-20 contract detection by probing contracts collected in Step A for the ERC-20 interface and checking if they hold tracked wrapped tokens
• Step B3: Holder decomposition for manually specified ERC-20 contracts (via extraErc20Contracts config option)
• Step C Enhancement: Integration of detected ERC-20 contracts' locked values into SC-locked calculations, with proportional distribution to token holders
• Step D Enhancement: Generation of individual BridgeExit entries for ERC-20 token holders instead of a single lump-sum exit
• Config Update: New extraErc20Contracts option and increased defaultStepAWindowSize from 5,000 to 150,000 blocks

🐛 Bug Fixes

None — this PR is purely additive functionality.

📋 Config Updates

{
  "options": {
    "stepAWindowSize": 150000,  // Default increased from 5,000
    "extraErc20Contracts": [     // New optional field
      "0xTokenAddress1",
      "0xTokenAddress2"
    ]
  }
}

Defaults: stepAWindowSize defaults to 150,000; extraErc20Contracts defaults to empty array (no breaking changes).

⚠️ Breaking Changes

None — all changes are backward compatible. The new extraErc20Contracts field is optional.

---

📊 Code Review Feedback

🟢 Strengths

1. Excellent Test Coverage: Comprehensive unit tests for Steps B2 and B3 (step_b2_test.go, step_b3_test.go) cover edge cases including empty inputs, RPC errors, mixed scenarios, and data validation.

2. Clear Separation of Concerns: B2 (automatic detection) and B3 (manual specification) are properly separated, allowing users to leverage both or either approach.

3. Robust Error Handling:

   • Silently treats non-ERC-20 contracts as discarded (step_b2.go:21)
   • Validates holder balances don't exceed contract holdings (step_c.go:97-102)
   • Proper context propagation for cancellation

4. Performance Optimizations:

   • Progress logging for long-running operations (step_b2.go:276-278)
   • Concurrent RPC batching with configurable limits
   • Short-circuit when extraErc20Contracts is empty (step_b3.go:24-27)

5. Documentation: Well-commented code with clear function headers explaining logic and data flow.

🟡 Suggestions for Improvement

1. Potential Data Race in detectERC20Contracts (step_b2.go:215-284)

   • Location: tools/exit_certificate/step_b2.go:276-278
   • Issue: Progress logging reads detected.Load() which is updated atomically, but the log message could theoretically be printed out of order due to goroutine scheduling.
   • Impact: Low — cosmetic logging issue only, no functional impact
   • Recommendation: Consider using a mutex-protected counter or accept the minor ordering inconsistency

2. Magic Number for Progress Granularity (step_b2.go:174)

   • Location: tools/exit_certificate/step_b2.go:174
   • Issue: probeProgressPct = 10 is hardcoded but not configurable
   • Impact: Low — works fine but could be a const with a comment explaining the choice
   • Recommendation: Add a comment explaining why 10% granularity was chosen

3. Holder Balance Validation Logic (step_c.go:97-102)

   • Location: tools/exit_certificate/step_c.go:97-102
   • Issue: Returns an error when distributed > contractHolds, which is correct, but the error message could be more actionable
   • Current: "corrupt balance data"
   • Suggestion: Add guidance like "— verify that Step B EOA collection includes all vault token holders"
   • Impact: Low — helps users debug configuration issues

4. Type Consistency in ERC20HolderBreakdown.Detected (types.go:140)

   • Location: tools/exit_certificate/types.go:140
   • Issue: The Detected field is a pointer (*DetectedERC20), which is appropriate for optional data, but requires nil checks everywhere
   • Impact: Low — current code handles this correctly
   • Observation: This is fine, but consider documenting the invariant: "Detected is non-nil iff the contract was in B2 detected list"

5. No Validation of ExtraERC20Contracts Addresses (config.go:286-291)

   • Location: tools/exit_certificate/config.go:286-291
   • Issue: Config parsing doesn't validate that extraErc20Contracts addresses are non-zero
   • Impact: Low — zero address would just result in no holders found
   • Recommendation: Consider logging a warning if any address is zero

6. Missing Test for B2→B3 Data Flow

   • Issue: While B2 and B3 have comprehensive unit tests, there's no integration test verifying the full pipeline where B2 detected data flows into B3
   • Impact: Low — the logic is straightforward (simple map lookup in B3), but an integration test would increase confidence
   • Recommendation: Consider adding an integration test in integration_test.go or a dedicated test file

🔴 Issues Requiring Attention

None identified — no critical security, performance, or correctness issues found.

---

🔍 Detailed Analysis

Architecture & Design

• ✅ Steps B2/B3 fit cleanly into the existing pipeline between B1 and C
• ✅ Config changes are backward compatible (new fields are optional)
• ✅ Type system properly models the domain (DetectedERC20, DiscardedERC20, ERC20HolderBreakdown)

Security

• ✅ No SQL injection or command injection risks (uses type-safe RPC calls)
• ✅ No unsafe pointer arithmetic
• ✅ Proper validation of holder distribution totals (step_c.go:97-102)

Performance

• ✅ Increased defaultStepAWindowSize to 150,000 reduces RPC round-trips (config.go:84)
• ✅ Concurrent ERC-20 probing with progress logging
• ✅ Short-circuit optimization when no extra contracts configured

Code Quality

• ✅ Follows Go naming conventions (CamelCase for exports)
• ✅ Proper error wrapping with context (fmt.Errorf with %w)
• ✅ Consistent use of require in tests (per CLAUDE.md guidelines)
• ✅ Line length under 120 chars (per linter config)

Test Coverage

• ✅ step_b2_test.go: 437 lines covering edge cases
• ✅ step_b3_test.go: 211 lines covering edge cases
• ✅ step_c_test.go: Extended with holder bridge tests
• ✅ Mock RPC server with realistic response handling

---

📝 Final Verdict

LGTM with minor suggestions — This is high-quality, well-tested code that follows best practices. The suggestions above are optional improvements, not blockers. The PR is ready to merge.

Recommendation: ✅ Approve and merge

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad4fcfe431

ℹ️ 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".

[chatgpt-codex-connector]

Populate collateral for extra ERC-20s missing from B2

When extraErc20Contracts contains a vault/staking token that was not part of the Step B1 contract list (for example, a passive ERC-20 holder whose account was not touched by traced transactions), b2Detected[addr] is nil here. Step C skips every breakdown with Detected == nil, so the holder balances fetched in B3 are never converted into holder bridge exits and the value remains routed to the generic exit address. B3 should fetch/attach the wrapped-token holdings for configured contracts that B2 did not detect, or include those contracts in the B2 probe.

Useful? React with 👍 / 👎.

[web3security]

LGTM my only comment is that in case we finally decide to distributed WETH as ERC20 tokens individually, we should anyway add it since the beginning to avoid adding it after the process started by mistake. Thanks!

[joanestebanr]

LGTM my only comment is that in case we finally decide to distributed WETH as ERC20 tokens individually, we should anyway add it since the beginning to avoid adding it after the process started by mistake. Thanks!

I suggest that the new step B2 can show that are more ERC20 contracts to consider to distribute individually. So even that you add from the beginning the WETH contract I suggest to verify the result of step B2 to identify possibles other contracts. WDYT?