Chain event handler

[tompro]

User description

📝 Description

Replaces all Nostr notification sending that need to have bill block data attached with new BillChainEvent on call site. The only missing one is BillAction::Mint (which was not implemented before). The Nostr event processor has been replaced with a version that can handle BlockEvent data and keys in addition to Notifictations. Chains, blocks and keys are validated and persisted in the according stores.

Relates to #424

---

✅ Checklist

Please ensure the following tasks are completed before requesting a review:

• My code adheres to the coding guidelines of this project.
• I have run cargo fmt.
• I have run cargo clippy.
• I have added or updated tests (if applicable).
• All CI/CD steps were successful.
• I have updated the documentation (if applicable).
• I have checked that there are no console errors or warnings.
• I have verified that the application builds without errors.
• I've described the changes made to the API. (modification, addition, deletion).

---

🚀 Changes Made

• New Features:
  • Sync Bill blocks via Nostr DMs

---

💡 How to Test

Please provide clear instructions on how reviewers can test your changes:

1. cargo test

---

🤝 Related Issues

List any related issues, pull requests, or discussions:

• Replace Gossipsub with Nostr #297

---

📋 Review Guidelines

Please focus on the following while reviewing:

• Does the code follow the repository's contribution guidelines?
• Are there any potential bugs or performance issues?
• Are there any typos or grammatical errors in the code or comments?

---

PR Type

Enhancement, Tests

---

Description

• Replaced BillActionEventPayload with BillChainEvent for enhanced event handling.

• Introduced BillChainEventHandler to process blockchain-related notifications.

• Refactored notification service to support BillChainEvent and new event types.

• Updated tests to validate BillChainEvent integration and event propagation.

---

Changes walkthrough 📝

	Relevant files
Enhancement	15 files default_service.rs Refactored notification service to use `BillChainEvent`    +739/-350 bill_chain_event_handler.rs Added `BillChainEventHandler` for blockchain event processing +690/-0  mod.rs Integrated `BillChainEventHandler` into notification handlers +105/-11 notification.rs Added `BillEventType` for detailed event categorization    +17/-16  nostr.rs Adjusted Nostr notification handling for `BillChainEvent` +5/-5      mod.rs Integrated `BillChainEventHandler` into notification service +8/-4      mod.rs Added `EventType` and updated event structure                        +18/-2    chain_event.rs Enhanced `BillChainEvent` for participant-specific event generation +38/-11  bill_events.rs Refactored event payloads for blockchain notifications      +8/-13    propagation.rs Refactored block action notifications to use `BillChainEvent` +17/-59  issue.rs Updated bill signing to use `BillChainEvent`                          +1/-1      notification_service.rs Updated notification service API for `BillChainEvent`        +11/-17  service_context.rs Updated service context to include blockchain stores          +3/-1      context.rs Updated WASM context to support blockchain stores                +3/-1      lib.rs Exported `BillChainEvent` and related types                            +2/-2
Tests	4 files mod.rs Updated tests to align with `BillChainEvent` changes          +19/-19  mod.rs Updated mock implementations for `BillChainEvent`                +12/-18  test_utils.rs Enhanced test utilities for `BillChainEvent`                          +9/-7      test_utils.rs Added utility for generating `BillKeys`                                    +7/-0
Miscellaneous	1 files notification.rs Removed unused imports in notification API                              +1/-27
Additional files	2 files bill_action_event_handler.rs +0/-114  Cargo.toml +0/-1

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[codecov]

Codecov Report

