refactor: scaffold multi-chain pipeline #910
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces multi-chain abstractions: new framework modules (cardano, ethereum, bitcoin, substrate), a Chain enum, and a chain-wrapped Record enum. Refactors Cardano paths across filters, sinks, and sources to use Record::Cardano and cardano::ChainConfig. Adds an optional Ethereum source (feature "eth") using alloy. Updates CLI args and daemon config to new chain types. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Pipeline
participant Sources as Sources::Config
participant Eth as sources::eth::Worker
participant WS as Ethereum WS Provider (alloy)
participant StageOut as Stage Output
User->>Pipeline: Run with feature "eth" and URL
Pipeline->>Sources: Config::bootstrapper(Ethereum{ url })
Sources-->>Pipeline: Bootstrapper::Ethereum(Stage)
Note right of Pipeline: spawn()
Pipeline->>Eth: start(Stage, Worker)
rect rgba(230,240,255,0.5)
Eth->>WS: connect_ws(url)
Eth->>WS: subscribe_blocks()
WS-->>Eth: stream of block headers
end
loop schedule
Eth->>Eth: poll next header
alt header available
Eth-->>Pipeline: Work::Unit{ hash }
Pipeline->>Eth: Execute(Unit{ hash })
Eth->>WS: get_block_by_hash(hash)
WS-->>Eth: Block or None
alt Block found
Eth->>StageOut: ChainEvent::Apply(point, Record::Ethereum(ParsedBlock))
else No block
Eth->>StageOut: log warn / no event
end
else idle
Eth-->>Pipeline: Work::Idle
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/sources/eth.rs (1)
56-57: Track multi-chain Point support.The TODO comment indicates that multi-chain Point support is needed. Currently,
pallas::network::miniprotocols::Point::Originis used as a placeholder.Do you want me to open a new issue to track implementing proper multi-chain Point support for Ethereum?
src/framework/substrate/mod.rs (1)
1-15: Use a Substrate-specific block representation
alloy::rpc::types::Blockis an Ethereum block; wiring it into the Substrate record means we can’t ever emit an actual Substrate block without serializing it into Ethereum’s shape. Please swap this for a Substrate-native type (e.g. fromsubxt/sp_runtime) or keep the placeholder as a neutral struct until that integration lands.src/sinks/terminal/format.rs (1)
100-101: Silence unused bindings on todo!() arms.Both the Bitcoin and Substrate matches bind
recordbut never use it, which triggers compiler warnings. Rename them to_record(or just_) so the build stays warning-free.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
Cargo.toml(2 hunks)src/bin/oura/dump.rs(4 hunks)src/bin/oura/watch.rs(4 hunks)src/daemon/mod.rs(1 hunks)src/filters/into_json.rs(1 hunks)src/filters/legacy_v1/cip15.rs(1 hunks)src/filters/legacy_v1/cip25.rs(1 hunks)src/filters/legacy_v1/crawl.rs(1 hunks)src/filters/legacy_v1/map.rs(1 hunks)src/filters/legacy_v1/mod.rs(2 hunks)src/filters/legacy_v1/prelude.rs(2 hunks)src/filters/parse_cbor.rs(1 hunks)src/filters/select/eval/mod.rs(1 hunks)src/filters/split_block.rs(1 hunks)src/filters/wasm_plugin.rs(3 hunks)src/framework/bitcoin/mod.rs(1 hunks)src/framework/cardano/mod.rs(1 hunks)src/framework/ethereum/mod.rs(1 hunks)src/framework/mod.rs(2 hunks)src/framework/substrate/mod.rs(1 hunks)src/sinks/assert/checks.rs(1 hunks)src/sinks/assert/mod.rs(2 hunks)src/sinks/assert/prelude.rs(1 hunks)src/sinks/aws_s3.rs(1 hunks)src/sinks/terminal/format.rs(4 hunks)src/sources/eth.rs(1 hunks)src/sources/hydra.rs(2 hunks)src/sources/mithril.rs(1 hunks)src/sources/mod.rs(6 hunks)src/sources/n2c.rs(2 hunks)src/sources/n2n.rs(2 hunks)src/sources/s3.rs(1 hunks)src/sources/u5c.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Check (ubuntu-latest, stable)
src/sinks/terminal/format.rs
[warning] 101-101:
unused variable: record
[warning] 100-100:
unused variable: record
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Suite
- GitHub Check: Check (windows-latest, stable)
🔇 Additional comments (18)
src/sinks/assert/prelude.rs (1)
1-1: LGTM! Import path correctly updated for multi-chain structure.The import path change aligns with the broader refactoring to organize Cardano-specific types under the
cardanonamespace.src/sinks/aws_s3.rs (1)
43-47: LGTM! Cardano record wrapping correctly implemented.The pattern matching correctly wraps the CBOR block in the Cardano-specific record variant, consistent with the multi-chain refactoring across the codebase.
src/sources/u5c.rs (1)
34-38: LGTM! Cardano record wrapping correctly applied to both code paths.Both the
ParsedBlockandCborBlockvariants are correctly wrapped in the Cardano-specific record type, maintaining consistency with the multi-chain architecture.src/filters/legacy_v1/crawl.rs (1)
6-6: LGTM! Import path correctly updated.The import path change from
framework::legacy_v1toframework::cardano::legacy_v1aligns with the namespace reorganization for multi-chain support.src/sources/s3.rs (1)
112-115: LGTM! Cardano record wrapping correctly implemented.The
ChainEvent::Applyconstruction correctly wraps the CBOR block in the Cardano-specific record variant, consistent with the multi-chain refactoring.src/sources/mithril.rs (1)
318-319: LGTM! Cardano record wrapping correctly implemented.The
ChainEvent::Applyconstruction correctly wraps the CBOR block in the Cardano-specific record variant, maintaining consistency with the multi-chain architecture.src/filters/into_json.rs (1)
29-32: LGTM! Multi-chain JSON conversion correctly implemented.The pattern matching correctly converts Cardano records to JSON while preserving other record types unchanged. This enables multi-chain support in the JSON filter.
src/sinks/assert/mod.rs (2)
5-5: LGTM!The import path correctly reflects the new Cardano-specific module structure for legacy_v1 events.
93-93: LGTM!The pattern match correctly handles the new Cardano-wrapped record structure, maintaining the same logic while adapting to the multi-chain abstraction.
src/filters/legacy_v1/prelude.rs (2)
1-1: LGTM!The import path correctly uses the Cardano-specific legacy_v1 module, aligning with the multi-chain framework structure.
45-48: LGTM!The event construction correctly wraps the OuraV1Event in the Cardano-specific record wrapper, maintaining consistency with the multi-chain abstraction.
src/filters/legacy_v1/mod.rs (1)
42-42: LGTM!The pattern match correctly handles the Cardano-wrapped CborBlock variant, consistent with the multi-chain refactoring.
src/sources/n2n.rs (1)
122-122: LGTM!The record construction correctly wraps the CborBlock in the Cardano-specific variant, maintaining consistency with the multi-chain framework.
src/sources/n2c.rs (1)
106-107: LGTM!The ChainEvent construction properly wraps the CborBlock in the Cardano-specific record variant, consistent with the multi-chain refactoring.
src/filters/parse_cbor.rs (2)
43-46: LGTM!The pattern matching and output construction correctly handle Cardano-wrapped CborBlock records, transforming them into ParsedBlock records while maintaining the Cardano wrapper.
48-51: LGTM!The CborTx handling follows the same pattern as CborBlock, correctly wrapping both input and output in the Cardano-specific record variant.
src/daemon/mod.rs (1)
19-19: Chain::default aligns with expected behavior
Default returnsChain::Cardano(Default::default()), matching the sole variant’s default config.src/framework/ethereum/mod.rs (1)
1-17: Alloy Block JSON serialization verified: Thealloy::rpc::types::Blockstruct in v1.0.30 derivesSerializewith the correct serde attributes (e.g.,rename_all="camelCase",flatten), sojson!(x)will include all expected fields.
| # TODO(p): add feature | ||
| alloy = { version = "1.0.30", features = ["full"] } |
There was a problem hiding this comment.
Make alloy dependency optional and feature-gated.
The alloy dependency is currently unconditional despite the TODO comment suggesting it should be feature-gated. This adds unnecessary dependencies and increases build times for users not using Ethereum features.
Apply this diff to make alloy optional and gated by the eth feature:
-# TODO(p): add feature
-alloy = { version = "1.0.30", features = ["full"] }
+alloy = { version = "1.0.30", features = ["full"], optional = true }Then update the eth feature definition at line 23 to include alloy:
-eth = ["futures-util"]
+eth = ["futures-util", "alloy"]📝 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.
| # TODO(p): add feature | |
| alloy = { version = "1.0.30", features = ["full"] } | |
| # In the [features] section (around line 23): | |
| eth = ["futures-util", "alloy"] | |
| # In the [dependencies] section (around lines 87–88): | |
| alloy = { version = "1.0.30", features = ["full"], optional = true } |
🤖 Prompt for AI Agents
In Cargo.toml around lines 87-88, the alloy dependency is unconditional; make it
optional and feature-gated by adding optional = true to the alloy dependency
entry and keeping its features = ["full"], then update the [features] section at
line 23 to include alloy by adding "alloy" to the eth feature list (i.e., make
eth = ["existing-items...", "alloy"]), so the crate is only pulled in when the
eth feature is enabled.
| let chain_config = match &ctx.chain { | ||
| Chain::Cardano(chain_config) => chain_config.clone(), | ||
| }; |
There was a problem hiding this comment.
Non-exhaustive pattern matching will panic on non-Cardano chains.
The pattern match on ctx.chain only handles the Chain::Cardano variant. According to the PR summary, the Chain enum includes Ethereum, Bitcoin, and Substrate variants. This will panic at runtime if a non-Cardano chain is configured.
Add proper error handling for non-Cardano chains:
- let chain_config = match &ctx.chain {
- Chain::Cardano(chain_config) => chain_config.clone(),
- };
+ let chain_config = match &ctx.chain {
+ Chain::Cardano(chain_config) => chain_config.clone(),
+ _ => return Err(Error::config("legacy_v1 filter only supports Cardano chain")),
+ };📝 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.
| let chain_config = match &ctx.chain { | |
| Chain::Cardano(chain_config) => chain_config.clone(), | |
| }; | |
| let chain_config = match &ctx.chain { | |
| Chain::Cardano(chain_config) => chain_config.clone(), | |
| _ => return Err(Error::config("legacy_v1 filter only supports Cardano chain")), | |
| }; |
🤖 Prompt for AI Agents
In src/filters/legacy_v1/mod.rs around lines 92-94, the match on ctx.chain only
handles Chain::Cardano and will panic for other variants; change it to an
exhaustive match that either extracts the Cardano config or returns a proper
error for non-Cardano variants (include the actual variant in the message), e.g.
match on Chain::{Cardano => chain_config.clone(), other => return Err(/*
appropriate error type with descriptive message including other */)}; ensure the
function signature and error type are adjusted/propagated accordingly instead of
letting the code panic.
|
|
||
| let subscription = provider.subscribe_blocks().await.or_panic()?; | ||
|
|
||
| let stream = subscription.into_stream().take(2); |
There was a problem hiding this comment.
Critical: Remove the .take(2) limit on the block stream.
The stream is artificially limited to only 2 blocks, which will cause the source to stop processing after 2 blocks. This appears to be leftover test or debug code.
Apply this diff to remove the limit:
- let stream = subscription.into_stream().take(2);
+ let stream = subscription.into_stream();🤖 Prompt for AI Agents
In src/sources/eth.rs around line 37, the block stream is incorrectly limited by
.take(2) which causes processing to stop after two blocks; remove the .take(2)
call so the code uses subscription.into_stream() (i.e., let stream =
subscription.into_stream();) ensuring the stream is unbounded and continues to
yield blocks; update any downstream code/comments if they assumed a bounded
stream.
| } | ||
|
|
||
| #[derive(Stage)] | ||
| #[stage(name = "source-utxorpc", unit = "Header", worker = "Worker")] |
There was a problem hiding this comment.
Incorrect stage name.
The stage name is set to "source-utxorpc", but this is an Ethereum source, not a UTXORPC source. This appears to be a copy-paste error.
Apply this diff to correct the stage name:
-#[stage(name = "source-utxorpc", unit = "Header", worker = "Worker")]
+#[stage(name = "source-eth", unit = "Header", worker = "Worker")]📝 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.
| #[stage(name = "source-utxorpc", unit = "Header", worker = "Worker")] | |
| #[stage(name = "source-eth", unit = "Header", worker = "Worker")] |
🤖 Prompt for AI Agents
In src/sources/eth.rs around line 68, the stage attribute incorrectly uses name
= "source-utxorpc"; update the stage attribute to the correct identifier for
this Ethereum source (e.g., name = "source-eth" or the project's canonical
ethereum source name) so it reflects that this is an Ethereum source rather than
UTXORPC; keep the rest of the attribute (unit = "Header", worker = "Worker")
unchanged.
| let chain_config = match &ctx.chain { | ||
| Chain::Cardano(chain_config) => chain_config.clone(), | ||
| }; |
There was a problem hiding this comment.
Non-exhaustive pattern matching will panic on non-Cardano chains.
This pattern match only handles Chain::Cardano, which will cause a panic if a non-Cardano chain is configured.
Add proper error handling:
- let chain_config = match &ctx.chain {
- Chain::Cardano(chain_config) => chain_config.clone(),
- };
+ let chain_config = match &ctx.chain {
+ Chain::Cardano(chain_config) => chain_config.clone(),
+ _ => return Err(Error::config("n2c source only supports Cardano chain")),
+ };📝 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.
| let chain_config = match &ctx.chain { | |
| Chain::Cardano(chain_config) => chain_config.clone(), | |
| }; | |
| let chain_config = match &ctx.chain { | |
| Chain::Cardano(chain_config) => chain_config.clone(), | |
| _ => return Err(Error::config("n2c source only supports Cardano chain")), | |
| }; |
| let chain_config = match &ctx.chain { | ||
| Chain::Cardano(chain_config) => chain_config.clone(), | ||
| }; |
There was a problem hiding this comment.
Non-exhaustive pattern matching will panic on non-Cardano chains.
Similar to the legacy_v1 filter, this pattern match only handles Chain::Cardano. This will panic if a non-Cardano chain is configured.
Add proper error handling:
- let chain_config = match &ctx.chain {
- Chain::Cardano(chain_config) => chain_config.clone(),
- };
+ let chain_config = match &ctx.chain {
+ Chain::Cardano(chain_config) => chain_config.clone(),
+ _ => return Err(Error::config("n2n source only supports Cardano chain")),
+ };🤖 Prompt for AI Agents
In src/sources/n2n.rs around lines 228 to 230, the match on ctx.chain only
handles Chain::Cardano and will panic for other variants; change it to handle
all Chain variants explicitly (or use an if let with an else) and return or
propagate a proper error instead of unwrapping/panicking. Specifically, detect
non-Cardano chains and return a Result::Err (or appropriate error type) with a
clear message, or add matching arms for other Chain variants that provide
correct behavior, ensuring no panics from non-exhaustive pattern matching.
Summary
This PR provides the initial scaffolding for adding multi-chain support to Oura. In particular, it introduces:
In order to accomplish this, we had to refactor the following: