Adding remote networks support to yaml#1477
Conversation
WalkthroughThe changes extend YAML parsing and configuration initialization by integrating remote network functionality into multiple modules. This includes updating error handling with new variants, modifying method signatures to utilize a provided context, and asynchronously fetching remote networks. A new module and cache mechanism have been introduced to manage remote network configurations within YAML parsing, along with additional tests to validate functionality and handle key conflicts. Changes
Sequence Diagram(s)sequenceDiagram
participant D as DotrainOrder::new
participant R as RemoteNetworksCfg::fetch_networks
participant Y as DotrainYaml (Cache)
participant O as OrderbookYaml::parse_all_from_yaml
participant C as Context
D->>R: Asynchronously fetch remote networks
R-->>D: Return fetched remote networks
D->>Y: Update cache with remote networks
O->>C: Integrate and utilize remote networks from cache during parsing
Assessment against linked issues
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
crates/common/src/dotrain_order/mod.rs(4 hunks)crates/settings/src/deployer.rs(1 hunks)crates/settings/src/gui.rs(2 hunks)crates/settings/src/lib.rs(1 hunks)crates/settings/src/network.rs(3 hunks)crates/settings/src/order.rs(1 hunks)crates/settings/src/orderbook.rs(1 hunks)crates/settings/src/remote/chains/chainid.rs(2 hunks)crates/settings/src/remote_networks.rs(1 hunks)crates/settings/src/scenario.rs(1 hunks)crates/settings/src/token.rs(1 hunks)crates/settings/src/yaml/cache.rs(1 hunks)crates/settings/src/yaml/context.rs(5 hunks)crates/settings/src/yaml/dotrain.rs(5 hunks)crates/settings/src/yaml/mod.rs(3 hunks)crates/settings/src/yaml/orderbook.rs(8 hunks)packages/orderbook/test/js_api/gui.test.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (8)
crates/settings/src/token.rs (4)
crates/settings/src/deployer.rs (1) (1)
parse_all_from_yaml(128-193)crates/settings/src/orderbook.rs (1) (1)
parse_all_from_yaml(73-158)crates/settings/src/order.rs (1) (1)
parse_all_from_yaml(475-767)crates/settings/src/scenario.rs (1) (1)
parse_all_from_yaml(328-378)
crates/settings/src/gui.rs (5)
crates/settings/src/deployer.rs (1) (1)
parse_all_from_yaml(128-193)crates/settings/src/orderbook.rs (1) (1)
parse_all_from_yaml(73-158)crates/settings/src/order.rs (1) (1)
parse_all_from_yaml(475-767)crates/settings/src/scenario.rs (1) (1)
parse_all_from_yaml(328-378)crates/settings/src/token.rs (1) (1)
parse_all_from_yaml(228-305)
crates/settings/src/order.rs (4)
crates/settings/src/deployer.rs (1) (1)
parse_all_from_yaml(128-193)crates/settings/src/orderbook.rs (1) (1)
parse_all_from_yaml(73-158)crates/settings/src/scenario.rs (1) (1)
parse_all_from_yaml(328-378)crates/settings/src/token.rs (1) (1)
parse_all_from_yaml(228-305)
crates/settings/src/deployer.rs (4)
crates/settings/src/orderbook.rs (1) (1)
parse_all_from_yaml(73-158)crates/settings/src/order.rs (1) (1)
parse_all_from_yaml(475-767)crates/settings/src/scenario.rs (1) (1)
parse_all_from_yaml(328-378)crates/settings/src/token.rs (1) (1)
parse_all_from_yaml(228-305)
crates/settings/src/orderbook.rs (4)
crates/settings/src/deployer.rs (1) (1)
parse_all_from_yaml(128-193)crates/settings/src/order.rs (1) (1)
parse_all_from_yaml(475-767)crates/settings/src/scenario.rs (1) (1)
parse_all_from_yaml(328-378)crates/settings/src/token.rs (1) (1)
parse_all_from_yaml(228-305)
crates/settings/src/yaml/orderbook.rs (3)
crates/settings/src/yaml/mod.rs (7) (7)
parse_all_from_yaml(48-51)parse_all_from_yaml(66-66)from_orderbook_yaml(28-28)new(26-26)parse_from_yaml(53-62)parse_from_yaml(70-70)parse_from_yaml(78-81)crates/settings/src/yaml/dotrain.rs (2) (2)
from_orderbook_yaml(59-64)new(26-50)crates/settings/src/yaml/context.rs (3) (3)
new(151-158)get_remote_networks(133-133)get_remote_networks(139-141)
crates/settings/src/yaml/mod.rs (2)
crates/settings/src/yaml/dotrain.rs (2) (2)
from_orderbook_yaml(59-64)from_dotrain_yaml(52-57)crates/settings/src/yaml/orderbook.rs (2) (2)
from_orderbook_yaml(59-64)from_dotrain_yaml(66-71)
crates/settings/src/yaml/context.rs (2)
crates/settings/src/yaml/orderbook.rs (2) (2)
get_remote_networks(89-92)new(29-57)crates/settings/src/yaml/dotrain.rs (1) (1)
new(26-50)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- 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: Deploy-Preview
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: git-clean
🔇 Additional comments (56)
crates/settings/src/lib.rs (1)
16-16: New module added for remote networks support.This addition introduces a new module dedicated to handling remote networks functionality, which aligns with the PR's goal of supporting external networks in YAML configuration.
packages/orderbook/test/js_api/gui.test.ts (2)
338-406: Well-structured test configuration for remote networks.The
dotrainForRemoteNetworkconstant provides a comprehensive configuration example that includes the keyusing-networks-fromfield needed for remote network functionality. It includes all necessary components like deployments, networks, tokens, and properly configures services to use the remote network.
1751-1825: Thorough test coverage for remote networks functionality.These tests properly verify two critical aspects of the remote networks feature:
- The ability to successfully fetch and use remote networks in configurations
- Proper error handling for network key shadowing (duplicate networks)
The mock server setup effectively simulates the remote network API responses needed for testing.
crates/settings/src/scenario.rs (1)
334-334: Proper context propagation for remote networks.The change properly forwards the context parameter to the
DeployerCfg::parse_all_from_yamlfunction, which is essential for propagating remote network information throughout the YAML parsing process. This ensures that deployer configurations can access remote network data during parsing.crates/settings/src/order.rs (1)
481-483: Proper context propagation for remote networks support.The change correctly propagates the
contextparameter to the related YAML parsing methods, enabling the remote networks functionality to work throughout the configuration hierarchy.crates/settings/src/orderbook.rs (2)
75-75: Named context parameter for improved clarity.Replaced the unused placeholder parameter with a named one to make the function signature more explicit about what's being passed.
79-80: Context propagation for remote networks.The context parameter is now properly passed to the parse methods for
NetworkCfgandSubgraphCfg, enabling support for remote networks in these configurations.crates/settings/src/remote/chains/chainid.rs (2)
1-6: Added necessary imports for NetworkCfg conversion.The imports are correctly added to support the new
try_into_network_cfgmethod.
89-112:Details
✅ Verification successful
Added method to convert ChainId to NetworkCfg.
This new method enables direct conversion from a ChainId object to NetworkCfg, which is a key component for supporting remote networks. The implementation properly handles error cases and follows the same pattern as the existing
TryFrom<ChainId>implementation.
🏁 Script executed:
#!/bin/bash # Check if the try_into_network_cfg implementation is consistent with the TryFrom implementation # Check for references to both implementations rg -B 5 -A 5 "try_into_network_cfg|TryFrom<ChainId>" crates/settings/src/remote/Length of output: 1697
Verification Successful: ChainId Conversion Method Matches Expected Pattern
The new
try_into_network_cfgmethod correctly mirrors the conversion logic of the existingTryFrom<ChainId>implementation by handling empty RPC lists and returning an appropriate error when no valid RPC URL is found. The conversion of the ChainId object into a NetworkCfg is implemented as expected, with consistent error handling and clear mapping of fields.crates/settings/src/gui.rs (2)
8-8: Added NetworkCfg to imports.Added NetworkCfg to support the new remote networks functionality.
567-569: Parse networks before tokens.Added network parsing as a prerequisite step before parsing tokens. This change ensures that remote networks are available when processing tokens, which is essential for the remote networks feature.
crates/settings/src/token.rs (1)
230-234: Improved context handling enables remote networks support.The changes now properly pass the context parameter to the
NetworkCfg::parse_all_from_yamlmethod instead of discarding it withNone. This is essential for the remote networks feature to work, as it allows network information contained in the context to be accessible during token parsing.crates/settings/src/deployer.rs (1)
130-134: Consistent context propagation for remote networks support.Similar to the token.rs changes, this modification properly passes the context to the NetworkCfg parser instead of discarding it. The implementation maintains consistency with the pattern applied across related files, ensuring that remote network information flows correctly through the parsing chain.
crates/settings/src/yaml/dotrain.rs (6)
1-1: Added Cache import for remote networks support.Properly imported the cache module which is essential for the remote networks feature implementation.
20-20: Added cache field to store remote networks.The DotrainYaml struct now includes a Cache field that will store remote network information, addressing issue #1396 for external networks support.
46-49: Initialize cache in constructor.The DotrainYaml constructor now properly initializes the cache field, ensuring that new instances always have a valid cache.
52-64: Added methods to maintain cache consistency across YAML conversions.Replaced the original
from_documentsmethod with more specific methods that ensure cache information is properly preserved when creating DotrainYaml instances from different sources:
from_dotrain_yaml: Copies both documents and cache from an existing DotrainYamlfrom_orderbook_yaml: Uses documents from OrderbookYaml but initializes a new cacheThis implementation ensures cache consistency throughout the object's lifecycle.
98-103: Context now receives remote networks from cache.The
get_guimethod now adds remote networks from the cache to the context, making them available during GUI configuration parsing. This is a key connection point that enables the remote networks feature to work across the system.
152-155: Cache initialization during deserialization.The deserializer now properly initializes the cache field, ensuring that even deserialized instances have a valid cache. This maintains consistency for all paths that create DotrainYaml instances.
crates/settings/src/network.rs (3)
145-145: Enabled context parameter for remote networks support.Updated the signature to use the context parameter rather than discarding it, which is necessary to access remote network information.
214-225: Added core remote networks integration logic.This code block implements the core functionality for remote networks support:
- Checks if context contains remote networks
- Iterates through remote networks and adds them to the local networks map
- Validates that remote network keys don't conflict with local ones
The implementation properly handles network integration while maintaining data integrity through conflict detection.
262-263: Added specific error variant for remote network conflicts.Created a dedicated error variant
RemoteNetworkKeyShadowingto handle the case where a remote network key conflicts with a local one. This provides clear error messaging for debugging and maintains the error type safety of the system.crates/common/src/dotrain_order/mod.rs (4)
12-12: Implementation properly imports remote networks module.The added import ensures that
ParseRemoteNetworksErrorandRemoteNetworksCfgare available for handling remote networks functionality.
84-85: Good error handling for remote networks parsing.Adding the
ParseRemoteNetworksErrorvariant to theDotrainOrderErrorenum with the#[from]attribute enables automatic error conversion, maintaining consistent error handling throughout the codebase.
146-151: Implementation correctly fetches and caches remote networks.This change fetches remote networks asynchronously and updates the DotrainYaml cache with the fetched networks, ensuring that remote network information is available for subsequent operations.
230-230: Updated to use consistent cache patterns.The
orderbook_yamlmethod now usesOrderbookYaml::from_dotrain_yamlinstead of the previous approach, ensuring that the cache containing remote networks is properly passed to the new OrderbookYaml instance.crates/settings/src/yaml/orderbook.rs (14)
1-4: Import structure properly updated for remote networks.The imports have been appropriately updated to include the new
cacheandremote_networksmodules.
23-23: Cache field added to OrderbookYaml struct.This addition enables storing and retrieving remote network information efficiently within the OrderbookYaml struct.
45-45: Validation for remote networks added.The
newmethod now validates RemoteNetworksCfg during initialization, ensuring that remote network configurations are correctly formatted.
53-56: Proper initialization of cache in OrderbookYaml constructor.The constructor now initializes the cache field with a default Cache instance, preparing it for storing remote network information.
59-70: Implementation preserves cache during conversion.Both
from_orderbook_yamlandfrom_dotrain_yamlmethods now ensure that the cache is properly transferred to the new instance, maintaining remote network information across conversions.
76-78: Context updated with remote networks in get_network_keys.This change ensures that remote networks from the cache are added to the context before parsing networks, making them available during the parsing process.
83-86: Context updated with remote networks in get_network.Similar to the previous comment, this ensures remote networks are available when fetching a specific network.
89-92: New method for retrieving remote networks.The
get_remote_networksmethod provides a clean interface for accessing remote network configurations, supporting the feature's functionality.
95-98: Context updated with remote networks in get_token_keys.This ensures remote networks are available when fetching token keys, which may reference networks.
117-120: Context updated with remote networks in get_orderbook_keys.This ensures remote networks are available when fetching orderbook keys.
124-127: Context updated with remote networks in get_orderbook.This ensures remote networks are available when fetching a specific orderbook.
142-145: Context updated with remote networks in get_deployer_keys.This ensures remote networks are available when fetching deployer keys.
149-152: Context updated with remote networks in get_deployer.This ensures remote networks are available when fetching a specific deployer.
310-315: Test added for the new remote networks functionality.The test validates that remote networks can be correctly retrieved with the new
get_remote_networksmethod.crates/settings/src/yaml/mod.rs (3)
1-1: Module declaration for cache added.The new public module for cache management is correctly exposed at the package level.
13-14: Type imports for DotrainYaml and OrderbookYaml added.These imports allow the types to be used in the trait definitions, particularly in the new methods.
28-29: YamlParsable trait updated for specific type conversions.The trait now has specific methods for converting between OrderbookYaml and DotrainYaml types, which supports the new caching mechanism for remote networks.
crates/settings/src/yaml/cache.rs (3)
1-7: New Cache struct with remote_networks field.The Cache struct provides a dedicated mechanism for storing and managing remote network configurations, with a HashMap for efficient key-based lookups.
8-14: Methods for updating remote networks.These methods provide a clean interface for updating the entire remote networks collection or individual network entries.
15-20: Methods for retrieving remote networks.These methods provide a consistent way to access the cached remote networks, returning clones to prevent modification of the cache from outside.
crates/settings/src/yaml/context.rs (4)
1-2: Imports look good and consistent.
These extensions to import statements correctly bring in the necessary types from across the codebase to implement the new remote networks functionality.
132-136: Trait design is clear and consistent.
The methods provide straightforward access to the remote networks data. Returning anOptionis acceptable if missing data is a valid scenario; otherwise, consider using aResultor guaranteed fields.
138-148: Implementation aligns well with the trait definitions.
These methods correctly handle the optionalremote_networksfield and returnOptionreferences.
156-156: Field initialization is coherent with the optional design.
The field is initialized toNoneto indicate an absence of remote networks. Just ensure that you handle this case safely throughout the code.crates/settings/src/remote_networks.rs (6)
1-15: Imports and dependencies are well-structured.
Your usage ofreqwest,serde,strict_yaml_rust, and others is aligned with the functionality for parsing remote network data and making network requests.
16-19: RemoteNetworks enum is straightforward.
It currently supports only one variant (ChainId). If future formats emerge, you can easily extend it.
21-31: RemoteNetworksCfg struct is well-defined.
Fields such asurlandformatare clearly identified. The usage of#[serde(skip)]fordocumentis appropriate for skipping YAML serialization of internal data.
78-129: YAML parsing integration is well-structured.
Extractingurlandformatfrom the YAML and performing validation inparse_all_from_yamlfollows the established parsing pattern. Good use of existing helpers likerequire_string.
131-145: Default and PartialEq implementations.
Providing a defaultRemoteNetworksCfgand implementing equality checks are helpful for testing and using as placeholders.
147-159: Comprehensive error enum for remote networks.
This covers multiple error conditions, including conflicting networks, URL parsing issues, and unknown formats. The error messages are user-friendly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
crates/settings/src/network.rs(3 hunks)crates/settings/src/yaml/context.rs(5 hunks)crates/settings/src/yaml/dotrain.rs(6 hunks)crates/settings/src/yaml/mod.rs(3 hunks)crates/settings/src/yaml/orderbook.rs(8 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
crates/settings/src/yaml/dotrain.rs (6)
crates/settings/src/order.rs (1)
default(771-781)crates/settings/src/network.rs (1)
default(237-239)crates/settings/src/yaml/mod.rs (7)
from_dotrain_yaml(29-29)from_orderbook_yaml(28-28)get_remote_networks_from_cache(97-97)parse_from_yaml(53-62)parse_from_yaml(70-70)parse_from_yaml(78-81)new(26-26)crates/settings/src/yaml/orderbook.rs (4)
from_dotrain_yaml(66-71)from_orderbook_yaml(59-64)get_remote_networks_from_cache(75-77)new(29-57)crates/settings/src/gui.rs (1)
parse_from_yaml(553-558)crates/settings/src/yaml/context.rs (1)
new(149-156)
crates/settings/src/yaml/mod.rs (3)
crates/settings/src/yaml/dotrain.rs (3)
from_orderbook_yaml(59-64)new(26-50)get_remote_networks_from_cache(68-70)crates/settings/src/yaml/orderbook.rs (3)
from_orderbook_yaml(59-64)new(29-57)get_remote_networks_from_cache(75-77)crates/settings/src/yaml/context.rs (3)
new(149-156)order(32-32)order(64-66)
crates/settings/src/yaml/context.rs (2)
crates/settings/src/yaml/orderbook.rs (2)
get_remote_networks(95-98)new(29-57)crates/settings/src/yaml/mod.rs (1)
new(26-26)
crates/settings/src/yaml/orderbook.rs (4)
crates/settings/src/yaml/mod.rs (8)
parse_all_from_yaml(48-51)parse_all_from_yaml(66-66)from_orderbook_yaml(28-28)from_dotrain_yaml(29-29)get_remote_networks_from_cache(97-97)parse_from_yaml(53-62)parse_from_yaml(70-70)parse_from_yaml(78-81)crates/settings/src/network.rs (2)
parse_all_from_yaml(143-233)default(237-239)crates/settings/src/yaml/dotrain.rs (3)
from_orderbook_yaml(59-64)from_dotrain_yaml(52-57)get_remote_networks_from_cache(68-70)crates/settings/src/yaml/context.rs (2)
get_remote_networks(133-133)get_remote_networks(139-141)
🔇 Additional comments (33)
crates/settings/src/network.rs (3)
145-146: Updated method signature looks good.Your addition of the
contextparameter toparse_all_from_yamlis consistent with integrating remote networks. No issues spotted.
214-224: Remote networks insertion logic is properly handled.The loop that checks for key conflicts before inserting remote networks into
networksis clear and straightforward. The immediate return on conflict prevents silent overwriting, which is good. No concerns here.
260-261: New parse error variant is appropriate.Adding
RemoteNetworkKeyShadowing(String)is a clean way to handle conflicting remote network keys. This maintains clarity in error handling.crates/settings/src/yaml/dotrain.rs (8)
1-1: Import usage approved.The addition of
cache::Cacheandorderbook::OrderbookYamlimports is necessary for the new caching behavior. No issues.
20-20: Newcachefield is well-introduced.Storing
CachewithinDotrainYamlcentralizes remote network data. This approach looks consistent with the rest of the PR.
46-49: Initialization of default cache is sound.Ensuring
cache: Cache::default()on creation provides a clean baseline for future caching logic. Looks good.
52-57: Incorporation offrom_dotrain_yamlis clear.Cloning the
documentsand reusing the cache from the providedDotrainYamlis straightforward and maintains consistency.
67-71: ContextProvider implementation looks good.
get_remote_networks_from_cachecleanly exposes cached networks. No concurrency or logical issues identified.
80-81: Expanding context with the current order is appropriate.Ensuring the context is updated before parsing helps maintain consistency for order-related logic.
99-100: Deployment context expansion is correctly placed.The code sets deployment context to enable accurate retrieval of deployment data. No concerns.
106-108: Context expansion for GUI is well-integrated.Incorporating remote networks before GUI parsing ensures the configuration remains consistent. No issues spotted.
crates/settings/src/yaml/mod.rs (5)
1-1: Module declarations are fine.Introducing
pub mod cache;aligns with the new caching setup. No issues noticed.
7-7: Additional imports appear necessary and correct.Bringing these parse error types into scope is consistent with extended error handling requirements.
13-14: Imports ofDotrainYamlandOrderbookYamlmaintain clarity.No concerns with referencing these modules here, especially as they are directly used by the updated traits.
28-29: Refined trait methods are well-structured.Replacing a generic
from_documentsapproach with targetedfrom_orderbook_yamlandfrom_dotrain_yamlfosters clearer usage.
89-119: NewContextProvidertrait enhances extensibility.The trait’s helper methods for setting remote networks and updating context fields are clearly named and facilitate reusability. The code is readable and logically coherent.
crates/settings/src/yaml/orderbook.rs (10)
1-1: No concerns about the new import.
The addedcache::Cachereference appears consistent with the codebase's caching approach.
3-4: Imports for remote network configurations look good.
The inclusion ofRemoteNetworksCfgaligns well with the intended feature for remote networks.
23-23: Introducing the cache field is appropriate.
Storing the newCacheinOrderbookYamlis consistent with the PR objective of caching remote networks.
45-45: Confirm ignoring the context during remote network parsing.
Here,RemoteNetworksCfg::parse_all_from_yamlis called withNoneas context. If remote networks rely on merging contextual data, consider passing the context to ensure consistency with the rest of the parsing logic.Would you like a script to verify whether ignoring context is intentional or if it possibly creates inconsistencies with other usage sites?
53-56: Default initialization of the cache looks fine.
ReturningCache::default()aligns with the rest of the constructor pattern and ensures a predictable state.
59-64: Conversion methods properly propagate the cache.
The newfrom_orderbook_yamlandfrom_dotrain_yamlconstructors, along with theContextProviderimplementation, correctly preserve theCache, ensuring remote network data is carried forward.Also applies to: 66-72, 74-77
82-85: Consistent passing of context for YAML parsing.
Each of these additions ensures that aContextwith remote networks is created/expanded and passed into the parsing functions (parse_all_from_yaml,parse_from_yaml). This pattern is coherent and supports the objective of integrating remote networks across the codebase.Also applies to: 89-93, 101-104, 108-111, 123-126, 130-133, 148-151, 155-158
216-219: Deserialization providing a default cache is correct.
InstantiatingOrderbookYamlwith an empty cache upon deserialization ensures consistency for all code paths.
242-245: New YAML fields for remote networks.
Adding theusing-networks-fromblock and related lines in the test YAML effectively demonstrates how remote networks are declared.
316-322: Remote networks test validations look good.
The assertions confirm that the remote networks are parsed and stored as anticipated, providing valuable coverage for this new feature.crates/settings/src/yaml/context.rs (7)
1-2: New imports for extended context support.
UsingNetworkCfgand additional crates is consistent with handling remote networks.
16-16: New field for remote networks.
Storing full-fledgedremote_networkswithinContextimproves accessibility for downstream parsing.
132-137: Trait definition for remote networks.
These trait methods (get_remote_networks/get_remote_network) are clearly defined and will help unify how remote networks are accessed.
138-146: Implementation of theRemoteNetworksTraitis straightforward.
Cloning is acceptable here to avoid mutating the original map. Final references to a single entry are also handled properly without additional overhead.
154-154: Default-initialized remote networks field.
Starting with an emptyHashMapinnew()is consistent with the rest of the settings code.
164-166: Cloning remote networks from the existing context.
Safely preserving the priorremote_networksensures continuity in the new context.
197-204: Theset_remote_networksmethod is clear.
Renaming toset_remote_networksaligns with typical naming conventions for a full overwrite, avoiding confusion with incremental merges.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/settings/src/yaml/dotrain.rs(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
crates/settings/src/yaml/dotrain.rs (5)
crates/settings/src/network.rs (1)
default(237-239)crates/settings/src/yaml/mod.rs (7)
from_dotrain_yaml(29-29)from_orderbook_yaml(28-28)get_remote_networks_from_cache(97-97)parse_from_yaml(53-62)parse_from_yaml(70-70)parse_from_yaml(78-81)new(26-26)crates/settings/src/yaml/orderbook.rs (4)
from_dotrain_yaml(66-71)from_orderbook_yaml(59-64)get_remote_networks_from_cache(75-77)new(29-57)crates/settings/src/gui.rs (1)
parse_from_yaml(553-558)crates/settings/src/yaml/context.rs (1)
new(149-156)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: git-clean
🔇 Additional comments (8)
crates/settings/src/yaml/dotrain.rs (8)
20-20: Good addition of the cache field.Introducing a dedicated
pub cache: Cachefield helps maintain remote network data in one place.
46-49: Initialization approach looks correct.Constructing a default cache for all new instances is aligned with a clean, default-based initialization strategy.
52-57: Neat reuse of the existing cache.Forwarding both
documentsandcachefromdotrain_yamlprevents losing any remote network data.
59-64: Consistent preservation of the cache.Copying
cachedirectly fromorderbook_yamlensures that previously fetched remote networks remain accessible.
67-69: Straightforward retrieval of remote networks.Returning a
HashMap<String, NetworkCfg>is convenient for users who need a complete snapshot of the cached networks.
80-81: Invocation appears coherent.Expanding context with the current order before parsing is a logical step to ensure remote network data is reflected.
99-100: Appropriate context expansion for deployments.This call sequence nicely ensures that any relevant remote network configurations are readied for subsequent parsing.
106-108: Remote networks now included in GUI context.Expanding the context to include remote networks is a key step to ensure accurate rendering in downstream logic.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
crates/common/src/dotrain_order/mod.rs(5 hunks)crates/settings/src/deployer.rs(1 hunks)crates/settings/src/gui.rs(2 hunks)crates/settings/src/network.rs(4 hunks)crates/settings/src/order.rs(1 hunks)crates/settings/src/orderbook.rs(1 hunks)crates/settings/src/remote_networks.rs(1 hunks)crates/settings/src/scenario.rs(1 hunks)crates/settings/src/token.rs(1 hunks)crates/settings/src/yaml/cache.rs(1 hunks)crates/settings/src/yaml/context.rs(5 hunks)crates/settings/src/yaml/dotrain.rs(6 hunks)crates/settings/src/yaml/mod.rs(3 hunks)packages/orderbook/test/js_api/gui.test.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (8)
crates/settings/src/gui.rs (3)
crates/settings/src/token.rs (1)
parse_all_from_yaml(234-311)crates/settings/src/yaml/mod.rs (2)
parse_all_from_yaml(48-51)parse_all_from_yaml(66-66)crates/settings/src/network.rs (1)
parse_all_from_yaml(143-233)
crates/settings/src/token.rs (6)
crates/settings/src/yaml/mod.rs (3)
new(26-26)parse_all_from_yaml(48-51)parse_all_from_yaml(66-66)crates/settings/src/deployer.rs (1)
parse_all_from_yaml(139-207)crates/settings/src/orderbook.rs (1)
parse_all_from_yaml(73-161)crates/settings/src/scenario.rs (1)
parse_all_from_yaml(331-381)crates/settings/src/order.rs (1)
parse_all_from_yaml(475-767)crates/settings/src/network.rs (1)
parse_all_from_yaml(143-233)
crates/settings/src/deployer.rs (7)
crates/settings/src/yaml/mod.rs (3)
new(26-26)parse_all_from_yaml(48-51)parse_all_from_yaml(66-66)crates/settings/src/orderbook.rs (1)
parse_all_from_yaml(73-161)crates/settings/src/scenario.rs (1)
parse_all_from_yaml(331-381)crates/settings/src/order.rs (1)
parse_all_from_yaml(475-767)crates/settings/src/token.rs (1)
parse_all_from_yaml(234-311)crates/settings/src/deployment.rs (1)
parse_all_from_yaml(61-135)crates/settings/src/network.rs (1)
parse_all_from_yaml(143-233)
crates/settings/src/order.rs (5)
crates/settings/src/deployer.rs (1)
parse_all_from_yaml(139-207)crates/settings/src/orderbook.rs (1)
parse_all_from_yaml(73-161)crates/settings/src/scenario.rs (1)
parse_all_from_yaml(331-381)crates/settings/src/token.rs (1)
parse_all_from_yaml(234-311)crates/settings/src/yaml/mod.rs (2)
parse_all_from_yaml(48-51)parse_all_from_yaml(66-66)
crates/settings/src/scenario.rs (3)
crates/settings/src/deployer.rs (1)
parse_all_from_yaml(139-207)crates/settings/src/order.rs (1)
parse_all_from_yaml(475-767)crates/settings/src/yaml/mod.rs (2)
parse_all_from_yaml(48-51)parse_all_from_yaml(66-66)
crates/settings/src/orderbook.rs (6)
crates/settings/src/yaml/mod.rs (2)
parse_all_from_yaml(48-51)parse_all_from_yaml(66-66)crates/settings/src/deployer.rs (1)
parse_all_from_yaml(139-207)crates/settings/src/order.rs (1)
parse_all_from_yaml(475-767)crates/settings/src/token.rs (1)
parse_all_from_yaml(234-311)crates/settings/src/network.rs (1)
parse_all_from_yaml(143-233)crates/settings/src/subgraph.rs (1)
parse_all_from_yaml(91-142)
crates/settings/src/yaml/mod.rs (3)
crates/settings/src/yaml/dotrain.rs (3)
from_orderbook_yaml(59-64)from_dotrain_yaml(52-57)new(26-50)crates/settings/src/yaml/orderbook.rs (3)
from_orderbook_yaml(59-64)from_dotrain_yaml(66-71)new(29-57)crates/settings/src/yaml/context.rs (3)
new(164-171)order(47-47)order(79-81)
crates/settings/src/yaml/dotrain.rs (2)
crates/settings/src/yaml/mod.rs (3)
from_dotrain_yaml(29-29)from_orderbook_yaml(28-28)get_remote_networks_from_cache(97-97)crates/settings/src/yaml/orderbook.rs (3)
from_dotrain_yaml(66-71)from_orderbook_yaml(59-64)get_remote_networks_from_cache(75-77)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- 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-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: Deploy-Preview
🔇 Additional comments (45)
crates/settings/src/scenario.rs (1)
337-337: Pass context to DeployerCfg for remote networks support.The change ensures that the
contextparameter is passed to theDeployerCfg::parse_all_from_yamlmethod, enabling remote network information to be properly utilized during deployer parsing.crates/settings/src/orderbook.rs (2)
75-75: Updated parameter name to reflect its usage.Changed from unused placeholder
_to named parametercontext, improving code readability by indicating that this parameter is now utilized.
79-80: Pass context to network and subgraph parsing.Now properly forwards the
contextparameter toNetworkCfgandSubgraphCfgparsing methods, allowing access to remote network configurations.crates/settings/src/order.rs (1)
481-483: Forward context to dependency configuration parsers.Now properly passes the
contextparameter to theDeployerCfg,OrderbookCfg, andTokenCfgparsing methods, ensuring consistent access to remote network information throughout the configuration chain.crates/settings/src/gui.rs (3)
8-8: Added NetworkCfg import for remote networks support.This change correctly adds the NetworkCfg type to the imports, which is necessary for the new remote networks functionality being implemented.
567-568: Added NetworkCfg parsing to support remote networks.This new call to
NetworkCfg::parse_all_from_yamlensures that network configurations are parsed from YAML documents (including remote networks) before processing tokens, which aligns with the PR objective to add external networks support.
569-569: Updated TokenCfg parsing to include context.The token parsing now correctly passes the context parameter, allowing tokens to reference networks defined in the context, including the remote networks that were just parsed.
packages/orderbook/test/js_api/gui.test.ts (2)
337-404: Well-structured YAML configuration for remote networks testing.The new
dotrainForRemoteNetworkconstant effectively defines a YAML configuration that includes all necessary components for testing remote network functionality, including theusing-networks-fromfield with appropriate URL and format parameters.
1992-2068: Good test coverage for remote networks functionality.The test suite verifies two important scenarios:
- Successfully fetching remote networks and validating deployment
- Proper error handling for network key conflicts (shadowing)
This ensures both the happy path and error conditions are covered for the new remote networks feature.
crates/settings/src/token.rs (2)
236-236: Updated method signature to use context parameter for remote networks.The parameter name was changed from the placeholder
_tocontext, making it explicit that this parameter is used in the implementation. This change is part of implementing the remote networks feature as specified in issue #1396.
240-240: Now passing context to NetworkCfg parser for remote networks support.The context parameter is now passed to the
NetworkCfg::parse_all_from_yamlmethod instead ofNone, allowing the network configuration to access remote networks information stored in the context.crates/settings/src/deployer.rs (2)
141-141: Updated method signature to use context parameter for remote networks.The parameter name was changed from the placeholder
_tocontext, making it explicit that this parameter is used in the implementation. This change is part of implementing the remote networks feature as specified in issue #1396.
145-145: Now passing context to NetworkCfg parser for remote networks support.The context parameter is now passed to the
NetworkCfg::parse_all_from_yamlmethod instead ofNone, allowing the network configuration to access remote networks information stored in the context.crates/settings/src/network.rs (4)
145-145: Updated method signature to use context parameter for remote networks.The parameter name was changed from the ignored placeholder
_tocontext, making it explicit that this parameter is used in the implementation. This change is part of implementing the remote networks feature as specified in issue #1396.
214-223: Added remote networks handling from context.This new block of code integrates remote networks into the parser by:
- Checking if context is provided
- Iterating through remote networks from the context
- Verifying no key conflicts between local and remote networks
- Adding the remote networks to the networks HashMap
This implementation properly handles potential conflicts between local and remote network configurations.
260-261: Added new error variant for remote network key conflicts.A new
RemoteNetworkKeyShadowingerror variant was added to handle cases where a remote network has the same key as a local network, preventing potential conflicts and providing clear error messaging.
279-282: Added error message for remote network key shadowing.Implemented the user-friendly error message for the new
RemoteNetworkKeyShadowingerror variant, which clearly informs users when a remote network key conflicts with a local one.crates/common/src/dotrain_order/mod.rs (5)
12-16: Updated imports to support remote networks functionality.Added imports for remote networks and cache-related functionality:
ParseRemoteNetworksErrorandRemoteNetworksCfgfor handling remote networksCachefor caching fetched remote networks data- Updated the path to existing imports to ensure proper module resolution
86-87: Added new error variant for remote networks parsing errors.Added
ParseRemoteNetworksErrorvariant to theDotrainOrderErrorenum with the#[from]attribute for automatic conversion, allowing propagation of remote network parsing errors to callers.
138-138: Updated dummy method to initialize cache.The
dummymethod now properly initializes thecachefield indotrain_yamlwith a newCacheinstance, ensuring all fields are initialized even in dummy instances.
160-166: Implemented remote networks fetching during initialization.The
newmethod now includes the following steps for remote networks support:
- Creates an
OrderbookYamlinstance to parse the YAML sources- Fetches remote networks asynchronously using
RemoteNetworksCfg::fetch_networks- Creates a
DotrainYamlinstance- Updates the cache with the fetched remote networks
This implementation properly integrates the remote networks feature into the initialization flow.
244-244: Updated orderbook_yaml method to use dotrain_yaml cache.Changed the implementation to use
OrderbookYaml::from_dotrain_yamlinstead of creating a new instance from scratch, ensuring that remote networks and other cached data fromdotrain_yamlare properly utilized.crates/settings/src/yaml/dotrain.rs (9)
1-1: New imports look good.
They align with the new caching and remote-network functionality.
46-49: Defaulting the cache on creation.
This approach is acceptable if you’re not restoring from stored data.
52-57: Straightforward pass-through constructor.
Cloning fields from oneDotrainYamlto another is clear and safe.
59-64: Preserving cache fromOrderbookYaml.
Properly inheriting cache data avoids losing remote-network info. Good fix.
67-70: ImplementsContextProvidertrait for remote networks.
Good job pulling networks from the cache. This centralizes how contexts retrieve network data.
80-81: Including current order in context.
This ensures correct behaviors downstream for order-specific calls.
99-100: Including current deployment in context.
Similar pattern ensures deployment data is readily accessible.
106-108: Expanding context with remote networks.
Injecting remote networks here is a clean addition. It keeps the logic consistent with other expansions.
157-160: Defaulting the cache on deserialization.
Defaulting thecacheupon deserialization remains acceptable. If the plan evolves to persist caches, consider reading them back.crates/settings/src/yaml/mod.rs (4)
7-7: Extended imports.
Bringing inNetworkCfgaligns with the new remote-network support.
13-14: IntroducingDotrainYaml&OrderbookYamlusage.
These imports unify the YAML types to facilitate their cross-conversion.
28-29: Trait additions for cross-YAML conversions.
Replacingfrom_documentswith these specialized methods is clearer for each YAML variant.
89-118:ContextProvidertrait provides a neat approach to unify context expansions.
Allows each YAML struct to define how it injects remote networks, current deployments, and orders.crates/settings/src/yaml/cache.rs (1)
1-27: New Cache module is clean and self-explanatory.
This struct neatly encapsulates remote-network storage and retrieval, making updates straightforward. Consider adding unit tests.crates/settings/src/yaml/context.rs (5)
16-16: Field addition looks good.Defining
remote_networksas aHashMap<String, NetworkCfg>with a default empty map is a clean approach that avoids the overhead of handling anOption.
153-161: Implementation matches trait expectations.The methods correctly expose the
remote_networksdata, withget_remote_networkreturning a reference to avoid cloning.
169-169: Default initialization is consistent.Initializing
remote_networksto an emptyHashMapinnew()matches the default usage pattern and avoidsNonechecks.
179-181: Consider merging instead of overwriting.You're cloning remote networks from an existing context, effectively overwriting them. If your use case calls for a merge or partial update, you may want to adopt a merge strategy.
212-218: Method usage aligns with naming.
set_remote_networksclearly indicates a full overwrite of networks, matching the method name.crates/settings/src/remote_networks.rs (4)
16-19: Good use of an enum variant for remote networks.
RemoteNetworks::ChainId(Vec<ChainId>)provides a clear and extensible pattern for introducing new formats or types in the future.
35-75: Consider adding a request timeout.Fetching remote networks is done via
reqwest::getcalls without specifying timeouts or a custom client. This can cause long waits or hangs if servers are unresponsive.
78-132: Robust YAML parsing logic.The parsing approach for
RemoteNetworksCfgcorrectly handles missing or invalid fields, providing valuable error messages.
164-317: Impressive test coverage.Your tests thoroughly demonstrate the behavior of various malformed YAML cases and duplicate key checks, ensuring high reliability. Good job!
|
@findolor I'm looking for a test in rust that actually tests the parsing of a url source in the correct format? |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/settings/src/remote_networks.rs(1 hunks)
🔇 Additional comments (5)
crates/settings/src/remote_networks.rs (5)
35-38: Well-structured URL validation.
Thevalidate_urlmethod cleanly handles any parsing errors and returns a descriptive enum variant. For more robust checks, consider additional validations if there's a need to enforce specific protocols or domains in future use cases.
48-49: Network fetching logic looks solid, but consider adding timeouts.
This repeats a past comment about adding default timeouts forreqwest::getcalls to avoid blocking if remote endpoints are unresponsive.
58-64: Excellent conflict detection.
Raising an error for conflicting network keys prevents silent overwrites and ensures clarity in identifying network collisions. This is a solid design choice.
78-132: Robust YAML parsing and validation.
The implementation thoroughly checks for missing or invalid fields (e.g., missing URL or invalid format). This reduces runtime surprises and improves error transparency for users.
319-377: Comprehensive testing of remote fetch.
Usinghttpmockto simulate replies ensures the fetching logic is tested in realistic scenarios. The coverage of multiple responses demonstrates good testing discipline.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
crates/settings/src/network.rs(4 hunks)crates/settings/src/orderbook.rs(1 hunks)crates/settings/src/remote_networks.rs(1 hunks)crates/settings/src/yaml/orderbook.rs(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
crates/settings/src/remote_networks.rs (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1477
File: crates/settings/src/remote_networks.rs:40-75
Timestamp: 2025-04-03T09:45:04.200Z
Learning: The RemoteNetworksCfg implementation in crates/settings/src/remote_networks.rs intentionally fetches remote networks sequentially rather than in parallel.
🧬 Code Definitions (1)
crates/settings/src/orderbook.rs (5)
crates/settings/src/yaml/mod.rs (2)
parse_all_from_yaml(48-51)parse_all_from_yaml(66-66)crates/settings/src/network.rs (1)
parse_all_from_yaml(143-233)crates/settings/src/deployer.rs (1)
parse_all_from_yaml(139-207)crates/settings/src/token.rs (1)
parse_all_from_yaml(234-311)crates/settings/src/subgraph.rs (1)
parse_all_from_yaml(91-142)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Deploy-Preview
- GitHub Check: test
🔇 Additional comments (13)
crates/settings/src/orderbook.rs (1)
75-80: Integratingcontextto parse remote resources
Passing thecontextparameter intoNetworkCfgandSubgraphCfgparsing aligns with the objective of supporting external/remote network definitions. The changes here are consistent and properly propagate the optional context for further processing.crates/settings/src/network.rs (1)
260-261: Clear error message for remote key shadowing
AddingRemoteNetworkKeyShadowingdistinctly notifies users about conflicts with existing network keys. The error text is concise and helps pinpoint the cause quickly.Also applies to: 279-282
crates/settings/src/yaml/orderbook.rs (8)
1-1: New imports for remote networks and cache
These imports properly introduce theRemoteNetworksCfglogic and the newCachestruct.Also applies to: 3-4
24-24: Introducing a cache field inOrderbookYaml
Storing a dedicatedCacheinOrderbookYamlprovides a clean mechanism to retain and reuse remote networks data.
46-46: Validating remote networks in the constructor
AddingRemoteNetworksCfg::parse_all_from_yaml(...)ensures remote network configurations are also validated at initialization.
54-58: Default-initializingcacheinOrderbookYaml::new
Establishing a default cache while constructingOrderbookYamlpaves the way for consistent usage of in-memory remote network data later.
60-66: Copying the cache when converting fromOrderbookYaml
Retaining the same cache reference ensures no data is lost during the transition, maintaining consistency between YAML instances.
67-73: Bridging DOTRAIN YAML to ORDERBOOK YAML
Introducingfrom_dotrain_yamlprovides a straightforward pathway for passing shared state, including the cache, across configurations.
75-79: ImplementingContextProvider
Exposingget_remote_networks_from_cachemeets the new context-driven approach of handling remote networks. This encapsulation is clean and reusable.
252-255: Newusing-networks-fromYAML block for remote networks
Embedding these lines in the test YAML demonstrates how remote networks can be specified and integrated. This is a good illustrative example.crates/settings/src/remote_networks.rs (3)
40-75: Acknowledgment of sequential fetch design.A previous review suggested parallelizing the fetch for potential performance gains. However, retrieved learnings indicate that sequential fetching is intentional. If future requirements allow, you could implement concurrency (e.g.,
futures::join_all) for large lists to increase performance. For now, the existing approach might be acceptable.
134-148: Maintain consistency if new fields are added.Comparing only
key,url, andformatsuffices right now. If you introduce additional fields inRemoteNetworksCfglater, remember to incorporate them intoPartialEqto preserve consistent equality checks.
320-397: Comprehensive positive test coverage for remote network fetching.The
test_fetch_remote_networkstest effectively verifies remote networks are fetched from a valid JSON response and mapped intoNetworkCfg. This addresses the PR reviewer’s request for a Rust test ensuring correct URL-based parsing. Great job including these thorough checks.
|
@findolor can you please respond to the outstanding AI comments? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/settings/src/yaml/orderbook.rs(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- 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-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: Deploy-Preview
- GitHub Check: test
🔇 Additional comments (20)
crates/settings/src/yaml/orderbook.rs (20)
1-1: Import for cache
Looks good. No issues found.
3-4: Import for remote networks
Looks good. No issues found.
31-31: No changes
Function signature looks consistent. No issues.
45-46: Validating remote networks
This addition ensures remote networks are validated. Implementation looks good.
54-57: Initialize cache
UsingCache::default()is fine. No issues found.
60-65: Construct from existingOrderbookYaml
Implementation is straightforward. No issues found.
67-73: Construct fromDotrainYaml
Implementation is straightforward. No issues found.
75-77: ContextProvider implementation
No issues found. Implementation looks correct.
82-86: Initialize context
New methodinitialize_context_and_expand_networksis helpful. No issues found.
88-90: Using expanded context
Ensures remote networks are loaded before parsing. Good approach.
94-96: Get network with expanded context
Implementation is consistent. No issues found.
98-100: Context usage is None
Confirm if you intentionally do not pass the expanded context for remote networks. Possibly no context is needed. Please verify usage is correct.
102-102: No visible changes
No code differences here. Skipping.
104-105: Token keys with expanded context
Implementation is consistent. No concerns.
109-110: Get token with expanded context
Implementation is consistent. No concerns.
122-123: Orderbook keys
Implementation is consistent. No concerns.
127-128: Missing context expansion
In contrast toget_orderbook_keys, you're not callingexpand_context_with_remote_networks()here. If remote networks are relevant, consider using the same approach.
152-153: Deployer keys
Implementation is consistent. No issues found.
157-158: Get deployer
Ensures remote networks are loaded before parsing. Looks good.
23-24:Details
❓ Verification inconclusive
Check concurrency
You introduced a newcachefield. If multi-threaded usage is expected, ensurecacheis concurrency safe. Please confirm.
🏁 Script executed:
#!/bin/bash # Example search script to locate concurrency usage of `OrderbookYaml` or `Cache`. fd "\.rs$" | xargs rg -A 5 "OrderbookYaml|Cache"Length of output: 20798
Concurrency Check for the New Cache Field
After verifying the code, it appears that the new
cachefield inOrderbookYaml(atcrates/settings/src/yaml/orderbook.rs, lines 23–24) is of typeCache, which is implemented incrates/settings/src/yaml/cache.rsas a plain struct with aHashMapand no explicit concurrency guards (e.g., noMutexorRwLock).Please confirm if:
- Multi-threaded access to
OrderbookYaml(and hence itscachefield) is expected.- The current design guarantees thread safety (for example, if mutations are restricted or managed externally), or if it’s necessary to wrap
Cache(or its internalHashMap) in a thread-safe container (likeArc<Mutex<_>>orArc<RwLock<_>>).Once you decide on the intended concurrency model, either document the single-threaded usage assumptions or update the implementation to ensure thread safety where required.
2 participants
Motivation
See issue: #1396
We didn't have support for external networks in our yaml file. This PR adds that support.
Solution
using-networks-fromfield in yamlChecks
By submitting this for review, I'm confirming I've done the following:
fix #1396
Summary by CodeRabbit
New Features
DotrainYamlandOrderbookYamlstructs with caching capabilities.ChainIdinstances intoNetworkCfgobjects.Bug Fixes
Tests