Attention: Patch coverage is 54.43038% with 180 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...-transport/src/handler/bill_chain_event_handler.rs	7.51%	160 Missing ⚠️
...r-ebill-api/src/service/bill_service/test_utils.rs	0.00%	6 Missing ⚠️
.../bcr-ebill-api/src/service/bill_service/payment.rs	25.00%	3 Missing ⚠️
crates/bcr-ebill-web/src/service_context.rs	0.00%	3 Missing ⚠️
...rc/service/notification_service/default_service.rs	98.42%	2 Missing ⚠️
...rates/bcr-ebill-transport/src/event/chain_event.rs	93.54%	2 Missing ⚠️
crates/bcr-ebill-wasm/src/context.rs	33.33%	2 Missing ⚠️
...es/bcr-ebill-api/src/service/bill_service/issue.rs	75.00%	1 Missing ⚠️
crates/bcr-ebill-core/src/notification.rs	91.66%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 297 - PR Code Verified Compliant requirements: Replace libp2p Gossipsub for events and topic subscription fully with Nostr Requires further human verification: If possible, replace coordination mechanisms using the DHT with Nostr (e.g., when a bill is created, participants can be notified using Nostr with information on where to get the keys and bill data)

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 Security concerns Sensitive information exposure: The BillChainEvent contains private keys (in the BillKeys structure) that are being transmitted over the network in the BillChainEventPayload. While these are likely encrypted during transmission, storing and passing private keys in event payloads increases the risk of key exposure. Consider implementing a more secure key management system that doesn't require transmitting private keys directly in event payloads.

⚡ Recommended focus areas for review Error Handling The error handling in the process_chain_data method could be improved. When adding blocks to an existing chain, it returns early on the first error, but doesn't roll back previous successful block additions, which could leave the chain in an inconsistent state. async fn process_chain_data( &self, bill_id: &str, blocks: Vec<BillBlock>, keys: Option<BillKeys>, ) -> Result<()> { match keys { Some(keys) => self.add_new_chain(blocks, &keys).await, None if !blocks.is_empty() => self.add_bill_blocks(bill_id, blocks).await, _ => Ok(()), } } Potential Performance Issue The send_all_events method processes events sequentially. For a large number of events, this could create performance bottlenecks. Consider processing events in parallel using futures::future::join_all or similar approaches. async fn send_all_events(&self, events: Vec<Event<BillChainEventPayload>>) -> Result<()> { for event_to_process in events.into_iter() { if let Ok(Some(identity)) = self .contact_service .get_identity_by_node_id(&event_to_process.node_id) .await { self.notification_transport .send(&identity, event_to_process.try_into()?) .await?; } } Ok(()) } Security Concern The add_new_chain method accepts and processes blocks without verifying the sender's identity or permissions, which could allow unauthorized parties to inject blocks into the chain. async fn add_new_chain(&self, blocks: Vec<BillBlock>, keys: &BillKeys) -> Result<()> { let (bill_id, chain) = self.get_valid_chain(blocks, keys)?; for block in chain.blocks() { self.save_block(&bill_id, block).await?; } self.save_keys(&bill_id, keys).await?; Ok(()) }

