Fund-wallet flow: full funding/success lifecycle + prompt card redesign - #504
Fund-wallet flow: full funding/success lifecycle + prompt card redesign#504dotsolo wants to merge 7 commits into
Conversation
…ompt card redesign
ee19549 to
4647f6b
Compare
ee19549 to
4647f6b
Compare
|
can you make your claude to update the description to include the original issue to this PR? @dotsolo |
WiktorStarczewski
left a comment
WiktorStarczewski
left a comment
There was a problem hiding this comment.
Really nice feature — the persisted marker for remount/restart survival is the right call, and a couple of genuinely subtle interactions are handled correctly: suppressing the pending-notes card while the hero is on stage (it sorts first and would otherwise shove the hero off-screen the instant the note lands), and ceding completion authority from the seeding effect to the hero sequence (the !awaitingFaucetFunds && !faucetFundsArrived guard). The comments explain why rather than what, which made this reviewable.
That said, there are three real correctness bugs — one critical — plus a couple of suggestions.
🔴 Critical — the funding marker is global; the "Funding" hero leaks across accounts
FAUCET_FUNDING_REQUESTED_AT_KEY = 'faucet_funding_requested_at_v1' (src/lib/wallet-prompts.ts) has no per-account scoping, and awaitingFaucetFunds / faucetFundsArrived live in HomePrompts state that is not reset on an account switch. Explore → HomePrompts gets account from useAccount() (Zustand currentAccount) and re-renders in place — there's no key={account.publicKey}, so switching accounts does not remount.
Repro (both accounts unfunded):
- On account A, tap Fund →
awaitingFaucetFunds = true, global marker stamped, Funding hero shows. - Switch to account B →
Explore/HomePromptsre-render in place with B's balances/notes; nothing resets the flags → B now shows the "Funding" hero for A's request. - B is genuinely unfunded, so the arrival effect never fires; the spurious hero holds until B's backstop — and that backstop calls
setFaucetFundingRequestedAt(null), erasing account A's still-in-flight marker, so an app restart during A's real wait no longer resumes.
The app-restart variant is the same: a fresh mount on B reads the global marker and resumes A's hero on B.
Fix: suffix the storage key with account.publicKey (e.g. faucet_funding_requested_at_v1:<pk>), read/write it per-account, and reset awaitingFaucetFunds / faucetFundsArrived in an effect keyed on account.publicKey.
🟠 "Funded!" fires on any existing or unrelated note/balance — an instant false success
The arrival effect gates on hasPendingNotes || hasBalance, where hasPendingNotes is the raw count of all current claimable notes, not a delta from the request baseline. The faucet card is seeded purely on !hasBalance, and incoming Miden notes sit unconsumed (hasBalance === false) — so a user with a pre-existing claimable note and zero balance sees the faucet card.
Repro: such a user taps Fund → fundWallet resolves and sets awaitingFaucetFunds = true; the arrival effect sees hasPendingNotes === true on the very next commit and fires immediately → the Funding hero flashes for ~0 ms and the card jumps straight to "Funds deposited — you have $X ready" and completes the faucet prompt, while the actual faucet mint is still 30–60 s away. It also mis-attributes the pre-existing note's USD amount to the faucet. The milder variant (an unrelated deposit arriving mid-wait) has the same root cause.
Fix: snapshot a baseline at request time (a note-id set, or note count / balance) and only trip arrival when it increases beyond that baseline.
🟠 The 3-minute backstop is anchored to mount, not to requestedAt — resumed waits can run ~6 min
The backstop effect schedules FAUCET_FUNDS_ARRIVAL_TIMEOUT_MS (3 min) from when awaitingFaucetFunds flips true. On resume, the resume effect sets awaitingFaucetFunds = true for any marker younger than 3 min — so a marker that's 2:59 old starts a fresh 3-min backstop, i.e. ~5:59 of "Funding" total if the mint silently failed. It's bounded, but the value is wrong.
Fix: schedule the backstop for max(0, requestedAt + FAUCET_FUNDS_ARRIVAL_TIMEOUT_MS − Date.now()), reading requestedAt from the marker.
🟡 Suggestions
faucet()has no timeout, and the initialloadinghero has no backstop.faucetHeroActiveincludesfaucetStatusIndicator === 'loading', but the backstop only coversawaitingFaucetFunds, andfaucet()is a barefetchwith noAbortController. If the request hangs,fundingRefstaystrue, the loading hero holds, and the pending-notes card stays suppressed with no in-session recovery (no marker was stamped yet, so it only self-heals on restart). Consider anAbortControllertimeout on the faucet call.- Test gaps let the three bugs above ship: no account-switch test (critical #1); the tap test starts with
claimableNotes={[]}and only adds notes on rerender, so the pre-existing-note instant-false-success (#2) is never exercised; and there's no backstop-timing assertion (#3). The beat tests also use real timers withwaitFor(…, { timeout: 3500 })— slow and flaky; prefer fake timers. - Minor / not reachable today: the resume effect returns without clearing a stale marker if
faucetIsTerminalis already true (cosmetic, overwritten next funding); the non-dismiss/non-hero branch always renders aChevronRighteven whenonClickis undefined (only matters if a non-dismissible prompt is ever added — all current ones are dismissible).
✅ Verified fine
handleDismiss/handleAction both stopPropagation(), so nested-button taps don't bubble to the card's onClick; the PromptCard hero/icon/dismiss/status branches all render correctly (an in-flight card correctly can't be dismissed); no timer leaks (beat + backstop each clearTimeout in cleanup; the removed successTimerRef is fully replaced); the duplicate completePrompt(Faucet) path is idempotent; i18n is fully wired across all locales including the $amount$ placeholder, and both hero icons resolve; and Confirmation.tsx correctly removes an invalid bare font-medium JSX attribute (it had been emitting font-medium="true" on the DOM).
Happy to re-review once the account-scoping (#1) and the arrival-baseline (#2) are addressed.
…quest-anchored backstop, 60s faucet timeout
|
@WiktorStarczewski Just updated the PR and it should include the requested fixes and @BrianSeong99 it should also be updated to address the original issue! |
|
Reviewed the current head ( 🟠 P2 — the timeout does not cancel the underlying faucet work
In the delayed-response case, both attempts could reach the faucet and mint notes. In the PoW case, the original loop can continue consuming resources after the UI reports failure. Fix: propagate an 🟠 P2 — whole-card actions are not keyboard accessible
Fix: add complete keyboard-button behavior or retain a semantic button for the primary action. 🟡 Suggestion — the backstop starts after the faucet acknowledgement
This remains bounded, so I do not consider it a blocker. Either capture the timestamp before awaiting the faucet or adjust the description to say the three-minute wait starts after acknowledgement. ✅ Verified fineThe persisted marker survives remounts, the baseline prevents a pre-existing note from causing immediate false success, and the resumed backstop is correctly calculated from its stored timestamp. The pending-notes suppression during the success beat also remains correct. PR housekeeping
|
|
|
||
| /** | ||
| * Indeterminate progress runner for PromptCard's loading state. | ||
| */ | ||
|
|
||
| @keyframes prompt-progress { | ||
| from { | ||
| transform: translateX(-100%); | ||
| } | ||
| to { | ||
| transform: translateX(300%); | ||
| } | ||
| } | ||
|
|
||
| .prompt-progress-runner { | ||
| animation: prompt-progress 1.4s ease-in-out infinite; | ||
| } | ||
|
|
||
| /** | ||
| * PromptCard hero lockup: hourglass flip while loading, pop-in on state swap. | ||
| */ | ||
|
|
||
| @keyframes prompt-hero-flip { | ||
| 0%, | ||
| 35% { | ||
| transform: rotate(0deg); | ||
| } | ||
| 50%, | ||
| 85% { | ||
| transform: rotate(180deg); | ||
| } | ||
| 100% { | ||
| transform: rotate(360deg); | ||
| } | ||
| } | ||
|
|
||
| .prompt-hero-flip { | ||
| animation: prompt-hero-flip 2.2s cubic-bezier(0.77, 0, 0.175, 1) infinite; | ||
| } | ||
|
|
||
| @keyframes prompt-hero-pop { | ||
| from { | ||
| transform: scale(0.92); | ||
| opacity: 0; | ||
| } | ||
| to { | ||
| transform: scale(1); | ||
| opacity: 1; | ||
| } | ||
| } | ||
|
|
||
| .prompt-hero-pop { | ||
| animation: prompt-hero-pop 240ms cubic-bezier(0.23, 1, 0.32, 1) both; | ||
| } |
There was a problem hiding this comment.
Is there a reason why you won't use framer motion here, I mean this works but we can do this by not polluting the main.css file when we already have framer motion and tailwind animate in the bundle
There was a problem hiding this comment.
how about create a skill.md for this and we can enforce this pattern to agents?
…rompt cards, pre-ack backstop anchor
|
Addressed the review in 277872e:
Housekeeping: the PR is retargeted onto |
WiktorStarczewski
left a comment
WiktorStarczewski
left a comment
There was a problem hiding this comment.
Re-reviewed the current branch (277872edc). The critical + high issues from my last review are all genuinely fixed — nice work, and the fixes have real tests behind them:
| Prior finding | Status | How |
|---|---|---|
| 🔴 global marker leaks the Funding hero across accounts | ✅ Fixed | per-account key faucet_funding_v2:${address}; awaitingFaucetFunds derives from fundingWait.address === account.publicKey; reset effect keyed on account.publicKey; the backstop no longer wipes the other account's marker |
| 🟠 false "Funded!" on any pre-existing note | ✅ Fixed | request-time baselineNoteIds snapshot; arrival trips only on a note beyond the baseline set |
| 🟠 backstop anchored to mount | ✅ Fixed | max(0, requestedAt + TIMEOUT − now), requestedAt read from the marker |
| 🟡 faucet timeout/abort | ✅ Fixed | AbortController races a 60s timeout, signal threaded into every fetch + checked each PoW iteration |
And the tests genuinely constrain each fix (account-switch leak, baseline false-success, backstop timing anchor, abort-stops-PoW-loop) rather than asserting nothing.
Requesting changes only on the two merge-gating items below; everything else is non-blocking.
Merge-gating
1. 🟠 MEDIUM — funding a second account is a dead-end while the first request is in flight. HomePrompts.tsx:282
fundWallet bails on a single boolean ref: if (fundingRef.current) return. On account A tap Fund → fundingRef=true; switch to B (the reset effect clears B's indicator to idle, so B looks tappable); tap Fund on B → it short-circuits before setFaucetStatusIndicator('loading'), so nothing happens — no request, no loading, no feedback — until A's request settles (up to 60s). Fix: scope the in-flight guard per address (a ref/Set of in-flight publicKeys) so a different account can fund concurrently while double-submit is still blocked per account.
2. 🟠 MEDIUM — arrival isn't faucet-specific; any inbound note/balance mid-wait false-succeeds. HomePrompts.tsx:344
const hasNewNote = pendingNoteIds.some(noteId => !baseline.has(noteId));
if (!hasNewNote && !hasBalance) return;pendingNoteIds is every claimable note id regardless of faucet (PendingNoteValue drops faucetId/sender), so during the 30–60s wait an unrelated inbound note (a friend's transfer, a bridge/earn deposit, any token) trips the "Funded!" beat, completes the Faucet prompt early, and shows the wrong $ (formattedPendingNotesUsdTotal sums all notes). No funds are lost — the real mint lands later as a normal pending note — but the lifecycle UI lies.
This is the "milder variant" I flagged last time and the PR marked out-of-scope. Fair to accept it, but it's more than cosmetic now that the hero completes the prompt. The clean fix: thread faucetId into PendingNoteValue and require the new note to be the MIDEN faucet's (and don't treat a bare hasBalance as arrival on its own). Your call: fix now, or accept-and-document explicitly — but let's decide rather than leave it implicit.
Non-blocking
- 🟡 LOW — success branch clobbers the current account's hero (no account guard).
HomePrompts.tsx:302— the two in-memory writes infundWallet's success path lack theaccountKeyRef.current === addressguard the catch branch has. Sequence: fund B (hero on B) → switch to A, fund A → switch back to B (resume restores B's hero) → A resolves andsetFundingWait({address:A})replaces B's wait; B's hero silently vanishes mid-mint, its arrival/backstop effects go inert, and re-tapping B double-mints. Narrow, but real — wrap both writes in the same guard the catch uses. - 🟡 LOW — "Funded!" amount re-mixes baseline notes.
HomePrompts.tsx:425— the sub-label sums all claimable notes, re-including the pre-existing baseline notes the arrival gate excluded, mis-attributing their value to this funding. Filter to ids beyondbaselineNoteIdsbefore totalling, or fall back to the generic sub-label. - 🟡 LOW — flaky success-beat tests.
HomePrompts.test.tsx:257,299,342— three completion assertions run on the real 2400msFAUCET_FUNDED_BEAT_MSwith a 600mswaitFormargin; under parallel CI load they can time out intermittently. Use fake timers (advanceTimersByTimeAsync(FAUCET_FUNDED_BEAT_MS)) like the backstop test at :438. - 🟡 LOW — dead code shipped. The redesign rewired Home to call
faucet()directly, soWalletFundingDrawer.tsx+lib/wallet-funding.ts(+ their two test files) are unreachable —openWalletFundinghas zero prod callers, andTabLayout's{walletFundingOpen && <WalletFundingDrawer/>}can never render. Recommend deleting the subsystem (and itsTabLayoutwiring) rather than shipping a duplicate faucet-request path plus tests that only prove the dead code still runs. If it's intended future work, gate/document it. - NIT — marker-clear writes are fire-and-forget.
HomePrompts.tsx:327,348,372— unlike the persist path (:299, which.catches), the threevoid setFaucetFundingMarker(address, null)clears have no.catch, so a storage write failure is an unhandled rejection (no crash/wedge — the in-memory backstop still clears within 3 min). Add the same.catch. - NIT — abort comment overclaims.
wallet-prompts.ts:263— a best-effort abort can't recall an already-dispatched/get_tokens, so a retry after a timeout can still mint a second note. Soften the comment, and/or distinguish the timeout rejection from a hard failure so an immediate retry doesn't race a late mint (minor — devnet test tokens). - NIT — CHANGELOG.
CHANGELOG.md:6— the 1.15.20 CHANGE bullet still describes the funding-drawer flow that this PR made unreachable; update it to the in-card lifecycle when you remove the drawer.
Verified clean
Abort/timeout plumbing (signal threaded through all three network steps + PoW loop, single timer cleared in finally, no unhandled rejection, timeout error distinct); marker read robust against malformed/failed storage (non-object/non-finite/non-array → null, reads .catched); rapid same-account double-tap fully guarded (ref latched synchronously before any await); empty/undefined notes on a fresh account safe; clock-skew/past-requestedAt math floored with no immediate-fire hazard; i18n complete across all 14 locales with the $amount$ placeholder; haptics self-gate isMobile; no any.
Overall: the correctness regressions are gone — I'd gate merge on #1 and a decision on #2, and the rest can ride along or follow up.
4 participants
Partially addresses #476 (the "show an explicit funding state from request through note availability" part — the recovery-prompt separation and security-copy items from that issue are not covered here).
Problem
On iOS, tapping Fund this wallet acked the faucet request within seconds, but the minted note only becomes visible after chain inclusion + a client sync (~30–60s). The prompt card flashed success on the ack and vanished, so the Home screen looked completely idle for the whole gap until the note notification appeared.
What changed
Funding lifecycle (HomePrompts):
faucet_funding_v2:<account>), and falls back to the actionable card 3 minutes after the original request if the mint never lands.PromptCard:
active:scale-[0.98]) on tappable cards, and a reusableherotakeover mode (icon tile + wordmark + sub-line lockup).hourglassicon added to the v2 icon set.Polish:
font-mediumJSX attribute that was never applied).Rebase note
This PR now targets
maindirectly (it previously stacked onfix/remove-imiden-swap-faucet, so until #498 merges the diff here includes that branch’s commits). Where the two collided (HomePrompts, its tests, locales), this branch's in-card funding lifecycle supersedes the base'sWalletFundingDrawerwiring — the drawer andlib/wallet-fundingremain in the tree but are no longer opened from the faucet card. The base's native-MIDEN-onlyfaucet()and swap-token ordering are preserved and compose with this flow.Review fixes (round 2)
Addressed from internal review:
requestedAt, not from the mount, so a resumed wait still ends 3 minutes after the original request.faucet()rejects if the PoW/mint request hangs, so the loading hero can't hold (and suppress the pending-notes card) with no in-session recovery.Review fixes (round 3)
faucet()aborts anAbortControllerwhen the timer fires; the signal is threaded throughgetPowChallenge/requestTokens(intofetch) and checked on every PoW iteration, so a delayed response can't mint a second note behind a retry and the PoW search stops burning CPU after the UI reports failure. Tests assert the signal actually aborts (and that the PoW loop stops), not just that the wrapper rejects.PromptCardis nowtabIndex={0}and activates on Enter/Space, with keys on the inner dismiss/CTA buttons ignored so one press can't fire both actions.requestedAtis captured beforeawait faucet(...), so a slow ack no longer stretches the wait toward ~4 minutes.Testing
HomePrompts,wallet-prompts,wallet-funding,WalletFundingDrawer,TabLayout), including coverage for: card-tap funding, the Funding→Funded! sequence, arrival via notes and via balance, the persisted-marker resume path, pending-notes suppression during the success beat, tap-to-retry after failure, the account-switch leak (hero stays off the other account, marker preserved, switch-back resumes), pre-existing-note no-instant-success, backstop timing anchored to the request (fake timers), per-account marker roundtrip/shape validation, and the faucet timeout.