Conversation
WalkthroughThe pull request enhances the YAML parsing logic across multiple modules by introducing new error variants, context-aware parsing, and caching for remote configurations. Changes include new methods and updated signatures that pass a context parameter to enable fetching and caching of remote networks and tokens. Additional modules and structures for handling remote tokens and networks have been added, along with new test cases to verify error handling for key collisions and duplicate entries. Changes
Sequence Diagram(s)Remote Networks Parsing FlowsequenceDiagram
participant Parser as NetworkCfgParser
participant Context
participant Cache
Parser->>Parser: parse_all_from_yaml(documents, context)
Note right of Parser: Checks if context has yaml_cache
Context->>Cache: Retrieve remote_networks
Cache-->>Parser: Return remote networks
Parser->>Parser: Check for key collision
alt Collision Detected
Parser-->>Parser: Return RemoteNetworkKeyShadowing error
else No Collision
Parser->>Parser: Update networks configuration
end
Remote Tokens Parsing FlowsequenceDiagram
participant Parser as TokenCfgParser
participant Context
participant Cache
Parser->>Parser: parse_all_from_yaml(documents, context)
Note right of Parser: Checks if context contains yaml_cache for remote_tokens
Context->>Cache: Retrieve remote_tokens
Cache-->>Parser: Return remote tokens
Parser->>Parser: Perform key collision check
alt Collision Detected
Parser-->>Parser: Return RemoteTokenKeyShadowing error
else No Collision
Parser->>Parser: Update token configuration
end
Assessment against linked issues
✨ 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: 15
🔭 Outside diff range comments (2)
crates/settings/src/remote_networks.rs (1)
161-311: 🧹 Nitpick (assertive)Test coverage focuses on YAML parsing.
These tests demonstrate resilience to malformed YAML or duplicated keys. Additional integration tests around thefetch_networksmethod could be beneficial.crates/settings/src/remote_tokens.rs (1)
129-172: 🧹 Nitpick (assertive)Expand test coverage to include “fetch_tokens”.
While the parsing tests are comprehensive, consider adding tests forfetch_tokens—especially verifying conflict detection and network call handling (mocked or otherwise).Would you like help in setting up a test using a mock server to simulate remote token data?
📜 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 (20)
crates/common/src/dotrain_order/mod.rs(4 hunks)crates/settings/src/deployer.rs(1 hunks)crates/settings/src/gui.rs(1 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/mod.rs(1 hunks)crates/settings/src/remote/tokens.rs(1 hunks)crates/settings/src/remote_networks.rs(1 hunks)crates/settings/src/remote_tokens.rs(1 hunks)crates/settings/src/scenario.rs(1 hunks)crates/settings/src/token.rs(3 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 (10)
crates/settings/src/order.rs (4)
crates/settings/src/token.rs (1) (1)
parse_all_from_yaml(228-318)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/orderbook.rs (4)
crates/settings/src/token.rs (1) (1)
parse_all_from_yaml(228-318)crates/settings/src/order.rs (1) (1)
parse_all_from_yaml(475-775)crates/settings/src/deployer.rs (1) (1)
parse_all_from_yaml(128-193)crates/settings/src/scenario.rs (1) (1)
parse_all_from_yaml(328-378)
crates/settings/src/yaml/mod.rs (2)
crates/settings/src/yaml/orderbook.rs (2) (2)
from_orderbook_yaml(67-72)from_dotrain_yaml(74-79)crates/settings/src/yaml/dotrain.rs (2) (2)
from_orderbook_yaml(66-71)from_dotrain_yaml(59-64)
crates/settings/src/remote_tokens.rs (1)
crates/settings/src/remote_networks.rs (4) (4)
validate_url(36-38)default(132-139)eq(142-144)reqwest(48-50)
crates/settings/src/deployer.rs (4)
crates/settings/src/token.rs (1) (1)
parse_all_from_yaml(228-318)crates/settings/src/order.rs (1) (1)
parse_all_from_yaml(475-775)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/scenario.rs (4)
crates/settings/src/token.rs (1) (1)
parse_all_from_yaml(228-318)crates/settings/src/order.rs (1) (1)
parse_all_from_yaml(475-775)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/yaml/dotrain.rs (2)
crates/settings/src/yaml/orderbook.rs (3) (3)
from_documents(60-65)from_dotrain_yaml(74-79)from_orderbook_yaml(67-72)crates/settings/src/yaml/mod.rs (3) (3)
from_documents(28-28)from_dotrain_yaml(30-30)from_orderbook_yaml(29-29)
crates/settings/src/token.rs (4)
crates/settings/src/order.rs (1) (1)
parse_all_from_yaml(475-775)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 (2) (2)
parse_all_from_yaml(328-378)key(197-197)
crates/settings/src/yaml/cache.rs (1)
crates/settings/src/yaml/context.rs (8) (8)
get_remote_networks(141-141)get_remote_networks(155-159)get_remote_network(143-143)get_remote_network(161-165)get_remote_tokens(145-145)get_remote_tokens(167-171)get_remote_token(147-147)get_remote_token(173-177)
crates/settings/src/remote/tokens.rs (2)
crates/settings/src/yaml/orderbook.rs (1) (1)
new(30-58)crates/common/src/dotrain_order/mod.rs (1) (1)
new(136-173)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: git-clean
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- 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-rs-static)
- 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-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: test
- GitHub Check: Deploy-Preview
- GitHub Check: build-tauri (ubuntu-22.04, true)
🔇 Additional comments (77)
crates/settings/src/remote/mod.rs (1)
2-2: Added new tokens module to remote package.This change exposes the new tokens module as part of the public API, enabling remote token configurations to be accessed from other parts of the codebase.
crates/settings/src/lib.rs (1)
16-17: Added new remote configuration modules.The addition of
remote_networksandremote_tokensmodules aligns with the PR objective to support remote token configurations. These modules will expose the functionality for parsing and managing remote network and token configurations.crates/settings/src/gui.rs (1)
567-567: Updated to use context in token parsing.This change passes the context parameter to
TokenCfg::parse_all_from_yamlinstead ofNone, which enables the function to access context information during parsing. This is essential for the remote token support, allowing the parser to access cached remote configurations.crates/settings/src/remote/chains/chainid.rs (2)
1-7: Added necessary imports for network configuration functionality.Added imports for
Arc,RwLock,StrictYaml, and updated the import forNetworkCfgto support the new network configuration conversion functionality.
89-112: Added method to convert ChainId to NetworkCfg.The new
try_into_network_cfgmethod converts a ChainId instance to a NetworkCfg, which is needed for remote network configuration support. The implementation follows similar logic to the existing NetworkConfigSource conversion.The method properly handles error cases:
- Returns
ChainIdError::NoRpcif no RPC URLs are provided- Returns
ChainIdError::UnsupportedRpcUrlsif none of the provided RPC URLs are compatible- Filters out URLs containing "API_KEY" or starting with "ws" scheme
crates/settings/src/orderbook.rs (2)
75-75: Updated parameter name to reflect actual usageThe parameter name has been changed from
_(indicating unused) tocontext, correctly reflecting that the parameter is now being used in the function body.
79-80: Propagating context to dependency parsersNow passing the context to both NetworkCfg and SubgraphCfg parsers, which allows for context-aware parsing and potential remote configuration support.
crates/settings/src/scenario.rs (1)
334-334: Propagating context to deployer parserNow passing the context to DeployerCfg parser instead of None, enabling context-aware parsing for deployers, consistent with changes in other modules.
crates/settings/src/yaml/mod.rs (3)
1-1: Added new cache moduleAdded a new module for caching, which is important for storing and retrieving remote token configurations efficiently.
13-14: Added imports for YAML typesImported DotrainYaml and OrderbookYaml types, which are necessary for the new trait methods to support remote tokens.
29-30: Extended YamlParsable trait with new conversion methodsAdded two new methods to allow conversion between different YAML representation types, enabling more flexible parsing flows for remote token configurations.
crates/settings/src/order.rs (2)
481-483: Propagating context to dependency parsersNow passing the context to deployers, orderbooks, and tokens parsers, which enables context-aware parsing throughout the dependency chain.
485-491: Enhanced error handling with context awarenessAdded conditional logic to handle token parsing errors based on context state. This ensures appropriate error propagation when token validation is required but fails.
crates/settings/src/deployer.rs (2)
130-130: Added context parameter to improve YAML parsing capabilities.The parameter change from
_: Option<&Context>tocontext: Option<&Context>allows this method to utilize contextual information during parsing, which is essential for the remote token support functionality.
134-134: Updated to pass context to NetworkCfg parser.This change ensures that any contextual information (particularly remote networks) is properly propagated to the network configuration parser.
crates/settings/src/network.rs (3)
145-145: Added context parameter to improve context-aware parsing.The parameter change from
_: Option<&Context>tocontext: Option<&Context>enables utilizing contextual information during network configuration parsing.
214-225: Enhanced parser to handle remote networks from context.This new code block properly handles remote networks by:
- Checking if context and yaml_cache are available
- Iterating through remote networks
- Preventing key collisions between local and remote networks
- Adding remote networks to the networks collection
This is a crucial part of the remote token support implementation.
262-263: Added appropriate error variant for remote network key shadowing.This new error variant provides clear error messaging when a remote network conflicts with a local network of the same name.
crates/settings/src/yaml/orderbook.rs (16)
1-1: Updated imports to include Cache.The new import supports the cache functionality needed for remote networks and tokens.
3-4: Added imports for RemoteNetworksCfg and RemoteTokensCfg.These imports provide access to the remote network and token functionality being implemented.
24-24: Added cache field to OrderbookYaml struct.This field allows the struct to store and manage cached remote networks and tokens.
45-46: Added validation for RemoteNetworksCfg.This ensures that the remote networks configuration is properly validated during initialization.
54-57: Initialize cache with default value.Ensures the cache is properly initialized when creating a new OrderbookYaml instance.
60-79: Updated instance creation methods to initialize cache.These modifications ensure that the cache is properly initialized in all ways that an OrderbookYaml instance can be created, maintaining consistency across the codebase.
83-89: Enhanced get_networks to utilize context with remote networks.This method now creates a context with remote networks from the cache, enabling proper merging of local and remote networks during parsing.
95-99: Enhanced get_network to utilize context with remote networks.Similar to get_networks, this method now uses context to ensure remote networks are considered when retrieving a specific network.
101-104: Added get_remote_networks method.This new method provides access to the remote networks configuration, which is essential for the remote token support functionality.
107-118: Updated token methods to utilize context with remote networks.These methods now create a context with remote networks from the cache, ensuring that tokens can be properly associated with networks, including remote ones.
120-127: Added get_remote_tokens method.This method provides access to the remote tokens configuration and ensures proper context with remote networks is used during parsing.
138-149: Updated orderbook methods to utilize context with remote networks.These methods now use context to ensure remote networks are considered when working with orderbooks.
163-174: Updated deployer methods to utilize context with remote networks.These methods now use context to ensure remote networks are considered when working with deployers.
231-234: Updated deserialization to initialize cache.Ensures that the cache is properly initialized when deserializing an OrderbookYaml instance.
257-260: Added using-networks-from configuration to test YAML.This addition to the test YAML provides coverage for the new remote networks functionality.
332-337: Added tests for remote networks functionality.These additional test assertions verify that the remote networks are properly parsed and accessible.
crates/common/src/dotrain_order/mod.rs (4)
12-13: Added imports for remote networks and tokens modules.These imports provide access to the remote configuration functionality being implemented.
85-88: Added error variants for remote networks and tokens parsing.These new error variants provide specific error handling for issues related to remote networks and tokens, improving error clarity and debugging.
149-168: Implemented remote networks and tokens fetching.This critical addition:
- Creates instances of OrderbookYaml and DotrainYaml
- Fetches remote networks from the configured sources
- Updates the caches in both YAML structures when remote networks are found
- Optionally fetches and caches remote tokens if configured
This is a core part of the remote token support implementation.
246-246: Updated orderbook_yaml method to use from_dotrain_yaml.This change ensures that the cache from dotrain_yaml is properly propagated to the OrderbookYaml instance.
packages/orderbook/test/js_api/gui.test.ts (7)
338-443: New constant for remote network and token testingThe new
dotrainForRemotesconstant adds comprehensive test configurations for remote networks and tokens functionality. This setup is well-structured with clear sections for GUI, networks, remote network sources, tokens, and deployments.
1789-1828: Successfully testing remote network fetchingGood test implementation for the happy path of remote network fetching. The test properly mocks HTTP responses and verifies that the application can successfully process remote network data.
1830-1870: Testing handling of duplicate network keys in responseThis test appropriately verifies error handling when a remote source returns duplicate network keys, ensuring the system detects conflicts properly.
1872-1925: Testing network key shadowing detectionThis test correctly validates that the system detects and rejects remote networks that would shadow existing network keys, preventing potential conflicts.
1929-1975: Successfully testing remote token fetchingGood implementation of the happy path test for remote token fetching. The test properly mocks HTTP responses and verifies that the application can successfully process token data.
1977-2033: Testing duplicate token keys in responseThis test appropriately verifies error handling when remote sources return duplicate token keys, ensuring the system detects conflicts properly.
2035-2082: Testing token key shadowing detectionThis test correctly validates that the system detects and rejects remote tokens that would shadow existing token keys, preventing potential conflicts.
crates/settings/src/yaml/dotrain.rs (7)
1-1: Added cache importThe new import for
Cacheis properly added to support the cache functionality being introduced in this PR.
20-20: Added cache field to DotrainYaml structThe cache field addition is appropriate for storing remote networks and tokens, supporting the PR's objective of adding remote token functionality.
46-49: Updated struct initialization with cacheThe
newmethod now properly initializes the cache field with a default value.
53-57: Updated from_documents method with cache initializationCorrectly initialized cache in the
from_documentsmethod to maintain caching capability when creating instances from YAML documents.
59-64: Added from_dotrain_yaml method for cache inheritanceNew method provides a way to create a
DotrainYamlfrom another instance while preserving the cache, which is important for maintaining state across transformations.
109-111: Added remote network and token contextThe
get_guimethod now properly adds remote networks and tokens to the context, enabling the GUI to use this data when rendering components.
160-163: Updated Deserialize implementation with cache initializationThe deserialization implementation now correctly initializes the cache when creating an instance from a sequence of YAML documents.
crates/settings/src/remote/tokens.rs (5)
1-11: Appropriate imports for token handlingThe imports cover all necessary dependencies for working with tokens, network configurations, serialization, and error handling.
12-18: Version struct for token metadataWell-structured struct with appropriate serialization annotations for handling version information in token metadata.
20-28: Token struct for remote token representationClean implementation of a token data structure with all necessary fields (chain_id, address, name, symbol, decimals) and proper serialization annotations.
30-39: Tokens collection structWell-designed container struct for holding a collection of tokens along with metadata such as name, timestamp, version, and logo URI.
71-79: Error handling for token operationsThe
RemoteTokensErrorenum provides clear and specific error types for addressing various failure scenarios in token processing.crates/settings/src/token.rs (2)
230-234: Incorporation ofcontextparameter looks good.
The addition ofcontext: Option<&Context>and passing it toNetworkCfg::parse_all_from_yamlenables remote configuration awareness during parsing. This design appropriately leverages the optional context for advanced YAML parsing without breaking existing usage.
353-354: New error variant aligns with existing error handling patterns.
DeclaringRemoteTokenKeyShadowingcleanly captures conflict scenarios for remote token merges and is consistent with the established domain error styles.crates/settings/src/yaml/cache.rs (4)
10-12: Bulk update method is straightforward.
Assigning the entireremote_networksmap is clear and self-explanatory.
13-15: Single-entry update method is well-defined.
Usinginsertto update or insert a singleremote_networkis concise.
23-25: Mirrors the pattern for bulk remote token update.
Same logic as updating networks. Straightforward approach.
26-28: Single-entry remote token upsert is clear.
Matches the method for networks, offering consistency.crates/settings/src/remote_networks.rs (6)
1-15: Imports and setup look standard.
Includes necessary crates (reqwest,url,serde, etc.) for async network requests and YAML parsing.
16-19:RemoteNetworksenum is well-defined.
TheChainIdvariant is a logical representation of chain-based network data.
21-31:RemoteNetworksCfgstruct for external network sources.
Use ofUrltype ensures compile-time validation of the string. Storingformatunifies handling of different data types.
78-129:parse_all_from_yamlproperly handles remote network references.
This aligns nicely with theYamlParsableHashtrait, providing detailed errors for missing or invalid fields. No obvious logical issues.
131-145:DefaultandPartialEqimplementations are straightforward.
They ensure testability and fallback usage forRemoteNetworksCfg.
147-159:ParseRemoteNetworksErrorenumerates relevant errors.
Captures URL parse failures, unknown formats, request failures, conflicts, and chain ID errors. This coverage is comprehensive.crates/settings/src/remote_tokens.rs (4)
1-15: Consider verifying the necessity of all imports.
Everything seems consistent with usage in this file, but ensure that the references tocrate::remote::tokens::{RemoteTokensError, Tokens}are correct and do not overlap confusingly with this module’s name,remote_tokens. If these are indeed separate functionalities, this is good to go.
27-31: URL validation approach is aligned with existing patterns.
The validation logic is straightforward and consistent withParseRemoteTokensError::UrlParseError. No issues spotted here.
68-101: Optional parsing logic looks correct.
Handling both zero or one occurrences ofusing-tokens-fromand erroring on duplicates ensures consistent behavior. The error mapping and field-level validations appear well-structured.
117-127: Error definitions are well-structured.
Your specialized error variants clearly communicate conflict, parsing, and request failures. This approach promotes targeted error handling.crates/settings/src/yaml/context.rs (4)
11-15: Cache struct design is concise.
DefiningYamlCachewith separate maps for networks and tokens is a neat way to keep them isolated. No immediate issues noticed.
138-148: Trait definition clarifies responsibilities.
TheYamlCacheTraitextension clearly spells out retrieval methods for remote network/token data, enhancing maintainability.
150-178: Implementation ensures structured access.
Usingand_thenis a clean way to deal with both theOptionnature ofyaml_cacheand the top-level fields. This is good standard Rust usage.
186-187: Deep cloning of YAML caches.
Cloningyaml_cacheensures new contexts get an independent copy. Confirm that full deep copying is desired, especially if the original context’s changes should remain decoupled.Also applies to: 196-197
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/settings/src/yaml/dotrain.rs(5 hunks)crates/settings/src/yaml/orderbook.rs(8 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
crates/settings/src/yaml/orderbook.rs (5)
crates/settings/src/yaml/mod.rs (11) (11)
parse_all_from_yaml(49-52)parse_all_from_yaml(67-67)from_documents(28-28)from_orderbook_yaml(29-29)from_dotrain_yaml(30-30)new(26-26)parse_from_yaml(54-63)parse_from_yaml(71-71)parse_from_yaml(79-82)parse_from_yaml_optional(73-75)parse_from_yaml_optional(84-87)crates/settings/src/network.rs (2) (2)
parse_all_from_yaml(143-235)default(239-241)crates/settings/src/yaml/dotrain.rs (4) (4)
from_documents(52-57)from_orderbook_yaml(66-71)from_dotrain_yaml(59-64)new(26-50)crates/common/src/dotrain_order/mod.rs (3) (3)
orderbook_yaml(245-247)dotrain_yaml(241-243)new(136-173)crates/settings/src/yaml/context.rs (5) (5)
new(181-188)get_remote_networks(141-141)get_remote_networks(155-159)get_remote_tokens(145-145)get_remote_tokens(167-171)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- 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-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: test
- GitHub Check: Deploy-Preview
🔇 Additional comments (21)
crates/settings/src/yaml/dotrain.rs (5)
46-49: Ensure clarity on default cache usage
Returning a default cache may surprise some users who expect a populated or inherited cache. Confirm that this default-initialized cache won't lead to key collisions or stale data.
53-58: Retaining a fresh cache
When constructingDotrainYamlfrom these documents, you default to a newCache. This is consistent with the approach innew(). Make sure any previously intended remote data is re-fetched if needed.
59-65: Inherited cache
Good job inheriting the cache fromdotrain_yamlinstead of re-initializing it. This keeps previously loaded remote data available and consistent.
66-70: Reusing cache fromOrderbookYaml
Passing along theorderbook_yaml.cacheensures any existing remote network or token data remains accessible inDotrainYaml.
105-111: Add remote networks and tokens to context
Enabling remote data by callingadd_remote_networksandadd_remote_tokensensures the GUI logic can incorporate external definitions. Consider verifying whether failures or empty data are handled gracefully.crates/settings/src/yaml/orderbook.rs (16)
1-1: ImportingCache
This line ensures theCachemodule is available for storing remote data. Straightforward addition with no major concerns.
3-5: Importing additional modules
The lines introduceRemoteNetworksCfgandRemoteTokensCfg, which enhance the ability to handle external resources. This appears consistent with the goal of adding remote token/network support.
46-46: Parsing remote networks during validation
IncludingRemoteNetworksCfg::parse_all_from_yamlhere ensures remote networks get validated together with other configurations. This helps unify local and remote checks.
67-72: Inheriting cache from existingOrderbookYaml
Retaining the same cache infrom_orderbook_yamlkeeps remote data consistent when converting. Looks aligned with previous caching feedback.
74-79: Cross-conversion from Dotrain
Seamlessly mergesdotrain_yamldocuments and cache intoOrderbookYaml. This is a logical approach to maintain uniform remote data references.
83-89: Composing local and remote networks
By creating a new context and adding remote networks from the cache,get_networksmerges both sets of data. Confirm potential naming collisions are handled gracefully.
91-91: Refactored retrieval
Callingself.get_networks()inget_network_keysis a DRY approach, ensuring consistent network loading logic.
95-99: Single network retrieval
Same pattern of merging local documents and remote data for an individual network. Consistency across both methods is good.
107-110: Remote networks for token context
Applying the remote network data in token retrieval aligns with the approach for network retrieval, ensuring references remain consistent across modules.
114-118: Token retrieval with remote context
Incorporating remote networks for a single token ensures consistent environment data. This approach keeps local and remote definitions synchronized.
138-141: Combine local and remote for orderbooks
Creating a context with remote networks before parsing local orderbook data is consistent. If remote definitions conflict with local ones, please confirm error handling.
145-148: Single orderbook retrieval
Mirroring the pattern used elsewhere, you load remote networks for a single key. This is consistent design and should reduce code duplication.
163-166: Deployer keys with remote data
Involving remote networks for deployers expands the possibilities for referencing them externally. The approach matches other parts of your code.
170-173: Deployer retrieval
Merging local documents with remote networks at fetch time is consistent. Be mindful of naming or key collisions if the same deployer keys appear in local and remote data.
257-260:using-networks-fromin test
This test snippet exercises the new remote network references. Confirm that offline tests or CI runs won’t be impacted if the external source is unavailable.
331-337: Validating chainid data
Verifying the remote URL for “chainid” ensures the feature is working end-to-end. Good test coverage for parsing remote network references.
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (2)
crates/settings/src/yaml/dotrain.rs (2)
173-176: 🧹 Nitpick (assertive)Deserializer defaults to a fresh cache.
As with the constructor, the deserializer does not rehydrate remote data. This might be acceptable if the cache is always fetched from external sources.
Ok(DotrainYaml { documents, - cache: Cache::default(), + // Possibly deserialize the cache if you want to preserve remote data across sessions. ... })
46-49: 🧹 Nitpick (assertive)Consider exposing an option to reuse an existing cache.
Here, a default cache is always injected into
DotrainYaml. This might prevent scenarios where a caller wants to share or reuse a cache across multiple YAML instances.Ok(DotrainYaml { documents, - cache: Cache::default(), + // Provide an optional parameter or a separate constructor if a non-default cache is desired. + cache: cache.unwrap_or_else(Cache::default), })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
crates/common/src/dotrain_order/mod.rs(5 hunks)crates/settings/src/deployer.rs(1 hunks)crates/settings/src/gui.rs(1 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/remote_tokens.rs(1 hunks)crates/settings/src/scenario.rs(1 hunks)crates/settings/src/token.rs(4 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)crates/settings/src/yaml/orderbook.rs(8 hunks)packages/orderbook/test/js_api/gui.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
crates/common/src/dotrain_order/mod.rs (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1477
File: crates/settings/src/yaml/dotrain.rs:20-20
Timestamp: 2025-03-31T18:53:12.208Z
Learning: In the DotrainYaml struct, the cache field is kept public (`pub cache: Cache`) without additional getter methods, which is consistent with the existing design pattern in the codebase where other fields like `documents` are also public.
crates/settings/src/yaml/dotrain.rs (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1477
File: crates/settings/src/yaml/dotrain.rs:20-20
Timestamp: 2025-03-31T18:53:12.208Z
Learning: In the DotrainYaml struct, the cache field is kept public (`pub cache: Cache`) without additional getter methods, which is consistent with the existing design pattern in the codebase where other fields like `documents` are also public.
🧬 Code Definitions (9)
crates/settings/src/orderbook.rs (6)
crates/settings/src/yaml/mod.rs (2)
parse_all_from_yaml(49-52)parse_all_from_yaml(67-67)crates/settings/src/order.rs (1)
parse_all_from_yaml(475-775)crates/settings/src/deployer.rs (1)
parse_all_from_yaml(139-207)crates/settings/src/scenario.rs (1)
parse_all_from_yaml(331-381)crates/settings/src/network.rs (1)
parse_all_from_yaml(143-235)crates/settings/src/subgraph.rs (1)
parse_all_from_yaml(91-142)
crates/settings/src/order.rs (5)
crates/settings/src/token.rs (1)
parse_all_from_yaml(234-324)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/yaml/mod.rs (2)
parse_all_from_yaml(49-52)parse_all_from_yaml(67-67)
crates/settings/src/scenario.rs (3)
crates/settings/src/order.rs (1)
parse_all_from_yaml(475-775)crates/settings/src/deployer.rs (1)
parse_all_from_yaml(139-207)crates/settings/src/yaml/mod.rs (2)
parse_all_from_yaml(49-52)parse_all_from_yaml(67-67)
crates/settings/src/deployer.rs (5)
crates/settings/src/yaml/mod.rs (3)
new(26-26)parse_all_from_yaml(49-52)parse_all_from_yaml(67-67)crates/settings/src/order.rs (1)
parse_all_from_yaml(475-775)crates/settings/src/scenario.rs (1)
parse_all_from_yaml(331-381)crates/settings/src/deployment.rs (1)
parse_all_from_yaml(61-135)crates/settings/src/network.rs (1)
parse_all_from_yaml(143-235)
crates/settings/src/token.rs (6)
crates/settings/src/yaml/context.rs (1)
new(196-203)crates/settings/src/yaml/mod.rs (3)
new(26-26)parse_all_from_yaml(49-52)parse_all_from_yaml(67-67)crates/settings/src/order.rs (1)
parse_all_from_yaml(475-775)crates/settings/src/deployer.rs (1)
parse_all_from_yaml(139-207)crates/settings/src/scenario.rs (1)
parse_all_from_yaml(331-381)crates/settings/src/network.rs (1)
parse_all_from_yaml(143-235)
crates/settings/src/yaml/cache.rs (1)
crates/settings/src/yaml/context.rs (9)
new(196-203)get_remote_networks(156-156)get_remote_networks(170-174)get_remote_network(158-158)get_remote_network(176-180)get_remote_tokens(160-160)get_remote_tokens(182-186)get_remote_token(162-162)get_remote_token(188-192)
crates/settings/src/yaml/context.rs (1)
crates/settings/src/yaml/cache.rs (5)
get_remote_networks(23-25)get_remote_network(26-28)get_remote_tokens(36-38)get_remote_token(39-41)new(10-15)
crates/settings/src/yaml/dotrain.rs (1)
crates/settings/src/yaml/orderbook.rs (5)
from_documents(60-65)from_dotrain_yaml(74-79)from_orderbook_yaml(67-72)get_remote_networks_from_cache(83-85)get_remote_tokens_from_cache(87-89)
crates/settings/src/gui.rs (4)
crates/settings/src/token.rs (1)
parse_all_from_yaml(234-324)crates/settings/src/order.rs (1)
parse_all_from_yaml(475-775)crates/settings/src/deployer.rs (1)
parse_all_from_yaml(139-207)crates/settings/src/yaml/mod.rs (2)
parse_all_from_yaml(49-52)parse_all_from_yaml(67-67)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- 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-wasm-artifacts)
- 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, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: Deploy-Preview
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (88)
crates/settings/src/gui.rs (1)
567-567: Pass context to enable remote token supportThis change enables the
contextparameter to be passed toTokenCfg::parse_all_from_yamlinstead ofNone, allowing the function to access remote tokens when parsing YAML configurations.crates/settings/src/orderbook.rs (2)
75-75: Updated parameter name to use contextRenamed the unused parameter
_tocontextto enable passing context information through the function call.
79-80: Pass context to network and subgraph parsingUpdated the calls to
NetworkCfg::parse_all_from_yamlandSubgraphCfg::parse_all_from_yamlto pass thecontextparameter, enabling access to remote networks and subgraphs.crates/settings/src/scenario.rs (1)
337-337: Context propagation ensures consistent value interpolation.The change to pass the
contextparameter toDeployerCfg::parse_all_from_yamlproperly propagates context down the configuration hierarchy, allowing for consistent YAML parsing behavior across different configuration types.This change allows for important functionality like remote token resolution and string interpolation to work consistently for deployer configurations too, aligning with this PR's goal of adding remote token support to YAML.
crates/settings/src/deployer.rs (2)
141-141: Naming parameter for consistency with interface.Properly naming the parameter
contextinstead of_allows the parameter to be used within the method implementation.
145-145: Context propagation improves network configuration parsing.Propagating the context parameter to
NetworkCfg::parse_all_from_yamlensures that network configurations can also benefit from context-aware parsing, which is essential for remote token and network resolution.This change is part of a consistent pattern throughout the codebase where context is passed down the configuration hierarchy, enabling features like interpolation and remote resource resolution at every level.
crates/settings/src/order.rs (2)
481-483: Updated parameter passing to support context-aware parsingThe changes now correctly pass the
contextparameter to the parsing functions for deployers, orderbooks, and tokens, enabling them to use contextual information during parsing.
485-491: Added early error handling for tokens parsingThe new conditional block adds appropriate error handling when the context is provided but
select_tokensis None. This ensures that token parsing errors are properly propagated early in the parse process, improving error feedback.crates/settings/src/network.rs (4)
145-145: Parameter naming improved for clarityChanged the parameter name from
_(indicating unused) tocontext, which better reflects its purpose and usage in the implementation.
214-225: Added support for remote networks from cacheThis new block properly handles remote networks from the context's yaml_cache, checking for key collisions before inserting them into the networks HashMap. This prevents duplicate network definitions and provides clear error messages when conflicts occur.
262-264: Added new error variant for remote network conflictsThe new
RemoteNetworkKeyShadowingerror variant handles the case when a remote network key conflicts with a local network definition, providing appropriate error information.
281-285: Added user-friendly error message for network key conflictsThe implementation provides a clear and descriptive error message for the remote network key shadowing case, helping users understand and resolve configuration conflicts.
packages/orderbook/test/js_api/gui.test.ts (3)
337-442: New configuration for remote token support looks good.The new
dotrainForRemotesconfiguration correctly sets up the remote networks and tokens functionality with appropriate test data. It includes both theusing-networks-fromandusing-tokens-fromfields needed to test the new feature.
2030-2177: Well-structured tests for remote network functionality.These tests cover the key scenarios for remote networks including successful fetching and proper error handling for duplicate keys. The test cases verify both the error message and the readable version of the error, which is good practice.
2179-2346: Comprehensive test coverage for remote tokens.The remote tokens tests mirror the network tests with appropriate scenarios: successful fetch, duplicate token handling within the response, and conflict detection with local tokens. The error messages are clear and helpful for debugging.
crates/common/src/dotrain_order/mod.rs (4)
12-17: New imports for remote tokens and networks look appropriate.These added imports align well with the new remote network and token fetching logic.
86-91: Properly introduced error variants for remote parsing issues.The new
ParseRemoteNetworksErrorandParseRemoteTokensErroradditions are well-defined, ensuring more granular error handling.
259-261: Switched tofrom_dotrain_yamlseamlessly.The new call to
OrderbookYaml::from_dotrain_yamlis consistent with the rest of the codebase and appears to be correctly integrated.
163-181:Details
❓ Verification inconclusive
Potential discrepancy in how remote tokens are cached vs. networks.
While both
orderbook_yamlanddotrain_yamlget updated with remote networks, onlydotrain_yamlis updated with remote tokens. Ensure this divergence is intentional, as it may leaveorderbook_yaml’s cache out of sync for tokens.If it’s unintended, I can help propose a fix or open a new issue to unify token caching.
Verify Token Caching Divergence
The code updates remote networks for both caches but only updates remote tokens for
dotrain_yaml. Please verify whether this divergence (i.e. omitting a token cache update fororderbook_yaml) is intentional. If it’s not by design, consider unifying the token caching between the two YAMLs or explain/examine why they should differ.crates/settings/src/token.rs (3)
236-240: Context-aware parsing of tokens.Allowing
parse_all_from_yamlto acceptcontextenables a more flexible parsing flow and dovetails nicely with the revised logic for merging remote tokens.
303-315: Properly merges remote tokens fromyaml_cache, but consider test coverage.Merging tokens from
context.yaml_cache.remote_tokenswith collision detection prevents unintended key shadowing. This is a repeat of a past suggestion about ensuring test coverage for these collision scenarios.
359-360: Refined error handling for remote token collisions.Introducing the
RemoteTokenKeyShadowingvariant is a clean way to signal conflicts.crates/settings/src/yaml/dotrain.rs (7)
20-20: Add doc comments or usage guidance for the publiccachefield.This mirrors a previous suggestion about documenting public fields to clarify usage and lifetime constraints.
59-63: Looks good for copying the cache from an existing DotrainYaml.This straightforward clone-and-transfer approach is consistent with typical struct conversions. No immediate issues spotted.
66-70: Consistent bridging from OrderbookYaml to DotrainYaml.This aligns well with the approach taken in
from_dotrain_yaml, ensuring that both documents and the cache are carried over.
74-81: ContextProvider implementation is consistent with OrderbookYaml.Methods
get_remote_networks_from_cache&get_remote_tokens_from_cachesimply forward calls to the underlying cache. This is straightforward and maintains parallel structure found elsewhere.
91-93: Context expansions appear correct but verify upstream usage.These calls ensure remote data is merged into context before parsing the order. Confirm that any partial or missing remote tokens do not lead to unhandled errors.
Would you like a script to scan for all calls to
get_orderand confirm correct error handling around missing tokens or networks?
112-114: Consistently expands deployment context with remote data.Repeating the same context expansion pattern as in
get_order. No obvious issues.
121-123: Extends GUI context with remote networks and tokens.Again, follows the same pattern—ensuring that remote references are recognized when rendering a GUI.
crates/settings/src/yaml/orderbook.rs (18)
24-24: Add doc comments or usage instructions for the publiccachefield.Documenting how and when the cache is refreshed or shared would help users understand concurrency and synchronization concerns.
46-46: Validate tokens after remote networks.The code calls
TokenCfg::parse_all_from_yamlafter loading remote networks. If remote tokens rely on remote networks, the order here is appropriate. No issues spotted.
67-71: Transfer existing cache seamlessly.Copying both documents and cache ensures no data is lost. Implementation looks correct.
74-79: Constructing from DotrainYaml.Mirrors the bridging approach in DotrainYaml. Straightforward, no immediate red flags.
82-90: ContextProvider for OrderbookYaml is consistent with DotrainYaml.The
get_remote_networks_from_cacheandget_remote_tokens_from_cachemethods are symmetrical. This alignment improves maintainability.
94-98: Check thatcreate_context()merges well with remote expansions.Ensure no overshadowing or redefinition of context fields occurs.
100-104: Expanding remote networks before callingNetworkCfg::parse_all_from_yaml.Well-structured approach; no obvious code smell.
106-110: Expanding remote networks before parsing a single network.Same pattern as above—no concerns.
118-121: Expanding context for tokens with remote data.Method is consistent with the approach for networks.
126-131: Same expansions forget_token.No new issues; consistent usage.
133-133:get_remote_tokensmerges context for networks before optional remote tokens.This is logical if tokens might reference networks.
134-139: Optional parse for remote tokens.No strong concerns, though ensure partial success or missing data is handled properly.
150-150: Fetchingorderbook_keyswith context expansion.Remains consistent with the rest of the expansions.
151-156: Ensure remote expansions are relevant for orderbooks.If an orderbook references a remote network, the expansions are crucial. Otherwise, no performance penalty is too large.
158-162: Expanding context for a single orderbook.Same pattern; no immediate issues.
176-178: Context expansion for deployers.Likely needed if deployers can reference remote networks.
179-179: Ensure any missing remote networks or invalid references are properly handled.Though
parse_all_from_yamlmight throw an error, verifying that caller code checks for it is helpful.
183-187: Same approach for retrieving a single deployer.Matches the expansions used elsewhere. Nicely uniform.
crates/settings/src/remote_networks.rs (6)
1-19: Imports, type definitions, and top-level enums look straightforward.
- Uses
reqwestandUrlfor HTTP and URL parsing.- Introduces
RemoteNetworksenum, focusing on “ChainId” data.
No glaring security or concurrency issues.
21-31:RemoteNetworksCfgstruct handles remote YAML config details.
- Holds URL, format, and a key for uniqueness.
- The
#[serde(skip, default)]approach fordocumentis flexible.
No immediate concerns, but confirm usage in concurrency-laden contexts.
78-132:YamlParsableHashimplementation for RemoteNetworksCfg.
- Extracts "using-networks-from" as a hash of remote configs in YAML.
- Validates that each entry has both a
urland aformat.- Returns a specialized error if the key is repeated or fields are invalid.
Implementation is solid and thorough.
134-148:DefaultandPartialEqfor RemoteNetworksCfg.
- The default sets a placeholder URL. That’s acceptable for test or fallback.
PartialEqchecks only the key, url, and format. Document thatdocumentis ignored in equality.Would you like to confirm that ignoring
documentin equality doesn’t produce unexpected comparisons in user code?
150-162:ParseRemoteNetworksErrorrobustly enumerates error cases.
- Covers URL parse errors, unknown formats, request failures, conflicts, plus chain ID issues.
- This thorough approach will help with debugging.
No further suggestions.
164-317: Test coverage looks detailed and valuable.
- Exercises missing
url, invalidurl, missingformat, invalidformat, duplicates, etc.- This helps ensure stable behavior.
crates/settings/src/yaml/cache.rs (10)
1-3: No concerns on imports.
These imports are straightforward and align with the module’s needs.
4-8: Struct definition is clear, but watch for large data clones.
This reiterates a previous note on potential cloning overhead for large datasets.
9-15: Constructor looks good.
This is a succinct and self-explanatory constructor.
17-19: Simple setter approach is acceptable.
Replacing the entire map may be useful for atomic updates.
23-25: Returning a cloned map may be expensive.
Consider returning a reference if it fits your read-use cases.
26-28: Cloning can be avoided here, too.
A reference might be enough for the caller, depending on usage.
30-32: Token setter approach is straightforward.
Using a single update method keeps data modifications uniform.
33-35: Insert pattern is consistent.
No immediate performance or safety concerns.
36-38: Returning a cloned token map.
As before, returning a reference might reduce overhead if mutability isn’t needed.
39-41: Cloning tokens on retrieval.
Once again, consider if you can return a reference for performance gains.crates/settings/src/yaml/mod.rs (5)
1-1: New submodule addition acknowledged.
Thecachemodule is clearly separated, aiding modular design.
7-9: Import changes look consistent.
These newly imported items appear relevant for network and token parsing.
13-14: DotrainYaml and OrderbookYaml usage.
These imports clarify the bridging between YAML source structs.
29-30: New methods in YamlParsable trait.
Extending the trait to parse fromOrderbookYamlandDotrainYamlis a clean approach for multi-source parsing.
90-124: ContextProvider trait adds structured context management.
These methods systematically expand context with remote data and handle optional fields. The design is flexible and consistent.crates/settings/src/remote_tokens.rs (13)
1-11: Imports for remote token logic.
No issues apparent; modules and crates are referenced properly.
12-15: WASM bridging references.
These look fine for cross-compilation scenarios.
16-23: Skippingdocumentfrom serialization remains a solid choice.
As previously noted, consider whether the URL should also be masked or skipped if it can contain sensitive data.
24-26: WASM trait generation.
Straightforward bridging for WASM usage.
32-42: Fetching tokens without timeout or retries.
Consider adding client settings for timeouts/retries to avoid indefinite waits.
43-54: Conflicting token check is thorough.
This block nicely prevents accidental key collisions.
56-57: Returning aggregated tokens.
Overall, a clear, short final step.
60-66: Unimplementedparse_from_yamlwith fallback error.
Consider adding documentation on why it’s not supported.
68-103: Optional remote tokens parsing.
Logic for discoveringusing-tokens-fromacross multiple documents is well-structured.
106-113: Default uses non-HTTPS URL.
As discussed before, adopting “https://example.com” can enhance security.
114-118: PartialEq ignoringdocumentfield.
Confirm if it’s intentional that onlyurlis compared, as previously noted.
120-130: ParseRemoteTokensError looks well-defined.
Covers conflicts, parse errors, and other relevant scenarios.
132-178: Tests thoroughly validate edge cases.
Both single and multiple-file scenarios are covered adequately.crates/settings/src/yaml/context.rs (7)
1-2: Imports are valid and consistent.
These newly introduced imports align with the references in this file, and no syntax or logical issues are apparent.
22-22: Potential overhead of storing large caches.
This line introduces an optionalyaml_cache. As previously noted, be mindful of the potential memory overhead when dealing with large datasets.
153-163: Trait definition is clear.
All trait methods are well-defined and use idiomatic return types for optional data.
201-201: Default initialization ofyaml_cacheis fine.
This is consistent with the pattern used for other optional fields in the struct.
211-211: Cloning existingyaml_cache.
This approach copies the entire cache for the new context, making logical sense if the intention is to keep them in sync.
242-255: Remote data injection overwrites prior networks.
As noted in previous reviews, consider whether merges should preserve or override existing data. Currently, existing entries are replaced completely.
257-267: Remote data injection overwrites prior tokens.
Similarly, this design choice entirely replaces existing remote tokens. If partial merges are required, additional logic may be needed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/settings/src/remote/tokens.rs (1)
41-69: 🧹 Nitpick (assertive)Remove unnecessary string clone
On line 58, using.clone()after.replace()is redundant because.replace()already returns a newString.- key: self.name.to_lowercase().replace(' ', "-").clone(), + key: self.name.to_lowercase().replace(' ', "-"),crates/settings/src/yaml/context.rs (1)
238-263: 🧹 Nitpick (assertive)Consider merging instead of overwriting
Theset_remote_networksandset_remote_tokensmethods overwrite all cached entries. If partial merges are desired, you can merge new entries while retaining existing ones:- yaml_cache.remote_networks = remote_networks; + yaml_cache.remote_networks.extend(remote_networks);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
crates/settings/src/order.rs(1 hunks)crates/settings/src/remote/tokens.rs(1 hunks)crates/settings/src/yaml/context.rs(5 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
crates/settings/src/order.rs (4)
crates/settings/src/token.rs (1)
parse_all_from_yaml(234-324)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/yaml/mod.rs (2)
parse_all_from_yaml(49-52)parse_all_from_yaml(67-67)
crates/settings/src/remote/tokens.rs (3)
crates/settings/src/yaml/context.rs (1)
new(192-199)crates/settings/src/yaml/orderbook.rs (1)
new(30-58)crates/common/src/dotrain_order/mod.rs (1)
new(150-187)
crates/settings/src/yaml/context.rs (1)
crates/settings/src/yaml/cache.rs (5)
get_remote_networks(23-25)get_remote_network(26-28)get_remote_tokens(36-38)get_remote_token(39-41)new(10-15)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: git-clean
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: test
- GitHub Check: Deploy-Preview
🔇 Additional comments (5)
crates/settings/src/order.rs (1)
481-484: Clean usage of context-based parsing
The updated code callsparse_all_from_yamlwith acontext, enabling more flexible, context-aware configuration retrieval.crates/settings/src/remote/tokens.rs (2)
1-39: Data structures look solid
TheVersion,Token, andTokensstructs are well-defined for managing remote token data.
71-79: Good error enumeration
TheRemoteTokensErrorenum comprehensively covers fundamental error conditions for remote tokens.crates/settings/src/yaml/context.rs (2)
1-2: Additions to support remote caching
The new imports,YamlCachestruct,yaml_cachefield inContext, and default initialization approach strengthen the ability to store and manage remote resources.Also applies to: 11-15, 22-22, 197-197, 207-207
153-189: Useful trait-based access to cached data
DefiningYamlCacheTraitand implementing it onContextclarifies how remote networks and tokens are retrieved, promoting a clean, modular design.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
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/remote_tokens.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 (3)
crates/settings/src/orderbook.rs (6)
crates/settings/src/yaml/mod.rs (2)
parse_all_from_yaml(49-52)parse_all_from_yaml(67-67)crates/settings/src/order.rs (1)
parse_all_from_yaml(475-773)crates/settings/src/network.rs (1)
parse_all_from_yaml(143-235)crates/settings/src/token.rs (1)
parse_all_from_yaml(234-324)crates/settings/src/deployer.rs (1)
parse_all_from_yaml(139-207)crates/settings/src/subgraph.rs (1)
parse_all_from_yaml(91-142)
crates/settings/src/remote_networks.rs (2)
crates/settings/src/remote_tokens.rs (4)
validate_url(28-30)default(107-112)eq(115-117)reqwest(38-40)crates/settings/src/yaml/cache.rs (1)
new(10-15)
crates/settings/src/remote_tokens.rs (1)
crates/settings/src/remote_networks.rs (4)
validate_url(36-38)default(135-142)eq(145-147)reqwest(48-50)
🔇 Additional comments (39)
crates/settings/src/remote_networks.rs (2)
58-64: Graceful handling of already-inserted networks.The code immediately returns an error upon detecting a
ConflictingNetworksscenario. If you want partial success—where non-conflicting networks are inserted—consider storing those valid entries and only rejecting the conflicting subset. Otherwise, this strict approach is correct if total consistency is required.
78-132: Review test coverage ofparse_all_from_yaml.Unit tests already show thorough checking of missing keys, invalid URLs, and key shadowing. Consider also validating real-world scenarios (e.g., multiple valid remote networks in the same YAML, or toggling different formats) to ensure robust coverage.
crates/settings/src/orderbook.rs (1)
75-75: Passingcontextis a solid improvement.Leveraging
contextwhen invokingNetworkCfg::parse_all_from_yamlandSubgraphCfg::parse_all_from_yamlenables remote or cached data usage without further refactoring. This is consistent with other refactors across the codebase.Also applies to: 79-80
crates/settings/src/network.rs (2)
145-146: Context usage enhances flexibility.Switching from a placeholder parameter to
context: Option<&Context>aligns the implementation with other modules, allowing remote network data to be merged if provided.
214-225: Local & remote networks merge logic looks correct.Appending remote networks from
context.yaml_cachewith a key-collision check is well-handled via theRemoteNetworkKeyShadowingerror variant. This ensures local definitions cannot be overridden inadvertently.crates/settings/src/yaml/orderbook.rs (22)
1-5: No concerns on updatedusestatements.
These additions are consistent with the newcacheusage and remote networks/tokens logic.
25-25: Consider documenting concurrency usage or refresh mechanism forpub cache.
Past reviews suggested adding doc comments for this public field.
47-47: Good addition to validate remote networks.
Propagating any parse error early is beneficial for robust error handling.
55-58: Returning a newCacheeach time
This might isolate eachOrderbookYamlinstance. If you need shared caching, consider an alternative design.
62-66: Initialization of a newCache
Same comment as above about shared vs local caches forOrderbookYaml.
68-73: Preservingcacheduring conversion
No concerns. This ensures continuity of caching data across transformations.
75-80: ReusingcachefromDotrainYaml
Looks good for consistent remote data usage.
83-90: Methods returning cached remote data
No immediate issues. These getters provide straightforward access to remote networks/tokens.
95-99: Expanding context with remote networks
The approach is consistent with your existing design. No issues.
100-105:get_networkswith updated context
No concerns, logic is clear.
107-112:get_networkwith expanded context
Implementation is consistent.
113-116:get_remote_networks
Allows retrieving rawRemoteNetworksCfg. This can be helpful for advanced usage or debugging.
119-123:get_token_keyswith expanded context
Ensures tokens from remote sources are included.
127-132:get_tokenwith expanded context
No issues identified.
134-140: Fetching optional remote tokens
Your approach of returningOption<RemoteTokensCfg>is a sensible design for optional data.
151-153: Expanding context with remote networks before parsing orderbooks
This approach is consistent with the pattern above.Also applies to: 155-155
159-162:get_orderbookwith remote expansion
All good, consistent usage.
185-189:get_deployer_keyswith expanded context
Same pattern, no issues found.
193-196:get_deployerwith expanded context
No concerns.
273-273: New test data fornetworks:
Expanding the test fixture with remote networks. All good.
280-281:using-networks-fromtest fixture
Ensures test coverage for external network references.
354-359: Test forget_remote_networks
Validates that the YAML includes remote network references and checks URL correctness. This is a strong addition to ensure correctness.crates/settings/src/remote_tokens.rs (12)
1-15: Intro imports and feature gating
No issues identified. Good modular approach for remote token logic.
16-23: Definition ofRemoteTokensCfg
Skippingdocumentduring serialization helps protect internal YAML data. Fields are well defined.
24-25: Wasm binding
No concerns, consistent with approach in other modules.
27-31:validate_urlfunction
Straightforward approach usingUrl::parse.
32-57: Asynchronous token fetching
Consider a configurable client with timeouts and retries to handle network issues.
68-104:parse_from_yaml_optional
Logic properly handles multiple definitions ofusing-tokens-from. Good job returning an error on shadowing.
106-113: Default url usinghttp://example.com
As previously suggested, consider using HTTPS for better security.
114-118: Ignoringdocumentfield inPartialEq
If this is intentional, add a comment explaining it.
120-130: Error enum for remote token parsing
Clear error variants for conflicts, parse failures, and network issues. Nicely done.
132-159: Unit test forparse_from_yaml_optional
Checks invalid relative URL usage scenario. Good coverage for error handling.
161-181: Test for multiple definitions
Properly verifies the shadowing conflict scenario.
183-285: Integration test forfetch_tokens
Covers a typical usage with mock server. Good approach to ensure correctness of remote fetching.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
crates/settings/src/yaml/orderbook.rs (4)
25-25: Publiccachefield lacks documentation.The new
cachefield is public but lacks documentation explaining how it's intended to be used, populated, or refreshed.
62-66: Fresh cache infrom_documents.Creating a new default cache for each instance isolates cache entries, which may limit sharing cache data between instances.
115-118: Direct retrieval of all remote networks.The method allows for retrieving all remote network configurations at once.
242-245: Deserialization provides a default cache.The deserialization initializes the cache to its default value, consistent with other constructors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/settings/src/yaml/orderbook.rs(8 hunks)
🔇 Additional comments (5)
crates/settings/src/yaml/orderbook.rs (5)
47-47: Remote networks validation added.Good addition of validation for remote networks when the
validateflag is set to true.
55-58: Default initialization of the cache.The
Cache::default()initialization is consistent with other initialization patterns in the codebase.
68-80: Added methods for creating from existing instances.These methods correctly maintain the cache when creating from existing instances.
83-91: ContextProvider implementation.Clean implementation of the ContextProvider trait that delegates to the cache's methods.
94-99: New context initialization method.Good refactoring to avoid code duplication in multiple methods. This streamlines context creation and expansion with remote data.
2 participants
Caution
Do not merge before #1477
Motivation
See issue: #1397
Solution
This format uses the links from https://tokenlists.org/
Checks
By submitting this for review, I'm confirming I've done the following:
fix #1397
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
DotrainYamlandOrderbookYamlstructs to include caching mechanisms for improved data management.Tests