closes "B-046 — Limits config hardcoded#217
Closed
dubemoyibe-star wants to merge 0 commit into
Closed
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 35 minutes and 34 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR introduces runtime-configurable limits with database persistence and TTL caching, adds database constraints for Stellar address validation, enhances permission type validation, and implements new API endpoints to fetch and update limit configurations per audience (retail, business, government). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Limits API
participant Cache as TTL Cache
participant DB as Database
rect rgba(100, 150, 200, 0.5)
Note over Client,DB: GET /limits/:audience - Fetch Configuration
Client->>API: GET /limits/retail
API->>Cache: Check cache for 'retail'
alt Cache Hit
Cache-->>API: Return cached config
else Cache Miss
API->>DB: Query limits for 'retail'
DB-->>API: Return limit values
API->>Cache: Store in cache (TTL: 5min)
end
API-->>Client: { audience, depositDailyUsd, ... }
end
rect rgba(200, 150, 100, 0.5)
Note over Client,DB: PATCH /limits/:audience - Update Configuration
Client->>API: PATCH /limits/retail<br/>{ depositDailyUsd: 5000 }
API->>API: Validate numeric values
alt Validation Fails
API-->>Client: 400 AppError
else Validation Passes
API->>DB: Update/Upsert limits record
DB-->>API: Confirm update
API->>Cache: Invalidate cache for 'retail'
API-->>Client: { audience, depositDailyUsd: 5000, ... }
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@dubemoyibe-star 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! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/middleware/auth.test.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorUse a static import for
PERMISSION_STRINGS.The new
require("./auth")violates@typescript-eslint/no-var-requires; import it with the other tested exports.Proposed fix
-import { validateApiKey, generateApiKey, hashApiKey } from "./auth"; +import { validateApiKey, generateApiKey, hashApiKey, PERMISSION_STRINGS } from "./auth"; ... - const { PERMISSION_STRINGS } = require("./auth"); const key = await generateApiKey("user-42", [...PERMISSION_STRINGS]);Also applies to: 211-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/auth.test.ts` at line 1, Replace the dynamic require with a static ES import: import PERMISSION_STRINGS alongside the other tested exports (validateApiKey, generateApiKey, hashApiKey) at the top of auth.test.ts; locate the require("./auth") usage and change it to a named import for PERMISSION_STRINGS so the file uses only static imports and complies with `@typescript-eslint/no-var-requires`.
🧹 Nitpick comments (3)
src/controllers/limitsController.ts (1)
5-11: Unusedprismaimport and unusedNextFunction.
prismaandNextFunctionare imported but never referenced in this file. Drop them (or, if you adopt the async-error fix above,NextFunctionwill be needed again).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controllers/limitsController.ts` around lines 5 - 11, The imports include unused symbols: remove the unused prisma import and the unused NextFunction import from the limits controller import list (symbols "prisma" and "NextFunction") to eliminate dead imports; if you later implement the async-error middleware change that requires a typed next parameter, re-add NextFunction to the import and to the relevant route handler signatures (e.g., the handlers in this file that call getLimitConfig/updateLimit).src/services/wallet/walletService.ts (1)
52-58: Redundant guard —assertValidStellarAddressalready checks theGprefix.
assertValidStellarAddress(persrc/utils/stellar.ts:28-34) delegates toisValidStellarAddress, which already enforcesstartsWith("G")and full Ed25519 checksum validation. The explicitstartsWith("G")check on line 56 is unreachable if line 53 passes. Remove it to keep a single source of truth.🧹 Proposed cleanup
// Guard: ensure generated address is valid before persisting assertValidStellarAddress(publicKey); - - // ensure address starts with G (Stellar public key prefix) - if (!publicKey.startsWith("G")) { - throw new Error(`Generated keypair has invalid prefix: ${publicKey.slice(0, 1)}`); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/wallet/walletService.ts` around lines 52 - 58, Remove the redundant explicit prefix check in walletService.ts: the assertValidStellarAddress(publicKey) call already delegates to isValidStellarAddress which verifies the "G" prefix and checksum, so delete the if (!publicKey.startsWith("G")) { throw new Error(...) } block; keep the assertValidStellarAddress(publicKey) guard as the single source of truth for Stellar public-key validation and ensure no other code relies on the removed manual check.prisma/schema.prisma (1)
497-499: Redundant index —@@unique([audience])already creates one.
@@unique([audience])provisions a unique B-tree index onaudience, so the adjacent@@index([audience], map: "idx_limit_audience")is duplicative and just adds write overhead. Drop the@@indexline (or drop@@uniqueand keepaudienceas@uniqueinline).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prisma/schema.prisma` around lines 497 - 499, The schema defines both @@unique([audience]) and @@index([audience], map: "idx_limit_audience"), which is redundant because @@unique already creates an index; remove the duplicate @@index([audience], map: "idx_limit_audience") line (or alternatively replace the table-level @@unique([audience]) with an inline field-level audience `@unique` if you prefer that style) so only a single unique index is created for audience.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@prisma/migrations/20260423000000_add_stellar_address_check/migration.sql`:
- Around line 17-27: The putWalletAddress handler in userController.ts currently
only checks length/prefix in the Zod schema and must also verify Stellar
checksum: update the Zod schema (the schema declared around line 195) to add
.refine((s) => isValidStellarAddress(s), "Invalid Stellar address (bad
checksum)"), or alternatively call assertValidStellarAddress(from
src/utils/stellar.ts) immediately before calling prisma.user.update() (the
update at ~line 211); also add a unit/integration test that submits a
regex-valid but checksum-corrupted G... address to the putWalletAddress endpoint
and asserts the request is rejected (400) to prevent bad-checksum addresses from
being persisted.
In `@prisma/migrations/20260423120000_init_limits/migration.sql`:
- Around line 2-13: The migration for the "limits" table is missing the
created_at column required by the Prisma Limit model; update the CREATE TABLE
statement (or add a new migration) to add a "created_at" TIMESTAMP WITH TIME
ZONE NOT NULL DEFAULT NOW() column so Prisma can read/write createdAt; reference
the "limits" table and the Prisma model Limit and ensure the column name matches
Prisma's mapping (created_at) and has NOT NULL with DEFAULT NOW() to avoid
breaking existing inserts/reads.
- Around line 4-10: Add DB-level CHECK constraints in the migration so the
"audience" column only accepts the supported values and all limit/percentage
columns cannot be negative (and percent columns constrained to a valid 0–100
range if applicable). Specifically, update the CREATE TABLE block that contains
"audience", "deposit_daily_usd", "deposit_monthly_usd",
"withdrawal_single_currency_daily_usd",
"withdrawal_single_currency_monthly_usd",
"circuit_breaker_reserve_weight_threshold_pct", and
"circuit_breaker_min_reserve_ratio" to include named CHECK constraints: one on
"audience" limiting it to the allowed enum/set of strings, checks on each
deposit/withdrawal integer column ensuring >= 0, and checks on the two DECIMAL
pct columns ensuring >= 0 (and <= 100 if that reflects valid percent semantics);
give each constraint a descriptive name so it’s easy to locate and reference.
In `@prisma/schema.prisma`:
- Around line 485-500: The schema declares model Limit fields depositDailyUsd,
depositMonthlyUsd, withdrawalSingleCurrencyDailyUsd,
withdrawalSingleCurrencyMonthlyUsd as Decimal and createdAt as non-optional with
`@default`(now()), but the limits table migration created those four fields as
INTEGER and omitted created_at; update the migration that creates the limits
table to change those four columns to DECIMAL(20,2) (matching Decimal
`@db.Decimal`(20, 2)) and add created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT
NOW() so the runtime types and defaults match the Prisma model (ensure you
update the same migration that defines the limits table and reapply or
regenerate migrations/client as needed).
In `@SOLUTION_SUMMARY.md`:
- Around line 57-62: The "Audit trail" benefit incorrectly claims an audit trail
based solely on the `updatedAt` timestamp; update the wording in the Benefits
section (where `updatedAt` is mentioned) to accurately reflect that only the
latest update time is stored, or implement full audit logging for limit changes
(e.g., store change history with who/what/previous values) and then keep the
original wording; reference the `updatedAt` symbol when changing the copy so it
no longer implies a full audit trail unless you add real audit data.
In `@src/config/limits.ts`:
- Around line 24-81: fetchLimitsFromDb currently calls prisma.limit.findMany()
without error handling, so a DB outage will throw and break hot-path limit
checks; wrap the DB call in a try/catch inside fetchLimitsFromDb (catch around
the prisma.limit.findMany() and the subsequent loop), import and use the
project's logger (e.g., logger) to emit a structured error with context, and on
any failure return the default config-based limits (the existing initial result
object built from config.limits) so callers get env-only defaults instead of
exceptions; ensure the catch returns that result and does not rethrow.
- Around line 98-175: updateLimit currently returns the raw Prisma row (dbLimit)
and never uses the assembled result object, causing wrong types and Decimal/DB
fields to leak; change updateLimit to return a sanitized LimitConfig object (not
the Prisma Limit) by merging dbLimit into the default config values (the local
result logic) and converting Decimal fields to numbers, then return that
LimitConfig; extract the default-building logic into a reusable helper (e.g.,
buildDefaultConfig or reuse fetchLimitsFromDb) and use prisma.limit.upsert to
persist, invalidate cache (limitsCache/cacheTimestamp) as before, but return the
merged, number-only LimitConfig instead of dbLimit so callers (e.g., the
controller) get the correct shape without id/audience/timestamps or Decimal
instances.
- Around line 70-75: The numeric defaults use the pattern Number(x) || fallback
which treats legitimate 0 values as missing; replace each occurrence (e.g., when
building the limits object fields depositDailyUsd, depositMonthlyUsd,
withdrawalSingleCurrencyDailyUsd, withdrawalSingleCurrencyMonthlyUsd,
circuitBreakerReserveWeightThresholdPct, circuitBreakerMinReserveRatio) with a
strict check that uses Number.isFinite(Number(value)) ? Number(value) : fallback
so 0 is preserved; apply the same replacement in the updateLimit merge logic
(the updateLimit function/merge block handling those same fields) to ensure
explicit zeros are not overwritten by the environment defaults.
- Around line 83-96: getLimitConfig was made async but callers in
checkDepositLimits and checkWithdrawalLimits are not awaiting it (so config is a
Promise and limits are bypassed); change both call sites in
src/services/limits/limitsService.ts to use const config = await
getLimitConfig(audience);; additionally fix updateLimit to actually return a
LimitConfig-shaped object (use the constructed result instead of returning
dbLimit and update the Promise<LimitConfig> signature accordingly), wrap
fetchLimitsFromDb in try/catch to fall back or rethrow a clearer error instead
of crashing, and replace Number(limit.xxx) || fallback patterns in
fetchLimitsFromDb and updateLimit with proper checks that preserve 0 (e.g., use
limit.xxx !== null && limit.xxx !== undefined ? Number(limit.xxx) : fallback or
the nullish coalescing operator).
In `@src/controllers/limitsController.ts`:
- Around line 22-30: The GET handler for router.get("/:audience") doesn't
validate the audience param before calling getLimitConfig, which can return
undefined entries; add the same audience validation used in the PATCH handler
(or reuse its helper/middleware) to ensure the param is one of the allowed
Audience values before proceeding; if invalid, return a 400/response matching
PATCH behavior. Locate the router.get("/:audience") handler and either call the
existing isValidAudience/validation helper used by PATCH or extract that logic
into a shared middleware and apply it so getLimitConfig is only called with a
validated Audience.
- Around line 11-56: The limits endpoints are missing authentication and
authorization; protect the router by applying validateApiKey (e.g.,
router.use(validateApiKey)) and restrict the PATCH handler with a strict
permission guard such as requireSegmentScope("enterprise:admin") or a dedicated
"limits:admin" scope; update the PATCH route that calls updateLimit to require
this scope and record an AuditTrail entry (including actor identity from the
validated request) whenever updateLimit is invoked; ensure GET (which calls
getLimitConfig) remains protected by validateApiKey as well so only
authenticated/authorized callers can read configs.
- Around line 46-52: The current numeric guard in the loop over partial only
filters numbers but allows non-number types to proceed, which can lead to
Prisma/Decimal coercion or 500s; update the validation in the handler that
builds partial (or inside updateLimit) to explicitly reject non-number values
for numeric fields (e.g., if a key is expected numeric and typeof val !==
"number" || !Number.isFinite(val) then throw AppError) or replace the ad-hoc
loop with a Zod schema (matching the approach used in auth.ts) to parse/validate
the request body before calling updateLimit and prisma.limit.upsert so only
valid finite numbers are passed through.
- Around line 26-56: The async route handlers (router.get and router.patch)
currently throw errors directly (e.g., new AppError(...) and rejections from
getLimitConfig/updateLimit) which Express 4 won't catch; wrap the body of both
handlers in try-catch blocks (or use a generic async wrapper) and call next(err)
in the catch so errors are forwarded to the error middleware, ensuring AppError
and other rejections are handled and that responses/error status are sent
correctly.
In `@src/middleware/auth.ts`:
- Around line 12-41: PERMISSION_STRINGS currently omits many ":admin" variants
which means validatePermissions will silently drop unsupported admin scopes;
please confirm this is intentional and add explicit documentation and in-repo
guidance: update the comment above PERMISSION_STRINGS to list which segments
intentionally support ":admin" (p2p, sme, enterprise, gateway), describe the
fail-safe behavior of validatePermissions, and include a rollout/migration plan
that shows how to detect legacy API keys containing unsupported ":admin" scopes
and how to remediate them (e.g., migration script or revocation + re-issue).
Also add a short note in the validatePermissions implementation referencing
PERMISSION_STRINGS so reviewers can trace behavior.
In `@src/middleware/segmentGuard.ts`:
- Around line 17-18: The current re-export uses a value export for Permission
which is a type-only alias (imported as a type-only symbol), causing a TS
compilation error; change the exports to re-export Permission as a type-only
export and export PermissionSchema as a value export—i.e., replace the combined
value export of Permission with a type-only re-export for Permission (export
type { Permission }) while keeping PermissionSchema in the normal export (export
{ PermissionSchema }) so the symbols match their imported kinds.
In `@src/routes/limitsRoutes.ts`:
- Around line 14-28: Protect the PATCH route by adding an
authentication/authorization middleware (e.g., validateApiKey or isAdmin) to the
router.patch("/:audience") handler, perform runtime validation of
req.params.audience and req.body (match allowed audience values
"retail"|"business"|"government" and validate LimitConfig fields) and throw
AppError on invalid input, and ensure async errors are forwarded to Express
error handling by either wrapping the route body in try/catch and calling
next(err) on failure or by importing/using express-async-errors; update the
router.get handler similarly to validate audience and catch/forward async errors
from getLimitConfig.
In `@src/services/wallet/walletService.ts`:
- Around line 31-46: The code currently clears both stellarAddress and
encryptedStellarSecret when isValidStellarAddress(user.stellarAddress) fails;
instead, before wiping the secret (encryptedStellarSecret) use the stored secret
to derive/validate the public key and only clear or repair when the secret is
truly invalid or mismatched: call your secret-decryption/derivation routine (the
function you use to decrypt/derive a pubkey from encryptedStellarSecret),
compare the derived pubkey to stellarAddress (or restore stellarAddress from the
derived pubkey if it is valid), and only call prisma.user.update to null fields
if both secret and derived pubkey are invalid; if you must mutate, emit an audit
log/event (include userId, truncated addresses, and reason) and throw or return
an explicit error rather than silently continuing; also consider performing
wallet generation in the same request path instead of returning {
wallet_created: false } so callers don’t need a second call (touchpoints:
isValidStellarAddress, encryptedStellarSecret, stellarAddress,
prisma.user.update, and the code path that returns { wallet_created: false }).
In `@tests/stellar.test.ts`:
- Around line 62-83: The test suite currently calls Prisma in
beforeEach/afterAll unconditionally; add a beforeAll that attempts
prisma.$connect() in a try/catch and sets a boolean (e.g., dbAvailable) so hooks
can early-return when DB is unavailable, or alternatively make the whole suite
integration-only by wrapping the describe in a runtime check of an INTEGRATION
env var and using describe.skip when not set; update beforeEach and afterAll to
guard calls to prisma.user.deleteMany and prisma.$disconnect with if
(!dbAvailable) return; reference describe("Database-level Stellar address
constraint"), beforeAll, beforeEach, afterAll, prisma.$connect,
prisma.user.deleteMany, and prisma.$disconnect.
- Around line 85-146: Run Prettier on the updated tests in tests/stellar.test.ts
to remove trailing spaces and add missing trailing commas so the lint job
passes; specifically, format the test block that defines invalidAddress,
validAddress, the prisma.user.create calls, and the expect assertions
(references: variables invalidAddress, validAddress, kp.publicKey(),
prisma.user.create, isValidStellarAddress, testUser) to comply with the
project's Prettier rules and ESLint formatting expectations.
---
Outside diff comments:
In `@src/middleware/auth.test.ts`:
- Line 1: Replace the dynamic require with a static ES import: import
PERMISSION_STRINGS alongside the other tested exports (validateApiKey,
generateApiKey, hashApiKey) at the top of auth.test.ts; locate the
require("./auth") usage and change it to a named import for PERMISSION_STRINGS
so the file uses only static imports and complies with
`@typescript-eslint/no-var-requires`.
---
Nitpick comments:
In `@prisma/schema.prisma`:
- Around line 497-499: The schema defines both @@unique([audience]) and
@@index([audience], map: "idx_limit_audience"), which is redundant because
@@unique already creates an index; remove the duplicate @@index([audience], map:
"idx_limit_audience") line (or alternatively replace the table-level
@@unique([audience]) with an inline field-level audience `@unique` if you prefer
that style) so only a single unique index is created for audience.
In `@src/controllers/limitsController.ts`:
- Around line 5-11: The imports include unused symbols: remove the unused prisma
import and the unused NextFunction import from the limits controller import list
(symbols "prisma" and "NextFunction") to eliminate dead imports; if you later
implement the async-error middleware change that requires a typed next
parameter, re-add NextFunction to the import and to the relevant route handler
signatures (e.g., the handlers in this file that call
getLimitConfig/updateLimit).
In `@src/services/wallet/walletService.ts`:
- Around line 52-58: Remove the redundant explicit prefix check in
walletService.ts: the assertValidStellarAddress(publicKey) call already
delegates to isValidStellarAddress which verifies the "G" prefix and checksum,
so delete the if (!publicKey.startsWith("G")) { throw new Error(...) } block;
keep the assertValidStellarAddress(publicKey) guard as the single source of
truth for Stellar public-key validation and ensure no other code relies on the
removed manual check.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7157e3b3-7cdb-4359-9981-a92022632bea
📒 Files selected for processing (14)
SOLUTION_SUMMARY.mdprisma/migrations/20260423000000_add_stellar_address_check/migration.sqlprisma/migrations/20260423120000_init_limits/migration.sqlprisma/schema.prismasrc/config/limits.tssrc/controllers/limitsController.tssrc/middleware/auth.test.tssrc/middleware/auth.tssrc/middleware/segmentGuard.tssrc/routes/index.tssrc/routes/limitsRoutes.tssrc/services/auth/authService.tssrc/services/wallet/walletService.tstests/stellar.test.ts
dubemoyibe-star
force-pushed
the
feature-branch-name
branch
from
8f763e2 to
96df229
Compare
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
1 participant
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.
Fixes #161" "B-048 — Permissions typed as loose strings Fixes #163" "B-013 — Placeholder / invalid Stellar address on signup Fixes #128"
Summary by CodeRabbit
New Features
Bug Fixes
Documentation