Deposit flow is implemented twice (DepositModal vs useLockFlow) with independently maintained, divergent state machines

[prodbycorne]

Problem

There are two separate, non-shared implementations of the exact same 'simulate → sign → submit → confirm' deposit flow:

1. DepositModal in src/app/farm/page.tsx (lines 86-429) manages its own local step/txHash/localError state and calls the useLockAssets mutation hook directly, with its own bespoke validation (amountValid, exceedsBalance, fee-preview gating via useLockAssetsFeePreview).
2. useLockFlow (src/hooks/useLockFlow.ts), used by PoolDetailClient (src/app/farm/[poolId]/PoolDetailClient.tsx), implements a different state machine with its own DepositStep progression, its own trackEvent analytics calls (deposit_initiated/deposit_succeeded/deposit_failed — entirely absent from DepositModal's path), and calls lockAssets (the free function) rather than the useLockAssets mutation hook, meaning it does not benefit from that hook's onSuccess cache invalidation (useLockFlow does its own separate, narrower invalidateQueries calls at lines 110-112 that omit stellarBalance and the 'all'/per-address USER_POSITION variants that useLockAssets invalidates).

Because these two flows were built independently for the farm-list deposit modal versus the pool-detail-page deposit modal, they have already drifted: analytics events differ, cache invalidation coverage differs, and validation logic (decimal-place limits, balance checks, fee-sponsorship detection) is implemented separately in each. Any future bug fix or security tightening (e.g. the wallet-liveness check requested elsewhere in this batch) has to be applied twice, and it's easy for the two entry points to silently diverge in behavior — which they already have.

Acceptance Criteria

• Consolidate on a single deposit-flow implementation (most likely by having DepositModal in farm/page.tsx switch to useLockFlow, or vice versa) so there is exactly one state machine, one analytics instrumentation path, and one cache-invalidation contract.
• Ensure the consolidated flow invalidates the full set of query keys currently covered by useLockAssets's onSuccess (POOLS, USER_POSITION in all its key variants, USER_CREDITS, PLATFORM_STATS, stellarBalance).
• Ensure trackEvent analytics coverage (deposit_initiated/deposit_succeeded/deposit_failed) is present from both the farm-list and pool-detail entry points after consolidation.
• Add/update tests so both entry points are covered by the same underlying flow's test suite rather than two independently-tested implementations.

Relevant Files

• src/app/farm/page.tsx — DepositModal (L86-429) implements its own deposit state machine using useLockAssets directly
• src/hooks/useLockFlow.ts — a second, independent deposit state machine used only by PoolDetailClient
• src/app/farm/[poolId]/PoolDetailClient.tsx — consumes useLockFlow, diverging from the Farm page's approach
• src/hooks/useSorobanQuery.ts — useLockAssets' onSuccess invalidation (~L146-172) is broader than useLockFlow's (~L110-112)

Further Investigation

Confirmed against current source — the two implementations really have diverged

DepositModal (src/app/farm/page.tsx, function-scoped inside the file, roughly lines 86-429): manages local step/txHash/localError state (visible directly in the "Try again" reset handler around line 419-425: setStep("idle"); setLocalError(null); lockMutation.reset();), calls useLockAssets() (a useMutation) directly, and its onSuccess invalidation (read directly from useLockAssets in src/hooks/useSorobanQuery.ts, lines ~146-172) covers: POOLS, USER_POSITION (bare), USER_POSITION scoped to variables.poolId, USER_POSITION scoped to ['all', publicKey], USER_CREDITS scoped to variables.poolId, PLATFORM_STATS, and ['stellarBalance', publicKey] — seven distinct invalidation calls.

useLockFlow (src/hooks/useLockFlow.ts, full file read, 130+ lines): a separate, self-contained hook with its own DepositStep/step state, its own execute() async function calling the free function lockAssets (not the useLockAssets mutation), and its own trackEvent calls (deposit_initiated at the top of execute, deposit_succeeded on success, deposit_failed in the catch block) — genuinely richer analytics instrumentation than DepositModal, which does not call trackEvent at all in the excerpt reviewed. Its invalidation, confirmed at lines ~110-112, is exactly three calls: USER_POSITION (bare), POOLS, PLATFORM_STATS — missing USER_CREDITS, the USER_POSITION per-pool and ['all', publicKey] variants, and stellarBalance entirely, relative to useLockAssets's onSuccess.

This confirms the issue precisely: not just two state machines, but two different invalidation contracts and two different analytics contracts, and useLockFlow is strictly narrower on caching (a deposit through the pool-detail page will leave stellarBalance and per-address USER_POSITION variants stale until the next poll) while being strictly richer on analytics (a deposit through the farm-list modal generates zero deposit_* events at all).

Why this is a genuine spike, not just a hard fix

The acceptance criteria ask to "consolidate on a single deposit-flow implementation" but explicitly hedge with "most likely... or vice versa" — because there is no obviously-correct target architecture without further investigation:

• Option A: Migrate DepositModal onto useLockFlow. Gains: trackEvent analytics parity, a leaner hook interface. Loses: useLockFlow calls the free lockAssets function directly rather than the useLockAssets mutation, so it would need its own manual invalidation calls expanded to match useLockAssets's seven calls (straightforward but easy to under-invalidate again, as it already has once) — or useLockFlow would need to be rewritten to wrap useLockAssets internally, which changes its external contract (currently exposes isPending/step/record/error/execute/reset; wrapping a mutation would need to reconcile the mutation's own isPending/isError with the hand-rolled DepositStep state).
• Option B: Migrate PoolDetailClient onto DepositModal's pattern (useLockAssets mutation directly + bespoke local step/error state). Gains: gets the fuller invalidation set for free (it's already on useLockAssets's onSuccess). Loses: needs trackEvent instrumentation added from scratch to match what useLockFlow already has, and DepositModal's validation (amountValid, exceedsBalance, useLockAssetsFeePreview fee-preview gating) would need to be extracted into something PoolDetailClient can reuse rather than copy-pasted a third time.
• Option C: Extract a new shared hook/primitive that neither current implementation is, combining useLockAssets's invalidation with useLockFlow's analytics and a fee-preview-aware validation layer, then have both DepositModal and PoolDetailClient consume it. This is the "do it right" option but requires deciding the new hook's public interface up front (state shape, whether it owns the modal's UI-facing step enum or just wraps the mutation, whether fee-preview belongs inside the hook or stays a separate concern the modal composes) — exactly the kind of interface-design research a spike is for.

