Implement transaction watcher timer and follow-up scan logic#690
Open
kartikangiras wants to merge 1 commit intobitcoinppl:masterfrom
Open
Implement transaction watcher timer and follow-up scan logic#690kartikangiras wants to merge 1 commit intobitcoinppl:masterfrom
kartikangiras wants to merge 1 commit intobitcoinppl:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Greptile SummaryThis PR refactors the transaction polling loop in
Confidence Score: 4/5Safe to merge with awareness of the coalesced-timer and watcher-leak edge cases, both of which only manifest when multiple transactions are confirmed in close succession or when a scan errors out. Both findings are P2: they affect uncommon edge cases (concurrent confirms, error paths) and are not regressions that break the primary flow. The core refactor is structurally sound and improves actor hygiene. Score is 4 rather than 5 because the timer-coalescing issue is a correctness deviation from the stated intent of the 30-second delay. rust/src/manager/wallet_manager/actor.rs (shared timer logic), rust/src/transaction_watcher.rs (watcher cleanup on error path) Important Files Changed
Sequence DiagramsequenceDiagram
participant TW as TransactionWatcher
participant WA as WalletActor
participant Timer_TW as TW Timer
participant Timer_WA as WA scan_timer
TW->>TW: start_watching() — build client, store in self.client
TW->>Timer_TW: set_timeout(Duration::ZERO)
Timer_TW-->>TW: tick()
TW->>TW: check_txn()
alt Transaction not found
TW->>Timer_TW: set_timeout(normal_wait_time)
Timer_TW-->>TW: tick() [repeat]
else Error
TW->>Timer_TW: set_timeout(30s)
else Transaction found
TW->>WA: send!(mark_transaction_found(tx_id))
TW->>Timer_TW: clear()
WA->>WA: update_block_id()
WA->>WA: perform_scan_for_single_tx_id(tx_id)
WA->>WA: pending_follow_up_scans.push(tx_id)
alt Timer not yet armed
WA->>Timer_WA: set_timeout_for_weak(30s)
WA->>WA: pending_scan_timer_armed = true
end
Timer_WA-->>WA: tick()
WA->>WA: pending_scan_timer_armed = false
loop for each tx_id in pending_follow_up_scans
WA->>WA: send!(perform_scan_for_single_tx_id)
WA->>WA: send!(remove_watcher_for_txn)
end
end
Reviews (1): Last reviewed commit: "Implement transaction watcher timer and ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
rust/src/transaction_watcher.rs (2)
60-64: Collapse duplicate match arms.
BitcoinandTestnet | Testnet4map to the sameDuration::from_secs(20).♻️ Proposed simplification
normal_wait_time: match network { - Network::Bitcoin => Duration::from_secs(20), - Network::Testnet | Network::Testnet4 => Duration::from_secs(20), + Network::Bitcoin | Network::Testnet | Network::Testnet4 => { + Duration::from_secs(20) + } Network::Signet => Duration::from_secs(10), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/transaction_watcher.rs` around lines 60 - 64, Collapse the duplicate match arms in the normal_wait_time match by combining Network::Bitcoin, Network::Testnet and Network::Testnet4 into a single pattern (e.g. Network::Bitcoin | Network::Testnet | Network::Testnet4 => Duration::from_secs(20)) and keep Network::Signet => Duration::from_secs(10); update the match in the normal_wait_time assignment in transaction_watcher.rs so it uses the combined pattern and Duration::from_secs references shown.
79-90: Prefer a concrete error type overBox<dyn std::error::Error>.The rest of the crate uses
eyre::Result/ typed errors;Box<dyn std::error::Error>here is less idiomatic and hasSendcaveats when crossing await points. Since the only failing call isget_confirmed_transaction,eyre::Result<WatchResult>(or the underlyingnode::client::Error) would be cleaner and consistent withactor.rs.♻️ Proposed change
- async fn check_txn(&mut self) -> Result<WatchResult, Box<dyn std::error::Error>> { + async fn check_txn(&mut self) -> eyre::Result<WatchResult> { let Some(client) = self.client.clone() else { error!("transaction watcher client not initialized"); return Ok(WatchResult::Continue); }; let txn = client.get_confirmed_transaction(self.tx_id.clone()).await?; match txn { Some(txn) => Ok(WatchResult::Found(txn)), None => Ok(WatchResult::Continue), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/transaction_watcher.rs` around lines 79 - 90, The function check_txn currently returns Result<WatchResult, Box<dyn std::error::Error>> which is non-idiomatic for this crate and can cause Send/cross-await issues; change its signature to use eyre::Result<WatchResult> (or the concrete node::client::Error if you prefer) and update the function to propagate errors with ? as before from client.get_confirmed_transaction(self.tx_id.clone()).await; ensure you add the appropriate use/import for eyre::Result and keep the same handling of the None/Some match that returns WatchResult::Continue/Found so callers (and actor.rs patterns) stay consistent.rust/src/manager/wallet_manager/actor.rs (1)
944-962: Comment and early-return path.Minor: the updated doc comment on line 953 ("wait 30 seconds…") is accurate only for the first queued txid in a batch; other txids may wait much less. Consider rephrasing to "batch a follow-up scan within the next 30 seconds, then remove watcher(s)."
Also, if
update_block_idor the immediateperform_scan_for_single_tx_idon lines 948–951 returnsErr, the txid is never enqueued and the watcher is never removed — the watcher will keep polling until it finds the txn again. That's pre-existing behavior, but worth a glance whether enqueueing before the immediate scan would be safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager/actor.rs` around lines 944 - 962, Rename the doc comment in mark_transaction_found to say something like "batch a follow-up scan within the next 30 seconds, then remove watcher(s)" to avoid implying every tx waits 30s; and to avoid leaving a watcher enqueued on transient scan errors, push the tx_id onto pending_follow_up_scans and arm the timer (use pending_follow_up_scans.push(tx_id) and set pending_scan_timer_armed/scan_timer.set_timeout_for_weak(self.addr.clone(), Duration::from_secs(30))) before calling update_block_id() and perform_scan_for_single_tx_id(tx_id), so that even if update_block_id or perform_scan_for_single_tx_id returns Err the tx has been enqueued and the watcher will be removed by the follow-up scan logic; ensure you still return Err on failure of the immediate scan after enqueueing so existing error handling remains intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/manager/wallet_manager/actor.rs`:
- Around line 166-182: The tick implementation on WalletActor shortens per-tx
follow-up delay and allows duplicate txids; to fix, (1) decide whether batched
timing is acceptable — if not, switch from a single scan_timer to per-tx
scheduling in mark_transaction_found (e.g., spawn a per-tx delay task or store
per-tx deadlines) so each txid gets ~30s from its mark, otherwise preserve
current batched behavior but document it; and (2) make pending_follow_up_scans a
HashSet<Txid> (replace Vec<Txid> with HashSet and update mark_transaction_found
to insert) and in tick use drain() or drain_filter to consume the set and call
addr.perform_scan_for_single_tx_id(tx_id) and addr.remove_watcher_for_txn(tx_id)
once per unique txid to eliminate duplicates (update types and the loop in tick
accordingly).
In `@rust/src/transaction_watcher.rs`:
- Around line 68-77: The start_watching() path can return early when
client_builder.build().await? fails leaving self.client None and never arming
the timer; modify start_watching (and/or the error path) so that when
client_builder.build() returns Err you still schedule a retry: catch the error
instead of using ? in start_watching, log it, set self.client = None if
necessary, and call self.timer.set_timeout_for_weak(self.addr.clone(),
retry_duration) (or re-send start_watching to self.addr) so the watcher will
retry; also update tick to detect self.client.is_none() and call
start_watching() to restart the connect attempt.
---
Nitpick comments:
In `@rust/src/manager/wallet_manager/actor.rs`:
- Around line 944-962: Rename the doc comment in mark_transaction_found to say
something like "batch a follow-up scan within the next 30 seconds, then remove
watcher(s)" to avoid implying every tx waits 30s; and to avoid leaving a watcher
enqueued on transient scan errors, push the tx_id onto pending_follow_up_scans
and arm the timer (use pending_follow_up_scans.push(tx_id) and set
pending_scan_timer_armed/scan_timer.set_timeout_for_weak(self.addr.clone(),
Duration::from_secs(30))) before calling update_block_id() and
perform_scan_for_single_tx_id(tx_id), so that even if update_block_id or
perform_scan_for_single_tx_id returns Err the tx has been enqueued and the
watcher will be removed by the follow-up scan logic; ensure you still return Err
on failure of the immediate scan after enqueueing so existing error handling
remains intact.
In `@rust/src/transaction_watcher.rs`:
- Around line 60-64: Collapse the duplicate match arms in the normal_wait_time
match by combining Network::Bitcoin, Network::Testnet and Network::Testnet4 into
a single pattern (e.g. Network::Bitcoin | Network::Testnet | Network::Testnet4
=> Duration::from_secs(20)) and keep Network::Signet => Duration::from_secs(10);
update the match in the normal_wait_time assignment in transaction_watcher.rs so
it uses the combined pattern and Duration::from_secs references shown.
- Around line 79-90: The function check_txn currently returns
Result<WatchResult, Box<dyn std::error::Error>> which is non-idiomatic for this
crate and can cause Send/cross-await issues; change its signature to use
eyre::Result<WatchResult> (or the concrete node::client::Error if you prefer)
and update the function to propagate errors with ? as before from
client.get_confirmed_transaction(self.tx_id.clone()).await; ensure you add the
appropriate use/import for eyre::Result and keep the same handling of the
None/Some match that returns WatchResult::Continue/Found so callers (and
actor.rs patterns) stay consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 618bb37e-8172-4431-8046-eb7826550201
📒 Files selected for processing (2)
rust/src/manager/wallet_manager/actor.rsrust/src/transaction_watcher.rs
praveenperera
requested changes
Apr 20, 2026
kartikangiras
force-pushed
the
ticktrait
branch
from
6013ad1 to
1fc6a2e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/manager/wallet_manager/actor.rs (1)
947-967:⚠️ Potential issue | 🟡 MinorWatcher is leaked if pre-queue steps fail.
pending_scans.pending_txids.push(tx_id)(line 957) only runs after the two?awaits onupdate_block_id()andperform_scan_for_single_tx_id()(lines 951, 954). If either returnsErr, the function returns early,tx_idis never queued, andremove_watcher_for_txnis never sent — theTransactionWatcherfortx_idstays inself.transaction_watchersindefinitely (its own timer was cleared inTickonFound, so it also does nothing). Consider enqueuing the txid (and arming the timer) before the fallible awaits, or scheduling the removal in a failure-tolerant way.🛠️ Sketch
pub async fn mark_transaction_found(&mut self, tx_id: Txid) -> ActorResult<()> { info!("marking transaction found: {tx_id}"); + // Queue for follow-up scan and watcher removal even if the immediate scan fails, + // otherwise a failing scan leaves the watcher stuck in the HashMap forever. + self.pending_scans.pending_txids.push(tx_id); + if !self.pending_scans.timer_armed { + self.pending_scans.timer_armed = true; + self.pending_scans + .timer + .set_timeout_for_weak(self.addr.clone(), Duration::from_secs(30)); + } + // update the height self.update_block_id().await?; // update the height and perform sync scan which will update the transactions self.perform_scan_for_single_tx_id(tx_id).await?; - // wait 30 seconds, run follow-up scans, then remove watcher(s). - self.pending_scans.pending_txids.push(tx_id); - - if !self.pending_scans.timer_armed { - self.pending_scans.timer_armed = true; - self.pending_scans - .timer - .set_timeout_for_weak(self.addr.clone(), Duration::from_secs(30)); - } - Produces::ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager/actor.rs` around lines 947 - 967, The current mark_transaction_found path can early-return on errors from update_block_id() or perform_scan_for_single_tx_id(), leaking the TransactionWatcher because pending_scans.pending_txids and timer_armed are only set after those fallible awaits; to fix, ensure the tx_id is queued and the timer is armed before calling update_block_id()/perform_scan_for_single_tx_id() (i.e., move the pending_scans.pending_txids.push(tx_id) and timer arming logic earlier in mark_transaction_found), or alternatively wrap the awaits and on any Err send the remove_watcher_for_txn message (or otherwise schedule removal) so that if update_block_id() or perform_scan_for_single_tx_id() fail the watcher is removed; refer to mark_transaction_found, pending_scans.pending_txids, pending_scans.timer_armed, update_block_id, perform_scan_for_single_tx_id, remove_watcher_for_txn, Tick and TransactionWatcher when making the change.
🧹 Nitpick comments (1)
rust/src/manager/wallet_manager/actor.rs (1)
66-71:timer_armedis derivable frompending_txids.The invariant "timer armed ⇔
pending_txidsnon-empty" holds: you only arm the timer when pushing a txid, andtick()drains the queue and clearstimer_armedtogether. Dropping the bool and usingpending_txids.is_empty()as the arm-check removes a redundant source of truth.♻️ Sketch
#[derive(Debug, Default)] struct PendingScanState { timer: Timer, pending_txids: Vec<Txid>, - timer_armed: bool, }async fn tick(&mut self) -> ActorResult<()> { if !self.pending_scans.timer.tick() { return Produces::ok(()); } - self.pending_scans.timer_armed = false; for tx_id in std::mem::take(&mut self.pending_scans.pending_txids) {- self.pending_scans.pending_txids.push(tx_id); - if !self.pending_scans.timer_armed { - self.pending_scans.timer_armed = true; + let was_empty = self.pending_scans.pending_txids.is_empty(); + self.pending_scans.pending_txids.push(tx_id); + if was_empty { self.pending_scans .timer .set_timeout_for_weak(self.addr.clone(), Duration::from_secs(30)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager/actor.rs` around lines 66 - 71, PendingScanState contains a redundant timer_armed bool; remove the timer_armed field and replace all uses with checks on pending_txids.is_empty() (or !pending_txids.is_empty()) so the invariant "timer armed ⇔ pending_txids non-empty" is enforced from a single source of truth. Specifically, remove timer_armed from struct PendingScanState, update any code that sets or clears timer_armed (e.g., the push/arm location that previously did `if !timer_armed { arm timer; timer_armed = true }` should become `if pending_txids.is_empty() { arm timer }` before pushing; and the tick() or drain logic that cleared timer_armed should stop/clear the timer based on pending_txids becoming empty after draining). Also update any Default/derive usages if necessary to reflect the removed field and adjust tests/usages that referenced timer_armed to use pending_txids.is_empty() instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@rust/src/manager/wallet_manager/actor.rs`:
- Around line 947-967: The current mark_transaction_found path can early-return
on errors from update_block_id() or perform_scan_for_single_tx_id(), leaking the
TransactionWatcher because pending_scans.pending_txids and timer_armed are only
set after those fallible awaits; to fix, ensure the tx_id is queued and the
timer is armed before calling update_block_id()/perform_scan_for_single_tx_id()
(i.e., move the pending_scans.pending_txids.push(tx_id) and timer arming logic
earlier in mark_transaction_found), or alternatively wrap the awaits and on any
Err send the remove_watcher_for_txn message (or otherwise schedule removal) so
that if update_block_id() or perform_scan_for_single_tx_id() fail the watcher is
removed; refer to mark_transaction_found, pending_scans.pending_txids,
pending_scans.timer_armed, update_block_id, perform_scan_for_single_tx_id,
remove_watcher_for_txn, Tick and TransactionWatcher when making the change.
---
Nitpick comments:
In `@rust/src/manager/wallet_manager/actor.rs`:
- Around line 66-71: PendingScanState contains a redundant timer_armed bool;
remove the timer_armed field and replace all uses with checks on
pending_txids.is_empty() (or !pending_txids.is_empty()) so the invariant "timer
armed ⇔ pending_txids non-empty" is enforced from a single source of truth.
Specifically, remove timer_armed from struct PendingScanState, update any code
that sets or clears timer_armed (e.g., the push/arm location that previously did
`if !timer_armed { arm timer; timer_armed = true }` should become `if
pending_txids.is_empty() { arm timer }` before pushing; and the tick() or drain
logic that cleared timer_armed should stop/clear the timer based on
pending_txids becoming empty after draining). Also update any Default/derive
usages if necessary to reflect the removed field and adjust tests/usages that
referenced timer_armed to use pending_txids.is_empty() instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 875d996c-ee1c-4ea7-b14e-591785df5daf
📒 Files selected for processing (2)
rust/src/manager/wallet_manager/actor.rsrust/src/transaction_watcher.rs
Contributor
|
hey @kartikangiras have you addressed all the coderabbit and greptile comments? if so just resolve them, if not address them or respond to them. thanks |
praveenperera
force-pushed
the
ticktrait
branch
from
1fc6a2e to
b6435d1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
rust/src/transaction_watcher.rs (1)
72-78:⚠️ Potential issue | 🟠 MajorKeep the watcher retrying when client initialization fails.
client_builder.build().await?can return before the timer is armed, leaving this transaction with no follow-up retry. Also, iftickever runs withclient == None, it only schedules another check without rebuilding the client.Retry client initialization from the timer path
pub async fn start_watching(&mut self) -> ActorResult<()> { debug!("start_watching for txn {}", self.tx_id); - let client = Arc::new(self.client_builder.build().await?); - self.client = Some(client); - - // Trigger the first check immediately. - self.timer.set_timeout_for_weak(self.addr.clone(), Duration::ZERO); + match self.client_builder.build().await { + Ok(client) => { + self.client = Some(Arc::new(client)); + // Trigger the first check immediately. + self.timer.set_timeout_for_weak(self.addr.clone(), Duration::ZERO); + } + Err(error) => { + error!("failed to build transaction watcher client, retrying: {error:?}"); + self.client = None; + self.timer.set_timeout_for_weak(self.addr.clone(), Duration::from_secs(30)); + } + } Produces::ok(()) } async fn check_txn(&mut self) -> Result<WatchResult, Box<dyn std::error::Error>> { let Some(client) = self.client.clone() else { error!("transaction watcher client not initialized"); return Ok(WatchResult::Continue); };And in
tick:debug!("checking txn: {}", self.tx_id); + + if self.client.is_none() { + return self.start_watching().await; + } match self.check_txn().await {Also applies to: 83-87, 119-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/transaction_watcher.rs` around lines 72 - 78, The current start_watching uses client_builder.build().await? which can early-return on error and leave the watcher without a retry; change start_watching to attempt client_builder.build().await inside a match/if let, and on Err schedule the timer via timer.set_timeout_for_weak(self.addr.clone(), <retry_duration>) and return Ok(()), and on Ok wrap in Arc and assign to self.client then arm the immediate timer; likewise update tick so that when self.client.is_none() it attempts to rebuild the client (using client_builder.build().await), sets self.client on success, and on failure schedules another timer retry instead of only scheduling a check—use the existing symbols start_watching, client_builder.build(), self.client, timer.set_timeout_for_weak, and tick to locate and implement the changes.rust/src/manager/wallet_manager/actor.rs (1)
66-70:⚠️ Potential issue | 🟡 MinorConfirm batching semantics and dedupe queued txids.
This still batches all txids behind the first 30-second timer, so txids added later may wait much less than 30 seconds.
Vec<Txid>also allows duplicate follow-up scans/removals for the same transaction. If batching is intended, consider documenting it and switching the queue to a set for idempotency.Possible dedupe-only adjustment
-use ahash::HashMap; +use ahash::{HashMap, HashSet}; #[derive(Debug, Default)] struct PendingScanState { timer: Timer, - pending_txids: Vec<Txid>, + pending_txids: HashSet<Txid>, timer_armed: bool, } - for tx_id in std::mem::take(&mut self.pending_scans.pending_txids) { + for tx_id in std::mem::take(&mut self.pending_scans.pending_txids) { send!(self.addr.perform_scan_for_single_tx_id(tx_id)); send!(self.addr.remove_watcher_for_txn(tx_id)); } - self.pending_scans.pending_txids.push(tx_id); + self.pending_scans.pending_txids.insert(tx_id);Also applies to: 182-194, 967-975
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager/actor.rs` around lines 66 - 70, PendingScanState currently stores pending_txids in a Vec<Txid> which allows duplicates and gives inconsistent wait times because new txids are batched behind the first timer; change pending_txids to a HashSet<Txid> (or BTreeSet) to dedupe and make scans idempotent, update all code that inserts/iterates/drains pending_txids (e.g., where PendingScanState::timer is armed/checked and where entries are collected for scan/remove), and add a short doc comment on PendingScanState (and the analogous structs/fields at the other occurrences) clarifying the batching semantics and expected max-wait behavior so callers know that txids are grouped behind the current timer rather than per-item delays.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@rust/src/manager/wallet_manager/actor.rs`:
- Around line 66-70: PendingScanState currently stores pending_txids in a
Vec<Txid> which allows duplicates and gives inconsistent wait times because new
txids are batched behind the first timer; change pending_txids to a
HashSet<Txid> (or BTreeSet) to dedupe and make scans idempotent, update all code
that inserts/iterates/drains pending_txids (e.g., where PendingScanState::timer
is armed/checked and where entries are collected for scan/remove), and add a
short doc comment on PendingScanState (and the analogous structs/fields at the
other occurrences) clarifying the batching semantics and expected max-wait
behavior so callers know that txids are grouped behind the current timer rather
than per-item delays.
In `@rust/src/transaction_watcher.rs`:
- Around line 72-78: The current start_watching uses
client_builder.build().await? which can early-return on error and leave the
watcher without a retry; change start_watching to attempt
client_builder.build().await inside a match/if let, and on Err schedule the
timer via timer.set_timeout_for_weak(self.addr.clone(), <retry_duration>) and
return Ok(()), and on Ok wrap in Arc and assign to self.client then arm the
immediate timer; likewise update tick so that when self.client.is_none() it
attempts to rebuild the client (using client_builder.build().await), sets
self.client on success, and on failure schedules another timer retry instead of
only scheduling a check—use the existing symbols start_watching,
client_builder.build(), self.client, timer.set_timeout_for_weak, and tick to
locate and implement the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c2be288-cf08-48a5-a61b-892b09af8516
📒 Files selected for processing (2)
rust/src/manager/wallet_manager/actor.rsrust/src/transaction_watcher.rs
kartikangiras
force-pushed
the
ticktrait
branch
4 times, most recently
from
974d0a4 to
f8618e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
rust/src/transaction_watcher.rs (1)
72-88:⚠️ Potential issue | 🟠 MajorWatcher stays dormant if
client_builder.build()keeps failing.When
build()fails, you arm the timer for 10 s and leaveself.client = None. On the nexttick,check_txn()short-circuits withWatchResult::Continue(lines 91–94) becauseself.clientisNone, then schedulesnormal_wait_timeand returns. There is no path that retriesclient_builder.build(), so a transient connectivity error at the momentstart_watchingruns leaves thisTransactionWatcherpermanently silent fortx_id. The previous review flagged this; the timer was added but the rebuild path ontickwas not.🛠️ One option: rebuild on tick when client is missing
async fn tick(&mut self) -> ActorResult<()> { if !self.timer.tick() { return Produces::ok(()); } + if self.client.is_none() { + return self.start_watching().await; + } + debug!("checking txn: {}", self.tx_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/transaction_watcher.rs` around lines 72 - 88, start_watching currently gives up if client_builder.build() fails and leaves self.client = None, but check_txn doesn't retry — fix by adding a rebuild attempt inside check_txn when self.client.is_none(): call client_builder.build().await, on success wrap in Arc and set self.client, on failure log the error and re-arm the timer via timer.set_timeout_for_weak(self.addr.clone(), Duration::from_secs(10)) and return WatchResult::Continue so the watcher retries later; keep start_watching's initial timeout behavior (Duration::ZERO) but ensure check_txn handles the rebuild path and schedules normal_wait_time only when a client exists.
🧹 Nitpick comments (1)
rust/src/transaction_watcher.rs (1)
90-101: Prefer aSend + Syncerror type.
Result<_, Box<dyn std::error::Error>>is notSend + Sync, which can break compilation under#[async_trait]if anything intickever needs to hold this error across an await on a non-current-thread runtime, and it loses the structured error info you already have available. Considereyre::Result<WatchResult>(already used elsewhere in this crate) orBox<dyn std::error::Error + Send + Sync>.♻️ Suggested signature
- async fn check_txn(&mut self) -> Result<WatchResult, Box<dyn std::error::Error>> { + async fn check_txn(&mut self) -> eyre::Result<WatchResult> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/transaction_watcher.rs` around lines 90 - 101, The check_txn function currently returns Result<WatchResult, Box<dyn std::error::Error>> which is not Send+Sync; change its signature to use a Send+Sync-capable error type (e.g., eyre::Result<WatchResult> or Result<WatchResult, Box<dyn std::error::Error + Send + Sync>>) so the async context (and any callers like tick) can hold errors across awaits; update the function signature for check_txn and any callers that propagate its Result accordingly, and ensure you import eyre or adjust the Box error bound where used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/manager/wallet_manager/actor.rs`:
- Around line 967-975: The comment above the pending_scans timer is misleading:
update it to state that the 30s timer is only for follow-up scans and that
watcher removal is performed eagerly by TransactionWatcher::tick (it calls
send!(self.wallet_actor.remove_watcher_for_txn(tx_id))), so by the time timer
fires the watcher is usually already gone; alternatively, if you prefer the
delayed-removal design, remove the immediate removal call inside
TransactionWatcher::tick and rely on
pending_scans.timer.set_timeout_for_weak(self.addr.clone(),
Duration::from_secs(30)) and the logic in wallet_actor.remove_watcher_for_txn to
perform removal—pick one approach and make the code/comment consistent (refer to
pending_scans, timer.set_timeout_for_weak, TransactionWatcher::tick, and
remove_watcher_for_txn).
In `@rust/src/transaction_watcher.rs`:
- Around line 113-131: The match arm handling Ok(WatchResult::Found(txn)) should
not send a redundant remove_watcher_for_txn; remove the
send!(self.wallet_actor.remove_watcher_for_txn(tx_id)) call, keep
send!(self.wallet_actor.mark_transaction_found(tx_id)), and instead of
self.timer.clear() call self.timer.set_inactive() so the watcher stays alive for
the wallet's delayed cleanup; locate this logic around check_txn(),
WatchResult::Found, mark_transaction_found, remove_watcher_for_txn, and
timer.clear()/set_inactive().
---
Duplicate comments:
In `@rust/src/transaction_watcher.rs`:
- Around line 72-88: start_watching currently gives up if client_builder.build()
fails and leaves self.client = None, but check_txn doesn't retry — fix by adding
a rebuild attempt inside check_txn when self.client.is_none(): call
client_builder.build().await, on success wrap in Arc and set self.client, on
failure log the error and re-arm the timer via
timer.set_timeout_for_weak(self.addr.clone(), Duration::from_secs(10)) and
return WatchResult::Continue so the watcher retries later; keep start_watching's
initial timeout behavior (Duration::ZERO) but ensure check_txn handles the
rebuild path and schedules normal_wait_time only when a client exists.
---
Nitpick comments:
In `@rust/src/transaction_watcher.rs`:
- Around line 90-101: The check_txn function currently returns
Result<WatchResult, Box<dyn std::error::Error>> which is not Send+Sync; change
its signature to use a Send+Sync-capable error type (e.g.,
eyre::Result<WatchResult> or Result<WatchResult, Box<dyn std::error::Error +
Send + Sync>>) so the async context (and any callers like tick) can hold errors
across awaits; update the function signature for check_txn and any callers that
propagate its Result accordingly, and ensure you import eyre or adjust the Box
error bound where used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba657d18-3556-4ec6-85ad-863b32b81d21
📒 Files selected for processing (2)
rust/src/manager/wallet_manager/actor.rsrust/src/transaction_watcher.rs
kartikangiras
force-pushed
the
ticktrait
branch
from
f8618e4 to
7e2b19c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/transaction_watcher.rs`:
- Around line 72-88: The build retry never actually gets re-run because tick()
sees self.client.is_none() and then schedules the normal wait, never re-invoking
start_watching; update tick() so that when check_txn() returns Continue because
self.client.is_none(), it schedules the client-retry timeout (use the same
timer.set_timeout_for_weak(self.addr.clone(), Duration::from_secs(10)) semantics
or a configurable retry delay) instead of scheduling normal_wait_time(),
ensuring start_watching() will be invoked again to retry client_builder.build();
reference tick(), check_txn(), start_watching(), client_builder.build(),
timer.set_timeout_for_weak(), and normal_wait_time() when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52a9926a-b988-4a8f-b857-6bf61af0c02b
📒 Files selected for processing (2)
rust/src/manager/wallet_manager/actor.rsrust/src/transaction_watcher.rs
Contributor
Author
|
@praveenperera I have address all the inline review comments and from bots as well. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactored actor timer implementations to use act-zero trait and Timer infrastructure instead of manual send_fut + tokio::time::sleep patterns.
fixes #495
Testing
Platform Coverage
Checklist
Summary by CodeRabbit