Repository navigation
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36ce36ed0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (await AppDataSource.getRepository(Currency).find()).map((c) => [c.id, c]), | ||
| ); | ||
| const accountsByVault = new Map<string, Map<string, ExpectedAccount>>(); | ||
| for (const row of rows) { |
There was a problem hiding this comment.
Because rows was filtered using the default 120-second settlement cutoff, deriving accounts from it ignores every recent transaction. If an account already has an eVault record, a check can therefore report it as synced even though its balance is stale; worse, --apply can overwrite a correct record with the balance preceding a recent transaction. The settlement window is useful for ledger-copy synchronization, but account state should be derived separately from the latest database row for each account and currency.
Useful? React with 👍 / 👎.
| typeof record.currencyEname === "string" && | ||
| normalizeEname(record.currencyEname) === a.payload.currencyEname; | ||
|
|
||
| const isAccountCurrent = (record: Record<string, unknown>, a: ExpectedAccount) => |
There was a problem hiding this comment.
When an existing Account envelope has the correct balance, type, name, and eNames but an incorrect or missing createdAt, this predicate classifies it as fully synced even though createdAt is part of the expected payload. Consequently --apply never repairs that field and the command can exit successfully while retaining incorrect account metadata; compare the stored timestamp with a.payload.createdAt as part of the current-state check.
Useful? React with 👍 / 👎.
…h the wrong currency id, balances from the chain
coodos
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62144ff829
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const currencyList = await AppDataSource.getRepository(Currency).find(); | ||
| const currencies = new Map(currencyList.map((c) => [c.id, c])); | ||
| const currencyReport = await syncCurrencies(currencyList, enameOf); |
There was a problem hiding this comment.
When --only=@target is supplied, this call still passes every currency to syncCurrencies, which queries every creator vault and, with --apply, can create records in vaults other than the requested target. Even in check-only mode, an unrelated currency failure can make a targeted check exit nonzero, so filter currencies or otherwise scope currency synchronization to the selected vault.
Useful? React with 👍 / 👎.
| if (mapped) { | ||
| currencyGlobalIds.set(c.id, mapped); | ||
| report.elsewhere.push(`${label}: mapped to ${mapped}, not in ${owner}`); | ||
| continue; |
There was a problem hiding this comment.
When a mapping exists but no matching Currency record is found in the queried vault, this branch assumes the record exists elsewhere without querying any other vault and does not add a failure. A stale mapping can therefore be copied into currencyGlobalIds, causing --apply to rewrite ledger entries with a dangling currencyId while the command can still exit successfully; locate and verify the mapped record before accepting its ID.
Useful? React with 👍 / 👎.
| const copies = records.filter((r) => isAccountCopy(r.parsed, a)); | ||
| const current = copies.find((r) => isAccountCurrent(r.parsed, a)); | ||
| const status: Status = current ? "synced" : copies.length ? "outdated" : "missing"; |
There was a problem hiding this comment.
When a vault contains duplicate Account envelopes for the same account and currency, finding any current copy classifies the account as synced even if another copy has an old balance; if all copies are stale, only the selected best copy is updated and the recheck then has the same false-success behavior. Consumers select by currencyEname, so they can encounter the stale duplicate; all matching copies need to be made current or duplicates must keep the account unsynced.
Useful? React with 👍 / 👎.
| const report: CurrencyReport = { synced: 0, missing: 0, created: 0, elsewhere: [], failed: [] }; | ||
| for (const c of currencies) { | ||
| const label = `${c.name} (${c.ename})`; | ||
| const owner = enameOf.get(`user:${c.createdBy}`); |
There was a problem hiding this comment.
For currencies whose group-admin owner differs from the creator, this hard-codes the wrong vault: currency.mapping.json resolves groups(group.admins[].ename) before falling back to the creator. Consequently a recovery with no local mapping can create a duplicate Currency record in the creator's vault instead of finding the adapter-written record, and a creator without an eName is reported as a failure even when the record exists in the configured admin vault.
Useful? React with 👍 / 👎.
… the holder's vault
There was a problem hiding this comment.
💡 Codex Review
prototype/platforms/ecurrency/api/src/scripts/sync-ledger.ts
Lines 559 to 560 in c5eaaa7
Propagate currency mapping write failures
When rememberId fails while saving a newly created or rediscovered currency mapping—for example because the mapping database is unavailable or the delete succeeds but the replacement insert fails—the error is reduced to a warning and syncCurrencies still records the global ID in memory and reports success. The current repair run can therefore exit 0, while subsequent live ledger synchronization has no persistent currency mapping and resumes writing internal currency UUIDs; currency mapping failures need to make the currency report fail.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const isComplete = (record: Record<string, unknown>, row: Ledger) => | ||
| !row.hash || record.hash === row.hash; | ||
| (!row.hash || record.hash === row.hash) && record.currencyId === expectedCurrencyId(row); |
There was a problem hiding this comment.
When --apply encounters an unmapped currency whose synchronization fails—for example because its owner cannot be resolved or the currency write/re-read fails—currencyGlobalIds remains empty and expectedCurrencyId falls back to the internal database UUID. This completeness check then classifies any existing ledger copy carrying the currency's real global ID as outdated and rewrites it with that internal UUID, corrupting otherwise valid links despite the run ultimately exiting nonzero. Ledger mutation should be skipped for currencies whose global ID was not successfully resolved.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Found on prod: CivEUR's Currency record was never written to any eVault, its ledger copies carry eCurrency's internal currency id, and Account records are missing (20) or stale (11), so moneyw3 shows wrong balances and no CivEUR history.