Open
Conversation
- Remove `RpcConfig` struct and `rpc_config` field from `Chain` in human config - Make `source_for` field optional in `Rpc` struct; when omitted, it defaults to "fallback" if HyperSync is available for the chain, or "sync" otherwise - Simplify `from_evm_network_config` to only handle `RpcSelection` variants - Update `has_rpc_sync_src` detection to account for optional `for` derivation - Regenerate evm.schema.json from updated Rust types - Migrate all test configs from rpc_config to rpc format - Update snapshot tests and file hash expectations https://claude.ai/code/session_015MisMioiSxUHJDazfN1FfR
Test configs that used rpc_config (always sync) now need explicit `for: sync` since the `for` field defaults to "fallback" on chains with HyperSync (e.g. chain 1). Only the config2.yaml snapshot changes: the fallback URL is now a separate Rpc object without sync config fields (defaults applied in Config.res). https://claude.ai/code/session_015MisMioiSxUHJDazfN1FfR
…ions - codegen_templates.rs: None case for source_for should be unreachable after resolution, not silently default to fallback - config2.yaml: restore original behavior where both URLs in a list share the same for: sync and sync config - test_codegen/config.yaml: add explicit for: sync - Restore all snapshots to match original expectations https://claude.ai/code/session_015MisMioiSxUHJDazfN1FfR
The sync config fields (initial_block_interval, backoff_multiplicative, etc.) were already flattened into Rpc via serde(flatten). This removes the intermediate RpcSyncConfig struct and places the fields directly on Rpc, simplifying field access throughout the codebase. https://claude.ai/code/session_015MisMioiSxUHJDazfN1FfR
DZakh
changed the title
rpc_config in favor of rpc
Contributor
📝 WalkthroughWalkthroughThe PR refactors the RPC configuration model by consolidating RpcConfig structures into a streamlined Rpc struct and For enum, making the "for" field optional. Chain configurations replace rpc_config with rpc throughout Rust code and test YAML files, simplifying RPC source selection semantics. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@codegenerator/cli/test/configs/config2.yaml`:
- Around line 25-41: The second RPC entry's inline comment ("Fallback RPC URL")
contradicts its setting `for: sync`; either update the comment to reflect that
this RPC is used for "sync" or change the `for` value to `fallback` if it truly
should be a fallback; locate the second mapping under the `rpc:` list (the entry
with url: https://eth.com/fallback) and make the corresponding change so the
comment and the `for:` field are consistent.
🧹 Nitpick comments (2)
codegenerator/cli/src/hbs_templating/codegen_templates.rs (1)
1461-1475:unreachable!onNonearm is a valid invariant, but consider a safer alternative.The
Nonearm on Line 1465 panics at runtime ifsource_forwas somehow not resolved. While the resolution inDataSource::from_evm_network_configguaranteesSome(...)for all RPCs that reach this path, a defensiveanyhow::bail!or error return would be safer against future refactors that might introduce a new path bypassing resolution.That said, since the
InternalRpcConfig.source_foris&'static str(non-optional), there's no sensible fallback value, sounreachable!is a reasonable choice here given the current design.codegenerator/cli/src/config_parsing/system_config.rs (1)
563-577: Duplicated "default for" resolution logic — consider extracting a shared helper.The defaulting logic (
None→ sync when no hypersync, fallback otherwise) is implemented twice:
- Here in
has_rpc_sync_src(lines 566-570), usingis_syncclosure- In
from_evm_network_config(lines 1026-1033), usingdefault_for+resolve_forIf these diverge in the future, the detection and resolution would disagree, causing subtle bugs (e.g., field selection validation could permit/reject fields incorrectly). Consider extracting the "default for" derivation into a shared helper.
♻️ Sketch of shared helper
// Shared helper at module level or on a relevant type fn default_rpc_for(chain_id: u64, hypersync_config: &Option<HypersyncConfig>) -> For { let has_hypersync = hypersync_config.is_some() || hypersync_endpoints::get_default_hypersync_endpoint(chain_id).is_ok(); if has_hypersync { For::Fallback } else { For::Sync } }Then both call sites can use this single source of truth.
Comment on lines
+25
to
+41
| rpc: | ||
| - url: https://eth.com # RPC URL that will be used to subscribe to blockchain data on this network | ||
| for: sync | ||
| initial_block_interval: 10000 | ||
| backoff_multiplicative: 0.8 | ||
| acceleration_additive: 2000 | ||
| interval_ceiling: 10000 | ||
| backoff_millis: 5000 | ||
| query_timeout_millis: 20000 | ||
| - url: https://eth.com/fallback # Fallback RPC URL | ||
| for: sync | ||
| initial_block_interval: 10000 | ||
| backoff_multiplicative: 0.8 | ||
| acceleration_additive: 2000 | ||
| interval_ceiling: 10000 | ||
| backoff_millis: 5000 | ||
| query_timeout_millis: 20000 |
Contributor
There was a problem hiding this comment.
Stale comment contradicts for: sync on the second RPC entry.
Line 34's comment says "Fallback RPC URL" but for is set to sync on Line 35. This is misleading after the migration. Consider updating the comment to reflect the actual role, or if this entry is genuinely intended as a fallback, change for to fallback.
- - url: https://eth.com/fallback # Fallback RPC URL
- for: sync
+ - url: https://eth.com/fallback # Secondary sync RPC URL
+ for: sync📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| rpc: | |
| - url: https://eth.com # RPC URL that will be used to subscribe to blockchain data on this network | |
| for: sync | |
| initial_block_interval: 10000 | |
| backoff_multiplicative: 0.8 | |
| acceleration_additive: 2000 | |
| interval_ceiling: 10000 | |
| backoff_millis: 5000 | |
| query_timeout_millis: 20000 | |
| - url: https://eth.com/fallback # Fallback RPC URL | |
| for: sync | |
| initial_block_interval: 10000 | |
| backoff_multiplicative: 0.8 | |
| acceleration_additive: 2000 | |
| interval_ceiling: 10000 | |
| backoff_millis: 5000 | |
| query_timeout_millis: 20000 | |
| rpc: | |
| - url: https://eth.com # RPC URL that will be used to subscribe to blockchain data on this network | |
| for: sync | |
| initial_block_interval: 10000 | |
| backoff_multiplicative: 0.8 | |
| acceleration_additive: 2000 | |
| interval_ceiling: 10000 | |
| backoff_millis: 5000 | |
| query_timeout_millis: 20000 | |
| - url: https://eth.com/fallback # Secondary sync RPC URL | |
| for: sync | |
| initial_block_interval: 10000 | |
| backoff_multiplicative: 0.8 | |
| acceleration_additive: 2000 | |
| interval_ceiling: 10000 | |
| backoff_millis: 5000 | |
| query_timeout_millis: 20000 |
🤖 Prompt for AI Agents
In `@codegenerator/cli/test/configs/config2.yaml` around lines 25 - 41, The second
RPC entry's inline comment ("Fallback RPC URL") contradicts its setting `for:
sync`; either update the comment to reflect that this RPC is used for "sync" or
change the `for` value to `fallback` if it truly should be a fallback; locate
the second mapping under the `rpc:` list (the entry with url:
https://eth.com/fallback) and make the corresponding change so the comment and
the `for:` field are consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR consolidates the RPC configuration structure by removing the deprecated
rpc_configfield and merging its functionality into therpcfield. Theforfield is now optional with intelligent defaults based on HyperSync availability.Key Changes
rpc_configfield: The separaterpc_configstructure that supported multiple URLs has been deprecated in favor of the unifiedrpcfieldRpcstruct instead of nested undersync_configforfield optional: Thesource_forfield now defaults to:"fallback"when HyperSync is available for the chain"sync"otherwiserpcas the primary field andforas optionalrpc_configtorpcformatDataSource::from_evm_network_configmethod now handles the new structure with automatic default resolution for theforfieldImplementation Details
Rpcstruct now contains all sync configuration fields directly (no nestedsync_config)Rpcobject, or a list ofRpcobjectsforvalue is resolved during config parsing based on HyperSync availabilityhttps://claude.ai/code/session_015MisMioiSxUHJDazfN1FfR
Summary by CodeRabbit
rpcfield.rpc_configtorpcthroughout configuration files and internal structures.