-
Notifications
You must be signed in to change notification settings - Fork 5
.pr_agent_accepted_suggestions
| PR 757 (2025-12-08) |
[possible issue] Avoid data loss on single failure
✅ Avoid data loss on single failure
In get_all, use filter_map instead of map and collect::<Option<...>> to prevent a single contact conversion failure from discarding the entire list of contacts. Log errors for failed conversions.
crates/bcr-ebill-persistence/src/db/nostr_contact_store.rs [69-76]
async fn get_all(&self) -> Result<Vec<NostrContact>> {
let result: Vec<NostrContactDb> = self.db.select_all(Self::TABLE).await?;
let values = result
.into_iter()
- .map(|c| c.to_owned().try_into().ok())
- .collect::<Option<Vec<NostrContact>>>();
- Ok(values.unwrap_or_default())
+ .filter_map(|c| match c.try_into() {
+ Ok(contact) => Some(contact),
+ Err(e) => {
+ log::error!("Failed to convert NostrContactDb to NostrContact: {}", e);
+ None
+ }
+ })
+ .collect();
+ Ok(values)
}Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a critical flaw where a single malformed contact record would cause all contacts to be discarded, impacting the new relay connection logic. The proposed fix using filter_map is robust and appropriate.
| PR 643 (2025-09-16) |
[learned best practice] Early-return on HTTP errors
✅ Early-return on HTTP errors
Short-circuit on non-success HTTP statuses before attempting to read or interpret the body to avoid misusing error responses as content.
crates/bcr-ebill-api/src/external/identity_proof.rs [109-143]
match self.cl.post(proxy_url).json(&proxy_req).send().await {
Ok(res) => {
let status = res.status();
+ if status.is_client_error() {
+ let body = res.text().await.unwrap_or_default();
+ error!("Error checking url: {url} for identity proof: {status}, {body}");
+ return IdentityProofStatus::FailureClient;
+ }
+ if status.is_server_error() {
+ let body = res.text().await.unwrap_or_default();
+ error!("Error checking url: {url} for identity proof: {status}, {body}");
+ return IdentityProofStatus::FailureServer;
+ }
match res.text().await {
Ok(body) => {
- if status.is_client_error() {
- error!(
- "Error checking url: {url} for identity proof: {status}, {body}"
- );
- return IdentityProofStatus::FailureClient;
- } else if status.is_server_error() {
- error!(
- "Error checking url: {url} for identity proof: {status}, {body}"
- );
- return IdentityProofStatus::FailureServer;
- }
-
- // Check if the identity proof is contained in the response
if identity_proof.is_contained_in(&body) {
IdentityProofStatus::Success
} else {
IdentityProofStatus::NotFound
}
}
Err(body_err) => {
error!("Error checking url: {url} for identity proof: {body_err}");
IdentityProofStatus::FailureClient
}
}
}
Err(req_err) => {
error!("Error checking url: {url} for identity proof: {req_err}");
IdentityProofStatus::FailureConnect
}
}Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Ensure early returns on failure paths to guard control flow and avoid continuing with invalid states.
| PR 620 (2025-09-04) |
[general] Improve resync error handling
✅ Improve resync error handling
The resync operation should have proper error handling and recovery mechanisms. Currently, if the first chain data fails to sync, it continues to the next without logging the specific error, potentially masking important sync failures.
crates/bcr-ebill-transport/src/handler/bill_chain_event_processor.rs [92-142]
async fn resync_chain(&self, bill_id: &BillId) -> Result<()> {
match (
self.bill_blockchain_store.get_chain(bill_id).await,
self.bill_store.get_keys(bill_id).await,
) {
(Ok(existing_chain), Ok(bill_keys)) => {
debug!("starting bill chain resync for {bill_id}");
let bcr_keys = BcrKeys::from_private_key(&bill_keys.private_key)?;
- if let Ok(chain_data) = resolve_event_chains(
+ match resolve_event_chains(
self.transport.clone(),
&bill_id.to_string(),
BlockchainType::Bill,
&bcr_keys,
)
.await
{
- for data in chain_data.iter() {
- let blocks: Vec<BillBlock> = data
- .iter()
- .filter_map(|d| match d.block.clone() {
- BlockData::Bill(block) => Some(block),
- _ => None,
- })
- .collect();
- if !data.is_empty()
- && self
- .add_bill_blocks(bill_id, existing_chain.clone(), blocks)
- .await
- .is_ok()
- {
- debug!("resynced bill {bill_id} with {} remote events", data.len());
- break;
+ Ok(chain_data) => {
+ let mut synced = false;
+ for data in chain_data.iter() {
+ let blocks: Vec<BillBlock> = data
+ .iter()
+ .filter_map(|d| match d.block.clone() {
+ BlockData::Bill(block) => Some(block),
+ _ => None,
+ })
+ .collect();
+ if !data.is_empty() {
+ match self.add_bill_blocks(bill_id, existing_chain.clone(), blocks).await {
+ Ok(_) => {
+ debug!("resynced bill {bill_id} with {} remote events", data.len());
+ synced = true;
+ break;
+ }
+ Err(e) => {
+ warn!("Failed to sync chain data for {bill_id}: {e}");
+ continue;
+ }
+ }
+ }
+ }
+ if synced {
+ debug!("finished bill chain resync for {bill_id}");
+ Ok(())
+ } else {
+ let message = format!("No valid chain data could be synced for {bill_id}");
+ error!("{message}");
+ Err(Error::Network(message))
}
}
- debug!("finished bill chain resync for {bill_id}");
- Ok(())
- } else {
- let message = format!("Could not refetch chain data from Nostr for {bill_id}");
- error!("{message}");
- Err(Error::Network(message))
+ Err(e) => {
+ let message = format!("Could not refetch chain data from Nostr for {bill_id}: {e}");
+ error!("{message}");
+ Err(Error::Network(message))
+ }
}
}
_ => {
let message = format!(
"Could not refetch chain for {bill_id} because the bill keys or chain could not be fetched"
);
error!("{message}");
Err(Error::Persistence(message))
}
}
}Suggestion importance[1-10]: 7
__
Why: The suggestion correctly points out that the loop for syncing chain data silently ignores errors. The proposed change improves robustness by logging failures and ensuring the function errors out if no chain can be successfully synced.
| PR 599 (2025-07-21) |
[general] Prevent duplicate signatory additions
✅ Prevent duplicate signatory additions
The AddSignatory operation doesn't check for duplicate signatories before adding. This could lead to the same signatory being added multiple times, which may cause inconsistent state or unexpected behavior in the company management system.
crates/bcr-ebill-core/src/company.rs [71-73]
-pub fn apply_block_data(&mut self, data: &CompanyBlockPayload) {
- match data {
- CompanyBlockPayload::Update(payload) => {
- self.name = payload.name.to_owned().unwrap_or(self.name.to_owned());
- self.email = payload.email.to_owned().unwrap_or(self.email.to_owned());
- ...
- }
- CompanyBlockPayload::AddSignatory(payload) => {
- self.signatories.push(payload.signatory.to_owned());
- }
- CompanyBlockPayload::RemoveSignatory(payload) => {
- self.signatories.retain(|i| i != &payload.signatory);
- }
- _ => {}
+CompanyBlockPayload::AddSignatory(payload) => {
+ if !self.signatories.contains(&payload.signatory) {
+ self.signatories.push(payload.signatory.to_owned());
}
}Suggestion importance[1-10]: 7
__
Why: The suggestion correctly points out that adding a signatory does not check for duplicates, which could lead to data integrity issues. Adding this check improves the robustness of the state update logic.
| PR 584 (2025-07-11) |
[general] Fix malformed comment with URL
Fix malformed comment with URL
The comment contains a malformed URL that appears to be accidentally concatenated with the text. This creates a confusing and unprofessional comment that should be properly formatted.
crates/bcr-ebill-transport/src/transport.rs [30]
-// that we will not be able to extract a chain even if there arehttps://github.com/BitcreditProtocol/Bitcredit-Core/issues/583 valid blocks on the relay.
+// that we will not be able to extract a chain even if there are valid blocks on the relay.Suggestion importance[1-10]: 3
__
Why: The suggestion correctly identifies that the PR introduced a malformed comment by accidentally pasting a URL into it, and the proposed fix correctly removes the erroneous URL.
| PR 458 (2025-04-01) |
[possible issue] Handle missing RequestToPay block
✅ Handle missing RequestToPay block
The code is missing a check for when there's no RequestToPay block. In that case, the function continues execution but may encounter issues later. You should add an early return when no RequestToPay block exists, as payment checking is only relevant when a payment has been requested.
crates/bcr-ebill-api/src/service/bill_service/payment.rs [37-50]
if let Some(req_to_pay) =
chain.get_last_version_block_with_op_code(BillOpCode::RequestToPay)
{
let deadline_base = get_deadline_base_for_req_to_pay(req_to_pay, &bill)?;
// deadline has expired - don't need to check payment
if util::date::check_if_deadline_has_passed(
deadline_base,
now,
PAYMENT_DEADLINE_SECONDS,
) {
info!("Payment deadline for bill {bill_id} expired - not checking");
return Ok(());
}
+} else {
+ // No RequestToPay block exists, so no payment to check
+ info!("No payment request found for bill {bill_id} - not checking");
+ return Ok(());
}Suggestion importance[1-10]: 8
__
Why: This suggestion addresses an important edge case where no RequestToPay block exists. Adding an early return prevents potential issues later in the code execution, improving robustness and error handling.
[general] Remove unnecessary parentheses
✅ Remove unnecessary parentheses
The parentheses around timestamp are unnecessary and could be confusing as they might suggest a more complex expression. Remove them to improve code clarity.
crates/bcr-ebill-core/src/bill/validation.rs [102-107]
// maturity date must have started
let maturity_date_start =
util::date::date_string_to_timestamp(&bill.maturity_date, None)?;
-if (timestamp) < maturity_date_start {
+if timestamp < maturity_date_start {
return Err(ValidationError::BillRequestedToPayBeforeMaturityDate);
}Suggestion importance[1-10]: 2
__
Why: This is a minor code style improvement that removes unnecessary parentheses around the timestamp variable. While it improves code clarity slightly, it doesn't affect functionality or fix any actual bugs.
[possible issue] Missing return after deadline check
✅ Missing return after deadline check
The function is missing a return statement after detecting that the payment deadline has expired. This could lead to unnecessary processing of expired payment requests.
crates/bcr-ebill-api/src/service/bill_service/payment.rs [42-48]
if util::date::check_if_deadline_has_passed(
deadline_base,
now,
PAYMENT_DEADLINE_SECONDS,
) {
info!("Payment deadline for bill {bill_id} expired");
+ return Ok(());
}Suggestion importance[1-10]: 8
__
Why: The function continues processing even after detecting an expired payment deadline, which could lead to unnecessary operations. Adding a return statement would prevent this inefficiency and potential issues.
| PR 448 (2025-03-20) |
[general] Replace println with logging
✅ Replace println with logging
Using println! in production code is not recommended. This debug statement should be replaced with a proper logging statement using the log crate that's already imported and used elsewhere in the file.
crates/bcr-ebill-api/src/service/notification_service/default_service.rs [411]
-println!("got next retry message {q}", q = queued_message.id);
+log::debug!("Processing retry message with id: {}", queued_message.id);Suggestion importance[1-10]: 5
__
Why: Using println! in production code is not recommended as it doesn't respect log levels and can't be filtered or redirected. Replacing it with log::debug! is more consistent with the rest of the codebase and follows better logging practices.
| PR 444 (2025-03-17) |
[possible issue] Check block addition success
✅ Check block addition success
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, █).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 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.
| PR 435 (2025-03-07) |
[possible issue] Fix method call name
✅ Fix method call name
Update the function to use the correct method name when calling "lastest_block()" (should be "latest_block()") to ensure consistency with the function definition.
crates/bcr-ebill-transport/src/event/chain_event.rs [42-48]
fn get_blocks_for_node(&self, node_id: &str) -> Vec<BillBlock> {
match self.participants.get(node_id) {
Some(height) if *height == self.chain.block_height() => self.chain.blocks().clone(),
- Some(_) => vec![self.lastest_block()],
+ Some(_) => vec![self.latest_block()],
None => Vec::new(),
}
}Suggestion importance[1-10]: 8
__
Why: This suggestion fixes a consistency issue where the method call uses the misspelled name "lastest_block()" which needs to match the function definition. This is important for code correctness and would prevent potential runtime errors.
[general] Fix function name typo
✅ Fix function name typo
Fix the typo in the function name from "lastest_block" to "latest_block" for better code readability and consistency.
crates/bcr-ebill-transport/src/event/chain_event.rs [35-38]
// Returns the latest block in the chain.
-fn lastest_block(&self) -> BillBlock {
+fn latest_block(&self) -> BillBlock {
self.chain.get_latest_block().clone()
}Suggestion importance[1-10]: 7
__
Why: The function name has a typo ("lastest_block" instead of "latest_block"), which affects code readability and could cause confusion. Fixing this improves code quality and maintainability.
| PR 431 (2025-03-05) |
[security] Missing identity validation
✅ Missing identity validation
The active method doesn't validate if the identity exists before returning it. If the personal identity node ID is invalid or doesn't exist in the system, this could lead to returning an invalid identity. Add a check to verify the identity exists before returning it.
crates/bcr-ebill-pwa/src/api/identity.rs [27-41]
#[wasm_bindgen]
pub async fn active(&self) -> Result<JsValue> {
let current_identity = get_current_identity();
+
+ // Verify that the identity exists
+ if !get_ctx().identity_service.identity_exists().await {
+ return Err(Error::NotFound.into());
+ }
+
let (node_id, t) = match current_identity.company {
None => (current_identity.personal, IdentityType::Person),
Some(company_node_id) => (company_node_id, IdentityType::Company),
};
let switch_identity = SwitchIdentity {
t: Some(t.into_web()),
node_id,
};
let res = serde_wasm_bindgen::to_value(&switch_identity)?;
Ok(res)
}Suggestion importance[1-10]: 8
__
Why: The method doesn't verify if the identity exists before returning it, which could lead to returning invalid identity information. This is a security concern that could affect authentication.
[security] Use testnet for development
✅ Use testnet for development
The configuration contains a hardcoded mainnet Bitcoin network setting, which is dangerous for testing and development. Using mainnet in non-production environments could lead to real financial transactions and potential loss of funds.
crates/bcr-ebill-pwa/main.js [3-12]
let config = {
- bitcoin_network: "mainnet",
- // nostr_relay: "wss://bitcr-cloud-run-04-550030097098.europe-west1.run.app".to_string(),
+ bitcoin_network: process.env.NODE_ENV === 'production' ? "mainnet" : "testnet",
nostr_relay: "wss://bitcr-cloud-run-03-550030097098.europe-west1.run.app",
- // surreal_db_connection: "ws://localhost:8800",
surreal_db_connection: "indxdb://default",
data_dir: ".",
job_runner_initial_delay_seconds: 1,
job_runner_check_interval_seconds: 600,
};Suggestion importance[1-10]: 7
__
Why: The suggestion addresses an important security concern about using Bitcoin mainnet in non-production environments, which could lead to real financial losses. However, like the previous suggestion, it relies on process.env.NODE_ENV which may not be available in this context.
| PR 415 (2025-02-19) |
[security] Fix incorrect key combination order
✅ Fix incorrect key combination order
The get_combined_bitcoin_key function is called with incorrect argument order - the caller's key should be passed after the bill key, not before. This could cause incorrect key combination and potential security issues.
crates/bcr-ebill-api/src/service/bill_service.rs [1850-1854]
let private_key = self.bitcoin_client.get_combined_private_key(
- &caller_keys.get_bitcoin_private_key(get_config().bitcoin_network()),
&BcrKeys::from_private_key(&bill_keys.private_key)?
.get_bitcoin_private_key(get_config().bitcoin_network()),
+ &caller_keys.get_bitcoin_private_key(get_config().bitcoin_network()),
)?;Suggestion importance[1-10]: 9
__
Why: Incorrect order of arguments in key combination could lead to serious security vulnerabilities by generating wrong combined keys. This is a critical security fix.