Retry and subscription fixes

[tompro]

📝 Description

Fixes invite delivery when on chain create all messages are undelivered. Completely removed NIP-17 time skew in favor of a limit query. More error logging for retry sending. Ensure published contact are updated.

Relates to #621

---

✅ 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).

---

📋 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?

[qodo-code-review]

Review Summary by Qodo

Improve message retry handling and remove time skew logic

🐞 Bug fix ✨ Enhancement

Walkthroughs

Description

• Remove NIP-17 time skew logic in favor of limit-based queries
• Add retry queuing for failed invite message deliveries
• Support legacy payload format deserialization in retry messages
• Improve retry identity resolution with fallback strategies
• Simplify contact publishing logic in WASM initialization

Diagram

flowchart LR
  A["Failed Invite Delivery"] -->|"Queue for Retry"| B["Retry Message Queue"]
  B -->|"Deserialize with Fallback"| C["EventEnvelope or Legacy Format"]
  C -->|"Resolve Identity"| D["Send Retry Message"]
  E["Remove Time Skew"] -->|"Use Limit Query"| F["Simplified Event Filtering"]

Loading

File Changes

1. crates/bcr-ebill-api/src/constants.rs ⚙️ Configuration changes +0/-1

Remove NIP-17 time skew constant

• Removed NOSTR_EVENT_TIME_SLACK constant (1 week time offset)
• Kept DEFAULT_INITIAL_SUBSCRIPTION_DELAY_SECONDS and NOSTR_MAX_RELAYS constants

crates/bcr-ebill-api/src/constants.rs

---

2. crates/bcr-ebill-transport/src/block_transport.rs Error handling +32/-5

Add retry queuing for failed invite deliveries

• Added error handling for company invite message delivery with retry queuing
• Added error handling for bill invite message delivery with retry queuing
• Convert messages to EventEnvelope before sending and encode with base58 for retry storage
• Import EventEnvelope type and bitcoin::base58 module

crates/bcr-ebill-transport/src/block_transport.rs

---

3. crates/bcr-ebill-transport/src/nostr.rs 🐞 Bug fix +3/-16

Remove time skew logic from event validation

• Removed NOSTR_EVENT_TIME_SLACK import and add_time_slack() function
• Simplified valid_time() function to skip time slack for encrypted messages
• Changed logic to use limit-based query instead of time offset

crates/bcr-ebill-transport/src/nostr.rs

---

View more (3)

4. crates/bcr-ebill-transport/src/nostr_transport.rs ✨ Enhancement +66/-11

Support legacy payload format and improve retry identity resolution

• Added LegacyBillEventMessage struct for backward compatibility deserialization
• Enhanced retry message deserialization with fallback to legacy format
• Added resolve_retry_identity() method with multiple fallback strategies
• Updated send_retry_private_message() to use new identity resolution method
• Changed message serialization to use EventEnvelope instead of raw event

crates/bcr-ebill-transport/src/nostr_transport.rs

---

5. crates/bcr-ebill-transport/src/transport_service.rs 🧪 Tests +150/-0

Add tests for retry message scenarios

• Added two new test cases for retry message handling
• Test for sending retry with NostrContact without trust level
• Test for sending retry with legacy payload format deserialization
• Added imports for HandshakeStatus, NostrContact, and TrustLevel

crates/bcr-ebill-transport/src/transport_service.rs

---

6. crates/bcr-ebill-wasm/src/lib.rs ✨ Enhancement +8/-36

Simplify contact publishing logic in WASM

• Simplified ensure_transport_contact_data_published() function logic
• Removed redundant contact resolution checks before publishing
• Changed to only log warnings on publish errors instead of checking existence first
• Applied simplification to both identity and company contact publishing

crates/bcr-ebill-wasm/src/lib.rs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (2) 📎 Requirement gaps (0)

1. by_node_id() error ignored ☑ 📘 Rule violation ⛯ Reliability

Description

resolve_retry_identity() silently ignores errors from nostr_contact_store.by_node_id(node_id)
and falls back without logging, reducing debuggability and potentially masking store failures. This
violates the requirement to handle conversion/addition failures explicitly (no silent suppression).

Code

crates/bcr-ebill-transport/src/nostr_transport.rs[R396-406]

+        if let Ok(Some(nostr)) = self.nostr_contact_store.by_node_id(node_id).await {
+            let relays = if nostr.relays.is_empty() {
+                self.nostr_relays.clone()
+            } else {
+                nostr.relays
+            };
+            return Some(BillParticipant::Anon(BillAnonParticipant {
+                node_id: node_id.to_owned(),
+                nostr_relays: relays,
+            }));
+        }

Evidence

PR Compliance ID 7 forbids silently ignoring failures. The new code only handles the Ok(Some(_))
path and provides no logging/handling for the Err(_) case from by_node_id, implicitly
suppressing that error.

