fix: sensitive architectural details #477#560
Merged
Junman140 merged 3 commits intoJun 27, 2026
Conversation
|
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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe PR rewrites currency-fix documentation, re-emits rates-related source and test files without behavior changes, reformats treasury docs and a migration, updates ChangesCurrency fix docs and source refresh
Treasury docs and repository cleanup
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
|
@wolfyres 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: 3
🤖 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 `@prisma/migrations/20260426000000_optimize_treasury_queries/migration.sql`:
- Around line 27-42: Update the migration comment to match the actual OracleRate
indexes defined in schema.prisma: remove the incorrect reference to
oracle_rates.idx_oracle_rates_currency and note that only
idx_oracle_rates_timestamp exists before this change. Keep the analysis accurate
by stating that idx_oracle_rates_currency_timestamp_desc is the new composite
index on OracleRate for currency and timestamp-based lookups.
In `@src/services/rates/currencyConverter.ts`:
- Around line 68-125: The convertLocalToUsd function currently allows negative
localAmount values, which can produce invalid negative USD results and affect
downstream limit checks. Add a non-negative validation at the start of
convertLocalToUsd, alongside the existing currency support checks, and throw an
AppError for any amount below zero before any rate lookups or Decimal conversion
occur.
- Around line 80-82: The `getLatestAcbuRate` call in `currencyConverter` is
swallowing the original failure by converting every error into null, which
removes useful context from logs and monitoring. Update the rate-fetching flow
in the conversion functions to catch the error explicitly, preserve the thrown
error details, and log or rethrow them with enough context to distinguish
timeout, connection, and missing-data cases. Use the existing
`getLatestAcbuRate` and the surrounding conversion helper(s) as the integration
points, and avoid the blanket `.catch(() => null)` pattern.
🪄 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: 11fe7e88-fda7-4c1c-9fc4-f94c3719d51f
📒 Files selected for processing (10)
.gitignoreCURRENCY_FIX_IMPLEMENTATION.mdCURRENCY_FIX_QUICK_REFERENCE.mdISSUES.mddocs/treasury.mdprisma/migrations/20260426000000_optimize_treasury_queries/migration.sqlsrc/controllers/enterpriseController.test.tssrc/controllers/mintController.test.tssrc/services/rates/currencyConverter.tssrc/services/rates/index.ts
💤 Files with no reviewable changes (1)
- ISSUES.md
Comment on lines
+27
to
+42
| -- ANALYSIS: | ||
| -- Current indexes already support primary query patterns: | ||
| -- 1. reserves.idx_reserves_currency_segment_timestamp(sort: Desc) - optimal for DISTINCT + ORDER BY | ||
| -- 2. oracle_rates.idx_oracle_rates_currency + idx_oracle_rates_timestamp | ||
| -- 3. transactions.idx_transactions_user_type already covers core lookups | ||
|
|
||
| -- New indexes improve: | ||
| -- 1. idx_oracle_rates_currency_timestamp_desc - FX fallback queries (7-day lookback) | ||
| -- 2. idx_transactions_type_currency - transaction aggregation by currency | ||
| -- 3. idx_reserves_segment_timestamp - reserve health by segment | ||
|
|
||
| -- Query Performance Targets (after indexes): | ||
| -- - getLatestReservesBySegment(): ~50ms for 10 currencies | ||
| -- - getFxRateWithFallback(): ~10ms per currency | ||
| -- - aggregateTransactionsBySegment(): ~100ms for 1M transactions | ||
| -- - Full getEnterpriseTreasury(): ~500ms for 10 currencies, 1M transactions |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct inaccurate index reference in comment.
Line 30 claims oracle_rates.idx_oracle_rates_currency exists, but schema.prisma only defines idx_oracle_rates_timestamp on OracleRate. There is no single-column currency index. The new idx_oracle_rates_currency_timestamp_desc is the first composite index on (currency, timestamp).
-- 1. reserves.idx_reserves_currency_segment_timestamp(sort: Desc) - optimal for DISTINCT + ORDER BY
--- 2. oracle_rates.idx_oracle_rates_currency + idx_oracle_rates_timestamp
+-- 2. oracle_rates.idx_oracle_rates_timestamp (existing single-column)
-- 3. transactions.idx_transactions_user_type already covers core lookups📝 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
| -- ANALYSIS: | |
| -- Current indexes already support primary query patterns: | |
| -- 1. reserves.idx_reserves_currency_segment_timestamp(sort: Desc) - optimal for DISTINCT + ORDER BY | |
| -- 2. oracle_rates.idx_oracle_rates_currency + idx_oracle_rates_timestamp | |
| -- 3. transactions.idx_transactions_user_type already covers core lookups | |
| -- New indexes improve: | |
| -- 1. idx_oracle_rates_currency_timestamp_desc - FX fallback queries (7-day lookback) | |
| -- 2. idx_transactions_type_currency - transaction aggregation by currency | |
| -- 3. idx_reserves_segment_timestamp - reserve health by segment | |
| -- Query Performance Targets (after indexes): | |
| -- - getLatestReservesBySegment(): ~50ms for 10 currencies | |
| -- - getFxRateWithFallback(): ~10ms per currency | |
| -- - aggregateTransactionsBySegment(): ~100ms for 1M transactions | |
| -- - Full getEnterpriseTreasury(): ~500ms for 10 currencies, 1M transactions | |
| -- ANALYSIS: | |
| -- Current indexes already support primary query patterns: | |
| -- 1. reserves.idx_reserves_currency_segment_timestamp(sort: Desc) - optimal for DISTINCT + ORDER BY | |
| -- 2. oracle_rates.idx_oracle_rates_timestamp (existing single-column) | |
| -- 3. transactions.idx_transactions_user_type already covers core lookups | |
| -- New indexes improve: | |
| -- 1. idx_oracle_rates_currency_timestamp_desc - FX fallback queries (7-day lookback) | |
| -- 2. idx_transactions_type_currency - transaction aggregation by currency | |
| -- 3. idx_reserves_segment_timestamp - reserve health by segment | |
| -- Query Performance Targets (after indexes): | |
| -- - getLatestReservesBySegment(): ~50ms for 10 currencies | |
| -- - getFxRateWithFallback(): ~10ms per currency | |
| -- - aggregateTransactionsBySegment(): ~100ms for 1M transactions | |
| -- - Full getEnterpriseTreasury(): ~500ms for 10 currencies, 1M transactions |
🤖 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 `@prisma/migrations/20260426000000_optimize_treasury_queries/migration.sql`
around lines 27 - 42, Update the migration comment to match the actual
OracleRate indexes defined in schema.prisma: remove the incorrect reference to
oracle_rates.idx_oracle_rates_currency and note that only
idx_oracle_rates_timestamp exists before this change. Keep the analysis accurate
by stating that idx_oracle_rates_currency_timestamp_desc is the new composite
index on OracleRate for currency and timestamp-based lookups.
Comment on lines
+68
to
+125
| export async function convertLocalToUsd( | ||
| localAmount: number, | ||
| currency: string, | ||
| ): Promise<number> { | ||
| // Validate currency is supported | ||
| if (!CURRENCY_TO_RATE_FIELD[currency]) { | ||
| throw new AppError( | ||
| `Currency ${currency} not supported for conversion. Please check the supported currency list.`, | ||
| 400, | ||
| ); | ||
| } | ||
|
|
||
| // Fetch the latest exchange rates | ||
| const latestRate = await getLatestAcbuRate().catch(() => null); | ||
|
|
||
| if (!latestRate) { | ||
| throw new AppError( | ||
| "Exchange rates not yet available. Please try again in a moment.", | ||
| 503, | ||
| ); | ||
| } | ||
|
|
||
| // Get the rate field name for this currency | ||
| const rateFieldName = CURRENCY_TO_RATE_FIELD[currency]; | ||
|
|
||
| // Retrieve the local-to-ACBU rate (how many units of local currency per 1 ACBU) | ||
| const localToAcbuRate = latestRate[rateFieldName as keyof typeof latestRate]; | ||
|
|
||
| if (!localToAcbuRate || localToAcbuRate.toNumber() <= 0) { | ||
| throw new AppError( | ||
| `Exchange rate for ${currency} is not available or invalid. Cannot process deposit at this time.`, | ||
| 503, | ||
| ); | ||
| } | ||
|
|
||
| // Convert using high-precision Decimal arithmetic | ||
| const localAmountDecimal = new Decimal(localAmount); | ||
| const rateDecimal = new Decimal(localToAcbuRate); | ||
|
|
||
| // Calculate ACBU equivalent | ||
| const acbuAmount = localAmountDecimal.div(rateDecimal); | ||
|
|
||
| // Get USD rate per ACBU | ||
| const acbuUsdRate = new Decimal(latestRate.acbuUsd); | ||
|
|
||
| if (acbuUsdRate.toNumber() <= 0) { | ||
| throw new AppError( | ||
| "USD conversion rate is invalid. Cannot process deposit at this time.", | ||
| 503, | ||
| ); | ||
| } | ||
|
|
||
| // Convert ACBU to USD | ||
| const usdAmount = acbuAmount.mul(acbuUsdRate); | ||
|
|
||
| // Return as number with precision | ||
| return usdAmount.toNumber(); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate that localAmount is non-negative.
convertLocalToUsd does not guard against negative localAmount. A negative value would produce negative USD, potentially reducing limit totals or bypassing checks.
export async function convertLocalToUsd(
localAmount: number,
currency: string,
): Promise<number> {
+ if (localAmount < 0) {
+ throw new AppError("Amount cannot be negative", 400);
+ }
// Validate currency is supported
if (!CURRENCY_TO_RATE_FIELD[currency]) {📝 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
| export async function convertLocalToUsd( | |
| localAmount: number, | |
| currency: string, | |
| ): Promise<number> { | |
| // Validate currency is supported | |
| if (!CURRENCY_TO_RATE_FIELD[currency]) { | |
| throw new AppError( | |
| `Currency ${currency} not supported for conversion. Please check the supported currency list.`, | |
| 400, | |
| ); | |
| } | |
| // Fetch the latest exchange rates | |
| const latestRate = await getLatestAcbuRate().catch(() => null); | |
| if (!latestRate) { | |
| throw new AppError( | |
| "Exchange rates not yet available. Please try again in a moment.", | |
| 503, | |
| ); | |
| } | |
| // Get the rate field name for this currency | |
| const rateFieldName = CURRENCY_TO_RATE_FIELD[currency]; | |
| // Retrieve the local-to-ACBU rate (how many units of local currency per 1 ACBU) | |
| const localToAcbuRate = latestRate[rateFieldName as keyof typeof latestRate]; | |
| if (!localToAcbuRate || localToAcbuRate.toNumber() <= 0) { | |
| throw new AppError( | |
| `Exchange rate for ${currency} is not available or invalid. Cannot process deposit at this time.`, | |
| 503, | |
| ); | |
| } | |
| // Convert using high-precision Decimal arithmetic | |
| const localAmountDecimal = new Decimal(localAmount); | |
| const rateDecimal = new Decimal(localToAcbuRate); | |
| // Calculate ACBU equivalent | |
| const acbuAmount = localAmountDecimal.div(rateDecimal); | |
| // Get USD rate per ACBU | |
| const acbuUsdRate = new Decimal(latestRate.acbuUsd); | |
| if (acbuUsdRate.toNumber() <= 0) { | |
| throw new AppError( | |
| "USD conversion rate is invalid. Cannot process deposit at this time.", | |
| 503, | |
| ); | |
| } | |
| // Convert ACBU to USD | |
| const usdAmount = acbuAmount.mul(acbuUsdRate); | |
| // Return as number with precision | |
| return usdAmount.toNumber(); | |
| } | |
| export async function convertLocalToUsd( | |
| localAmount: number, | |
| currency: string, | |
| ): Promise<number> { | |
| if (localAmount < 0) { | |
| throw new AppError("Amount cannot be negative", 400); | |
| } | |
| // Validate currency is supported | |
| if (!CURRENCY_TO_RATE_FIELD[currency]) { | |
| throw new AppError( | |
| `Currency ${currency} not supported for conversion. Please check the supported currency list.`, | |
| 400, | |
| ); | |
| } | |
| // Fetch the latest exchange rates | |
| const latestRate = await getLatestAcbuRate().catch(() => null); | |
| if (!latestRate) { | |
| throw new AppError( | |
| "Exchange rates not yet available. Please try again in a moment.", | |
| 503, | |
| ); | |
| } | |
| // Get the rate field name for this currency | |
| const rateFieldName = CURRENCY_TO_RATE_FIELD[currency]; | |
| // Retrieve the local-to-ACBU rate (how many units of local currency per 1 ACBU) | |
| const localToAcbuRate = latestRate[rateFieldName as keyof typeof latestRate]; | |
| if (!localToAcbuRate || localToAcbuRate.toNumber() <= 0) { | |
| throw new AppError( | |
| `Exchange rate for ${currency} is not available or invalid. Cannot process deposit at this time.`, | |
| 503, | |
| ); | |
| } | |
| // Convert using high-precision Decimal arithmetic | |
| const localAmountDecimal = new Decimal(localAmount); | |
| const rateDecimal = new Decimal(localToAcbuRate); | |
| // Calculate ACBU equivalent | |
| const acbuAmount = localAmountDecimal.div(rateDecimal); | |
| // Get USD rate per ACBU | |
| const acbuUsdRate = new Decimal(latestRate.acbuUsd); | |
| if (acbuUsdRate.toNumber() <= 0) { | |
| throw new AppError( | |
| "USD conversion rate is invalid. Cannot process deposit at this time.", | |
| 503, | |
| ); | |
| } | |
| // Convert ACBU to USD | |
| const usdAmount = acbuAmount.mul(acbuUsdRate); | |
| // Return as number with precision | |
| return usdAmount.toNumber(); | |
| } |
🤖 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/services/rates/currencyConverter.ts` around lines 68 - 125, The
convertLocalToUsd function currently allows negative localAmount values, which
can produce invalid negative USD results and affect downstream limit checks. Add
a non-negative validation at the start of convertLocalToUsd, alongside the
existing currency support checks, and throw an AppError for any amount below
zero before any rate lookups or Decimal conversion occur.
Comment on lines
+80
to
+82
| // Fetch the latest exchange rates | ||
| const latestRate = await getLatestAcbuRate().catch(() => null); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve error context from getLatestAcbuRate failures.
The .catch(() => null) pattern swallows the original error, making it impossible to distinguish between DB timeouts, connection failures, and missing data in logs/monitoring.
// Fetch the latest exchange rates
- const latestRate = await getLatestAcbuRate().catch(() => null);
+ const latestRate = await getLatestAcbuRate().catch((err) => {
+ logger.warn("Failed to fetch ACBU rates", { error: err.message });
+ return null;
+ });Would you like me to generate a patch that adds structured logging to both conversion functions?
🤖 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/services/rates/currencyConverter.ts` around lines 80 - 82, The
`getLatestAcbuRate` call in `currencyConverter` is swallowing the original
failure by converting every error into null, which removes useful context from
logs and monitoring. Update the rate-fetching flow in the conversion functions
to catch the error explicitly, preserve the thrown error details, and log or
rethrow them with enough context to distinguish timeout, connection, and
missing-data cases. Use the existing `getLatestAcbuRate` and the surrounding
conversion helper(s) as the integration points, and avoid the blanket `.catch(()
=> null)` pattern.
wolfyres
changed the title
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.
Closes #477
Resolved the TypeScript any errors, cleaned up ESLint warnings, and fixed the broken Sentry import.
Summary by CodeRabbit