None of these is obviously "the" answer without reading how useLockAssetsFeePreview (referenced in DepositModal's description but not yet read in this pass) integrates, and without a decision from whoever owns deposit-flow UX about whether PoolDetailClient's page-level deposit experience should visually/behaviorally match the farm-list modal's experience going forward (a product decision, not just a code decision) — which is precisely why this issue is being labeled spike rather than a well-specified fix.

Additional edge cases / cross-cutting concerns for whichever approach is chosen

1. useLockAssetsFeePreview gating — mentioned in the issue as unique to DepositModal's validation; whichever consolidated implementation is chosen must decide whether PoolDetailClient's deposit flow currently lacks fee-preview gating entirely (a UX regression risk if consolidation removes something DepositModal users currently rely on) or already has an equivalent the issue text doesn't mention — this needs to be checked directly in PoolDetailClient.tsx before deciding a target design.
2. Wallet-liveness / guard-against-disconnect concerns raised elsewhere in this batch (issue lockAssets/unlockAssets have no guard against the wallet disconnecting between simulate and sign #70: "lockAssets/unlockAssets have no guard against the wallet disconnecting between simulate and sign") — the issue body already anticipates this: "Any future bug fix or security tightening... has to be applied twice." Since lockAssets/unlockAssets have no guard against the wallet disconnecting between simulate and sign #70 touches the same lockAssets/unlockAssets free functions both DepositModal (indirectly, via useLockAssets) and useLockFlow (directly) call into, a fix for lockAssets/unlockAssets have no guard against the wallet disconnecting between simulate and sign #70 landing before this consolidation would need to be applied to both current call sites, and a fix for lockAssets/unlockAssets have no guard against the wallet disconnecting between simulate and sign #70 landing after this consolidation only needs one. Recommend sequencing: if both issues are being worked in the same cycle, resolving Deposit flow is implemented twice (DepositModal vs useLockFlow) with independently maintained, divergent state machines #83's consolidation first would reduce lockAssets/unlockAssets have no guard against the wallet disconnecting between simulate and sign #70's surface area to one call site instead of two — worth flagging to whoever schedules bounty work across this batch.
3. Decimal-place / balance-check validation logic — the issue notes both flows implement this separately; a spike investigation should confirm whether the two implementations' validation rules are even in agreement today (e.g. same minimum amount, same decimal precision limit) — if they've already silently diverged the way invalidation and analytics have, that's an additional user-facing inconsistency (a user could submit an amount accepted by the farm-list modal but rejected by the pool-detail page, or vice versa) worth documenting as part of the spike's findings even before a fix is chosen.

Testing strategy for reviewers

• A PR that "closes" this issue by picking one of the three options above should be judged primarily on whether it states which option it chose and why — given the acceptance criteria itself doesn't mandate a specific direction, a reviewer should reject a PR that silently unifies the two flows without documenting the tradeoff it resolved (this is the core spike deliverable, arguably more important than the code itself).
• Verify the final single implementation's cache invalidation is a superset of both prior invalidation sets (all seven from useLockAssets's onSuccess), not just whichever set the "winning" implementation happened to have.
• Verify trackEvent fires deposit_initiated/deposit_succeeded/deposit_failed from both the farm-list and pool-detail entry points post-consolidation — add a test at each entry point (not just one shared test for the underlying hook) since the whole point of consolidation is that both call sites route through the same instrumented path.
• Verify no regression in decimal-place/balance validation for either entry point per edge case 3 above — ideally a shared validation unit test exercised from both component-level tests.
• Cross-reference: issue lockAssets/unlockAssets have no guard against the wallet disconnecting between simulate and sign #70 (wallet-disconnect guard between simulate and sign) touches the same lockAssets/unlockAssets free functions this issue's flows call into; a reviewer coordinating both issues should check whether lockAssets/unlockAssets have no guard against the wallet disconnecting between simulate and sign #70's guard was added to only one of the two current implementations before this consolidation lands, which would make the consolidation choice load-bearing for security, not just code cleanliness.