Materialize vault deltas for balance sync - #2592
Conversation
How to use the Graphite Merge QueueAdd the label Raindex-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
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:
📝 WalkthroughWalkthroughAdds a maintained derived_vault_deltas read-model: schema + indexes, a delete+insert upsert batch with unit and SQLite integration tests, pipeline wiring (after decoded inserts, before vault-balance upserts), downstream query rewires, DB schema bump to v4 with test fixture updates, scheduler readiness changes, and UI/constant tweaks. ChangesDerived Vault Deltas Read-Model
Scheduler readiness
UI & constants
Sequence DiagramsequenceDiagram
participant ApplyPipeline
participant UpsertBatch as upsert_derived_vault_deltas_batch
participant DB
participant VaultBalances as UpsertVaultBalances
ApplyPipeline->>UpsertBatch: build_batch(start_block, target_block, raindex)
UpsertBatch->>DB: DELETE FROM derived_vault_deltas WHERE block_number BETWEEN start..end
UpsertBatch->>DB: INSERT OR REPLACE INTO derived_vault_deltas SELECT ... FROM vault_deltas vd WHERE block_number BETWEEN start..end
ApplyPipeline->>VaultBalances: trigger vault balance upsert (reads derived_vault_deltas)
VaultBalances->>DB: SELECT from derived_vault_deltas for running_balance computation
DB->>VaultBalances: rows for balance aggregation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 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
packages/ui-components/src/__tests__/TanstackAppTable.test.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
packages/ui-components/src/lib/components/TanstackAppTable.svelteOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
packages/webapp/src/lib/constants.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
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 |
d36008f to
fbd5d6b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/common/src/local_db/query/upsert_derived_vault_deltas/mod.rs`:
- Around line 8-17: The batch builder upsert_derived_vault_deltas_batch must
guard against an inverted block window (start_block > end_block) so it doesn't
silently produce empty BETWEEN predicates; update
upsert_derived_vault_deltas_batch to check the window and either normalize it
(swap start/end) or return an explicit no-op SqlStatementBatch (empty) when
start_block > end_block, and ensure this check happens before calling
delete_derived_vault_deltas_stmt and insert_derived_vault_deltas_stmt to avoid
generating statements with empty ranges.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8605a942-d4e7-4348-8327-e65562aae98f
📒 Files selected for processing (15)
crates/cli/src/commands/local_db/pipeline/runner/manifest.rscrates/common/src/local_db/pipeline/adapters/apply.rscrates/common/src/local_db/pipeline/runner/environment.rscrates/common/src/local_db/pipeline/runner/remotes.rscrates/common/src/local_db/query/clear_raindex_data/query.sqlcrates/common/src/local_db/query/clear_tables/query.sqlcrates/common/src/local_db/query/create_tables/mod.rscrates/common/src/local_db/query/create_tables/query.sqlcrates/common/src/local_db/query/mod.rscrates/common/src/local_db/query/upsert_derived_vault_deltas/mod.rscrates/common/src/local_db/query/upsert_derived_vault_deltas/query.sqlcrates/common/src/local_db/query/upsert_vault_balances/insert_balance_changes.sqlcrates/common/src/local_db/query/upsert_vault_balances/insert_running_balances.sqlcrates/settings/src/local_db_manifest.rscrates/settings/src/remote/manifest.rs
✅ Files skipped from review due to trivial changes (4)
- crates/common/src/local_db/query/clear_raindex_data/query.sql
- crates/common/src/local_db/query/mod.rs
- crates/common/src/local_db/query/clear_tables/query.sql
- crates/settings/src/remote/manifest.rs
| pub fn upsert_derived_vault_deltas_batch( | ||
| raindex_id: &RaindexIdentifier, | ||
| start_block: u64, | ||
| end_block: u64, | ||
| ) -> SqlStatementBatch { | ||
| SqlStatementBatch::from(vec![ | ||
| delete_derived_vault_deltas_stmt(raindex_id, start_block, end_block), | ||
| insert_derived_vault_deltas_stmt(raindex_id, start_block, end_block), | ||
| ]) | ||
| } |
There was a problem hiding this comment.
Guard against inverted block windows in batch construction.
If start_block > end_block, both BETWEEN ?3 AND ?4 predicates become empty and the rebuild silently does nothing, which can leave stale rows in derived_vault_deltas.
Suggested fix
pub fn upsert_derived_vault_deltas_batch(
raindex_id: &RaindexIdentifier,
start_block: u64,
end_block: u64,
) -> SqlStatementBatch {
+ let (start_block, end_block) = if start_block <= end_block {
+ (start_block, end_block)
+ } else {
+ (end_block, start_block)
+ };
+
SqlStatementBatch::from(vec![
delete_derived_vault_deltas_stmt(raindex_id, start_block, end_block),
insert_derived_vault_deltas_stmt(raindex_id, start_block, end_block),
])
}🤖 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 `@crates/common/src/local_db/query/upsert_derived_vault_deltas/mod.rs` around
lines 8 - 17, The batch builder upsert_derived_vault_deltas_batch must guard
against an inverted block window (start_block > end_block) so it doesn't
silently produce empty BETWEEN predicates; update
upsert_derived_vault_deltas_batch to check the window and either normalize it
(swap start/end) or return an explicit no-op SqlStatementBatch (empty) when
start_block > end_block, and ensure this check happens before calling
delete_derived_vault_deltas_stmt and insert_derived_vault_deltas_stmt to avoid
generating statements with empty ranges.
fbd5d6b to
087150a
Compare
087150a to
6f9e176
Compare
3f8691a to
7e32a38
Compare
bab7d79 to
d6970b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/common/src/local_db/query/upsert_derived_vault_deltas/mod.rs (1)
8-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard inverted block windows before constructing statements.
If
start_block > end_block, bothBETWEEN ?3 AND ?4clauses no-op and the targeted window is not refreshed.Suggested fix
pub fn upsert_derived_vault_deltas_batch( raindex_id: &RaindexIdentifier, start_block: u64, end_block: u64, ) -> SqlStatementBatch { + let (start_block, end_block) = if start_block <= end_block { + (start_block, end_block) + } else { + (end_block, start_block) + }; + SqlStatementBatch::from(vec![ delete_derived_vault_deltas_stmt(raindex_id, start_block, end_block), insert_derived_vault_deltas_stmt(raindex_id, start_block, end_block), ]) }🤖 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 `@crates/common/src/local_db/query/upsert_derived_vault_deltas/mod.rs` around lines 8 - 16, In upsert_derived_vault_deltas_batch, guard against inverted block windows by checking if start_block > end_block before building the statements (the calls to delete_derived_vault_deltas_stmt and insert_derived_vault_deltas_stmt); if the window is inverted, return an empty SqlStatementBatch (or otherwise no-op) instead of constructing statements with BETWEEN ?3 AND ?4 that will silently no-op, otherwise proceed to construct and return the batch as before.
🤖 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 `@packages/ui-components/src/lib/components/TanstackAppTable.svelte`:
- Around line 214-326: The PR is missing the required screenshot demonstrating
the frontend change in
packages/ui-components/src/lib/components/TanstackAppTable.svelte; please build
the app and capture a screenshot showing each visible state (loading skeleton -
element with data-testid="tableLoadingSkeleton", empty state -
data-testid="emptyMessage" showing emptyMessage text, populated table -
container data-testid="tanstackTableContainer" with rows rendered, and the "Load
More" button data-testid="loadMoreButton" in both enabled/disabled states as
applicable), then attach the image(s) to the pull request (or upload to the CI
artifacts for the build) so reviewers can verify the UI change.
---
Duplicate comments:
In `@crates/common/src/local_db/query/upsert_derived_vault_deltas/mod.rs`:
- Around line 8-16: In upsert_derived_vault_deltas_batch, guard against inverted
block windows by checking if start_block > end_block before building the
statements (the calls to delete_derived_vault_deltas_stmt and
insert_derived_vault_deltas_stmt); if the window is inverted, return an empty
SqlStatementBatch (or otherwise no-op) instead of constructing statements with
BETWEEN ?3 AND ?4 that will silently no-op, otherwise proceed to construct and
return the batch as before.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 917aa396-bf81-413f-b5e3-a72aff005179
📒 Files selected for processing (20)
crates/cli/src/commands/local_db/pipeline/runner/manifest.rscrates/common/src/local_db/pipeline/adapters/apply.rscrates/common/src/local_db/pipeline/runner/environment.rscrates/common/src/local_db/pipeline/runner/remotes.rscrates/common/src/local_db/query/clear_raindex_data/query.sqlcrates/common/src/local_db/query/clear_tables/query.sqlcrates/common/src/local_db/query/create_tables/mod.rscrates/common/src/local_db/query/create_tables/query.sqlcrates/common/src/local_db/query/mod.rscrates/common/src/local_db/query/upsert_derived_vault_deltas/mod.rscrates/common/src/local_db/query/upsert_derived_vault_deltas/query.sqlcrates/common/src/local_db/query/upsert_vault_balances/insert_balance_changes.sqlcrates/common/src/local_db/query/upsert_vault_balances/insert_running_balances.sqlcrates/common/src/raindex_client/local_db/pipeline/runner/scheduler/native.rscrates/common/src/raindex_client/local_db/pipeline/runner/scheduler/wasm.rscrates/settings/src/local_db_manifest.rscrates/settings/src/remote/manifest.rspackages/ui-components/src/__tests__/TanstackAppTable.test.tspackages/ui-components/src/lib/components/TanstackAppTable.sveltepackages/webapp/src/lib/constants.ts
✅ Files skipped from review due to trivial changes (2)
- crates/common/src/local_db/query/clear_tables/query.sql
- crates/cli/src/commands/local_db/pipeline/runner/manifest.rs
Merge activity
|
## Summary - add `derived_vault_deltas` as a pipeline-maintained read model for the existing `vault_deltas` view output - refresh derived vault deltas in the apply transaction before `vault_balance_changes` / `running_vault_balances` - switch vault balance maintenance SQL to read `derived_vault_deltas` instead of recomputing the view - include the table in required schema, clear/reset paths, export/import, and bump local DB schema version from 3 to 4 This PR is standalone on `main` and does not depend on the derived-trades stack. ## Benchmarks Measured with `nix develop` on a copied Base ST0x local DB, not the live DB. Before this PR, the same balance maintenance path measured: - recent 10k window: ~13.38s combined vault upsert batch - busy window: `upsert running_vault_balances` alone hit ~184.54s With `derived_vault_deltas` backfilled on the copy: - one-time full derived backfill: ~6.65s for 29,682 rows - recent 10k window: insert changes ~2.19ms, update running balances ~4.53ms - densest 10k window found: 644 derived rows, insert changes ~91.33ms, update running balances ~56.28ms ## Tests - `COMMIT_SHA=local nix develop -c cargo test -p raindex_common local_db::query --lib` - `COMMIT_SHA=local nix develop -c cargo test -p raindex_common local_db::pipeline::adapters::apply --lib` - `COMMIT_SHA=local nix develop -c cargo test -p raindex_common local_db::export --lib` - `COMMIT_SHA=local nix develop -c cargo test -p raindex_common raindex_client::local_db::pipeline::bootstrap --lib` - `COMMIT_SHA=local nix develop -c cargo test -p raindex_app_settings local_db_manifest --lib` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a derived vault-deltas read-model with windowed refreshes; pipeline now refreshes it during processing. * **Bug Fixes / Behavior** * Balance upserts now use derived deltas for consistent running balances and correct incremental updates. * Clear/reset operations now include derived deltas. * Sync scheduler marks chains as ready even when not leader. * **Chores** * Bumped DB schema to v4; tests/fixtures updated. * UI table now shows an initial loading skeleton. * Updated registry URL. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/rainlanguage/raindex/pull/2592?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/rainlanguage/raindex/pull/2592?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
ab3eb52 to
691e0ff
Compare
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)
crates/common/src/raindex_client/local_db/pipeline/runner/scheduler/wasm.rs (1)
512-513:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe NotLeader readiness assertion is not isolated from leader recovery in this test.
With Line 512 including a succeeding
Some(false)cycle, readiness at Line 559 can be set by the leader path rather than the NotLeader branch.Proposed test hardening
- let runner = RecordingRunner::new( + let runner = RecordingRunner::new( 1, Rc::clone(&calls), Rc::clone(&failures), - vec![None, Some(false)], + vec![None], ); spawn_network_loop( runner, noop_local_db(), Some(Rc::new(status_callback)), Rc::clone(&stop_flag), - 1, + 10_000, readiness.clone(), LocalDbSyncStatusStore::new(), ); - TimeoutFuture::new(0).await; - TimeoutFuture::new(5).await; + while calls.get() < 1 { + TimeoutFuture::new(1).await; + } stop_flag.set(true); TimeoutFuture::new(5).await;Also applies to: 528-529, 558-561
🤖 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
`@crates/common/src/raindex_client/local_db/pipeline/runner/scheduler/native.rs`:
- Around line 584-586: The test network_loop_handles_not_leader is seeding
RecordingRunner with a single None so the runner can fall through to a later
success outcome instead of exercising the NotLeader readiness path; modify the
RecordingRunner::new invocation (and analogous calls around RecordingRunner
usage) to supply enough None entries or explicit NotLeader outcomes (e.g.,
vec![None, None, Some(NotLeader(...))] or explicit failure entries) to cover the
leader readiness check path so readiness at the place currently checked by the
test comes from the intended NotLeader scenario; update any related
stop_flag/runner setup to match the expanded outcome sequence so the test
deterministically exercises the NotLeader readiness branch.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f640014c-7d59-4ca3-abf6-4b341bce1ca9
📒 Files selected for processing (20)
crates/cli/src/commands/local_db/pipeline/runner/manifest.rscrates/common/src/local_db/pipeline/adapters/apply.rscrates/common/src/local_db/pipeline/runner/environment.rscrates/common/src/local_db/pipeline/runner/remotes.rscrates/common/src/local_db/query/clear_raindex_data/query.sqlcrates/common/src/local_db/query/clear_tables/query.sqlcrates/common/src/local_db/query/create_tables/mod.rscrates/common/src/local_db/query/create_tables/query.sqlcrates/common/src/local_db/query/mod.rscrates/common/src/local_db/query/upsert_derived_vault_deltas/mod.rscrates/common/src/local_db/query/upsert_derived_vault_deltas/query.sqlcrates/common/src/local_db/query/upsert_vault_balances/insert_balance_changes.sqlcrates/common/src/local_db/query/upsert_vault_balances/insert_running_balances.sqlcrates/common/src/raindex_client/local_db/pipeline/runner/scheduler/native.rscrates/common/src/raindex_client/local_db/pipeline/runner/scheduler/wasm.rscrates/settings/src/local_db_manifest.rscrates/settings/src/remote/manifest.rspackages/ui-components/src/__tests__/TanstackAppTable.test.tspackages/ui-components/src/lib/components/TanstackAppTable.sveltepackages/webapp/src/lib/constants.ts
✅ Files skipped from review due to trivial changes (4)
- crates/common/src/local_db/query/mod.rs
- crates/common/src/local_db/query/clear_tables/query.sql
- crates/common/src/local_db/query/upsert_vault_balances/insert_running_balances.sql
- packages/webapp/src/lib/constants.ts
| let runner = | ||
| RecordingRunner::new(Arc::clone(&calls), Arc::clone(&failures), vec![None]); | ||
| let stop_flag = Arc::new(AtomicBool::new(false)); |
There was a problem hiding this comment.
network_loop_handles_not_leader can pass without actually validating the NotLeader readiness path.
Line 585 only seeds one None, but the runner falls back to success once outcomes are exhausted; readiness at Line 609 can therefore come from a later leader cycle.
Proposed test hardening
- let runner =
- RecordingRunner::new(Arc::clone(&calls), Arc::clone(&failures), vec![None]);
+ let runner =
+ RecordingRunner::new(Arc::clone(&calls), Arc::clone(&failures), vec![None]);
tokio::task::spawn_local(run_network_loop(
runner,
noop_local_db(),
NativeNetworkLoopContext {
stop_flag: Arc::clone(&stop_flag),
- interval_ms: 1,
+ interval_ms: 10_000,
network_key: "test".to_string(),
chain_id: 1,
sync_readiness: readiness.clone(),
status_store: LocalDbSyncStatusStore::new(),
},
));
- tokio::time::sleep(std::time::Duration::from_millis(30)).await;
+ for _ in 0..100 {
+ if calls.load(Ordering::SeqCst) >= 1 {
+ break;
+ }
+ tokio::time::sleep(std::time::Duration::from_millis(1)).await;
+ }
stop_flag.store(true, Ordering::SeqCst);
tokio::time::sleep(std::time::Duration::from_millis(10)).await;
- assert!(calls.load(Ordering::SeqCst) >= 1);
+ assert_eq!(calls.load(Ordering::SeqCst), 1);Also applies to: 606-610
🤖 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
`@crates/common/src/raindex_client/local_db/pipeline/runner/scheduler/native.rs`
around lines 584 - 586, The test network_loop_handles_not_leader is seeding
RecordingRunner with a single None so the runner can fall through to a later
success outcome instead of exercising the NotLeader readiness path; modify the
RecordingRunner::new invocation (and analogous calls around RecordingRunner
usage) to supply enough None entries or explicit NotLeader outcomes (e.g.,
vec![None, None, Some(NotLeader(...))] or explicit failure entries) to cover the
leader readiness check path so readiness at the place currently checked by the
test comes from the intended NotLeader scenario; update any related
stop_flag/runner setup to match the expanded outcome sequence so the test
deterministically exercises the NotLeader readiness branch.
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
|
Tip For best results, initiate chat on the files or code changes. SIZE=L |
3 participants
Summary
derived_vault_deltasas a pipeline-maintained read model for the existingvault_deltasview outputvault_balance_changes/running_vault_balancesderived_vault_deltasinstead of recomputing the viewThis PR is standalone on
mainand does not depend on the derived-trades stack.Benchmarks
Measured with
nix developon a copied Base ST0x local DB, not the live DB.Before this PR, the same balance maintenance path measured:
upsert running_vault_balancesalone hit ~184.54sWith
derived_vault_deltasbackfilled on the copy:Tests
COMMIT_SHA=local nix develop -c cargo test -p raindex_common local_db::query --libCOMMIT_SHA=local nix develop -c cargo test -p raindex_common local_db::pipeline::adapters::apply --libCOMMIT_SHA=local nix develop -c cargo test -p raindex_common local_db::export --libCOMMIT_SHA=local nix develop -c cargo test -p raindex_common raindex_client::local_db::pipeline::bootstrap --libCOMMIT_SHA=local nix develop -c cargo test -p raindex_app_settings local_db_manifest --libSummary by CodeRabbit
New Features
Bug Fixes / Behavior
Chores