Repository navigation
feat(core): implement auto-extension rate limiter and parse resource estimates - #298
Merged
AbdulmalikAlayande merged 1 commit intoJul 4, 2026
Merged
Conversation
…estimates - Add countExtensionsInLastHour() to repositories.ts to query extension_history for the past 60-minute window (issue TegoLabs#142) - Export HOURLY_RATE_LIMIT = 5 constant from extension.ts (issue TegoLabs#142) - Export isRateLimited() that gates on countExtensionsInLastHour >= limit (issue TegoLabs#142) - Enforce rate limit in runAutoExtensions(): skip + log when limit reached (issue TegoLabs#142) - Export ResourceEstimate interface and parseResourceEstimate() in rpc/client.ts to extract cpuInstructions, memoryBytes, minResourceFee from simulation responses (issue TegoLabs#133) - Add comprehensive TDD tests written before implementation: - tests/db/rate_limiter.test.ts: countExtensionsInLastHour edge cases - tests/core/rate_limiter.test.ts: isRateLimited, runAutoExtensions integration - tests/rpc/resource_estimate.test.ts: parseResourceEstimate + failure edge cases Closes TegoLabs#133 Closes TegoLabs#137 Closes TegoLabs#142
|
@Bokky73 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughAdds a per-contract hourly rate limiter for auto-extensions: a new ChangesAuto-Extension Rate Limiter
RPC Resource Estimate Parsing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/extension.ts`:
- Around line 332-337: The rate-limit guard in the extension flow is only a
read-before-submit check, so overlapping runs can both pass it and oversubscribe
the 5/hour cap. Update the contract extension path in `src/core/extension.ts`
around `isRateLimited`, `countExtensionsInLastHour`, and the submit/record flow
so the limit is enforced atomically in the database, or serialize extension
attempts per `contract.id` before making the network call. Ensure the
reservation/transaction happens before submission and blocks concurrent
schedulers from proceeding.
In `@src/db/repositories.ts`:
- Around line 547-553: The recent-count query in the extension history
repository is comparing executed_at directly against datetime('now', '-1 hour'),
which can miscount ISO-8601 timestamps because SQLite treats them as TEXT;
update the query in the repository method that uses
db.prepare(...).get(contractId) to normalize executed_at before the comparison,
using a consistent datetime conversion on the stored value so the filter
correctly reflects the last hour.
In `@src/rpc/client.ts`:
- Around line 148-151: The safeParseNumber helper in rpc/client.ts currently
accepts Infinity and negative values via Number(), which lets malformed RPC data
slip through as valid ResourceEstimate values. Update safeParseNumber to only
return non-negative finite integers, and fall back to 0 for anything else; make
sure the ResourceEstimate parsing path that uses this helper (for CPU
instructions, memory bytes, and stroop fees) only preserves valid domain values.
In `@tests/rpc/resource_estimate.test.ts`:
- Around line 145-154: The test for parseResourceEstimate currently only
verifies that the fee-only input does not throw, so it does not assert the
intended null contract. Update the test case in resource_estimate.test.ts to
explicitly check the return value from parseResourceEstimate(sim) and assert the
expected fee-only behavior (null, if that is the contract) instead of accepting
any non-throwing result. Use the parseResourceEstimate helper and the existing
sim fixture to keep the assertion focused on this missing-cost path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 876d5626-641c-451b-9ecd-13230f78a22d
📒 Files selected for processing (6)
src/core/extension.tssrc/db/repositories.tssrc/rpc/client.tstests/core/rate_limiter.test.tstests/db/rate_limiter.test.tstests/rpc/resource_estimate.test.ts
Comment on lines
+332
to
+337
| if (isRateLimited(db, contract.id)) { | ||
| const count = countExtensionsInLastHour(db, contract.id); | ||
| const msg = `Contract ${contract.id}: rate limit reached — ${count}/${HOURLY_RATE_LIMIT} extensions in the last hour. Skipping.`; | ||
| logger.warn(msg); | ||
| result.errors.push(msg); | ||
| return; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
This rate-limit check is still bypassable across overlapping runs.
The gate is a read-before-submit check only. If two schedulers/processes hit the same contract at the same time, both can see count === 4, both pass Line 332, and both submit an extension before either call records the new extension_history row. That defeats the 5/hour safety cap in exactly the runaway scenario this PR is trying to stop.
Please move the limit enforcement into an atomic DB-backed reservation/transaction, or serialize extension attempts per contract before the network call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/extension.ts` around lines 332 - 337, The rate-limit guard in the
extension flow is only a read-before-submit check, so overlapping runs can both
pass it and oversubscribe the 5/hour cap. Update the contract extension path in
`src/core/extension.ts` around `isRateLimited`, `countExtensionsInLastHour`, and
the submit/record flow so the limit is enforced atomically in the database, or
serialize extension attempts per `contract.id` before making the network call.
Ensure the reservation/transaction happens before submission and blocks
concurrent schedulers from proceeding.
Comment on lines
+547
to
+553
| const row = db.prepare(` | ||
| SELECT COUNT(*) AS cnt | ||
| FROM extension_history | ||
| WHERE contract_id = ? | ||
| AND executed_at >= datetime('now', '-1 hour') | ||
| `).get(contractId) as { cnt: number }; | ||
| return row?.cnt ?? 0; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "CREATE TABLE\\s+extension_history|executed_at" src tests
python - <<'PY'
import sqlite3
con = sqlite3.connect(":memory:")
cur = con.cursor()
cur.execute("CREATE TABLE extension_history (executed_at TEXT)")
cur.execute("INSERT INTO extension_history VALUES (?)", ("2026-06-29T11:00:00.000Z",))
raw = cur.execute("""
SELECT COUNT(*)
FROM extension_history
WHERE executed_at >= datetime('2026-06-29T12:30:00Z', '-1 hour')
""").fetchone()[0]
normalized = cur.execute("""
SELECT COUNT(*)
FROM extension_history
WHERE datetime(executed_at) >= datetime('2026-06-29T12:30:00Z', '-1 hour')
""").fetchone()[0]
print({"raw_compare": raw, "normalized_compare": normalized})
PYRepository: AbdulmalikAlayande/sorokeep
Length of output: 3229
Normalize executed_at before comparing it to datetime('now', '-1 hour').
SQLite compares these as TEXT, so ISO-8601 values like 2026-06-29T11:00:00.000Z can still sort after the datetime() result and get counted even when they’re older than an hour.
Suggested fix
export function countExtensionsInLastHour(db: Database.Database, contractId: string): number {
const row = db.prepare(`
SELECT COUNT(*) AS cnt
FROM extension_history
WHERE contract_id = ?
- AND executed_at >= datetime('now', '-1 hour')
+ AND datetime(executed_at) >= datetime('now', '-1 hour')
`).get(contractId) as { cnt: number };
return row?.cnt ?? 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const row = db.prepare(` | |
| SELECT COUNT(*) AS cnt | |
| FROM extension_history | |
| WHERE contract_id = ? | |
| AND executed_at >= datetime('now', '-1 hour') | |
| `).get(contractId) as { cnt: number }; | |
| return row?.cnt ?? 0; | |
| export function countExtensionsInLastHour(db: Database.Database, contractId: string): number { | |
| const row = db.prepare(` | |
| SELECT COUNT(*) AS cnt | |
| FROM extension_history | |
| WHERE contract_id = ? | |
| AND datetime(executed_at) >= datetime('now', '-1 hour') | |
| `).get(contractId) as { cnt: number }; | |
| return row?.cnt ?? 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db/repositories.ts` around lines 547 - 553, The recent-count query in the
extension history repository is comparing executed_at directly against
datetime('now', '-1 hour'), which can miscount ISO-8601 timestamps because
SQLite treats them as TEXT; update the query in the repository method that uses
db.prepare(...).get(contractId) to normalize executed_at before the comparison,
using a consistent datetime conversion on the stored value so the filter
correctly reflects the last hour.
Comment on lines
+148
to
+151
| function safeParseNumber(value: unknown): number { | ||
| if (value === undefined || value === null) return 0; | ||
| const n = typeof value === "number" ? value : Number(value); | ||
| return Number.isNaN(n) ? 0 : n; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject out-of-domain numeric values here.
Number() still turns "Infinity" into Infinity and preserves negatives, so malformed RPC data is returned as a valid ResourceEstimate instead of falling back safely. For CPU instructions, memory bytes, and stroop fees, only non-negative finite integers should survive parsing.
Proposed fix
function safeParseNumber(value: unknown): number {
if (value === undefined || value === null) return 0;
const n = typeof value === "number" ? value : Number(value);
- return Number.isNaN(n) ? 0 : n;
+ return Number.isSafeInteger(n) && n >= 0 ? n : 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function safeParseNumber(value: unknown): number { | |
| if (value === undefined || value === null) return 0; | |
| const n = typeof value === "number" ? value : Number(value); | |
| return Number.isNaN(n) ? 0 : n; | |
| function safeParseNumber(value: unknown): number { | |
| if (value === undefined || value === null) return 0; | |
| const n = typeof value === "number" ? value : Number(value); | |
| return Number.isSafeInteger(n) && n >= 0 ? n : 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rpc/client.ts` around lines 148 - 151, The safeParseNumber helper in
rpc/client.ts currently accepts Infinity and negative values via Number(), which
lets malformed RPC data slip through as valid ResourceEstimate values. Update
safeParseNumber to only return non-negative finite integers, and fall back to 0
for anything else; make sure the ResourceEstimate parsing path that uses this
helper (for CPU instructions, memory bytes, and stroop fees) only preserves
valid domain values.
Comment on lines
+145
to
+154
| it("returns null when cost field is absent entirely", () => { | ||
| const sim: Record<string, unknown> = { | ||
| minResourceFee: "100", | ||
| results: [{ xdr: "AAAAAA==" }], | ||
| latestLedger: "100000", | ||
| }; | ||
| // Without cost, cpu/mem cannot be read — implementation may return null | ||
| // OR return with 0s. We accept either as long as it does not throw. | ||
| expect(() => parseResourceEstimate(sim)).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make this test assert the fee-only contract explicitly.
The title says this path returns null, but Lines 151-153 intentionally accept either null or an estimate and only check “does not throw”. That leaves fee-only responses effectively untested.
Proposed fix
- it("returns null when cost field is absent entirely", () => {
+ it("returns a zeroed CPU/memory estimate when only minResourceFee is present", () => {
const sim: Record<string, unknown> = {
minResourceFee: "100",
results: [{ xdr: "AAAAAA==" }],
latestLedger: "100000",
};
- // Without cost, cpu/mem cannot be read — implementation may return null
- // OR return with 0s. We accept either as long as it does not throw.
- expect(() => parseResourceEstimate(sim)).not.toThrow();
+ expect(parseResourceEstimate(sim)).toEqual({
+ cpuInstructions: 0,
+ memoryBytes: 0,
+ minResourceFee: 100,
+ });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("returns null when cost field is absent entirely", () => { | |
| const sim: Record<string, unknown> = { | |
| minResourceFee: "100", | |
| results: [{ xdr: "AAAAAA==" }], | |
| latestLedger: "100000", | |
| }; | |
| // Without cost, cpu/mem cannot be read — implementation may return null | |
| // OR return with 0s. We accept either as long as it does not throw. | |
| expect(() => parseResourceEstimate(sim)).not.toThrow(); | |
| }); | |
| it("returns a zeroed CPU/memory estimate when only minResourceFee is present", () => { | |
| const sim: Record<string, unknown> = { | |
| minResourceFee: "100", | |
| results: [{ xdr: "AAAAAA==" }], | |
| latestLedger: "100000", | |
| }; | |
| expect(parseResourceEstimate(sim)).toEqual({ | |
| cpuInstructions: 0, | |
| memoryBytes: 0, | |
| minResourceFee: 100, | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/rpc/resource_estimate.test.ts` around lines 145 - 154, The test for
parseResourceEstimate currently only verifies that the fee-only input does not
throw, so it does not assert the intended null contract. Update the test case in
resource_estimate.test.ts to explicitly check the return value from
parseResourceEstimate(sim) and assert the expected fee-only behavior (null, if
that is the contract) instead of accepting any non-throwing result. Use the
parseResourceEstimate helper and the existing sim fixture to keep the assertion
focused on this missing-cost path.
Collaborator
|
Here is a patch that resolves the CodeRabbit review issues around rate limiting and resource estimate tests: src/core/extension.ts | 881 +++++++++++++++------------- diff --git a/src/core/extension.ts b/src/core/extension.ts
// G��G��G�� Rate limiter G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G�� +/**
/**
// G��G��G�� Public contract G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G�� export interface ExtensionResult {
export interface AutoExtensionResult {
export interface RestoreResult {
// G��G��G�� Core implementation G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��
/**
/**
/**
// G��G��G�� Private helpers G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��G��
+export async function resolveSecretKey(
-/** Parse a value to a non-NaN number, defaulting to 0. /
export interface FeeStatsResult {
`` |
This was referenced Jul 28, 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
2 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
This PR implements two related safety features for the auto-extension system:
Issue #142 — Auto-Extension Rate Limiter
Prevents runaway loops of transaction fee submissions under extreme network load by enforcing a maximum of 5 auto-extension transactions per contract per hour.
Changes:
src/db/repositories.ts: AddedcountExtensionsInLastHour(db, contractId)— queriesextension_historyfor records within the past 60-minute window usingexecuted_at >= datetime('now', '-1 hour')src/core/extension.ts: ExportedHOURLY_RATE_LIMIT = 5constant andisRateLimited(db, contractId, limit?)functionsrc/core/extension.ts: Enforced rate limit insiderunAutoExtensions()— skips the contract and records a descriptive error when the limit is reached; each contract is checked independentlyIssue #133 — Parse Resource Limits from simulateTransaction
Extracts estimated resource usage from simulation responses to enable budget safety checks before executing auto-extensions.
Changes:
src/rpc/client.ts: AddedResourceEstimateinterface (cpuInstructions,memoryBytes,minResourceFee)src/rpc/client.ts: AddedparseResourceEstimate(response)— safely parses simulation JSON responses, defaults missing fields to0, returnsnullon error responses or invalid inputIssue #137 — Edge Case Tests for Simulation Failures
Comprehensive edge case coverage for simulation failure scenarios is included in
tests/rpc/resource_estimate.test.ts.Test-Driven Development
Tests were written before the implementation per the project's strict TDD requirements:
tests/db/rate_limiter.test.tscountExtensionsInLastHour: zero count, recent vs. old records, per-contract isolation, boundary conditionstests/core/rate_limiter.test.tsisRateLimited,HOURLY_RATE_LIMIT,runAutoExtensionsintegration: skips rate-limited contracts, per-contract independence, error message contenttests/rpc/resource_estimate.test.tsparseResourceEstimate: successful parsing, error responses, null/undefined input, missing fields default to 0, non-numeric strings, boundary valuesWhat was tested
submitExtensionis never called for a rate-limited contract ✅null— never throw ✅Closes #133
Closes #137
Closes #142
Closes #141