feat: Integrate Midnight network support #175
Conversation
✅ Deploy Preview for openzeppelin-monitor ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
shahnami
added
T-feature
* refactor: Draft websockets and HTTP endpoint managers * test: Add integration tests for websocket logic * chore: Replace generic ws client with midnight specific * refactor: Use solely websocket for RPC requests (#253) * refactor: Use solely websocket for RPC requests * test: Fix tests and add helper methods * chore: Fix tests and ignore ones that rely on substrate * chore: Improve substrateclienttrait mocks * test: Add more coverage for transport * test: Add more test coverage for midnight client * docs: Add missing docstrings * test: Add coverage for monitor execution * test: Add coverage for filter_block * test: Fix remaining pool errors * chore: Remove outdated conditions and update docs
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if let Err(ConfigError::ValidationError(err)) = result { | ||
| assert!(err | ||
| .message | ||
| .contains("Protocol: must start with one of: wss://, ws://")); |
Check failure
Code scanning / Semgrep OSS
Semgrep Finding: javascript.lang.security.detect-insecure-websocket.detect-insecure-websocket
| if let Err(ConfigError::ValidationError(err)) = result { | ||
| assert!(err | ||
| .message | ||
| .contains("Protocol: must start with one of: http://, https://, wss://, ws://")); |
Check failure
Code scanning / Semgrep OSS
Semgrep Finding: javascript.lang.security.detect-insecure-websocket.detect-insecure-websocket
🚨 Bugbot Trial ExpiredYour team's Bugbot trial has expired. Please contact your team administrator to turn on the paid plan to continue using Bugbot. A team admin can activate the plan in the Cursor dashboard. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds comprehensive Midnight blockchain support alongside existing EVM and Stellar networks, including new data models for blocks, events, transactions, and monitors; WebSocket-based transport infrastructure; filter implementations; Midnight client implementations; and extensive test coverage with fixtures and mocks. Changes
Sequence Diagram(s)sequenceDiagram
participant Monitor
participant Pool as ClientPool
participant MC as MidnightClient
participant WS as WsTransport
participant EM as EndpointManager
participant Filter as MidnightFilter
participant Events as Event Handler
Monitor->>Pool: get_midnight_client(network)
Pool->>MC: new(network)
MC->>WS: new(network, config)
WS->>EM: new(config, active_url, fallbacks)
WS-->>MC: initialized
MC-->>Pool: MidnightClient ready
Pool-->>Monitor: Arc<MidnightClient>
Monitor->>MC: get_blocks(start, end)
MC->>WS: send_raw_request("chain_getBlockHash", ...)
alt Connection Healthy
WS-->>MC: block hashes
else Connection Failed
WS->>EM: rotate_url()
EM->>WS: try_connect(new_url)
WS-->>EM: success
WS-->>MC: block hashes (retry)
end
MC->>MC: deserialize blocks
MC-->>Monitor: Vec<BlockType>
Monitor->>Filter: filter_block(client, network, block, monitors)
Filter->>MC: get_events(start_block, end_block)
MC-->>Filter: Vec<MidnightEvent>
Filter->>Filter: match_functions(transactions)
Filter->>Filter: match_events(transactions)
Filter->>Filter: evaluate_expressions
Filter-->>Monitor: Vec<MonitorMatch>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: This is a substantial, multi-layered addition of Midnight blockchain support spanning data models, services, filters, clients, transports, and tests. While individual file changes often follow consistent patterns (new model types, test builders, fixtures), the breadth of affected systems—including new infrastructure (WebSocket transport, endpoint management, event decoding), complex filter logic, and async client implementations—demands careful review of logic density, type safety, error handling, and integration points. The heterogeneity is moderate but concentrated in critical services (clients, filters, transport) rather than scattered across unrelated areas. Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
src/services/blockchain/transports/http/endpoint_manager.rs (1)
246-364: Prevent infinite retry/rotation loops; cap attempts and surface context.On repeated 429/network errors across rotating URLs, the loop can toggle indefinitely. Add an attempt limit (e.g., active + fallbacks), then return a consolidated error.
Apply:
pub async fn send_raw_request< @@ - loop { + let mut attempts = 0usize; + // Try each URL at most once per call (active + all fallbacks) + let max_attempts = 1 + self.fallback_urls.read().await.len(); + loop { + if attempts >= max_attempts { + return Err(TransportError::network( + format!("Exhausted {} attempt(s) across active/fallback URLs", attempts), + None, + None, + )); + } let current_url_snapshot = self.active_url.read().await.clone(); @@ match attempt_result { // Handle successful response SingleRequestAttemptOutcome::Success(response) => { @@ } // Handle network error, try rotation SingleRequestAttemptOutcome::NetworkError(network_error) => { @@ - match self.try_rotate_url(transport).await { + attempts += 1; + match self.try_rotate_url(transport).await { @@ } // Non-retryable serialization error SingleRequestAttemptOutcome::SerializationError(serialization_error) => { return Err(serialization_error); } } }Optional: rotate to next index rather than first “!= active” to avoid bias; I can supply a small helper if useful.
src/services/filter/filters/evm/filter.rs (2)
320-333: Inconsistent hex_signature formatting; add 0x prefix in both branches.One branch sets hex_signature with "0x" while the other omits it.
Apply:
- hex_signature: Some(format!( - "0x{}", - hex::encode(function.selector()) - )), + hex_signature: Some(format!( + "0x{}", + hex::encode(function.selector()) + )), @@ - hex_signature: Some(hex::encode( - function.selector(), - )), + hex_signature: Some(format!( + "0x{}", + hex::encode(function.selector()) + )),Also applies to: 352-357
684-690: Usenumber()method consistently; alignhex_signatureprefix formatting.Two issues verified:
- Lines 686 vs 688: Mixing method and field access. Line 686 calls
number()returningOption<u64>, but line 688 accesses.numberfield returningOption<U64>with redundant.to::<u64>()conversion. Use the method consistently:- let current_block_number = evm_block.number.unwrap_or(U64::from(0)).to::<u64>(); + let current_block_number = evm_block.number().unwrap_or(0);
- hex_signature prefix inconsistency: Lines 326–328 and 622 format with
"0x{}"prefix, but line 354 uses barehex::encode()without prefix. Choose one approach and apply throughout.tests/integration/filters/common.rs (1)
118-131: Fix test match arms to safely handle Midnight contract spec variantTwo test assertions in tests/integration/bootstrap/main.rs use panicking fallback patterns that will fail when Midnight specs are encountered:
- Line 1273:
_ => panic!("Expected EVM contract spec")should handle Midnight case- Line 1362:
_ => panic!("Expected Stellar contract spec")should handle Midnight caseWhile service-level code in src/bootstrap/mod.rs and src/services/filter/filters/ safely handles the Midnight variant (via warning logs or returning None), these test matches will panic. Update both to either skip Midnight assertions or explicitly handle the variant.
src/models/config/network_config.rs (1)
165-186: Ensure at least one RPC URL is providedValidation promises this guarantee but doesn’t enforce it when rpc_urls is empty.
fn validate(&self) -> Result<(), ConfigError> { ... + // Require at least one RPC URL + if self.rpc_urls.is_empty() { + return Err(ConfigError::validation_error( + "At least one RPC URL must be specified", + None, + None, + )); + } // Validate RPC URL types and formats based on network let (supported_types, supported_protocols) = ...Also applies to: 207-231
src/utils/tests/builders/stellar/monitor.rs (1)
1-4: Doc header refers to EVM but this is StellarUpdate the module docs to avoid confusion.
-//! Test helper utilities for the EVM Monitor +//! Test helper utilities for the Stellar Monitortests/integration/blockchain/pool.rs (1)
200-205: Fix Stellar test network construction.Network2 is built as EVM; use the Stellar helper for consistency.
- let network2 = NetworkBuilder::new() - .name("test-2") - .slug("test-2") - .network_type(BlockChainType::EVM) - .rpc_urls(vec![&mock_server_2.url()]) - .build(); + let network2 = create_stellar_test_network_with_urls(vec![&mock_server_2.url()]);src/services/blockchain/pool.rs (1)
111-116: Avoid awaiting under a write lock; fix double-checked creationYou await client creation while holding a write lock, blocking readers and risking priority inversion. Create outside the lock, then insert with a second check.
- // Slow path: create new client - let mut clients = storage.clients.write().await; - let client = Arc::new(create_fn(network).await?); - clients.insert(network.slug.clone(), client.clone()); - Ok(client) + // Slow path: create new client outside the lock + let created = Arc::new(create_fn(network).await?); + let mut clients = storage.clients.write().await; + // Double-check in case another task inserted meanwhile + if let Some(existing) = clients.get(&network.slug) { + return Ok(existing.clone()); + } + clients.insert(network.slug.clone(), created.clone()); + Ok(created)tests/integration/monitor/execution.rs (1)
745-791: Test is mislabeled and misconfigured; it doesn’t exercise “Stellar get_latest_block_number failed.”
- Uses EVM network/client and provides a block number, yet asserts “not found.”
- Should use Stellar network/client and omit
block_numberto force latest lookup, then fail onget_latest_block_number().Apply this fix:
@@ -async fn test_execute_monitor_stellar_get_latest_block_number_failed() { - let test_data = TestDataBuilder::new("stellar").build(); +async fn test_execute_monitor_stellar_get_latest_block_number_failed() { + let test_data = TestDataBuilder::new("stellar").build(); @@ - let mut mock_pool = MockClientPool::new(); - let mock_network_service = - setup_mocked_network_service("Ethereum", "ethereum_mainnet", BlockChainType::EVM); - let mock_client = MockEvmClientTrait::new(); + let mut mock_pool = MockClientPool::new(); + let mock_network_service = + setup_mocked_network_service("Stellar", "stellar_testnet", BlockChainType::Stellar); + let mut mock_client = MockStellarClientTrait::new(); @@ - let mock_client = Arc::new(mock_client); + mock_client + .expect_get_latest_block_number() + .return_once(|| Err(anyhow::anyhow!("Failed to get latest block number"))); + let mock_client = Arc::new(mock_client); @@ - mock_pool - .expect_get_evm_client() - .return_once(move |_| Ok(mock_client)); + mock_pool + .expect_get_stellar_client() + .times(2) // execute_monitor calls client twice for Stellar (blocks/spec) + .returning(move |_| Ok(mock_client.clone())); @@ - let block_number = 22197425; + // Force latest lookup + let block_number = None; @@ - network_slug: Some("ethereum_goerli".to_string()), - block_number: Some(block_number), + network_slug: Some("stellar_testnet".to_string()), + block_number: block_number, @@ - assert!(result.is_err()); - assert!(result.unwrap_err().to_string().contains("not found")); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("Failed to get latest block number"));
♻️ Duplicate comments (4)
src/services/blockchain/clients/midnight/client.rs (1)
239-245: Confirm Midnight‑specific RPCs and add safe fallback.midnight_decodeEvents and midnight_jsonBlock look custom. Please confirm these exist on the target Midnight node/version and won’t regress in other environments. If unavailable, fall back to standard Substrate calls (e.g., chain_getBlock + local decode via subxt metadata).
Also, JSON‑RPC usually expects hex strings with 0x prefix. Suggest prefixing event bytes:
- let params = json!([hex::encode(event_bytes)]); + let params = json!([format!("0x{}", hex::encode(event_bytes))]);Optionally gate the custom path and keep a standard fallback:
- let response = client - .send_raw_request("midnight_jsonBlock", Some(params)) + let response = client + .send_raw_request("midnight_jsonBlock", Some(params.clone())) .await .with_context(|| format!("Failed to get block: {}", block_number))?; + // If method is not available, fall back: + // if response has error -32601 (Method not found), try chain_getBlockWould you like a follow‑up patch adding the fallback?
Also applies to: 377-381
src/models/config/network_config.rs (1)
201-206: Tighten protocol validation by network type (security and correctness)Currently:
- EVM/Stellar accept ws/wss, but their transports are HTTP.
- Midnight accepts ws://, which is insecure.
Proposed:
- EVM/Stellar: only http(s)
- Midnight: only wss
This prevents misconfiguration and addresses the “insecure websocket” warning.
- let (supported_types, supported_protocols) = match self.network_type { - BlockChainType::Midnight => (vec!["ws_rpc"], vec!["wss://", "ws://"]), - _ => (vec!["rpc"], vec!["http://", "https://", "wss://", "ws://"]), - }; + let (supported_types, supported_protocols) = match self.network_type { + BlockChainType::Midnight => (vec!["ws_rpc"], vec!["wss://"]), + _ => (vec!["rpc"], vec!["http://", "https://"]), + };Consider relaxing to allow ws:// only behind a feature flag or env for local dev.
src/services/filter/filters/midnight/filter.rs (1)
153-160: Event matching stub acknowledged.Event matching is empty, so event-based monitors won’t trigger yet. If intentional (per prior note), consider a TODO and unit test asserting current behavior (no event matches).
src/models/blockchain/midnight/event.rs (1)
202-205: Doc fix: method describes the wrong checkThis docstring repeats the previous method’s description. It should state that it checks for the guaranteed-only variant.
- /// Check if the event is a transaction applied event. + /// Check if the event is an only guaranteed transaction applied event.
🧹 Nitpick comments (73)
src/models/core/network.rs (1)
7-7: Doc: explicitly mention Midnight and field applicability.The new Midnight support isn’t reflected in the docs. Small clarity win to enumerate supported networks and note per-field applicability.
Apply on Line 7:
-/// Defines connection details and operational parameters for a specific blockchain network. +/// Defines connection details and operational parameters for a specific blockchain network (EVM, Stellar, Midnight).Optionally, also update these comments (outside this hunk) for precision:
/// Type of blockchain (EVM, Stellar, Midnight)/// Chain ID for EVM networks (not used for Stellar or Midnight)/// Network passphrase for Stellar networks (not used for EVM or Midnight)src/services/blockchain/transports/ws/config.rs (2)
72-74: from_network currently ignores NetworkConsider using Network fields (e.g., block_time_ms) to derive sensible defaults (heartbeat, timeouts) per network; otherwise, document why default is sufficient.
136-149: Redundant build()build() returns Self unchanged; callers can use the value directly after chaining. Either remove build() or make it validate/normalize values (e.g., clamp zero/overflow).
src/services/filter/filters/evm/helpers.rs (1)
69-74: Broader whitespace normalization: good; add tests for tabs/newlines.Behavior change is intentional; strengthen tests to cover '\t', '\n', and Unicode spaces in addresses/signatures.
Add tests:
@@ #[test] fn test_normalize_address() { @@ assert_eq!( normalize_address("0x0123456789abcdef 0123456789abcdef01234567"), "0123456789abcdef0123456789abcdef01234567" ); + assert_eq!( + normalize_address("0x0123\t4567\n89ab\r\ncdef0123456789abcdef01234567"), + "0123456789abcdef0123456789abcdef01234567" + ); } @@ #[test] fn test_normalize_signature() { @@ assert_eq!( normalize_signature("transfer (address , uint256 )"), "transfer(address,uint256)" ); + assert_eq!( + normalize_signature("transfer(\n address,\tuint256 \r)"), + "transfer(address,uint256)" + ); }Also applies to: 95-97
docs/project-structure.mdx (1)
47-48: Consider hyphenating compound adjectives for grammar.For proper grammar, compound adjectives should be hyphenated: "blockchain-specific" rather than "blockchain specific". This applies to both the Stellar and Midnight entries.
Apply this diff:
* `stellar/`: Stellar blockchain specific types - * `midnight/`: Midnight blockchain specific types + * `stellar/`: Stellar blockchain-specific types + * `midnight/`: Midnight blockchain-specific typessrc/services/filter/filter_match.rs (1)
237-321: Consider extracting common match handling logic.The Midnight match handling (lines 237-321) is nearly identical to the Stellar branch (lines 152-236), with only variable names and match types differing. This creates maintenance overhead—future changes to the payload structure would require updates in multiple places.
Consider extracting the common logic into a helper function:
async fn handle_generic_match<T: TriggerExecutionServiceTrait>( monitor_name: &str, transaction_hash: String, triggers: &[String], matched_on: &MatchConditions, matched_on_args: &Option<MatchedOnArgs>, matching_monitor: &MonitorMatch, trigger_service: &T, trigger_scripts: &HashMap<String, (ScriptLanguage, String)>, ) -> Result<(), FilterError> { let mut data_json = json!({ "monitor": { "name": monitor_name }, "transaction": { "hash": transaction_hash }, "functions": [], "events": [] }); // Process functions and events (shared logic) // ... let _ = trigger_service .execute(triggers, json_to_hashmap(&data_json), matching_monitor, trigger_scripts) .await; Ok(()) }Then call it from both Stellar and Midnight branches. This would centralize the payload construction logic and reduce duplication.
src/models/blockchain/stellar/monitor.rs (1)
244-250: Doc typo + optional forward-compat attribute
- Fix “used to for” → “used for”.
- Consider marking the struct non-exhaustive to allow adding fields later without breaking downstream code.
-/// This configuration is used to for additional fields in the monitor configuration +/// This configuration is used for additional fields in the monitor configuration /// that are specific to Stellar. -#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Default)] +#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Default)] +#[non_exhaustive] pub struct MonitorConfig {}tests/integration/blockchain/transports/midnight/transport.rs (2)
156-166: Remove unused local mock setupThe events_mock created inside get_events_at() isn’t used and can be dropped.
new_mock.expect_get_events_at().returning(|_| { - let mut events_mock = MockSubstrateClient::new(); - events_mock - .expect_clone() - .returning(MockSubstrateClient::new); Ok(mock_empty_events()) });
952-952: Brittle clone count expectationtimes(103) tightly couples the test to internal cloning behavior. Consider relaxing to at_least(…) or removing the count to reduce brittleness.
- mock_midnight.expect_clone().times(103).returning(|| { + mock_midnight.expect_clone().returning(|| {tests/integration/mocks/models.rs (1)
87-95: Align Midnight test network builder with EVM/Stellar helpersVerification confirms the inconsistency. Both
create_evm_test_network_with_urlsandcreate_stellar_test_network_with_urlssetcron_schedule("*/5 * * * * *"),confirmation_blocks(1),store_blocks(false), andblock_time_ms(5000). The Midnight helper omits these, creating inconsistent test defaults.pub fn create_midnight_test_network_with_urls(urls: Vec<&str>) -> Network { NetworkBuilder::new() .name("test") .slug("test") .network_type(BlockChainType::Midnight) + .cron_schedule("*/5 * * * * *") + .confirmation_blocks(1) + .store_blocks(false) + .block_time_ms(5000) .websocket_rpc_urls(urls) .build() }src/models/blockchain/evm/monitor.rs (1)
128-133: Consider adding a comment about future use of MonitorConfig.The
MonitorConfigstruct is currently empty but documented as "for additional fields... specific to EVM." Adding a TODO or more explicit comment about planned fields would clarify the purpose of this placeholder type./// EVM-specific configuration /// /// This configuration is used to for additional fields in the monitor configuration /// that are specific to EVM. +/// +/// TODO: Add EVM-specific monitor configuration fields as needed #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Default)] pub struct MonitorConfig {}docs/quickstart.mdx (3)
631-635: Fix copy path for Midnight network configPath has an extra "examples". Use the existing example at examples/config/networks/midnight_testnet.json.
Apply this diff:
-cp examples/config/networks/examples/midnight_testnet.json config/networks/midnight_testnet.json +cp examples/config/networks/midnight_testnet.json config/networks/midnight_testnet.json
757-759: Remove stray "----" in Docker section code blockThe extra line breaks markdown formatting and copy/paste.
----- cargo make docker-compose-up
102-106: Bullet list formatting consistencyUse markdown bullets instead of leading periods for “Next Steps”.
- . Customize the copied configurations in `config/` directories - . Update RPC URLs and notification credentials - . Run the monitor with `./openzeppelin-monitor` + - Customize the copied configurations in `config/` directories + - Update RPC URLs and notification credentials + - Run the monitor with `./openzeppelin-monitor`docs/rpc.mdx (7)
61-68: List Midnight transport alongside EVM/StellarDocument the websocket client used for Midnight.
2. **Network-Specific Transports**: * `EVMTransportClient` for EVM networks * `StellarTransportClient` for Stellar networks + * `MidnightWsTransportClient` for Midnight networks
47-49: Secret type casing should match JSONExamples use "plain"/"environment"/"hashicorpCloudVault". Align the table to avoid confusion.
-| `**url.type**` | `String` | Secret type ("Plain", "Environment", or "HashicorpCloudVault") | +| `**url.type**` | `String` | Secret type ("plain", "environment", or "hashicorpCloudVault") |
255-263: Correct Substrate RPC method spellingUse chain_getFinalizedHead (with “z”). Current spelling may mislead readers.
- D4 -->|chain_getFinalisedHead| E1[Get Finalized Block Hash] + D4 -->|chain_getFinalizedHead| E1[Get Finalized Block Hash]
300-301: Consistent method naming in calloutUse chain_getFinalizedHead here as well.
-The Midnight client uses `chain_getFinalisedHead` to ensure... +The Midnight client uses `chain_getFinalizedHead` to ensure...
264-265: Remove stray “....” under mermaid diagramThis breaks diagram rendering.
-....
304-305: Fix heading syntaxUse markdown heading instead of “==”.
-== Best Practices +## Best Practices
47-51: Clarify protocol support per network typeDocs imply ws/wss are acceptable everywhere. Given HTTP transports for EVM/Stellar and WS for Midnight, recommend documenting:
- EVM/Stellar: http(s) only
- Midnight: wss (websocket secure) only
-| `**type_**` | `String` | Type of endpoint ("rpc" or "ws_rpc") | +| `**type_**` | `String` | Type of endpoint ("rpc" for http(s), "ws_rpc" for websocket) | ... -For high-availability setups, configure at least 3 (private) RPC endpoints... +For high-availability setups, configure at least 3 private endpoints: + - EVM/Stellar: http(s) "rpc" URLs + - Midnight: wss "ws_rpc" URLsAlso applies to: 201-206
src/utils/tests/builders/evm/monitor.rs (1)
20-21: Expose a builder setter for chain_configurationsAllow tests to override/append per-chain configs explicitly; also assert default isn’t empty.
pub struct MonitorBuilder { ... triggers: Vec<String>, chain_configurations: Vec<ChainConfiguration>, } impl MonitorBuilder { ... + pub fn chain_configurations(mut self, cfgs: Vec<ChainConfiguration>) -> Self { + self.chain_configurations = cfgs; + self + } }Optionally extend test_default_monitor() to assert non-empty chain_configurations.
Also applies to: 40-44
examples/config/triggers/slack_notifications.json (1)
44-57: Prefer environment-based secret for Slack webhook in examplesAvoid embedding webhooks, even in examples. Use "environment" to promote safer defaults.
- "slack_url": { - "type": "plain", - "value": "https://hooks.slack.com/services/A/B/C" - }, + "slack_url": { + "type": "environment", + "value": "SLACK_WEBHOOK_URL" + },Consider applying the same change to other Slack entries in this file.
src/utils/tests/builders/network.rs (2)
210-231: Align example types with validated valuesTests use "http"/"ws" as type_, whereas config validation expects "rpc"/"ws_rpc". To reduce confusion for readers of test code, consider switching to "rpc"/"ws_rpc" here.
96-106: Add variant that appends ws URLs instead of replacingwebsocket_rpc_urls replaces existing entries. A convenience append_ws_rpc_url(s) can improve ergonomics in multi-endpoint tests.
pub fn websocket_rpc_urls(mut self, urls: Vec<&str>) -> Self { self.rpc_urls = urls .into_iter() .map(|url| RpcUrl { type_: "ws_rpc".to_string(), url: SecretValue::Plain(SecretString::new(url.to_string())), weight: 100, }) .collect(); self } + +pub fn add_websocket_rpc_url(mut self, url: &str, weight: u32) -> Self { + self.rpc_urls.push(RpcUrl { + type_: "ws_rpc".to_string(), + url: SecretValue::Plain(SecretString::new(url.to_string())), + weight, + }); + self +}src/services/blockchain/transports/ws/connection.rs (2)
21-25: Restrict field visibility to avoid accidental external mutation
streamandis_healthyare public; external modules can break invariants. Limit to crate-level and use methods for state changes. This still allowstransport.rsto update them.- pub stream: Option<WebSocketStream<MaybeTlsStream<TcpStream>>>, - pub is_healthy: bool, + pub(crate) stream: Option<WebSocketStream<MaybeTlsStream<TcpStream>>>, + pub(crate) is_healthy: bool,Optionally add a helper to clear state atomically:
impl WebSocketConnection { + /// Mark connection unhealthy and drop the stream atomically + pub fn mark_unhealthy(&mut self) { + self.is_healthy = false; + self.stream = None; + } }
43-63: Expose elapsed idle time helper for heartbeat logicA small helper improves readability for heartbeat/timeout checks and avoids scattered
Instant::now()diffs.impl WebSocketConnection { + /// How long since the last activity on this connection. + pub fn idle_for(&self) -> std::time::Duration { + Instant::now().saturating_duration_since(self.last_activity) + } }src/models/config/monitor_config.rs (1)
22-41: Simplify async collection withtry_join_allCurrent
FuturesUnordered + try_collectworks but is verbose.try_join_allis simpler and keeps concurrency.- for chain_configuration in &mut monitor.chain_configurations { + for chain_configuration in &mut monitor.chain_configurations { if let Some(midnight) = &mut chain_configuration.midnight { - midnight.viewing_keys = midnight - .viewing_keys - .iter() - .map(|key| async { - key.resolve().await.map(SecretValue::Plain).map_err(|e| { - ConfigError::parse_error( - format!("failed to resolve viewing key: {}", e), - Some(Box::new(e)), - None, - ) - }) - }) - .collect::<futures::stream::FuturesUnordered<_>>() - .try_collect() - .await?; + midnight.viewing_keys = futures::future::try_join_all( + midnight.viewing_keys.iter().map(|key| async { + key.resolve().await.map(SecretValue::Plain).map_err(|e| { + ConfigError::parse_error( + format!("failed to resolve viewing key: {}", e), + Some(Box::new(e)), + None, + ) + }) + }), + ) + .await?; } }Add import:
-use futures::TryStreamExt; +use futures::{future::try_join_all, TryStreamExt};src/utils/tests/builders/stellar/monitor.rs (1)
20-21: Expose a setter forchain_configurationsin the builderYou added default Stellar chain config, but tests may need to override/compose configs. Provide a setter.
impl MonitorBuilder { + pub fn chain_configurations( + mut self, + chain_configurations: Vec<ChainConfiguration>, + ) -> Self { + self.chain_configurations = chain_configurations; + self + }Also applies to: 40-43, 163-173
tests/integration/mocks/transports.rs (2)
372-376: Embed fixture at compile time to avoid FS dependencyUse
include_str!for deterministic, faster tests and fewer path issues.- let data = - std::fs::read_to_string("tests/integration/fixtures/midnight/state_call.json").unwrap(); - let json_response: Value = serde_json::from_str(&data).unwrap(); + const STATE_CALL_JSON: &str = + include_str!("../fixtures/midnight/state_call.json"); + let json_response: Value = serde_json::from_str(STATE_CALL_JSON).unwrap();
134-159: Align generic bounds with the trait (optional)The trait requires
Serialize. While compilers accept omitting it in the impl signature, mirroring bounds avoids confusion.- where - P: Into<Value> + Send + Clone, + where + P: Into<Value> + Send + Clone + Serialize,Apply similarly in the EVM/Stellar impls above for consistency.
src/services/blockchain/transports/midnight/ws.rs (1)
26-29: Naming parity with the codebase (Ws suffix).Elsewhere the code registers/uses MidnightWsTransportClient. Ensure a public alias exists to avoid type mismatches.
If missing, add this alias (exported where you re-export transports):
+/// Alias to keep naming parity with other transports (EVM/Stellar). +pub type MidnightWsTransportClient = MidnightTransportClient;tests/integration/blockchain/transports/ws/endpoint_manager.rs (2)
50-67: Assert no transport calls when no fallbacks.Tighten this test to prove we short‑circuit without dialing or updating.
let manager = WsEndpointManager::new(&config, &url, vec![]); -let transport = MockMidnightWsTransportClient::new(); +let mut transport = MockMidnightWsTransportClient::new(); +transport.expect_try_connect().times(0); +transport.expect_update_client().times(0);
320-346: Avoid false positives: ensure update_client isn’t called on retry failure.Add an explicit expectation that update_client is never invoked when all reconnect attempts fail.
let mut transport = MockMidnightWsTransportClient::new(); // Set up mock to fail connection attempts transport .expect_try_connect() .with(predicate::eq(invalid_url)) .times(2) .returning(|_| Err(anyhow::anyhow!("Failed to connect to fallback URL"))); +transport.expect_update_client().times(0);tests/integration/filters/midnight/filter.rs (3)
293-298: Clarify expected behavior for invalid block types.Test allows both Err and Ok([]). Pick one contract and assert it to avoid brittle tests.
445-533: “Key collision” test doesn’t exercise an actual collision.Because Midnight args are private, this currently only checks the function signature key. Either rename the test to reflect intent (signature retained), or inject a synthetic arg map to assert that “functions.0.signature” and “functions.0.args.signature” coexist distinctly.
I can draft a helper to inject synthetic args into the captured data for this test if desired.
350-369: Add a test asserting trigger errors are swallowed by handle_match.Current tests only check success. Add one where execute returns Err; assert handle_match returns Ok(()).
Also applies to: 430-439
tests/integration/blockchain/clients/midnight/client.rs (3)
170-173: Remove unused get_current_url expectation.It’s not exercised here; dropping it simplifies the test.
-mock_midnight - .expect_get_current_url() - .returning(|| "ws://dummy".to_string());
116-126: Add a unit test for the real JSON‑RPC parsing path.Mock send_raw_request for chain_getFinalisedHead and chain_getHeader to exercise hex parsing and error mapping in get_latest_block_number().
I can sketch the exact mock expectations if helpful.
240-266: Extend error tests to cover null ‘midnight_jsonBlock’ response.Add a case where ‘result’ is the JSON string "null" so the client returns “Block not found”.
tests/integration/blockchain/transports/midnight/ws.rs (1)
329-336: Stabilize timeout assertion to reduce env‑specific flakiness.
DNS failures may surface as immediate “Failed to connect” instead of a timeout on some runners. Accept either to keep the test robust.Apply this change:
-assert!(result.is_err()); -assert!(result.unwrap_err().to_string().contains("timeout")); +assert!(result.is_err()); +let msg = result.unwrap_err().to_string(); +assert!( + msg.contains("timeout") || msg.contains("Failed to connect"), + "Expected timeout or connection failure, got: {msg}" +);src/utils/tests/builders/midnight/monitor.rs (2)
5-10: Import ContractSpec to enable per‑address spec helpers.
Prepare for an optional address_with_spec helper below.Apply:
-use crate::models::{ - AddressWithSpec, ChainConfiguration, EventCondition, FunctionCondition, MatchConditions, - MidnightMonitorConfig, Monitor, ScriptLanguage, TransactionCondition, TransactionStatus, - TriggerConditions, -}; +use crate::models::{ + AddressWithSpec, ChainConfiguration, ContractSpec, EventCondition, FunctionCondition, + MatchConditions, MidnightMonitorConfig, Monitor, ScriptLanguage, TransactionCondition, + TransactionStatus, TriggerConditions, +};
74-103: Add address_with_spec helper for parity and explicitness.
Even if Midnight’s spec is a unit, having the API mirrors other builders and clarifies intent in tests.Insert after the existing address(...) method:
+ /// Set a single address with an optional contract spec (e.g., Midnight) + pub fn address_with_spec( + mut self, + address: &str, + contract_spec: Option<ContractSpec>, + ) -> Self { + self.addresses = vec![AddressWithSpec { + address: address.to_string(), + contract_spec, + }]; + self + }src/models/blockchain/mod.rs (2)
55-64: Consider consistent boxing across TransactionType variants.
Mixing boxed (Stellar) and non‑boxed (EVM/Midnight) variants can bloat enum size. Unifying to Box for all three reduces size variance and copy costs.Example:
pub enum TransactionType { - EVM(evm::EVMTransaction), - Stellar(Box<stellar::StellarTransaction>), - Midnight(midnight::MidnightTransaction), + EVM(Box<evm::EVMTransaction>), + Stellar(Box<stellar::StellarTransaction>), + Midnight(Box<midnight::MidnightTransaction>), }If you prefer the current shape, please confirm max enum size stays acceptable in hot paths (e.g., metrics, queues).
67-76: Validate serde shape for ContractSpec::Midnight (untagged + unit).
With untagged enums, a unit variant typically serializes/deserializes as null. If you want explicit “midnight” in JSON instead of null, consider a tagged enum or a newtype. Otherwise, document the null behavior near API boundaries.Please confirm this representation is intentional for config and API payloads.
src/models/blockchain/midnight/monitor.rs (1)
46-48: Doc-type mismatch for hex_signature.Field is String but doc says “bytes”. Either clarify as “hex-encoded string” or change the type to Vec with hex serde.
- /// Raw function/event signature as bytes + /// Raw function/event signature as a hex-encoded stringsrc/models/blockchain/midnight/block.rs (1)
27-31: Harden against RPC field name drift.Support both transactions_index and transactionsIndex via serde alias.
- #[serde(rename = "transactions_index")] + #[serde(rename = "transactions_index", alias = "transactionsIndex")] pub transactions_index: Vec<(String, String)>,tests/integration/blockchain/pool.rs (1)
65-70: Unify generics for Midnight client count assertions.Use MidnightClient to match registered type.
- pool.get_client_count::<MidnightClient<WsTransportClient>>(BlockChainType::Midnight) + pool.get_client_count::<MidnightClient<MidnightWsTransportClient>>(BlockChainType::Midnight) .await,Apply at:
- Line 66 (EVM cache test)
- Line 321 (default pool test)
- Line 373 (EVM error path test)
Also applies to: 321-324, 373-376
tests/integration/blockchain/transports/ws/transport.rs (1)
365-369: Make timeout assertion robust to DNS/connect vs timeout errors.Networking errors can be “Failed to connect …” rather than “timeout” depending on resolution timing.
- let result = WsTransportClient::new(&network, Some(config)).await; - assert!(result.is_err()); - assert!(result.unwrap_err().to_string().contains("timeout")); + let result = WsTransportClient::new(&network, Some(config)).await; + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("timeout") || err.contains("Failed to connect"), + "Unexpected error: {err}" + );tests/integration/mocks/subxt.rs (2)
110-121: Tighten generic bounds on eventsYou only encode records; Decode isn’t required here.
-pub fn events<E: Decode + Encode>( +pub fn events<E: Encode>(
98-102: Prefer expect over unwrap for clearer test failuresImprove error message if metadata conversion fails.
- let metadata: subxt::Metadata = runtime_metadata.try_into().unwrap(); + let metadata: subxt::Metadata = runtime_metadata + .try_into() + .expect("Failed to convert RuntimeMetadataPrefixed into subxt::Metadata");src/utils/tests/builders/midnight/transaction.rs (1)
23-34: Optional: improve builder ergonomics and reduce clonesAccept Into and IntoIterator where applicable.
-pub fn hash(mut self, hash: String) -> Self { +pub fn hash<S: Into<String>>(mut self, hash: S) -> Self { - self.tx_hash = hash; + self.tx_hash = hash.into(); self } -pub fn operations(mut self, operations: Vec<MidnightOperation>) -> Self { - self.operations = operations; +pub fn operations<I>(mut self, operations: I) -> Self +where + I: IntoIterator<Item = MidnightOperation>, +{ + self.operations = operations.into_iter().collect(); self } -pub fn add_call_operation(mut self, address: String, entry_point: String) -> Self { +pub fn add_call_operation<A: Into<String>, E: Into<String>>( + mut self, + address: A, + entry_point: E, +) -> Self { self.operations.push(MidnightOperation::Call { - address, - entry_point, + address: address.into(), + entry_point: entry_point.into(), }); self } -pub fn add_deploy_operation(mut self, address: String) -> Self { +pub fn add_deploy_operation<A: Into<String>>(mut self, address: A) -> Self { - self.operations.push(MidnightOperation::Deploy { address }); + self + .operations + .push(MidnightOperation::Deploy { address: address.into() }); self } -pub fn add_maintain_operation(mut self, address: String) -> Self { +pub fn add_maintain_operation<A: Into<String>>(mut self, address: A) -> Self { self.operations - .push(MidnightOperation::Maintain { address }); + .push(MidnightOperation::Maintain { address: address.into() }); self } -pub fn identifiers(mut self, identifiers: Vec<String>) -> Self { - self.identifiers = identifiers; +pub fn identifiers<I, S>(mut self, identifiers: I) -> Self +where + I: IntoIterator<Item = S>, + S: Into<String>, +{ + self.identifiers = identifiers.into_iter().map(Into::into).collect(); self }Also applies to: 35-46, 56-86, 88-104
src/services/blockchain/pool.rs (1)
100-105: Enrich error context for invalid client typeInclude the requested client_type in the error for faster debugging.
- .with_context(|| "Invalid client type")?; + .with_context({ + let client_type = client_type.clone(); + move || format!("Invalid client type: {:?}", client_type) + })?;src/models/blockchain/midnight/mod.rs (1)
28-31: Optional: reconsider alias naming for clarity
MidnightRpcTransaction as MidnightBaseTransactioncan be confusing. ConsiderMidnightRpcTransactionandMidnightTransactionwithout aliasing to reduce cognitive overhead.src/utils/tests/builders/midnight/block.rs (3)
78-83: Avoid unnecessary clone on pushNo need to clone before pushing to body.
- self.body.push(tx_operation.clone()); + self.body.push(tx_operation);
16-31: Optional: unify number formattingDefault header uses "0" while setter uses hex ("0x…"). Consider defaulting to "0x0" for consistency with parsing logic that trims "0x".
- number: "0".to_string(), + number: "0x0".to_string(),If you adopt this, adjust the assertion in test_builder_default accordingly.
70-84: Optional: populate tx_raw and transactions_index realisticallyIf available, serialize the transaction (or a stable hash) into tx_raw and index to reduce future refactors.
- let tx_operation = MidnightRpcTransactionEnum::MidnightTransaction { - tx_raw: "".to_string(), - tx: transaction, - }; + let tx_raw = String::new(); // TODO: serialize transaction once format decided + let tx_operation = MidnightRpcTransactionEnum::MidnightTransaction { tx_raw, tx: transaction };src/services/blockchain/transports/ws/endpoint_manager.rs (1)
98-136: Consider deduping fallback URLs after rotations.Failed and timed‑out URLs are pushed back into
fallback_urlswithout dedupe; over time this can accumulate duplicates and skew rotation. Suggest deduping when pushing back.tests/integration/monitor/execution.rs (5)
793-848: Duplicate of earlier EVM “failed to get block” scenarios; consolidate to reduce flakiness.This adds another EVM negative test overlapping with existing coverage. Recommend merging assertions or removing duplicates to keep CI time down.
850-903: Redundant with existing “get latest block number failed” test for EVM.Please remove or consolidate with the prior test to avoid duplicate coverage.
1000-1055: Stellar “failed to get block (not found)” test duplicates earlier coverage.Earlier tests already validate empty blocks and not‑found error mapping. Consider removing this duplicate.
1058-1105: Stellar “failed to get client” test is duplicated.There’s an earlier version with the same assertion; keep a single canonical case.
1106-1166: Stellar “failed to get block by number” duplicates earlier negative path.Consolidate or differentiate assertions (e.g., error content vs type) to justify both tests.
src/services/blockchain/transports/ws/transport.rs (1)
120-134: Avoid double handshake on startup.You first
connect_asyncto probe, thenconnect()callstry_connectagain. Consider reusing the successful stream from probing to initializeconnection.streamand skip the second handshake.src/utils/monitor/execution.rs (2)
321-336: Avoid cloning the entire matches vector during notifications.Iterate by reference or by iterator cloning to reduce allocations.
Apply:
- for match_result in all_matches.clone() { + for match_result in all_matches.iter().cloned() {
261-314: Consistency nit: add block fetch/debug logs for Midnight as done for EVM.Parity helps troubleshooting across networks.
Example:
+ tracing::debug!(block = %block_number, "Fetching block"); ... + tracing::debug!(block = %block_number, "Filtering block");src/services/filter/filters/midnight/filter.rs (2)
188-242: Be resilient to per-transaction decode failures.Currently, a single parse error aborts the whole block. Prefer log-and-skip to avoid losing matches from other txs.
Example:
- Err(e) => { - return Err(FilterError::network_error( - "Error deserializing transaction", - Some(e.into()), - None, - )); - } + Err(e) => { + tracing::warn!("Skipping tx {} due to deserialize error: {}", hash, e); + continue; + }
279-285: map_chain_type is case-sensitive; lower-case first to avoid surprises.Update helper to compare in lower-case (see helpers.rs suggestion).
src/services/filter/filters/midnight/helpers.rs (1)
88-99: Make map_chain_type case-insensitive.Current contains() checks are case-sensitive. Normalize once.
Apply:
-pub fn map_chain_type(chain_type: &str) -> NetworkId { - if chain_type.contains("testnet") { +pub fn map_chain_type(chain_type: &str) -> NetworkId { + let ct = chain_type.to_lowercase(); + if ct.contains("testnet") { NetworkId::TestNet - } else if chain_type.contains("mainnet") { + } else if ct.contains("mainnet") { NetworkId::MainNet - } else if chain_type.contains("devnet") { + } else if ct.contains("devnet") { NetworkId::DevNet } else { NetworkId::Undeployed } }src/models/blockchain/midnight/event.rs (4)
215-229: Avoid clones in getters; return referencesReduce allocations by returning references. Callers needing owned values can clone at the edge.
- pub fn get_tx_hash(&self) -> Option<String> { + pub fn get_tx_hash(&self) -> Option<&str> { match &self.0 { - EventType::MidnightTxApplied(details) => Some(details.tx_hash.clone()), - EventType::MidnightOnlyGuaranteedTxApplied(details) => Some(details.tx_hash.clone()), - EventType::MidnightCallContract(details) => Some(details.tx_hash.clone()), - EventType::MidnightDeployContract(details) => Some(details.tx_hash.clone()), - EventType::MidnightMaintainContract(details) => Some(details.tx_hash.clone()), - EventType::MidnightClaimMint(details) => Some(details.tx_hash.clone()), + EventType::MidnightTxApplied(details) => Some(details.tx_hash.as_str()), + EventType::MidnightOnlyGuaranteedTxApplied(details) => Some(details.tx_hash.as_str()), + EventType::MidnightCallContract(details) => Some(details.tx_hash.as_str()), + EventType::MidnightDeployContract(details) => Some(details.tx_hash.as_str()), + EventType::MidnightMaintainContract(details) => Some(details.tx_hash.as_str()), + EventType::MidnightClaimMint(details) => Some(details.tx_hash.as_str()), EventType::MidnightPayoutMinted(_) => None, EventType::Unknown(_) => None, } } @@ - pub fn get_topics(&self) -> Option<Vec<String>> { + pub fn get_topics(&self) -> Option<&[String]> { match &self.0 { - EventType::MidnightTxApplied(details) => Some(details.topics.topics.clone()), - EventType::MidnightOnlyGuaranteedTxApplied(details) => { - Some(details.topics.topics.clone()) - } - EventType::MidnightCallContract(details) => Some(details.topics.topics.clone()), - EventType::MidnightDeployContract(details) => Some(details.topics.topics.clone()), - EventType::MidnightMaintainContract(details) => Some(details.topics.topics.clone()), - EventType::MidnightPayoutMinted(details) => Some(details.topics.topics.clone()), - EventType::MidnightClaimMint(details) => Some(details.topics.topics.clone()), + EventType::MidnightTxApplied(details) => Some(&details.topics.topics), + EventType::MidnightOnlyGuaranteedTxApplied(details) => Some(&details.topics.topics), + EventType::MidnightCallContract(details) => Some(&details.topics.topics), + EventType::MidnightDeployContract(details) => Some(&details.topics.topics), + EventType::MidnightMaintainContract(details) => Some(&details.topics.topics), + EventType::MidnightPayoutMinted(details) => Some(&details.topics.topics), + EventType::MidnightClaimMint(details) => Some(&details.topics.topics), EventType::Unknown(_) => None, } }Also applies to: 231-247
21-30: Make Phase Copy and drop clones in get_phasePhase is small and trivially Copy. This removes several clones and simplifies call sites.
-#[derive(Clone, PartialEq, Eq, Debug, Serialize, Deserialize)] +#[derive(Clone, Copy, PartialEq, Eq, Debug, Serialize, Deserialize)] pub enum Phase { @@ pub fn get_phase(&self) -> Option<Phase> { match &self.0 { - EventType::MidnightTxApplied(details) => Some(details.phase.clone()), - EventType::MidnightOnlyGuaranteedTxApplied(details) => Some(details.phase.clone()), - EventType::MidnightCallContract(details) => Some(details.phase.clone()), - EventType::MidnightDeployContract(details) => Some(details.phase.clone()), - EventType::MidnightMaintainContract(details) => Some(details.phase.clone()), - EventType::MidnightPayoutMinted(details) => Some(details.phase.clone()), - EventType::MidnightClaimMint(details) => Some(details.phase.clone()), + EventType::MidnightTxApplied(details) => Some(details.phase), + EventType::MidnightOnlyGuaranteedTxApplied(details) => Some(details.phase), + EventType::MidnightCallContract(details) => Some(details.phase), + EventType::MidnightDeployContract(details) => Some(details.phase), + EventType::MidnightMaintainContract(details) => Some(details.phase), + EventType::MidnightPayoutMinted(details) => Some(details.phase), + EventType::MidnightClaimMint(details) => Some(details.phase), EventType::Unknown(_) => None, } }Also applies to: 253-263
293-303: Differentiate parse errors from unknown variants; remove brittle error parsing
- Comma-based trimming of serde errors is brittle.
- Consider a non-lossy constructor (TryFrom) and keep From as a lenient fallback if you want ergonomics.
-impl From<Value> for Event { - fn from(value: Value) -> Self { - match serde_json::from_value::<EventType>(value) { - Ok(event_type) => Event(event_type), - Err(e) => Event(EventType::Unknown(format!( - "Failed to deserialize event: {}", - e.to_string().split(",").next().unwrap_or_default() - ))), - } - } -} +impl TryFrom<Value> for Event { + type Error = serde_json::Error; + fn try_from(value: Value) -> Result<Self, Self::Error> { + serde_json::from_value::<EventType>(value).map(Event) + } +} + +impl From<Value> for Event { + fn from(value: Value) -> Self { + Self::try_from(value).unwrap_or_else(|e| { + Event(EventType::Unknown(format!("Failed to deserialize event: {e}"))) + }) + } +}Optional: if you want to separate concerns, introduce a distinct variant for parse errors (e.g., Invalid(String)) and reserve Unknown(String) strictly for unknown event kinds.
266-279: Add AsRef/AsMut for ergonomics (keep Deref/DerefMut as-is)Deref/DerefMut works, but AsRef/AsMut improves generic ergonomics without relying on assignment via deref.
+impl AsRef<EventType> for Event { + fn as_ref(&self) -> &EventType { + &self.0 + } +} + +impl AsMut<EventType> for Event { + fn as_mut(&mut self) -> &mut EventType { + &mut self.0 + } +}
* chore: Upgrade midnight packages * chore: Remove patch
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #175 +/- ##
=======================================
- Coverage 96.6% 95.5% -1.1%
=======================================
Files 76 87 +11
Lines 26630 24543 -2087
=======================================
- Hits 25739 23456 -2283
- Misses 891 1087 +196
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
https://linear.app/openzeppelin-development/issue/PLAT-6492/add-network-and-models
This PR lays the foundational groundwork for integrating the Midnight network into the OpenZeppelin Monitor. It introduces essential models and initial test coverage to support Midnight's unique blockchain structure.
Key Changes:
Reviewer Focus Areas:
Merged PRs:
Testing Process
Checklist
Summary by CodeRabbit
Release Notes
New Features
Documentation
Configuration