[qodo-code-review]

Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Check block addition success Suggestion Impact: The commit implemented the core idea of the suggestion by checking the return value of try_add_block() and handling failure cases, though with different error messages and by combining the block addition check with the chain validation check code diff: - chain.try_add_block(block.clone()); - if !chain.is_chain_valid() { - error!("Received block is not valid for bill {bill_id}"); - return Err(Error::BlockChain( - "Received bill block is not valid".to_string(), + if !chain.try_add_block(block.clone()) { + error!("Received invalid block for bill {bill_id}"); + return Err(Error::Blockchain( + "Received invalid block for bill".to_string(), )); } The function ignores the return value of chain.try_add_block(block.clone()), which returns a boolean indicating success or failure. If the block addition fails but doesn't invalidate the chain, this error will be silently ignored, and the function will proceed to save an invalid block. crates/bcr-ebill-transport/src/handler/bill_chain_event_handler.rs [113-132] async fn add_bill_blocks(&self, bill_id: &str, blocks: Vec<BillBlock>) -> Result<()> { if let Ok(mut chain) = self.bill_blockchain_store.get_chain(bill_id).await { for block in blocks { - chain.try_add_block(block.clone()); + let block_added = chain.try_add_block(block.clone()); + if !block_added { + error!("Failed to add block to chain for bill {bill_id}"); + return Err(Error::BlockChain( + "Failed to add block to chain".to_string(), + )); + } if !chain.is_chain_valid() { - error!("Received block is not valid for bill {bill_id}"); + error!("Chain became invalid after adding block for bill {bill_id}"); return Err(Error::BlockChain( - "Received bill block is not valid".to_string(), + "Chain became invalid after adding block".to_string(), )); } self.save_block(bill_id, &block).await? } Ok(()) } else { error!("Failed to get chain for received bill block {bill_id}"); Err(Error::BlockChain( "Failed to get chain for bill".to_string(), )) } } [Suggestion has been applied] Suggestion importance[1-10]: 9 __ Why: The suggestion identifies a critical bug where the return value of try_add_block is ignored, potentially allowing invalid blocks to be saved. This could lead to data corruption or security issues, making this a high-impact fix.	High
	Propagate errors instead of swallowing The function swallows errors from process_chain_data and create_notification without propagating them to the caller. This could lead to silent failures where the event appears to be handled successfully but critical operations failed. Consider returning these errors to the caller or at least logging and returning a specific error type. crates/bcr-ebill-transport/src/handler/bill_chain_event_handler.rs [200-221] async fn handle_event(&self, event: EventEnvelope, node_id: &str) -> Result<()> { if let Ok(decoded) = Event::<BillChainEventPayload>::try_from(event.clone()) { if !decoded.data.blocks.is_empty() { - if let Err(e) = self + match self .process_chain_data( &decoded.data.bill_id, decoded.data.blocks.clone(), decoded.data.keys.clone(), ) .await { - error!("Failed to process chain data: {}", e); + Ok(_) => {}, + Err(e) => { + error!("Failed to process chain data: {}", e); + return Err(Error::BlockChain(format!("Failed to process chain data: {}", e))); + } } } if let Err(e) = self.create_notification(&decoded.data, node_id).await { error!("Failed to create notification for bill event: {}", e); + return Err(Error::Notification(format!("Failed to create notification: {}", e))); } } else { warn!("Could not decode event to BillChainEventPayload {event:?}"); + return Err(Error::Deserialization("Could not decode event to BillChainEventPayload".to_string())); } Ok(()) } Apply this suggestion Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant issue where errors from critical operations are logged but not propagated, which could lead to silent failures. Properly propagating these errors would improve error handling and make debugging easier.	Medium
	Merge duplicate implementation blocks There's a duplicate implementation of the impl DefaultNotificationService block. The first implementation at line 29 contains the send_all_events method, while the second implementation at line 46 contains the new method. These should be merged into a single implementation block to avoid confusion and potential issues. crates/bcr-ebill-api/src/service/notification_service/default_service.rs [29-44] impl DefaultNotificationService { pub fn new( notification_transport: Box<dyn NotificationJsonTransportApi>, notification_store: Arc<dyn NotificationStoreApi>, contact_service: Arc<dyn ContactServiceApi>, ) -> Self { Self { notification_transport, notification_store, contact_service, } } + + async fn send_all_events(&self, events: Vec<Event<BillChainEventPayload>>) -> Result<()> { + for event_to_process in events.into_iter() { + if let Ok(Some(identity)) = self + .contact_service + .get_identity_by_node_id(&event_to_process.node_id) + .await + { + self.notification_transport + .send(&identity, event_to_process.try_into()?) + .await?; + } + } + Ok(()) + } } Apply this suggestion Suggestion importance[1-10]: 7 __ Why: The PR introduces two separate implementation blocks for DefaultNotificationService, which is confusing and violates the DRY principle. Merging them would improve code organization and maintainability.	Medium
	Distinguish between error types The function doesn't distinguish between a failed blockchain creation and an invalid blockchain. The catch-all _ pattern in the second match arm will capture both Err results from new_from_blocks and Ok(chain) where chain.is_chain_valid() is false. This makes debugging difficult and could mask different types of errors. crates/bcr-ebill-transport/src/handler/bill_chain_event_handler.rs [143-167] fn get_valid_chain( &self, blocks: Vec<BillBlock>, keys: &BillKeys, ) -> Result<(String, BillBlockchain)> { match BillBlockchain::new_from_blocks(blocks) { Ok(chain) if chain.is_chain_valid() => match chain.get_first_version_bill(keys) { Ok(bill) => Ok((bill.id, chain)), Err(e) => { error!( "Failed to get first version bill from newly received chain: {}", e ); Err(Error::Crypto(format!( "Failed to decrypt new bill chain with given keys: {e}" ))) } }, - _ => { + Ok(_) => { error!("Newly received chain is not valid"); Err(Error::BlockChain( "Newly received chain is not valid".to_string(), + )) + }, + Err(e) => { + error!("Failed to create blockchain from blocks: {}", e); + Err(Error::BlockChain( + format!("Failed to create blockchain from blocks: {}", e) )) } } } Apply this suggestion Suggestion importance[1-10]: 7 __ Why: This suggestion improves error handling by distinguishing between different failure modes in blockchain validation. The current catch-all pattern makes debugging difficult by masking different error types, while the improved version provides more specific error messages.	Medium
Security	Protect sensitive key data The BillChainEventPayload struct contains sensitive data in the keys field which might expose private keys. Consider implementing proper serialization/deserialization to ensure private keys are never transmitted over the network. crates/bcr-ebill-transport/src/event/bill_events.rs [13-21] #[derive(Serialize, Deserialize, Debug, Clone, Default)] pub struct BillChainEventPayload { pub event_type: BillEventType, pub bill_id: String, pub action_type: Option<ActionType>, pub sum: Option<u64>, + #[serde(skip_serializing_if = "Option::is_none")] pub keys: Option<BillKeys>, pub blocks: Vec<BillBlock>, } Apply this suggestion Suggestion importance[1-10]: 9 __ Why: Adding the skip_serializing_if attribute to the keys field is a critical security improvement that prevents potentially sensitive private key data from being unnecessarily transmitted over the network when the field is None.	High
General	Rename misleading parameter The parameter name bill is misleading as it's actually a BillChainEvent type, not a bill. This is inconsistent with other similar methods in the same implementation that use event as the parameter name. Rename the parameter to event for consistency and clarity. crates/bcr-ebill-api/src/service/notification_service/default_service.rs [142-153] -async fn send_bill_is_endorsed_event(&self, bill: &BillChainEvent) -> Result<()> { - let all_events = bill.generate_action_messages( +async fn send_bill_is_endorsed_event(&self, event: &BillChainEvent) -> Result<()> { + let all_events = event.generate_action_messages( HashMap::from_iter(vec![( - bill.bill.endorsee.as_ref().unwrap().node_id.clone(), + event.bill.endorsee.as_ref().unwrap().node_id.clone(), (BillEventType::BillEndorsed, ActionType::CheckBill), )]), None, None, ); self.send_all_events(all_events).await?; Ok(()) } Apply this suggestion Suggestion importance[1-10]: 5 __ Why: The parameter name 'bill' is inconsistent with other similar methods that use 'event' for the same type. Renaming it improves code consistency and readability, making the codebase easier to maintain.	Low
	Parameterize test function The create_test_event function uses a hardcoded string "node_id" which might not be appropriate for all test scenarios. Consider parameterizing the node_id to make the function more flexible for different test cases. crates/bcr-ebill-api/src/service/notification_service/test_utils.rs [89-95] -pub fn create_test_event(event_type: &BillEventType) -> Event<TestEventPayload> { +pub fn create_test_event(event_type: &BillEventType, node_id: &str) -> Event<TestEventPayload> { Event::new( EventType::Bill, - "node_id", + node_id, create_test_event_payload(event_type), ) } Apply this suggestion Suggestion importance[1-10]: 5 __ Why: Parameterizing the node_id in the create_test_event function makes the test utility more flexible and reusable across different test scenarios, improving test coverage and maintainability.	Low
Update

[tompro]

The Qodo Merge tool is amazing, it Pointe to exactly the things I would too.

[mtbitcr]

lgtm!

[mtbitcr]

in recourse (both not accept and not paid) we always ask recoursee to pay. Not to accept

[mtbitcr]

This will see mint admin. SO text can be New request to mint or smth similar

[mtbitcr]

also will be quote expired and quote rejected

[zupzup]

Very, very nice! 🙌 Looking forward to full block propagation with Nostr!