[Chore] Deduplicate retry/error-handling logic in the accrual keeper

[collinsezedike]

Summary

packages/stellar-sdk-helpers/src/accrual-keeper.ts reimplements several pieces of logic that already exist elsewhere in stellar-sdk-helpers, instead of reusing them. None of these are correctness bugs, the duplicated versions currently behave the same as the originals, but they're a maintenance risk: a future fix to the shared version won't propagate to the duplicate, and vice versa.

Motivation

This surfaced during review of #484 (the Blend accrual keeper). The correctness/security findings from that review were fixed directly on the PR; these reuse findings were deliberately deferred to keep that PR focused, rather than bundling a refactor into a security fix. Filing them here so they're tracked instead of forgotten.

Update: #469's migration keeper needed most of this same submission/retry logic. Duplicating it a third time would have made the problem worse, so several items below were resolved as part of that work rather than deferred further; see the acceptance criteria for current status.

Proposed Solution

• withKeeperRetry reimplements withRetry (packages/shared/src/utils.ts), which tx.ts already uses. withRetry already supports attempts, base delay, doubling backoff, and a shouldRetry predicate, everything withKeeperRetry needs. Extend withRetry to optionally report { value, attempts } and accept a logging hook, then have tx.ts, the accrual keeper, and the migration keeper all share it.
• KeeperRpcServer (now in keeper-tx.ts) hand-declares 4 methods that already exist on the real rpc.Server type from @stellar/stellar-sdk. Replace with Pick<rpc.Server, "getAccount" | "simulateTransaction" | "sendTransaction" | "getTransaction">.
• SkippedAdapter.reason is hardcoded to the literal "non-blend" regardless of which protocol was actually skipped. Once a second protocol is discoverable (e.g. DeFindex), every skip reports the same undifferentiated reason. Store the actual protocol name instead.
• getAccount (and its sequence number) is re-fetched from RPC on every adapter/vault iteration in each keeper's sequential submission loop, instead of tracking the sequence locally and incrementing after each successful submit. Real, if modest, avoidable RPC load, worth revisiting once more than a couple of vaults are live.
• Both discoverLiveAdapters (accrual-keeper.ts) and discoverMigrationVaults (migration-keeper.ts) retry a multi-call closure as one unit. If an early call succeeds but a later one in the same closure fails transiently, the retry re-issues the already-succeeded calls too instead of resuming from the failed one. Splitting each call into its own retry boundary avoids the redundant reads.

Scope

Field	Value
Area	SDK
Protocol affected	None
Network	testnet
Breaking change?	No (internal refactor, no exported behavior changes)

Alternatives Considered

Leaving it as-is: the duplicated code is currently correct, so there's no urgency, but every future fix to the shared originals (withRetry, describeSendError, defaultSleep) has to be remembered and re-applied to each keeper's private copy, or they silently drift.

Acceptance Criteria

