-
Notifications
You must be signed in to change notification settings - Fork 5
.pr_agent_auto_best_practices
Pattern 1: Early-return on invalid or irrelevant states before performing further processing, such as non-2xx HTTP responses or missing prerequisite domain blocks, to avoid misinterpreting data and wasting work.
Example code before:
let res = client.get(url).send().await?;
let status = res.status();
let body = res.text().await?; // body read even on 4xx/5xx
process(body)?;
if deadline_passed(now) {
log::info!("Expired");
// continues processing
}
Example code after:
let res = client.get(url).send().await?;
let status = res.status();
if status.is_client_error() || status.is_server_error() {
let body = res.text().await.unwrap_or_default();
log::error!("Request failed: {status} {body}");
return Err(Error::RequestFailed);
}
let body = res.text().await?;
process(body)?;
if deadline_passed(now) {
log::info!("Expired");
return Ok(());
}
Relevant past accepted suggestions:
Suggestion 1:
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 2:
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 3:
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(());
}Pattern 2: Validate and act on operation outcomes explicitly by checking return values, handling errors per-iteration, and logging context (e.g., add_block success, resync steps) instead of ignoring results or continuing silently.
Example code before:
for item in items {
store.try_add(item); // boolean ignored
// continue even if add failed
}
let chains = resolve_chains().await?;
for c in chains { apply(c).await.ok(); } // errors swallowed
Example code after:
for item in items {
let added = store.try_add(item.clone());
if !added {
log::error!("Failed to add item {}", item.id());
return Err(Error::AddFailed);
}
}
match resolve_chains().await {
Ok(chains) => {
let mut synced = false;
for c in chains {
match apply(c.clone()).await {
Ok(_) => { synced = true; break; }
Err(e) => { log::warn!("Apply failed: {e}"); continue; }
}
}
if !synced { return Err(Error::NoValidChain); }
}
Err(e) => {
log::error!("Resolve failed: {e}");
return Err(Error::Network(e.to_string()));
}
}
Relevant past accepted suggestions:
Suggestion 1:
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 2:
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(),
))
}
}[Auto-generated best practices - 2025-12-03]