crates/bcr-ebill-transport/src/nostr_transport.rs[396-406]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`resolve_retry_identity()` suppresses errors from `nostr_contact_store.by_node_id(node_id)` by only matching `Ok(Some(_))` and not logging/handling `Err(_)`, which violates the no-silent-error-suppression requirement.
## Issue Context
The code currently falls back to `self.nostr_relays` when it cannot find a stored contact, but store failures should be explicitly logged/handled so they can be diagnosed.
## Fix Focus Areas
- crates/bcr-ebill-transport/src/nostr_transport.rs[396-406]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Old DMs can replay 🐞 Bug ✓ Correctness

Description

valid_time() now ignores the since cutoff for EncryptedDirectMessage/GiftWrap, while the DM
subscription has no since and only limit(1000). This can cause processing of very old private
messages (e.g., bill/company invites) after offset-store reset or first run, triggering expensive
chain resolution and side effects.

Code

crates/bcr-ebill-transport/src/nostr.rs[R1078-1084]

fn valid_time(kind: Kind, created: nostr::Timestamp, since: Option<Timestamp>) -> bool {
   match since {
-        Some(ts) => {
-            let time = if matches!(kind, Kind::EncryptedDirectMessage | Kind::GiftWrap) {
-                add_time_slack(ts)
-            } else {
-                ts
-            };
+        Some(time) if !matches!(kind, Kind::EncryptedDirectMessage | Kind::GiftWrap) => {
           created.as_u64() >= time.inner()
       }
-        None => true,
+        _ => true,
   }

Evidence

DMs/GiftWraps are subscribed without since (so the relay can return any age of events, capped only
by 1000), and should_process passes a since value that valid_time explicitly does not apply to
DMs anymore. Invite handlers can perform heavy stateful work (chain resolution + processing), so
replaying old DMs is not just log noise.

crates/bcr-ebill-transport/src/nostr.rs[861-867]
crates/bcr-ebill-transport/src/nostr.rs[906-915]
crates/bcr-ebill-transport/src/nostr.rs[1078-1084]
crates/bcr-ebill-transport/src/handler/bill_invite_handler.rs[45-76]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`valid_time()` now treats EncryptedDirectMessage/GiftWrap as always valid regardless of `since`, while the DM subscription uses `limit(1000)` with no `since`. This combination can cause processing/reprocessing of very old DMs (invites/contact shares) when the offset store is empty/reset, which can trigger expensive chain resolution and unintended side effects.
## Issue Context
The consumer already computes an `earliest_offset` and passes it into `should_process(...)`, but that value is no longer enforced for DMs.
## Fix Focus Areas
- crates/bcr-ebill-transport/src/nostr.rs[861-887]
- crates/bcr-ebill-transport/src/nostr.rs[1078-1084]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. publish_contact() missing precheck 📘 Rule violation ⛯ Reliability

Description

ensure_transport_contact_data_published() now publishes identity/company contacts unconditionally
(subject only to time-based rate limiting), which removes an explicit validation step that
previously prevented unnecessary/duplicate updates. This can cause repeated state/config updates and
extra network traffic, violating the requirement to validate updates and prevent duplicates where
applicable.

Code

crates/bcr-ebill-wasm/src/lib.rs[R150-165]

+    if let Ok(full_identity) = ctx.identity_service.get_full_identity().await
+        && let Err(e) = ctx
+            .identity_service
+            .publish_contact(&full_identity.identity, &full_identity.key_pair)
           .await
-        {
-            Ok(None) => {
-                if let Err(e) = ctx
-                    .identity_service
-                    .publish_contact(&full_identity.identity, &full_identity.key_pair)
-                    .await
-                {
-                    warn!("Could not publish identity details to Nostr: {e}")
-                }
-            }
-            Ok(Some(_)) => (),
-            Err(e) => {
-                warn!("Could not resolve personal identity details on Nostr: {e}")
-            }
-        }
+    {
+        warn!("Could not publish identity details to Nostr: {e}")
   }

   if let Ok(companies) = ctx.company_service.get_list_of_companies().await {
       for c in companies.iter() {
           if let Ok((company, keys)) = ctx.company_service.get_company_and_keys_by_id(&c.id).await
+                && let Err(e) = ctx.company_service.publish_contact(&company, &keys).await
           {
-                match ctx
-                    .transport_service
-                    .contact_transport()
-                    .resolve_contact(&company.id)
-                    .await
-                {
-                    Ok(None) => {
-                        if let Err(e) = ctx.company_service.publish_contact(&company, &keys).await {
-                            warn!("Could not publish company details to Nostr: {e}")
-                        }
-                    }
-                    Ok(Some(_)) => (),
-                    Err(e) => {
-                        warn!("Could not resolve company details on Nostr: {e}")
-                    }
-                }
+                warn!("Could not publish company details to Nostr: {e}")
           }

Evidence

PR Compliance ID 8 requires explicit validation during state/config updates (including preventing
duplicates). The updated code directly calls publish_contact(...) for the user and each company
without any existence/duplication check before updating remote state.

crates/bcr-ebill-wasm/src/lib.rs[150-165]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ensure_transport_contact_data_published()` publishes contact data unconditionally, which removes a validation/duplicate-prevention step and can cause repeated updates and extra network calls.
## Issue Context
Compliance requires explicit validations during state/config updates (e.g., preventing duplicates). If `publish_contact` is not strictly idempotent, repeated publishes can create duplicates or unnecessary remote changes.
## Fix Focus Areas
- crates/bcr-ebill-wasm/src/lib.rs[150-165]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Retry sends to banned ☑ 🐞 Bug ⛨ Security

Description

resolve_retry_identity() will construct a recipient from nostr_contact_store.by_node_id()
without filtering out TrustLevel::Banned. This can result in sending queued private messages to
contacts explicitly marked as banned, which contradicts other codepaths that exclude banned contacts
from Nostr interaction.

Code

crates/bcr-ebill-transport/src/nostr_transport.rs[R396-406]

+        if let Ok(Some(nostr)) = self.nostr_contact_store.by_node_id(node_id).await {
+            let relays = if nostr.relays.is_empty() {
+                self.nostr_relays.clone()
+            } else {
+                nostr.relays
+            };
+            return Some(BillParticipant::Anon(BillAnonParticipant {
+                node_id: node_id.to_owned(),
+                nostr_relays: relays,
+            }));
+        }

Evidence

The retry identity resolution does not inspect trust_level at all, so banned contacts can still be
selected as recipients for retries. In contrast, relay-set calculation explicitly filters eligible
contacts to only Trusted/Participant, and the core model defines Banned as an active ban from
communication.

crates/bcr-ebill-transport/src/nostr_transport.rs[396-406]
crates/bcr-ebill-core/src/application/nostr_contact.rs[99-111]
crates/bcr-ebill-transport/src/nostr.rs[1580-1584]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Retry delivery can currently select a recipient from `nostr_contact_store.by_node_id()` without checking trust level, which allows sending queued private messages to contacts marked `Banned`.
## Issue Context
Other parts of the system (relay set calculation) exclude banned contacts, and `TrustLevel::Banned` is documented as an active ban.
## Fix Focus Areas
- crates/bcr-ebill-transport/src/nostr_transport.rs[387-416]
- crates/bcr-ebill-transport/src/nostr.rs[1580-1584]
- crates/bcr-ebill-core/src/application/nostr_contact.rs[99-111]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[Copilot]

Pull request overview

This PR improves Nostr delivery reliability by queuing failed private invite/block notifications for retry, adjusts retry logic to resolve recipients more flexibly, and removes the prior NIP-17 DM time-slack approach in favor of limit-based DM subscriptions.

Changes:

• Queue failed private company/bill invite sends (and bill notifications) into the retry queue with a consistent EventEnvelope payload format.
• Add retry support for legacy queued private payloads and expand retry recipient resolution to use Nostr contacts even without trust.
• Remove the Nostr event time-slack constant and simplify time filtering logic.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
crates/bcr-ebill-wasm/src/lib.rs	Simplifies/changes contact publishing behavior during transport startup/reconnect.
crates/bcr-ebill-transport/src/transport_service.rs	Adds tests covering retry behavior (nostr contact without trust + legacy payload format).
crates/bcr-ebill-transport/src/nostr_transport.rs	Updates retry payload format to EventEnvelope, supports legacy decode, and adds resolve_retry_identity.
crates/bcr-ebill-transport/src/nostr.rs	Removes time-slack logic and simplifies valid_time filtering.
crates/bcr-ebill-transport/src/block_transport.rs	Queues failed private invite sends for retry (instead of failing the call).
crates/bcr-ebill-api/src/constants.rs	Removes NOSTR_EVENT_TIME_SLACK constant.

[codecov]

Codecov Report

❌ Patch coverage is 64.28571% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.48%. Comparing base (6f9bd5b) to head (a285c85).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
crates/bcr-ebill-wasm/src/lib.rs	0.00%	7 Missing ⚠️
crates/bcr-ebill-transport/src/nostr.rs	0.00%	2 Missing ⚠️
crates/bcr-ebill-transport/src/nostr_transport.rs	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #836      +/-   ##
==========================================
+ Coverage   70.39%   70.48%   +0.08%     
==========================================
  Files         132      132              
  Lines       24836    24821      -15     
==========================================
+ Hits        17484    17494      +10     
+ Misses       7352     7327      -25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[zupzup]

LGTM 👍