From a80aebef1519dc316ac2e6c3d16c9ae1a2cf4649 Mon Sep 17 00:00:00 2001 From: Jiggy <142459849+bbjiggy@users.noreply.github.com> Date: Mon, 17 Aug 2026 19:08:29 +0100 Subject: [PATCH 1/5] docs(spike): resolve retry/session/multisig spike for issue #79 Audits the retry, session, and multisig state gaps described in #79, finds most of the retry gap already closed by the tx-pipeline and axios-retry work, and proposes/prototypes the rest: - Retry: remove src/stellar/rpc.ts, dead and unretried code fully superseded by TransactionPipeline. Document an idempotency policy per call type (simulate/prepare safe to retry, submit is not). - Session: make storage pluggable via a SessionStorageAdapter, add a Node-safe in-memory default instead of the previous silent no-op, and add expiry metadata plus isSessionExpired(). - Multisig: define the target MultiSigStateStore abstraction for cross-process coordination, and add exportState/importState to MultiSigEscrowClient as a non-breaking stopgap ahead of a native backend-backed store. Full writeup, idempotency table, and blocking unknowns in docs/spikes/issue-79-retry-session-multisig.md. Closes #79 --- .../spikes/issue-79-retry-session-multisig.md | 125 ++++++++++++++++++ src/auth/session.ts | 125 +++++++++++++++--- src/escrow/multisig.ts | 38 ++++++ src/stellar/rpc.ts | 20 --- src/types/multisig.ts | 26 ++++ tests/auth.test.ts | 55 +++++++- tests/multisig.test.ts | 78 +++++++++++ tests/session-node-default.test.ts | 24 ++++ 8 files changed, 455 insertions(+), 36 deletions(-) create mode 100644 docs/spikes/issue-79-retry-session-multisig.md delete mode 100644 src/stellar/rpc.ts create mode 100644 tests/session-node-default.test.ts diff --git a/docs/spikes/issue-79-retry-session-multisig.md b/docs/spikes/issue-79-retry-session-multisig.md new file mode 100644 index 0000000..7cd2e9c --- /dev/null +++ b/docs/spikes/issue-79-retry-session-multisig.md @@ -0,0 +1,125 @@ +# Spike: retry/resilience policy, auth/session lifecycle, multisig state coordination + +Tracking issue: [#79](https://github.com/trustflow-protocol/trustflow-sdk/issues/79) + +## 1. Current-state audit + +The issue was filed against an earlier snapshot of the code. Since then `TransactionPipeline` +(`src/tx-pipeline/pipeline.ts`) and `createApiHttpClient` (`src/utils/http.ts`) landed and already +cover a meaningful slice of the retry gap. This spike re-audits what's actually missing before +proposing changes. + +| Call site | Retry today | Notes | +|---|---|---| +| `auth/challenge.ts` (`requestChallenge`, `verifyAndGetToken`) | Yes — via `createApiHttpClient`'s `axios-retry` config | Retries network errors, `429`, `5xx`. Does **not** retry `4xx` auth failures, which is correct. | +| `TransactionPipeline.prepare` (Soroban `simulateTransaction`) | Yes — local `withRetry`, exponential backoff | Simulation is read-only, safe to retry. | +| `TransactionPipeline.submit` (Soroban `sendTransaction` + poll) | Yes — local `withRetry`, escalates to fee-bump on fee-related errors | Distinguishes `TRY_AGAIN_LATER` (safe to retry) from on-chain `FAILED` (not retried, surfaced immediately). | +| `stellar/rpc.ts` (`simulateAndAssemble`) | **No** | Confirmed dead: not imported by any module, not re-exported from `src/stellar/index.ts` or `src/index.ts`, no test references it. Fully superseded by `TransactionPipeline.prepare`. | +| `src/utils/retry.ts` (`retry()`) | N/A — generic helper | Exported from `src/utils/index.ts` (public API surface) but never called from within `src/`. Has its own unit tests (`tests/retry.test.ts`), so it is a documented public utility, not dead code — just unused internally. | + +**Conclusion:** the "dead retry code" gap described in the issue was mostly closed by the tx-pipeline +work. What's left is cleanup (remove the genuinely dead, unretried `simulateAndAssemble`) and a +documented idempotency policy so future call sites are wired correctly instead of by accident. + +## 2. Retry / resilience policy (recommendation) + +Idempotency rules per call type: + +| Call type | Safe to blindly retry? | Why | +|---|---|---| +| `simulateTransaction` / `simulateAndAssemble` | Yes, always | Read-only, no state mutation on-chain or on the backend. | +| `prepare` (simulate + assemble resource fee) | Yes | Same as above — no submission happens in this step. | +| `sendTransaction` returning `TRY_AGAIN_LATER` | Yes | Node explicitly signals the tx was not accepted into its queue; nothing was broadcast. | +| `sendTransaction` returning `ERROR` / on-chain `FAILED` | **No** | The tx may already be included; blind resubmission risks confusing double-submit semantics. `TransactionPipeline` already does the right thing: it does not retry these, it surfaces them so the caller can decide (e.g. escalate to fee-bump). | +| Backend REST calls (`/auth/challenge`, `/auth/verify`) | Yes for network/`429`/`5xx` only | `4xx` (bad signature, unknown address) is a client error, not transient — retrying wastes time and can trip rate limits. `axios-retry`'s condition already encodes this correctly. | + +Actions taken in this PR: +- Removed `src/stellar/rpc.ts` — dead code, zero retry/timeout handling, fully superseded by + `TransactionPipeline`. +- Kept `src/utils/retry.ts` as-is: it's a legitimate public generic-purpose utility with its own + tests: removing it would be a breaking change for consumers who may already depend on it. + +Follow-up (not done in this spike, filed as a separate issue): `TransactionPipeline`'s internal +`withRetry` duplicates the backoff loop in `utils/retry.ts` with a different signature (attempt +callback, policy object). Worth consolidating so there is one retry primitive, but that's a +refactor of tested, shipped code and deserves its own review rather than riding along on a spike. + +## 3. Session storage & token lifecycle (recommendation) + +Problems in `auth/session.ts` today: +- `localStorage`-only; every call is a silent no-op under Node (CLI/backend integrators), which + looks like it "works" (no exception) but never persists anything. +- No expiry metadata is stored alongside the token, so nothing can tell the SDK the session is + stale until the backend itself returns a `401`. + +**Recommendation, implemented as a prototype in this PR:** +- Introduce a `SessionStorageAdapter` interface (`get`/`set`/`remove`) and make storage pluggable + via `configureSessionStorage()`. +- Auto-select a sane default per environment: `localStorage` in the browser (unchanged behavior), + an in-memory adapter under Node instead of a silent no-op — at least the token now survives for + the lifetime of the process instead of vanishing immediately. Node/CLI/backend integrators who + need durability across process restarts (e.g. a long-running server) should inject their own + adapter (file-backed, Redis, keytar, etc.) via `configureSessionStorage()` — that dependency + doesn't belong in the SDK itself. +- Add optional `expiresAt` to the persisted session and an `isSessionExpired()` helper so callers + can proactively re-run the challenge flow instead of waiting for a `401`. + +**Blocking unknown:** the backend's `/auth/verify` response currently only returns `{ token }` +with no TTL. Without a backend-supplied expiry, the SDK cannot know the *real* token lifetime — it +can only apply a conservative client-side default (implemented here as 15 minutes, configurable) +and treat that as a lower bound, not a guarantee. **Needs backend coordination**: add +`expiresIn`/`expiresAt` to the `/auth/verify` response. Flagged as a follow-up issue. + +## 4. Multisig operation-state coordination (recommendation) + +`MultiSigEscrowClient` keeps operation state in an in-memory `Map`, scoped to one process. Per the +README, signers are expected to submit their signed XDR independently — which requires state +visible across processes. + +Options considered: +1. **Backend-persisted store (recommended).** The SDK already talks to a TrustFlow backend for + auth; extending it with multisig-operation endpoints (create / add-signature / get-status) is + the natural home. All signer processes read/write through the same backend, which already has + the auth/session machinery to authorize who can contribute a signature. +2. **On-chain.** Not applicable here — this is off-chain signature collection over an assembled + Soroban transaction, not a native multisig account primitive. Storing partial signature sets + on-chain isn't possible before the transaction is submitted. +3. **Dedicated relay/pub-sub service.** Would work but is extra infrastructure the project doesn't + have today, solving a problem the existing backend can already solve. + +**Decision: option 1.** This spike does **not** implement a backend-backed store — that requires +new backend endpoints that don't exist yet, which is real implementation work, not a spike +prototype. Instead, this PR: +- Defines the target abstraction, `MultiSigStateStore` (see `src/types/multisig.ts`), documenting + the interface a future backend-backed implementation must satisfy, with the current in-memory + map as the reference default/local-testing implementation. +- Adds `exportState()` / `importState()` to `MultiSigEscrowClient` as a stopgap: it lets an + integrator serialize an operation's state out of one process and rehydrate it in another (e.g. + by round-tripping it through their own backend today) without waiting for the SDK to grow native + async storage. This is deliberately additive — it does not change any existing method's + signature or behavior, so it doesn't destabilize the tested sync API multisig consumers already + depend on. +- Full async, pluggable `MultiSigStateStore` wiring into `MultiSigEscrowClient` (which is a + breaking API change, since every method would become `Promise`-returning) is left to the + follow-up implementation issue, once the backend endpoints exist to back it. + +## 5. Follow-up implementation issues filed + +- Backend: add `expiresIn`/`expiresAt` to the `/auth/verify` response so the SDK can trust a real + token TTL instead of a client-side default. +- SDK: implement a backend-backed `MultiSigStateStore` and wire it into `MultiSigEscrowClient` + (async API — breaking change, needs a major version bump) once the corresponding backend + endpoints exist. +- SDK: consolidate `TransactionPipeline`'s internal `withRetry` on top of `src/utils/retry.ts` to + remove the duplicated backoff implementation. + +(Links added once the issues are created — see PR description.) + +## 6. Blocking unknowns + +- Real token TTL is unknown until the backend team confirms whether/when `/auth/verify` will + return an expiry. Client-side default (15 min) is a guess, not a guarantee. +- Whether multisig coordination should be a new set of REST endpoints on the existing TrustFlow + backend, or a separate service, is a product/infra decision outside this SDK repo's scope — + recommendation above assumes reusing the existing backend, but that needs sign-off from whoever + owns it. diff --git a/src/auth/session.ts b/src/auth/session.ts index bd0e2bd..ed2252d 100644 --- a/src/auth/session.ts +++ b/src/auth/session.ts @@ -1,30 +1,125 @@ const TOKEN_KEY = 'trustflow_token'; const ADDRESS_KEY = 'trustflow_address'; +const EXPIRES_AT_KEY = 'trustflow_expires_at'; -export function saveSession(token: string, address: string): void { - if (typeof localStorage === 'undefined') { - return; +/** Default client-side token lifetime, used only when the backend doesn't supply one. */ +const DEFAULT_SESSION_TTL_MS = 15 * 60_000; + +export interface SessionStorageAdapter { + get(key: string): string | null; + set(key: string, value: string): void; + remove(key: string): void; +} + +/** Browser adapter — unchanged behavior from before this session redesign. */ +class LocalStorageAdapter implements SessionStorageAdapter { + get(key: string): string | null { + return localStorage.getItem(key); + } + set(key: string, value: string): void { + localStorage.setItem(key, value); + } + remove(key: string): void { + localStorage.removeItem(key); } - localStorage.setItem(TOKEN_KEY, token); - localStorage.setItem(ADDRESS_KEY, address); } -export function loadSession(): { token: string; address: string } | null { - if (typeof localStorage === 'undefined') { - return null; +/** + * Process-lifetime fallback for Node/CLI/backend usage. + * + * This does NOT survive process restarts. Integrators that need durability + * (long-running servers, CLIs invoked repeatedly) should call + * `configureSessionStorage()` with their own adapter (file-backed, Redis, + * keytar, etc.) — that dependency choice belongs to the integrator, not the SDK. + */ +class InMemoryStorageAdapter implements SessionStorageAdapter { + private readonly store = new Map(); + get(key: string): string | null { + return this.store.get(key) ?? null; + } + set(key: string, value: string): void { + this.store.set(key, value); + } + remove(key: string): void { + this.store.delete(key); + } +} + +// Falls back to the in-memory adapter for the lifetime of the process the first +// time it's needed; resolved lazily (not at module load) so environment detection +// reflects the actual environment at call time, not at import time. +let inMemoryFallback: SessionStorageAdapter | undefined; +let override: SessionStorageAdapter | undefined; + +function getStorage(): SessionStorageAdapter { + if (override) { + return override; + } + if (typeof localStorage !== 'undefined') { + return new LocalStorageAdapter(); } - const token = localStorage.getItem(TOKEN_KEY); - const address = localStorage.getItem(ADDRESS_KEY); + return (inMemoryFallback ??= new InMemoryStorageAdapter()); +} + +/** + * Overrides the storage backend used for session persistence. + * Intended for Node/CLI/backend integrators who need durability across + * process restarts, and for tests. + */ +export function configureSessionStorage(adapter: SessionStorageAdapter): void { + override = adapter; +} + +/** Resets the storage backend to the environment default (browser localStorage or in-memory). */ +export function resetSessionStorage(): void { + override = undefined; + inMemoryFallback = undefined; +} + +export interface Session { + token: string; + address: string; + /** UNIX ms timestamp after which the token should be treated as stale. */ + expiresAt: number; +} + +/** + * Persists a session token. + * + * @param expiresAt - UNIX ms timestamp when the token expires. Defaults to + * `DEFAULT_SESSION_TTL_MS` from now when omitted, since the backend does + * not currently return a token TTL (see docs/spikes/issue-79-retry-session-multisig.md). + */ +export function saveSession(token: string, address: string, expiresAt?: number): void { + const storage = getStorage(); + storage.set(TOKEN_KEY, token); + storage.set(ADDRESS_KEY, address); + storage.set(EXPIRES_AT_KEY, String(expiresAt ?? Date.now() + DEFAULT_SESSION_TTL_MS)); +} + +export function loadSession(): Session | null { + const storage = getStorage(); + const token = storage.get(TOKEN_KEY); + const address = storage.get(ADDRESS_KEY); if (!token || !address) { return null; } - return { token, address }; + const expiresAtRaw = storage.get(EXPIRES_AT_KEY); + const expiresAt = expiresAtRaw ? Number(expiresAtRaw) : Date.now() + DEFAULT_SESSION_TTL_MS; + return { token, address, expiresAt }; } export function clearSession(): void { - if (typeof localStorage === 'undefined') { - return; + const storage = getStorage(); + storage.remove(TOKEN_KEY); + storage.remove(ADDRESS_KEY); + storage.remove(EXPIRES_AT_KEY); +} + +/** True when the stored session is missing or past its `expiresAt`. */ +export function isSessionExpired(session: Session | null = loadSession()): boolean { + if (!session) { + return true; } - localStorage.removeItem(TOKEN_KEY); - localStorage.removeItem(ADDRESS_KEY); + return Date.now() >= session.expiresAt; } diff --git a/src/escrow/multisig.ts b/src/escrow/multisig.ts index 9812a6a..391225a 100644 --- a/src/escrow/multisig.ts +++ b/src/escrow/multisig.ts @@ -11,6 +11,7 @@ import type { GetStatusResult, SubmitMultiSigResult, GetXdrResult, + MultiSigStateSnapshot, } from '../types/multisig'; import { submitTransaction } from '../stellar/transaction'; @@ -229,6 +230,43 @@ export class MultiSigEscrowClient { return Array.from(this.operations.values()).filter((op) => op.escrowId === escrowId); } + /** + * Serializes one operation's state so it can be handed to an external + * store (e.g. an integrator's own backend) and later restored via + * `importState`, letting independent signer processes coordinate without + * sharing this client's in-memory `Map`. + * + * Stopgap ahead of a native `MultiSigStateStore` — see + * docs/spikes/issue-79-retry-session-multisig.md. + * + * @param operationId - ID returned by `initMultiSigOperation` + */ + exportState(operationId: string): MultiSigStateSnapshot | undefined { + const operation = this.operations.get(operationId); + return operation + ? { + ...operation, + signers: [...operation.signers], + collectedSignatures: [...operation.collectedSignatures], + } + : undefined; + } + + /** + * Restores a previously-exported operation snapshot into this client, + * making it available to subsequent `addSignature` / `getMultiSigStatus` + * / `submitWhenReady` calls in this process. + * + * @param snapshot - A value previously returned by `exportState` + */ + importState(snapshot: MultiSigStateSnapshot): void { + this.operations.set(snapshot.operationId, { + ...snapshot, + signers: [...snapshot.signers], + collectedSignatures: [...snapshot.collectedSignatures], + }); + } + // --------------------------------------------------------------------------- // Private helpers // --------------------------------------------------------------------------- diff --git a/src/stellar/rpc.ts b/src/stellar/rpc.ts deleted file mode 100644 index c76cffc..0000000 --- a/src/stellar/rpc.ts +++ /dev/null @@ -1,20 +0,0 @@ -export async function simulateAndAssemble( - rpcUrl: string, - txXdr: string, -): Promise<{ xdr: string; cost: { cpuInsns: string; memBytes: string } }> { - const res = await fetch(rpcUrl, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ - jsonrpc: '2.0', - id: 1, - method: 'simulateTransaction', - params: { transaction: txXdr }, - }), - }); - const { result } = await res.json(); - if (result.error) { - throw new Error(result.error.message); - } - return { xdr: result.transactionData, cost: result.cost ?? { cpuInsns: '0', memBytes: '0' } }; -} diff --git a/src/types/multisig.ts b/src/types/multisig.ts index a40a247..5d87906 100644 --- a/src/types/multisig.ts +++ b/src/types/multisig.ts @@ -105,3 +105,29 @@ export type AddSignatureResult = SDKResult; export type GetStatusResult = SDKResult; export type SubmitMultiSigResult = SDKResult; export type GetXdrResult = SDKResult<{ xdr: string }>; + +/** + * Target abstraction for coordinating multisig operation state across + * independent signer processes (e.g. backed by the TrustFlow backend's REST + * API), as recommended in docs/spikes/issue-79-retry-session-multisig.md. + * + * Not yet wired into `MultiSigEscrowClient` — that requires backend endpoints + * that don't exist yet, and would be a breaking (sync -> async) API change. + * Tracked as a follow-up implementation issue. The client's default, + * in-process store today is a plain `Map`, which satisfies this shape + * synchronously. + */ +export interface MultiSigStateStore { + get(operationId: string): Promise; + set(operationId: string, operation: MultiSigOperation): Promise; + delete(operationId: string): Promise; + listByEscrow(escrowId: EscrowId): Promise; +} + +/** + * Serializable snapshot of one multisig operation, for round-tripping state + * through an external store (e.g. an integrator's own backend) between + * `MultiSigEscrowClient.exportState` / `importState` calls, ahead of native + * `MultiSigStateStore` support. + */ +export type MultiSigStateSnapshot = MultiSigOperation; diff --git a/tests/auth.test.ts b/tests/auth.test.ts index 87c8a79..1f921d4 100644 --- a/tests/auth.test.ts +++ b/tests/auth.test.ts @@ -1,4 +1,12 @@ -import { saveSession, loadSession, clearSession } from '../src/auth/session'; +import { + saveSession, + loadSession, + clearSession, + isSessionExpired, + configureSessionStorage, + resetSessionStorage, + SessionStorageAdapter, +} from '../src/auth/session'; describe('Session management', () => { const mockStorage: Record = {}; @@ -22,4 +30,49 @@ describe('Session management', () => { clearSession(); expect(loadSession()).toBeNull(); }); + + describe('token expiry', () => { + afterEach(() => clearSession()); + + it('defaults to a non-expired session when no expiresAt is given', () => { + saveSession('tok123', 'GABC'); + expect(isSessionExpired()).toBe(false); + }); + + it('honors an explicit expiresAt in the past', () => { + saveSession('tok123', 'GABC', Date.now() - 1000); + expect(isSessionExpired()).toBe(true); + }); + + it('honors an explicit expiresAt in the future', () => { + saveSession('tok123', 'GABC', Date.now() + 60_000); + expect(isSessionExpired()).toBe(false); + }); + + it('treats a missing session as expired', () => { + expect(isSessionExpired()).toBe(true); + }); + }); + + describe('configureSessionStorage', () => { + afterEach(() => resetSessionStorage()); + + it('routes reads/writes through an injected adapter', () => { + const backing: Record = {}; + const adapter: SessionStorageAdapter = { + get: (k) => backing[k] ?? null, + set: (k, v) => { backing[k] = v; }, + remove: (k) => { delete backing[k]; }, + }; + configureSessionStorage(adapter); + + saveSession('custom-tok', 'GXYZ'); + expect(loadSession()?.token).toBe('custom-tok'); + expect(backing['trustflow_token']).toBe('custom-tok'); + + // The globally-mocked localStorage from the outer describe block must + // not have been touched while the override is active. + expect(mockStorage['trustflow_token']).not.toBe('custom-tok'); + }); + }); }); diff --git a/tests/multisig.test.ts b/tests/multisig.test.ts index f2b4031..c7e2018 100644 --- a/tests/multisig.test.ts +++ b/tests/multisig.test.ts @@ -498,4 +498,82 @@ describe('MultiSigEscrowClient', () => { expect(client.listOperations('no-such-escrow')).toHaveLength(0); }); }); + + // ------------------------------------------------------------------------- + // exportState / importState + // ------------------------------------------------------------------------- + describe('exportState / importState', () => { + it('returns undefined for an unknown operationId', () => { + expect(client.exportState('no-such-op')).toBeUndefined(); + }); + + it('round-trips operation state through export/import into a fresh client', () => { + const init = client.initMultiSigOperation({ + escrowId: ESCROW_ID, + signers: [KP_A.publicKey(), KP_B.publicKey()], + threshold: 2, + operationType: 'release', + unsignedXdr: BASE_XDR, + networkPassphrase: NETWORK_PASSPHRASE, + }); + expect(init.ok).toBe(true); + if (!init.ok) return; + const operationId = init.data.operationId; + + client.addSignature({ operationId, signerAddress: KP_A.publicKey(), signedXdr: SIGNED_A }); + + const snapshot = client.exportState(operationId); + expect(snapshot).toBeDefined(); + if (!snapshot) return; + + const otherClient = new MultiSigEscrowClient(CONTRACT_CONFIG); + expect(otherClient.getMultiSigStatus(operationId).ok).toBe(false); + + otherClient.importState(snapshot); + + const status = otherClient.getMultiSigStatus(operationId); + expect(status.ok).toBe(true); + if (status.ok) { + expect(status.data.signaturesCollected).toBe(1); + expect(status.data.signersSigned).toContain(KP_A.publicKey()); + } + + // Continuing the flow on the second process's client should work as normal. + const completed = otherClient.addSignature({ + operationId, + signerAddress: KP_B.publicKey(), + signedXdr: SIGNED_B, + }); + expect(completed.ok).toBe(true); + if (completed.ok) { + expect(completed.data.isReady).toBe(true); + } + }); + + it('does not mutate the exporting client when the importing client is mutated', () => { + const init = client.initMultiSigOperation({ + escrowId: ESCROW_ID, + signers: [KP_A.publicKey(), KP_B.publicKey()], + threshold: 2, + operationType: 'release', + unsignedXdr: BASE_XDR, + networkPassphrase: NETWORK_PASSPHRASE, + }); + expect(init.ok).toBe(true); + if (!init.ok) return; + const operationId = init.data.operationId; + + const snapshot = client.exportState(operationId)!; + const otherClient = new MultiSigEscrowClient(CONTRACT_CONFIG); + otherClient.importState(snapshot); + + otherClient.addSignature({ operationId, signerAddress: KP_A.publicKey(), signedXdr: SIGNED_A }); + + const original = client.getMultiSigStatus(operationId); + expect(original.ok).toBe(true); + if (original.ok) { + expect(original.data.signaturesCollected).toBe(0); + } + }); + }); }); diff --git a/tests/session-node-default.test.ts b/tests/session-node-default.test.ts new file mode 100644 index 0000000..cbe626d --- /dev/null +++ b/tests/session-node-default.test.ts @@ -0,0 +1,24 @@ +import { saveSession, loadSession, clearSession } from '../src/auth/session'; + +// Deliberately does NOT mock `localStorage` — this file verifies the Node +// fallback (in-memory adapter) that replaces the old silent no-op behavior. +describe('Session management (Node default, no localStorage)', () => { + beforeAll(() => { + expect(typeof (global as any).localStorage).toBe('undefined'); + }); + + afterEach(() => clearSession()); + + it('persists sessions in-memory for the lifetime of the process', () => { + saveSession('node-tok', 'GNODE'); + const s = loadSession(); + expect(s?.token).toBe('node-tok'); + expect(s?.address).toBe('GNODE'); + }); + + it('clears the in-memory session', () => { + saveSession('node-tok', 'GNODE'); + clearSession(); + expect(loadSession()).toBeNull(); + }); +}); From 84b6b8044da932325325d769e9c36cde60de1434 Mon Sep 17 00:00:00 2001 From: Jiggy <142459849+bbjiggy@users.noreply.github.com> Date: Mon, 17 Aug 2026 19:26:25 +0100 Subject: [PATCH 2/5] docs(spike): link filed follow-up issues in the spike writeup --- docs/spikes/issue-79-retry-session-multisig.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/spikes/issue-79-retry-session-multisig.md b/docs/spikes/issue-79-retry-session-multisig.md index 7cd2e9c..6d66400 100644 --- a/docs/spikes/issue-79-retry-session-multisig.md +++ b/docs/spikes/issue-79-retry-session-multisig.md @@ -105,15 +105,15 @@ prototype. Instead, this PR: ## 5. Follow-up implementation issues filed -- Backend: add `expiresIn`/`expiresAt` to the `/auth/verify` response so the SDK can trust a real - token TTL instead of a client-side default. -- SDK: implement a backend-backed `MultiSigStateStore` and wire it into `MultiSigEscrowClient` - (async API — breaking change, needs a major version bump) once the corresponding backend - endpoints exist. -- SDK: consolidate `TransactionPipeline`'s internal `withRetry` on top of `src/utils/retry.ts` to - remove the duplicated backoff implementation. - -(Links added once the issues are created — see PR description.) +- [#82](https://github.com/trustflow-protocol/trustflow-sdk/issues/82) — Backend: add + `expiresIn`/`expiresAt` to the `/auth/verify` response so the SDK can trust a real token TTL + instead of a client-side default. +- [#83](https://github.com/trustflow-protocol/trustflow-sdk/issues/83) — SDK: implement a + backend-backed `MultiSigStateStore` and wire it into `MultiSigEscrowClient` (async API — + breaking change, needs a major version bump) once the corresponding backend endpoints exist. +- [#84](https://github.com/trustflow-protocol/trustflow-sdk/issues/84) — SDK: consolidate + `TransactionPipeline`'s internal `withRetry` on top of `src/utils/retry.ts` to remove the + duplicated backoff implementation. ## 6. Blocking unknowns From 82d0864725051c44917291a7ae9202ac6ed4a909 Mon Sep 17 00:00:00 2001 From: Jiggy <142459849+bbjiggy@users.noreply.github.com> Date: Tue, 18 Aug 2026 03:14:51 +0100 Subject: [PATCH 3/5] =?UTF-8?q?fix(sdk):=20address=20review=20=E2=80=94=20?= =?UTF-8?q?malformed=20expiry,=20importState=20validation,=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses meshackyaro's review on the retry/session/multisig spike PR: Blocking: - Add an explicit best-effort compatibility note (JSDoc on Session/ saveSession, README, spike doc) for the client-side expiresAt default, since the backend doesn't return a token TTL yet (#82). - Re-verify and document that removing src/stellar/rpc.ts has no public API or documentation footprint (never re-exported, never mentioned in README/API.md, no tests). Also: - Fix isSessionExpired() treating a malformed stored expiresAt as non-expiring forever; it's now treated as already-expired. - Make importState() validate the snapshot shape and return an SDKResult instead of throwing, matching the rest of the class's error-handling convention (no thrown exceptions in public APIs). - Add tests for adapter failures, malformed expiresAt, malformed multisig snapshots, and explicit Node/browser environment-detection switching within a single test. - Document session storage environment detection (incl. SSR/bundler caveats) and multisig exportState/importState usage + conflict semantics in the README; add a CHANGELOG entry. --- CHANGELOG.md | 15 +++++ README.md | 59 +++++++++++++++++++ .../spikes/issue-79-retry-session-multisig.md | 29 ++++++++- src/auth/session.ts | 24 ++++++-- src/escrow/multisig.ts | 54 ++++++++++++++++- src/types/multisig.ts | 2 + tests/auth.test.ts | 29 +++++++++ tests/multisig.test.ts | 39 +++++++++++- tests/session-node-default.test.ts | 31 ++++++++++ 9 files changed, 274 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5c7e9c..ed07a43 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,20 @@ # Changelog +## [Unreleased] +- Session storage (`auth/session.ts`) is now pluggable via a `SessionStorageAdapter` and + `configureSessionStorage()`. Node/CLI/backend usage now defaults to an in-memory adapter + instead of silently no-op'ing; browser usage is unchanged (`localStorage`). +- Sessions now carry an `expiresAt`, checked via the new `isSessionExpired()`. Best-effort only — + the backend does not yet return a token TTL (tracked in #82) — see the README's "Session + Storage" section. +- Removed `src/stellar/rpc.ts` (`simulateAndAssemble`): dead code, never referenced or exported, + fully superseded by `TransactionPipeline.prepare`. Not part of any documented public API. +- Added `MultiSigStateStore` (target abstraction for a future backend-backed store, #83) and + `MultiSigEscrowClient.exportState`/`importState` (non-breaking stopgap for coordinating signers + across processes today) to `src/types/multisig.ts` / `src/escrow/multisig.ts`. +- See `docs/spikes/issue-79-retry-session-multisig.md` for the full retry/session/multisig design + writeup this release is based on. + ## [0.2.1] - 2026-06-29 - Add shared backend API transport in `src/utils/http.ts` using `axios` + `axios-retry` - Add automatic retries for transient backend failures (`429`, `5xx`, network errors) diff --git a/README.md b/README.md index 31a5049..8d345ea 100644 --- a/README.md +++ b/README.md @@ -130,6 +130,65 @@ console.log('Released! tx:', result.data?.txHash); See [examples/multisig-escrow.ts](./examples/multisig-escrow.ts) for the full walkthrough. +### Session Storage (Browser vs Node) + +`saveSession` / `loadSession` / `clearSession` detect their environment per call (via +`typeof localStorage`), so no setup is needed in either place: + +- **Browser**: uses `localStorage` automatically — sessions survive page reloads. +- **Node / CLI / backend**: falls back to an in-memory store scoped to the current process. + This **does not survive process restarts.** If you need durability (a long-running server, a + CLI invoked repeatedly), inject your own adapter: + + ```typescript + import { configureSessionStorage } from '@trustflow/sdk'; + + configureSessionStorage({ + get: (key) => myFileOrRedisStore.get(key), + set: (key, value) => myFileOrRedisStore.set(key, value), + remove: (key) => myFileOrRedisStore.delete(key), + }); + ``` + +- **SSR / bundler edge cases** (Next.js, Remix, etc.): `typeof localStorage` can be ambiguous + when server and client code share a module graph. If session calls run on the server during + SSR, they'll silently use the in-memory fallback for that request rather than throwing — which + is usually not what you want. Call `configureSessionStorage()` explicitly with a no-op or + server-appropriate adapter for server-rendered code paths, and only rely on the automatic + `localStorage` detection in code you know runs client-side. + +Sessions also carry an `expiresAt`, checked via `isSessionExpired()`. This is a **best-effort, +client-side value** — the backend does not currently return a token TTL (tracked in +[#82](https://github.com/trustflow-protocol/trustflow-sdk/issues/82)), so treat it as a lower +bound, not a guarantee, and still handle a `401` from the backend even when +`isSessionExpired()` returns `false`. + +### Multisig Cross-Process Coordination + +`MultiSigEscrowClient` keeps operation state in-memory per process. To coordinate signers running +in separate processes today, round-trip state through your own store with `exportState()` / +`importState()`: + +```typescript +// Process A (initiator) +const snapshot = client.exportState(operationId); // -> hand this to your own backend/queue + +// Process B (a signer), after fetching that snapshot from your store +const imported = client.importState(snapshot); +if (!imported.ok) { + throw new Error(imported.error); // malformed/corrupted snapshot +} +client.addSignature({ operationId, signerAddress, signedXdr }); +const reExported = client.exportState(operationId); // hand the updated state back to your store +``` + +`importState` overwrites any existing local operation with the same `operationId` — **last write +wins.** If two processes both mutate after diverging from the same snapshot and both re-export, +importing one after the other discards the first's signatures rather than merging them. +Serializing concurrent writes (e.g. one writer at a time through your store) is the caller's +responsibility until a native, backend-backed `MultiSigStateStore` lands — tracked in +[#83](https://github.com/trustflow-protocol/trustflow-sdk/issues/83). + --- ## ✨ Features diff --git a/docs/spikes/issue-79-retry-session-multisig.md b/docs/spikes/issue-79-retry-session-multisig.md index 6d66400..326d4c7 100644 --- a/docs/spikes/issue-79-retry-session-multisig.md +++ b/docs/spikes/issue-79-retry-session-multisig.md @@ -39,6 +39,15 @@ Actions taken in this PR: - Kept `src/utils/retry.ts` as-is: it's a legitimate public generic-purpose utility with its own tests: removing it would be a breaking change for consumers who may already depend on it. +**Removal-safety verification for `src/stellar/rpc.ts`** (re-confirmed per review request): +repo-wide search (`grep -rn "stellar/rpc\|simulateAndAssemble"` across `src/`, `tests/`, +`examples/`, `README.md`, `docs/`) turns up zero hits outside this spike's own doc/PR. It was +never re-exported from `src/stellar/index.ts` or `src/index.ts` (both barrels list their exports +explicitly and never named `rpc`), never had a test file, and — checking its git history +(`2858c1c feat(sdk): add Soroban RPC simulate helper`) — was never mentioned in `README.md` or +`docs/API.md`. There is no public API surface or documentation to deprecate; the removal has no +external footprint. + Follow-up (not done in this spike, filed as a separate issue): `TransactionPipeline`'s internal `withRetry` duplicates the backoff loop in `utils/retry.ts` with a different signature (attempt callback, policy object). Worth consolidating so there is one retry primitive, but that's a @@ -68,7 +77,17 @@ Problems in `auth/session.ts` today: with no TTL. Without a backend-supplied expiry, the SDK cannot know the *real* token lifetime — it can only apply a conservative client-side default (implemented here as 15 minutes, configurable) and treat that as a lower bound, not a guarantee. **Needs backend coordination**: add -`expiresIn`/`expiresAt` to the `/auth/verify` response. Flagged as a follow-up issue. +`expiresIn`/`expiresAt` to the `/auth/verify` response. Flagged as follow-up issue +[#82](https://github.com/trustflow-protocol/trustflow-sdk/issues/82). + +**Compatibility note (client-side `expiresAt` is best-effort, not a merge blocker):** this PR does +not wait on #82 to land. `isSessionExpired()` and the persisted `expiresAt` are documented — in +the `Session` interface's JSDoc, in `saveSession`'s JSDoc, and in the README's "Session Storage" +section — as a best-effort client-side signal only, not a guarantee of the token's real +server-side lifetime. Callers must still be prepared to handle a `401` from the backend even when +`isSessionExpired()` reports `false`. Once #82 lands, `verifyAndGetToken` can pass a real +`expiresAt` through to `saveSession` and the guessed default stops being used — no shape change +required on the SDK side. ## 4. Multisig operation-state coordination (recommendation) @@ -98,7 +117,13 @@ prototype. Instead, this PR: by round-tripping it through their own backend today) without waiting for the SDK to grow native async storage. This is deliberately additive — it does not change any existing method's signature or behavior, so it doesn't destabilize the tested sync API multisig consumers already - depend on. + depend on. `importState` validates the snapshot's shape and returns an `SDKResult` (matching the + rest of the class's error convention) rather than throwing on malformed input. + Conflict semantics — deliberately simple for a stopgap: `importState` is last-write-wins: + concurrent writers who diverge from the same exported snapshot and both re-export will have one + overwrite the other's signatures rather than merge. Serializing concurrent writes is the + caller's responsibility until the native store lands. Usage example and this caveat are also in + the README's "Multisig Cross-Process Coordination" section. - Full async, pluggable `MultiSigStateStore` wiring into `MultiSigEscrowClient` (which is a breaking API change, since every method would become `Promise`-returning) is left to the follow-up implementation issue, once the backend endpoints exist to back it. diff --git a/src/auth/session.ts b/src/auth/session.ts index ed2252d..00317a5 100644 --- a/src/auth/session.ts +++ b/src/auth/session.ts @@ -79,7 +79,18 @@ export function resetSessionStorage(): void { export interface Session { token: string; address: string; - /** UNIX ms timestamp after which the token should be treated as stale. */ + /** + * UNIX ms timestamp after which the token should be treated as stale. + * + * Best-effort only: the backend's `/auth/verify` response does not + * currently return a token TTL, so unless a caller passes `expiresAt` + * explicitly to `saveSession`, this is a conservative client-side guess + * (`DEFAULT_SESSION_TTL_MS`), not a guarantee of the token's real + * server-side lifetime. Do not rely on it for security-sensitive + * decisions — always be prepared to handle a `401` from the backend even + * when `isSessionExpired()` reports `false`. Tracked in + * https://github.com/trustflow-protocol/trustflow-sdk/issues/82. + */ expiresAt: number; } @@ -88,7 +99,8 @@ export interface Session { * * @param expiresAt - UNIX ms timestamp when the token expires. Defaults to * `DEFAULT_SESSION_TTL_MS` from now when omitted, since the backend does - * not currently return a token TTL (see docs/spikes/issue-79-retry-session-multisig.md). + * not currently return a token TTL — see the `expiresAt` caveat on + * {@link Session} and docs/spikes/issue-79-retry-session-multisig.md. */ export function saveSession(token: string, address: string, expiresAt?: number): void { const storage = getStorage(); @@ -105,8 +117,12 @@ export function loadSession(): Session | null { return null; } const expiresAtRaw = storage.get(EXPIRES_AT_KEY); - const expiresAt = expiresAtRaw ? Number(expiresAtRaw) : Date.now() + DEFAULT_SESSION_TTL_MS; - return { token, address, expiresAt }; + // A missing value defaults to a fresh TTL (session predates expiry tracking). + // A malformed value (corrupted storage, hand-edited) is treated as already + // expired rather than silently valid forever. + const expiresAt = + expiresAtRaw === null ? Date.now() + DEFAULT_SESSION_TTL_MS : Number(expiresAtRaw); + return { token, address, expiresAt: Number.isFinite(expiresAt) ? expiresAt : 0 }; } export function clearSession(): void { diff --git a/src/escrow/multisig.ts b/src/escrow/multisig.ts index 391225a..44ac581 100644 --- a/src/escrow/multisig.ts +++ b/src/escrow/multisig.ts @@ -4,6 +4,7 @@ import type { InitMultiSigParams, AddSignatureParams, MultiSigOperation, + MultiSigOperationStatus, MultiSigStatus, SignatureEntry, InitMultiSigResult, @@ -12,6 +13,7 @@ import type { SubmitMultiSigResult, GetXdrResult, MultiSigStateSnapshot, + ImportStateResult, } from '../types/multisig'; import { submitTransaction } from '../stellar/transaction'; @@ -257,20 +259,70 @@ export class MultiSigEscrowClient { * making it available to subsequent `addSignature` / `getMultiSigStatus` * / `submitWhenReady` calls in this process. * + * Conflict semantics: this overwrites any existing local operation with + * the same `operationId` — last write wins. If two processes both mutate + * (e.g. `addSignature`) after diverging from the same exported snapshot + * and both re-export, importing one after the other discards the first's + * signatures rather than merging them. Coordinating concurrent writers is + * the caller's responsibility until a native `MultiSigStateStore` backend + * (https://github.com/trustflow-protocol/trustflow-sdk/issues/83) can + * serialize writes centrally. + * * @param snapshot - A value previously returned by `exportState` */ - importState(snapshot: MultiSigStateSnapshot): void { + importState(snapshot: MultiSigStateSnapshot): ImportStateResult { + const validation = this._validateSnapshot(snapshot); + if (!validation.ok) { + return validation; + } + this.operations.set(snapshot.operationId, { ...snapshot, signers: [...snapshot.signers], collectedSignatures: [...snapshot.collectedSignatures], }); + return { ok: true, data: { operationId: snapshot.operationId } }; } // --------------------------------------------------------------------------- // Private helpers // --------------------------------------------------------------------------- + /** Validates the shape of a snapshot before it's admitted into `this.operations`. */ + private _validateSnapshot( + snapshot: MultiSigStateSnapshot, + ): { ok: true } | { ok: false; error: string } { + if (!snapshot || typeof snapshot !== 'object') { + return { ok: false, error: 'snapshot must be an object' }; + } + if (typeof snapshot.operationId !== 'string' || !snapshot.operationId) { + return { ok: false, error: 'snapshot.operationId must be a non-empty string' }; + } + if (typeof snapshot.escrowId !== 'string' || !snapshot.escrowId) { + return { ok: false, error: 'snapshot.escrowId must be a non-empty string' }; + } + if (typeof snapshot.unsignedXdr !== 'string' || !snapshot.unsignedXdr) { + return { ok: false, error: 'snapshot.unsignedXdr must be a non-empty string' }; + } + if (typeof snapshot.networkPassphrase !== 'string' || !snapshot.networkPassphrase) { + return { ok: false, error: 'snapshot.networkPassphrase must be a non-empty string' }; + } + if (!Array.isArray(snapshot.signers)) { + return { ok: false, error: 'snapshot.signers must be an array' }; + } + if (!Array.isArray(snapshot.collectedSignatures)) { + return { ok: false, error: 'snapshot.collectedSignatures must be an array' }; + } + if (typeof snapshot.threshold !== 'number' || snapshot.threshold < 1) { + return { ok: false, error: 'snapshot.threshold must be a number >= 1' }; + } + const validStatuses: MultiSigOperationStatus[] = ['pending', 'ready', 'submitted', 'expired']; + if (!validStatuses.includes(snapshot.status)) { + return { ok: false, error: `snapshot.status must be one of: ${validStatuses.join(', ')}` }; + } + return { ok: true }; + } + private _validateInitParams(params: InitMultiSigParams): InitMultiSigResult | { ok: true } { if (!params.escrowId) { return { ok: false, error: 'escrowId is required' }; diff --git a/src/types/multisig.ts b/src/types/multisig.ts index 5d87906..21bb608 100644 --- a/src/types/multisig.ts +++ b/src/types/multisig.ts @@ -131,3 +131,5 @@ export interface MultiSigStateStore { * `MultiSigStateStore` support. */ export type MultiSigStateSnapshot = MultiSigOperation; + +export type ImportStateResult = SDKResult<{ operationId: string }>; diff --git a/tests/auth.test.ts b/tests/auth.test.ts index 1f921d4..c782d9c 100644 --- a/tests/auth.test.ts +++ b/tests/auth.test.ts @@ -52,6 +52,17 @@ describe('Session management', () => { it('treats a missing session as expired', () => { expect(isSessionExpired()).toBe(true); }); + + it('treats a malformed stored expiresAt as expired rather than valid forever', () => { + saveSession('tok123', 'GABC'); + // Corrupt the persisted expiry directly, as if storage was hand-edited + // or written by an older/incompatible client. + (global as any).localStorage.setItem('trustflow_expires_at', 'not-a-number'); + + expect(isSessionExpired()).toBe(true); + // The token/address themselves should still load fine. + expect(loadSession()?.token).toBe('tok123'); + }); }); describe('configureSessionStorage', () => { @@ -74,5 +85,23 @@ describe('Session management', () => { // not have been touched while the override is active. expect(mockStorage['trustflow_token']).not.toBe('custom-tok'); }); + + it('propagates errors from an adapter that fails, rather than swallowing them', () => { + const throwingAdapter: SessionStorageAdapter = { + get: () => { + throw new Error('storage unavailable'); + }, + set: () => { + throw new Error('storage unavailable'); + }, + remove: () => { + throw new Error('storage unavailable'); + }, + }; + configureSessionStorage(throwingAdapter); + + expect(() => saveSession('tok', 'GABC')).toThrow('storage unavailable'); + expect(() => loadSession()).toThrow('storage unavailable'); + }); }); }); diff --git a/tests/multisig.test.ts b/tests/multisig.test.ts index c7e2018..b3dc770 100644 --- a/tests/multisig.test.ts +++ b/tests/multisig.test.ts @@ -529,7 +529,8 @@ describe('MultiSigEscrowClient', () => { const otherClient = new MultiSigEscrowClient(CONTRACT_CONFIG); expect(otherClient.getMultiSigStatus(operationId).ok).toBe(false); - otherClient.importState(snapshot); + const imported = otherClient.importState(snapshot); + expect(imported.ok).toBe(true); const status = otherClient.getMultiSigStatus(operationId); expect(status.ok).toBe(true); @@ -575,5 +576,41 @@ describe('MultiSigEscrowClient', () => { expect(original.data.signaturesCollected).toBe(0); } }); + + it('rejects a malformed snapshot instead of throwing', () => { + const malformed = { operationId: 'op-1' } as unknown as ReturnType< + MultiSigEscrowClient['exportState'] + >; + const result = client.importState(malformed!); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error).toMatch(/escrowId/); + } + // Nothing should have been admitted into the client's state. + expect(client.getMultiSigStatus('op-1').ok).toBe(false); + }); + + it('rejects a snapshot with a non-array signers field', () => { + const init = client.initMultiSigOperation({ + escrowId: ESCROW_ID, + signers: [KP_A.publicKey()], + threshold: 1, + operationType: 'release', + unsignedXdr: BASE_XDR, + networkPassphrase: NETWORK_PASSPHRASE, + }); + expect(init.ok).toBe(true); + if (!init.ok) return; + + const snapshot = client.exportState(init.data.operationId)!; + const corrupted = { ...snapshot, signers: 'not-an-array' } as unknown as typeof snapshot; + + const otherClient = new MultiSigEscrowClient(CONTRACT_CONFIG); + const result = otherClient.importState(corrupted); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error).toMatch(/signers/); + } + }); }); }); diff --git a/tests/session-node-default.test.ts b/tests/session-node-default.test.ts index cbe626d..4841fc3 100644 --- a/tests/session-node-default.test.ts +++ b/tests/session-node-default.test.ts @@ -22,3 +22,34 @@ describe('Session management (Node default, no localStorage)', () => { expect(loadSession()).toBeNull(); }); }); + +describe('Session management (environment detection)', () => { + afterEach(() => { + delete (global as any).localStorage; + clearSession(); + }); + + it('picks the in-memory adapter when localStorage is absent, and localStorage when present', () => { + expect(typeof (global as any).localStorage).toBe('undefined'); + saveSession('node-tok', 'GNODE'); + expect(loadSession()?.token).toBe('node-tok'); + + const backing: Record = {}; + (global as any).localStorage = { + getItem: (k: string) => backing[k] ?? null, + setItem: (k: string, v: string) => { + backing[k] = v; + }, + removeItem: (k: string) => { + delete backing[k]; + }, + }; + + // A session saved after localStorage becomes available goes through it, + // not the earlier in-memory fallback — detection happens per-call, not + // once at import time, so this doesn't require re-importing the module. + saveSession('browser-tok', 'GBROWSER'); + expect(backing['trustflow_token']).toBe('browser-tok'); + expect(loadSession()?.token).toBe('browser-tok'); + }); +}); From e321bd1e7c36b2095662bb932c6a16a81fcc7329 Mon Sep 17 00:00:00 2001 From: Jiggy <142459849+bbjiggy@users.noreply.github.com> Date: Tue, 18 Aug 2026 03:34:19 +0100 Subject: [PATCH 4/5] =?UTF-8?q?fix(sdk):=20address=20second=20review=20rou?= =?UTF-8?q?nd=20=E2=80=94=20snapshot=20versioning,=20expiry=20compat?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses meshackyaro's follow-up review on the retry/session/multisig spike PR (#82, #83, #84): - Add a version field (MULTISIG_SNAPSHOT_VERSION) to exported multisig snapshots. importState() now rejects a missing or mismatched version outright instead of silently misinterpreting an unfamiliar shape, giving future schema changes an explicit negotiation point. - Document and explicitly test the backward-compatible path in loadSession(): a session with no stored expiresAt key at all (written before expiry tracking existed) is treated as not-yet-expired, distinct from a malformed value (already treated as expired). - Add a "Compatibility & migration" note to the top of the spike doc and the CHANGELOG confirming no breaking changes, and cross-reference the follow-up issues (#82-#84) more explicitly throughout both. - Re-confirmed (repo-wide search) that removing src/stellar/rpc.ts still has no lingering references. --- CHANGELOG.md | 21 +++++++++--- .../spikes/issue-79-retry-session-multisig.md | 15 +++++++++ src/auth/session.ts | 11 +++++-- src/escrow/multisig.ts | 21 +++++++++--- src/types/multisig.ts | 17 +++++++++- tests/auth.test.ts | 15 +++++++++ tests/multisig.test.ts | 33 ++++++++++++++++++- 7 files changed, 118 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ed07a43..7d78d7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,19 +1,30 @@ # Changelog ## [Unreleased] +- **Breaking changes: none.** Everything below is additive; existing `saveSession`/`loadSession`/ + `clearSession` and `MultiSigEscrowClient` call signatures are unchanged. See the "Compatibility + & migration" note at the top of `docs/spikes/issue-79-retry-session-multisig.md`. - Session storage (`auth/session.ts`) is now pluggable via a `SessionStorageAdapter` and `configureSessionStorage()`. Node/CLI/backend usage now defaults to an in-memory adapter - instead of silently no-op'ing; browser usage is unchanged (`localStorage`). + instead of silently no-op'ing; browser usage is unchanged (`localStorage`). A pre-existing + session with no stored expiry (written before this change, or by an older SDK version) is + treated as not-yet-expired rather than retroactively expired. - Sessions now carry an `expiresAt`, checked via the new `isSessionExpired()`. Best-effort only — the backend does not yet return a token TTL (tracked in #82) — see the README's "Session - Storage" section. + Storage" section. A malformed/corrupted stored `expiresAt` is treated as already expired rather + than valid forever. - Removed `src/stellar/rpc.ts` (`simulateAndAssemble`): dead code, never referenced or exported, - fully superseded by `TransactionPipeline.prepare`. Not part of any documented public API. + fully superseded by `TransactionPipeline.prepare`. Not part of any documented public API + (verified via repo-wide search of `src/`, `tests/`, `examples/`, and docs). - Added `MultiSigStateStore` (target abstraction for a future backend-backed store, #83) and `MultiSigEscrowClient.exportState`/`importState` (non-breaking stopgap for coordinating signers - across processes today) to `src/types/multisig.ts` / `src/escrow/multisig.ts`. + across processes today) to `src/types/multisig.ts` / `src/escrow/multisig.ts`. Exported + snapshots carry a `version` field (`MULTISIG_SNAPSHOT_VERSION`) so a future schema change can be + detected and rejected by `importState` instead of silently misinterpreted. +- Retry: `src/utils/retry.ts` kept as-is (tested public utility); consolidating it with + `TransactionPipeline`'s internal retry loop is tracked separately (#84). - See `docs/spikes/issue-79-retry-session-multisig.md` for the full retry/session/multisig design - writeup this release is based on. + writeup this release is based on. Follow-up implementation issues: #82, #83, #84. ## [0.2.1] - 2026-06-29 - Add shared backend API transport in `src/utils/http.ts` using `axios` + `axios-retry` diff --git a/docs/spikes/issue-79-retry-session-multisig.md b/docs/spikes/issue-79-retry-session-multisig.md index 326d4c7..581dcbc 100644 --- a/docs/spikes/issue-79-retry-session-multisig.md +++ b/docs/spikes/issue-79-retry-session-multisig.md @@ -2,6 +2,14 @@ Tracking issue: [#79](https://github.com/trustflow-protocol/trustflow-sdk/issues/79) +**Compatibility & migration:** no breaking changes for existing consumers. `saveSession`, +`loadSession`, `clearSession`, and every `MultiSigEscrowClient` method that existed before this +spike keep their original signatures and behavior. The only new public surface — the pluggable +session storage adapter, session `expiresAt`/`isSessionExpired`, `MultiSigStateStore`, and +`MultiSigEscrowClient.exportState`/`importState` — is purely additive. Runtime behavior does +change under Node: `saveSession` used to silently no-op there and now persists in-memory for the +process lifetime (see §3). + ## 1. Current-state audit The issue was filed against an earlier snapshot of the code. Since then `TransactionPipeline` @@ -124,6 +132,13 @@ prototype. Instead, this PR: overwrite the other's signatures rather than merge. Serializing concurrent writes is the caller's responsibility until the native store lands. Usage example and this caveat are also in the README's "Multisig Cross-Process Coordination" section. + Every exported snapshot carries a `version` field (`MULTISIG_SNAPSHOT_VERSION`, currently `1`, + in `src/types/multisig.ts`). `importState` rejects a snapshot whose version is missing or + doesn't match, rather than guessing at an unfamiliar shape. This is the version-negotiation hook + for the day `MultiSigStateSnapshot`'s shape needs to change — bump the constant and give + `importState` an explicit per-version migration/rejection path then. It's a no-op today (only + version `1` exists), but is cheap to add now versus retrofitting it once real snapshots are + already stored in integrators' backends. - Full async, pluggable `MultiSigStateStore` wiring into `MultiSigEscrowClient` (which is a breaking API change, since every method would become `Promise`-returning) is left to the follow-up implementation issue, once the backend endpoints exist to back it. diff --git a/src/auth/session.ts b/src/auth/session.ts index 00317a5..0e5f94b 100644 --- a/src/auth/session.ts +++ b/src/auth/session.ts @@ -117,9 +117,14 @@ export function loadSession(): Session | null { return null; } const expiresAtRaw = storage.get(EXPIRES_AT_KEY); - // A missing value defaults to a fresh TTL (session predates expiry tracking). - // A malformed value (corrupted storage, hand-edited) is treated as already - // expired rather than silently valid forever. + // Backward compatibility: a session written before expiry tracking existed + // (or by an older version of this SDK) has no `EXPIRES_AT_KEY` entry at + // all — `storage.get` returns `null`, not a malformed string. Treat that + // as unknown-but-fine and default to a fresh TTL from now, so upgrading + // doesn't retroactively expire sessions that predate this field. + // A *malformed* value (non-null, but not parseable — corrupted storage, + // hand-edited), by contrast, is treated as already expired rather than + // silently valid forever (see isSessionExpired()). const expiresAt = expiresAtRaw === null ? Date.now() + DEFAULT_SESSION_TTL_MS : Number(expiresAtRaw); return { token, address, expiresAt: Number.isFinite(expiresAt) ? expiresAt : 0 }; diff --git a/src/escrow/multisig.ts b/src/escrow/multisig.ts index 44ac581..3cc1b76 100644 --- a/src/escrow/multisig.ts +++ b/src/escrow/multisig.ts @@ -15,6 +15,7 @@ import type { MultiSigStateSnapshot, ImportStateResult, } from '../types/multisig'; +import { MULTISIG_SNAPSHOT_VERSION } from '../types/multisig'; import { submitTransaction } from '../stellar/transaction'; /** @@ -248,6 +249,7 @@ export class MultiSigEscrowClient { return operation ? { ...operation, + version: MULTISIG_SNAPSHOT_VERSION, signers: [...operation.signers], collectedSignatures: [...operation.collectedSignatures], } @@ -276,12 +278,15 @@ export class MultiSigEscrowClient { return validation; } - this.operations.set(snapshot.operationId, { - ...snapshot, - signers: [...snapshot.signers], - collectedSignatures: [...snapshot.collectedSignatures], + // `version` is a snapshot-transport concern, not part of the operation's + // own state — don't let it leak into the in-memory record. + const { version: _version, ...operation } = snapshot; + this.operations.set(operation.operationId, { + ...operation, + signers: [...operation.signers], + collectedSignatures: [...operation.collectedSignatures], }); - return { ok: true, data: { operationId: snapshot.operationId } }; + return { ok: true, data: { operationId: operation.operationId } }; } // --------------------------------------------------------------------------- @@ -295,6 +300,12 @@ export class MultiSigEscrowClient { if (!snapshot || typeof snapshot !== 'object') { return { ok: false, error: 'snapshot must be an object' }; } + if (snapshot.version !== MULTISIG_SNAPSHOT_VERSION) { + return { + ok: false, + error: `snapshot.version ${String(snapshot.version)} is not supported by this SDK (expected ${MULTISIG_SNAPSHOT_VERSION})`, + }; + } if (typeof snapshot.operationId !== 'string' || !snapshot.operationId) { return { ok: false, error: 'snapshot.operationId must be a non-empty string' }; } diff --git a/src/types/multisig.ts b/src/types/multisig.ts index 21bb608..5e05e97 100644 --- a/src/types/multisig.ts +++ b/src/types/multisig.ts @@ -124,12 +124,27 @@ export interface MultiSigStateStore { listByEscrow(escrowId: EscrowId): Promise; } +/** + * Snapshot schema version produced by `MultiSigEscrowClient.exportState` and + * expected by `importState`. Bump this — and give `importState` an explicit + * migration/rejection path for older versions — if `MultiSigStateSnapshot`'s + * shape ever changes in a way older snapshots wouldn't satisfy. Versioning + * this now (even with only one version in existence) means a future schema + * change doesn't silently misinterpret an older snapshot serialized by an + * integrator's store; `importState` rejects a mismatched version outright + * instead of guessing. + */ +export const MULTISIG_SNAPSHOT_VERSION = 1; + /** * Serializable snapshot of one multisig operation, for round-tripping state * through an external store (e.g. an integrator's own backend) between * `MultiSigEscrowClient.exportState` / `importState` calls, ahead of native * `MultiSigStateStore` support. */ -export type MultiSigStateSnapshot = MultiSigOperation; +export interface MultiSigStateSnapshot extends MultiSigOperation { + /** See {@link MULTISIG_SNAPSHOT_VERSION}. */ + version: number; +} export type ImportStateResult = SDKResult<{ operationId: string }>; diff --git a/tests/auth.test.ts b/tests/auth.test.ts index c782d9c..a562ebd 100644 --- a/tests/auth.test.ts +++ b/tests/auth.test.ts @@ -53,6 +53,21 @@ describe('Session management', () => { expect(isSessionExpired()).toBe(true); }); + it('treats a pre-existing session with no stored expiresAt key as not expired (backward compatibility)', () => { + // Simulates a session written by a pre-expiry version of this SDK: + // only token/address were ever persisted, no `trustflow_expires_at` + // key exists at all (distinct from a malformed value — see below). + // loadSession() computes a fresh default TTL from "now" in this case, + // so an old session isn't treated as already-expired just because it + // predates expiry tracking. + (global as any).localStorage.setItem('trustflow_token', 'legacy-tok'); + (global as any).localStorage.setItem('trustflow_address', 'GLEGACY'); + + const s = loadSession(); + expect(s?.token).toBe('legacy-tok'); + expect(isSessionExpired(s)).toBe(false); + }); + it('treats a malformed stored expiresAt as expired rather than valid forever', () => { saveSession('tok123', 'GABC'); // Corrupt the persisted expiry directly, as if storage was hand-edited diff --git a/tests/multisig.test.ts b/tests/multisig.test.ts index b3dc770..aac58ff 100644 --- a/tests/multisig.test.ts +++ b/tests/multisig.test.ts @@ -525,6 +525,7 @@ describe('MultiSigEscrowClient', () => { const snapshot = client.exportState(operationId); expect(snapshot).toBeDefined(); if (!snapshot) return; + expect(snapshot.version).toBe(1); const otherClient = new MultiSigEscrowClient(CONTRACT_CONFIG); expect(otherClient.getMultiSigStatus(operationId).ok).toBe(false); @@ -578,7 +579,7 @@ describe('MultiSigEscrowClient', () => { }); it('rejects a malformed snapshot instead of throwing', () => { - const malformed = { operationId: 'op-1' } as unknown as ReturnType< + const malformed = { version: 1, operationId: 'op-1' } as unknown as ReturnType< MultiSigEscrowClient['exportState'] >; const result = client.importState(malformed!); @@ -590,6 +591,36 @@ describe('MultiSigEscrowClient', () => { expect(client.getMultiSigStatus('op-1').ok).toBe(false); }); + it('rejects a snapshot with a missing or mismatched version', () => { + const init = client.initMultiSigOperation({ + escrowId: ESCROW_ID, + signers: [KP_A.publicKey()], + threshold: 1, + operationType: 'release', + unsignedXdr: BASE_XDR, + networkPassphrase: NETWORK_PASSPHRASE, + }); + expect(init.ok).toBe(true); + if (!init.ok) return; + + const snapshot = client.exportState(init.data.operationId)!; + + const missingVersion = { ...snapshot } as { version?: number }; + delete missingVersion.version; + const resultMissing = client.importState(missingVersion as typeof snapshot); + expect(resultMissing.ok).toBe(false); + if (!resultMissing.ok) { + expect(resultMissing.error).toMatch(/version/); + } + + const futureVersion = { ...snapshot, version: 999 }; + const resultFuture = client.importState(futureVersion); + expect(resultFuture.ok).toBe(false); + if (!resultFuture.ok) { + expect(resultFuture.error).toMatch(/version/); + } + }); + it('rejects a snapshot with a non-array signers field', () => { const init = client.initMultiSigOperation({ escrowId: ESCROW_ID, From 56b5ed452f0219850a391f31b3331441fa147af2 Mon Sep 17 00:00:00 2001 From: Jiggy <142459849+bbjiggy@users.noreply.github.com> Date: Tue, 18 Aug 2026 03:46:24 +0100 Subject: [PATCH 5/5] docs(spike): cross-link README's session storage section from the doc Small follow-up to meshackyaro's third review round: the spike doc described the Node-default session storage recommendation in prose but didn't point readers at the README's concrete configureSessionStorage() adapter-injection example. --- docs/spikes/issue-79-retry-session-multisig.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/spikes/issue-79-retry-session-multisig.md b/docs/spikes/issue-79-retry-session-multisig.md index 581dcbc..7b50278 100644 --- a/docs/spikes/issue-79-retry-session-multisig.md +++ b/docs/spikes/issue-79-retry-session-multisig.md @@ -63,6 +63,10 @@ refactor of tested, shipped code and deserves its own review rather than riding ## 3. Session storage & token lifecycle (recommendation) +> See the README's **["Session Storage (Browser vs Node)"](../../README.md#session-storage-browser-vs-node)** +> section for the concrete `configureSessionStorage()` adapter-injection example and the +> SSR/bundler edge-case notes — this section covers the design rationale, not the how-to. + Problems in `auth/session.ts` today: - `localStorage`-only; every call is a silent no-op under Node (CLI/backend integrators), which looks like it "works" (no exception) but never persists anything.