feat: Add ratelimit sanity check (SC-1164)

[supercontracts]

Summary by CodeRabbit

• Bug Fixes
  • Enhanced rate-limit validation with stricter runtime checks to ensure rate-limit data integrity and system reliability during transaction processing.

[coderabbitai]

Walkthrough

This PR enhances rate-limit validation in the test harness by introducing context-aware checks. A new internal helper function validates rate-limit data values (maxAmount and slope) for each rate-limit id, while the existing key verification function is updated to invoke this validation during checks.

Changes

Cohort / File(s)	Summary
Rate-limit validation enhancement src/test-harness/SparkLiquidityLayerTests.sol	Added _checkRateLimitValue() helper function to validate rate-limit data values with sanity bounds; updated _checkRateLimitKeys() signature to accept SparkLiquidityLayerContext parameter and invoke per-id value validation; updated all call sites to pass context.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• The new _checkRateLimitValue() logic requires careful verification of validation constraints (maxAmount and slope checks)
• All call sites passing the context parameter need verification to ensure completeness
• The transition from a pure function to a stateful context-aware function warrants attention

Possibly related PRs

• feat: Check Swap Rate limit on Deposit (SC-1174) #136 — Also modifies rate-limit validation in SparkLiquidityLayerTests.sol; adds test assertion for pool swap rateLimit maxAmount > 0 when deposit limits exist, complementing this PR's value validation logic.

Suggested reviewers

• lucas-manuel

Poem

🐰 A hop, a check, a value to see,
Rate-limits dance with context spree,
Sanity bounds and slopes align,
The validation logic shines so fine! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	No pull request description was provided; the template requires multiple sections including Forum Post, Dependencies, Addresses, Notes, and Conditions for deployment.	Add a comprehensive description following the template with at least Forum Post URL, Dependencies, and Notes for Reviewers sections to explain the ratelimit sanity check changes.
Linked Issues check	❓ Inconclusive	The linked issue SC-1164 provides only a title with no detailed coding requirements or objectives specified for validation.	Ensure the linked issue SC-1164 contains detailed coding requirements and acceptance criteria so compliance with specific objectives can be properly assessed.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title meets all requirements: it starts with 'feat:' prefix, is concise and descriptive, and includes the Linear issue identifier 'SC-1164' in parentheses.
Out of Scope Changes check	✅ Passed	The changes focus on adding ratelimit value validation logic to SparkLiquidityLayerTests.sol, which aligns with the ratelimit sanity check objective from SC-1164.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/sc-1164-ratelimit-sanity-check

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

src/test-harness/SparkLiquidityLayerTests.sol (3)

366-390: Context‑aware rate‑limit key check wiring looks correct

Passing ctx into _checkRateLimitKeys here keeps the existing key‑coverage semantics while enabling value sanity checks via ctx.rateLimits. Since _checkRateLimitKeys currently only uses ctx.rateLimits, reusing the same pre‑execution ctx for both calls is safe; if you later extend it to depend on the controller, consider re‑fetching ctx with isPostExecution: true for the second call to make that intent explicit.

---

2791-2818: Domain E2E runner correctly updated to pass context into key checks

The _runSLLE2ETestsForDomain flow now validates rate‑limit values for all integrations pre‑ and post‑execution using the domain’s ctx.rateLimits, which is exactly what the new helper requires. As with the Ethereum test, if _checkRateLimitKeys ever starts depending on controller state, it may be worth calling it with the post‑execution ctx you compute later in this function for clarity.

---

3602-3612: Rate‑limit magnitude sanity check is reasonable; consider named constants/comments

_checkRateLimitValue enforces that maxAmount and slope are either 0, type(uint256).max, or ≤ ~1e10 in 18‑dec units, which is a sensible upper bound to catch obvious misconfigurations (e.g., missing scaling). The logic is straightforward and side‑effect‑free.

For readability and future tuning, it would help to:

• Extract the 1e10 and 1e18 values into named constants (e.g., MAX_NORMALIZED_RATE_LIMIT and DECIMALS) or
• Add a short comment indicating this is “~10B units at 18 decimals” so the intent is clear at call sites.

Functionally, this is solid as‑is.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 68a82c8 and 229196c.

📒 Files selected for processing (1)

• src/test-harness/SparkLiquidityLayerTests.sol (6 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: test

🔇 Additional comments (1)

src/test-harness/SparkLiquidityLayerTests.sol (1)

3564-3599: New _checkRateLimitKeys behavior preserves coverage and adds value sanity

Pulling SparkLiquidityLayerContext into _checkRateLimitKeys and invoking _checkRateLimitValue for each non‑zero entryId/entryId2/exitId/exitId2 gives you per‑id sanity checking on top of the existing “every key must be covered and removed” invariant. The require on non‑empty integrations and the final assertTrue(rateLimitKeys.length == 0) keep the original safety properties intact, with _remove still guaranteeing that every integration id appears in rateLimitKeys.