Conversation
Metaboard config now requires a contract address alongside the URL. Updated parsing from plain string to hash format, added set_record_to_yaml for upsert behavior, and added validation + tests for the new field.
…query Extract get_metaboard_cfg helper and use the address field directly from MetaboardCfg, removing the need to query the metaboard subgraph for the contract address at order submission time.
Add resolve_metaboard_address helper that first checks config, then falls back to querying the metaboard subgraph. Use set_metaboard (upsert) instead of add_metaboard when overriding via CLI flag.
Update all metaboard entries across test fixtures, inline YAML strings, and documentation from the old plain-string format to the new hash format with url and address fields.
WalkthroughAdds a required Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (words/chart)
participant SG as MetaboardSubgraph (sg)
participant Settings as Settings/YAML orderbook
CLI->>Settings: check metaboard entry for URL/address
alt address present
Settings-->>CLI: return address
else address missing
CLI->>SG: query metaboard by URL (async)
SG-->>CLI: respond with contract address
CLI->>Settings: set_metaboard(key, url, address)
Settings-->>CLI: ack
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
packages/orderbook/README.md (1)
50-65:⚠️ Potential issue | 🟡 MinorUpdate the example spec version to 5.
The README’s config snippet still shows
version: 4, which conflicts with the new spec version. This can mislead users following the guide.✏️ Proposed update
-version: 4 +version: 5crates/settings/ARCHITECTURE.md (2)
153-153:⚠️ Potential issue | 🟡 MinorStale spec version reference in documentation.
Line 153 still states
CURRENT_SPEC_VERSION = "3", but the PR bumps the version to"5". Line 288 also references"3". Both should be updated to match the new constant.📝 Proposed fix
At line 153:
-- `SpecVersion` reads required root scalar `version`. Const `CURRENT_SPEC_VERSION = "3"` and helpers `current()` / `is_current()`. +- `SpecVersion` reads required root scalar `version`. Const `CURRENT_SPEC_VERSION = "5"` and helpers `current()` / `is_current()`.At line 288:
-- `spec_version.rs`: required root scalar `version` and helpers to compare to the current spec version (constant "3"). +- `spec_version.rs`: required root scalar `version` and helpers to compare to the current spec version (constant "5").
60-60:⚠️ Potential issue | 🟡 Minor
add_metaboardsignature andset_metaboardnot documented.Line 60 lists
add_metaboard(key, url)but the PR adds anaddressparameter and introducesset_metaboard. This should be updated to reflect the current API.packages/ui-components/src/lib/__fixtures__/settings.yaml (1)
1-1:⚠️ Potential issue | 🟡 MinorUpdate or remove unused fixture
settings.yaml— it hasversion: 2but current spec is5.The fixture is never parsed in any tests or code (tests mock settings as string literals); however, the fixture is outdated and should either be updated to match the current spec version or removed if no longer needed.
crates/js_api/src/gui/order_operations.rs (2)
825-835: 🧹 Nitpick | 🔵 Trivial
get_metaboard_cfg()is called redundantly whenshould_emit_meta_call()returnstrue.
should_emit_meta_call()→get_metaboard_client()→get_metaboard_cfg()already resolves the config. Then Line 826 callsget_metaboard_cfg()again. Consider havingshould_emit_meta_call(or a combined helper) return theMetaboardCfgso the caller can reuse it, avoiding the duplicate deployment lookup + YAML parse.
1064-1069: 🧹 Nitpick | 🔵 TrivialVerify that
replacestill targets only the URL in the new multi-line metaboard format.
get_yaml().replace("https://metaboard.com", url)works because the literalhttps://metaboard.comstill appears as-is in the YAML. This is fine now, but is brittle if the fixture ever adds another occurrence or the format changes. Consider adding a brief comment noting the coupling.crates/js_api/src/gui/mod.rs (1)
704-705:⚠️ Potential issue | 🔴 CriticalRemove the unused
NoAddressInMetaboardSubgrapherror variant.This error is never constructed anywhere in the codebase. The metaboard address is now sourced from config (
MetaboardCfg.address), making this error unreachable. While it has a match arm into_readable_msg, the variant itself is dead code and should be removed to keep the error type clean.
🤖 Fix all issues with AI agents
In `@crates/cli/src/commands/words.rs`:
- Around line 82-98: The function resolve_metaboard_address currently returns
the address from dotrain_order.orderbook_yaml().get_metaboard(network_key) even
when a different metaboard_url is passed in, effectively ignoring the
metaboard_url override; add a brief comment above the early-return branch
(referencing resolve_metaboard_address and the get_metaboard call) that
documents this is intentional because the on-chain contract address is
authoritative and metaboard_url is only used when no config entry exists, so
future readers understand why the CLI URL override isn’t used in that case.
| async fn resolve_metaboard_address( | ||
| dotrain_order: &DotrainOrder, | ||
| network_key: &str, | ||
| metaboard_url: &str, | ||
| ) -> Result<String> { | ||
| if let Ok(metaboard) = dotrain_order.orderbook_yaml().get_metaboard(network_key) { | ||
| return Ok(metaboard.address.to_string()); | ||
| } | ||
|
|
||
| let client = MetaboardSubgraphClient::new(Url::parse(metaboard_url)?); | ||
| let addresses = client.get_metaboard_addresses(None, None).await?; | ||
| let address = addresses.first().ok_or(anyhow!( | ||
| "could not resolve metaboard contract address from the provided metaboard subgraph url" | ||
| ))?; | ||
|
|
||
| Ok(address.to_string()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
resolve_metaboard_address returns the existing config address even when the CLI overrides the URL.
When a metaboard already exists in config for network_key, this function returns its address regardless of whether metaboard_url differs from the config's URL. This is likely intentional (the on-chain contract address doesn't change), but it means the metaboard_url parameter is ignored in that branch. Worth a brief comment documenting this intent.
🤖 Prompt for AI Agents
In `@crates/cli/src/commands/words.rs` around lines 82 - 98, The function
resolve_metaboard_address currently returns the address from
dotrain_order.orderbook_yaml().get_metaboard(network_key) even when a different
metaboard_url is passed in, effectively ignoring the metaboard_url override; add
a brief comment above the early-return branch (referencing
resolve_metaboard_address and the get_metaboard call) that documents this is
intentional because the on-chain contract address is authoritative and
metaboard_url is only used when no config entry exists, so future readers
understand why the CLI URL override isn’t used in that case.
There was a problem hiding this comment.
cli will be removed soon so no need to do any changes in here
There was a problem hiding this comment.
@findolor, understood! No point in documenting code that's on its way out. Thanks for the context.
(\_/)
(o.o)
( > <) noted for future
🧠 Learnings used
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2163
File: crates/common/src/raindex_client/orders.rs:738-741
Timestamp: 2025-10-02T19:17:20.332Z
Learning: In crates/common/src/raindex_client/orders.rs, fetch_dotrain_source() is intentionally called in try_from_sg_order for every order conversion because the dotrain source information is needed immediately. A future optimization with local DB logic is planned to eliminate the network round-trip concern.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1938
File: crates/settings/src/yaml/orderbook.rs:180-199
Timestamp: 2025-06-18T18:24:32.049Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers to avoid refactoring duplicate search logic between get_orderbook_by_address and get_orderbook_by_network_key when there are only 2 functions, indicating they would consider it if more similar functions are added in the future.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1903
File: crates/settings/src/yaml/orderbook.rs:371-377
Timestamp: 2025-06-17T16:21:24.384Z
Learning: In crates/settings/src/yaml/orderbook.rs tests, the user findolor considers RPC ordering in Vec<Url> assertions to be intentional and not a test brittleness issue. The ordering of RPCs in tests should be preserved as specified.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cli/src/commands/words.rs (1)
656-680:⚠️ Potential issue | 🟡 MinorUnhappy test is missing the
versionfield — it may fail at config parse time rather than at contract call time.All happy-path tests include
version: {spec_version}, but this unhappy test omits it. If the spec-v5 parser now requires a version field, the test still passes (is_err()), but for the wrong reason (config parse error instead of the intended contract-interface error). Consider adding the version field so the test validates the expected failure path.Suggested fix
let dotrain_content = format!( " +version: {spec_version} networks: some-network: rpcs: - {} chain-id: 123 network-id: 123 currency: ETH metaboards: some-network: url: {} address: 0x59401c9302e79eb8ac6aea659b8b3ae475715e86 deployers: some-deployer: network: some-network address: 0xF14E09601A47552De6aBd3A0B165607FaFd2B5Ba --- `#binding` :;", server.url("/rpc"), - server.url("/sg") + server.url("/sg"), + spec_version = SpecVersion::current() );
🤖 Fix all issues with AI agents
In `@crates/cli/src/commands/words.rs`:
- Around line 82-98: In resolve_metaboard_address, avoid silently picking
addresses.first() from MetaboardSubgraphClient::get_metaboard_addresses; add a
defensive check after fetching addresses: if addresses.is_empty() return the
existing error, and if addresses.len() > 1 return an Err indicating multiple
metaboard addresses were returned (include the metaboard_url and network_key in
the message) so misconfiguration is surfaced early; alternatively, document why
selecting the first element is safe with a clear comment above the
addresses.first() use if you intentionally allow it.
| async fn resolve_metaboard_address( | ||
| dotrain_order: &DotrainOrder, | ||
| network_key: &str, | ||
| metaboard_url: &str, | ||
| ) -> Result<String> { | ||
| if let Ok(metaboard) = dotrain_order.orderbook_yaml().get_metaboard(network_key) { | ||
| return Ok(metaboard.address.to_string()); | ||
| } | ||
|
|
||
| let client = MetaboardSubgraphClient::new(Url::parse(metaboard_url)?); | ||
| let addresses = client.get_metaboard_addresses(None, None).await?; | ||
| let address = addresses.first().ok_or(anyhow!( | ||
| "could not resolve metaboard contract address from the provided metaboard subgraph url" | ||
| ))?; | ||
|
|
||
| Ok(address.to_string()) | ||
| } |
There was a problem hiding this comment.
addresses.first() silently picks an arbitrary metaboard when the subgraph returns multiple.
If the subgraph indexes more than one metaboard contract, .first() returns whichever the subgraph orders first, with no deterministic guarantee. This is likely fine today (one metaboard per subgraph), but worth a brief comment or a guard that errors when addresses.len() > 1 to surface misconfiguration early.
Suggested defensive guard
let client = MetaboardSubgraphClient::new(Url::parse(metaboard_url)?);
let addresses = client.get_metaboard_addresses(None, None).await?;
+ if addresses.len() > 1 {
+ return Err(anyhow!(
+ "expected a single metaboard contract address from the subgraph, got {}",
+ addresses.len()
+ ));
+ }
let address = addresses.first().ok_or(anyhow!(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn resolve_metaboard_address( | |
| dotrain_order: &DotrainOrder, | |
| network_key: &str, | |
| metaboard_url: &str, | |
| ) -> Result<String> { | |
| if let Ok(metaboard) = dotrain_order.orderbook_yaml().get_metaboard(network_key) { | |
| return Ok(metaboard.address.to_string()); | |
| } | |
| let client = MetaboardSubgraphClient::new(Url::parse(metaboard_url)?); | |
| let addresses = client.get_metaboard_addresses(None, None).await?; | |
| let address = addresses.first().ok_or(anyhow!( | |
| "could not resolve metaboard contract address from the provided metaboard subgraph url" | |
| ))?; | |
| Ok(address.to_string()) | |
| } | |
| async fn resolve_metaboard_address( | |
| dotrain_order: &DotrainOrder, | |
| network_key: &str, | |
| metaboard_url: &str, | |
| ) -> Result<String> { | |
| if let Ok(metaboard) = dotrain_order.orderbook_yaml().get_metaboard(network_key) { | |
| return Ok(metaboard.address.to_string()); | |
| } | |
| let client = MetaboardSubgraphClient::new(Url::parse(metaboard_url)?); | |
| let addresses = client.get_metaboard_addresses(None, None).await?; | |
| if addresses.len() > 1 { | |
| return Err(anyhow!( | |
| "expected a single metaboard contract address from the subgraph, got {}", | |
| addresses.len() | |
| )); | |
| } | |
| let address = addresses.first().ok_or(anyhow!( | |
| "could not resolve metaboard contract address from the provided metaboard subgraph url" | |
| ))?; | |
| Ok(address.to_string()) | |
| } |
🤖 Prompt for AI Agents
In `@crates/cli/src/commands/words.rs` around lines 82 - 98, In
resolve_metaboard_address, avoid silently picking addresses.first() from
MetaboardSubgraphClient::get_metaboard_addresses; add a defensive check after
fetching addresses: if addresses.is_empty() return the existing error, and if
addresses.len() > 1 return an Err indicating multiple metaboard addresses were
returned (include the metaboard_url and network_key in the message) so
misconfiguration is surfaced early; alternatively, document why selecting the
first element is safe with a clear comment above the addresses.first() use if
you intentionally allow it.
1 participant
Dependent PRs
Motivation
See issues:
The metaboard config previously only stored a subgraph URL. The sync pipeline and GUI need to know the on-chain metaboard contract address to index events and emit meta calls. Previously, the GUI had to make an extra subgraph query at runtime to resolve the metaboard address, which was fragile and unnecessary when the address can be provided upfront in configuration.
Solution
addressfield (Addresstype) toMetaboardCfgin the settings crate. The YAML format changes frommetaboards: { key: url }tometaboards: { key: { url, address } }.set_record_to_yamlfor upsert semantics alongside the existingadd_record_to_yaml.order_operations.rsto read the metaboard address directly from config (get_metaboard_cfg) instead of querying the subgraph at runtime.wordscommand to resolve the metaboard address — first from config, then falling back to a subgraph query — when a metaboard URL is passed via CLI flag.settings,common,js_api,orderbook,ui-components, andwebappto the new metaboard format.Checks
By submitting this for review, I'm confirming I've done the following:
fix #2444
Summary by CodeRabbit
New Features
Documentation
Chores
Tests