feat: extend monitoring timeout configuration for settlement readiness#312
Merged
nahimterrazas merged 6 commits intomainfrom Mar 24, 2026
Merged
feat: extend monitoring timeout configuration for settlement readiness#312nahimterrazas merged 6 commits intomainfrom
nahimterrazas merged 6 commits intomainfrom
Conversation
- Updated the monitoring timeout validation to allow a maximum of 14 days (1,209,600 seconds) to accommodate optimistic-rollup broadcaster routes. - Added to various configuration structures and tests, ensuring it defaults to the new maximum when not explicitly set. - Enhanced documentation to clarify the purpose and usage of the field in configuration files.
nahimterrazas
requested review from
NicoMolinaOZ,
pepebndc and
shahnami
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExpanded solver monitoring timeout upper bound to 1,209,600 seconds (14 days); added optional Changes
Sequence Diagram(s)sequenceDiagram
participant Seed as SeedOverrides
participant Init as Initializer
participant Merger as ConfigMerger
participant Operator as OperatorConfig
participant Runtime as Runtime
rect rgba(200,220,255,0.5)
Seed->>Merger: provide monitoring_timeout_seconds (Option<u64>)
Init->>Merger: provide monitoring_timeout_seconds (Option<u64>)
end
rect rgba(200,255,200,0.5)
Merger->>Operator: merge (prefer initializer, fallback to seed, else default)
Operator->>Runtime: include OperatorSolverConfig.monitoring_timeout_seconds (u64)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/config-storage.md (1)
100-100: Consider documenting the accepted numeric bounds inline.Adding the allowed range (
30to1,209,600) in this row would help operators avoid invalid bootstrap values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/config-storage.md` at line 100, Update the table row for the `monitoring_timeout_seconds` setting to include the accepted numeric bounds (30 to 1,209,600 seconds) inline so operators know valid values; keep the default mention (`28800`) and example long-latency value (`864000`) and ensure the description still states the unit (seconds) and purpose (top-level solver monitoring timeout for post-fill settlement polling).crates/solver-config/src/lib.rs (1)
550-561: Add explicit boundary tests for the new cap.Please add/adjust tests to cover accept at
30and1_209_600, and reject at29and1_209_601.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-config/src/lib.rs` around lines 550 - 561, Add unit tests that exercise the monitoring timeout boundary checks around monitoring_timeout_seconds: create Config instances (or modify a test fixture that sets self.solver.monitoring_timeout_seconds) and call the validation routine (e.g., validate or Config::validate) to assert success for values 30 and 1_209_600 and assert ConfigError::Validation for values 29 and 1_209_601; place these tests alongside existing config validation tests and reuse the same builder/fixture used elsewhere so you only change the timeout field and assert Ok for 30 and 1_209_600 and Err for 29 and 1_209_601.crates/solver-service/src/config_merge.rs (1)
3285-3293: Add a default-path test alongside the override-path test.This new test correctly checks explicit override behavior. Please add a companion case asserting the unset default value (expected 1,209,600) so fallback behavior is regression-safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-service/src/config_merge.rs` around lines 3285 - 3293, Add a new unit test alongside test_merge_config_applies_monitoring_timeout_override that verifies the default fallback for monitoring_timeout_seconds: call test_seed_overrides() but do NOT set overrides.monitoring_timeout_seconds, call merge_config(overrides, &TESTNET_SEED).unwrap(), and assert that config.solver.monitoring_timeout_seconds == 1_209_600; place the test near test_merge_config_applies_monitoring_timeout_override and use the same helpers (test_seed_overrides, merge_config, TESTNET_SEED) and naming convention (e.g., test_merge_config_uses_default_monitoring_timeout) so the behavior is regression-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/solver-service/src/config_merge.rs`:
- Around line 415-417: The current fallback for monitoring_timeout_seconds uses
seed.defaults.monitoring_timeout_seconds which can be below the required 14-day
baseline; change the assignment so the chosen value enforces a minimum of
1_209_600 seconds (14 days). Specifically, update the logic around
initializer.monitoring_timeout_seconds /
seed.defaults.monitoring_timeout_seconds to apply a clamp/min (e.g.,
.max(1_209_600)) so the final monitoring_timeout_seconds is never less than
1_209_600, referencing the existing initializer.monitoring_timeout_seconds and
seed.defaults.monitoring_timeout_seconds symbols.
---
Nitpick comments:
In `@crates/solver-config/src/lib.rs`:
- Around line 550-561: Add unit tests that exercise the monitoring timeout
boundary checks around monitoring_timeout_seconds: create Config instances (or
modify a test fixture that sets self.solver.monitoring_timeout_seconds) and call
the validation routine (e.g., validate or Config::validate) to assert success
for values 30 and 1_209_600 and assert ConfigError::Validation for values 29 and
1_209_601; place these tests alongside existing config validation tests and
reuse the same builder/fixture used elsewhere so you only change the timeout
field and assert Ok for 30 and 1_209_600 and Err for 29 and 1_209_601.
In `@crates/solver-service/src/config_merge.rs`:
- Around line 3285-3293: Add a new unit test alongside
test_merge_config_applies_monitoring_timeout_override that verifies the default
fallback for monitoring_timeout_seconds: call test_seed_overrides() but do NOT
set overrides.monitoring_timeout_seconds, call merge_config(overrides,
&TESTNET_SEED).unwrap(), and assert that
config.solver.monitoring_timeout_seconds == 1_209_600; place the test near
test_merge_config_applies_monitoring_timeout_override and use the same helpers
(test_seed_overrides, merge_config, TESTNET_SEED) and naming convention (e.g.,
test_merge_config_uses_default_monitoring_timeout) so the behavior is
regression-safe.
In `@docs/config-storage.md`:
- Line 100: Update the table row for the `monitoring_timeout_seconds` setting to
include the accepted numeric bounds (30 to 1,209,600 seconds) inline so
operators know valid values; keep the default mention (`28800`) and example
long-latency value (`864000`) and ensure the description still states the unit
(seconds) and purpose (top-level solver monitoring timeout for post-fill
settlement polling).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c5bc45d-9a4d-4762-8d30-e9e684c16a87
📒 Files selected for processing (6)
crates/solver-config/src/lib.rscrates/solver-demo/src/operations/init/mod.rscrates/solver-service/src/config_merge.rscrates/solver-types/src/seed_overrides.rsdocs/config-storage.mddocs/oracles/settlement-timing-configuration.md
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
pepebndc
approved these changes
Mar 18, 2026
shahnami
approved these changes
Mar 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing Process
Checklist
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests
Documentation