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 👍 / 👎.
This branch has not been deployed
1 participant
The live adapter never writes Account records (only the one-off backfill-accounts.ts did), so new holders have none and every existing one has a stale balance.
pnpm sync:ledgernow also checks each holder's Account (one per holder and currency) against the eVault: missing ones are created, ones with a wrong balance or details are updated in place, then re-read to verify. Exit code 0 now also requires every Account to be current.