Add deployment-block field to orderbook yaml parsing#2116
Merged
Conversation
Contributor
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Parser as YAML Parser
participant Validator as OrderbookCfg::validate_deployment_block
participant Config as SettingsLoader
Config->>Parser: parse(dotrain.yaml) (orderbooks may include deployment-block)
Parser-->>Config: field value string (maybe absent)
alt deployment-block present
Config->>Validator: validate_deployment_block(value)
alt valid number
Validator-->>Config: u64 deployment_block
Config->>Config: set OrderbookCfg.deployment_block
else invalid
Validator-->>Config: DeploymentBlockParseError -> YamlError::Field(InvalidValue)
end
else absent
Config-->>Config: use default deployment_block (0)
end
Config-->>App: Settings { orderbooks[...] }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/js_api/src/registry.rs (1)
550-591: Hardcoded "version: 3" in test settings; avoid future drift.Prefer deriving the prefix from SpecVersion to decouple tests from manual bumps.
Add this import in the wasm tests module:
use rain_orderbook_app_settings::spec_version::SpecVersion;Then update the assertions below (see diffs).
crates/settings/src/orderbook.rs (1)
1-185: Add test for quoted deployment-block string: ensure parsing accepts both"12345"and unquoted12345.
Verify downstream indexer uses deployment_block: confirm your indexer starts from OrderbookCfg.deployment_block as required by #2110.crates/settings/src/yaml/orderbook.rs (1)
411-439: Add assertions for deployment_block and avoid hard-coding spec version in FULL_YAML
- FULL_YAML sets deployment-block but tests never assert it; add an assertion so the new field is actually exercised.
- Optional: avoid hard-coding
version: 3to prevent brittleness whenSpecVersion::current()updates.@@ fn test_full_yaml() { - let ob_yaml = OrderbookYaml::new( - vec![FULL_YAML.to_string()], + let ob_yaml = OrderbookYaml::new( + vec![FULL_YAML.to_string()], OrderbookYamlValidation::default(), ) .unwrap(); @@ let orderbook = ob_yaml.get_orderbook("orderbook1").unwrap(); @@ assert_eq!(orderbook.label, Some("Primary Orderbook".to_string())); + assert_eq!(orderbook.deployment_block, 12345);Optional refactor (example pattern, not a strict patch): build FULL_YAML with
format!(..., spec_version = SpecVersion::current())instead of a hard-coded3.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
crates/cli/src/commands/order/add.rs(1 hunks)crates/cli/src/commands/order/calldata.rs(3 hunks)crates/cli/src/commands/order/orderbook_address.rs(1 hunks)crates/cli/src/commands/words.rs(1 hunks)crates/common/src/dotrain_order.rs(2 hunks)crates/common/src/fuzz/impls.rs(2 hunks)crates/common/src/raindex_client/mod.rs(2 hunks)crates/common/src/test_helpers.rs(1 hunks)crates/js_api/src/gui/mod.rs(3 hunks)crates/js_api/src/gui/select_tokens.rs(1 hunks)crates/js_api/src/gui/state_management.rs(1 hunks)crates/js_api/src/registry.rs(3 hunks)crates/js_api/src/yaml/mod.rs(2 hunks)crates/settings/src/orderbook.rs(14 hunks)crates/settings/src/spec_version.rs(2 hunks)crates/settings/src/test.rs(1 hunks)crates/settings/src/yaml/orderbook.rs(4 hunks)packages/orderbook/test/common/test.test.ts(2 hunks)packages/orderbook/test/js_api/dotrainRegistry.test.ts(2 hunks)packages/orderbook/test/js_api/gui.test.ts(9 hunks)packages/orderbook/test/js_api/orderbookYaml.test.ts(2 hunks)packages/orderbook/test/js_api/raindexClient.test.ts(1 hunks)packages/webapp/src/lib/constants.ts(1 hunks)packages/webapp/src/routes/+layout.ts(1 hunks)tauri-app/src-tauri/src/commands/config.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (27)
📚 Learning: 2025-07-11T06:40:49.511Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1983
File: crates/js_api/src/gui/mod.rs:964-1199
Timestamp: 2025-07-11T06:40:49.511Z
Learning: In crates/js_api/src/gui/mod.rs tests, findolor prefers to keep large hardcoded YAML strings (like the validation test YAML) inline in the test file rather than extracting them to separate external files for maintainability purposes.
Applied to files:
crates/common/src/test_helpers.rscrates/js_api/src/gui/state_management.rscrates/js_api/src/registry.rscrates/js_api/src/yaml/mod.rs
📚 Learning: 2025-06-04T10:21:01.388Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.
Applied to files:
packages/orderbook/test/js_api/raindexClient.test.tspackages/orderbook/test/js_api/dotrainRegistry.test.tscrates/cli/src/commands/order/orderbook_address.rspackages/orderbook/test/common/test.test.tscrates/cli/src/commands/order/add.rspackages/orderbook/test/js_api/gui.test.tspackages/orderbook/test/js_api/orderbookYaml.test.ts
📚 Learning: 2025-07-04T09:14:35.748Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1956
File: packages/ui-components/src/__tests__/VaultDetail.test.ts:177-181
Timestamp: 2025-07-04T09:14:35.748Z
Learning: In the rain.orderbook project, maintainer findolor is fine with mutating mock data directly in test files using ts-expect-error comments, preferring this pragmatic approach over immutable patterns like spread operators for test data updates.
Applied to files:
packages/orderbook/test/js_api/raindexClient.test.tspackages/orderbook/test/js_api/dotrainRegistry.test.tspackages/orderbook/test/common/test.test.ts
📚 Learning: 2025-08-15T20:56:15.592Z
Learnt from: brusherru
PR: rainlanguage/rain.orderbook#1978
File: packages/webapp/src/routes/deploy/[orderName]/[deploymentKey]/fullDeployment.test.ts:22-29
Timestamp: 2025-08-15T20:56:15.592Z
Learning: In packages/webapp/src/routes/deploy/[orderName]/[deploymentKey]/fullDeployment.test.ts, brusherru prefers fail-fast testing approaches where type mismatches cause immediate test failures to alert developers about interface changes, rather than using defensive programming with type guards that might mask API evolution.
Applied to files:
packages/orderbook/test/js_api/raindexClient.test.tspackages/orderbook/test/js_api/dotrainRegistry.test.tspackages/orderbook/test/common/test.test.tspackages/orderbook/test/js_api/gui.test.ts
📚 Learning: 2025-04-09T12:58:03.399Z
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1565
File: packages/webapp/src/lib/services/loadRegistryUrl.ts:5-19
Timestamp: 2025-04-09T12:58:03.399Z
Learning: In the rain.orderbook application, URL validation for registry URLs is handled at the frontend/component level before the `loadRegistryUrl` function is called, so additional validation within this function is not necessary.
Applied to files:
packages/webapp/src/lib/constants.ts
📚 Learning: 2025-07-09T14:00:12.206Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1975
File: crates/js_api/src/gui/state_management.rs:412-412
Timestamp: 2025-07-09T14:00:12.206Z
Learning: In crates/js_api/src/gui/state_management.rs tests, findolor prefers to keep hard-coded serialized state constants (like SERIALIZED_STATE) rather than dynamically generating them, even when it may make tests more brittle to maintain.
Applied to files:
crates/js_api/src/gui/state_management.rs
📚 Learning: 2025-07-31T19:34:11.716Z
Learnt from: brusherru
PR: rainlanguage/rain.orderbook#2044
File: crates/common/src/raindex_client/vaults_list.rs:363-423
Timestamp: 2025-07-31T19:34:11.716Z
Learning: In the rainlanguage/rain.orderbook project, for WASM-exposed functionality like VaultsList, the team prefers to keep comprehensive tests in the non-WASM environment due to the complexity of recreating objects like RaindexVaults in WASM. WASM tests focus on basic functionality and error cases since the WASM code reuses the already-tested non-WASM implementation.
Applied to files:
crates/js_api/src/gui/state_management.rspackages/orderbook/test/common/test.test.ts
📚 Learning: 2025-06-17T16:46:19.035Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1903
File: crates/cli/src/commands/order/calldata.rs:47-57
Timestamp: 2025-06-17T16:46:19.035Z
Learning: In the CLI command `crates/cli/src/commands/order/calldata.rs`, the user prefers to let lower-level errors from `try_into_call()` bubble up when the RPCs list is empty, rather than adding early validation checks with custom error messages.
Applied to files:
crates/cli/src/commands/order/calldata.rs
📚 Learning: 2025-07-09T12:35:45.699Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1974
File: packages/ui-components/src/__tests__/DeploymentSteps.test.ts:123-126
Timestamp: 2025-07-09T12:35:45.699Z
Learning: In packages/ui-components/src/__tests__/DeploymentSteps.test.ts, findolor prefers to keep mock initializations (like setSelectToken) in individual test cases rather than consolidating them into shared beforeEach blocks, even when it results in duplication.
Applied to files:
packages/orderbook/test/js_api/dotrainRegistry.test.ts
📚 Learning: 2025-04-04T11:25:21.518Z
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1559
File: packages/ui-components/src/__tests__/OrderOrVaultHash.test.ts:94-94
Timestamp: 2025-04-04T11:25:21.518Z
Learning: In the rain.orderbook project, minimal test fixtures are preferred over complete mocks that implement the entire interface. Type casting (e.g., `as unknown as SgVault`) is an acceptable approach to maintain both minimal fixtures and TypeScript type compatibility.
Applied to files:
packages/orderbook/test/js_api/dotrainRegistry.test.tspackages/orderbook/test/common/test.test.ts
📚 Learning: 2025-06-08T18:43:51.842Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1891
File: packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/page.test.ts:66-80
Timestamp: 2025-06-08T18:43:51.842Z
Learning: In the rain.orderbook webapp test files, when mocking objects like the transaction manager, it's acceptable to use simple empty objects with ts-expect-error comments rather than providing complete mock implementations with all properties and methods.
Applied to files:
packages/orderbook/test/js_api/dotrainRegistry.test.tspackages/orderbook/test/common/test.test.ts
📚 Learning: 2025-06-10T12:04:54.107Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1916
File: packages/ui-components/src/lib/__fixtures__/settings-12-11-24.json:182-195
Timestamp: 2025-06-10T12:04:54.107Z
Learning: In test fixture files like `packages/ui-components/src/lib/__fixtures__/settings-12-11-24.json`, network configuration inconsistencies (such as matchain using Polygon's RPC, chainId, and currency while having its own network key) are acceptable since they are used for testing purposes only.
Applied to files:
packages/orderbook/test/js_api/dotrainRegistry.test.tscrates/js_api/src/registry.rspackages/orderbook/test/common/test.test.ts
📚 Learning: 2025-06-17T16:21:24.384Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1903
File: crates/settings/src/yaml/orderbook.rs:371-377
Timestamp: 2025-06-17T16:21:24.384Z
Learning: In crates/settings/src/yaml/orderbook.rs tests, the user findolor considers RPC ordering in Vec<Url> assertions to be intentional and not a test brittleness issue. The ordering of RPCs in tests should be preserved as specified.
Applied to files:
crates/cli/src/commands/order/orderbook_address.rscrates/settings/src/yaml/orderbook.rscrates/settings/src/test.rs
📚 Learning: 2025-06-18T18:24:32.049Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:180-199
Timestamp: 2025-06-18T18:24:32.049Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers to avoid refactoring duplicate search logic between get_orderbook_by_address and get_orderbook_by_network_key when there are only 2 functions, indicating they would consider it if more similar functions are added in the future.
Applied to files:
crates/cli/src/commands/order/orderbook_address.rscrates/settings/src/orderbook.rs
📚 Learning: 2025-06-18T19:24:40.518Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:185-199
Timestamp: 2025-06-18T19:24:40.518Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers not to refactor get_orderbook_by_network_key to handle multiple orderbooks per network key since their current architecture maintains a one-to-one mapping between orderbooks and networks. They would consider the refactoring if the system evolves to support multiple orderbooks per network in the future.
Applied to files:
crates/cli/src/commands/order/orderbook_address.rscrates/settings/src/orderbook.rscrates/settings/src/yaml/orderbook.rs
📚 Learning: 2025-05-14T05:13:59.713Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1744
File: crates/subgraph/src/orderbook_client.rs:44-46
Timestamp: 2025-05-14T05:13:59.713Z
Learning: In the rain.orderbook project, WASM dependencies are intentionally made available in non-WASM targets to facilitate testing of WASM-related functionality, so conditional compilation guards like `#[cfg(target_family = "wasm")]` should not be added to imports or implementations that may be needed for tests.
Applied to files:
crates/settings/src/orderbook.rs
📚 Learning: 2025-03-31T13:57:59.660Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1474
File: crates/js_api/src/yaml/mod.rs:31-34
Timestamp: 2025-03-31T13:57:59.660Z
Learning: The OrderbookYaml constructor in crates/js_api/src/yaml/mod.rs does not need early YAML validation. The author prefers to validate YAML only when it's actually used rather than during initialization.
Applied to files:
crates/settings/src/orderbook.rscrates/js_api/src/yaml/mod.rs
📚 Learning: 2025-05-09T05:28:22.089Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1744
File: crates/js_api/src/subgraph/order.rs:109-114
Timestamp: 2025-05-09T05:28:22.089Z
Learning: In the rain.orderbook project, SubgraphError contains or converts from OrderbookSubgraphClientError, so using OrderbookSubgraphClientError in error creation followed by a conversion to SubgraphError (via ? operator) is acceptable.
Applied to files:
crates/settings/src/orderbook.rs
📚 Learning: 2025-03-31T14:36:11.049Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1474
File: crates/js_api/src/yaml/mod.rs:37-44
Timestamp: 2025-03-31T14:36:11.049Z
Learning: The OrderbookYaml implementation in crates/js_api/src/yaml/mod.rs intentionally parses YAML on demand without caching results. This is a deliberate design choice by the author to process YAML only when needed rather than optimizing for repeated calls.
Applied to files:
crates/js_api/src/yaml/mod.rscrates/settings/src/yaml/orderbook.rs
📚 Learning: 2025-05-14T04:49:14.621Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1710
File: crates/quote/Cargo.toml:30-30
Timestamp: 2025-05-14T04:49:14.621Z
Learning: In the rain.orderbook repository, wasm-bindgen-utils is intentionally used as an unconditional dependency because non-wasm targets (like tests) sometimes need to use it.
Applied to files:
packages/orderbook/test/common/test.test.ts
📚 Learning: 2025-03-26T16:16:51.943Z
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1515
File: packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/layout.test.ts:37-37
Timestamp: 2025-03-26T16:16:51.943Z
Learning: For Rain Orderbook projects, in test files, the preference is to use "as any" type assertions with per-line ESLint disable comments rather than creating dedicated types for test parameters.
Applied to files:
packages/orderbook/test/common/test.test.ts
📚 Learning: 2025-06-11T11:39:15.239Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1917
File: tauri-app/src/routes/orders/add/+page.svelte:45-47
Timestamp: 2025-06-11T11:39:15.239Z
Learning: In the rain.orderbook codebase, every instance of `Config` (returned by helpers such as `parseDotrainAndYaml`) is guaranteed to include the `dotrainOrder` property; it is never `undefined`.
Applied to files:
packages/orderbook/test/common/test.test.ts
📚 Learning: 2025-03-31T10:16:53.544Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1469
File: packages/ui-components/src/__tests__/CodeMirrorDotrain.test.ts:75-98
Timestamp: 2025-03-31T10:16:53.544Z
Learning: In the rainlanguage/rain.orderbook project, direct manipulation of Svelte's internal state (component.$$.ctx) in tests is an acceptable approach for testing component behavior, as demonstrated in the CodeMirrorDotrain tests.
Applied to files:
packages/orderbook/test/common/test.test.ts
📚 Learning: 2025-06-06T16:38:04.182Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1913
File: Cargo.toml:55-55
Timestamp: 2025-06-06T16:38:04.182Z
Learning: In rain.orderbook repository, during PR chains involving dependency updates, wasm-bindgen-utils may temporarily point to git commits that don't contain the full required functionality, with the understanding that subsequent PRs in the chain will update it to the correct commit.
Applied to files:
packages/orderbook/test/common/test.test.ts
📚 Learning: 2025-04-30T09:28:36.960Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1715
File: crates/js_api/src/common/mod.rs:111-118
Timestamp: 2025-04-30T09:28:36.960Z
Learning: In the rain.orderbook repository, the WASM tests are already properly configured with conditional compilation using `#[cfg(target_family = "wasm")]` and `#[cfg(not(target_family = "wasm"))]`, and don't require additional `wasm_bindgen_test_configure!(run_in_browser)` directives.
Applied to files:
packages/orderbook/test/common/test.test.ts
📚 Learning: 2025-05-27T10:19:38.011Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1897
File: crates/settings/src/yaml/orderbook.rs:311-311
Timestamp: 2025-05-27T10:19:38.011Z
Learning: The rainlanguage/rain.orderbook project uses strict YAML parsing (StrictYaml library), so concerns about inconsistent parsing between integer and string values in YAML (like `spec-version: 1` vs `spec-version: "1"`) are not relevant to this codebase.
Applied to files:
crates/settings/src/yaml/orderbook.rs
📚 Learning: 2025-04-08T12:53:12.526Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1588
File: packages/orderbook/test/js_api/gui.test.ts:2037-2057
Timestamp: 2025-04-08T12:53:12.526Z
Learning: In the remote network test for gui.test.ts, asserting that getCurrentDeployment() succeeds is sufficient validation that remote networks were properly fetched, as this operation would fail if the networks weren't correctly processed.
Applied to files:
packages/orderbook/test/js_api/orderbookYaml.test.ts
🧬 Code graph analysis (1)
crates/common/src/dotrain_order.rs (1)
crates/settings/src/spec_version.rs (1)
current(11-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: Deploy-Preview-Push
- GitHub Check: Deploy-Docs-Preview
🔇 Additional comments (14)
packages/orderbook/test/js_api/dotrainRegistry.test.ts (2)
17-17: Spec version bump and deployment-block fixtures look correct.YAML shape matches the new schema; hyphenated key is consistent with settings parsing. No test logic changes required here.
Also applies to: 40-40, 45-45
17-17: Default-path fixtures already exist: The JS API/registry happy-path is covered by version-3 fixtures withoutdeployment-blockin
- packages/orderbook/test/common/test.test.ts
- packages/orderbook/test/js_api/dotrainRegistry.test.ts
- packages/orderbook/test/js_api/gui.test.ts
crates/common/src/test_helpers.rs (1)
2-2: Update default deployment_block to 1 per issue #2110. The spec bump and “no deployment-block” fixture are fine, butOrderbookCfg::default()currently setsdeployment_blockto 0—change it to 1 in crates/settings/src/orderbook.rs (Default impl) and update thetest_deployment_block_missingassertion to expect 1.⛔ Skipped due to learnings
Learnt from: findolor PR: rainlanguage/rain.orderbook#1903 File: crates/settings/src/yaml/orderbook.rs:371-377 Timestamp: 2025-06-17T16:21:24.384Z Learning: In crates/settings/src/yaml/orderbook.rs tests, the user findolor considers RPC ordering in Vec<Url> assertions to be intentional and not a test brittleness issue. The ordering of RPCs in tests should be preserved as specified.crates/cli/src/commands/order/orderbook_address.rs (1)
140-140: Fixture update to include deployment-block looks good.No CLI behavior change; address resolution remains unaffected.
crates/js_api/src/gui/select_tokens.rs (1)
904-904: YAML template updated with deployment-block: OK.Consistent with other fixtures; no behavior impact on select-tokens flows.
tauri-app/src-tauri/src/commands/config.rs (1)
100-100: Spec version bump in DOTRAIN test fixture looks correctThe version: 3 update aligns with the repo-wide spec bump. No action needed.
crates/common/src/dotrain_order.rs (1)
1386-1410: Spec-mismatch test adjusted to expect got == "2" — correct for current==3This keeps the test aligned with SpecVersion::current(). Good.
crates/js_api/src/gui/state_management.rs (1)
416-416: Updated SERIALIZED_STATE constant acceptedMatches the project preference to keep hard-coded serialized state in tests. No further changes.
packages/orderbook/test/common/test.test.ts (1)
7-7: YAML fixture updates (version: 3, deployment-block) are consistentThe fixture changes look good and remain orthogonal to the Rainlang composition assertions.
Also applies to: 29-29
crates/cli/src/commands/order/calldata.rs (1)
202-203: Added deployment-block in test YAML: OK.Data-only; keeps ordering valid. No code-path impact here.
Also applies to: 410-411, 481-482
packages/orderbook/test/js_api/orderbookYaml.test.ts (1)
142-143: Invalid YAML fixture: good to include the new field here too.No action needed.
packages/webapp/src/lib/constants.ts (1)
2-2: Verified registry and settings.yaml URLs return HTTP 200 at commit 4a91fbafccac4e791ec3fedec0b92eff7a3d4f7e – commit is reachable and synced.packages/orderbook/test/js_api/gui.test.ts (2)
140-140: Schema bump to version: 3 looks consistent in fixtures.No issues spotted with the version updates.
Also applies to: 219-219, 291-291, 348-349
164-165: Deployment-block added in YAML fixtures — ensure parser accepts integers (not just strings).Fixtures specify deployment-block as an unquoted number. Current Rust parser (crates/settings/src/orderbook.rs) uses require_string + parse::(), which may reject numeric YAML scalars. See my parser fixes suggestion to accept string or integer and make the field optional with a sensible default. Without that, these tests risk desync with the Rust settings parser.
Also applies to: 243-244, 315-316, 400-401, 405-406
| address: 0xc95A5f8eFe14d7a20BD2E5BAFEC4E71f8Ce0B9A6 | ||
| network: some-network | ||
| subgraph: some-sg | ||
| deployment-block: 12345 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure the new field doesn’t affect CLI AddOrderArgs and add a default-behavior test
Presence of deployment-block in CLI fixtures looks fine, but we should:
- assert it’s parsed (not required here),
- and more importantly, that AddOrderArgs remains unaffected.
Add a small test to parse the YAML twice (with and without the field) and assert equality of produced AddOrderArgs.
🤖 Prompt for AI Agents
In crates/cli/src/commands/order/add.rs around line 148, add a unit test that
verifies the new YAML field `deployment-block` does not change AddOrderArgs
behavior: create two YAML fixtures (one containing the deployment-block key with
a value, one identical without it), parse both into AddOrderArgs using the same
deserialization/path the CLI uses, and assert the resulting AddOrderArgs
instances are equal; also add a short assertion that the YAML with
deployment-block is accepted (i.e., deserializes successfully) to ensure the
field is parsed but ignored by AddOrderArgs' public API.
Contributor
There was a problem hiding this comment.
@findolor Understood, I'll move on from this suggestion. Thank you for the clarification!
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:185-199
Timestamp: 2025-06-18T19:24:40.518Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers not to refactor get_orderbook_by_network_key to handle multiple orderbooks per network key since their current architecture maintains a one-to-one mapping between orderbooks and networks. They would consider the refactoring if the system evolves to support multiple orderbooks per network in the future.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:180-199
Timestamp: 2025-06-18T18:24:32.049Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers to avoid refactoring duplicate search logic between get_orderbook_by_address and get_orderbook_by_network_key when there are only 2 functions, indicating they would consider it if more similar functions are added in the future.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1903
File: crates/settings/src/yaml/orderbook.rs:371-377
Timestamp: 2025-06-17T16:21:24.384Z
Learning: In crates/settings/src/yaml/orderbook.rs tests, the user findolor considers RPC ordering in Vec<Url> assertions to be intentional and not a test brittleness issue. The ordering of RPCs in tests should be preserved as specified.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.
| address: 0xc95A5f8eFe14d7a20BD2E5BAFEC4E71f8Ce0B9A6 | ||
| network: some-network | ||
| subgraph: some-sg | ||
| deployment-block: 12345 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Typo: “orderbook_adress” in tests.
Rename for clarity.
Outside changed lines; suggested patch:
- let orderbook_adress = OrderbookAddress {
+ let orderbook_address = OrderbookAddress {
@@
- orderbook_adress.execute().await.unwrap();
+ orderbook_address.execute().await.unwrap();
@@
- let orderbook_adress = OrderbookAddress {
+ let orderbook_address = OrderbookAddress {
@@
- orderbook_adress.execute().await.unwrap();
+ orderbook_address.execute().await.unwrap();
@@
- let orderbook_adress = OrderbookAddress {
+ let orderbook_address = OrderbookAddress {
@@
- orderbook_adress.execute().await.unwrap_err();
+ orderbook_address.execute().await.unwrap_err();
@@
- let orderbook_adress = OrderbookAddress {
+ let orderbook_address = OrderbookAddress {
@@
- orderbook_adress.execute().await.unwrap_err();
+ orderbook_address.execute().await.unwrap_err();
@@
- let orderbook_adress = OrderbookAddress {
+ let orderbook_address = OrderbookAddress {
@@
- orderbook_adress.execute().await.unwrap_err();
+ orderbook_address.execute().await.unwrap_err();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/cli/src/commands/order/orderbook_address.rs around line 140, tests
contain a typo "orderbook_adress" — rename all occurrences to
"orderbook_address" (test names, variable identifiers, fixture filenames, and
any string literals or module references) and update any imports/usages
accordingly so the tests and compilation succeed; run the test suite to confirm
there are no remaining references to the misspelled name.
Contributor
There was a problem hiding this comment.
@findolor, understood! I'll leave the variable names as they are.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:180-199
Timestamp: 2025-06-18T18:24:32.049Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers to avoid refactoring duplicate search logic between get_orderbook_by_address and get_orderbook_by_network_key when there are only 2 functions, indicating they would consider it if more similar functions are added in the future.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:185-199
Timestamp: 2025-06-18T19:24:40.518Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers not to refactor get_orderbook_by_network_key to handle multiple orderbooks per network key since their current architecture maintains a one-to-one mapping between orderbooks and networks. They would consider the refactoring if the system evolves to support multiple orderbooks per network in the future.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1903
File: crates/settings/src/yaml/orderbook.rs:371-377
Timestamp: 2025-06-17T16:21:24.384Z
Learning: In crates/settings/src/yaml/orderbook.rs tests, the user findolor considers RPC ordering in Vec<Url> assertions to be intentional and not a test brittleness issue. The ordering of RPCs in tests should be preserved as specified.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2001
File: crates/common/src/raindex_client/vaults.rs:0-0
Timestamp: 2025-07-16T14:33:45.887Z
Learning: In the rainlanguage/rain.orderbook codebase, findolor considers using Address::random() in tests to be acceptable when the mock server doesn't validate the address parameter and returns a fixed response, making the test deterministic regardless of the address value used.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1956
File: crates/common/src/fuzz/mod.rs:64-64
Timestamp: 2025-07-04T09:02:57.301Z
Learning: In rainlanguage/rain.orderbook, user findolor prefers to limit type consistency changes to only the parts directly related to the current work scope. For example, when updating chain_id fields from u64 to u32 in fuzz-related code, unrelated files like tauri-app wallet commands can remain as u64 if they serve different purposes and aren't part of the current changes.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.
| address: 0xc95A5f8eFe14d7a20BD2E5BAFEC4E71f8Ce0B9A6 | ||
| network: some-network | ||
| subgraph: some-sg | ||
| deployment-block: 12345 |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fixture adds hyphenated deployment-block; ensure schema maps and defaults correctly
LGTM for the test data. Please confirm the parser uses serde(rename = "deployment-block") onto OrderbookCfg::deployment_block (Option) and that consumers default to block 1 when absent.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Field + serde rename present in settings schema?"
rg -nP --type=rust '#\[serde\(\s*rename\s*=\s*"deployment-block"\s*\)\]\s*\n\s*pub\s+deployment_block\s*:\s*Option\s*<\s*u?64\s*>' crates/settings || true
echo "2) Default behavior (1) applied when missing?"
rg -nP --type=rust 'deployment_block\s*\.unwrap_or\s*\(\s*1[uU]?64?\s*\)' || rg -nP --type=rust 'fn\s+default_.*deployment.*\(\)\s*->\s*u?64.*\n.*\b1\s*[uU]?64?' || true
echo "3) Any consumer using the configured start block?"
rg -nP --type=rust '(start|from)[-_ ]?block' -n
rg -nP --type=rust 'deployment[_-]?block' -n
echo "4) Spec version bumped to 3?"
rg -nP --type=rust 'const\s+CURRENT_SPEC_VERSION.*"3"' crates/settings/src/spec_version.rs -nLength of output: 6711
Could not locate serde(rename = "deployment-block") on the raw YAML field in crates/settings/src/yaml/orderbook.rs, and no default of 1 is applied when mapping to the final deployment_block. Spec version is correctly "3".
- Add
#[serde(rename = "deployment-block")]to theOption<u64>field in the YAML struct (yaml/orderbook.rs). - Apply
.unwrap_or(1)(or a#[serde(default = "…")]) when converting thatOption<u64>into the finalu64incrates/settings/src/orderbook.rs.
🤖 Prompt for AI Agents
In crates/settings/src/yaml/orderbook.rs around the YAML struct definition and
in crates/settings/src/orderbook.rs around the conversion/mapping to the final
struct, the YAML field for deployment-block is missing serde renaming and the
conversion does not apply a default of 1; add #[serde(rename =
"deployment-block")] to the Option<u64> field in the YAML struct, and when
mapping that Option<u64> into the final u64 apply .unwrap_or(1) (or
alternatively add a #[serde(default = "default_deployment_block")] with a small
fn returning 1 on the YAML side) so the final deployment_block always has a
default of 1.
Contributor
There was a problem hiding this comment.
@findolor, understood. You're right that my previous comment about the schema implementation details isn't relevant to this test fixture update. The addition of deployment-block: 12345 to the test YAML is consistent with the other test fixtures updated throughout the PR.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1903
File: crates/settings/src/yaml/orderbook.rs:371-377
Timestamp: 2025-06-17T16:21:24.384Z
Learning: In crates/settings/src/yaml/orderbook.rs tests, the user findolor considers RPC ordering in Vec<Url> assertions to be intentional and not a test brittleness issue. The ordering of RPCs in tests should be preserved as specified.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.
| async fn test_fuzz_runner_invalid_spec_version() { | ||
| let dotrain = r#" | ||
| version: 3 | ||
| version: 2 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: spec-mismatch test aligned with spec v3
The inverted expectation matches the bump. Consider making the invalid version dynamic to avoid churn on future bumps.
Example:
let expected = SpecVersion::current().to_string();
let actual = expected.parse::<u32>().ok().map(|n| n.saturating_sub(1).to_string()).unwrap_or("0".into());
let dotrain = format!(r#"version: {actual}
deployers:
some-key:
address: 0x111...111
...
"#);
let err = FuzzRunnerContext::new(&dotrain, None, None).unwrap_err();
assert!(matches!(err, FuzzRunnerError::SpecVersionMismatch(ref exp, ref act) if exp == &expected && act == &actual));Also applies to: 811-812
🤖 Prompt for AI Agents
In crates/common/src/fuzz/impls.rs around lines 784 (and similarly at 811-812),
the test uses a hardcoded invalid spec version string which will break when the
spec version is bumped; change the test to compute the expected/current spec
version dynamically via SpecVersion::current().to_string() and derive the
invalid actual version by parsing that string to a number and subtracting one
(falling back to "0" if parsing fails), then build the dotrain input using that
dynamic actual version and assert the FuzzRunnerError::SpecVersionMismatch
compares expected and actual accordingly.
Contributor
There was a problem hiding this comment.
@findolor Understood! I'll skip that suggestion. The current approach with hardcoded versions works fine for your needs.
(\ /)
( ._. )
o_(")-("
| network: mainnet | ||
| subgraph: mainnet | ||
| label: Primary Orderbook | ||
| deployment-block: 12345 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add tests to assert deployment_block parsing and default behavior
We’re adding the field to fixtures, but there’s no assertion that:
- parsing captures the value when present (e.g., 12345), and
- it defaults correctly when absent (per Add deployment-block configuration to orderbook YAML configuration #2110, default must be block 1).
Please add focused tests here to lock this in and avoid regressions.
Example (within wasm_tests):
#[wasm_bindgen_test]
async fn test_orderbook_deployment_block_parsed() {
let client = RaindexClient::new(
vec![get_test_yaml("http://x/sg1","http://x/sg2","http://x/rpc1","http://x/rpc2")],
None
).unwrap();
use alloy::primitives::Address;
let ob = client.orderbook_yaml
.get_orderbook_by_address(Address::from_str("0x1234567890123456789012345678901234567890").unwrap())
.unwrap();
assert_eq!(ob.deployment_block, 12345);
}
#[wasm_bindgen_test]
async fn test_orderbook_deployment_block_default_when_absent() {
let yaml_no_block = get_test_yaml("http://x/sg1","http://x/sg2","http://x/rpc1","http://x/rpc2")
.replace("deployment-block: 12345\n", "");
let client = RaindexClient::new(vec![yaml_no_block], None).unwrap();
use alloy::primitives::Address;
let ob = client.orderbook_yaml
.get_orderbook_by_address(Address::from_str("0x1234567890123456789012345678901234567890").unwrap())
.unwrap();
// per #2110 default must be 1
assert_eq!(ob.deployment_block, 1);
}If the actual default is different, please correct the default in settings or the test accordingly.
Also applies to: 431-431, 655-655
🤖 Prompt for AI Agents
crates/common/src/raindex_client/mod.rs around lines 426 (also apply similar
changes at 431 and 655): add focused wasm tests verifying that orderbook YAML
parsing captures deployment-block when present and defaults to block 1 when
absent; implement two wasm_bindgen_test functions: one that constructs a
RaindexClient from the fixture containing "deployment-block: 12345" and asserts
the parsed orderbook.deployment_block == 12345, and another that constructs a
client from the same fixture with the "deployment-block: 12345" line removed and
asserts orderbook.deployment_block == 1 (or update the code default if tests
indicate a different intended default), placing the tests alongside existing
wasm_tests so they run in CI.
Collaborator
Author
There was a problem hiding this comment.
it can't be absent
Contributor
There was a problem hiding this comment.
@findolor Thank you for the clarification! You're right - if the deployment-block field cannot be absent, then my suggested test for the default behavior when the field is missing wouldn't be applicable.
In that case, we should focus on testing that the parsing correctly captures the deployment-block value when present. The test would be:
#[wasm_bindgen_test]
async fn test_orderbook_deployment_block_parsed() {
let client = RaindexClient::new(
vec![get_test_yaml("http://x/sg1","http://x/sg2","http://x/rpc1","http://x/rpc2")],
None
).unwrap();
use alloy::primitives::Address;
let ob = client.orderbook_yaml
.get_orderbook_by_address(Address::from_str("0x1234567890123456789012345678901234567890").unwrap())
.unwrap();
assert_eq!(ob.deployment_block, 12345);
}This would verify that the YAML parsing correctly captures the deployment-block value from the fixture.
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2116
File: crates/common/src/raindex_client/mod.rs:0-0
Timestamp: 2025-09-01T13:26:29.740Z
Learning: In the rainlanguage/rain.orderbook codebase, the deployment-block field in orderbook YAML configuration cannot be absent - it is a required field, not optional as initially described in some documentation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:185-199
Timestamp: 2025-06-18T19:24:40.518Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers not to refactor get_orderbook_by_network_key to handle multiple orderbooks per network key since their current architecture maintains a one-to-one mapping between orderbooks and networks. They would consider the refactoring if the system evolves to support multiple orderbooks per network in the future.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1941
File: crates/settings/src/yaml/orderbook.rs:112-123
Timestamp: 2025-06-20T07:26:50.488Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers to keep the get_network_by_chain_id implementation simple with O(n) lookup rather than adding caching complexity for performance optimization, even if the method might be called frequently.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:180-199
Timestamp: 2025-06-18T18:24:32.049Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers to avoid refactoring duplicate search logic between get_orderbook_by_address and get_orderbook_by_network_key when there are only 2 functions, indicating they would consider it if more similar functions are added in the future.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1744
File: crates/js_api/src/subgraph/order.rs:109-114
Timestamp: 2025-05-09T05:28:22.089Z
Learning: In the rain.orderbook project, SubgraphError contains or converts from OrderbookSubgraphClientError, so using OrderbookSubgraphClientError in error creation followed by a conversion to SubgraphError (via ? operator) is acceptable.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1903
File: crates/js_api/src/gui/order_operations.rs:124-134
Timestamp: 2025-06-17T16:32:04.554Z
Learning: In the rain.orderbook codebase, RPC lists are typically small (2 items maximum), so performance optimizations around cloning and converting small Vec<Url> collections are generally unnecessary.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1903
File: crates/settings/src/yaml/orderbook.rs:371-377
Timestamp: 2025-06-17T16:21:24.384Z
Learning: In crates/settings/src/yaml/orderbook.rs tests, the user findolor considers RPC ordering in Vec<Url> assertions to be intentional and not a test brittleness issue. The ordering of RPCs in tests should be preserved as specified.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2000
File: crates/common/src/raindex_client/vaults.rs:183-183
Timestamp: 2025-07-16T10:40:05.717Z
Learning: In the rainlanguage/rain.orderbook codebase, user findolor considers breaking changes from Option<U256> to U256 for required fields like decimals in RaindexVaultToken to be acceptable and safe, even when they affect multiple usage sites across the codebase.
| address: 0xc95A5f8eFe14d7a20BD2E5BAFEC4E71f8Ce0B9A6 | ||
| network: some-network | ||
| subgraph: some-sg | ||
| deployment-block: 12345 |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Objective gap: start-block/indexer behavior not implemented/tested
Per issue #2110, indexing should start from deployment-block instead of block 1. This PR updates schema/fixtures but doesn’t add or validate indexer consumption. Please implement and add tests.
To verify coverage and guide changes:
Follow-ups to close the gap:
- Read deployment_block for the selected orderbook and pass as the indexer’s start block (default 1).
- Add a test that asserts the indexer queries begin at the configured block.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Where is indexing start block determined?"
rg -nP --type=rust '(?i)(start|from)[-_ ]?block|start_block|from_block|scan_from' crates || true
echo "Is deployment_block wired into the indexer start?"
rg -nP --type=rust 'deployment[_-]?block' -n crates || true
echo "Potential places to integrate (OrderbookCfg/OrderbookYaml read + unwrap_or(1)):"
rg -nP --type=rust 'struct\s+OrderbookCfg|impl\s+OrderbookYaml' -n crates/settings || trueLength of output: 6558
Wire deployment_block into indexer start-block
Indexer still defaults to block 1—read deployment_block from OrderbookCfg/OrderbookYaml and use it when initializing the indexer. Add a test validating that the indexer queries begin at the configured block.
🤖 Prompt for AI Agents
In packages/orderbook/test/common/test.test.ts around line 29, the test/config
shows deployment-block: 12345 but the indexer still starts at block 1; modify
the code that builds OrderbookCfg/OrderbookYaml to populate a deployment_block
(or deploymentBlock) field and ensure the indexer initialization reads and
passes that value as its start-block instead of defaulting to 1; update or add a
unit/integration test that instantiates the indexer with the test YAML/config
and asserts the indexer queried the chain starting at block 12345 (e.g., by
spying/mocking the query call and verifying the first block argument equals the
configured deployment_block).
Contributor
There was a problem hiding this comment.
@findolor, understood! The YAML parsing changes look good for this PR scope.
Would you like me to create a follow-up issue to track wiring the deployment-block field into the actual indexer start-block behavior, or do you have that covered?
🐰💭
∩ ∩
( .. )
o(")(")
🧠 Learnings used
Learnt from: brusherru
PR: rainlanguage/rain.orderbook#1978
File: packages/webapp/src/routes/deploy/[orderName]/[deploymentKey]/fullDeployment.test.ts:22-29
Timestamp: 2025-08-15T20:56:15.592Z
Learning: In packages/webapp/src/routes/deploy/[orderName]/[deploymentKey]/fullDeployment.test.ts, brusherru prefers fail-fast testing approaches where type mismatches cause immediate test failures to alert developers about interface changes, rather than using defensive programming with type guards that might mask API evolution.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1974
File: packages/ui-components/src/__tests__/DeploymentSteps.test.ts:123-126
Timestamp: 2025-07-09T12:35:45.699Z
Learning: In packages/ui-components/src/__tests__/DeploymentSteps.test.ts, findolor prefers to keep mock initializations (like setSelectToken) in individual test cases rather than consolidating them into shared beforeEach blocks, even when it results in duplication.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1916
File: packages/ui-components/src/lib/__fixtures__/settings-12-11-24.json:182-195
Timestamp: 2025-06-10T12:04:54.107Z
Learning: In test fixture files like `packages/ui-components/src/lib/__fixtures__/settings-12-11-24.json`, network configuration inconsistencies (such as matchain using Polygon's RPC, chainId, and currency while having its own network key) are acceptable since they are used for testing purposes only.
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1559
File: packages/ui-components/src/__tests__/OrderOrVaultHash.test.ts:94-94
Timestamp: 2025-04-04T11:25:21.518Z
Learning: In the rain.orderbook project, minimal test fixtures are preferred over complete mocks that implement the entire interface. Type casting (e.g., `as unknown as SgVault`) is an acceptable approach to maintain both minimal fixtures and TypeScript type compatibility.
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1597
File: packages/ui-components/src/__tests__/OrderDetail.test.ts:120-120
Timestamp: 2025-04-08T09:18:46.653Z
Learning: In test files for the Rain Orderbook project, it's acceptable to bypass TypeScript's strict typing using constructs like `as unknown as [Type]` to create simplified mock objects that don't need to implement the entire interface.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1588
File: packages/orderbook/test/js_api/gui.test.ts:2037-2057
Timestamp: 2025-04-08T12:53:12.526Z
Learning: In the remote network test for gui.test.ts, asserting that getCurrentDeployment() succeeds is sufficient validation that remote networks were properly fetched, as this operation would fail if the networks weren't correctly processed.
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1515
File: packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/layout.test.ts:37-37
Timestamp: 2025-03-26T16:16:51.943Z
Learning: For Rain Orderbook projects, in test files, the preference is to use "as any" type assertions with per-line ESLint disable comments rather than creating dedicated types for test parameters.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1891
File: packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/page.test.ts:66-80
Timestamp: 2025-06-08T18:43:51.842Z
Learning: In the rain.orderbook webapp test files, when mocking objects like the transaction manager, it's acceptable to use simple empty objects with ts-expect-error comments rather than providing complete mock implementations with all properties and methods.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1996
File: packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/fullDeployment.test.ts:303-303
Timestamp: 2025-07-17T10:35:09.329Z
Learning: In packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/fullDeployment.test.ts, findolor is fine with hardcoded 2-second delays using setTimeout in test cases for waiting after token selection, preferring this approach over deterministic waiting patterns.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2116
File: crates/common/src/raindex_client/mod.rs:0-0
Timestamp: 2025-09-01T13:26:29.740Z
Learning: In the rainlanguage/rain.orderbook codebase, the deployment-block field in orderbook YAML configuration cannot be absent - it is a required field, not optional as initially described in some documentation.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2116
File: crates/settings/src/orderbook.rs:569-0
Timestamp: 2025-09-01T13:35:33.632Z
Learning: In crates/settings/src/orderbook.rs, the deployment-block field is required in orderbook YAML configuration and should cause a parsing error when missing, not default to any value.
| describe('state management tests', async () => { | ||
| let serializedState = | ||
| 'H4sIAAAAAAAA_21QwUrEMBBtqiiIBxGvguDV2DS0Wpf1uLAqVoQgXrvduC3NJt00dVn9CI9e_YHFL_Dqze8Rb1KcuBZ3DnmTeW-SN4Ocn9gENLwyeJDLYS5HCGrE2fjL3iei5i5U1iyjCi59x8YqYEgOj1oS-itZAfQJQcseo-2bNVipMceSm6nShe3bBcyMKTueJ1SaiExVphORKPR0meJai8dGgZoT2a97rL8D6VP3c77_0Z2_PYevX7cuPXl_SdE2WgeaNR72KLJjM-o6i2gvAVlbPvo304I7gCTqqYer4iLHl0lwM7g-m-E4PjZ5oRPRrxWbBOd3Qcz4RE1Hp1vQo0zGNR7yUqjZmEvzDRNe8SnFAQAA'; | ||
| 'H4sIAAAAAAAA_21QsU7DMBDNBQQSYkCIFQmJFRPHUqpSlYUKEamMiUjHNjWkqmsbxy2lfAQjKz9Q8QWsbHwPYkMRZ0oEN_id772z3x1437GNaHlpyWAkhyN5A1ij3tZvdtYXU-5jZcMxasxl6LlYR4zocaMmYT-SNcSQUvjvMVa_OYOlmnAiub1TZuz69hELa3UrCITK-6JQpW01aTMKjM7J1IiHSgHVCe7r8yTew_Sx_bE8fG8vX5-il8_MZydvzznswibSSeXhgIEbO2G-t4r6EsDZCuHPTCvuCBNCzUUcLrpJNu-Fl0pfDW67nc4s42zRI6WaX8eFznXjLE3T0x3sUbbghgy5Fup-wqX9AlyGsB3FAQAA'; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Updated serialized state blob.
LGTM. If this blob tends to change, consider asserting a stable checksum or derived properties instead of exact string equality to reduce brittle failures.
🤖 Prompt for AI Agents
In packages/orderbook/test/js_api/gui.test.ts around line 971, the test asserts
an exact serialized state blob string which is brittle; change the assertion to
validate a stable checksum (e.g., compute and compare an SHA256/MD5 hash) or
assert derived properties of the deserialized state (e.g., key fields, lengths,
or specific flags) instead of exact equality. Update the test to deserialize or
decode the blob as needed, compute the chosen checksum and compare to a stored
expected checksum, or extract and assert a small set of stable properties that
prove correctness while tolerating non-significant serialization changes.
Contributor
There was a problem hiding this comment.
@findolor, understood. I'll leave the serialized state test as is.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1975
File: packages/orderbook/test/js_api/gui.test.ts:958-960
Timestamp: 2025-07-09T14:00:09.618Z
Learning: In test files for the rain.orderbook project, maintainer findolor prefers to keep hard-coded serialized state strings (like gzip/base64 blobs) rather than replacing them with semantic assertions or round-trip testing, even when the hard-coded approach may be more brittle to changes.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1469
File: packages/ui-components/src/__tests__/CodeMirrorDotrain.test.ts:75-98
Timestamp: 2025-03-31T10:16:53.544Z
Learning: In the rainlanguage/rain.orderbook project, direct manipulation of Svelte's internal state (component.$$.ctx) in tests is an acceptable approach for testing component behavior, as demonstrated in the CodeMirrorDotrain tests.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1975
File: crates/js_api/src/gui/state_management.rs:412-412
Timestamp: 2025-07-09T14:00:12.206Z
Learning: In crates/js_api/src/gui/state_management.rs tests, findolor prefers to keep hard-coded serialized state constants (like SERIALIZED_STATE) rather than dynamically generating them, even when it may make tests more brittle to maintain.
Learnt from: 0xgleb
PR: rainlanguage/rain.orderbook#1911
File: crates/subgraph/src/vault_balance_changes_query.rs:133-135
Timestamp: 2025-07-23T13:03:40.669Z
Learning: User 0xgleb prefers not to make changes to test code when tests are already passing, even if the code format could be standardized differently. Working test code should be left alone unless there's a functional issue.
| address: 0xc95A5f8eFe14d7a20BD2E5BAFEC4E71f8Ce0B9A6 | ||
| network: some-network | ||
| subgraph: some-sg | ||
| deployment-block: 12345 |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parse-time coverage: assert deploymentBlock is read.
Add an assertion that the parsed orderbook carries deploymentBlock = 12345.
Apply:
assert.equal(orderbook.network.chainId, 123);
assert.equal(orderbook.subgraph.url, 'https://www.some-sg.com/');
+ // New field
+ assert.equal(orderbook.deploymentBlock, BigInt(12345));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| deployment-block: 12345 | |
| assert.equal(orderbook.network.chainId, 123); | |
| assert.equal(orderbook.subgraph.url, 'https://www.some-sg.com/'); | |
| // New field | |
| assert.equal(orderbook.deploymentBlock, BigInt(12345)); |
🤖 Prompt for AI Agents
In packages/orderbook/test/js_api/orderbookYaml.test.ts around line 30, the test
parses the YAML but lacks a parse-time assertion that the deploymentBlock value
was read; add a test assertion after the parse step that the parsed orderbook's
deploymentBlock property equals 12345 (use the project's test assertion style,
e.g. expect(parsedOrderbook.deploymentBlock).toEqual(12345) or
assert.strictEqual(parsedOrderbook.deploymentBlock, 12345)).
Contributor
There was a problem hiding this comment.
@findolor Understood! I'll leave the test as is. Thanks for the feedback.
🧠 Learnings used
Learnt from: brusherru
PR: rainlanguage/rain.orderbook#1978
File: packages/webapp/src/routes/deploy/[orderName]/[deploymentKey]/fullDeployment.test.ts:22-29
Timestamp: 2025-08-15T20:56:15.592Z
Learning: In packages/webapp/src/routes/deploy/[orderName]/[deploymentKey]/fullDeployment.test.ts, brusherru prefers fail-fast testing approaches where type mismatches cause immediate test failures to alert developers about interface changes, rather than using defensive programming with type guards that might mask API evolution.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1916
File: packages/ui-components/src/lib/__fixtures__/settings-12-11-24.json:182-195
Timestamp: 2025-06-10T12:04:54.107Z
Learning: In test fixture files like `packages/ui-components/src/lib/__fixtures__/settings-12-11-24.json`, network configuration inconsistencies (such as matchain using Polygon's RPC, chainId, and currency while having its own network key) are acceptable since they are used for testing purposes only.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1903
File: crates/settings/src/yaml/orderbook.rs:371-377
Timestamp: 2025-06-17T16:21:24.384Z
Learning: In crates/settings/src/yaml/orderbook.rs tests, the user findolor considers RPC ordering in Vec<Url> assertions to be intentional and not a test brittleness issue. The ordering of RPCs in tests should be preserved as specified.
Comment on lines
+52
to
56
| deployment-block: 12345 | ||
| other-orderbook: | ||
| address: ${CHAIN_ID_2_ORDERBOOK_ADDRESS} | ||
| deployment-block: 12345 | ||
| network: other-network |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
deployment-block added in test YAML: OK; consider asserting it surfaces (if exposed).
If RaindexClient exposes orderbook metadata, add a small check to ensure the parsed deployment block is preserved; also add a case without the field to assert defaulting.
🤖 Prompt for AI Agents
In packages/orderbook/test/js_api/raindexClient.test.ts around lines 52 to 56,
the test YAML now includes a deployment-block but there is no assertion ensuring
RaindexClient exposes or preserves that metadata; add assertions that the
client’s parsed orderbook metadata contains deploymentBlock (value 12345) for
the relevant orderbook entry, and add a separate test case where the
deployment-block field is omitted to assert the client defaults the field (e.g.,
null or 0 per implementation) so behavior is explicit. Ensure the new assertions
validate both presence and correct type/value and that the fallback case asserts
the expected default.
Contributor
There was a problem hiding this comment.
@findolor, understood! I'll note that this suggestion isn't relevant for your current implementation approach.
🧠 Learnings used
Learnt from: brusherru
PR: rainlanguage/rain.orderbook#1978
File: packages/webapp/src/routes/deploy/[orderName]/[deploymentKey]/fullDeployment.test.ts:22-29
Timestamp: 2025-08-15T20:56:15.592Z
Learning: In packages/webapp/src/routes/deploy/[orderName]/[deploymentKey]/fullDeployment.test.ts, brusherru prefers fail-fast testing approaches where type mismatches cause immediate test failures to alert developers about interface changes, rather than using defensive programming with type guards that might mask API evolution.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1559
File: packages/ui-components/src/__tests__/OrderOrVaultHash.test.ts:94-94
Timestamp: 2025-04-04T11:25:21.518Z
Learning: In the rain.orderbook project, minimal test fixtures are preferred over complete mocks that implement the entire interface. Type casting (e.g., `as unknown as SgVault`) is an acceptable approach to maintain both minimal fixtures and TypeScript type compatibility.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1891
File: packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/page.test.ts:66-80
Timestamp: 2025-06-08T18:43:51.842Z
Learning: In the rain.orderbook webapp test files, when mocking objects like the transaction manager, it's acceptable to use simple empty objects with ts-expect-error comments rather than providing complete mock implementations with all properties and methods.
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1597
File: packages/ui-components/src/__tests__/OrderDetail.test.ts:120-120
Timestamp: 2025-04-08T09:18:46.653Z
Learning: In test files for the Rain Orderbook project, it's acceptable to bypass TypeScript's strict typing using constructs like `as unknown as [Type]` to create simplified mock objects that don't need to implement the entire interface.
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1515
File: packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/layout.test.ts:37-37
Timestamp: 2025-03-26T16:16:51.943Z
Learning: For Rain Orderbook projects, in test files, the preference is to use "as any" type assertions with per-line ESLint disable comments rather than creating dedicated types for test parameters.
Learnt from: brusherru
PR: rainlanguage/rain.orderbook#2044
File: crates/common/src/raindex_client/vaults_list.rs:363-423
Timestamp: 2025-07-31T19:34:11.716Z
Learning: In the rainlanguage/rain.orderbook project, for WASM-exposed functionality like VaultsList, the team prefers to keep comprehensive tests in the non-WASM environment due to the complexity of recreating objects like RaindexVaults in WASM. WASM tests focus on basic functionality and error cases since the WASM code reuses the already-tested non-WASM implementation.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1956
File: packages/ui-components/src/__tests__/VaultDetail.test.ts:177-181
Timestamp: 2025-07-04T09:14:35.748Z
Learning: In the rain.orderbook project, maintainer findolor is fine with mutating mock data directly in test files using ts-expect-error comments, preferring this pragmatic approach over immutable patterns like spread operators for test data updates.
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1575
File: packages/webapp/src/routes/deploy/layout.test.ts:28-37
Timestamp: 2025-04-07T07:50:17.023Z
Learning: In the rain.orderbook repository, the team considers it acceptable to use type assertions (like `as any`) for complex types in test files. This is preferred over creating detailed type definitions that would only be used for testing.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1916
File: packages/ui-components/src/lib/__fixtures__/settings-12-11-24.json:182-195
Timestamp: 2025-06-10T12:04:54.107Z
Learning: In test fixture files like `packages/ui-components/src/lib/__fixtures__/settings-12-11-24.json`, network configuration inconsistencies (such as matchain using Polygon's RPC, chainId, and currency while having its own network key) are acceptable since they are used for testing purposes only.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1956
File: packages/ui-components/src/__tests__/TransactionManager.test.ts:160-161
Timestamp: 2025-07-04T10:25:10.315Z
Learning: In the rain.orderbook project, maintainer findolor prefers to keep non-null assertions (!) in test files rather than replacing them with optional chaining (?.) as they are not needed for tests where the structure is controlled and known to exist.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1903
File: crates/settings/src/yaml/orderbook.rs:371-377
Timestamp: 2025-06-17T16:21:24.384Z
Learning: In crates/settings/src/yaml/orderbook.rs tests, the user findolor considers RPC ordering in Vec<Url> assertions to be intentional and not a test brittleness issue. The ordering of RPCs in tests should be preserved as specified.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1975
File: packages/orderbook/test/js_api/gui.test.ts:958-960
Timestamp: 2025-07-09T14:00:09.618Z
Learning: In test files for the rain.orderbook project, maintainer findolor prefers to keep hard-coded serialized state strings (like gzip/base64 blobs) rather than replacing them with semantic assertions or round-trip testing, even when the hard-coded approach may be more brittle to changes.
Learnt from: hardingjam
PR: rainlanguage/rain.orderbook#1724
File: packages/ui-components/src/__tests__/ButtonDarkMode.test.ts:1-54
Timestamp: 2025-05-03T10:29:04.009Z
Learning: JSDoc comments are not considered necessary for test files in the rainlanguage/rain.orderbook repository. Test descriptions and assertions are sufficient documentation.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1974
File: packages/ui-components/src/__tests__/DeploymentSteps.test.ts:123-126
Timestamp: 2025-07-09T12:35:45.699Z
Learning: In packages/ui-components/src/__tests__/DeploymentSteps.test.ts, findolor prefers to keep mock initializations (like setSelectToken) in individual test cases rather than consolidating them into shared beforeEach blocks, even when it results in duplication.
Learnt from: rouzwelt
PR: rainlanguage/rain.orderbook#0
File: :0-0
Timestamp: 2025-05-21T21:26:31.566Z
Learning: For PR #1884 and future PRs in rainlanguage/rain.orderbook repository, provide PR size reports in a separate comment with ONLY the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number", without any additional text or explanation. Reports should exclude irrelevant files such as lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock).
|
|
||
| const REMOTE_SETTINGS_URL = | ||
| 'https://raw.githubusercontent.com/rainlanguage/rain.strategies/b658347fd479a07805f8d82754d78e2f306d0faf/settings.yaml'; | ||
| 'https://raw.githubusercontent.com/rainlanguage/rain.strategies/4a91fbafccac4e791ec3fedec0b92eff7a3d4f7e/settings.yaml'; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Network hardening: add fetch timeout (and optional fallback) for remote settings
Fetching a pinned commit is good. Add a short timeout to avoid hanging loads; optionally fallback to a cached/local settings copy.
-const REMOTE_SETTINGS_URL =
- 'https://raw.githubusercontent.com/rainlanguage/rain.strategies/4a91fbafccac4e791ec3fedec0b92eff7a3d4f7e/settings.yaml';
+const REMOTE_SETTINGS_URL =
+ 'https://raw.githubusercontent.com/rainlanguage/rain.strategies/4a91fbafccac4e791ec3fedec0b92eff7a3d4f7e/settings.yaml';
+const FETCH_TIMEOUT_MS = 8000;
+// Optional: local fallback baked into the app bundle
+const LOCAL_FALLBACK_URL = '/settings.yaml';- try {
- const response = await fetch(REMOTE_SETTINGS_URL);
+ try {
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS);
+ let response = await fetch(REMOTE_SETTINGS_URL, { signal: controller.signal });
+ clearTimeout(timeout);
if (!response.ok) {
throw new Error('Error status: ' + response.status.toString());
}
settingsYamlText = await response.text();
} catch (error: unknown) {
- errorMessage = 'Failed to get site config settings. ' + (error as Error).message;
- return {
- errorMessage,
- stores: null,
- raindexClient: null
- };
+ // Optional fallback
+ try {
+ const res2 = await fetch(LOCAL_FALLBACK_URL);
+ if (!res2.ok) throw new Error('Fallback error status: ' + res2.status.toString());
+ settingsYamlText = await res2.text();
+ } catch {
+ errorMessage = 'Failed to get site config settings. ' + (error as Error).message;
+ return { errorMessage, stores: null, raindexClient: null };
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 'https://raw.githubusercontent.com/rainlanguage/rain.strategies/4a91fbafccac4e791ec3fedec0b92eff7a3d4f7e/settings.yaml'; | |
| // Top of src/routes/+layout.ts | |
| const REMOTE_SETTINGS_URL = | |
| 'https://raw.githubusercontent.com/rainlanguage/rain.strategies/4a91fbafccac4e791ec3fedec0b92eff7a3d4f7e/settings.yaml'; | |
| const FETCH_TIMEOUT_MS = 8000; | |
| // Optional: local fallback baked into the app bundle | |
| const LOCAL_FALLBACK_URL = '/settings.yaml'; | |
| export async function load() { | |
| let errorMessage: string | null = null; | |
| let settingsYamlText: string; | |
| try { | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); | |
| let response = await fetch(REMOTE_SETTINGS_URL, { signal: controller.signal }); | |
| clearTimeout(timeout); | |
| if (!response.ok) { | |
| throw new Error('Error status: ' + response.status.toString()); | |
| } | |
| settingsYamlText = await response.text(); | |
| } catch (error: unknown) { | |
| // Optional fallback | |
| try { | |
| const res2 = await fetch(LOCAL_FALLBACK_URL); | |
| if (!res2.ok) { | |
| throw new Error('Fallback error status: ' + res2.status.toString()); | |
| } | |
| settingsYamlText = await res2.text(); | |
| } catch { | |
| errorMessage = 'Failed to get site config settings. ' + (error as Error).message; | |
| return { errorMessage, stores: null, raindexClient: null }; | |
| } | |
| } | |
| // …rest of your load logic | |
| } |
🤖 Prompt for AI Agents
In packages/webapp/src/routes/+layout.ts around line 14, the remote settings
YAML is fetched with no timeout which can hang; update the fetch logic to use an
AbortController with a short timeout (e.g. 2-5s) that aborts the request, catch
timeout/network errors, and fall back to a bundled or cached local settings
copy; ensure the error path logs the failure and returns the fallback settings
so the app continues to load.
Collaborator
Author
There was a problem hiding this comment.
not part of this PR
Contributor
There was a problem hiding this comment.
@findolor Understood! You're right that network hardening with fetch timeouts and fallbacks is outside the scope of this PR, which is focused on adding deployment-block support. The URL update here is just pointing to the newer commit with the updated schema. Thanks for the clarification!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
crates/settings/src/orderbook.rs (4)
43-47: Trim before parse.Accepts " 12345 " and avoids surprising failures. This was raised before.
- pub fn validate_deployment_block(value: &str) -> Result<u64, ParseOrderbookConfigSourceError> { - value + pub fn validate_deployment_block(value: &str) -> Result<u64, ParseOrderbookConfigSourceError> { + value.trim() .parse::<u64>() .map_err(ParseOrderbookConfigSourceError::DeploymentBlockParseError) }
141-156: Spec mismatch: make deployment-block optional, accept int or string, default to 1.Issue #2110 requires optional with default 1. Current code requires a string and Default is 0. Fix parsing and default. Previously flagged.
- let deployment_block_str = require_string( - orderbook_yaml, - Some("deployment-block"), - Some(location.clone()), - )?; - let deployment_block = OrderbookCfg::validate_deployment_block( - &deployment_block_str, - ) - .map_err(|e| YamlError::Field { - kind: FieldErrorKind::InvalidValue { - field: "deployment-block".to_string(), - reason: e.to_string(), - }, - location: location.clone(), - })?; + let deployment_block = { + let mut val: Option<u64> = None; + if let StrictYaml::Hash(ref h) = orderbook_yaml { + if let Some(v) = h.get(&StrictYaml::String("deployment-block".to_string())) { + match v { + StrictYaml::String(s) => { + val = Some(OrderbookCfg::validate_deployment_block(s).map_err(|e| YamlError::Field { + kind: FieldErrorKind::InvalidValue { field: "deployment-block".to_string(), reason: e.to_string() }, + location: location.clone(), + })?); + } + StrictYaml::Integer(i) => { + val = Some(u64::try_from(*i).map_err(|_| YamlError::Field { + kind: FieldErrorKind::InvalidValue { field: "deployment-block".to_string(), reason: "deployment block cannot be negative".to_string() }, + location: location.clone(), + })?); + } + StrictYaml::Real(s) => { + val = Some(OrderbookCfg::validate_deployment_block(s).map_err(|e| YamlError::Field { + kind: FieldErrorKind::InvalidValue { field: "deployment-block".to_string(), reason: e.to_string() }, + location: location.clone(), + })?); + } + _ => { + return Err(YamlError::Field { + kind: FieldErrorKind::InvalidType { field: "deployment-block".to_string(), expected: "a string or integer".to_string() }, + location: location.clone(), + }); + } + } + } + } + val.unwrap_or(1) + };- deployment_block: 0, + deployment_block: 1,Also applies to: 198-198
221-223: Clarify error copy: “non-negative integer”.Sharper guidance without being overly technical. Previously suggested.
- #[error("Failed to parse deployment block: {0}")] + #[error("Failed to parse deployment block: {0}")] DeploymentBlockParseError(ParseIntError),- ParseOrderbookConfigSourceError::DeploymentBlockParseError(err) => - format!("The deployment block in your orderbook configuration must be a valid number: {}", err), + ParseOrderbookConfigSourceError::DeploymentBlockParseError(err) => + format!("The deployment block in your orderbook configuration must be a valid non-negative integer: {}", err),Also applies to: 234-236
569-597: Test must reflect optional field with default=1.Missing deployment-block should not error; assert default to 1. Previously raised.
- #[test] - fn test_deployment_block_missing() { + #[test] + fn test_deployment_block_missing_defaults_to_one() { let yaml = r#" @@ "#; - let error = OrderbookCfg::parse_all_from_yaml(vec![get_document(yaml)], None).unwrap_err(); - assert_eq!( - error, - YamlError::Field { - kind: FieldErrorKind::Missing("deployment-block".to_string()), - location: "orderbook 'TestOrderbook'".to_string(), - } - ); - assert_eq!( - error.to_readable_msg(), - "Missing required field 'deployment-block' in orderbook 'TestOrderbook'" - ); + let orderbooks = OrderbookCfg::parse_all_from_yaml(vec![get_document(yaml)], None).unwrap(); + let ob = orderbooks.get("TestOrderbook").unwrap(); + assert_eq!(ob.deployment_block, 1); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
crates/settings/src/orderbook.rs(14 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-07-21T16:34:31.193Z
Learnt from: 0xgleb
PR: rainlanguage/rain.orderbook#1911
File: crates/subgraph/src/types/impls.rs:7-15
Timestamp: 2025-07-21T16:34:31.193Z
Learning: In the rainlanguage/rain.orderbook codebase, user 0xgleb considers breaking changes that remove unsafe default behaviors to be intentional and acceptable. Specifically, the get_decimals() method in crates/subgraph/src/types/impls.rs was intentionally changed to return MissingDecimals error instead of defaulting to 18 decimals, as defaulting to 18 is considered unsafe and should never have been done.
Applied to files:
crates/settings/src/orderbook.rs
📚 Learning: 2025-05-27T10:19:38.011Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1897
File: crates/settings/src/yaml/orderbook.rs:311-311
Timestamp: 2025-05-27T10:19:38.011Z
Learning: The rainlanguage/rain.orderbook project uses strict YAML parsing (StrictYaml library), so concerns about inconsistent parsing between integer and string values in YAML (like `spec-version: 1` vs `spec-version: "1"`) are not relevant to this codebase.
Applied to files:
crates/settings/src/orderbook.rs
📚 Learning: 2025-03-31T13:57:59.660Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1474
File: crates/js_api/src/yaml/mod.rs:31-34
Timestamp: 2025-03-31T13:57:59.660Z
Learning: The OrderbookYaml constructor in crates/js_api/src/yaml/mod.rs does not need early YAML validation. The author prefers to validate YAML only when it's actually used rather than during initialization.
Applied to files:
crates/settings/src/orderbook.rs
📚 Learning: 2025-07-16T10:40:05.717Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2000
File: crates/common/src/raindex_client/vaults.rs:183-183
Timestamp: 2025-07-16T10:40:05.717Z
Learning: In the rainlanguage/rain.orderbook codebase, user findolor considers breaking changes from Option<U256> to U256 for required fields like decimals in RaindexVaultToken to be acceptable and safe, even when they affect multiple usage sites across the codebase.
Applied to files:
crates/settings/src/orderbook.rs
📚 Learning: 2025-07-11T08:37:24.423Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1983
File: crates/settings/src/gui.rs:965-973
Timestamp: 2025-07-11T08:37:24.423Z
Learning: In the rainlanguage/rain.orderbook project, user findolor prefers to keep user-facing error messages generic and user-friendly rather than technically specific. For example, using "a valid number" instead of "a valid u8 (0-255)" in GUI validation error messages to maintain simplicity for end users.
Applied to files:
crates/settings/src/orderbook.rs
📚 Learning: 2025-06-17T16:21:24.384Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1903
File: crates/settings/src/yaml/orderbook.rs:371-377
Timestamp: 2025-06-17T16:21:24.384Z
Learning: In crates/settings/src/yaml/orderbook.rs tests, the user findolor considers RPC ordering in Vec<Url> assertions to be intentional and not a test brittleness issue. The ordering of RPCs in tests should be preserved as specified.
Applied to files:
crates/settings/src/orderbook.rs
📚 Learning: 2025-05-14T05:13:59.713Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1744
File: crates/subgraph/src/orderbook_client.rs:44-46
Timestamp: 2025-05-14T05:13:59.713Z
Learning: In the rain.orderbook project, WASM dependencies are intentionally made available in non-WASM targets to facilitate testing of WASM-related functionality, so conditional compilation guards like `#[cfg(target_family = "wasm")]` should not be added to imports or implementations that may be needed for tests.
Applied to files:
crates/settings/src/orderbook.rs
📚 Learning: 2025-06-18T19:24:40.518Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:185-199
Timestamp: 2025-06-18T19:24:40.518Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers not to refactor get_orderbook_by_network_key to handle multiple orderbooks per network key since their current architecture maintains a one-to-one mapping between orderbooks and networks. They would consider the refactoring if the system evolves to support multiple orderbooks per network in the future.
Applied to files:
crates/settings/src/orderbook.rs
📚 Learning: 2025-05-09T05:28:22.089Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1744
File: crates/js_api/src/subgraph/order.rs:109-114
Timestamp: 2025-05-09T05:28:22.089Z
Learning: In the rain.orderbook project, SubgraphError contains or converts from OrderbookSubgraphClientError, so using OrderbookSubgraphClientError in error creation followed by a conversion to SubgraphError (via ? operator) is acceptable.
Applied to files:
crates/settings/src/orderbook.rs
📚 Learning: 2025-06-18T18:24:32.049Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:180-199
Timestamp: 2025-06-18T18:24:32.049Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers to avoid refactoring duplicate search logic between get_orderbook_by_address and get_orderbook_by_network_key when there are only 2 functions, indicating they would consider it if more similar functions are added in the future.
Applied to files:
crates/settings/src/orderbook.rs
🧬 Code graph analysis (1)
crates/settings/src/orderbook.rs (3)
crates/settings/src/network.rs (4)
value(55-56)value(60-61)parse_all_from_yaml(160-258)to_readable_msg(290-309)crates/settings/src/yaml/mod.rs (5)
require_string(369-404)new(41-41)parse_all_from_yaml(64-67)parse_all_from_yaml(82-82)to_readable_msg(273-358)crates/settings/src/yaml/orderbook.rs (1)
new(94-137)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: Deploy-Preview-Push
- GitHub Check: test
- GitHub Check: Deploy-Docs-Preview
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
🔇 Additional comments (4)
crates/settings/src/orderbook.rs (4)
600-640: Sanity check: valid values suite looks good.Covers 0, max u64, typical positive. Will continue to pass after parser changes.
736-765: Decimal should remain invalid.Your current expectation (“invalid digit found in string”) will still hold after the parser accepts reals via string path. No change needed.
157-166: Ensure indexer honors deployment_block
OrderbookCfg correctly exposes deployment_block and is only consumed here in tests and the JS API. Manually verify in your indexer implementation (separate crate/service) that this field is used as the start block for event scans, falling back to block 1 if unset.
643-671: Do not update the test yet. The parser still usesu64::parseand emits the existing “Failed to parse deployment block: …” error; implement an explicit negative‐value check invalidate_deployment_blockbefore changing the expected message.Likely an incorrect or invalid review comment.
Comment on lines
31
to
34
| #[cfg_attr(target_family = "wasm", tsify(optional))] | ||
| pub label: Option<String>, | ||
| pub deployment_block: u64, | ||
| } |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Expose u64 safely to WASM/TS (avoid >2^53 precision loss).
Annotate deployment_block for WASM as string to prevent precision loss in JS.
#[cfg_attr(target_family = "wasm", tsify(optional))]
pub label: Option<String>,
- pub deployment_block: u64,
+ #[cfg_attr(target_family = "wasm", tsify(type = "string"))]
+ pub deployment_block: u64,Would this field be consumed by the JS API? If yes, the above is required.
🏁 Script executed:
#!/bin/bash
set -e
rg -n "tsify" -g "crates/settings/src/*.rs"Length of output: 4669
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,80p' crates/settings/src/orderbook.rsLength of output: 2979
🏁 Script executed:
#!/bin/bash
set -e
rg -n "deployment_block" -g "crates/settings/src/*.rs"Length of output: 1698
Annotate deployment_block as string for WASM/TS
crates/settings/src/orderbook.rs – insert
#[cfg_attr(target_family = "wasm", tsify(type = "string"))]directly above
pub deployment_block: u64,to prevent >2⁵³ precision loss when this u64 is exposed in the generated JS bindings.
🤖 Prompt for AI Agents
In crates/settings/src/orderbook.rs around lines 31–34, the u64 field
deployment_block needs a TS annotation for WASM to avoid >2⁵³ precision loss;
insert the attribute #[cfg_attr(target_family = "wasm", tsify(type = "string"))]
directly above the line declaring pub deployment_block: u64 so the generated
TypeScript uses string for this field when compiled for wasm.
Contributor
There was a problem hiding this comment.
@findolor Understood! You've decided to keep the current implementation as-is. That works perfectly fine for your use case.
🐰
╭─────╮
│ ◕ ◕ │
│ ─ │
╰─────╯
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2116
File: crates/settings/src/orderbook.rs:569-0
Timestamp: 2025-09-01T13:35:33.632Z
Learning: In crates/settings/src/orderbook.rs, the deployment-block field is required in orderbook YAML configuration and should cause a parsing error when missing, not default to any value.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:185-199
Timestamp: 2025-06-18T19:24:40.518Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers not to refactor get_orderbook_by_network_key to handle multiple orderbooks per network key since their current architecture maintains a one-to-one mapping between orderbooks and networks. They would consider the refactoring if the system evolves to support multiple orderbooks per network in the future.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2116
File: crates/common/src/raindex_client/mod.rs:0-0
Timestamp: 2025-09-01T13:26:29.740Z
Learning: In the rainlanguage/rain.orderbook codebase, the deployment-block field in orderbook YAML configuration cannot be absent - it is a required field, not optional as initially described in some documentation.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:180-199
Timestamp: 2025-06-18T18:24:32.049Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers to avoid refactoring duplicate search logic between get_orderbook_by_address and get_orderbook_by_network_key when there are only 2 functions, indicating they would consider it if more similar functions are added in the future.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tauri-app/src-tauri/src/commands/config.rs(2 hunks)tauri-app/src-tauri/src/commands/order.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2116
File: crates/common/src/raindex_client/mod.rs:0-0
Timestamp: 2025-09-01T13:26:29.740Z
Learning: In the rainlanguage/rain.orderbook codebase, the deployment-block field in orderbook YAML configuration cannot be absent - it is a required field, not optional as initially described in some documentation.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2116
File: crates/settings/src/orderbook.rs:569-0
Timestamp: 2025-09-01T13:35:33.632Z
Learning: In crates/settings/src/orderbook.rs, the deployment-block field is required in orderbook YAML configuration and should cause a parsing error when missing, not default to any value.
📚 Learning: 2025-06-04T10:21:01.388Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.
Applied to files:
tauri-app/src-tauri/src/commands/order.rstauri-app/src-tauri/src/commands/config.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: git-clean
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Deploy-Preview-Push
- GitHub Check: Deploy-Docs-Preview
🔇 Additional comments (1)
tauri-app/src-tauri/src/commands/config.rs (1)
100-100: Spec bump to 3 acknowledged—confirm UX for older files.Ensure the Tauri error surfaced for version 2 files is clear/actionable, or add a fallback if v2 should still load.
| address: 0xc95A5f8eFe14d7a20BD2E5BAFEC4E71f8Ce0B9A6 | ||
| network: some-network | ||
| subgraph: some-sg | ||
| deployment-block: 12345 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
deployment-block added—clarify required vs optional and add a negative test.
Repo learnings indicate this field is required (parse error if missing). If that’s the intent, align issue #2110 text and add a test here that fails when the field is omitted. If it should be optional with a default, update settings parsers accordingly.
Apply this diff to add a missing-field test:
@@
mod tests {
use super::*;
@@
async fn test_get_scenarios() {
let scenarios = get_scenarios(DOTRAIN.to_string(), None).await.unwrap();
assert_eq!(scenarios.len(), 2);
assert!(scenarios.contains_key("some-scenario"));
assert!(scenarios.contains_key("some-scenario.sub-scenario"));
}
+
+ #[tokio::test]
+ async fn test_check_settings_errors_requires_deployment_block() {
+ // Same as DOTRAIN but without deployment-block under orderbooks.some-orderbook
+ let settings_without_block = r#"
+version: 3
+networks:
+ some-network:
+ rpcs: [http://localhost:8085/rpc-url]
+ chain-id: 123
+ network-id: 123
+ currency: ETH
+subgraphs:
+ some-sg: https://www.some-sg.com
+orderbooks:
+ some-orderbook:
+ address: 0xc95A5f8eFe14d7a20BD2E5BAFEC4E71f8Ce0B9A6
+ network: some-network
+ subgraph: some-sg
+"#;
+ let res = check_settings_errors(vec![settings_without_block.to_string()]).await;
+ assert!(res.is_err(), "expected parse error when deployment-block is missing");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| deployment-block: 12345 | |
| mod tests { | |
| use super::*; | |
| #[tokio::test] | |
| async fn test_get_scenarios() { | |
| let scenarios = get_scenarios(DOTRAIN.to_string(), None).await.unwrap(); | |
| assert_eq!(scenarios.len(), 2); | |
| assert!(scenarios.contains_key("some-scenario")); | |
| assert!(scenarios.contains_key("some-scenario.sub-scenario")); | |
| } | |
| #[tokio::test] | |
| async fn test_check_settings_errors_requires_deployment_block() { | |
| // Same as DOTRAIN but without deployment-block under orderbooks.some-orderbook | |
| let settings_without_block = r#" | |
| version: 3 | |
| networks: | |
| some-network: | |
| rpcs: [http://localhost:8085/rpc-url] | |
| chain-id: 123 | |
| network-id: 123 | |
| currency: ETH | |
| subgraphs: | |
| some-sg: https://www.some-sg.com | |
| orderbooks: | |
| some-orderbook: | |
| address: 0xc95A5f8eFe14d7a20BD2E5BAFEC4E71f8Ce0B9A6 | |
| network: some-network | |
| subgraph: some-sg | |
| "#; | |
| let res = check_settings_errors(vec![settings_without_block.to_string()]).await; | |
| assert!(res.is_err(), "expected parse error when deployment-block is missing"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In tauri-app/src-tauri/src/commands/config.rs around line 124, the new
deployment-block field is present but it's unclear whether it is required or
optional; update the code and tests to reflect the intended behavior: if
deployment-block is meant to be required, update issue #2110 text to state it's
required and add a negative unit/integration test that attempts to parse
settings without deployment-block and asserts a parse error; if it should be
optional, modify the settings parser to provide a sensible default (or Option<>
return) and add a positive test that omits the field and verifies the default is
applied; ensure the new test file is added to the test suite and CI, and update
any documentation/comments to match the chosen behavior.
Contributor
There was a problem hiding this comment.
@findolor, understood! I'll leave the implementation as-is since you're satisfied with the current state. The deployment-block field is working correctly in the tests, and you can always revisit the test coverage later if needed.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2116
File: crates/settings/src/orderbook.rs:569-0
Timestamp: 2025-09-01T13:35:33.632Z
Learning: In crates/settings/src/orderbook.rs, the deployment-block field is required in orderbook YAML configuration and should cause a parsing error when missing, not default to any value.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2116
File: crates/common/src/raindex_client/mod.rs:0-0
Timestamp: 2025-09-01T13:26:29.740Z
Learning: In the rainlanguage/rain.orderbook codebase, the deployment-block field in orderbook YAML configuration cannot be absent - it is a required field, not optional as initially described in some documentation.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.
|
|
||
| let dotrain = r#" | ||
| version: 3 | ||
| version: 4 |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid hard-coding the mismatched spec version ("4") to keep the test future-proof.
Make the "actual" (invalid) version dynamic (e.g., current+1) so the test won’t need edits on every spec bump.
Apply this minimal change to the YAML line, and then convert the surrounding assignment to use format! with a computed invalid_version:
-version: 4
+version: {invalid_version}Example (outside this line’s scope):
let invalid_version = (format!("{}", SpecVersion::current()).parse::<u64>().unwrap() + 1).to_string();
let dotrain = format!(r#"
version: {invalid_version}
...
"#, invalid_version = invalid_version);To find any other hard-coded spec versions in tests:
🏁 Script executed:
#!/bin/bash
rg -nP 'version:\s*\d+' -C2Length of output: 6215
Avoid hard-coding the mismatched spec version; compute it dynamically
Replace version: 4 with a formatted {invalid_version} (e.g. let invalid_version = SpecVersion::current().as_u64() + 1; and format!) to future-proof this test. Run rg -nP 'version:\s*\d+' -C2 to locate and refactor other hard-coded instances.
🤖 Prompt for AI Agents
In tauri-app/src-tauri/src/commands/order.rs around line 772, the test
hard-codes an invalid spec version as `version: 4`; change this to compute the
mismatched version dynamically by deriving the current spec version at runtime
(e.g. get SpecVersion::current(), convert to u64, add 1) and inject it via
formatting so the test remains valid if the spec increments; also run a search
(rg -nP 'version:\s*\d+' -C2) to find and refactor other hard-coded numeric spec
versions similarly.
Contributor
There was a problem hiding this comment.
@findolor Understood! I'll leave the hard-coded version as is. Thanks for the feedback.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2116
File: crates/common/src/raindex_client/mod.rs:0-0
Timestamp: 2025-09-01T13:26:29.740Z
Learning: In the rainlanguage/rain.orderbook codebase, the deployment-block field in orderbook YAML configuration cannot be absent - it is a required field, not optional as initially described in some documentation.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2116
File: crates/settings/src/orderbook.rs:569-0
Timestamp: 2025-09-01T13:35:33.632Z
Learning: In crates/settings/src/orderbook.rs, the deployment-block field is required in orderbook YAML configuration and should cause a parsing error when missing, not default to any value.
Comment on lines
791
to
795
| assert!(matches!( | ||
| err, | ||
| CommandError::DotrainOrderError(DotrainOrderError::SpecVersionMismatch(ref expected, ref actual)) | ||
| if expected == "2" && actual == "3" | ||
| if expected == "3" && actual == "4" | ||
| )); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Compare against current spec and assert inequality instead of a fixed "4".
Reduces brittleness and decouples the test from specific numbers.
Apply:
- if expected == "3" && actual == "4"
+ if expected == &SpecVersion::current().to_string() && expected != actual📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert!(matches!( | |
| err, | |
| CommandError::DotrainOrderError(DotrainOrderError::SpecVersionMismatch(ref expected, ref actual)) | |
| if expected == "2" && actual == "3" | |
| if expected == "3" && actual == "4" | |
| )); | |
| assert!(matches!( | |
| err, | |
| CommandError::DotrainOrderError(DotrainOrderError::SpecVersionMismatch(ref expected, ref actual)) | |
| if expected == &SpecVersion::current().to_string() && expected != actual | |
| )); |
🤖 Prompt for AI Agents
In tauri-app/src-tauri/src/commands/order.rs around lines 791–795, the test
currently asserts the actual spec version is the literal "4"; replace that
brittle literal with the module's canonical current spec constant (e.g.
CURRENT_SPEC_VERSION or SPEC_VERSION) and update the assertion to check against
that constant instead of "4" — also assert inequality between expected and the
current spec (or assert_eq! that actual equals the current spec and
assert_ne!(expected, current_spec)) so the test compares to the project-wide
spec value rather than a hardcoded number.
Contributor
There was a problem hiding this comment.
@findolor Understood! I'll accept the current test assertion approach as you've decided it works well for your needs.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2116
File: crates/common/src/raindex_client/mod.rs:0-0
Timestamp: 2025-09-01T13:26:29.740Z
Learning: In the rainlanguage/rain.orderbook codebase, the deployment-block field in orderbook YAML configuration cannot be absent - it is a required field, not optional as initially described in some documentation.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2116
File: crates/settings/src/orderbook.rs:569-0
Timestamp: 2025-09-01T13:35:33.632Z
Learning: In crates/settings/src/orderbook.rs, the deployment-block field is required in orderbook YAML configuration and should cause a parsing error when missing, not default to any value.
hardyjosh
approved these changes
Sep 2, 2025
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. 🧩 Analysis chainSIZE=L 🏁 Script executed: Length of output: 1007 Based on the comprehensive AI summary and the scope of changes across approximately 21 files spanning multiple crates and packages, including:
This represents a feature addition that touches multiple modules with comprehensive test coverage, requiring significant domain knowledge to review properly and would take 45+ minutes to review thoroughly. SIZE=L 🏁 Script executed: Length of output: 509 Based on the script results showing the actual scope of changes, my assessment remains: SIZE=L 🏁 Script executed: Length of output: 658 SIZE=L 🧠 Learnings used |
This was referenced Oct 7, 2025
This was referenced Oct 27, 2025
Merged
4 tasks
4 tasks
4 tasks
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
2 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
See issue: #2110
Solution
deployment-blockfield to orderbook yaml parsingChecks
By submitting this for review, I'm confirming I've done the following:
fix #2110
Summary by CodeRabbit