feat: Treat already-monitored GER tx as expected #1490

[joanestebanr]

🔄 Changes Summary

• Cherry-pick feat: convert Error failed to add propose GER transaction: failed to add tx to get monitored: already exists into Info #1479

⚠️ Breaking Changes

None

📋 Config Updates

None

✅ Testing

• 🤖 Automatic: Unittest

🐞 Issues

• Closes feat: convert Error failed to add propose GER transaction: failed to add tx to get monitored: already exists into Info #1479

🔗 Related PRs

• feat: convert Error failed to add propose GER transaction: failed to add tx to get monitored: already exists into Info #1479
• zkevm-ethtx-manager #136

📝 Notes

• [Optional: design decisions, tradeoffs, or TODOs]

[claude]

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

---

📋 Code Review Complete ✅

---

🚀 What's New

This PR improves error handling in the GER (Global Exit Root) transaction submission logic to properly handle the ErrAlreadyExists case. When a transaction already exists in the monitoring database, the code now continues monitoring instead of failing immediately, and logs an appropriate message to distinguish between newly submitted transactions and already-monitored ones.

🐛 Bug Fixes

• Fixed GER transaction monitoring for duplicate submissions: Previously, when ethTxMan.Add() returned ErrAlreadyExists, the function would fail with an error. Now it correctly continues monitoring the existing transaction until it's mined, which prevents unnecessary failures when the same GER is submitted multiple times.

📋 Config Updates

None.

⚠️ Breaking Changes

None.

---

Code Review Feedback

✅ Overall Assessment

This is a good bug fix that addresses a real issue with the transaction monitoring logic. The code changes are minimal, focused, and well-tested. The test coverage for the new case is excellent.

Code Quality & Best Practices

🟢 Positive Observations:

1. Excellent test coverage: The new test case TestEVMChainGERSender_SubmitTransaction includes a specific scenario for ErrAlreadyExists, ensuring the fix is properly validated.
2. Clear error handling: The refactored error handling logic properly distinguishes between three cases: success, already exists, and actual errors.
3. Appropriate logging: Different log messages for new submissions vs. already-existing transactions help with debugging.
4. Import hygiene: Correctly added the errors import to handle errors.Is().

🟡 Suggested Improvements:

1. Logic flow could be simplified (aggoracle/chaingersender/evm.go:231-240)

   The current implementation has a slightly awkward flow with two separate if err != nil blocks. Consider this cleaner approach:

   id, err := c.ethTxMan.Add(ctx, targetAddr, common.Big0, txInput, c.gasOffset, nil)
   if err != nil {
       if errors.Is(err, ethtxmanager.ErrAlreadyExists) {
           c.logger.Infof("%s GER transaction already exists in monitoring DB with ID: %s. GER: %s", action, id.Hex(), ger.Hex())
       } else {
           return fmt.Errorf("failed to add %s GER transaction: %w", action, err)
       }
   } else {
       c.logger.Infof("%s GER transaction submitted with ID: %s. GER: %s", action, id.Hex(), ger.Hex())
   }

   This structure is more conventional: check for error first, handle the special case, then handle the success case. It eliminates the need to check if err != nil twice.

2. Test naming convention (aggoracle/chaingersender/evm_test.go:397)

   The test case name is quite long: "ErrAlreadyExists is not an error - monitoring continues until mined". Consider shortening to: "transaction already exists - continues monitoring" for consistency with other test names in the file.

Security & Performance

✅ No security issues identified. The error handling change doesn't introduce any security vulnerabilities.

✅ No performance concerns. The change has negligible performance impact.

Correctness Analysis

🟢 Logic is correct: The fix properly handles the ErrAlreadyExists case by:

1. Not treating it as a fatal error (continues execution)
2. Logging appropriately to distinguish from new submissions
3. Continuing to monitor the transaction until completion

One potential consideration:

🟡 Return value when ErrAlreadyExists (aggoracle/chaingersender/evm.go:231)

When ethTxMan.Add() returns ErrAlreadyExists, the id variable should still contain the transaction ID of the existing transaction (based on the test expectations at line 401). However, it would be good to verify this assumption is documented in the EthTxManager interface or implementation. If id could potentially be empty/zero when ErrAlreadyExists is returned, calling id.Hex() at line 239 could log a zero hash, which might be confusing.

Consider adding a defensive check:

if err != nil {
    if errors.Is(err, ethtxmanager.ErrAlreadyExists) {
        if id != (common.Hash{}) {
            c.logger.Infof("%s GER transaction already exists in monitoring DB with ID: %s. GER: %s", action, id.Hex(), ger.Hex())
        } else {
            c.logger.Warnf("%s GER transaction already exists in monitoring DB but ID not returned. GER: %s", action, ger.Hex())
        }
    } else {
        return fmt.Errorf("failed to add %s GER transaction: %w", action, err)
    }
}

However, if the contract guarantees id is always returned with ErrAlreadyExists, the current implementation is fine.

---

Summary

This PR effectively fixes a bug where duplicate GER transaction submissions would cause failures instead of continuing to monitor the existing transaction. The changes are well-tested and follow Go best practices. The suggested improvements are minor style/clarity enhancements that don't affect correctness.

Recommendation: ✅ Approve with optional minor refinements

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud