Add derived trades local DB read model#2587
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a ChangesDerived Trades Read-Model Introduction and Query Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR is substantial and involves multiple layers of refactoring across the trade-query architecture. While changes are cohesive in purpose (migrate to precomputed trades table), they span numerous files with heterogeneous edit patterns: table/index definitions, complex SQL CTEs, parameter-binding logic refactors, test updates, and fixture changes. The upsert SQL logic and query refactoring patterns require careful verification across different query builders, and the pipeline integration ordering requires tracing statement sequences through apply.rs tests. 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/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 |
Collaborator
Author
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. |
This was referenced May 22, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/settings/src/local_db_manifest.rs`:
- Line 12: Tests in this file still assert and use the old db-schema-version (3)
after updating the constant DB_SCHEMA_VERSION to 4, causing
parse_manifest_header/parse_manifest_doc to fail; update all fixtures and
assertions to use the new schema version or, better, make fixtures driven by the
DB_SCHEMA_VERSION constant (e.g., use the constant when constructing test
manifest headers and replace hard-coded `db-schema-version: 3` and `sv == 3`
checks with DB_SCHEMA_VERSION or a helper that emits the current
DB_SCHEMA_VERSION) so parse_manifest_header/parse_manifest_doc and sv assertions
stay in sync.
🪄 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: fe951c46-c297-4b06-911f-ef16e953f9f4
📒 Files selected for processing (5)
crates/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/settings/src/local_db_manifest.rs
findolor
force-pushed
the
arda/derived-trades-schema
branch
from
46756c6 to
381599c
Compare
findolor
changed the title
findolor
force-pushed
the
arda/derived-trades-schema
branch
from
2334e5b to
710fddf
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/settings/src/remote/manifest.rs (1)
212-272:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject mixed schema generations when fetching multiple manifests.
These tests all move to
db-schema-version: 4, butfetch_multiple_manifestsstill just folds results into a map, so different URLs can return differentdb_schema_versions without tripping an error. That undermines the goal of keeping bootstrap/import sources and incremental sync on the same local-DB protocol, and the existingInconsistentManifestVersionspath is never exercised.🤖 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/settings/src/remote/manifest.rs` around lines 212 - 272, fetch_multiple_manifests currently collects per-URL manifests into a map without checking db-schema-version consistency, allowing mixed schema generations; update the fetch_multiple_manifests function to validate that all fetched Manifest instances share the same db_schema_version (the Manifest.db_schema_version field) and if any differ return the existing InconsistentManifestVersions error (or a clear error variant) instead of folding them silently into the map, ensuring the check runs after parsing each manifest and before returning the map so callers never get mixed-schema results.
🧹 Nitpick comments (1)
crates/common/src/local_db/query/fetch_trades_by_tx/mod.rs (1)
49-88: ⚡ Quick winAdd one test that binds both list clauses at once.
The refactor only exercises chain IDs alone and raindex addresses alone. A case with both populated is the one that proves the second
bind_list_clausestarts numbering at the correct offset.🧪 Suggested test shape
+ #[test] + fn builds_with_chain_ids_and_raindex_address_filters() { + let tx_hash = b256!("0x00000000000000000000000000000000000000000000000000000000deadbeef"); + let ob = address!("0x2f209e5b67a33b8fe96e28f24628df6da301c8eb"); + let stmt = build_fetch_trades_by_tx_stmt(&FetchTradesByTxArgs { + chain_ids: vec![137, 1], + raindex_addresses: vec![ob], + tx_hash, + }) + .unwrap(); + + assert_eq!(stmt.params.len(), 4); + assert!(stmt.sql.contains("tws.chain_id IN (?2, ?3)")); + assert!(stmt.sql.contains("tws.raindex_address IN (?4)")); + }🤖 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/fetch_trades_by_tx/mod.rs` around lines 49 - 88, Add a new unit test that calls build_fetch_trades_by_tx_stmt with FetchTradesByTxArgs where both chain_ids and raindex_addresses are non-empty (e.g., two chain IDs and two raindex addresses) to verify bind_list_clause numbering continues across multiple list clauses; in the new test (name it something like builds_with_chain_ids_and_raindex_addresses) assert stmt.params length equals 1 + num_chain_ids + num_raindex_addresses, assert params[0] is the tx_hash Text, assert the subsequent params match the chain IDs (SqlValue::U64) followed by the raindex addresses (SqlValue::Text(hex::encode_prefixed(...))), and assert the generated SQL contains the expected IN clauses for both chain IDs and raindex addresses (e.g., "tws.chain_id IN (?2, ?3)" and "tws.raindex_address IN (?4, ?5)") and does not contain the CHAIN_IDS_CLAUSE or RAINDEXES_CLAUSE placeholders.
🤖 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/create_tables/query.sql`:
- Around line 384-395: The current set of indexes on derived_trades adds
predicates (input_token, output_token, order_owner, transaction_sender,
order_hash, transaction_hash) before the time columns which prevents efficient
use of chain/raindex/time-only queries; add a plain time-prefixed index for the
main trade-list path by creating an index (e.g., idx_derived_trades_time) on
derived_trades(chain_id, raindex_address, block_timestamp DESC, block_number
DESC, log_index DESC) alongside the other CREATE INDEX statements so
fetch_trades/query.sql can use the chain+raindex+time filter without
scanning/sorting the whole table.
In `@crates/common/src/local_db/query/upsert_derived_trades/query.sql`:
- Around line 78-104: The query's JOIN/NOT EXISTS uses order_events lookups by
oe.order_owner and oe.order_nonce (symbols: JOIN order_events oe,
oe.order_owner, oe.order_nonce, NOT EXISTS (SELECT 1 FROM order_events newer
...)), which will cause full scans for active owners; add a supporting b-tree
index to order_events to cover (chain_id, raindex_address, order_owner,
order_nonce, event_type, block_number, log_index) (suggested index name:
idx_order_events_owner_nonce_time) in create_tables/query.sql so those equality
and range predicates use the index and avoid scanning all historical orders.
---
Outside diff comments:
In `@crates/settings/src/remote/manifest.rs`:
- Around line 212-272: fetch_multiple_manifests currently collects per-URL
manifests into a map without checking db-schema-version consistency, allowing
mixed schema generations; update the fetch_multiple_manifests function to
validate that all fetched Manifest instances share the same db_schema_version
(the Manifest.db_schema_version field) and if any differ return the existing
InconsistentManifestVersions error (or a clear error variant) instead of folding
them silently into the map, ensuring the check runs after parsing each manifest
and before returning the map so callers never get mixed-schema results.
---
Nitpick comments:
In `@crates/common/src/local_db/query/fetch_trades_by_tx/mod.rs`:
- Around line 49-88: Add a new unit test that calls
build_fetch_trades_by_tx_stmt with FetchTradesByTxArgs where both chain_ids and
raindex_addresses are non-empty (e.g., two chain IDs and two raindex addresses)
to verify bind_list_clause numbering continues across multiple list clauses; in
the new test (name it something like
builds_with_chain_ids_and_raindex_addresses) assert stmt.params length equals 1
+ num_chain_ids + num_raindex_addresses, assert params[0] is the tx_hash Text,
assert the subsequent params match the chain IDs (SqlValue::U64) followed by the
raindex addresses (SqlValue::Text(hex::encode_prefixed(...))), and assert the
generated SQL contains the expected IN clauses for both chain IDs and raindex
addresses (e.g., "tws.chain_id IN (?2, ?3)" and "tws.raindex_address IN (?4,
?5)") and does not contain the CHAIN_IDS_CLAUSE or RAINDEXES_CLAUSE
placeholders.
🪄 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: 07ef357b-717f-459d-b2d9-e5b990860d2e
📒 Files selected for processing (27)
crates/cli/src/commands/local_db/pipeline/runner/manifest.rscrates/common/src/local_db/export.rscrates/common/src/local_db/pipeline/adapters/apply.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/fetch_order_trades/mod.rscrates/common/src/local_db/query/fetch_order_trades/query.sqlcrates/common/src/local_db/query/fetch_order_trades_count/mod.rscrates/common/src/local_db/query/fetch_order_trades_count/query.sqlcrates/common/src/local_db/query/fetch_owner_trades/mod.rscrates/common/src/local_db/query/fetch_owner_trades/query.sqlcrates/common/src/local_db/query/fetch_owner_trades_common.rscrates/common/src/local_db/query/fetch_owner_trades_count/mod.rscrates/common/src/local_db/query/fetch_owner_trades_count/query.sqlcrates/common/src/local_db/query/fetch_trades/count_query.sqlcrates/common/src/local_db/query/fetch_trades/mod.rscrates/common/src/local_db/query/fetch_trades/query.sqlcrates/common/src/local_db/query/fetch_trades_by_tx/mod.rscrates/common/src/local_db/query/fetch_trades_by_tx/query.sqlcrates/common/src/local_db/query/mod.rscrates/common/src/local_db/query/upsert_derived_trades/mod.rscrates/common/src/local_db/query/upsert_derived_trades/query.sqlcrates/common/src/raindex_client/trades/mod.rscrates/settings/src/local_db_manifest.rscrates/settings/src/remote/manifest.rs
✅ Files skipped from review due to trivial changes (2)
- crates/common/src/local_db/query/clear_tables/query.sql
- crates/common/src/local_db/query/fetch_order_trades/mod.rs
findolor
force-pushed
the
arda/derived-trades-schema
branch
from
710fddf to
cce428b
Compare
4 tasks
findolor
force-pushed
the
arda/derived-trades-schema
branch
from
cce428b to
79565b3
Compare
findolor
changed the base branch from
graphite-base/2587
to
local-db-manifest-urls
This was referenced May 24, 2026
JuaniRios
approved these changes
May 25, 2026
findolor
force-pushed
the
arda/derived-trades-schema
branch
from
79565b3 to
efde380
Compare
0xgleb
approved these changes
May 25, 2026
graphite-app
Bot
changed the base branch from
local-db-manifest-urls
to
graphite-base/2587
graphite-app
Bot
force-pushed
the
graphite-base/2587
branch
from
3f8691a to
7e32a38
Compare
graphite-app
Bot
force-pushed
the
arda/derived-trades-schema
branch
from
efde380 to
282ec63
Compare
graphite-app
Bot
force-pushed
the
arda/derived-trades-schema
branch
from
282ec63 to
e1eb035
Compare
findolor
added a commit
to rainlanguage/rain.local-db.remote
that referenced
this pull request
May 26, 2026
## Depends on - #8 - rainlanguage/raindex#2587 ## Motivation We want to test the deployment-id local DB remote flow with the raindex changes from rainlanguage/raindex#2587 before deploying a named sync instance. That upstream PR adds a `derived_trades` local DB read model and bumps the local DB schema to version 4. Testing it through this repository lets the remote deployment workflow build and install the CLI from the exact raindex commit that contains those schema changes. ## Solution - bump `lib/raindex` from `9b33a3c4eca44726b4602f1a2d4046982584ef93` to `cce428bf874443f55280705d8d2da157ef9921ab` - use the latest commit from rainlanguage/raindex#2587: `cce428bf874443f55280705d8d2da157ef9921ab` - include the upstream derived-trades changes needed for the local DB schema version 4 deployment test Relevant upstream raindex PR context: - PR: rainlanguage/raindex#2587 - Title: Add derived trades local DB read model - Branch: `arda/derived-trades-schema` - Latest commit: `cce428bf874443f55280705d8d2da157ef9921ab` - Main behavior: maintain `derived_trades` during local DB sync and read trade lists from that derived table ## Checks By submitting this for review, I am confirming I have done the following: - [x] made this PR as small as possible - [ ] unit-tested any new functionality - [x] linked any relevant issues or PRs - [ ] included screenshots (if this involves a front-end change) Validated locally with: - `git submodule status lib/raindex` - `git diff --stat feature/deployment-id-ops..HEAD` Not run: - no deploy was run from this branch - no local raindex build was run
findolor
force-pushed
the
arda/derived-trades-schema
branch
from
d091973 to
da40522
Compare
Merge activity
|
## Motivation Trade-list reads should use a pipeline-maintained local DB read model instead of reconstructing trades from source event tables on every request. This keeps producer bootstrap DBs and client incremental syncs on the same schema generation and avoids landing partial local DB protocol changes on `main`. ## Solution - Add `derived_trades` as the derived/read-model table for local DB trades. - Maintain `derived_trades` in the shared local DB apply pipeline in the same transaction as source event/state writes. - Rebuild derived rows by sync window while allowing order resolution to look back to older active `AddOrderV3` events. - Include the table in required-table checks, clear/reset paths, bootstrap export/import, and schema versioning. - Update trade local DB read/count queries to read from `derived_trades` instead of reconstructing from event tables. - Cover maintenance, incremental sync, bootstrap/export, clear/reset, and fetch equivalence with tests. ## Review note This was folded from the previous split stack (#2588, #2589, #2590) so the local DB schema generation lands atomically. ## Checks - `COMMIT_SHA=local nix develop -c cargo test -p raindex_app_settings --lib` - `COMMIT_SHA=local nix develop -c cargo test -p raindex_common local_db::query --lib` - `COMMIT_SHA=local nix develop -c cargo fmt --all` - `git diff --check` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a pipeline-maintained "derived trades" table with multiple indexes and an upsert refresh to speed and simplify trade queries. * Queries and exports now read from the derived-trades read model for faster, more consistent results. * **Chores** * Database schema version bumped to 4; local DBs must be migrated/reinitialized to match the new schema. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
graphite-app
Bot
force-pushed
the
arda/derived-trades-schema
branch
from
da24ba2 to
d6777c9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/common/src/local_db/query/upsert_derived_trades/mod.rs (1)
161-168: 💤 Low valuePotential overflow in
u64toi64cast.The conversion
u as i64at Line 165 can overflow for values >i64::MAX. While block numbers won't realistically exceed this limit, this is test-only code and the pattern is acceptable for integration tests.Defensive alternative
SqlValue::U64(u) => rusqlite::types::Value::Integer( - u as i64 + i64::try_from(u).expect("u64 value exceeds i64 range") ),🤖 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_trades/mod.rs` around lines 161 - 168, The match arm in sqlvalue_to_rusqlite for SqlValue::U64 does a direct cast (u as i64) which can overflow; update the SqlValue::U64 branch in function sqlvalue_to_rusqlite to defensively handle values > i64::MAX (e.g., check if u <= i64::MAX and cast, otherwise clamp to i64::MAX or choose a safe fallback) so we never perform an unchecked u64-to-i64 cast that could overflow at runtime.crates/common/src/local_db/query/fetch_owner_trades_count/mod.rs (1)
10-11: ⚡ Quick winInconsistent timestamp column qualification.
The timestamp body strings use unqualified
block_timestampwhile the corresponding code infetch_order_trades_count/mod.rs(lines 18-20) usestws.block_timestamp. Though the query will work since there's only one table, qualifying withtws.maintains consistency across the codebase.♻️ Suggested fix for consistency
-const START_TS_BODY: &str = "\nAND block_timestamp >= {param}\n"; -const END_TS_BODY: &str = "\nAND block_timestamp <= {param}\n"; +const START_TS_BODY: &str = "\nAND tws.block_timestamp >= {param}\n"; +const END_TS_BODY: &str = "\nAND tws.block_timestamp <= {param}\n";🤖 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/fetch_owner_trades_count/mod.rs` around lines 10 - 11, The constants START_TS_BODY and END_TS_BODY use an unqualified column name `block_timestamp`; update these string constants to use the qualified column `tws.block_timestamp` (i.e., replace `block_timestamp` with `tws.block_timestamp`) so they match the style used in fetch_order_trades_count (and keep consistency across queries that reference the `tws` alias).crates/common/src/local_db/query/fetch_trades/mod.rs (1)
62-63: ⚡ Quick winSimplify statement initialization to use the provided template directly.
The statement is created from
QUERY_TEMPLATEbut immediately overwritten withtemplate. This creates unnecessary overhead.♻️ Suggested fix
- let mut stmt = SqlStatement::new(QUERY_TEMPLATE); - stmt.sql = template.to_string(); + let mut stmt = SqlStatement::new(template);🤖 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/fetch_trades/mod.rs` around lines 62 - 63, Replace the two-step initialization that creates SqlStatement from QUERY_TEMPLATE then overwrites stmt.sql with template by initializing it directly from template; specifically, stop constructing with QUERY_TEMPLATE and instead create the SqlStatement using template (e.g., call SqlStatement::new(template.to_string()) or otherwise pass template into the constructor) so you no longer set stmt.sql after construction (remove usage of QUERY_TEMPLATE and the stmt.sql assignment).
🤖 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/webapp/src/lib/constants.ts`:
- Around line 1-2: The PR is missing the required built-webapp screenshot for
frontend changes; attach a screenshot of the built webapp that shows the effect
of your change (e.g., capture the app page after building and running the site)
to the PR so reviewers can verify UI output; reference the frontend change in
packages/webapp/src/lib/constants.ts (REGISTRY_URL) in the screenshot or PR
description, and add the image file to the PR (upload to the PR conversation or
include under the repo's doc/artifacts folder and link it) before merging.
---
Nitpick comments:
In `@crates/common/src/local_db/query/fetch_owner_trades_count/mod.rs`:
- Around line 10-11: The constants START_TS_BODY and END_TS_BODY use an
unqualified column name `block_timestamp`; update these string constants to use
the qualified column `tws.block_timestamp` (i.e., replace `block_timestamp` with
`tws.block_timestamp`) so they match the style used in fetch_order_trades_count
(and keep consistency across queries that reference the `tws` alias).
In `@crates/common/src/local_db/query/fetch_trades/mod.rs`:
- Around line 62-63: Replace the two-step initialization that creates
SqlStatement from QUERY_TEMPLATE then overwrites stmt.sql with template by
initializing it directly from template; specifically, stop constructing with
QUERY_TEMPLATE and instead create the SqlStatement using template (e.g., call
SqlStatement::new(template.to_string()) or otherwise pass template into the
constructor) so you no longer set stmt.sql after construction (remove usage of
QUERY_TEMPLATE and the stmt.sql assignment).
In `@crates/common/src/local_db/query/upsert_derived_trades/mod.rs`:
- Around line 161-168: The match arm in sqlvalue_to_rusqlite for SqlValue::U64
does a direct cast (u as i64) which can overflow; update the SqlValue::U64
branch in function sqlvalue_to_rusqlite to defensively handle values > i64::MAX
(e.g., check if u <= i64::MAX and cast, otherwise clamp to i64::MAX or choose a
safe fallback) so we never perform an unchecked u64-to-i64 cast that could
overflow at runtime.
🪄 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: d7f3b189-7381-4536-8d56-a763f8f6b780
📒 Files selected for processing (28)
crates/common/src/local_db/export.rscrates/common/src/local_db/pipeline/adapters/apply.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/fetch_order_trades/mod.rscrates/common/src/local_db/query/fetch_order_trades/query.sqlcrates/common/src/local_db/query/fetch_order_trades_count/mod.rscrates/common/src/local_db/query/fetch_order_trades_count/query.sqlcrates/common/src/local_db/query/fetch_owner_trades/mod.rscrates/common/src/local_db/query/fetch_owner_trades/query.sqlcrates/common/src/local_db/query/fetch_owner_trades_common.rscrates/common/src/local_db/query/fetch_owner_trades_count/mod.rscrates/common/src/local_db/query/fetch_owner_trades_count/query.sqlcrates/common/src/local_db/query/fetch_trades/count_query.sqlcrates/common/src/local_db/query/fetch_trades/mod.rscrates/common/src/local_db/query/fetch_trades/query.sqlcrates/common/src/local_db/query/fetch_trades_by_tx/mod.rscrates/common/src/local_db/query/fetch_trades_by_tx/query.sqlcrates/common/src/local_db/query/mod.rscrates/common/src/local_db/query/upsert_derived_trades/mod.rscrates/common/src/local_db/query/upsert_derived_trades/query.sqlcrates/common/src/raindex_client/trades/mod.rscrates/settings/src/local_db_manifest.rscrates/settings/src/remote/manifest.rspackages/webapp/src/lib/constants.ts
✅ Files skipped from review due to trivial changes (2)
- crates/common/src/local_db/query/fetch_order_trades/mod.rs
- crates/settings/src/remote/manifest.rs
Comment on lines
1
to
+2
| export const REGISTRY_URL = | ||
| "https://raw.githubusercontent.com/rainlanguage/rain.strategies/1a4cd6e3f30b3a66d30501743d4282054d5c66fd/registry"; | ||
| "https://raw.githubusercontent.com/rainlanguage/rain.strategies/96007c4d5a06cad390b67112a55f8a892dad29fc/registry"; |
Contributor
There was a problem hiding this comment.
Missing required frontend screenshot artifact.
Please attach a screenshot of the built webapp for this frontend change before merge.
As per coding guidelines, "packages/{webapp,ui-components}/**/*.{svelte,ts,tsx,js,jsx}: When modifying frontend code or functionality, you MUST provide a screenshot of the built webapp reflecting your change."
🤖 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 `@packages/webapp/src/lib/constants.ts` around lines 1 - 2, The PR is missing
the required built-webapp screenshot for frontend changes; attach a screenshot
of the built webapp that shows the effect of your change (e.g., capture the app
page after building and running the site) to the PR so reviewers can verify UI
output; reference the frontend change in packages/webapp/src/lib/constants.ts
(REGISTRY_URL) in the screenshot or PR description, and add the image file to
the PR (upload to the PR conversation or include under the repo's doc/artifacts
folder and link it) before merging.
Contributor
|
@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:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. SIZE=L |
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
3 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.
Motivation
Trade-list reads should use a pipeline-maintained local DB read model instead of reconstructing trades from source event tables on every request. This keeps producer bootstrap DBs and client incremental syncs on the same schema generation and avoids landing partial local DB protocol changes on
main.Solution
derived_tradesas the derived/read-model table for local DB trades.derived_tradesin the shared local DB apply pipeline in the same transaction as source event/state writes.AddOrderV3events.derived_tradesinstead of reconstructing from event tables.Review note
This was folded from the previous split stack (#2588, #2589, #2590) so the local DB schema generation lands atomically.
Checks
COMMIT_SHA=local nix develop -c cargo test -p raindex_app_settings --libCOMMIT_SHA=local nix develop -c cargo test -p raindex_common local_db::query --libCOMMIT_SHA=local nix develop -c cargo fmt --allgit diff --checkSummary by CodeRabbit
Release Notes
New Features
Chores
rain.strategiesresource.