Repository navigation
feat(database): recalculate all balances migration - #5
Closed
einhornimmond wants to merge 23 commits into
Closed
einhornimmond wants to merge 23 commits into
einhornimmond wants to merge 23 commits into
Conversation
added 2 commits
May 19, 2026 13:45
…h --use-only-legacy-ledger-anchor-ids, use db account balances in dlt-migration
|
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:
📝 WalkthroughWalkthroughThis PR adds a migration to recompute transaction balances using native decay logic, enables big-number and multiple-statement MySQL pool options for migrations, and centralizes ledger-anchor selection across multiple sync roles controlled by a new Context CLI flag. ChangesBalance Recalculation & Ledger Anchor Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/LocalTransactionsSync.role.ts (1)
130-161:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep this balance path decay-type aware.
calculateBalances()still usesupdateLegacyDecay(...)for both accounts, but Line 186 now invokes it for every row. For transactions whoseitem.decayCalculationTypeis no longerDECIMAL_JS_FIXED_FACTOR, this replays legacy-derived balances intoaddToBlockchain(...)and can diverge from the migrated balances.Possible safe fallback until this helper supports both algorithms
addToBlockchain( this.buildTransaction(communityContext, item, senderKeyPair, recipientKeyPair).build(), blockchain, this.getLedgerAnchor(item), - this.calculateBalances(item, communityContext, senderPublicKey, recipientPublicKey), + item.decayCalculationType === DecayCalculationType.DECIMAL_JS_FIXED_FACTOR + ? this.calculateBalances( + item, + communityContext, + senderPublicKey, + recipientPublicKey, + ) + : undefined, )Also applies to: 186-187
🤖 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 `@dlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/LocalTransactionsSync.role.ts` around lines 130 - 161, calculateBalances currently always calls updateLegacyDecay on sender and recipient balances, which replays legacy decay for rows that may have a different decay algorithm; update the logic in calculateBalances (and the similar calls around updateLegacyDecay at the other occurrence noted) to inspect item.decayCalculationType (compare to DECIMAL_JS_FIXED_FACTOR) after obtaining balances via getLastBalanceForUser and call the appropriate decay updater (e.g., updateLegacyDecay for DECIMAL_JS_FIXED_FACTOR, otherwise call the new decay-aware updater such as updateDecay/updateWithCurrentAlgorithm) and only log/throw via logLastBalanceChangingTransactions / throw when a NegativeBalanceError arises from the correct decay path so migrated balances aren’t overwritten before addToBlockchain.
🧹 Nitpick comments (1)
database/migration/migrations/0102-recalculate_balances.ts (1)
12-18: ⚖️ Poor tradeoffMemory usage: Loading all transactions per user into memory.
For users with thousands of transactions, loading all rows at once could consume significant memory. While this is likely acceptable for most users, consider whether streaming/cursor-based iteration would be more robust.
This is a data-size concern rather than a correctness issue. If user transaction counts are bounded (e.g., < 10,000 per user), the current approach is acceptable. Otherwise, consider cursor-based iteration:
// Pseudo-code - check if mysql library supports cursors const cursor = await queryFn.cursor(` SELECT id, amount_gdd4, balance_gdd4, decay_gdd4, balance_date FROM transactions WHERE user_id = ? ORDER BY balance_date ASC `, [users[u].id]) for await (const transaction of cursor) { // Process transaction }🤖 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 `@database/migration/migrations/0102-recalculate_balances.ts` around lines 12 - 18, The current code loads all rows for a user into the transactions array via queryFn(...) (selecting id, amount_gdd4, balance_gdd4, decay_gdd4, balance_date WHERE user_id = ${users[u].id} ORDER BY balance_date ASC), which can OOM for large users; change this to stream or iterate results incrementally (use your DB client's cursor/stream API or implement batched pagination with ORDER BY + LIMIT/OFFSET or keyset pagination) so you process each transaction one-by-one (or in small batches) and update balances without accumulating the full result set in memory; ensure you preserve the ORDER BY balance_date ASC behavior and replace usages of the transactions array with the incremental iterator/cursor results.
🤖 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 `@database/migration/index.ts`:
- Around line 49-51: The migration enables multipleStatements which increases
SQL injection risk; update migration 0102-recalculate_balances.ts to stop using
string-interpolated SQL and instead pass values as parameters via the second
argument to queryFn (e.g., replace interpolated fragments at the places noted
around lines 15, 41-43, 57) so queries use parameter placeholders and a params
array/object; also remove or avoid relying on multipleStatements: true for these
queries where possible and ensure functions like queryFn and any helper (e.g.,
recalculateBalances or similar within the migration) receive typed values and
pass them as parameters rather than concatenating into SQL strings.
In `@database/migration/migrations/0102-recalculate_balances.ts`:
- Line 6: The migration currently pushes all per-user update promises into
runningRequests and calls Promise.all (using runningRequests) which can create
thousands of concurrent DB operations; change this to a bounded-concurrency
approach by introducing a constant BATCH_SIZE (e.g., 20–50) and processing users
in chunks or using a simple concurrency pool: build up at most BATCH_SIZE
promises (from the same per-user update logic that populates runningRequests),
await Promise.all on that batch, then continue to the next batch until all users
are processed; update the code paths that reference runningRequests and the
final Promise.all call to use this batched/limited-await pattern so DB
connections are limited and backpressure is applied.
- Around line 4-64: Wrap the entire upgrade function body in an explicit DB
transaction by calling await queryFn('START TRANSACTION') at the start, then run
the existing logic (including collecting and awaiting runningRequests and the
final UPDATE using DecayCalculationType), then call await queryFn('COMMIT') in
the try path; on any catch call await queryFn('ROLLBACK') and rethrow the error.
Ensure the Promise.all(runningRequests) and the `UPDATE transactions set
decay_calculation_type = ...` request are executed/awaited inside the try before
committing so all updates are included in the transaction; keep function name
upgrade and existing variables (runningRequests, transactionsToUpdate,
countDiffs, DecayCalculationType) unchanged.
- Around line 12-18: The SQL uses string interpolation and must be changed to
parameterized queries: replace the inline interpolation in the SELECT that uses
users[u].id (and all other interpolated queries around lines referencing
transactions, transaction.id, balance, decay) by calling queryFn with a
parameterized SQL string and a values array (e.g., "WHERE user_id = $1" and
[users[u].id]); similarly change the UPDATEs that reference transaction.id,
balance, and decay (and the batch update logic around transactionsToUpdate) to
use parameter placeholders and pass the corresponding values
(balance.toString(), decay.toString(), transaction.id) via the queryFn values
parameter or a transaction/batching API so no raw values are concatenated into
SQL.
- Around line 67-70: Migration 0102's downgrade assumes columns balance_legacy
and decay_legacy (created in migration 0100) still exist, which breaks if 0100
was already downgraded first; update the 0102 downgrade handler to guard against
this by checking for the existence of balance_legacy and decay_legacy before
running any SQL that references them (or return/throw a clear error explaining
the required downgrade order), and add a brief comment documenting the
dependency on migration 0100; locate this logic in the 0102 migration file's
downgrade/exported down function and reference migration 0100 in the comment.
In
`@dlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/TransactionLinkFundingsSync.role.ts`:
- Around line 171-177: Restore the decay-cutover guard before calling
calculateBalances so legacy decay logic is only applied to pre-cutover link
fundings: check the record's cutover determinant (e.g., blockHeight or
timestamp) against the decay cutover threshold and, if the record is before
cutover, call calculateBalances which will run updateLegacyDecay(...); otherwise
skip/branch to the post-cutover balance path so calculateBalances does not emit
legacy-derived balances for post-cutover rows (apply this check immediately
before the calculateBalances(...) invocation that currently lives at the
TransactionLinkFundingsSync role).
In
`@dlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/UsersSync.role.ts`:
- Line 27: Replace the hardcoded legacy anchor type usage in the role with the
centralized ledger-anchor helper: remove passing
LedgerAnchor.Type_LEGACY_GRADIDO_DB_USER_ID into super(...) and instead call the
shared ledger-anchor helper used elsewhere to compute the anchor id/type for
this role; update pushToBlockchain() so it uses that same helper (not
item.messageId) to construct the anchor and to honor the
--use-only-legacy-ledger-anchor-ids flag; make corresponding changes where
item.messageId is preferred (lines ~124-129) to prefer the helper’s output or an
explicit fallback so the flag is not bypassed.
---
Outside diff comments:
In
`@dlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/LocalTransactionsSync.role.ts`:
- Around line 130-161: calculateBalances currently always calls
updateLegacyDecay on sender and recipient balances, which replays legacy decay
for rows that may have a different decay algorithm; update the logic in
calculateBalances (and the similar calls around updateLegacyDecay at the other
occurrence noted) to inspect item.decayCalculationType (compare to
DECIMAL_JS_FIXED_FACTOR) after obtaining balances via getLastBalanceForUser and
call the appropriate decay updater (e.g., updateLegacyDecay for
DECIMAL_JS_FIXED_FACTOR, otherwise call the new decay-aware updater such as
updateDecay/updateWithCurrentAlgorithm) and only log/throw via
logLastBalanceChangingTransactions / throw when a NegativeBalanceError arises
from the correct decay path so migrated balances aren’t overwritten before
addToBlockchain.
---
Nitpick comments:
In `@database/migration/migrations/0102-recalculate_balances.ts`:
- Around line 12-18: The current code loads all rows for a user into the
transactions array via queryFn(...) (selecting id, amount_gdd4, balance_gdd4,
decay_gdd4, balance_date WHERE user_id = ${users[u].id} ORDER BY balance_date
ASC), which can OOM for large users; change this to stream or iterate results
incrementally (use your DB client's cursor/stream API or implement batched
pagination with ORDER BY + LIMIT/OFFSET or keyset pagination) so you process
each transaction one-by-one (or in small batches) and update balances without
accumulating the full result set in memory; ensure you preserve the ORDER BY
balance_date ASC behavior and replace usages of the transactions array with the
incremental iterator/cursor results.
🪄 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 Plus
Run ID: f5b75f02-548d-40f1-91c7-e3c7ea1f858f
📒 Files selected for processing (12)
database/migration/index.tsdatabase/migration/migrations/0102-recalculate_balances.tsdlt-connector/package.jsondlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/Context.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/AbstractSync.role.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/CreationsSync.role.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/DeletedTransactionLinksSync.role.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/LocalTransactionsSync.role.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/RedeemTransactionLinksSync.role.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/RemoteTransactionsSync.role.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/TransactionLinkFundingsSync.role.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/UsersSync.role.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
database/migration/migrations/0102-recalculate_balances.ts (1)
41-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate rows when
decay_gdd4changes too.This guard only compares
balance_gdd4, but the migration also recomputesdecay_gdd4. A row whose balance happens to match while its decay differs will be skipped and then still get the newdecay_calculation_type.💡 Suggested fix
- if (BigInt(transaction.balance_gdd4) !== balance) { + if ( + BigInt(transaction.balance_gdd4) !== balance || + BigInt(transaction.decay_gdd4) !== decay + ) {🤖 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 `@database/migration/migrations/0102-recalculate_balances.ts` around lines 41 - 47, The current conditional only checks BigInt(transaction.balance_gdd4) !== balance so rows where decay_gdd4 changed are skipped; change the guard to also compare BigInt(transaction.decay_gdd4) !== decay (i.e., trigger when balance OR decay differs) so you still increment countDiffs and push the UPDATE into transactionsToUpdate (which sets balance_gdd4, decay_gdd4 and decay_calculation_type) when either value differs; update the condition that references transaction.balance_gdd4, transaction.decay_gdd4, balance and decay accordingly.dlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/TransactionLinkFundingsSync.role.ts (1)
28-29:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftSeparate the legacy anchor namespace for link funding from link close-out events.
TransactionLinkFundingsSyncRole,DeletedTransactionLinksSyncRole, andRedeemTransactionLinksSyncRoleall reuseLedgerAnchor.Type_LEGACY_GRADIDO_DB_TRANSACTION_LINK_ID. With--use-only-legacy-ledger-anchor-idsenabled, a funding transaction and the later delete/redeem transaction for the sametransactionLinks.idcollapse onto the same ledger anchor, risking replay rejection or incorrect lineage attachment. Use a distinct legacy type here, or encode the event kind in the legacy fallback id withingetLedgerAnchor().🤖 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 `@dlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/TransactionLinkFundingsSync.role.ts` around lines 28 - 29, The legacy anchor type LedgerAnchor.Type_LEGACY_GRADIDO_DB_TRANSACTION_LINK_ID is being reused across TransactionLinkFundingsSyncRole, DeletedTransactionLinksSyncRole, and RedeemTransactionLinksSyncRole causing funding and close-out events for the same transactionLinks.id to collapse; update the roles to produce distinct legacy fallback anchors by either introducing a new legacy type constant for funding (and new ones for delete/redeem) or by encoding the event kind into the legacy fallback id inside getLedgerAnchor() for TransactionLinkFundingsSyncRole (and analogous getLedgerAnchor implementations in DeletedTransactionLinksSyncRole and RedeemTransactionLinksSyncRole) so each event kind maps to a unique legacy anchor when --use-only-legacy-ledger-anchor-ids is enabled.
🤖 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 `@database/migration/migrations/0102-recalculate_balances.ts`:
- Around line 12-18: The SELECT used in the migration (the queryFn call that
reads id, amount_gdd4, balance_gdd4, decay_gdd4, balance_date from the
transactions table) orders only by balance_date, which is nondeterministic for
equal timestamps; make the ordering deterministic by adding a stable tie-breaker
using the primary key (id) as a secondary sort (i.e., sort by balance_date then
id) so rows with identical balance_date are processed in a consistent order when
recalculating balance_gdd4.
- Around line 59-64: The bulk marker update is being started immediately because
pushing queryFn(...) into runningRequests invokes it right away; change the
sequence so that per-user recalculation promises (the array runningRequests
created for recalculations) are awaited first and only after
Promise.all(runningRequests) completes successfully call queryFn to update
decay_calculation_type to DecayCalculationType.NATIVE_C_FIXED_FACTOR_INTEGER.
Concretely, remove the early push of queryFn(...) and move the call that runs
`UPDATE transactions set decay_calculation_type = ?` (using queryFn and the
DecayCalculationType.NATIVE_C_FIXED_FACTOR_INTEGER argument) to execute after
the await Promise.all(runningRequests) so the marker flips only on success.
- Line 47: The current loop pushes UPDATE strings into transactionsToUpdate
using string interpolation (transactionsToUpdate.push(... WHERE id =
${Number(transaction.id)})), which loses precision for big integer IDs and risks
SQL injection; change this to execute each UPDATE individually using
parameterized query bindings (use the same binding style shown around lines
59–62) and pass transaction.id as a bound parameter (do not cast to Number),
while supplying balance.toString() and decay.toString() as bound values so the
DB receives exact values for id, balance and decay.
---
Outside diff comments:
In `@database/migration/migrations/0102-recalculate_balances.ts`:
- Around line 41-47: The current conditional only checks
BigInt(transaction.balance_gdd4) !== balance so rows where decay_gdd4 changed
are skipped; change the guard to also compare BigInt(transaction.decay_gdd4) !==
decay (i.e., trigger when balance OR decay differs) so you still increment
countDiffs and push the UPDATE into transactionsToUpdate (which sets
balance_gdd4, decay_gdd4 and decay_calculation_type) when either value differs;
update the condition that references transaction.balance_gdd4,
transaction.decay_gdd4, balance and decay accordingly.
In
`@dlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/TransactionLinkFundingsSync.role.ts`:
- Around line 28-29: The legacy anchor type
LedgerAnchor.Type_LEGACY_GRADIDO_DB_TRANSACTION_LINK_ID is being reused across
TransactionLinkFundingsSyncRole, DeletedTransactionLinksSyncRole, and
RedeemTransactionLinksSyncRole causing funding and close-out events for the same
transactionLinks.id to collapse; update the roles to produce distinct legacy
fallback anchors by either introducing a new legacy type constant for funding
(and new ones for delete/redeem) or by encoding the event kind into the legacy
fallback id inside getLedgerAnchor() for TransactionLinkFundingsSyncRole (and
analogous getLedgerAnchor implementations in DeletedTransactionLinksSyncRole and
RedeemTransactionLinksSyncRole) so each event kind maps to a unique legacy
anchor when --use-only-legacy-ledger-anchor-ids is enabled.
🪄 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 Plus
Run ID: 33ce6325-b027-49aa-9101-398ea7a74c4b
⛔ Files ignored due to path filters (1)
dlt-connector/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
database/migration/migrations/0102-recalculate_balances.tsdlt-connector/package.jsondlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/data/Balance.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/drizzle.schema.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/CreationsSync.role.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/DeletedTransactionLinksSync.role.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/LocalTransactionsSync.role.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/RedeemTransactionLinksSync.role.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/RemoteTransactionsSync.role.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/TransactionLinkFundingsSync.role.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/interaction/syncDbWithBlockchain/UsersSync.role.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/utils.tsdlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/valibot.schema.tsdlt-connector/src/schemas/typeGuard.schema.ts
💤 Files with no reviewable changes (5)
- dlt-connector/package.json
- dlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/drizzle.schema.ts
- dlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/utils.ts
- dlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/data/Balance.ts
- dlt-connector/src/migrations/db-v2.7.0_to_blockchain-v3.7/valibot.schema.ts
✅ Files skipped from review due to trivial changes (1)
- dlt-connector/src/schemas/typeGuard.schema.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
database/migration/migrations/0102-recalculate_balances.ts (1)
41-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate rows when
decay_gdd4changes, not only whenbalance_gdd4changes.Line 41 gates the UPDATE solely on
balance_gdd4. That can leave a staledecay_gdd4in place while Lines 60-62 still mark the row asNATIVE_C_FIXED_FACTOR_INTEGER, which makes the stored decay metadata inconsistent with the selected calculation type.💡 Suggested fix
- if (BigInt(transaction.balance_gdd4) !== balance) { + if ( + BigInt(transaction.balance_gdd4) !== balance || + BigInt(transaction.decay_gdd4) !== decay + ) { countDiffs++ transactionsToUpdate.push( `UPDATE transactions SET balance_gdd4 = '${balance.toString()}', decay_gdd4 = '${decay.toString()}'🤖 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 `@database/migration/migrations/0102-recalculate_balances.ts` around lines 41 - 49, The UPDATE is only triggered when BigInt(transaction.balance_gdd4) !== balance, leaving decay_gdd4 stale; change the conditional that currently references transaction.balance_gdd4 to also compare decay: check if BigInt(transaction.balance_gdd4) !== balance OR BigInt(transaction.decay_gdd4) !== decay, and only then increment countDiffs and push the UPDATE SQL into transactionsToUpdate (still setting both balance_gdd4 and decay_gdd4 using balance.toString() and decay.toString()); reference the existing variables/fields transaction.balance_gdd4, transaction.decay_gdd4, balance, decay, countDiffs, and transactionsToUpdate when making this change.
🤖 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.
Outside diff comments:
In `@database/migration/migrations/0102-recalculate_balances.ts`:
- Around line 41-49: The UPDATE is only triggered when
BigInt(transaction.balance_gdd4) !== balance, leaving decay_gdd4 stale; change
the conditional that currently references transaction.balance_gdd4 to also
compare decay: check if BigInt(transaction.balance_gdd4) !== balance OR
BigInt(transaction.decay_gdd4) !== decay, and only then increment countDiffs and
push the UPDATE SQL into transactionsToUpdate (still setting both balance_gdd4
and decay_gdd4 using balance.toString() and decay.toString()); reference the
existing variables/fields transaction.balance_gdd4, transaction.decay_gdd4,
balance, decay, countDiffs, and transactionsToUpdate when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 326a1bc4-2b4a-4bfa-bc48-a3ce1e4f6e57
📒 Files selected for processing (1)
database/migration/migrations/0102-recalculate_balances.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
database/migration/migrations/0102-recalculate_balances.ts (1)
55-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompare
decay_gdd4too before skipping an UPDATE.Right now a row is updated only when
balance_gdd4changes. If the recomputeddecay_gdd4differs while the running balance happens to match, the old decay is kept but Line 81 still marks the row asNATIVE_C_FIXED_FACTOR_INTEGER.💡 Minimal fix
- if (BigInt(transaction.balance_gdd4) !== balance) { + if ( + BigInt(transaction.balance_gdd4) !== balance || + BigInt(transaction.decay_gdd4) !== decay + ) {🤖 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 `@database/migration/migrations/0102-recalculate_balances.ts` around lines 55 - 67, The current conditional only checks balance_gdd4 and skips UPDATEs when balance matches, causing decay_gdd4 mismatches to be left unchanged while the row is still marked as NATIVE_C_FIXED_FACTOR_INTEGER; change the conditional that checks BigInt(transaction.balance_gdd4) !== balance to also check decay: update it to if (BigInt(transaction.balance_gdd4) !== balance || BigInt(transaction.decay_gdd4) !== decay) so rows are updated when either value differs, update the console logging (logEachStep) to include decay.toString() as well, and ensure transactionsToUpdate, countDiffs and the SET clause still assign both balance_gdd4 and decay_gdd4 when the condition is true.
🤖 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 `@database/migration/migrations/0102-recalculate_balances.ts`:
- Line 14: Remove the hardcoded debug prints that stream user 764's balance
history and legacy balance in the migration (the SELECT/print blocks around the
lines showing "SELECT id, amount_gdd4, balance_gdd4, balance_legacy, decay_gdd4,
balance_date" and the subsequent printing loops). Delete the console.log/print
statements that specifically target userId 764 (and any loops that dump rows for
that user), or wrap them behind a strict debug guard (e.g.,
process.env.MIGRATION_DEBUG === '1') so no financial data is logged by default;
keep the migration logic (recalculate balances) intact in the functions handling
the migration/up/down but remove the debug-only output.
---
Outside diff comments:
In `@database/migration/migrations/0102-recalculate_balances.ts`:
- Around line 55-67: The current conditional only checks balance_gdd4 and skips
UPDATEs when balance matches, causing decay_gdd4 mismatches to be left unchanged
while the row is still marked as NATIVE_C_FIXED_FACTOR_INTEGER; change the
conditional that checks BigInt(transaction.balance_gdd4) !== balance to also
check decay: update it to if (BigInt(transaction.balance_gdd4) !== balance ||
BigInt(transaction.decay_gdd4) !== decay) so rows are updated when either value
differs, update the console logging (logEachStep) to include decay.toString() as
well, and ensure transactionsToUpdate, countDiffs and the SET clause still
assign both balance_gdd4 and decay_gdd4 when the condition is true.
🪄 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 Plus
Run ID: ad2e269d-9170-4af2-a8b0-2ec0c0cfb6f4
📒 Files selected for processing (1)
database/migration/migrations/0102-recalculate_balances.ts
added 13 commits
May 20, 2026 15:42
…lculate_balances_all_with_new_decay_algo2
einhornimmond
deleted the
database_recalculate_balances_all_with_new_decay_algo2
branch
October 21, 2026 11:06
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
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.
🍰 Pullrequest
Issues
Todo
Summary by CodeRabbit
New Features
Chores