Skip to content

fix(tangle-cloud): resolve round-2 payment review findings#3148

Merged
drewstone merged 2 commits into
developfrom
fix/payments-review-round2
Mar 21, 2026
Merged

fix(tangle-cloud): resolve round-2 payment review findings#3148
drewstone merged 2 commits into
developfrom
fix/payments-review-round2

Conversation

@drewstone
Copy link
Copy Markdown
Contributor

Summary

Fixes all findings from the codex/claude ensemble review on #3147.

Write queue fix

  • notesRef.current updated synchronously inside the promise chain so back-to-back addNote calls see each other's results (was dropping notes)

Credit key lifecycle

  • saveCreditKeys now requires encryption key (throws if missing — no more plaintext storage)
  • StoredCreditKeys gains isLocked field — locked accounts clearly marked in UI
  • deleteCreditKeys verifies owner address before deleting (was cross-wallet deletable)
  • FundCreditsContainer requires unlocked keypair, removed misleading proof progress
  • SpendAuthContainer rejects signing with locked/empty private keys

Wallet switch safety

  • CreditsProvider uses generation counter for stale-response guard
  • useKeypair resets isLoading on address change (prevents permanently stuck state)

IDB

  • onversionchange handler closes and invalidates stale connections across tabs

Layout regression fix

  • PageLayout defaults to md:px-5 (original instances/rewards/earnings padding)
  • blueprints/operators layouts explicitly pass md:px-8 lg:px-10 relative

