fix: cherry-pick #1471 Fix typo 'DelayBeetweenCertificates'

[joanestebanr]

Cherry-pick from develop #1471

🔄 Changes Summary

• Fix typo: AggSender.TriggerASAP.DelayBeetweenCertificates

⚠️ Breaking Changes

• Deprecated field AggSender.TriggerASAP.DelayBeetweenCertificatesin favour ofDelayBetweenCertificates`
  DelayBetweenCertificates = "1s"

[AggSender.TriggerASAP]

📋 Config Updates

• 🧾 Diff/Config snippet: [Optional: Enumerate config changes/provide snippet of changes]

🔗 Related PRs

• fix: typo #1470

[claude]

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

---

Code Review Complete ✅

---

🚀 What's New

Fixed typo in configuration field name from DelayBeetweenCertificates to DelayBetweenCertificates (corrected spelling of "Between"). Added comprehensive unit tests for TriggerASAPConfig covering default initialization, string representation, and validation logic.

🐛 Bug Fixes

None - this is a typo correction in field naming.

📋 Config Updates

Breaking Change - Configuration field renamed:

• Old (deprecated): DelayBeetweenCertificates
• New: DelayBetweenCertificates
• Default value: "1s"
• Location: [AggSender.TriggerASAP] section
• Example:

[AggSender.TriggerASAP]
    DelayBetweenCertificates = "1s"
    MinimumNewCertificateInterval = "5m"
    OnNewL2Bridge = false

⚠️ Breaking Changes

🔴 Required: Configuration file update needed

The field AggSender.TriggerASAP.DelayBeetweenCertificates has been renamed to DelayBetweenCertificates (correct spelling).

Impact: Users must update their configuration files to use the new field name. The old field name is no longer recognized and will be ignored by the config parser due to the mapstructure tag change.

Migration: Replace DelayBeetweenCertificates with DelayBetweenCertificates in all TOML configuration files.

---

Review Findings

✅ Positive Aspects

1. Comprehensive test coverage - The PR adds excellent unit tests (134 lines) covering:

   • Default configuration initialization (aggsender/config/config_test.go:170-179)
   • String representation with multiple scenarios (aggsender/config/config_test.go:181-226)
   • Validation logic including edge cases (aggsender/config/config_test.go:228-302)
   • All tests follow Go best practices with t.Parallel() and require assertions per CLAUDE.md guidelines

2. Consistent changes across codebase - All 6 files updated consistently:

   • Struct definition and mapstructure tag (aggsender/config/config.go:22)
   • Default initialization (aggsender/config/config.go:31)
   • String() method (aggsender/config/config.go:37-38)
   • Validate() method (aggsender/config/config.go:44-45)
   • Usage in trigger logic (aggsender/trigger/trigger_asap.go:147, 158)
   • Test assertions (aggsender/trigger/trigger_asap_test.go:78)
   • Documentation (docs/aggsender.md:230, 238, 242)
   • Default config template (config/default.go:223)

3. Proper documentation updates - Documentation correctly reflects the new field name

🟡 Suggestions

1. No backward compatibility handling - The PR completely removes the old field name without providing a migration path. Consider adding deprecation handling:

   In aggsender/config/config.go, the struct could temporarily support both fields:

   type TriggerASAPConfig struct {
       DelayBetweenCertificates types.Duration `mapstructure:"DelayBetweenCertificates"`
       // Deprecated: Use DelayBetweenCertificates instead
       DelayBeetweenCertificates types.Duration `mapstructure:"DelayBeetweenCertificates"`
       // ...
   }

   Then add initialization logic to prefer the new field but fall back to old if present, with a warning log.

   However, since this is a cherry-pick to a release branch (release/0.9) and the original PR fix: typo AggSender.TriggerASAP.DelayBeetweenCertificates #1471 was already merged to develop, this approach may not be desired for the release. The team should decide if this breaking change is acceptable for release/0.9.

2. Validation error message inconsistency (aggsender/config/config.go:48):

   return fmt.Errorf("MinimumNewCertificateInterval must be >= 0")

   The validation checks <= 0 but the error says >= 0. The logic appears incorrect - should allow 0 (as mentioned in docs "0 = no minimum interval"). The condition should likely be:

   if c.MinimumNewCertificateInterval.Duration < 0 {
       return fmt.Errorf("MinimumNewCertificateInterval cannot be negative")
   }

   Note: This bug exists in the base code and is NOT introduced by this PR, but since you're adding tests that cover this validation, it would be a good opportunity to fix it.

3. Test coverage for migration scenario - If backward compatibility is added, tests should verify that:

   • New field takes precedence when both are present
   • Warning is logged when old field is used
   • Configuration works with only the old field (during migration period)

🟢 Optional Improvements

1. Comment formatting - The comment on line 21 could be more concise per Go conventions:

   // DelayBetweenCertificates is the delay after a certificate settles before sending the next one

2. Default config comment - Consider adding a deprecation notice in config/default.go if users might reference this file.

🔍 No Issues Found

• ✅ No security concerns
• ✅ No performance impacts
• ✅ No race conditions introduced
• ✅ Proper Go formatting and naming conventions followed
• ✅ No stuttering in naming (per CLAUDE.md guidelines)
• ✅ Error handling is appropriate

Summary

This is a well-executed typo fix with excellent test coverage. The main consideration is the breaking change without backward compatibility, which may impact users upgrading to this version. The validation error message issue (point 2 in suggestions) is a pre-existing bug that could be addressed as a follow-up.

Recommendation: ✅ Approve with the understanding that this is a breaking change requiring configuration updates from users. Consider documenting the migration requirement in release notes.

---

CI Status

• ✅ Lint: Passed
• ✅ Go Vulnerability Check: Passed
• ✅ PR Semantic Check: Passed
• ⏳ Test Unit and SonarCloud: In Progress
• ⏳ CodeQL: In Progress
• ⏳ Bats E2E Tests: In Progress

[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