feat(sdk): add utility class for subscribing to token balance changes#265
Conversation
…l#182) Replaces the minimal stub in test-utils/mockRpcServer.ts with a fully-typed, reusable mock that covers every Soroban RPC method used by the SDK. Tests no longer depend on any network connection. Changes: - Expand mockRpcServer.ts to mock all 9 RPC methods: getAccount, simulateTransaction, sendTransaction, getTransaction, getNetwork, getLatestLedger, getLedger, getFeeStats, getEvents - Add 12 pre-canned scenario helpers (rpc.scenarios.*) for the most common test situations (success, pendingThenSuccess, simulationError, transactionFailed, accountNotFound, networkError, etc.) - Intercept both @stellar/stellar-sdk/rpc and @stellar/stellar-sdk import paths via vi.mock() so all SDK modules are covered - Add resetMockRpcServer() that clears call history and return values for clean test isolation in beforeEach - Add mockRpcServer.test.ts with 49 tests covering the mock contract - Fix missing mockTxNone helper in DistributorClient.test.ts and PaymentStreamClient.test.ts (caused ReferenceError at runtime) Closes Fundable-Protocol#182
…Fundable-Protocol#181) Enhances BalanceWatcher with the full feature set described in issue Fundable-Protocol#181: New capabilities: - watchStream(stream, callback) — subscribes to both sender and recipient of a payment stream in a single call; returns a single unsub function that tears down both subscriptions - fetchBalances(pairs) — bulk-fetches multiple address/token pairs in parallel; failures are captured per-pair instead of aborting the batch - getLastKnownBalance(address, token) — reads the cached balance from the last successful poll without making any RPC call - onError option — replaces the silent console.error with a typed callback so callers control error handling - unwatch(address, token, callback) — explicit unsubscribe method in addition to the returned unsub closure Improvements to existing behaviour: - Polling errors for one pair no longer silently swallow; they route to onError (or console.error as fallback) while other pairs keep polling - start() guards against duplicate intervals (idempotent) - stop() is idempotent — calling it when already stopped does not throw - void operator on pollBalances() call to satisfy no-floating-promises Tests (BalanceWatcher.test.ts) — expanded from 11 to 54 tests covering: - fetchBalance: happy path, zero balance, i128 max, error response, no result, non-bigint retval, network rejection, passphrase caching - fetchBalances: all pairs succeed, per-pair error capture, empty input - getLastKnownBalance: before poll, after poll, after clear, multi-poll - watch/unwatch: change detection, no-change suppression, multiple callbacks, multi-pair independence, update shape validation - watchStream: sender + recipient registration, per-party callbacks, unsubscribe clears both, correct token forwarding - onError: invocation, correct args, handler throws safely, per-pair isolation (error in one pair does not block others) - Lifecycle: isActive, start/stop/clear, idempotency, interval timing Closes Fundable-Protocol#181
|
@mandyslovestories-sudo Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughThe PR rewrites the shared mock RPC server, expands BalanceWatcher with stream watching, batch fetching, cached reads, and configurable poll-error handling, and adds undefined-result transaction test helpers. ChangesMock RPC server overhaul
BalanceWatcher polling and cache
Undefined-result transaction mocks
Sequence Diagram(s)sequenceDiagram
participant BalanceWatcher
participant RpcServer as rpc.Server
participant BalanceCallback
participant OnError as onError
BalanceWatcher->>BalanceWatcher: watch(address, token, callback)
BalanceWatcher->>BalanceWatcher: pollBalances()
BalanceWatcher->>RpcServer: fetchBalance(address, token)
RpcServer-->>BalanceWatcher: simulation result or error
alt balance changed
BalanceWatcher->>BalanceCallback: BalanceUpdate
end
opt fetch failure
BalanceWatcher->>OnError: error, address, token
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/sdk/src/__tests__/BalanceWatcher.test.tsParsing error: Cannot read file '/tsconfig.json'. packages/sdk/src/__tests__/DistributorClient.test.tsParsing error: Cannot read file '/tsconfig.json'. packages/sdk/src/__tests__/PaymentStreamClient.test.tsParsing error: Cannot read file '/tsconfig.json'.
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
packages/sdk/src/__tests__/PaymentStreamClient.test.ts (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelegate to
mockTx()to avoid duplication.
mockTxNoneduplicatesmockTx()sincemockTxalready defaultsresulttoundefined. Have it callmockTx()instead.♻️ Proposed change
-const mockTxNone = () => ({ result: undefined, signAndSend: mockSignAndSend }); +const mockTxNone = () => mockTx();Note:
DistributorClient.test.tsuses a freshvi.fn()permockTxcall, while this file sharesmockSignAndSend. Consider aligning the two files for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/__tests__/PaymentStreamClient.test.ts` at line 15, `mockTxNone` in `PaymentStreamClient.test.ts` duplicates `mockTx()` because `mockTx()` already defaults `result` to `undefined`; update `mockTxNone` to delegate to `mockTx()` instead of constructing the object manually. Keep the shared `mockSignAndSend` behavior in this file, and align the helper pattern with `mockTx`/`mockTxNone` so the test utilities stay consistent and avoid duplicated setup.packages/sdk/src/__tests__/DistributorClient.test.ts (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEliminate redundancy with
mockTx().
mockTxNoneis functionally identical tomockTx()sincemockTxalready defaultsresulttoundefined. Delegate tomockTxto avoid a second source of truth for the same mock shape.♻️ Proposed change
-const mockTxNone = () => ({ result: undefined, signAndSend: vi.fn() }); +const mockTxNone = () => mockTx();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/__tests__/DistributorClient.test.ts` around lines 13 - 14, The mock helper mockTxNone in DistributorClient.test.ts is redundant because it duplicates the default behavior already provided by mockTx. Update mockTxNone to delegate to mockTx instead of constructing its own object so there is only one source of truth for the mock shape and default undefined result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/sdk/src/__tests__/BalanceWatcher.test.ts`:
- Around line 76-90: The BalanceWatcher tests still allow the automatic initial
poll triggered by watch() to run, so manual poll() calls can overlap and consume
mocks unexpectedly. Update the test setup around the BalanceWatcher helper and
the watch()/pollBalances flow to explicitly account for or await the initial
poll before invoking poll(), using the BalanceWatcher mock helpers and the
poll(watcher) helper so deterministic tests don’t have two in-flight polls.
- Around line 606-614: The BalanceWatcher test for repeated start() calls is
only checking isActive() and watcher count, which does not directly prove there
is a single scheduled interval. Update the test around makeWatcher(), watch(),
and start() to assert the fake-timer count or spy on setInterval so you verify
only one interval is created even when start() is called multiple times.
In `@packages/sdk/src/test-utils/mockRpcServer.ts`:
- Around line 444-453: The resetMockRpcServer helper is only resetting object
values, so the top-level vi.fn() RPC mocks are skipped and their call state
leaks between tests. Update resetMockRpcServer to iterate over the mock entries
and reset function-valued mocks (using mockReset on vi.fn() functions) while
still excluding the scenarios container, and keep the logic localized to
resetMockRpcServer and the mock object it walks.
In `@packages/sdk/src/utils/BalanceWatcher.ts`:
- Around line 297-302: The BalanceWatcher polling loop can overlap and let a
slower, older poll overwrite newer balance data. Update the polling flow in
BalanceWatcher.pollBalances() and the setInterval startup path to serialize
requests or discard stale results using a monotonic sequence/token so only the
latest poll may update lastBalance and emit balance changes. Keep the initial
immediate poll and all interval-triggered polls coordinated through the same
guard so updates cannot arrive out of order.
- Around line 436-438: The fallback error log in BalanceWatcher should not print
full address/token identifiers by default. Update the console.error path in
BalanceWatcher’s balance-fetch error handling to redact or partially mask
record.address and record.token, and only emit full identifiers when explicitly
opted in through onError or a similar caller-controlled flag. Keep the error
object logging, but make the identifier formatting privacy-safe in this fallback
branch.
- Around line 180-182: The watch flow in BalanceWatcher.watch currently only
calls start() when isRunning is false, so newly added address/token pairs are
not polled immediately if the watcher is already active. Update watch() so it
triggers an immediate poll for the newly registered pair regardless of
isRunning, while preserving the existing running state and interval loop; use
the watch() and start() logic in BalanceWatcher to locate and adjust this
behavior.
---
Nitpick comments:
In `@packages/sdk/src/__tests__/DistributorClient.test.ts`:
- Around line 13-14: The mock helper mockTxNone in DistributorClient.test.ts is
redundant because it duplicates the default behavior already provided by mockTx.
Update mockTxNone to delegate to mockTx instead of constructing its own object
so there is only one source of truth for the mock shape and default undefined
result.
In `@packages/sdk/src/__tests__/PaymentStreamClient.test.ts`:
- Line 15: `mockTxNone` in `PaymentStreamClient.test.ts` duplicates `mockTx()`
because `mockTx()` already defaults `result` to `undefined`; update `mockTxNone`
to delegate to `mockTx()` instead of constructing the object manually. Keep the
shared `mockSignAndSend` behavior in this file, and align the helper pattern
with `mockTx`/`mockTxNone` so the test utilities stay consistent and avoid
duplicated setup.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c73757f-f12a-450a-9ab1-b599e40929cc
📒 Files selected for processing (6)
packages/sdk/src/__tests__/BalanceWatcher.test.tspackages/sdk/src/__tests__/DistributorClient.test.tspackages/sdk/src/__tests__/PaymentStreamClient.test.tspackages/sdk/src/__tests__/mockRpcServer.test.tspackages/sdk/src/test-utils/mockRpcServer.tspackages/sdk/src/utils/BalanceWatcher.ts
| pollInterval: 60_000, // long interval — prevents auto-polls interfering | ||
| ...opts, | ||
| }); | ||
| } | ||
|
|
||
| function mockSuccess(balance: bigint) { | ||
| /** Wire up the SDK mocks so that fetchBalance(ADDRESS, TOKEN) returns `balance`. */ | ||
| function mockBalanceSuccess(balance: bigint) { | ||
| const { scValToNative } = require('@stellar/stellar-sdk'); | ||
| mockSimulateTransaction.mockResolvedValue({ | ||
| result: { retval: {} }, | ||
| }); | ||
| mockSimulateTransaction.mockResolvedValue({ result: { retval: {} } }); | ||
| (scValToNative as ReturnType<typeof vi.fn>).mockReturnValue(balance); | ||
| } | ||
|
|
||
| /** Shorthand: manually invoke the private pollBalances method. */ | ||
| function poll(watcher: BalanceWatcher): Promise<void> { | ||
| return (watcher as unknown as { pollBalances(): Promise<void> }).pollBalances(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Account for the automatic initial poll in deterministic tests.
The long interval does not prevent watch() from immediately calling pollBalances(). Tests that call the private poll() after watch() can have an extra in-flight poll consuming mock return values or invoking callbacks unexpectedly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sdk/src/__tests__/BalanceWatcher.test.ts` around lines 76 - 90, The
BalanceWatcher tests still allow the automatic initial poll triggered by watch()
to run, so manual poll() calls can overlap and consume mocks unexpectedly.
Update the test setup around the BalanceWatcher helper and the
watch()/pollBalances flow to explicitly account for or await the initial poll
before invoking poll(), using the BalanceWatcher mock helpers and the
poll(watcher) helper so deterministic tests don’t have two in-flight polls.
| it('calling start() multiple times does not create duplicate intervals', () => { | ||
| vi.useFakeTimers(); | ||
| const watcher = makeWatcher(); | ||
| watcher.watch(ADDRESS, TOKEN, vi.fn()); | ||
| watcher.start(); // already running from watch() | ||
| watcher.start(); // should be a no-op | ||
| // Only one interval should exist — validated via getWatcherCount staying stable | ||
| expect(watcher.isActive()).toBe(true); | ||
| watcher.clear(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate lifecycle timer tests and available Vitest timer assertions in this file.
rg -n -C3 'useFakeTimers|getTimerCount|setInterval|start\\(\\)' packages/sdk/src/__tests__/BalanceWatcher.test.tsRepository: Fundable-Protocol/stellar_client_os
Length of output: 776
Assert the timer count directly here. isActive() and watcher counts don’t prove only one scheduled interval; check the fake-timer count or spy on setInterval so repeated start() calls can’t silently add duplicates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sdk/src/__tests__/BalanceWatcher.test.ts` around lines 606 - 614,
The BalanceWatcher test for repeated start() calls is only checking isActive()
and watcher count, which does not directly prove there is a single scheduled
interval. Update the test around makeWatcher(), watch(), and start() to assert
the fake-timer count or spy on setInterval so you verify only one interval is
created even when start() is called multiple times.
| export function resetMockRpcServer(): void { | ||
| (Object.values(mock) as unknown[]).forEach((value) => { | ||
| if (value !== null && typeof value === 'object' && !('scenarios' in (value as object))) { | ||
| const fn = value as { mockReset?: () => void }; | ||
| if (typeof fn.mockReset === 'function') { | ||
| fn.mockReset(); | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Reset the top-level vi.fn() methods.
Line 446 filters for typeof value === 'object', but every RPC mock is a vi.fn() function, so none of the mocks are reset. Call history and resolved values will leak between tests.
Proposed fix
export function resetMockRpcServer(): void {
- (Object.values(mock) as unknown[]).forEach((value) => {
- if (value !== null && typeof value === 'object' && !('scenarios' in (value as object))) {
- const fn = value as { mockReset?: () => void };
- if (typeof fn.mockReset === 'function') {
- fn.mockReset();
- }
+ Object.entries(mock).forEach(([key, value]) => {
+ if (key === 'scenarios') {
+ return;
+ }
+
+ const fn = value as { mockReset?: () => void };
+ if (typeof fn.mockReset === 'function') {
+ fn.mockReset();
}
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function resetMockRpcServer(): void { | |
| (Object.values(mock) as unknown[]).forEach((value) => { | |
| if (value !== null && typeof value === 'object' && !('scenarios' in (value as object))) { | |
| const fn = value as { mockReset?: () => void }; | |
| if (typeof fn.mockReset === 'function') { | |
| fn.mockReset(); | |
| } | |
| } | |
| }); | |
| } | |
| export function resetMockRpcServer(): void { | |
| Object.entries(mock).forEach(([key, value]) => { | |
| if (key === 'scenarios') { | |
| return; | |
| } | |
| const fn = value as { mockReset?: () => void }; | |
| if (typeof fn.mockReset === 'function') { | |
| fn.mockReset(); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sdk/src/test-utils/mockRpcServer.ts` around lines 444 - 453, The
resetMockRpcServer helper is only resetting object values, so the top-level
vi.fn() RPC mocks are skipped and their call state leaks between tests. Update
resetMockRpcServer to iterate over the mock entries and reset function-valued
mocks (using mockReset on vi.fn() functions) while still excluding the scenarios
container, and keep the logic localized to resetMockRpcServer and the mock
object it walks.
| if (!this.isRunning) { | ||
| this.start(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Poll newly added pairs immediately even when the watcher is already running.
watch() only triggers an immediate poll through start() when isRunning is false. Adding a new address/token pair while another pair is already being watched waits until the next interval, despite the method contract saying the pair is polled immediately.
Proposed direction
+ const isNewPair = !this.watchers.has(key);
+
- if (!this.watchers.has(key)) {
+ if (isNewPair) {
this.watchers.set(key, {
address,
token,
lastBalance: null,
callbacks: new Set(),
});
}
@@
if (!this.isRunning) {
this.start();
+ } else if (isNewPair) {
+ void this.pollBalances();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!this.isRunning) { | |
| this.start(); | |
| } | |
| if (!this.isRunning) { | |
| this.start(); | |
| } else if (isNewPair) { | |
| void this.pollBalances(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sdk/src/utils/BalanceWatcher.ts` around lines 180 - 182, The watch
flow in BalanceWatcher.watch currently only calls start() when isRunning is
false, so newly added address/token pairs are not polled immediately if the
watcher is already active. Update watch() so it triggers an immediate poll for
the newly registered pair regardless of isRunning, while preserving the existing
running state and interval loop; use the watch() and start() logic in
BalanceWatcher to locate and adjust this behavior.
| this.intervalId = setInterval(() => { | ||
| this.pollBalances(); | ||
| void this.pollBalances(); | ||
| }, this.pollInterval); | ||
|
|
||
| // Initial poll | ||
| this.pollBalances(); | ||
| // Initial poll immediately so callers get a balance update right away | ||
| void this.pollBalances(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent overlapping poll cycles from racing the cache backward.
Each interval fires pollBalances() without waiting for the previous cycle. If an older, slower RPC response resolves after a newer one, it can overwrite lastBalance with stale data and emit out-of-order balance updates.
Proposed direction
+ private pollInFlight = false;
+
private async pollBalances(): Promise<void> {
+ if (this.pollInFlight) return;
+ this.pollInFlight = true;
+ try {
const promises = Array.from(this.watchers.values()).map(
async (record) => {
// existing per-record polling logic
},
);
await Promise.allSettled(promises);
+ } finally {
+ this.pollInFlight = false;
+ }
}Also applies to: 400-445
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sdk/src/utils/BalanceWatcher.ts` around lines 297 - 302, The
BalanceWatcher polling loop can overlap and let a slower, older poll overwrite
newer balance data. Update the polling flow in BalanceWatcher.pollBalances() and
the setInterval startup path to serialize requests or discard stale results
using a monotonic sequence/token so only the latest poll may update lastBalance
and emit balance changes. Keep the initial immediate poll and all
interval-triggered polls coordinated through the same guard so updates cannot
arrive out of order.
| console.error( | ||
| `BalanceWatcher: error fetching balance for ${record.address}:${record.token}:`, | ||
| err, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid logging full address/token identifiers by default.
Address/token pairs can identify user holdings or activity. Prefer redaction in the fallback log path, or require callers to opt into full identifiers through onError.
Proposed fix
console.error(
- `BalanceWatcher: error fetching balance for ${record.address}:${record.token}:`,
+ "BalanceWatcher: error fetching balance for watched pair:",
err,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.error( | |
| `BalanceWatcher: error fetching balance for ${record.address}:${record.token}:`, | |
| err, | |
| console.error( | |
| "BalanceWatcher: error fetching balance for watched pair:", | |
| err, |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 435-438: Avoid logging sensitive data
Context: console.error(
BalanceWatcher: error fetching balance for ${record.address}:${record.token}:,
err,
)
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-data-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sdk/src/utils/BalanceWatcher.ts` around lines 436 - 438, The
fallback error log in BalanceWatcher should not print full address/token
identifiers by default. Update the console.error path in BalanceWatcher’s
balance-fetch error handling to redact or partially mask record.address and
record.token, and only emit full identifiers when explicitly opted in through
onError or a similar caller-controlled flag. Keep the error object logging, but
make the identifier formatting privacy-safe in this fallback branch.
Source: Linters/SAST tools
Summary
Closes #181
Enhances
BalanceWatcherwith the full feature set described in the issue: stream-level subscriptions, bulk balance fetching, a last-known-balance cache, a typed error callback, and an explicit unsubscribe method. The test suite is expanded from 11 to 54 tests covering every new and existing behaviour.Changes
src/utils/BalanceWatcher.tsNew methods:
watchStream(stream, callback)senderandrecipientof aStreamin one call. Returns a single unsub function that tears down both subscriptions atomically.fetchBalances(pairs){ balance: null, error }) so a single RPC error never aborts the batch.getLastKnownBalance(address, token)nullbefore the first poll or when the pair is not watched.unwatch(address, token, callback)watch()).New constructor option:
onError(error, address, token)console.error. Gives callers full control over error handling (logging, alerting, retrying).Improvements to existing behaviour:
start()is idempotent — calling it when already running does not create duplicate intervalsstop()is idempotent — calling it when already stopped does not throwvoidoperator on internalpollBalances()call to avoid floating-promise lint warningsNew exported types:
BalancePair,BalanceFetchResultsrc/__tests__/BalanceWatcher.test.tsExpanded from 11 to 54 tests:
fetchBalancefetchBalancesgetLastKnownBalancewatch / unwatchunwatchwatchStreamonErrorLifecycleisActive,start/stop/clear, idempotency,getWatcherCount, interval timingUsage example
Testing
pnpm --filter @fundable/sdk testSummary by CodeRabbit
New Features
Bug Fixes