Spike: retry/resilience policy, session lifecycle, and multisig state coordination

[bbjiggy]

Summary

Resolves the spike in #79: define a retry/resilience policy, a Node-vs-browser-aware session
storage + token lifecycle strategy, and a multisig operation-state coordination strategy, with
a prototype of each and any blocking unknowns flagged.

Full writeup (idempotency table, options considered, decisions, unknowns):
docs/spikes/issue-79-retry-session-multisig.md.

What changed

• Retry. Re-audited the current state first: TransactionPipeline and createApiHttpClient
  (axios-retry) already cover most of the gap described in the issue. What was left was dead code:
  removed src/stellar/rpc.ts (simulateAndAssemble) — unreferenced anywhere, not retried, not
  timed out, fully superseded by TransactionPipeline.prepare. src/utils/retry.ts is kept as-is
  (it's a tested public utility, not dead).
• Session. auth/session.ts now supports a pluggable SessionStorageAdapter
  (configureSessionStorage/resetSessionStorage), defaults to an in-memory adapter under Node
  instead of silently no-op'ing, and persists an expiresAt alongside the token with a new
  isSessionExpired() helper. Backward compatible — existing saveSession(token, address) calls
  still work.
• Multisig. Added the target MultiSigStateStore interface (src/types/multisig.ts) as the
  abstraction a future backend-backed store should satisfy, plus exportState/importState on
  MultiSigEscrowClient as a non-breaking stopgap that lets an operation's state be round-tripped
  through an external store today, ahead of native async storage support.

Follow-up implementation issues filed

• Backend: add token expiry (expiresIn/expiresAt) to /auth/verify response #82 — Backend: add token expiry to /auth/verify
• Implement backend-backed MultiSigStateStore for cross-process signer coordination #83 — Implement backend-backed MultiSigStateStore
• Consolidate TransactionPipeline's internal retry loop onto utils/retry.ts #84 — Consolidate TransactionPipeline's retry loop onto utils/retry.ts

Closes #79

Test plan

• npm test — 129/129 passing (added coverage for session expiry, pluggable storage, Node
  in-memory default, and multisig exportState/importState)
• npm run lint — 0 errors (pre-existing no-explicit-any warnings elsewhere untouched)
• npm run build — CJS/ESM/DTS artifacts build cleanly

[meshackyaro]

Summary

Resolves the spike in #79: define a retry/resilience policy, a Node-vs-browser-aware session storage + token lifecycle strategy, and a multisig operation-state coordination strategy, with a prototype of each and any blocking unknowns flagged.

Full writeup (idempotency table, options considered, decisions, unknowns): docs/spikes/issue-79-retry-session-multisig.md.

What changed

• Retry. Re-audited the current state first: TransactionPipeline and createApiHttpClient
  (axios-retry) already cover most of the gap described in the issue. What was left was dead code:
  removed src/stellar/rpc.ts (simulateAndAssemble) — unreferenced anywhere, not retried, not
  timed out, fully superseded by TransactionPipeline.prepare. src/utils/retry.ts is kept as-is
  (it's a tested public utility, not dead).
• Session. auth/session.ts now supports a pluggable SessionStorageAdapter
  (configureSessionStorage/resetSessionStorage), defaults to an in-memory adapter under Node
  instead of silently no-op'ing, and persists an expiresAt alongside the token with a new
  isSessionExpired() helper. Backward compatible — existing saveSession(token, address) calls
  still work.
• Multisig. Added the target MultiSigStateStore interface (src/types/multisig.ts) as the
  abstraction a future backend-backed store should satisfy, plus exportState/importState on
  MultiSigEscrowClient as a non-breaking stopgap that lets an operation's state be round-tripped
  through an external store today, ahead of native async storage support.

Follow-up implementation issues filed

• Backend: add token expiry (expiresIn/expiresAt) to /auth/verify response #82 — Backend: add token expiry to /auth/verify
• Implement backend-backed MultiSigStateStore for cross-process signer coordination #83 — Implement backend-backed MultiSigStateStore
• Consolidate TransactionPipeline's internal retry loop onto utils/retry.ts #84 — Consolidate TransactionPipeline's retry loop onto utils/retry.ts

Closes #79

Test plan

• npm test — 129/129 passing (added coverage for session expiry, pluggable storage, Node
  in-memory default, and multisig exportState/importState)
• npm run lint — 0 errors (pre-existing no-explicit-any warnings elsewhere untouched)
• npm run build — CJS/ESM/DTS artifacts build cleanly

Summary

• Nice work — this is a well-scoped spike that meaningfully improves retry/session/multisig ergonomics. Tests/build/lint all pass and the PR message + docs give good context. I'm on the direction; a few questions and small changes remain before merging.

Here's what I reviewed

• Read the PR description and docs/spikes/issue-79-retry-session-multisig.md.
• Verified the intent and the follow-up issues (Backend: add token expiry (expiresIn/expiresAt) to /auth/verify response #82, Implement backend-backed MultiSigStateStore for cross-process signer coordination #83, Consolidate TransactionPipeline's internal retry loop onto utils/retry.ts #84) described in the PR.
• Skimmed the behavioral changes you described (removed dead rpc.ts, session storage adapter + expiresAt/isSessionExpired, MultiSigStateStore + export/import on MultiSigEscrowClient).
• Noted tests all pass (129/129) and build/lint are clean.

What I like

• Clear writeup and decision rationale in docs — makes reviewing the tradeoffs easy.
• Good testing: added tests for session expiry, pluggable storage, Node in-memory default, and multisig export/import.
• Backwards-compatible session API and small non-breaking extension (expiresAt + isSessionExpired).
• Thoughtful separation: introducing MultiSigStateStore interface and keeping export/import as a stopgap is a pragmatic approach.
• You opened follow-up issues for the remaining work (token expiry on backend, backend-backed state store, retry consolidation) — good tracking.

Blocking / high-priority items (please address before merge)

1. Backend token expiry consistency (follow-up Backend: add token expiry (expiresIn/expiresAt) to /auth/verify response #82)
   • The client now persists expiresAt and relies on isSessionExpired. We must ensure the backend returns expiry information or otherwise support the client behavior.
   • Either: delay merging until Backend: add token expiry (expiresIn/expiresAt) to /auth/verify response #82 is implemented, or add a clear doc/compatibility note in README and code comments that the client-side expiresAt is best-effort until /auth/verify returns expiry. If the client relies on expiresAt for security sensitive decisions, prefer blocking until the server-side change exists.
2. Verify removal of src/stellar/rpc.ts is safe
   • You said it was unreferenced, but please run a repo-wide search to confirm there are no remaining references, exports, or published API surface that might rely on it (including examples/docs). If it was ever exported from a barrel file, make sure to update exports.
   • If this file was publicly documented earlier, add a short deprecation note in the spike doc or CHANGELOG entry.

Non-blocking but recommended changes

1. Document Node vs Browser default behavior
   • In the PR you default to a Node in-memory adapter rather than a silent no-op. Please add a short section in README (or docs/spikes) that:
     • Explains the runtime detection strategy (how you detect Node vs browser).
     • States explicitly the default behavior in each environment and the recommended adapter for browser usage (e.g., localStorage adapter).
   • Add a test that verifies the browser-default behavior or add a unit test that simulates both environments to avoid bundler surprises.
2. Note bundler/runtime detection edge cases
   • If detection uses typeof window or process checks, some bundlers or SSR setups can confuse that. Consider a small runtime guard or doc note explaining how to opt-in to an adapter in ambiguous environments (e.g., Next.js SSR).
3. Multisig export/import — docs + conflict expectations
   • Add a short usage example in docs showing exportState/importState lifecycle and describe expected conflict semantics (what happens if two actors import, make divergent changes, and export again). If you have no resolution strategy yet, document that it’s the caller’s responsibility and link to Implement backend-backed MultiSigStateStore for cross-process signer coordination #83.
4. Tests for error/failure paths
   • You added coverage for the happy and some edge cases — please add tests for:
     • Session adapter failures (adapter throws or rejects) and how save/load handles that.
     • isSessionExpired when expiresAt is malformed/missing.
     • Multisig import with malformed state.
5. Small API nit: naming and exports
   • Ensure exported types/interfaces are named/described in index barrel files where appropriate so consumers can import them easily (e.g., MultiSigStateStore).
   • Consider a short JSDoc comment on MultiSigStateStore and MultiSigEscrowClient.exportState/importState describing recommended usage and size/serialization expectations.
6. CHANGELOG entry
   • Add a short CHANGELOG/Unreleased note: summarize the session adapter change, expiresAt support, and multisig export/import so package consumers see the change log.

Optional / future improvements

• Consolidate retry loops onto utils/retry.ts (Consolidate TransactionPipeline's internal retry loop onto utils/retry.ts #84) — you filed this and it will be a good follow-up cleanup.
• Consider an example app (or tests folder example) showing multi-environment usage: browser (localStorage), Node (in-memory), and server-backed state store (mock).
• If the multisig state format may evolve, add a version field to exported state to make future migrations easier.

Tone / wording suggestions for the PR description

• The PR description is already good. Consider adding one-liner "Compatibility & migration" near the top summarizing whether this is a breaking change for any consumers (answer: no, backwards compatible, but runtime behavior changes under Node).

Merge readiness checklist

• Confirm server-side expiry support or add strong compatibility note (address blocking item Setup Tsup Bundler #1).
• Repo-wide check that removed file is truly unused (address blocking item Create Base TrustFlow Client class #2).
• Add the small docs/README entries for session storage behavior and multisig example (recommended).
• Optional: add CHANGELOG entry.

Final assessment

• This is high quality work and the design choices look sound. After addressing the two blockers above (server-side expiry consistency or clear compatibility note, and confirming the dead file removal is safe), I'm comfortable merging.

Thanks — great spike and great tests; the follow-up issues give a clear path to finish the remaining integration work.

[bbjiggy]

Thanks for the thorough review! Pushed a follow-up commit addressing both blockers and most of the recommended items:

Blocking

1. Backend token expiry consistency — added an explicit best-effort compatibility note: JSDoc on Session.expiresAt and saveSession, a "Session Storage" section in the README, and a callout in the spike doc, all stating that client-side expiresAt is a guess (not a guarantee) until Backend: add token expiry (expiresIn/expiresAt) to /auth/verify response #82 lands, and that callers must still handle a 401 regardless of what isSessionExpired() reports. Not blocking merge on Backend: add token expiry (expiresIn/expiresAt) to /auth/verify response #82, per your suggestion.
2. Verified src/stellar/rpc.ts removal is safe — re-ran a repo-wide search (src/, tests/, examples/, README.md, docs/) for stellar/rpc/simulateAndAssemble: zero hits outside this PR's own doc. Checked its git history too (2858c1c) — it was never re-exported from a barrel file and never mentioned in README/API.md, so there's nothing to deprecate. Details in the spike doc.

Also addressed

• Found and fixed a real bug your review prompted me to look for: a malformed/corrupted stored expiresAt was being read as NaN, and Date.now() >= NaN is always false — so a corrupted session looked permanently valid instead of expired. Now treated as already-expired.
• importState() now validates the snapshot shape and returns an SDKResult ({ ok, error }) instead of throwing on malformed input — matches the rest of the class's convention (no thrown exceptions in public APIs, per docs/ARCHITECTURE.md).
• Added tests for: an adapter that throws, a malformed expiresAt, malformed multisig snapshots (missing field, wrong type), and an explicit single-test-file environment-detection case that toggles localStorage presence to prove Node vs browser adapter selection happens per-call.
• README: new "Session Storage (Browser vs Node)" section covering the detection strategy, Node/browser defaults, and SSR/bundler edge cases (calling configureSessionStorage() explicitly for server-rendered code paths); new "Multisig Cross-Process Coordination" section with an exportState/importState usage example and the last-write-wins conflict semantics, linking to Implement backend-backed MultiSigStateStore for cross-process signer coordination #83.
• MultiSigStateStore and SessionStorageAdapter were already exported from the top-level barrel (confirmed in dist/index.d.ts), so no export changes were needed there.
• Added a [Unreleased] CHANGELOG entry summarizing all of the above.

All still green: 134/134 tests, lint clean, build succeeds.

Left as follow-up (not blocking, per your note): the example-app / mock server-backed store idea, and a version field on exported multisig state — happy to fold either into #83 if you'd like them scoped there instead of a new issue.

[meshackyaro]

Thanks for the thorough review! Pushed a follow-up commit addressing both blockers and most of the recommended items:

Blocking

1. Backend token expiry consistency — added an explicit best-effort compatibility note: JSDoc on Session.expiresAt and saveSession, a "Session Storage" section in the README, and a callout in the spike doc, all stating that client-side expiresAt is a guess (not a guarantee) until Backend: add token expiry (expiresIn/expiresAt) to /auth/verify response #82 lands, and that callers must still handle a 401 regardless of what isSessionExpired() reports. Not blocking merge on Backend: add token expiry (expiresIn/expiresAt) to /auth/verify response #82, per your suggestion.
2. Verified src/stellar/rpc.ts removal is safe — re-ran a repo-wide search (src/, tests/, examples/, README.md, docs/) for stellar/rpc/simulateAndAssemble: zero hits outside this PR's own doc. Checked its git history too (2858c1c) — it was never re-exported from a barrel file and never mentioned in README/API.md, so there's nothing to deprecate. Details in the spike doc.

Also addressed

• Found and fixed a real bug your review prompted me to look for: a malformed/corrupted stored expiresAt was being read as NaN, and Date.now() >= NaN is always false — so a corrupted session looked permanently valid instead of expired. Now treated as already-expired.
• importState() now validates the snapshot shape and returns an SDKResult ({ ok, error }) instead of throwing on malformed input — matches the rest of the class's convention (no thrown exceptions in public APIs, per docs/ARCHITECTURE.md).
• Added tests for: an adapter that throws, a malformed expiresAt, malformed multisig snapshots (missing field, wrong type), and an explicit single-test-file environment-detection case that toggles localStorage presence to prove Node vs browser adapter selection happens per-call.
• README: new "Session Storage (Browser vs Node)" section covering the detection strategy, Node/browser defaults, and SSR/bundler edge cases (calling configureSessionStorage() explicitly for server-rendered code paths); new "Multisig Cross-Process Coordination" section with an exportState/importState usage example and the last-write-wins conflict semantics, linking to Implement backend-backed MultiSigStateStore for cross-process signer coordination #83.
• MultiSigStateStore and SessionStorageAdapter were already exported from the top-level barrel (confirmed in dist/index.d.ts), so no export changes were needed there.
• Added a [Unreleased] CHANGELOG entry summarizing all of the above.

All still green: 134/134 tests, lint clean, build succeeds.

Left as follow-up (not blocking, per your note): the example-app / mock server-backed store idea, and a version field on exported multisig state — happy to fold either into #83 if you'd like them scoped there instead of a new issue.

Thanks — nice follow-up and good cleanup across the spike.

What I looked for

• Session: pluggable SessionStorageAdapter, Node in-memory default, persisted expiresAt, and isSessionExpired().
• Retry: removal of dead simulateAndAssemble / src/stellar/rpc.ts and retained utils/retry.ts.
• Multisig: added MultiSigStateStore interface and exportState/importState on MultiSigEscrowClient.
• Tests/lint/build: all green per your description.

Ship-it with a couple of small follow-ups before I merge:

1. Session expiry backward-compatibility: please explicitly handle the case where saved sessions (or /auth/verify responses) do not contain expiresAt. Either:

   • keep the current behavior but add a short code comment in auth/session.ts documenting that expiresAt is optional and what isSessionExpired() returns when expiresAt is missing, and add one unit test proving the behavior; OR
   • if you already treat absent expiresAt as “not expired”, add the explicit test and a comment to avoid future regressions.

2. Multisig exported-state versioning: add a lightweight version field to the exported state shape (e.g. { version: 1, ...state }) and a short comment in src/types/multisig.ts describing the version negotiation intent. That will make future schema changes non-breaking and is cheap to add now.

3. Doc/update note: add a one-line entry to docs/spikes/issue-79-retry-session-multisig.md (or a CHANGELOG entry) summarizing the breaking/changed surface (removed rpc.ts, new session adapter, export/import API) so downstream consumers know what changed.

4. Small housekeeping: run a quick repo-wide search to confirm there are no lingering references to src/stellar/rpc.ts (looks fine from the tests, but worth double-checking) and add a brief note in the commit message/body referencing the follow-up issues (Backend: add token expiry (expiresIn/expiresAt) to /auth/verify response #82–Consolidate TransactionPipeline's internal retry loop onto utils/retry.ts #84) so the PR history is explicit.

None of these are blockers for the approach — tests + build passing and the follow-up issues filed are exactly the right pattern. If you want I can push a tiny follow-up commit that adds the exported-state version field and the missing-session-expires test; otherwise please add the two small changes above and I’ll approve/merge.

[bbjiggy]

Pushed both requested changes:

1. Session expiry backward-compatibility — added an explicit comment + unit test in auth/session.ts/tests/auth.test.ts for the "no stored expiresAt key at all" case (a session written before expiry tracking existed, or by an older SDK version): loadSession() computes a fresh default TTL from now, so upgrading doesn't retroactively expire pre-existing sessions. This is distinct from — and now explicitly comment-documented next to — the malformed-value case (treated as already-expired, from the previous round).

2. Multisig exported-state versioning — added MULTISIG_SNAPSHOT_VERSION (currently 1) in src/types/multisig.ts, included automatically on every exportState() snapshot, and importState() now rejects a missing or mismatched version outright with a clear error rather than guessing at an unfamiliar shape. Added tests for both the missing-version and wrong-version cases. Comment on the constant describes the version-negotiation intent for when the shape needs to change later.

3. Doc/update note — added a "Compatibility & migration" line at the top of the spike doc and a "Breaking changes: none" line at the top of the [Unreleased] CHANGELOG entry, both cross-referencing Backend: add token expiry (expiresIn/expiresAt) to /auth/verify response #82/Implement backend-backed MultiSigStateStore for cross-process signer coordination #83/Consolidate TransactionPipeline's internal retry loop onto utils/retry.ts #84.

4. Housekeeping — re-ran the repo-wide search for stellar/rpc/simulateAndAssemble, still zero hits outside this PR's own doc.

136/136 tests passing, lint clean, build succeeds.

[meshackyaro]

Pushed both requested changes:

1. Session expiry backward-compatibility — added an explicit comment + unit test in auth/session.ts/tests/auth.test.ts for the "no stored expiresAt key at all" case (a session written before expiry tracking existed, or by an older SDK version): loadSession() computes a fresh default TTL from now, so upgrading doesn't retroactively expire pre-existing sessions. This is distinct from — and now explicitly comment-documented next to — the malformed-value case (treated as already-expired, from the previous round).
2. Multisig exported-state versioning — added MULTISIG_SNAPSHOT_VERSION (currently 1) in src/types/multisig.ts, included automatically on every exportState() snapshot, and importState() now rejects a missing or mismatched version outright with a clear error rather than guessing at an unfamiliar shape. Added tests for both the missing-version and wrong-version cases. Comment on the constant describes the version-negotiation intent for when the shape needs to change later.
3. Doc/update note — added a "Compatibility & migration" line at the top of the spike doc and a "Breaking changes: none" line at the top of the [Unreleased] CHANGELOG entry, both cross-referencing Backend: add token expiry (expiresIn/expiresAt) to /auth/verify response #82/Implement backend-backed MultiSigStateStore for cross-process signer coordination #83/Consolidate TransactionPipeline's internal retry loop onto utils/retry.ts #84.
4. Housekeeping — re-ran the repo-wide search for stellar/rpc/simulateAndAssemble, still zero hits outside this PR's own doc.

136/136 tests passing, lint clean, build succeeds.

Thanks — this is an excellent, thorough spike and implementation. A few highlights I appreciated:

• Cleaned up dead code (removed simulateAndAssemble) and re-audited retry behavior instead of re-implementing it.
• Session lifecycle improvements are well-designed: pluggable SessionStorageAdapter, Node in-memory default (no silent no-op), and the new isSessionExpired helper while remaining backwards-compatible with saveSession.
• Multisig state approach is practical: a clear MultiSigStateStore interface plus non-breaking exportState/importState on MultiSigEscrowClient to bridge to an external store today.
• Tests, lint, and build all pass in CI (129 tests) and you added coverage for the new behaviors — excellent.
• Nice follow-up issue tracking (Backend: add token expiry (expiresIn/expiresAt) to /auth/verify response #82, Implement backend-backed MultiSigStateStore for cross-process signer coordination #83, Consolidate TransactionPipeline's internal retry loop onto utils/retry.ts #84) so implementation work is decoupled and actionable.

Non-blocking suggestions:

• Consider a short note in docs (or the spike doc) that explicitly documents the Node default SessionStorageAdapter and how to plug a backend adapter — makes the migration story easier for integrators.
• Optionally add a one-line changelog entry mentioning the session default and multisig export/import so users notice the behavioral change.

Overall: Looks good to me — this is ready to approve/merge from my side once you’re happy with the small docs/changelog follow-ups.

[bbjiggy]

Thanks for the kind words and the careful re-review through all three rounds!

Both non-blocking suggestions are actually already in place from the earlier rounds:

• README's "Session Storage (Browser vs Node)" section documents the Node in-memory default and shows the configureSessionStorage() call for plugging in a backend adapter.
• The [Unreleased] CHANGELOG entry already calls out the session default behavior change and the multisig exportState/importState addition.

One gap I do see, though: the spike doc itself (docs/spikes/issue-79-retry-session-multisig.md §3) describes the Node-default recommendation in prose but doesn't cross-link to the README section with the actual adapter-injection example — so someone reading only the spike doc wouldn't find the "how" as easily as someone reading the README. I'll add that cross-reference now and push a small follow-up commit, then report back here once it's in and CI is green.

[bbjiggy]

Pushed the cross-reference: docs/spikes/issue-79-retry-session-multisig.md §3 now links directly to the README's "Session Storage (Browser vs Node)" section for the configureSessionStorage() example and SSR/bundler notes, right where it describes the Node-default recommendation.

136/136 tests passing, lint clean, build succeeds. This is a docs-only change, so no behavioral risk. Let me know if there's anything else — otherwise this should be ready from my side.

[meshackyaro]

Thank you — this is an excellent follow-up and a solid, well-constructed and well-tested implementation of the spike. I’m happy to approve this for merge. Great Job!