• withKeeperRetry is removed in favor of an extended withRetry
• describeSendError and sleep/defaultSleep are each defined once and imported, not duplicated (describeSendError exported from tx.ts; sleep/errorMessage/withKeeperRetry/KeeperRetryError/KeeperLogger/KeeperFailure moved to a shared keeper-retry.ts, used by both keepers)
• KeeperRpcServer is a Pick<rpc.Server, ...> instead of a hand-written interface
• Attempts/transient extraction is a single shared helper (retryOutcome() in keeper-retry.ts, used by both keepers)
• Signing keypair is derived once per adapter, not once per retry attempt (submitKeeperOperation in keeper-tx.ts: the priorHash branch returns before ever reaching Keypair.fromSecret, so a retry only re-derives it if the prior attempt failed before obtaining a hash at all)
• SkippedAdapter.reason names the actual protocol, not a hardcoded string
• Each discovery call retries independently instead of one combined closure (now applies to both discoverLiveAdapters and discoverMigrationVaults)
• Existing keeper test suite still passes with no behavior changes (166 tests passing in stellar-sdk-helpers, including the accrual keeper's original suite unchanged)

[Chigybillionz]

Hi, I’ve reviewed this issue and I’m confident I can fix it. I’d like to take it on.

[collinsezedike]

Update from #469's migration keeper work: several of these items got resolved as a side effect rather than deferred further, since duplicating this logic a third time (for the new migration keeper) would have made the problem worse, not better.

Resolved:

• describeSendError is now defined once in tx.ts (exported) and imported everywhere else, no more duplicate.
• withKeeperRetry, KeeperRetryError, KeeperLogger, KeeperFailure, sleep, and errorMessage moved to a new shared keeper-retry.ts, used by both the accrual and migration keepers instead of each keeper defining its own copy.
• KeeperRpcServer, SubmissionInFlightError, SubmissionFailedError, isDefinitiveOnChainFailure, expectString, and the whole build/sign/send/confirm-with-priorHash-tracking transaction flow moved to a new shared keeper-tx.ts (submitKeeperOperation), used by both keepers instead of each reimplementing transaction submission.
• Added a retryOutcome() helper for the attempts/transient extraction pattern, now used by both keepers instead of being copy-pasted at each catch site.
• The signing keypair re-derivation concern is largely moot now: submitKeeperOperation's priorHash branch returns before ever reaching Keypair.fromSecret, so a keypair is only re-derived on a retry that failed before obtaining a hash in the first place, not on every attempt.

Still open, and now touching two files instead of one:

• withKeeperRetry is still its own implementation, not merged into withRetry (packages/shared/src/utils.ts). tx.ts's own retry usage (submitTx, prepareSorobanTx) still goes through withRetry directly, unmerged.
• KeeperRpcServer is still a hand-written interface, not Pick<rpc.Server, ...>.
• Both discoverLiveAdapters (accrual-keeper.ts) and discoverMigrationVaults (migration-keeper.ts) retry a multi-call closure as one unit (get_adapter+get_protocol in the first, get_adapter+get_protocol+get_pool in the second), so a transient failure on the last call re-issues the earlier, already-succeeded calls too.
• SkippedAdapter.reason is still hardcoded to "non-blend".
• getAccount is still re-fetched per adapter/vault in each keeper's sequential submission loop.

[collinsezedike]

Two more items from reviewing #512 (the migration keeper PR):

• `discoverMigrationVaults` (migration-keeper.ts) duplicates `discoverLiveAdapters`'s (accrual-keeper.ts) concurrent-discovery orchestration almost line-for-line: the `Promise.allSettled` + per-target `withKeeperRetry` + result/failure-splitting shape is copy-pasted, only the per-target async body differs.
• `runMigrationKeeper`'s submission loop duplicates `runBlendAccrualKeeper`'s ~70-line shape: deadline check, `priorHash` tracking via a `.catch` capturing `SubmissionInFlightError.sentHash`, the `withKeeperRetry` call, and success/failure object assembly are hand-copied between the two files.
• `api/v1/keepers/rebalance.ts` duplicates `api/v1/keepers/accrue.ts`'s handler boilerplate structurally: method check, rate limit, `CRON_SECRET` missing/503, `isCronAuthorized`/401, try/catch with the same error-response shape. Only the config loader/runner call and log prefix differ.

Not fixed as part of #512: extracting these is a bigger structural change than the two keepers already share (retry primitives, transaction submission, transient-error classification, all already deduped in that PR), and didn't feel proportionate to do a third time while that PR was already large. Adding here rather than letting them get lost.

[collinsezedike]

One more from the second review round on #512: `keeper-retry.ts`/`keeper-tx.ts` (the shared modules this issue is largely about) have no dedicated test file of their own, coverage is only indirect, through `accrual-keeper.test.ts` and `migration-keeper.test.ts`. Once these modules are the sole home of the retry/backoff/deadline/submission logic (per this issue's acceptance criteria), a `keeper-retry.test.ts`/`keeper-tx.test.ts` pinning their contract directly would catch a regression that happens to still pass both keepers' specific mocks/timings.

Also: `withKeeperRetry`'s backoff is pure exponential with no jitter. Both keepers can fire many concurrent retries at once (discovery across vaults, now candidate evaluation too), so a shared transient blip (e.g. a rate-limit window) causes every one of them to retry in synchronized bursts rather than spread out. Worth adding ±20-30% jitter when this gets touched.

[collinsezedike]

Two smaller items from #512's third review round:

• checkRateLimit (api/_lib/middleware.ts) keys its bucket by IP only, no route dimension. Now shared by both accrue.ts (15 min) and rebalance.ts (hourly), both called from Vercel Cron's same egress IP range. Low likelihood given the call frequency, but a burst on one route's budget can starve the other's scheduled call under the in-memory fallback (no Upstash configured). Worth a route-scoped key (IP+path) if this ever becomes a real symptom.
• The !CRON_SECRET && VERCEL_ENV !== undefined 503 condition is duplicated in both accrue.ts and rebalance.ts, and re-derives logic isCronAuthorized (middleware.ts) already computes internally for its own permissive-local-dev branch. Same root cause as the endpoint-boilerplate duplication already listed here, but specifically: the shared handler wrapper this issue proposes should expose that check directly rather than have each route reimplement it.

[collinsezedike]

@Chigybillionz can you accommodate the new scope for this issue?

[collinsezedike]

@Chigybillionz following up, no rush if you're mid-work elsewhere, but this scope grew significantly since you applied. If you're not able to take it on as currently scoped, that's fine, just let me know so it's open for someone else.