UX honesty

  • Withdraw shows confirmed-notes balance only, not total (unconfirmed notes aren't spendable)
  • AmountInput MAX works correctly for 0n balance

Test plan

  • yarn nx typecheck tangle-cloud passes
  • yarn nx lint tangle-cloud passes (0 errors, 0 warnings)
  • yarn nx test tangle-cloud passes (52/52)

Write queue:
- notesRef.current updated synchronously inside queue so back-to-back
  addNote calls see each other's results

Credit key lifecycle:
- saveCreditKeys requires encryption key (throws if missing)
- StoredCreditKeys gains isLocked field — locked accounts clearly marked
- deleteCreditKeys verifies owner address before deleting
- FundCreditsContainer requires unlocked keypair, no misleading progress
- SpendAuthContainer rejects signing with locked/empty private keys

Wallet switch safety:
- CreditsProvider uses generation counter for stale-response guard
- useKeypair resets isLoading on address change (prevents stuck state)

IDB:
- onversionchange handler closes and invalidates stale connections

Layout regression:
- PageLayout defaults to md:px-5 (original instances/rewards/earnings)
- blueprints/operators layouts pass md:px-8 lg:px-10 relative

UX honesty:
- Withdraw shows confirmed-notes balance, not total shielded balance
- AmountInput MAX works for 0n balance (was falsy check on "0")
@drewstone drewstone requested a review from AtelyPham as a code owner March 21, 2026 00:45
@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 21, 2026

Deploy Preview for tangle-dapp ready!

Name Link
🔨 Latest commit c7df153
🔍 Latest deploy log https://app.netlify.com/projects/tangle-dapp/deploys/69bdeaa918c9d80008f5cc6a
😎 Deploy Preview https://deploy-preview-3148--tangle-dapp.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 21, 2026

Deploy Preview for tangle-leaderboard ready!

Name Link
🔨 Latest commit c7df153
🔍 Latest deploy log https://app.netlify.com/projects/tangle-leaderboard/deploys/69bdeaa9056ba40007e8eef5
😎 Deploy Preview https://deploy-preview-3148--tangle-leaderboard.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 21, 2026

Deploy Preview for tangle-cloud ready!

Name Link
🔨 Latest commit c7df153
🔍 Latest deploy log https://app.netlify.com/projects/tangle-cloud/deploys/69bdeaa94b99310008dff7a3
😎 Deploy Preview https://deploy-preview-3148--tangle-cloud.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@claude
Copy link
Copy Markdown

claude Bot commented Mar 21, 2026

Claude encountered an error —— View job


I'll analyze this and get back to you.

@drewstone drewstone merged commit dc116ec into develop Mar 21, 2026
20 checks passed
@drewstone drewstone deleted the fix/payments-review-round2 branch March 21, 2026 01:03
@drewstone
Copy link
Copy Markdown
Contributor Author

❌ PR Review w/ codex, claude

Don't merge. High: IndexedDB save failure poisons write queue and diverges in-memory state from persisted state in apps/tangle-cloud/src/app/ShieldedProvider.tsx:99. Medium: In-flight credit-key generation can append into the next wallet's state in apps/tangle-cloud/src/app/CreditsProvider.tsx:106. Medium: generateAndStoreCreditKeys can pollute state after address switch in apps/tangle-cloud/src/app/CreditsProvider.tsx:109. Plus 6 more findings.

Recommendation Needs Work (39/100)
Findings 9 total — 🔴 1 high, 🟠 5 medium, 🟡 3 low
Ensemble 2 reviewers × 3 tracks
Files reviewed 14 files changed
Validator claude
Provenance individual reviewer outputs

🔴 HIGH (1)

  • IndexedDB save failure poisons write queue and diverges in-memory state from persisted state apps/tangle-cloud/src/app/ShieldedProvider.tsx:99

    The persist callback chains writes via writeQueueRef.current.then(async () => { ... }). If storageRef.current.save() rejects, the returned promise rejects, which means writeQueueRef.current holds a rejected promise. Every subsequent .then() chained on it will be skipped — the entire write queue is dead. No future addNote, removeNote, or importNotes call will execute.

Before this PR, setNotes(updated) happened AFTER the save, so a save failure at least kept in-memory state cons…

🟠 MEDIUM (5)

  • In-flight credit-key generation can append into the next wallet's state apps/tangle-cloud/src/app/CreditsProvider.tsx:106

    CreditsProvider protects the reload effect with genRef, but generateAndStoreCreditKeys still captures address/encryptionKey, awaits saveCreditKeys(...), and then unconditionally does setCreditAccounts((prev) => [...prev, keys]). If the user starts generation on wallet A and switches wallets or changes lock state before the async save finishes, the old request can repopulate the current provider state with A's newly generated account. The stored record is written under the old owner…

  • generateAndStoreCreditKeys can pollute state after address switch apps/tangle-cloud/src/app/CreditsProvider.tsx:109

    The genRef guard correctly prevents stale loads from overwriting state, but generateAndStoreCreditKeys has no equivalent guard. If the user switches wallets while key generation is in-flight, the sequence is: (1) generateAndStoreCreditKeys captures address=A, encryptionKey=K1 via closure; (2) user switches to address B; (3) effect runs: setCreditAccounts([]), increments genRef, starts loading address B's keys; (4) the in-flight generate completes and calls setCreditAccounts(prev => [...prev, k…

  • Create-account warning uses stored-keypair state instead of actual unlocked state apps/tangle-cloud/src/containers/payments/FundCreditsContainer.tsx:57

    FundCreditsContainer shows its unlock warning only when !hasDerivedKeypair, but it disables the label input and submit button when !isUnlocked. Those are no longer the same condition: CreditsProvider.isUnlocked is only true when there is a live encryptionKey from an unlocked shielded keypair, while useKeypair still marks the address as having a keypair as soon as it finds a saved entry in local storage. In the common case where a user has previously derived a shielded keypair but has…

  • SpendAuthContainer lets users select locked accounts and fill out the entire form before discovering the account is locked apps/tangle-cloud/src/containers/payments/SpendAuthContainer.tsx:224

    The locked-account guard at line 77 only fires inside handleSign — after the user has selected a locked account, filled in service ID, job index, amount, operator, and expiry. There's no visual indication in the dropdown or form that the selected account is locked. The user discovers this only after clicking 'Sign Authorization'. This is a valid-user trap: someone who reconnects their wallet (keypair not yet unlocked) sees their old credit accounts listed as if they're usable, fills out the whol…

  • Failed note save now leaves optimistic state applied and poisons the write queue apps/tangle-cloud/src/app/ShieldedProvider.tsx:101

    updateQueuedNotes now mutates notesRef and React state before awaiting IndexedDB persistence, but the queue still has no rejection recovery. If storageRef.current.save(...) rejects once, the user sees the new notes in memory even though they were never durably stored, and writeQueueRef.current becomes a rejected promise. Because later calls chain with .then(...) and no rejection handler, every subsequent queued note update is skipped. In practice, one storage error can make shielded no…

🟡 LOW (3)

  • onversionchange closes DB under in-flight callers without recovery apps/tangle-cloud/src/utils/payments/db.ts:44

    When another tab triggers a schema upgrade, the onversionchange handler calls db.close() and nullifies dbPromise. Any code that already obtained the db reference from a prior openDb() call and has an in-flight or upcoming transaction will get an InvalidStateError. The next openDb() call will reconnect, but there's no retry or error propagation to the caller that was mid-operation. In practice this is rare (only happens during version bumps with multiple tabs), but a failed mid-operation save or …

  • FundCreditsContainer warning alert uses hasDerivedKeypair but button uses isUnlocked — different source, same semantics today, fragile coupling apps/tangle-cloud/src/containers/payments/FundCreditsContainer.tsx:57

    Line 57 shows the warning alert when !hasDerivedKeypair (from ShieldedProvider, meaning keypair === null). Line 91 disables the button when !isUnlocked (from CreditsProvider, meaning encryptionKey is falsy, which is derived from keypair being null). Today these are equivalent because encryptionKey is derived directly from keypair.privateKey. But the component pulls from two different providers for what is conceptually the same gate. If either derivation changes independently (e.g., CreditsPr…

  • SpendAuthContainer button disabled check doesn't account for locked accounts apps/tangle-cloud/src/containers/payments/SpendAuthContainer.tsx:339

    The Sign Authorization button's disabled condition (line 339-346) checks for selectedAccount truthiness but not selectedAccount.isLocked. A user can select a locked account and the button remains enabled. The runtime guard at line 77 catches this and shows an error, so it's not a functional bug, but the button should be disabled for locked accounts to match the pattern used in FundCreditsContainer where the button is proactively disabled rather than relying on runtime error messages.


🎯 What would get this approved

  • Fix [high] IndexedDB save failure poisons write queue and diverges in-memory state from persisted state in apps/tangle-cloud/src/app/ShieldedProvider.tsx

pr-reviewer v0.5.0 · review #1 · 2026-03-21T01:04:11.842126+00:00

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant