Readonly wallet#277
Conversation
WalkthroughAdds explicit read-only wallet support: introduces reader/writer SvcWallet, new initReadonlyWallet(pubkey), wallet.isReadonly flag, gates write actions/UI behind writer presence, updates types/libs/providers, UI changes, and e2e tests for readonly flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Init/Restore Screen
participant WalletCtx as Wallet Provider
participant SDK as Service Worker SDK
participant Storage
User->>UI: Enter key (npub or nsec)
alt npub (public only)
UI->>UI: decode npub (nip19)
UI->>WalletCtx: initReadonlyWallet(pubkey)
WalletCtx->>WalletCtx: initSvcWorkerWallet({ publicKey })
WalletCtx->>SDK: create readonly service worker (reader)
SDK->>Storage: persist readonly config
WalletCtx-->>UI: initialized (isReadonly=true)
UI->>UI: navigate to Wallet (readonly)
else nsec (private)
UI->>UI: decode nsec
UI->>WalletCtx: initWallet(privateKey)
WalletCtx->>WalletCtx: initSvcWorkerWallet({ privateKey, publicKey })
WalletCtx->>SDK: create writer + reader
SDK->>Storage: persist full wallet
WalletCtx-->>UI: initialized (isReadonly=false)
UI->>UI: navigate to Wallet (full)
end
sequenceDiagram
participant User
participant WalletScreen
participant WalletCtx
participant SDK as Service Worker
WalletScreen->>WalletCtx: read wallet.isReadonly
alt isReadonly = true
WalletScreen->>WalletCtx: svcWallet.reader.getBalance()
WalletCtx->>SDK: reader.getBalance()
SDK-->>WalletCtx: balance
WalletScreen-->>User: show balance, hide Send/Apps, show Readonly warning
else isReadonly = false
User->>WalletScreen: Tap Send
WalletScreen->>WalletCtx: svcWallet.writer.sendBitcoin()
WalletCtx->>SDK: writer signs & broadcasts
SDK-->>WalletCtx: tx result
WalletScreen-->>User: show send success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying wallet-bitcoin with
|
| Latest commit: |
83c3836
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c8d0dd64.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://readonly-wallet.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
83c3836
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://809b0896.arkade-wallet.pages.dev |
| Branch Preview URL: | https://readonly-wallet.arkade-wallet.pages.dev |
pietro909
force-pushed
the
readonly-wallet
branch
from
58abcf0 to
ddbf039
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/providers/wallet.tsx (1)
214-232: Fix closure capturing stalesvcWalletstate.The
handleServiceWorkerMessagesclosure capturessvcWalletfrom the React state at the timeinitSvcWorkerWalletis called. SincesetSvcWallet(newSvcWallet)(line 212) updates state asynchronously, the capturedsvcWalletwill be stale (undefined or the previous wallet). When messages arrive,reloadWalletwill use the wrong wallet instance.Apply this diff to use the newly created wallet:
const handleServiceWorkerMessages = (event: MessageEvent) => { if (event.data && ['VTXO_UPDATE', 'UTXO_UPDATE'].includes(event.data.type)) { - reloadWallet(svcWallet) + reloadWallet(newSvcWallet) // reload again after a delay to give the indexer time to update its cache - setTimeout(() => reloadWallet(svcWallet), 5000) + setTimeout(() => reloadWallet(newSvcWallet), 5000) } }
🧹 Nitpick comments (2)
src/screens/Init/Connect.tsx (1)
21-29: Potential stale closure issue withinitInforeference.The
initInfoin the.thencallback on line 24 captures the value from the initial render. IfinitInfochanges before the promise resolves, the spread will use stale data. This matches the existing pattern on line 34 but warrants attention.Consider using a functional update if
setInitInfosupports it, or simply pass only the fields that need clearing:initReadonlyWallet(publicKey) .then(() => { - setInitInfo({ ...initInfo, publicKey: undefined }) + setInitInfo((prev) => ({ ...prev, publicKey: undefined })) navigate(Pages.Wallet) }) .catch(consoleError)If
setInitInfodoesn't support functional updates, the current approach is acceptable given the empty dependency array ensures this runs only once.src/screens/Init/Restore.tsx (1)
38-68: Clarify the error message for compressed public key validation.At line 44, the error message states "Compressed public key must be 33 bytes," but the check compares
someKey.length !== 66. While technically correct (33 bytes = 66 hex characters), the message could be clearer for users entering a hex string.Apply this diff to clarify the error message:
- if (someKey.length !== 66) throw new Error('Compressed public key must be 33 bytes') + if (someKey.length !== 66) throw new Error('Compressed public key must be 66 hex characters (33 bytes)')
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/App.tsx(1 hunks)src/components/Keyboard.tsx(1 hunks)src/components/Refresher.tsx(1 hunks)src/lib/asp.ts(5 hunks)src/lib/types.ts(1 hunks)src/lib/utxo.ts(2 hunks)src/lib/wallet.ts(2 hunks)src/providers/flow.tsx(1 hunks)src/providers/lightning.tsx(3 hunks)src/providers/navigation.tsx(3 hunks)src/providers/wallet.tsx(10 hunks)src/screens/Apps/Lendasat/Index.tsx(4 hunks)src/screens/Apps/Lendaswap/Index.tsx(3 hunks)src/screens/Init/Connect.tsx(1 hunks)src/screens/Init/Init.tsx(2 hunks)src/screens/Init/Restore.tsx(2 hunks)src/screens/Settings/Backup.tsx(4 hunks)src/screens/Settings/Support.tsx(1 hunks)src/screens/Settings/Vtxos.tsx(3 hunks)src/screens/Wallet/Index.tsx(2 hunks)src/screens/Wallet/Notes/Redeem.tsx(1 hunks)src/screens/Wallet/Receive/Amount.tsx(1 hunks)src/screens/Wallet/Send/Details.tsx(1 hunks)src/screens/Wallet/Send/Form.tsx(2 hunks)src/screens/Wallet/Transaction.tsx(1 hunks)src/test/e2e/init.test.ts(1 hunks)src/test/screens/mocks.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/components/Keyboard.tsxsrc/screens/Settings/Support.tsxsrc/screens/Wallet/Receive/Amount.tsxsrc/screens/Apps/Lendaswap/Index.tsxsrc/lib/wallet.tssrc/providers/flow.tsxsrc/providers/lightning.tsxsrc/screens/Wallet/Transaction.tsxsrc/lib/utxo.tssrc/test/screens/mocks.tssrc/components/Refresher.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Apps/Lendasat/Index.tsxsrc/screens/Wallet/Index.tsxsrc/screens/Wallet/Send/Details.tsxsrc/lib/types.tssrc/screens/Wallet/Notes/Redeem.tsxsrc/screens/Wallet/Send/Form.tsxsrc/screens/Init/Connect.tsxsrc/providers/wallet.tsxsrc/lib/asp.ts
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/components/Keyboard.tsxsrc/screens/Init/Init.tsxsrc/providers/flow.tsxsrc/App.tsxsrc/screens/Init/Restore.tsxsrc/screens/Wallet/Index.tsxsrc/screens/Init/Connect.tsxsrc/providers/wallet.tsxsrc/screens/Settings/Backup.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/components/Keyboard.tsxsrc/App.tsxsrc/screens/Init/Restore.tsxsrc/screens/Init/Connect.tsxsrc/screens/Settings/Backup.tsx
📚 Learning: 2025-06-30T18:23:34.163Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
Applied to files:
src/components/Keyboard.tsxsrc/screens/Apps/Lendaswap/Index.tsxsrc/providers/lightning.tsxsrc/providers/wallet.tsxsrc/lib/asp.ts
📚 Learning: 2025-07-07T14:41:35.991Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Applied to files:
src/screens/Apps/Lendaswap/Index.tsxsrc/providers/lightning.tsxsrc/lib/asp.ts
🧬 Code graph analysis (18)
src/screens/Settings/Support.tsx (1)
src/lib/asp.ts (1)
getReceivingAddresses(194-200)
src/screens/Wallet/Receive/Amount.tsx (1)
src/lib/asp.ts (1)
getReceivingAddresses(194-200)
src/screens/Apps/Lendaswap/Index.tsx (2)
src/lib/asp.ts (2)
getReceivingAddresses(194-200)collaborativeExit(44-78)src/lib/address.ts (1)
isBTCAddress(22-26)
src/screens/Init/Init.tsx (1)
src/components/Button.tsx (1)
Button(21-64)
src/lib/wallet.ts (2)
src/lib/types.ts (1)
Vtxo(97-97)src/providers/asp.tsx (1)
AspInfo(6-6)
src/providers/lightning.tsx (1)
src/lib/asp.ts (1)
sendOffChain(224-226)
src/screens/Settings/Vtxos.tsx (2)
src/lib/logs.ts (1)
consoleError(40-44)src/lib/asp.ts (2)
getInputsToSettle(232-239)settleVtxos(241-268)
src/screens/Apps/Lendasat/Index.tsx (2)
src/lib/asp.ts (2)
getReceivingAddresses(194-200)collaborativeExit(44-78)src/lib/address.ts (2)
isArkAddress(13-20)isBTCAddress(22-26)
src/providers/navigation.tsx (1)
src/screens/Init/Restore.tsx (1)
InitRestore(24-139)
src/screens/Init/Restore.tsx (5)
src/lib/logs.ts (1)
consoleError(40-44)src/lib/error.ts (1)
extractError(1-10)src/lib/privateKey.ts (2)
nsecToPrivateKey(24-28)invalidPrivateKey(6-10)src/lib/constants.ts (1)
defaultPassword(7-7)src/lib/backup.ts (1)
BackupProvider(17-168)
src/screens/Wallet/Index.tsx (3)
src/providers/wallet.tsx (1)
WalletContext(54-69)src/components/Button.tsx (1)
Button(21-64)src/icons/Send.tsx (1)
SendIcon(1-12)
src/screens/Wallet/Send/Details.tsx (1)
src/lib/asp.ts (2)
sendOffChain(224-226)collaborativeExitWithFees(80-123)
src/screens/Wallet/Notes/Redeem.tsx (2)
src/components/Loading.tsx (1)
Loading(5-18)src/lib/asp.ts (1)
redeemNotes(202-222)
src/screens/Wallet/Send/Form.tsx (1)
src/lib/asp.ts (1)
getReceivingAddresses(194-200)
src/screens/Init/Connect.tsx (2)
src/providers/wallet.tsx (1)
WalletContext(54-69)src/lib/logs.ts (1)
consoleError(40-44)
src/providers/wallet.tsx (2)
src/lib/types.ts (1)
Wallet(99-107)src/lib/asp.ts (3)
getVtxos(181-192)getTxHistory(145-179)getBalance(139-143)
src/lib/asp.ts (1)
src/lib/types.ts (4)
Satoshis(37-37)Tx(71-81)Vtxo(97-97)Addresses(3-6)
src/screens/Settings/Backup.tsx (3)
src/lib/clipboard.ts (1)
copyToClipboard(3-11)src/lib/toast.ts (1)
copiedToClipboard(13-16)src/components/InputFake.tsx (1)
InputFake(3-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (41)
src/lib/types.ts (1)
106-106: LGTM! Clean addition of readonly flag.The optional
isReadonlyfield is a non-breaking change that enables the wallet to track its read-only state. This aligns well with the broader reader/writer separation pattern introduced throughout the PR.src/screens/Settings/Backup.tsx (4)
38-38: LGTM! Clean state management for public key.The new
pubkeystate and its initialization effect properly separate public key handling from the existing private key (nsec) logic.Also applies to: 49-53
64-74: LGTM! Well-structured copy handlers.The renaming of
handleCopytohandleNsecCopyand the addition ofhandlePubkeyCopyclearly separates the two copy operations. Both handlers follow the same pattern with proper null checks.
167-189: LGTM! Appropriate conditional rendering for readonly mode.The private key section is correctly hidden when
wallet.isReadonlyis true, preventing exposure of private key UI in readonly mode. The structure and messaging remain clear.
190-202: LGTM! Public key section properly updated.The public key section now uses the dedicated
pubkeystate andhandlePubkeyCopyhandler. The updated message clearly communicates the readonly restoration capability.src/lib/asp.ts (5)
139-143: LGTM! Appropriate use of IReadonlyWallet for read operation.The
getBalancefunction correctly usesIReadonlyWalletsince it only performs read operations. This aligns with the read/write separation pattern introduced in this PR.
145-179: LGTM! Correct readonly wallet usage for transaction history.The
getTxHistoryfunction appropriately usesIReadonlyWalletas it only reads transaction data without performing any write operations.
181-192: LGTM! Proper read-only interface for getVtxos.The
getVtxosfunction correctly usesIReadonlyWalletsince it only retrieves and categorizes VTXOs without modifying wallet state.
194-200: LGTM! Consistent readonly usage for address retrieval.The
getReceivingAddressesfunction appropriately usesIReadonlyWalletas it only retrieves addresses without performing write operations.
279-296: LGTM! Readonly wallet usage in error capture.The
captureSettleErrorfunction correctly usesIReadonlyWalletsince it only needs to read the wallet address for error context, not perform any write operations.src/screens/Settings/Support.tsx (1)
34-34: LGTM! Correct usage of reader interface.The call to
getReceivingAddressesnow usessvcWallet.reader, which aligns with the updated function signature that acceptsIReadonlyWalletinsrc/lib/asp.ts.src/components/Keyboard.tsx (1)
43-43: LGTM! Proper reader interface usage for balance retrieval.The balance fetch now uses
svcWallet.reader.getBalance(), correctly separating read operations to the reader interface.src/App.tsx (1)
72-72: LGTM! Correctly extends setup guard for readonly wallet.Adding
initInfo.publicKeyto the early-return condition prevents interrupting the readonly wallet initialization flow, consistent with howpasswordandprivateKeyare handled.src/screens/Wallet/Send/Form.tsx (2)
87-87: LGTM! Correct reader usage for address retrieval.The call to
getReceivingAddressesnow usessvcWallet.reader, aligning with the updated function signature insrc/lib/asp.ts.
108-110: LGTM! Proper reader interface for balance fetch.The balance retrieval now uses
svcWallet.reader.getBalance(), correctly routing read operations through the reader interface.src/providers/flow.tsx (1)
8-9: Add publicKey to emptyInitInfo for consistency.The
publicKeyfield was added to theInitInfointerface but not initialized inemptyInitInfo. For consistency withpasswordandprivateKey, which are also optional fields explicitly set toundefined,publicKeyshould be included in the initialization.src/screens/Apps/Lendaswap/Index.tsx (2)
22-33: Verify reader property safety.The
useEffectat line 25 accessessvcWallet.readerwithout optional chaining on thereaderproperty, even though it checkssvcWalletexists. IfsvcWalletexists butreaderisundefined, this will throw a runtime error.Consider adding defensive checks or verify that
readeris always present whensvcWalletexists.
70-109: LGTM! Proper writer usage for write operations.The code correctly:
- Checks for
svcWallet?.writerpresence before write operations- Uses
writer.sendBitcoinfor Ark address sends (line 82)- Passes
writertocollaborativeExitfor BTC address sends (line 89)This aligns with the read/write wallet split model where write operations require the writer interface.
src/screens/Wallet/Receive/Amount.tsx (1)
60-73: Verify reader property safety.The code checks
if (!svcWallet)but then accessessvcWallet.readerwithout optional chaining. Ifreadercan beundefinedwhensvcWalletexists, this will cause a runtime error.Verify whether
readeris always initialized whensvcWalletexists, or add defensive checks:if (!svcWallet) return -getReceivingAddresses(svcWallet.reader) +getReceivingAddresses(svcWallet.reader!)Or update the guard:
-if (!svcWallet) return +if (!svcWallet?.reader) returnsrc/lib/utxo.ts (1)
18-22: LGTM! Type signature correctly tightened.The function only performs read operations (
getBoardingUtxos), so changing the parameter type fromIWallettoIReadonlyWalletis appropriate and improves type safety. This correctly implements the read/write wallet split.src/test/e2e/init.test.ts (1)
23-32: LGTM! Good test coverage for readonly wallet flow.The test correctly validates:
- Navigation through the init flow to restore options
- Presence of both standard and readonly restore paths
- Uses
exact: truefor precise text matchingThis provides good coverage for the new readonly wallet feature.
src/screens/Wallet/Transaction.tsx (1)
69-76: LGTM! Correct writer usage for settlement operations.The code correctly:
- Guards on
svcWallet?.writerpresence (line 70)- Passes
writertogetInputsToSettlefor settlement operations (line 71)Settlement is a write operation, so using the writer interface is appropriate and aligns with the read/write wallet split.
src/screens/Wallet/Notes/Redeem.tsx (1)
43-56: LGTM! Proper writer usage for redeem operations.The code correctly:
- Guards on
svcWallet?.writerpresence (line 43) and returns early if absent- Uses non-null assertion
svcWallet.writer!safely (line 49) since the guard ensures presence- Routes redeem operations through the writer interface
Redeeming notes involves settlement (write operation), so using the writer interface is appropriate.
src/test/screens/mocks.ts (1)
82-99: LGTM! Mock properly extended for readonly wallet support.The addition of
initReadonlyWalletto the mock context (line 84) provides proper test support for the new readonly wallet initialization path. This is consistent with the existing mock pattern and enables testing of readonly wallet flows.src/components/Refresher.tsx (1)
9-9: The code is correct. Thereaderproperty is defined as required in theSvcWallettype (no optional operator), and initialization ensures it's always assigned before use. The codebase consistently applies optional chaining only tosvcWallet(which may be undefined) while accessingreaderdirectly, as it's guaranteed to exist whensvcWalletis defined.Likely an incorrect or invalid review comment.
src/screens/Init/Init.tsx (1)
40-40: LGTM!The new readonly restore handler and button follow the established patterns in this component. The button styling and disabled state handling are consistent with the existing "Restore wallet" option.
Also applies to: 61-67
src/screens/Wallet/Index.tsx (1)
27-27: LGTM!Correctly disables the Send button when
svcWallet.writeris unavailable, which properly enforces read-only mode while allowing users to still receive funds.Also applies to: 54-54
src/providers/navigation.tsx (1)
40-40: LGTM!The new
InitRestoreReadonlypage is properly integrated into the navigation system with correct tab mapping and component rendering. Passing thereadonlyprop toInitRestoreenables code reuse for both flows.Also applies to: 79-79, 122-123
src/screens/Wallet/Send/Details.tsx (1)
99-99: LGTM!The guard correctly ensures
svcWallet.writeris available before attempting write operations. BothsendOffChainandcollaborativeExitWithFeesrequireIWallet, andsvcWallet.writerprovides this.Also applies to: 102-102, 110-112
src/providers/lightning.tsx (1)
77-77: LGTM!The Lightning provider correctly uses
svcWallet.writerfor all write operations. TheArkadeLightninginstance is only created when a writer is available, andpayInvoiceproperly guards against missing writer before attempting the off-chain send.Also applies to: 89-90, 163-163, 171-171
src/screens/Settings/Vtxos.tsx (1)
88-96: LGTM!Correct separation of concerns: read operations (
getVtxos,getBoardingUtxos) usesvcWallet.reader, while write operations (getInputsToSettle,settleVtxos) usesvcWallet.writer. The non-null assertion on line 126 is safe given the loading guard on line 119.Also applies to: 101-102, 119-119, 126-126
src/lib/wallet.ts (1)
7-7: Type signature correctly narrowed toIReadonlyWallet.The function only performs read operations via
getConfirmedAndNotExpiredUtxos, so narrowing the type is appropriate. Verification confirmsgetConfirmedAndNotExpiredUtxosinsrc/lib/utxo.tsacceptsIReadonlyWallet.src/screens/Apps/Lendasat/Index.tsx (4)
32-40: LGTM!The address loading correctly uses
svcWallet.readerfor the read-only operation, which aligns with the new reader/writer separation model.
69-87: LGTM!The send operation correctly guards against missing writer and routes both Ark and Bitcoin address sends through the appropriate writer methods.
142-168: LGTM!Both signing operations (
onSignPsbtandonSignMessage) correctly require the writer, as these operations need access to the private key and should fail gracefully in readonly mode.
105-112: No issue - readonly wallets properly exposeidentity.compressedPublicKey().The
ServiceWorkerReadonlyWalletis initialized with anidentityproperty set toReadonlySingleKey.fromPublicKey()in the wallet provider. The check at line 106 validates thatsvcWalletexists, which ensures the reader with its initialized identity is available. The API is correctly used.Likely an incorrect or invalid review comment.
src/screens/Init/Restore.tsx (2)
72-87: LGTM!The dual-path
handleProceedcorrectly handles both readonly (public key only, no backup restoration) and full wallet restoration (private key with backup). The non-null assertion at line 79 is safe due to the disabled button guard.
93-115: LGTM!The readonly restoration UI provides clear labeling and helpful guidance to users, distinguishing it well from the standard private key restoration flow.
src/providers/wallet.tsx (3)
174-212: LGTM!The initialization logic correctly handles both private key (full wallet with reader = writer) and public key (readonly wallet with reader only) modes, with proper validation.
300-313: LGTM!The
initReadonlyWalletfunction correctly mirrorsinitWalletbut uses the public key path and setsisReadonly: true.
315-350: LGTM!The wallet lifecycle functions correctly use
readerfor read-only operations (lock, reset, status checks) andwriterfor write operations (settle), properly supporting both readonly and full wallet modes.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/screens/mocks.ts (1)
129-165: Update mockSvcWallet to use reader/writer sub-objects.The
mockSvcWalletmust includereaderandwritersub-objects to match the actualSvcWallettype definition:type SvcWallet = { reader: ServiceWorkerReadonlyWallet; writer?: ServiceWorkerWallet }Current usage throughout the codebase accesses wallet methods via
svcWallet.reader(e.g.,getVtxos(),getStatus()) andsvcWallet.writer(e.g.,sendBitcoin(),identity.sign()). The flat mock structure hides this mismatch via theas anycast in tests.
♻️ Duplicate comments (2)
src/providers/wallet.tsx (1)
212-252: Fix references to stalesvcWalletstate.After
setSvcWallet(newSvcWallet)at line 212, the code at lines 218, 220, 234, 240-243, and 249-252 references the oldsvcWalletstate variable. React state updates are asynchronous, sosvcWalletstill holds the previous value (likelyundefined). This causes:
- Line 234: Incorrect status check using stale wallet
- Lines 240-243: Interval polling the wrong wallet instance
- Lines 249-252: Attempting to renew coins on the wrong wallet
- Lines 218, 220: Message handler reloading the wrong wallet
Apply this diff to use the newly created wallet instance:
// handle messages from the service worker // we listen for UTXO/VTXO updates to refresh the tx history and balance const handleServiceWorkerMessages = (event: MessageEvent) => { if (event.data && ['VTXO_UPDATE', 'UTXO_UPDATE'].includes(event.data.type)) { - reloadWallet(svcWallet) + reloadWallet(newSvcWallet) // reload again after a delay to give the indexer time to update its cache - setTimeout(() => reloadWallet(svcWallet), 5000) + setTimeout(() => reloadWallet(newSvcWallet), 5000) } } // listen for messages from the service worker if (listeningForServiceWorker.current) { navigator.serviceWorker.removeEventListener('message', handleServiceWorkerMessages) navigator.serviceWorker.addEventListener('message', handleServiceWorkerMessages) } else { navigator.serviceWorker.addEventListener('message', handleServiceWorkerMessages) listeningForServiceWorker.current = true } // check if the service worker wallet is initialized - const { walletInitialized } = (await svcWallet?.reader.getStatus()) ?? { walletInitialized: false } + const { walletInitialized } = await newSvcWallet.reader.getStatus() setInitialized(walletInitialized) // ping the service worker wallet status every 1 second setInterval(async () => { try { - if (svcWallet) { - const { walletInitialized } = await svcWallet.reader.getStatus() + if (newSvcWallet) { + const { walletInitialized } = await newSvcWallet.reader.getStatus() setInitialized(walletInitialized) } } catch (err) { consoleError(err, 'Error pinging wallet status') } }, 1_000) - if (svcWallet?.writer) { + if (newSvcWallet.writer) { // renew expiring coins on startup - renewCoins(svcWallet.writer, aspInfo.dust, wallet.thresholdMs).catch(() => {}) + renewCoins(newSvcWallet.writer, aspInfo.dust, wallet.thresholdMs).catch(() => {}) }src/screens/Settings/Vtxos.tsx (1)
288-288: WarningBox is now rendered unconditionally.The success
WarningBoxis always visible in the date view, regardless of whether a renewal just succeeded. Line 256 correctly guards it with{success ? ... : null}.Apply this diff to restore conditional rendering:
- <WarningBox green text='Coins renewed successfully' /> + {success ? <WarningBox green text='Coins renewed successfully' /> : null}
🧹 Nitpick comments (2)
src/screens/Apps/Lendasat/Index.tsx (1)
105-112: Guard could be more defensive.The guard checks
!svcWalletbut then accessessvcWallet.reader.identity. Consider guarding against!svcWallet?.readerfor consistency with how writer guards are structured.onGetPublicKey: async () => { - if (!svcWallet) { + if (!svcWallet?.reader) { throw new Error('Wallet not initialized') } const pk = await svcWallet.reader.identity.compressedPublicKey()src/screens/Settings/Backup.tsx (1)
194-195: Consider handling empty public key state.If
wallet.pubkeyis not set (e.g., during loading or edge cases), theInputFakewill display an empty string and the copy button will still be clickable. Consider showing a loading state or disabling the button whenpubkeyis empty.- <InputFake testId='public-key' text={pubkey} /> - <Button onClick={handlePubkeyCopy} label='Copy to clipboard' /> + <InputFake testId='public-key' text={pubkey || 'Loading...'} /> + <Button onClick={handlePubkeyCopy} label='Copy to clipboard' disabled={!pubkey} />
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/App.tsx(1 hunks)src/components/Keyboard.tsx(1 hunks)src/components/Refresher.tsx(1 hunks)src/lib/asp.ts(5 hunks)src/lib/types.ts(1 hunks)src/lib/utxo.ts(2 hunks)src/lib/wallet.ts(2 hunks)src/providers/flow.tsx(1 hunks)src/providers/lightning.tsx(3 hunks)src/providers/navigation.tsx(3 hunks)src/providers/wallet.tsx(10 hunks)src/screens/Apps/Lendasat/Index.tsx(4 hunks)src/screens/Apps/Lendaswap/Index.tsx(3 hunks)src/screens/Init/Connect.tsx(1 hunks)src/screens/Init/Init.tsx(2 hunks)src/screens/Init/Restore.tsx(2 hunks)src/screens/Settings/Backup.tsx(4 hunks)src/screens/Settings/Support.tsx(1 hunks)src/screens/Settings/Vtxos.tsx(3 hunks)src/screens/Wallet/Index.tsx(2 hunks)src/screens/Wallet/Notes/Redeem.tsx(1 hunks)src/screens/Wallet/Receive/Amount.tsx(1 hunks)src/screens/Wallet/Send/Details.tsx(1 hunks)src/screens/Wallet/Send/Form.tsx(2 hunks)src/screens/Wallet/Transaction.tsx(1 hunks)src/test/e2e/init.test.ts(1 hunks)src/test/screens/mocks.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- src/screens/Wallet/Send/Details.tsx
- src/screens/Wallet/Index.tsx
- src/components/Keyboard.tsx
- src/lib/utxo.ts
- src/test/e2e/init.test.ts
- src/lib/wallet.ts
- src/App.tsx
- src/screens/Init/Restore.tsx
- src/screens/Wallet/Notes/Redeem.tsx
- src/lib/types.ts
- src/screens/Settings/Support.tsx
- src/screens/Wallet/Transaction.tsx
- src/screens/Wallet/Receive/Amount.tsx
- src/screens/Wallet/Send/Form.tsx
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Settings/Vtxos.tsxsrc/screens/Apps/Lendasat/Index.tsxsrc/providers/wallet.tsxsrc/lib/asp.tssrc/screens/Apps/Lendaswap/Index.tsxsrc/components/Refresher.tsxsrc/providers/lightning.tsxsrc/test/screens/mocks.tssrc/providers/flow.tsx
📚 Learning: 2025-06-30T18:23:34.163Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
Applied to files:
src/screens/Settings/Vtxos.tsxsrc/providers/wallet.tsxsrc/screens/Apps/Lendaswap/Index.tsxsrc/components/Refresher.tsxsrc/providers/lightning.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Init/Connect.tsxsrc/screens/Init/Init.tsxsrc/providers/wallet.tsxsrc/screens/Settings/Backup.tsxsrc/providers/flow.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Settings/Backup.tsx
📚 Learning: 2025-07-07T14:41:35.991Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Applied to files:
src/lib/asp.tssrc/screens/Apps/Lendaswap/Index.tsxsrc/providers/lightning.tsx
🧬 Code graph analysis (9)
src/screens/Settings/Vtxos.tsx (4)
src/lib/logs.ts (1)
consoleError(40-44)src/lib/asp.ts (2)
getInputsToSettle(232-239)settleVtxos(241-268)src/components/Loading.tsx (1)
Loading(5-18)src/components/Warning.tsx (1)
WarningBox(12-37)
src/screens/Apps/Lendasat/Index.tsx (1)
src/lib/asp.ts (2)
getReceivingAddresses(194-200)collaborativeExit(44-78)
src/screens/Init/Connect.tsx (2)
src/providers/wallet.tsx (1)
WalletContext(54-69)src/lib/logs.ts (1)
consoleError(40-44)
src/screens/Init/Init.tsx (1)
src/components/Button.tsx (1)
Button(21-64)
src/providers/wallet.tsx (5)
src/lib/types.ts (1)
Wallet(99-107)src/lib/wallet.ts (2)
calcBatchLifetimeMs(37-48)calcNextRollover(21-35)src/lib/constants.ts (1)
maxPercentage(9-9)src/lib/asp.ts (5)
getVtxos(181-192)getTxHistory(145-179)getBalance(139-143)renewCoins(270-273)settleVtxos(241-268)src/lib/explorers.ts (1)
getRestApiExplorerURL(34-36)
src/screens/Settings/Backup.tsx (3)
src/lib/clipboard.ts (1)
copyToClipboard(3-11)src/lib/toast.ts (1)
copiedToClipboard(13-16)src/components/InputFake.tsx (1)
InputFake(3-20)
src/lib/asp.ts (1)
src/lib/types.ts (4)
Satoshis(37-37)Tx(71-81)Vtxo(97-97)Addresses(3-6)
src/screens/Apps/Lendaswap/Index.tsx (1)
src/lib/asp.ts (2)
getReceivingAddresses(194-200)collaborativeExit(44-78)
src/providers/lightning.tsx (1)
src/lib/asp.ts (1)
sendOffChain(224-226)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (23)
src/components/Refresher.tsx (1)
9-9: No changes needed. The code is type-safe as written.The type definition confirms that
readeris a required property onSvcWallet(line 34:type SvcWallet = { reader: ServiceWorkerReadonlyWallet; writer?: ServiceWorkerWallet }). WhensvcWalletis defined,readeris guaranteed to exist, sosvcWallet?.reader.reload()will not throw a TypeError. Additional optional chaining at thereaderlevel is unnecessary.Likely an incorrect or invalid review comment.
src/screens/Apps/Lendaswap/Index.tsx (2)
25-25: LGTM!Correctly uses
svcWallet.readerfor the read-onlygetReceivingAddressescall, aligning with theIReadonlyWalletparameter type.
70-89: LGTM!Write operations are properly guarded by checking
svcWallet?.writerand consistently use thewriterforsendBitcoinandcollaborativeExitcalls.src/screens/Apps/Lendasat/Index.tsx (4)
34-34: LGTM!Correctly uses
svcWallet.readerfor the read-onlygetReceivingAddressescall.
69-89: LGTM!
onSendToAddressproperly guardssvcWallet?.writerand correctly useswriter.sendBitcoinandcollaborativeExit(svcWallet.writer, ...)for write operations.
142-152: LGTM!PSBT signing correctly guards
svcWallet?.writerand useswriter.identity.signfor the signing operation.
153-168: LGTM!Message signing correctly guards
svcWallet?.writerand useswriter.identity.signMessagefor the ECDSA signature.src/providers/flow.tsx (1)
5-11: LGTM!The optional
publicKeyfield is appropriately typed asUint8Arrayto match theprivateKeytype, and the comment clearly documents its purpose for readonly wallet initialization.src/test/screens/mocks.ts (1)
84-84: LGTM!Mock correctly added for the new
initReadonlyWalletfunction.src/screens/Init/Init.tsx (2)
40-40: LGTM!Clean addition of the readonly wallet restoration handler.
61-67: LGTM!Button is consistently styled with the existing "Restore wallet" option and clearly labeled as "(readonly)" to distinguish the two restoration paths.
src/providers/navigation.tsx (3)
40-40: LGTM!New enum value properly added for the readonly restore page.
79-79: LGTM!Correct tab mapping for the initialization flow.
122-123: LGTM!The readonly prop is passed to
InitRestoreto enable the readonly-specific UI flow.src/providers/lightning.tsx (2)
76-97: LGTM!The ArkadeLightning initialization correctly guards for
svcWallet?.writerand passessvcWallet.writerto the constructor. Lightning operations are write operations, so using the writer is appropriate.
162-172: LGTM!
payInvoicecorrectly guardssvcWallet?.writerand usessvcWallet.writerfor thesendOffChaincall.src/screens/Settings/Backup.tsx (4)
49-53: LGTM!Public key synchronization from
wallet.pubkeyis correctly implemented with a useEffect hook.
64-74: LGTM!Separate copy handlers for private key (nsec) and public key are appropriately implemented with proper null checks.
167-189: LGTM!Private key section is correctly hidden for readonly wallets using
!wallet.isReadonly. This prevents confusion since readonly wallets don't have access to the private key.
190-202: LGTM!Public key section is always visible and clearly explains that it can be used to restore in read-only mode. This enables users to back up their public key for readonly restoration.
src/screens/Init/Connect.tsx (1)
21-29: LGTM: Readonly wallet initialization path is correctly implemented.The logic properly initializes a readonly wallet when
publicKeyis present, clears sensitive data frominitInfo, and navigates appropriately. The early return ensures the password/privateKey flow is skipped for readonly wallets.src/lib/asp.ts (1)
139-139: LGTM: Read operations correctly acceptIReadonlyWallet.The updates properly distinguish read operations (getBalance, getTxHistory, getVtxos, getReceivingAddresses, captureSettleError) that now accept
IReadonlyWalletfrom write operations (settleVtxos, redeemNotes, collaborativeExit) that still requireIWallet. This aligns with readonly wallet support and maintains the correct contract boundaries.Based on learnings, the previous guidance about maintaining IWallet-only signatures was about avoiding multiple concrete types (IWallet AND ServiceWorkerWallet), not about restricting the use of a common readonly interface.
Also applies to: 145-145, 181-181, 194-194, 281-281
src/providers/wallet.tsx (1)
300-313: LGTM: Readonly wallet initialization is correctly implemented.The
initReadonlyWalletfunction properly extracts the public key, initializes the service worker with readonly credentials, updates configuration, and sets theisReadonlyflag. The parallel structure withinitWalletensures consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/providers/wallet.tsx (1)
214-231: Fix references to stale wallet state in message handler.Lines 218 and 220 call
reloadWallet(svcWallet)withininitSvcWorkerWallet, but at this pointsvcWalletis the stale React state. ThesetSvcWallet(newSvcWallet)call at line 212 hasn't updated the state yet, so these reload calls operate on the old wallet instance (or undefined on first init).Apply this diff to use the newly created wallet:
const handleServiceWorkerMessages = (event: MessageEvent) => { if (event.data && ['VTXO_UPDATE', 'UTXO_UPDATE'].includes(event.data.type)) { - reloadWallet(svcWallet) + reloadWallet(newSvcWallet) // reload again after a delay to give the indexer time to update its cache - setTimeout(() => reloadWallet(svcWallet), 5000) + setTimeout(() => reloadWallet(newSvcWallet), 5000) } }
♻️ Duplicate comments (1)
src/screens/Settings/Vtxos.tsx (1)
290-294: Restore conditional rendering based on success state.The success
WarningBoxat line 291 is now always visible in the date view wheneversvcWallet.writerexists, regardless of whether a renewal just succeeded. This differs from line 258 where it's correctly guarded by thesuccessstate. The message "Coins renewed successfully" should only appear immediately after a successful renewal.Apply this diff to restore conditional rendering:
- {svcWallet.writer ? ( - <WarningBox green text='Coins renewed successfully' /> - ) : ( - <WarningBox text='This is a readonly wallet' /> - )} + {success ? ( + <WarningBox green text='Coins renewed successfully' /> + ) : !svcWallet.writer ? ( + <WarningBox text='This is a readonly wallet' /> + ) : null}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/App.tsx(2 hunks)src/lib/utxo.ts(2 hunks)src/providers/navigation.tsx(4 hunks)src/providers/wallet.tsx(10 hunks)src/screens/Apps/Index.tsx(2 hunks)src/screens/Settings/Vtxos.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/App.tsx
- src/lib/utxo.ts
- src/providers/navigation.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Settings/Vtxos.tsxsrc/providers/wallet.tsx
📚 Learning: 2025-06-30T18:23:34.163Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
Applied to files:
src/screens/Settings/Vtxos.tsxsrc/providers/wallet.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Settings/Vtxos.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/providers/wallet.tsx
🧬 Code graph analysis (3)
src/screens/Apps/Index.tsx (6)
src/components/Content.tsx (1)
Content(9-16)src/components/Warning.tsx (1)
WarningBox(12-37)src/components/FlexCol.tsx (1)
FlexCol(16-46)src/screens/Apps/Lendasat/LendasatIcon.tsx (1)
LendasatIcon(1-13)src/screens/Apps/Lendaswap/LendaswapIcon.tsx (1)
LendaswapIcon(1-35)src/icons/FujiMoney.tsx (1)
FujiMoneyIcon(1-3)
src/screens/Settings/Vtxos.tsx (2)
src/lib/logs.ts (1)
consoleError(40-44)src/lib/asp.ts (2)
getInputsToSettle(232-239)settleVtxos(241-268)
src/providers/wallet.tsx (6)
src/lib/types.ts (1)
Wallet(99-107)src/lib/wallet.ts (1)
calcNextRollover(21-35)src/lib/constants.ts (1)
maxPercentage(9-9)src/lib/logs.ts (1)
consoleError(40-44)src/lib/explorers.ts (1)
getRestApiExplorerURL(34-36)src/lib/storage.ts (1)
clearStorage(4-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (11)
src/screens/Settings/Vtxos.tsx (3)
88-96: LGTM! Read operations correctly use the reader.The read operations (
getVtxos,getBoardingUtxos) correctly usesvcWallet.reader, which enables these calls to work in both readonly and read-write wallet modes.
101-109: Correct guard for write-operation precalculation.The guard on
svcWallet?.writeris appropriate here sincegetInputsToSettleis used to determine whether the renewal button should be shown. In readonly mode, the writer is undefined, so these state variables remain false and the renewal button is correctly hidden.
123-136: LGTM! Write operation properly guarded.The
handleRolloverfunction correctly checks forsvcWallet?.writerbefore performing the settlement operation, ensuring write operations are only attempted when the wallet has write capabilities.src/screens/Apps/Index.tsx (2)
14-14: LGTM! Clean addition of readonly prop.The
readonlyprop is correctly typed as optional and the component signature is properly updated to accept it.Also applies to: 94-95
101-133: LGTM! Appropriate readonly mode handling.The conditional rendering correctly prevents access to apps in readonly mode, showing a clear warning message instead. This aligns with the PR objective of disabling functionality that requires
IWallet.sendBitcoinorIWallet.settle.src/providers/wallet.tsx (6)
14-26: LGTM! Clean type definitions for read/write separation.The imports and
SvcWallettype correctly model the reader/writer split architecture, wherereaderis always present andwriteris optional (undefined in readonly mode). SettingdefaultWallet.isReadonly: trueis a safe default.Also applies to: 31-34
36-69: LGTM! Interface correctly extended for readonly support.The
WalletContextPropsinterface properly addsinitReadonlyWalletand updates thesvcWallettype to reflect the newSvcWalletstructure with reader/writer separation.
159-172: LGTM! Read operations correctly use reader.The
reloadWalletfunction correctly usesswWallet.readerfor all read operations (getVtxos,getTxHistory,getBalance), enabling it to work in both readonly and read-write wallet modes.
174-212: LGTM! Initialization correctly handles readonly and read-write paths.The
initSvcWorkerWalletfunction properly supports both modes:
- Private key: creates
ServiceWorkerWalletfor both writer and reader- Public key: creates
ServiceWorkerReadonlyWalletfor reader onlyThe logic correctly throws an error if neither key type is provided.
285-313: LGTM! Init functions correctly handle readonly and read-write modes.Both
initWalletandinitReadonlyWalletfollow the expected parallel structure:
initWalletderives the public key from the private key, initializes with write capabilities, and setsisReadonly: falseinitReadonlyWalletuses the provided public key directly, initializes in readonly mode, and setsisReadonly: true
315-350: LGTM! Wallet operations correctly use reader/writer.All operations correctly interact with the appropriate wallet instance:
lockWalletandresetWalletusereader(available in both modes)settlePreconfirmedguards onwriterand throws if unavailable (write-only operation)isLockedchecks status viareaderThis properly enforces the readonly constraint.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/screens/Init/Restore.tsx (3)
72-88: Consider consistency in non-null assertions.Line 80 uses a non-null assertion (
privateKey!) when creating the BackupProvider, but line 74 doesn't use a similar assertion forpublicKeyin the readonly path. Both are protected by thedisabledstate check at line 90, so neither will be undefined whenhandleProceedis called. For consistency and type safety clarity, consider either:
- Adding
publicKey!at line 74, or- Removing
privateKey!at line 80 (both are guaranteed non-null by the guard)Apply this diff for consistency:
if (readonly) { - setInitInfo({ publicKey, restoring: true }) + setInitInfo({ publicKey: publicKey!, restoring: true }) // BackupProvider.restore required a private key navigate(Pages.InitSuccess)
90-90: LGTM with optional simplification.The disabled logic correctly checks for the appropriate key based on the mode. The
Boolean()wrapper is redundant since the expression already evaluates to a boolean, but this doesn't affect functionality.Optional: Remove redundant
Boolean()wrapper:- const disabled = Boolean((readonly ? !publicKey : !privateKey) || error) + const disabled = (readonly ? !publicKey : !privateKey) || Boolean(error)
94-116: Rename input for clarity.Line 102 uses
name='ark-address'for the public key input, which is misleading since it accepts a compressed public key, not an ARK address. For consistency and clarity, consider usingname='public-key'to match the naming pattern of the private key input at line 125 (name='private-key').Apply this diff:
- <Input name='ark-address' label='Public Key (compressed)' onChange={setSomeKey} /> + <Input name='public-key' label='Public Key (compressed)' onChange={setSomeKey} />
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/providers/flow.tsx(2 hunks)src/screens/Init/Restore.tsx(2 hunks)src/screens/Wallet/Unlock.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/providers/flow.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Wallet/Unlock.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Wallet/Unlock.tsxsrc/screens/Init/Restore.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Wallet/Unlock.tsxsrc/screens/Init/Restore.tsx
🧬 Code graph analysis (2)
src/screens/Wallet/Unlock.tsx (4)
src/providers/wallet.tsx (1)
WalletContext(54-69)src/lib/storage.ts (1)
readWalletFromStorage(35-37)src/lib/logs.ts (1)
consoleError(40-44)src/lib/privateKey.ts (1)
getPrivateKey(40-44)
src/screens/Init/Restore.tsx (4)
src/lib/logs.ts (1)
consoleError(40-44)src/lib/privateKey.ts (2)
nsecToPrivateKey(24-28)invalidPrivateKey(6-10)src/lib/constants.ts (1)
defaultPassword(7-7)src/lib/backup.ts (1)
BackupProvider(17-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (7)
src/screens/Wallet/Unlock.tsx (2)
10-11: LGTM!The new imports are appropriate for the readonly wallet initialization flow.
32-43: LGTM!The existing wallet unlock logic has been correctly preserved in the else branch. The conditional error handling (only showing errors when a password was explicitly provided) maintains the original behavior.
src/screens/Init/Restore.tsx (5)
21-21: LGTM!The
hexToBytesimport from@noble/hashes/utils.jsis appropriate for converting the compressed public key hex string to bytes in readonly mode.
23-24: LGTM!The Props type and component signature changes correctly introduce the optional
readonlyflag to support the new restoration path.
34-34: LGTM!The
publicKeystate addition is appropriate and follows the same pattern as the existingprivateKeystate.
72-77: [rewritten review comment]
[classification tag]
57-58: Add "0x" prefix stripping for hex private key input at line 58.The
hex.decodefunction from@scure/base2.0.0 does not automatically handle "0x" prefixes. If a user provides a hex private key with a "0x" prefix, the decode will fail due to odd string length. Strip the prefix before decoding:else privateKey = hex.decode(someKey.replace(/^0x/, ''))
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/providers/wallet.tsx (2)
218-221: Stale closure:reloadWallet(svcWallet)uses outer state that may be undefined.When
handleServiceWorkerMessagesis registered, it captures the currentsvcWalletstate (likelyundefinedat registration time). When messages arrive,reloadWallet(svcWallet)will receiveundefinedinstead of the newly created wallet.Use
newSvcWalletdirectly:const handleServiceWorkerMessages = (event: MessageEvent) => { if (event.data && ['VTXO_UPDATE', 'UTXO_UPDATE'].includes(event.data.type)) { - reloadWallet(svcWallet) + reloadWallet(newSvcWallet) // reload again after a delay to give the indexer time to update its cache - setTimeout(() => reloadWallet(svcWallet), 5000) + setTimeout(() => reloadWallet(newSvcWallet), 5000) } }
265-271: MissingpublicKeyin retry call causes readonly wallet retry to fail.When a readonly wallet initialization times out, the retry call only passes
privateKey(which is undefined for readonly). This causes the retry to throw "No private key or public key provided".return initSvcWorkerWallet({ arkServerUrl, esploraUrl, privateKey, + publicKey, retryCount: retryCount + 1, maxRetries, })
♻️ Duplicate comments (1)
src/screens/Wallet/Unlock.tsx (1)
29-30: Add validation beforehexToBytesto prevent runtime exception.If the stored
pubkeyis malformed (not valid hex),hexToByteswill throw an unhandled exception. Wrap with validation or try-catch per the earlier review feedback.if (walletFromStorage?.isReadonly && walletFromStorage.pubkey) { + try { + if (!/^[0-9a-fA-F]+$/.test(walletFromStorage.pubkey)) { + throw new Error('Invalid public key format') + } initReadonlyWallet(hexToBytes(walletFromStorage.pubkey)) .then(() => navigate(Pages.Wallet)) .catch((err) => { consoleError(err, 'error initializing readonly wallet') setStage('failed-publickey') }) + } catch (err) { + consoleError(err, 'invalid pubkey in storage') + setStage('failed-publickey') + } } else {
🧹 Nitpick comments (1)
src/providers/wallet.tsx (1)
197-211: Unsafe type assertion could be avoided with proper initialization.The
as unknown as SvcWalletpattern bypasses type safety. Consider initializing with a more explicit pattern:- const newSvcWallet = { reader: undefined, writer: undefined } as unknown as SvcWallet + let reader: ServiceWorkerReadonlyWallet + let writer: ServiceWorkerWallet | undefined if (privateKey) { - newSvcWallet.writer = await ServiceWorkerWallet.setup({ + writer = await ServiceWorkerWallet.setup({ ...initSvcWallet, identity: SingleKey.fromHex(privateKey!), }) - newSvcWallet.reader = newSvcWallet.writer + reader = writer } else if (publicKey) { - newSvcWallet.reader = await ServiceWorkerReadonlyWallet.setup({ + reader = await ServiceWorkerReadonlyWallet.setup({ ...initSvcWallet, identity: ReadonlySingleKey.fromPublicKey(hexToBytes(publicKey)), }) } else { throw new Error('No private key or public key provided') } - setSvcWallet(newSvcWallet) + const newSvcWallet: SvcWallet = { reader, writer } + setSvcWallet(newSvcWallet)This ensures
readeris always assigned before creatingnewSvcWallet.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/providers/wallet.tsx(10 hunks)src/screens/Settings/Vtxos.tsx(3 hunks)src/screens/Wallet/Unlock.tsx(1 hunks)src/test/e2e/nostr.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/Settings/Vtxos.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Wallet/Unlock.tsxsrc/test/e2e/nostr.test.tssrc/providers/wallet.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Wallet/Unlock.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Wallet/Unlock.tsxsrc/providers/wallet.tsx
📚 Learning: 2025-06-30T18:23:34.163Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
Applied to files:
src/providers/wallet.tsx
🧬 Code graph analysis (2)
src/screens/Wallet/Unlock.tsx (9)
src/providers/wallet.tsx (1)
WalletContext(54-69)src/providers/navigation.tsx (1)
NavigationContext(173-177)src/lib/constants.ts (1)
defaultPassword(7-7)src/lib/storage.ts (2)
readWalletFromStorage(35-37)clearStorage(4-8)src/lib/privateKey.ts (1)
getPrivateKey(40-44)src/components/NeedsPassword.tsx (1)
NeedsPassword(21-57)src/components/Content.tsx (1)
Content(9-16)src/components/Padded.tsx (1)
Padded(7-14)src/components/Warning.tsx (1)
WarningBox(12-37)
src/providers/wallet.tsx (3)
src/lib/constants.ts (1)
maxPercentage(9-9)src/lib/asp.ts (4)
getVtxos(181-192)getBalance(139-143)renewCoins(270-273)settleVtxos(241-268)src/lib/explorers.ts (1)
getRestApiExplorerURL(34-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (5)
src/test/e2e/nostr.test.ts (1)
86-86: LGTM! Necessary for test reliability with new readonly restore option.Adding
{ exact: true }correctly prevents these tests from accidentally matching "Restore wallet (readonly)" when they intend to restore a full wallet with write capabilities. This is consistent with the existing pattern used for 'backup' and 'general' selectors elsewhere in the file.Also applies to: 212-212
src/providers/wallet.tsx (4)
31-31: Verify thatisReadonly: trueas default is intentional.The default wallet state has
isReadonly: true. Ensure this doesn't affect any conditional logic that checkswallet.isReadonlybefore initialization completes.
300-313: LGTM: Clean implementation of readonly wallet initialization.The
initReadonlyWalletfunction properly encodes the public key, initializes the service worker with readonly identity, and sets the wallet state withisReadonly: true.
328-332: Appropriate guard for write operation.
settlePreconfirmedcorrectly checks forsvcWallet?.writerbefore attempting the write operation, ensuring readonly wallets cannot trigger settlement.
159-172: LGTM: Read operations correctly useswWallet.reader.
reloadWalletproperly routes all read operations through the reader interface.
pietro909
force-pushed
the
readonly-wallet
branch
from
7805769 to
eb6ebd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/providers/wallet.tsx (2)
216-222: Stale closure: message handler uses outdatedsvcWalletreference.The handler captures
svcWalletfrom the outer scope at the timeinitSvcWorkerWalletruns. Since React state updates are asynchronous,svcWalletwill still be the old value (likelyundefined) when messages arrive, causingreloadWalletto no-op.🔎 Apply this diff to use the newly created wallet:
const handleServiceWorkerMessages = (event: MessageEvent) => { if (event.data && ['VTXO_UPDATE', 'UTXO_UPDATE'].includes(event.data.type)) { - reloadWallet(svcWallet) + reloadWallet(newSvcWallet) // reload again after a delay to give the indexer time to update its cache - setTimeout(() => reloadWallet(svcWallet), 5000) + setTimeout(() => reloadWallet(newSvcWallet), 5000) } }
265-271: Retry logic dropspublicKey, breaking readonly wallet retries.When service worker activation times out for a readonly wallet, the retry call only forwards
privateKey(which will beundefined), causing the retry to throw "No private key or public key provided".🔎 Apply this diff to forward publicKey in retries:
return initSvcWorkerWallet({ arkServerUrl, esploraUrl, privateKey, + publicKey, retryCount: retryCount + 1, maxRetries, })
♻️ Duplicate comments (1)
src/screens/Init/Restore.tsx (1)
41-53: Validation still missing "0x" prefix handling and byte length check.The issues raised in the previous review remain unaddressed:
hexToBytes(someKey)will fail if the user provides a "0x"-prefixed public key- No validation that the decoded byte array is exactly 33 bytes after conversion
🔎 Suggested fix:
if (readonly) { let pubkey: Uint8Array | undefined = undefined try { - if (someKey.length !== 66) throw new Error('Compressed public key must be 33 bytes') - pubkey = hexToBytes(someKey) + const cleanKey = someKey.startsWith('0x') ? someKey.slice(2) : someKey + if (cleanKey.length !== 66) throw new Error('Compressed public key must be 33 bytes (66 hex characters)') + pubkey = hexToBytes(cleanKey) + if (pubkey.length !== 33) throw new Error('Invalid public key: must be exactly 33 bytes') setError('') setLabel(buttonLabel) } catch (e) {
🧹 Nitpick comments (4)
src/screens/Settings/Vtxos.tsx (1)
119-119: Consider checkingsvcWallet?.readerfor consistency.The guard checks
!svcWalletbut since lines 88-96 accesssvcWallet.reader, it would be more explicit to check!svcWallet?.readerto ensure the reader is available before attempting read operations.🔎 Suggested change:
- if (!svcWallet) return <Loading text='Loading...' /> + if (!svcWallet?.reader) return <Loading text='Loading...' />src/screens/Init/Connect.tsx (1)
20-29: Consider adding user feedback on readonly initialization failure.If
initReadonlyWalletfails, the error is only logged viaconsoleErrorbut the user remains on the "Connecting to server" loading screen with no indication of the failure. Consider navigating back to the init screen or displaying an error message.🔎 Suggested improvement:
if (publicKey) { initReadonlyWallet(publicKey) .then(() => { setInitInfo({ ...initInfo, publicKey: undefined }) navigate(Pages.Wallet) }) - .catch(consoleError) + .catch((err) => { + consoleError(err, 'error initializing readonly wallet') + navigate(Pages.Init) + }) return }src/screens/Wallet/Unlock.tsx (2)
26-47: Add validation for pubkey beforehexToBytesconversion.If
walletFromStorage.pubkeycontains invalid hex characters,hexToByteswill throw an exception. While this is caught and transitions to'failed-publickey', adding validation would provide a clearer error path.Additionally, the effect re-runs on
passwordchanges, but for readonly wallets the password is irrelevant. Consider adding a guard to prevent redundant readonly initialization attempts.🔎 Suggested improvement:
useEffect(() => { const pass = password ? password : defaultPassword const walletFromStorage = readWalletFromStorage() if (walletFromStorage?.isReadonly && walletFromStorage.pubkey) { + // Validate pubkey is valid hex + if (!/^[0-9a-fA-F]+$/.test(walletFromStorage.pubkey)) { + consoleError('Invalid public key format in storage') + setStage('failed-publickey') + return + } initReadonlyWallet(hexToBytes(walletFromStorage.pubkey)) .then(() => navigate(Pages.Wallet)) .catch((err) => { consoleError(err, 'error initializing readonly wallet') setStage('failed-publickey') }) } else {
50-52: Consider adding confirmation before wallet reset.
handleResetimmediately clears storage and reloads the page. For a destructive action, consider adding a confirmation dialog to prevent accidental data loss.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
src/App.tsx(2 hunks)src/components/Keyboard.tsx(1 hunks)src/components/Refresher.tsx(1 hunks)src/lib/asp.ts(5 hunks)src/lib/types.ts(1 hunks)src/lib/utxo.ts(2 hunks)src/lib/wallet.ts(2 hunks)src/providers/flow.tsx(2 hunks)src/providers/lightning.tsx(3 hunks)src/providers/navigation.tsx(4 hunks)src/providers/wallet.tsx(10 hunks)src/screens/Apps/Index.tsx(2 hunks)src/screens/Apps/Lendasat/Index.tsx(4 hunks)src/screens/Apps/Lendaswap/Index.tsx(3 hunks)src/screens/Init/Connect.tsx(1 hunks)src/screens/Init/Init.tsx(2 hunks)src/screens/Init/Restore.tsx(2 hunks)src/screens/Settings/Backup.tsx(4 hunks)src/screens/Settings/Support.tsx(1 hunks)src/screens/Settings/Vtxos.tsx(3 hunks)src/screens/Wallet/Index.tsx(2 hunks)src/screens/Wallet/Notes/Redeem.tsx(1 hunks)src/screens/Wallet/Receive/Amount.tsx(1 hunks)src/screens/Wallet/Send/Details.tsx(1 hunks)src/screens/Wallet/Send/Form.tsx(2 hunks)src/screens/Wallet/Transaction.tsx(1 hunks)src/screens/Wallet/Unlock.tsx(1 hunks)src/test/e2e/init.test.ts(1 hunks)src/test/e2e/nostr.test.ts(2 hunks)src/test/screens/mocks.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/lib/types.ts
- src/components/Keyboard.tsx
- src/test/e2e/init.test.ts
- src/components/Refresher.tsx
- src/lib/asp.ts
- src/screens/Wallet/Notes/Redeem.tsx
- src/screens/Wallet/Receive/Amount.tsx
- src/test/screens/mocks.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Send/Form.tsxsrc/providers/flow.tsxsrc/screens/Wallet/Transaction.tsxsrc/screens/Apps/Lendaswap/Index.tsxsrc/providers/lightning.tsxsrc/screens/Wallet/Index.tsxsrc/screens/Settings/Support.tsxsrc/screens/Wallet/Unlock.tsxsrc/screens/Init/Connect.tsxsrc/lib/wallet.tssrc/lib/utxo.tssrc/providers/wallet.tsxsrc/App.tsxsrc/screens/Wallet/Send/Details.tsxsrc/test/e2e/nostr.test.tssrc/screens/Apps/Lendasat/Index.tsx
📚 Learning: 2025-06-30T18:23:34.163Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
Applied to files:
src/screens/Settings/Vtxos.tsxsrc/providers/lightning.tsxsrc/providers/wallet.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Unlock.tsxsrc/screens/Init/Restore.tsxsrc/screens/Settings/Backup.tsxsrc/screens/Init/Connect.tsxsrc/App.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/providers/flow.tsxsrc/screens/Wallet/Unlock.tsxsrc/screens/Init/Restore.tsxsrc/screens/Settings/Backup.tsxsrc/screens/Init/Connect.tsxsrc/screens/Init/Init.tsxsrc/providers/wallet.tsxsrc/App.tsx
📚 Learning: 2025-07-07T14:41:35.991Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Applied to files:
src/providers/lightning.tsx
🧬 Code graph analysis (18)
src/screens/Settings/Vtxos.tsx (1)
src/lib/asp.ts (2)
getInputsToSettle(232-239)settleVtxos(241-268)
src/screens/Wallet/Send/Form.tsx (1)
src/lib/asp.ts (1)
getReceivingAddresses(194-200)
src/screens/Wallet/Transaction.tsx (1)
src/lib/asp.ts (1)
getInputsToSettle(232-239)
src/screens/Apps/Lendaswap/Index.tsx (1)
src/lib/asp.ts (2)
getReceivingAddresses(194-200)collaborativeExit(44-78)
src/providers/lightning.tsx (1)
src/lib/asp.ts (1)
sendOffChain(224-226)
src/screens/Wallet/Index.tsx (2)
src/providers/wallet.tsx (1)
WalletContext(54-69)src/components/Button.tsx (1)
Button(21-64)
src/screens/Settings/Support.tsx (1)
src/lib/asp.ts (1)
getReceivingAddresses(194-200)
src/screens/Wallet/Unlock.tsx (9)
src/providers/wallet.tsx (1)
WalletContext(54-69)src/lib/constants.ts (1)
defaultPassword(7-7)src/lib/storage.ts (2)
readWalletFromStorage(35-37)clearStorage(4-8)src/lib/logs.ts (1)
consoleError(40-44)src/lib/privateKey.ts (1)
getPrivateKey(40-44)src/components/Content.tsx (1)
Content(9-16)src/components/Padded.tsx (1)
Padded(7-14)src/components/Warning.tsx (1)
WarningBox(12-37)src/components/Button.tsx (1)
Button(21-64)
src/screens/Init/Restore.tsx (4)
src/lib/logs.ts (1)
consoleError(40-44)src/lib/privateKey.ts (2)
nsecToPrivateKey(24-28)invalidPrivateKey(6-10)src/lib/constants.ts (1)
defaultPassword(7-7)src/lib/backup.ts (1)
BackupProvider(17-168)
src/screens/Settings/Backup.tsx (4)
src/lib/clipboard.ts (1)
copyToClipboard(3-11)src/lib/toast.ts (1)
copiedToClipboard(13-16)src/components/InputFake.tsx (1)
InputFake(3-20)src/components/Button.tsx (1)
Button(21-64)
src/screens/Init/Connect.tsx (2)
src/providers/wallet.tsx (1)
WalletContext(54-69)src/lib/logs.ts (1)
consoleError(40-44)
src/screens/Init/Init.tsx (1)
src/components/Button.tsx (1)
Button(21-64)
src/lib/wallet.ts (2)
src/lib/types.ts (1)
Vtxo(97-97)src/providers/asp.tsx (1)
AspInfo(6-6)
src/App.tsx (1)
src/providers/navigation.tsx (1)
pageComponent(102-165)
src/screens/Wallet/Send/Details.tsx (1)
src/lib/asp.ts (2)
sendOffChain(224-226)collaborativeExitWithFees(80-123)
src/screens/Apps/Lendasat/Index.tsx (1)
src/lib/asp.ts (2)
getReceivingAddresses(194-200)collaborativeExit(44-78)
src/providers/navigation.tsx (3)
src/screens/Apps/Index.tsx (1)
Apps(95-138)src/screens/Init/Init.tsx (1)
Init(19-72)src/screens/Init/Restore.tsx (1)
InitRestore(24-140)
src/screens/Apps/Index.tsx (2)
src/components/Warning.tsx (1)
WarningBox(12-37)src/icons/FujiMoney.tsx (1)
FujiMoneyIcon(1-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (42)
src/providers/lightning.tsx (1)
77-77: LGTM! Correct routing of write operations throughsvcWallet.writer.All changes properly gate Lightning/Boltz integration behind the writer wallet:
- ArkadeLightning initialization uses
svcWallet.writerfor write-capable operations- Guards correctly check for
svcWallet?.writerpresencesendOffChaincorrectly receivessvcWallet.writeras its IWallet parameterThis aligns with the readonly wallet separation pattern.
Also applies to: 90-90, 163-163, 171-171
src/screens/Settings/Support.tsx (1)
34-34: LGTM! Correct use of reader for read-only operation.The change correctly routes
getReceivingAddressesthroughsvcWallet.reader, which aligns with the function signature expectingIReadonlyWallet.src/screens/Wallet/Transaction.tsx (1)
70-71: LGTM! Proper use of writer for settlement-related operations.The change correctly gates
getInputsToSettlebehindsvcWallet.writersince settlement operations require write access. The guard ensures the writer is available before attempting to gather inputs.src/screens/Wallet/Send/Form.tsx (1)
87-87: LGTM! Correct use of reader for read-only operations.Both changes properly route read-only operations through
svcWallet.reader:
getReceivingAddressesis a read-only operationgetBalanceretrieves balance information without modificationThis aligns with the readonly wallet pattern.
Also applies to: 108-111
src/test/e2e/nostr.test.ts (1)
86-86: LGTM! Improved test selector precision.Adding
{ exact: true }to the "Restore wallet" text selectors ensures the tests target the correct button, especially important now that there may be multiple restore options (e.g., readonly vs full wallet restoration).Also applies to: 212-212
src/screens/Apps/Lendaswap/Index.tsx (2)
25-25: LGTM! Correct use of reader for address retrieval.The change properly routes
getReceivingAddressesthroughsvcWallet.readerfor the read-only operation of fetching Ark addresses.
71-71: LGTM! Proper routing of write operations through writer.All write operations correctly use
svcWallet.writer:
- Guard checks for writer presence before allowing sends
sendBitcoinroutes through writer for Ark address sendscollaborativeExitreceives writer for on-chain settlementThis aligns with the learned pattern that
collaborativeExitaccepts IWallet.Also applies to: 82-82, 89-89
src/providers/flow.tsx (1)
8-9: LGTM! Clean addition of publicKey field for readonly wallet support.The new
publicKeyfield inInitInfoenables readonly wallet initialization. The field is:
- Properly typed as
Uint8Array- Clearly documented with a comment
- Correctly initialized to
undefinedin the empty stateAlso applies to: 69-69
src/App.tsx (2)
72-72: LGTM! Proper guard for readonly wallet initialization.The expanded guard correctly prevents navigation away from the initialization flow when
publicKeyis set, treating it consistently withpasswordandprivateKeyinitialization paths.
121-121: LGTM! Proper propagation of readonly state to page components.The change correctly passes the readonly flag to
pageComponent, enabling pages (like Apps) to adapt their UI based on whether the wallet is in readonly mode. This aligns with the updatedpageComponentsignature.src/screens/Wallet/Index.tsx (1)
27-27: LGTM! Proper gating for readonly wallet mode.The Send button is correctly disabled when
svcWallet?.writeris unavailable, allowing readonly wallets to view balance and transaction history while preventing send operations. The Receive button remains accessible, which is appropriate since generating receive addresses is a read operation.Also applies to: 54-54
src/screens/Init/Init.tsx (1)
40-40: LGTM! Clean addition of readonly wallet restore flow.The new readonly restore option follows the same pattern as the existing restore wallet flow - properly disabled on error, consistent styling with
fancyandsecondaryprops, and clear labeling that distinguishes it from the full restore.Also applies to: 61-67
src/screens/Wallet/Send/Details.tsx (1)
99-99: LGTM! Proper writer-based gating for send operations.The guard correctly checks for
svcWallet?.writerbefore proceeding with send operations. BothsendOffChainandcollaborativeExitWithFeesreceive the writer instance which aligns with theirIWallettype signatures.Also applies to: 102-102, 110-112
src/lib/wallet.ts (1)
7-7: LGTM! Appropriate type narrowing to IReadonlyWallet.The
calcNextRolloverfunction only performs read operations (fetching VTXOs and UTXOs), so narrowing the parameter type fromIWallettoIReadonlyWalletis semantically correct and improves type safety by documenting the function's read-only contract.Also applies to: 21-21
src/screens/Settings/Vtxos.tsx (2)
88-96: LGTM! Correct use of reader for read operations.VTXOs and boarding UTXOs are fetched via
svcWallet.reader, which is appropriate since these are read-only operations that should work in both regular and readonly wallet modes.
101-108: Note: Inputs-to-settle calculation is unavailable in readonly mode.The guard
if (!svcWallet?.writer) returnmeans readonly wallets won't calculatehasInputsToSettle, so the renew button won't appear. This is consistent with the design since readonly wallets can't perform settlements anyway.src/screens/Apps/Lendasat/Index.tsx (4)
34-34: LGTM!Correctly uses
svcWallet.readerfor the read-onlygetReceivingAddressesoperation, aligning with the reader/writer separation pattern.
69-84: LGTM!The write operations (
sendBitcoin,collaborativeExit) correctly guard onsvcWallet?.writerand usesvcWallet.writerfor execution. Based on learnings,collaborativeExitis the one function that should acceptIWallet, which aligns with passingsvcWallet.writerhere.
105-111: LGTM!Public key retrieval correctly uses
svcWallet.reader.identity.compressedPublicKey()since this is a read-only operation that should be available even in readonly mode.
142-167: LGTM!Signing operations (
onSignPsbt,onSignMessage) correctly guard onsvcWallet?.writerand use the writer's identity for signing, which is appropriate since signing requires the private key and should be unavailable in readonly mode.src/lib/utxo.ts (2)
1-1: LGTM!The import change from
IWallettoIReadonlyWalletis appropriate since this module only performs read operations.
18-22: LGTM!Widening the parameter type to
IReadonlyWalletis correct sincegetConfirmedAndNotExpiredUtxosonly callsgetBoardingUtxos(), which is a read-only operation. This allows the function to work with both full and readonly wallets.src/screens/Settings/Backup.tsx (3)
38-53: LGTM!The new
pubkeystate and effect correctly synchronize withwallet.pubkey. The conditional checkif (wallet.pubkey)prevents setting empty/undefined values.
64-74: LGTM!Both copy handlers correctly guard against empty values before copying to clipboard. Renaming to
handleNsecCopyand addinghandlePubkeyCopyimproves clarity.
167-202: LGTM!Good implementation of readonly-aware backup UI:
- Private key section is hidden for readonly wallets (appropriate since they don't have access to it)
- Public key section is always visible with clear messaging about read-only restoration
- The public key is displayed directly (not masked), which is correct since public keys are safe to share
src/screens/Init/Connect.tsx (1)
16-18: LGTM!Correctly destructures
initReadonlyWalletfrom context andpublicKeyfrominitInfoto support the new readonly initialization path.src/providers/navigation.tsx (3)
40-40: LGTM!New
Pages.InitRestoreReadonlyenum member correctly added for the readonly wallet restoration flow.
79-79: LGTM!Correctly maps
InitRestoreReadonlytoTabs.None, consistent with other init pages.
102-123: LGTM!The signature change to accept a
ctxobject withreadonlyflag is well-designed for extensibility. The newInitRestoreReadonlycase correctly renders<InitRestore readonly />. All callers ofpageComponenthave been properly updated—the only call site atsrc/App.tsx:121correctly passes{ readonly: wallet.isReadonly }.src/screens/Apps/Index.tsx (2)
14-14: LGTM!Appropriate import of
WarningBoxcomponent for the readonly mode warning.
94-133: LGTM!Clean implementation of readonly mode handling:
- Props type with optional
readonlyprop maintains backward compatibility- Conditional rendering clearly separates the warning from the app list
- Warning message is clear and user-friendly
src/screens/Wallet/Unlock.tsx (3)
10-16: LGTM!Appropriate imports added for the new readonly wallet unlock flow, including storage utilities, byte conversion, and UI components.
24-24: Typo fixed.The previous review flagged the typo
'inital'→'initial'. This has been corrected.
54-78: LGTM!The stage-based rendering provides clear UI states:
'initial': Shows loading spinner during initialization'failed-privatekey': Shows password prompt for encrypted wallets'failed-publickey': Shows error with reset option for readonly walletsThis addresses the previous review concern about misleading UX when readonly initialization fails.
src/screens/Init/Restore.tsx (3)
23-24: LGTM on the Props type and component signature.Clean addition of the optional
readonlyprop to support the new readonly wallet restoration flow.
72-87: LGTM on the handleProceed branching logic.The readonly path correctly skips BackupProvider restoration (which requires signing capability) and navigates directly to InitSuccess. The comment on line 75 clarifies the intent.
94-116: LGTM on the readonly UI.The readonly restoration UI is well-structured with clear labeling and helpful explanatory text for users.
src/providers/wallet.tsx (5)
34-34: LGTM on the SvcWallet type design.Clean separation of reader (always required) and optional writer for readonly support. This pattern enables compile-time safety for write operations.
240-247: Status ping now correctly usesnewSvcWallet.The previous review concern about the stale state check has been addressed—the code now properly checks and accesses
newSvcWallet.reader.
249-252: LGTM on guarding renewCoins behind writer check.Write operations are correctly gated to only execute when a writer is available, preventing errors in readonly mode.
300-313: LGTM on initReadonlyWallet implementation.The readonly initialization path correctly:
- Encodes the public key and updates config
- Initializes with
publicKeyparameter (notprivateKey)- Sets
isReadonly: trueon the wallet
328-331: LGTM on settlePreconfirmed writer guard.Correctly throws an error if
writeris unavailable, preventing write operations in readonly mode.
|
We should export the pubkey in npub format (and not hex) as we export the privkey in nsec format. |
|
Should we say something on the success page when in read-only mode? 
|
|
@pietro909 we have a regression error somewhere: after receiving an onchain address or redeeming a note the tx is not present in the tx list. It just happens with your branch, master is ok. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/screens/Wallet/Index.tsx (1)
27-27: Remove unusedsvcWalletvariable.The
svcWalletvariable is destructured but never used in this component. Remove it to clean up the code.🔎 Proposed fix
- const { balance, txs, svcWallet, wallet } = useContext(WalletContext) + const { balance, txs, wallet } = useContext(WalletContext)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/App.tsxsrc/providers/navigation.tsxsrc/screens/Init/Restore.tsxsrc/screens/Settings/Backup.tsxsrc/screens/Wallet/Index.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Settings/Backup.tsxsrc/App.tsxsrc/screens/Init/Restore.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Settings/Backup.tsxsrc/App.tsxsrc/screens/Init/Restore.tsxsrc/screens/Wallet/Index.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/App.tsxsrc/screens/Wallet/Index.tsx
🧬 Code graph analysis (3)
src/screens/Settings/Backup.tsx (3)
src/lib/clipboard.ts (1)
copyToClipboard(3-11)src/lib/toast.ts (1)
copiedToClipboard(13-16)src/components/InputFake.tsx (1)
InputFake(3-20)
src/providers/navigation.tsx (1)
src/screens/Apps/Index.tsx (1)
Apps(95-138)
src/screens/Wallet/Index.tsx (1)
src/providers/wallet.tsx (1)
WalletContext(54-69)
🪛 GitHub Check: test
src/screens/Init/Restore.tsx
[warning] 19-19:
'TextSecondary' is defined but never used
[warning] 19-19:
'TextSecondary' is defined but never used
src/screens/Wallet/Index.tsx
[warning] 27-27:
'svcWallet' is assigned a value but never used
[warning] 27-27:
'svcWallet' is assigned a value but never used
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (6)
src/screens/Settings/Backup.tsx (1)
168-203: LGTM - Clean separation of private/public key UI.The conditional rendering correctly shows the private key section only for non-readonly wallets while always displaying the public key section. The messaging clearly communicates the readonly restoration capability.
src/screens/Wallet/Index.tsx (1)
30-30: LGTM - Correct readonly mode implementation.The Send button is appropriately hidden when the wallet is in readonly mode, maintaining a clean UX that aligns with readonly wallet constraints.
Also applies to: 55-55
src/providers/navigation.tsx (1)
100-113: LGTM - Clean readonly context propagation.The signature change enables readonly state to flow through the navigation system to child components. The implementation is minimal and focused.
src/App.tsx (1)
72-72: LGTM - Consistent readonly mode integration.The changes correctly:
- Prevent navigation interruption during publicKey-based initialization (line 72)
- Propagate readonly state to page components (line 121)
- Hide the Apps tab in readonly mode, maintaining UX consistency (lines 146-153)
Also applies to: 121-121, 146-153
src/screens/Init/Restore.tsx (2)
79-93: LGTM - Clean dual-path restoration logic.The function correctly handles both readonly (publicKey) and full (privateKey) restoration paths. The readonly path skips backup restoration and navigates directly to success, while the private key path preserves the existing backup restoration flow.
106-110: Clear readonly mode communication.The warning box effectively communicates the limitations of readonly mode to users before they proceed with restoration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/providers/wallet.tsx (2)
163-177: Guard condition!walletStates.vtxosis always falsy.Since
walletStates.vtxosis initialized as{ spendable: [], spent: [] }, the check will never short-circuit. Consider checking for meaningful content if that's the intent, or remove the redundant guard.Suggested fix
- if (!initialized || !walletStates.vtxos || !svcWallet) return + if (!initialized || !svcWallet) return
271-274: Consider logging when renewCoins is skipped for readonly wallets.Silent skip when
svcWallet?.writeris undefined makes sense, but a debug log could help troubleshoot if someone wonders why coins aren't being renewed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/providers/wallet.tsxsrc/screens/Init/Restore.tsxsrc/screens/Wallet/Index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/Init/Restore.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/providers/wallet.tsxsrc/screens/Wallet/Index.tsx
📚 Learning: 2025-06-30T18:23:34.163Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
Applied to files:
src/providers/wallet.tsx
📚 Learning: 2025-12-20T10:09:41.030Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 278
File: src/providers/annoucements.tsx:32-50
Timestamp: 2025-12-20T10:09:41.030Z
Learning: In src/providers/annoucements.tsx, the useEffect hook depends on [config] (not just [configLoaded]) because it needs to react to config changes after nostr restoration. The seen.current ref guard prevents showing multiple announcements per session, even though the effect may re-run on config updates.
Applied to files:
src/providers/wallet.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/providers/wallet.tsxsrc/screens/Wallet/Index.tsx
🧬 Code graph analysis (2)
src/providers/wallet.tsx (5)
src/lib/types.ts (3)
Wallet(99-107)Tx(71-81)Vtxo(97-97)src/lib/wallet.ts (1)
calcBatchLifetimeMs(37-48)src/lib/constants.ts (1)
maxPercentage(9-9)src/lib/asp.ts (3)
getTxHistory(145-179)getVtxos(181-192)getBalance(139-143)src/lib/explorers.ts (1)
getRestApiExplorerURL(34-36)
src/screens/Wallet/Index.tsx (2)
src/providers/wallet.tsx (1)
WalletContext(56-71)src/components/Button.tsx (1)
Button(21-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (6)
src/providers/wallet.tsx (4)
73-136: Well-structured initialization with exponential backoff retry.The
initSvcWorkerWalletfunction cleanly handles both writer (privateKey) and reader-only (publicKey) paths with appropriate retry logic for service worker activation timeouts.
253-282: Interval cleanup now properly implemented.The
useEffectcorrectly returns a cleanup function that clears both the polling interval and removes the message listener. The stale state concern from previous review is resolved since the effect re-runs whensvcWalletchanges, ensuring fresh closures.
303-316: Clean separation of init paths for read-write and read-only wallets.Both
initWalletandinitReadonlyWalletcorrectly setisReadonlybased on the initialization path and store the public key in config. The async flows properly awaitsetupSvcWorkerbefore updating wallet state.Also applies to: 318-331
346-350: Good guard for write-only operations.
settlePreconfirmedcorrectly throws when no writer is available, preventing silent failures in readonly mode.src/screens/Wallet/Index.tsx (2)
27-30: Clean implementation of readonly check.The
isReadonlyflag is correctly derived from the wallet context and the double-negation ensures a boolean value.
54-57: Send button correctly hidden in readonly mode while Receive remains visible.This aligns with the PR objective: readonly wallets can still view receiving addresses (derived from public key) but cannot initiate sends.
One small UX consideration: users in readonly mode might benefit from a visual indicator that they're viewing a readonly wallet (e.g., a badge or different styling). This was mentioned in the PR comments—worth confirming whether the success page or wallet screen should indicate readonly status.
pietro909
force-pushed
the
readonly-wallet
branch
from
ac43783 to
09e9e41
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/screens/Settings/Vtxos.tsx (1)
292-292: Success message rendered unconditionally in date view.The success
WarningBoxat line 292 is always visible in the date view, regardless of whether a renewal just succeeded. Compare with line 260 where it's correctly guarded by{success ? ... : null}.🔎 Apply this diff to restore the conditional rendering
- <WarningBox green text='Coins renewed successfully' /> + {success ? <WarningBox green text='Coins renewed successfully' /> : null}src/screens/Wallet/Unlock.tsx (1)
27-31: Validate public key format before conversion.The code calls
hexToBytes(walletFromStorage.pubkey)without validating the format. While the error is caught by the.catch()handler, it's more robust to validate the hex format before the conversion rather than relying on exception handling. This is inconsistent with the validation pattern used inRestore.tsxwhere the input is validated beforehexToBytesis called.Consider adding a format check:
const walletFromStorage = readWalletFromStorage() if (walletFromStorage?.isReadonly && walletFromStorage.pubkey) { - initReadonlyWallet(hexToBytes(walletFromStorage.pubkey)).catch((err) => { - consoleError(err, 'error initializing readonly wallet') - setStage('failed-publickey') - }) + if (!/^[0-9a-fA-F]*$/.test(walletFromStorage.pubkey)) { + consoleError('Invalid public key format in storage', 'invalid pubkey') + setStage('failed-publickey') + } else { + initReadonlyWallet(hexToBytes(walletFromStorage.pubkey)).catch((err) => { + consoleError(err, 'error initializing readonly wallet') + setStage('failed-publickey') + }) + }
🧹 Nitpick comments (2)
src/screens/Init/Connect.tsx (2)
21-24: Consider adding missing dependencies to the effect.The effect uses
reloadWalletandnavigatebut doesn't include them in the dependency array. While these are likely stable references from context, it's best practice to include all dependencies.🔎 Proposed fix
useEffect(() => { if (svcWallet && initialized) { reloadWallet().then(() => navigate(Pages.Wallet)) } - }, [svcWallet, initialized]) + }, [svcWallet, initialized, reloadWallet, navigate])
26-33: Consider removing debug console.log before production.The
console.logat line 28 appears to be debugging code and should be removed or replaced with a proper logging function.🔎 Proposed fix
useEffect(() => { if (publicKey) { - console.log('init readonly wallet with public key', publicKey) initReadonlyWallet(publicKey) .then(() => setInitInfo({ ...initInfo, publicKey: undefined })) .catch(consoleError) return }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/ReadonlyWallet.tsxsrc/providers/navigation.tsxsrc/providers/wallet.tsxsrc/screens/Init/Connect.tsxsrc/screens/Init/Success.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Index.tsxsrc/screens/Wallet/Unlock.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/providers/navigation.tsx
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Init/Success.tsxsrc/screens/Wallet/Unlock.tsxsrc/screens/Init/Connect.tsxsrc/screens/Wallet/Index.tsxsrc/providers/wallet.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Init/Success.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Unlock.tsxsrc/screens/Init/Connect.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/components/ReadonlyWallet.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Unlock.tsxsrc/screens/Init/Connect.tsxsrc/screens/Wallet/Index.tsxsrc/providers/wallet.tsx
📚 Learning: 2025-06-30T18:23:34.163Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
Applied to files:
src/screens/Settings/Vtxos.tsxsrc/providers/wallet.tsx
📚 Learning: 2025-12-20T10:09:41.030Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 278
File: src/providers/annoucements.tsx:32-50
Timestamp: 2025-12-20T10:09:41.030Z
Learning: In src/providers/annoucements.tsx, the useEffect hook depends on [config] (not just [configLoaded]) because it needs to react to config changes after nostr restoration. The seen.current ref guard prevents showing multiple announcements per session, even though the effect may re-run on config updates.
Applied to files:
src/providers/wallet.tsx
🧬 Code graph analysis (5)
src/components/ReadonlyWallet.tsx (2)
src/components/FlexCol.tsx (1)
FlexCol(16-46)src/components/Warning.tsx (1)
WarningBox(12-37)
src/screens/Settings/Vtxos.tsx (2)
src/lib/asp.ts (2)
getInputsToSettle(232-239)settleVtxos(241-268)src/components/ReadonlyWallet.tsx (1)
ReadonlyWallet(4-10)
src/screens/Init/Connect.tsx (3)
src/providers/wallet.tsx (1)
WalletContext(55-70)src/lib/logs.ts (1)
consoleError(40-44)src/lib/privateKey.ts (1)
setPrivateKey(46-54)
src/screens/Wallet/Index.tsx (3)
src/lib/types.ts (1)
Wallet(99-107)src/providers/wallet.tsx (1)
WalletContext(55-70)src/components/ReadonlyWallet.tsx (1)
ReadonlyWallet(4-10)
src/providers/wallet.tsx (3)
src/lib/types.ts (3)
Wallet(99-107)Tx(71-81)Vtxo(97-97)src/lib/wallet.ts (2)
calcBatchLifetimeMs(37-48)calcNextRollover(21-35)src/lib/asp.ts (3)
getTxHistory(145-179)renewCoins(270-273)settleVtxos(241-268)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (22)
src/screens/Init/Success.tsx (2)
17-17: LGTM: Readonly detection logic is correct.The condition correctly identifies a readonly wallet by checking for the presence of
publicKeyand absence ofprivateKey.
19-25: LGTM: Success message correctly handles readonly wallet state.The text prioritization appropriately distinguishes between readonly wallet creation, restoration with/without Nostr backup, and new wallet creation. This addresses the question raised in PR comments about explicitly stating readonly mode on the success page.
src/screens/Wallet/Index.tsx (3)
28-31: LGTM: Readonly state derivation is safe and correct.The code properly derives
isReadonlyfrom the wallet context with appropriate optional chaining.
55-55: LGTM: Readonly wallet banner correctly displayed.The warning banner is appropriately shown only when the wallet is in readonly mode.
57-58: LGTM: Send button correctly hidden in readonly mode.The Send button is appropriately hidden when the wallet is readonly, while the Receive button remains available. This aligns with the readonly wallet's purpose of viewing transaction history and receiving funds.
src/screens/Settings/Vtxos.tsx (5)
89-97: LGTM: Read operations correctly use reader.The code appropriately uses
svcWallet.readerfor read-only operations (getVtxos, getBoardingUtxos), which is available in both readonly and writable wallet modes.
102-103: LGTM: Writer guard correctly protects settlement-related operations.The guard properly checks for
svcWallet?.writerbefore fetching inputs to settle, which is appropriate since settlement is a write operation unavailable in readonly mode.
124-136: LGTM: Rollover handler correctly guards and uses writer.The function appropriately guards against missing writer and uses the non-null assertion operator after the guard check.
248-248: LGTM: Readonly wallet banner correctly displayed.The warning banner is appropriately shown only when the wallet is in readonly mode, consistent with the pattern used elsewhere in the application.
296-301: LGTM: Automatic renewal text correctly guarded for writable wallets.The automatic renewal message is appropriately shown only when both
svcWallet.writerandwallet.thresholdMsare available, which is correct since automatic renewal requires write capabilities.src/screens/Init/Connect.tsx (1)
34-39: LGTM: Private key initialization and cleanup is correct.The code properly clears sensitive data (password and privateKey) from initInfo after wallet initialization.
src/screens/Wallet/Unlock.tsx (3)
22-22: LGTM: Stage state properly typed.The three-stage flow (initial, failed-publickey, failed-privatekey) is well-defined and correctly typed.
45-47: LGTM: Reset handler correctly clears storage and reloads.The reset function appropriately clears storage and reloads the page to allow the user to start fresh after a readonly wallet initialization failure.
49-73: LGTM: Stage-based rendering is well-structured.The switch statement provides appropriate UI for each stage, with clear error handling and recovery options for both readonly and private key unlock failures.
src/providers/wallet.tsx (8)
35-70: LGTM: Reader/writer architecture is well-designed.The
SvcWallettype with mandatoryreaderand optionalwriteris a clean design that enables both readonly and writable wallet modes. The interface extension forinitReadonlyWalletappropriately exposes the new capability.
80-135: LGTM: Service worker initialization with robust retry logic.The
initSvcWorkerWallethelper is well-implemented with:
- Clear separation between readonly (public key) and writable (private key) initialization
- Exponential backoff retry logic for service worker activation timeouts
- Appropriate error handling and propagation
144-176: LGTM: Wallet state consolidation and threshold calculation.The refactor to consolidate wallet state (txs, vtxos, balance) into a single object improves code organization. The threshold calculation correctly uses
svcWallet.readerfor read operations.
220-234: LGTM: Reload wallet correctly uses reader for all operations.The
reloadWalletfunction appropriately usessvcWallet.readerfor all read operations and is properly memoized withuseCallback.
238-253: LGTM: Service worker message handling is correct.The message handler appropriately reacts to VTXO/UTXO updates and includes a delayed second reload to account for indexer cache updates.
256-287: LGTM: Service worker lifecycle management is correct.The effect properly:
- Reloads wallet state when svcWallet changes
- Sets up status polling with proper cleanup
- Triggers coin renewal for writable wallets
- Manages service worker message listeners with cleanup
The interval cleanup addresses the concern raised in past comments.
308-334: LGTM: Wallet initialization correctly handles both modes.Both
initWalletandinitReadonlyWalletare properly implemented, correctly setting theisReadonlyflag and calling the appropriate setup functions.
336-371: LGTM: Wallet operations correctly use reader/writer split.All methods appropriately:
- Use
readerfor read-only operations (lock, reset, status check)- Guard and use
writerfor write operations (settle preconfirmed)- Align with the readonly wallet architecture
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/e2e/init.test.ts (1)
33-33: Implement comprehensive restore wallet test.The TODO comment indicates that a full restore wallet test is needed. Given the PR's read-only wallet functionality and the regression mentioned in the comments (transactions not appearing after restore), comprehensive testing would be valuable.
Do you want me to help generate a complete end-to-end test that:
- Tests restoring a wallet with a public key
- Verifies read-only mode restrictions (no Send button, no Apps tab)
- Validates transaction history display
- Confirms the regression fix for transaction visibility
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/e2e/init.test.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/test/e2e/init.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/Init/Connect.tsx (1)
20-38: Add missing dependencies to the useEffect hook.The effect accesses several values that are not included in the dependency array:
initInfo,navigate,initReadonlyWallet,initWallet, andsetInitInfo. Per React hooks rules, all reactive values used inside an effect should be listed in its dependency array to prevent stale closures.🔎 Proposed fix
- }, []) + }, [initInfo, navigate, initReadonlyWallet, initWallet, setInitInfo, password, privateKey, publicKey])Alternatively, if the effect should only run once on mount by design, restructure it to avoid referencing external values directly, or add a comment explaining why the empty array is intentional despite the linter warnings.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/App.tsxsrc/screens/Init/Connect.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/App.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Init/Connect.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Init/Connect.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Init/Connect.tsx
🧬 Code graph analysis (1)
src/screens/Init/Connect.tsx (2)
src/providers/wallet.tsx (1)
WalletContext(55-70)src/lib/logs.ts (1)
consoleError(40-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
| if (publicKey) { | ||
| initReadonlyWallet(publicKey) | ||
| .then(() => { | ||
| setInitInfo({ ...initInfo, publicKey: undefined }) | ||
| navigate(Pages.Wallet) | ||
| }) | ||
| .catch(consoleError) | ||
| return | ||
| } |
There was a problem hiding this comment.
Improve error handling to provide user feedback.
When initReadonlyWallet fails, the error is only logged to the console via consoleError. The user remains on the loading screen with no indication that initialization failed. Consider navigating back to the previous screen, displaying an error message, or providing a retry option to improve the user experience.
Note: The existing password/privateKey flow (lines 31-37) has the same limitation and should also be addressed.
🔎 Example fix with navigation on error
.catch(consoleError)
+ .catch(err => {
+ consoleError(err)
+ navigate(Pages.Init)
+ })
returnThis approach navigates the user back to the Init screen on error. Alternatively, consider showing an error toast/message before navigating back.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (publicKey) { | |
| initReadonlyWallet(publicKey) | |
| .then(() => { | |
| setInitInfo({ ...initInfo, publicKey: undefined }) | |
| navigate(Pages.Wallet) | |
| }) | |
| .catch(consoleError) | |
| return | |
| } | |
| if (publicKey) { | |
| initReadonlyWallet(publicKey) | |
| .then(() => { | |
| setInitInfo({ ...initInfo, publicKey: undefined }) | |
| navigate(Pages.Wallet) | |
| }) | |
| .catch(err => { | |
| consoleError(err) | |
| navigate(Pages.Init) | |
| }) | |
| return | |
| } |
pietro909
force-pushed
the
readonly-wallet
branch
from
fd7f2db to
7803306
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/Settings/Vtxos.tsx (1)
288-293: Success message rendered unconditionally.The success
WarningBoxat line 292 is always visible in the "Next Renewal" view, regardless of whether a renewal just succeeded. Compare with line 260 where it's correctly guarded by{success ? ... : null}.🔎 Proposed fix
<Text>{prettyAgo(wallet.nextRollover)}</Text> </Box> - <WarningBox green text='Coins renewed successfully' /> + {success ? <WarningBox green text='Coins renewed successfully' /> : null} </FlexCol>
♻️ Duplicate comments (3)
src/screens/Init/Connect.tsx (1)
21-29: Error handling still missing for readonly initialization failure.When
initReadonlyWalletfails, the error is only logged viaconsoleError. The user remains on the loading screen with no indication that initialization failed and no way to recover. Consider navigating back to the Init screen or displaying an error message.🔎 Suggested fix
if (publicKey) { initReadonlyWallet(publicKey) .then(() => { setInitInfo({ ...initInfo, publicKey: undefined }) navigate(Pages.Wallet) }) - .catch(consoleError) + .catch((err) => { + consoleError(err) + navigate(Pages.Init) + }) return }src/screens/Init/Restore.tsx (1)
42-58: Add public key byte length validation after decoding.After converting the npub to bytes, the code doesn't validate that the resulting
Uint8Arrayis exactly 33 bytes (compressed public key format). This is inconsistent with how the private key path validates usinginvalidPrivateKey.🔎 Suggested validation
try { const decodedPubkey = nip19.decode(someKey) if (decodedPubkey.type !== 'npub') throw new Error('It must be a valid npub key') pubkey = hexToBytes(decodedPubkey.data) + if (pubkey.length !== 33) throw new Error('Invalid public key: must be exactly 33 bytes') setError('') setLabel(buttonLabel) } catch (e) {src/screens/Wallet/Unlock.tsx (1)
28-35: Add validation for pubkey hex format before conversion.If
walletFromStorage.pubkeyis malformed (not valid hex),hexToBytes()will throw an error that isn't caught, potentially crashing the component. Consider validating the hex format or wrapping the conversion in a try-catch.🔎 Suggested defensive fix
if (walletFromStorage?.isReadonly && walletFromStorage.pubkey) { + // Validate hex format before conversion + if (!/^[0-9a-fA-F]+$/.test(walletFromStorage.pubkey)) { + setStage('failed-publickey') + return + } initReadonlyWallet(hexToBytes(walletFromStorage.pubkey)) .then(() => navigate(Pages.Wallet)) .catch((err) => { consoleError(err, 'error initializing readonly wallet') setStage('failed-publickey') })
🧹 Nitpick comments (5)
src/screens/Apps/Lendasat/Index.tsx (1)
105-112: Guard check is inconsistent with property access.The guard at line 106 checks
!svcWallet, but the code accessessvcWallet.reader.identity. While this works becausereaderis always present whensvcWalletexists, consider aligning the guard with the pattern used elsewhere for clarity.🔎 Suggested consistency fix
onGetPublicKey: async () => { - if (!svcWallet) { + if (!svcWallet?.reader) { throw new Error('Wallet not initialized') } const pk = await svcWallet.reader.identity.compressedPublicKey() return bytesToHex(pk) },src/providers/wallet.tsx (4)
102-108: Consider validating public key format before conversion.The
publicKeyparameter is passed tohexToBytes()without validation. If an invalid hex string is provided, the error will be thrown from within the noble library rather than here, potentially making debugging harder for users.🔎 Suggested validation
if (publicKey) { + // Validate hex format + if (!/^[0-9a-fA-F]+$/.test(publicKey) || publicKey.length !== 66) { + throw new Error('Invalid public key format: expected 66-character hex string') + } const reader = await ServiceWorkerReadonlyWallet.setup({ ...config, identity: ReadonlySingleKey.fromPublicKey(hexToBytes(publicKey)), }) return { reader } }
258-261: Race condition workaround is fragile.The 250ms
setTimeoutworkaround (line 260) to handle a race condition where the UI updates before transactions are available is fragile and could still fail if the timing changes.Consider a more deterministic approach:
- Add a flag or promise that resolves when the service worker has confirmed transaction availability
- Use a retry mechanism with exponential backoff
- Update the service worker to send a confirmation message when data is ready
This might also be related to the regression mentioned in the PR comments about transactions not appearing after onchain receives.
296-296: Use consistent error logging.Line 296 uses
console.errordirectly, while the rest of the file uses theconsoleErrorhelper (e.g., lines 172, 229, 270, 298). UseconsoleErrorfor consistency with the codebase pattern.🔎 Suggested fix
} catch (err) { - console.error(err) + consoleError(err, 'Service worker setup failed') if (err instanceof Error && err.message.includes('Service worker activation timed out')) { consoleError(err, 'Service worker activation failed') return }
243-247: The double-reload pattern intentionally works around indexer caching delays.The code comment at line 244 explicitly documents this: "reload again after a delay to give the indexer time to update its cache." This pattern appears designed to address the regression where transactions don't appear immediately after onchain receives or note redemptions.
However, the 5-second delay is hardcoded without documented justification. If the indexer takes longer than 5 seconds to update its cache, transactions could still not appear, and the approach remains fragile.
Consider a more robust solution such as polling with exponential backoff until the expected transaction appears, or adding acknowledgment from the indexer once it confirms the update, rather than relying on a fixed timeout.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
src/App.tsxsrc/components/Keyboard.tsxsrc/components/ReadonlyWallet.tsxsrc/components/Refresher.tsxsrc/lib/asp.tssrc/lib/types.tssrc/lib/utxo.tssrc/lib/wallet.tssrc/providers/flow.tsxsrc/providers/lightning.tsxsrc/providers/wallet.tsxsrc/screens/Apps/Lendasat/Index.tsxsrc/screens/Apps/Lendaswap/Index.tsxsrc/screens/Init/Connect.tsxsrc/screens/Init/Restore.tsxsrc/screens/Init/Success.tsxsrc/screens/Settings/Backup.tsxsrc/screens/Settings/Support.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Index.tsxsrc/screens/Wallet/Notes/Redeem.tsxsrc/screens/Wallet/Receive/Amount.tsxsrc/screens/Wallet/Send/Details.tsxsrc/screens/Wallet/Send/Form.tsxsrc/screens/Wallet/Transaction.tsxsrc/screens/Wallet/Unlock.tsxsrc/test/e2e/init.test.tssrc/test/e2e/nostr.test.tssrc/test/screens/mocks.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/test/e2e/init.test.ts
- src/screens/Settings/Support.tsx
- src/screens/Wallet/Transaction.tsx
- src/providers/flow.tsx
- src/components/Refresher.tsx
- src/screens/Wallet/Index.tsx
- src/providers/lightning.tsx
- src/test/screens/mocks.ts
- src/screens/Wallet/Send/Form.tsx
- src/lib/wallet.ts
- src/App.tsx
- src/test/e2e/nostr.test.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/lib/types.tssrc/screens/Wallet/Send/Details.tsxsrc/components/Keyboard.tsxsrc/screens/Init/Connect.tsxsrc/screens/Wallet/Unlock.tsxsrc/lib/utxo.tssrc/screens/Settings/Backup.tsxsrc/components/ReadonlyWallet.tsxsrc/screens/Apps/Lendaswap/Index.tsxsrc/screens/Apps/Lendasat/Index.tsxsrc/lib/asp.tssrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Receive/Amount.tsxsrc/screens/Wallet/Notes/Redeem.tsxsrc/providers/wallet.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/components/Keyboard.tsxsrc/screens/Init/Connect.tsxsrc/screens/Wallet/Unlock.tsxsrc/screens/Settings/Backup.tsxsrc/screens/Init/Success.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Init/Restore.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/components/Keyboard.tsxsrc/screens/Init/Connect.tsxsrc/screens/Wallet/Unlock.tsxsrc/screens/Settings/Backup.tsxsrc/screens/Init/Success.tsxsrc/providers/wallet.tsxsrc/screens/Init/Restore.tsx
📚 Learning: 2025-07-07T14:41:35.991Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Applied to files:
src/screens/Settings/Backup.tsxsrc/screens/Apps/Lendasat/Index.tsx
📚 Learning: 2025-08-28T15:58:17.245Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 142
File: src/lib/bip21.ts:7-13
Timestamp: 2025-08-28T15:58:17.245Z
Learning: In src/lib/bip21.ts, the encodeBip21 function is used by the wallet which always has an arkAddress available to pass to the function, while decodeBip21 is used to parse received possible BIP21 URIs which may not contain arkAddress. This explains why arkAddress is required in encodeBip21 but optional in the Bip21Decoded interface.
Applied to files:
src/screens/Settings/Backup.tsx
📚 Learning: 2025-06-30T18:23:34.163Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
Applied to files:
src/screens/Settings/Backup.tsxsrc/components/ReadonlyWallet.tsxsrc/screens/Apps/Lendaswap/Index.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Receive/Amount.tsxsrc/providers/wallet.tsx
📚 Learning: 2025-12-20T10:09:41.030Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 278
File: src/providers/annoucements.tsx:32-50
Timestamp: 2025-12-20T10:09:41.030Z
Learning: In src/providers/annoucements.tsx, the useEffect hook depends on [config] (not just [configLoaded]) because it needs to react to config changes after nostr restoration. The seen.current ref guard prevents showing multiple announcements per session, even though the effect may re-run on config updates.
Applied to files:
src/providers/wallet.tsx
🧬 Code graph analysis (10)
src/screens/Wallet/Send/Details.tsx (1)
src/lib/asp.ts (2)
sendOffChain(224-226)collaborativeExitWithFees(80-123)
src/screens/Init/Connect.tsx (2)
src/providers/wallet.tsx (1)
WalletContext(55-70)src/lib/logs.ts (1)
consoleError(40-44)
src/screens/Wallet/Unlock.tsx (5)
src/providers/wallet.tsx (1)
WalletContext(55-70)src/providers/navigation.tsx (1)
NavigationContext(169-173)src/lib/constants.ts (1)
defaultPassword(7-7)src/lib/storage.ts (2)
readWalletFromStorage(35-37)clearStorage(4-8)src/lib/privateKey.ts (1)
getPrivateKey(40-44)
src/screens/Settings/Backup.tsx (4)
src/lib/clipboard.ts (1)
copyToClipboard(3-11)src/lib/toast.ts (1)
copiedToClipboard(13-16)src/components/Text.tsx (2)
Text(25-79)TextSecondary(91-97)src/components/InputFake.tsx (1)
InputFake(3-20)
src/components/ReadonlyWallet.tsx (2)
src/components/FlexCol.tsx (1)
FlexCol(16-46)src/components/Warning.tsx (1)
WarningBox(12-37)
src/screens/Apps/Lendaswap/Index.tsx (1)
src/lib/asp.ts (2)
getReceivingAddresses(194-200)collaborativeExit(44-78)
src/screens/Apps/Lendasat/Index.tsx (2)
src/lib/asp.ts (2)
getReceivingAddresses(194-200)collaborativeExit(44-78)src/lib/address.ts (2)
isArkAddress(13-20)isBTCAddress(22-26)
src/screens/Settings/Vtxos.tsx (2)
src/lib/asp.ts (2)
getInputsToSettle(232-239)settleVtxos(241-268)src/components/ReadonlyWallet.tsx (1)
ReadonlyWallet(4-10)
src/screens/Wallet/Receive/Amount.tsx (1)
src/lib/asp.ts (1)
getReceivingAddresses(194-200)
src/screens/Wallet/Notes/Redeem.tsx (1)
src/lib/asp.ts (1)
redeemNotes(202-222)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (18)
src/screens/Init/Success.tsx (1)
17-25: LGTM!The readonly wallet detection logic and conditional messaging are correctly implemented. The readonly flag is properly derived from the presence of a public key without a private key, and the success message appropriately reflects the wallet type.
src/components/ReadonlyWallet.tsx (1)
4-10: LGTM!Simple, focused presentational component that correctly displays the read-only wallet warning. The grammar has been corrected to "transaction history" (singular).
src/screens/Apps/Lendasat/Index.tsx (2)
69-84: LGTM!The send flow correctly requires
svcWallet.writerfor write operations, andcollaborativeExitis properly called with the writer instance. This aligns with the retrieved learning thatcollaborativeExitshould acceptIWallettypes.
142-167: LGTM!The signing operations (
onSignPsbt,onSignMessage) correctly guard withsvcWallet?.writerand use the writer's identity for signing. This properly restricts these operations in read-only mode.src/screens/Settings/Vtxos.tsx (2)
86-98: LGTM!Read operations (
getVtxos,getBoardingUtxos) correctly usesvcWallet.reader, enabling these operations in read-only mode while the component appropriately shows the coins list.
248-249: LGTM!The
ReadonlyWalletcomponent is correctly rendered whenwallet.isReadonlyis truthy, providing appropriate visual feedback to users in read-only mode.src/screens/Settings/Backup.tsx (3)
168-190: LGTM!The private key section is correctly hidden when in read-only mode (
!wallet.isReadonly), preventing access to private key operations that aren't available for read-only wallets.
191-203: LGTM!The public key section with npub format and copy functionality is correctly rendered for all wallet modes, enabling users to share their public key for read-only restoration.
50-54: The code at lines 50-54 is correct — no changes needed.The past review incorrectly stated that
nip19.npubEncode()expects aUint8Array. In reality,nip19.npubEncode()in nostr-tools 2.12.0 accepts a hex string (32 bytes / 64 hex characters) and returns the bech32 "npub..." encoded string. Passingwallet.pubkey(a hex string) directly tonip19.npubEncode()is the correct usage.src/screens/Wallet/Unlock.tsx (1)
54-78: LGTM!The stage-based rendering cleanly separates the different unlock states. The
failed-publickeystage correctly offers a reset option, which is appropriate since entering a password won't help recover a failed readonly wallet initialization.src/screens/Init/Restore.tsx (2)
78-93: LGTM!The dual-path proceed logic correctly handles both scenarios: public key restoration navigates directly to success (no backup needed for readonly), while private key restoration performs the full backup restoration flow. This aligns with the readonly wallet feature design.
106-110: LGTM!The updated input label clearly indicates both formats are accepted, and the
WarningBoxappropriately informs users about the read-only mode limitations when restoring with a public key.src/lib/asp.ts (4)
1-1: LGTM!The addition of
IReadonlyWalletimport enables proper typing for read-only wallet operations.
139-143: LGTM!Changing
getBalanceto acceptIReadonlyWalletis correct since it only performs read operations. This enables the function to work with both full and read-only wallets.
181-192: LGTM!The
getVtxossignature change toIReadonlyWalletis appropriate since it only reads VTXO data without performing any write operations.
44-78: LGTM!The
collaborativeExitfunction correctly maintains itsIWalletsignature since it performs write operations (wallet.settle). This aligns with the retrieved learning about maintaining proper wallet type signatures.src/providers/wallet.tsx (2)
144-148: Good consolidation of wallet state.The refactoring to consolidate
txs,vtxos, andbalanceinto a singlewalletStatesobject is a good improvement. This ensures atomic updates and prevents potential race conditions where these values could become out of sync.
372-392: Provider implementation correctly exposes read-only wallet API.The context provider properly exposes the new
initReadonlyWalletfunction and correctly derivestxs,balance, andvtxosfrom the consolidatedwalletStatesobject. The implementation maintains consistency with the interface definition.
pietro909
force-pushed
the
readonly-wallet
branch
from
7803306 to
f7fe97d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/screens/Init/Connect.tsx (1)
21-29: Improve error handling to provide user feedback.When
initReadonlyWalletfails, the error is only logged viaconsoleError. The user remains on the loading screen with no indication of failure. Consider navigating back, displaying an error message, or providing a retry option.🔎 Example fix
if (publicKey) { initReadonlyWallet(publicKey) .then(() => { setInitInfo({ ...initInfo, publicKey: undefined }) navigate(Pages.Wallet) }) - .catch(consoleError) + .catch(err => { + consoleError(err) + navigate(Pages.Init) + }) return }src/screens/Init/Restore.tsx (1)
42-58: Add public key length validation after decoding.After decoding the npub and converting to bytes (Line 47), validate that the resulting
Uint8Arrayis exactly 33 bytes (compressed public key format). This ensures consistency with the private key validation at Line 65.🔎 Proposed validation
if (someKey.startsWith('npub')) { let pubkey: Uint8Array | undefined = undefined try { const decodedPubkey = nip19.decode(someKey) if (decodedPubkey.type !== 'npub') throw new Error('It must be a valid npub key') pubkey = hexToBytes(decodedPubkey.data) + if (pubkey.length !== 33) throw new Error('Invalid public key: must be exactly 33 bytes') setError('') setLabel(buttonLabel) } catch (e) {
🧹 Nitpick comments (1)
src/screens/Apps/Index.tsx (1)
15-15: Remove unused import.The
WarningBoximport is not used anywhere in this file.🔎 Proposed fix
-import WarningBox from '../../components/Warning'
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
src/App.tsxsrc/components/Keyboard.tsxsrc/components/ReadonlyWallet.tsxsrc/components/Refresher.tsxsrc/lib/asp.tssrc/lib/types.tssrc/lib/utxo.tssrc/lib/wallet.tssrc/providers/flow.tsxsrc/providers/lightning.tsxsrc/providers/wallet.tsxsrc/screens/Apps/Index.tsxsrc/screens/Apps/Lendasat/Index.tsxsrc/screens/Apps/Lendaswap/Index.tsxsrc/screens/Init/Connect.tsxsrc/screens/Init/Restore.tsxsrc/screens/Init/Success.tsxsrc/screens/Settings/Backup.tsxsrc/screens/Settings/Support.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Index.tsxsrc/screens/Wallet/Notes/Redeem.tsxsrc/screens/Wallet/Receive/Amount.tsxsrc/screens/Wallet/Send/Details.tsxsrc/screens/Wallet/Send/Form.tsxsrc/screens/Wallet/Transaction.tsxsrc/screens/Wallet/Unlock.tsxsrc/test/e2e/init.test.tssrc/test/e2e/nostr.test.tssrc/test/screens/mocks.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- src/screens/Wallet/Send/Form.tsx
- src/test/e2e/init.test.ts
- src/lib/types.ts
- src/screens/Wallet/Notes/Redeem.tsx
- src/components/Keyboard.tsx
- src/screens/Settings/Vtxos.tsx
- src/screens/Wallet/Send/Details.tsx
- src/screens/Init/Success.tsx
- src/screens/Wallet/Index.tsx
- src/screens/Apps/Lendaswap/Index.tsx
- src/screens/Settings/Backup.tsx
- src/screens/Wallet/Receive/Amount.tsx
- src/lib/asp.ts
- src/providers/flow.tsx
- src/components/Refresher.tsx
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Wallet/Transaction.tsxsrc/App.tsxsrc/components/ReadonlyWallet.tsxsrc/providers/lightning.tsxsrc/screens/Apps/Lendasat/Index.tsxsrc/lib/wallet.tssrc/lib/utxo.tssrc/screens/Init/Connect.tsxsrc/screens/Settings/Support.tsxsrc/test/screens/mocks.tssrc/screens/Wallet/Unlock.tsxsrc/test/e2e/nostr.test.tssrc/providers/wallet.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/App.tsxsrc/screens/Init/Restore.tsxsrc/screens/Init/Connect.tsxsrc/screens/Settings/Support.tsxsrc/screens/Wallet/Unlock.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/App.tsxsrc/components/ReadonlyWallet.tsxsrc/screens/Init/Restore.tsxsrc/screens/Init/Connect.tsxsrc/screens/Wallet/Unlock.tsxsrc/providers/wallet.tsx
📚 Learning: 2025-07-07T14:41:35.991Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Applied to files:
src/providers/lightning.tsxsrc/screens/Apps/Lendasat/Index.tsx
📚 Learning: 2025-06-30T18:23:34.163Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
Applied to files:
src/providers/lightning.tsxsrc/providers/wallet.tsx
📚 Learning: 2025-12-20T10:09:41.030Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 278
File: src/providers/annoucements.tsx:32-50
Timestamp: 2025-12-20T10:09:41.030Z
Learning: In src/providers/annoucements.tsx, the useEffect hook depends on [config] (not just [configLoaded]) because it needs to react to config changes after nostr restoration. The seen.current ref guard prevents showing multiple announcements per session, even though the effect may re-run on config updates.
Applied to files:
src/providers/wallet.tsx
🧬 Code graph analysis (11)
src/screens/Wallet/Transaction.tsx (2)
src/lib/asp.ts (1)
getInputsToSettle(232-239)src/components/ReadonlyWallet.tsx (1)
ReadonlyWallet(4-10)
src/App.tsx (3)
src/components/Focusable.tsx (1)
Focusable(3-55)src/components/FlexCol.tsx (1)
FlexCol(16-46)src/icons/Apps.tsx (1)
AppsIcon(1-12)
src/components/ReadonlyWallet.tsx (2)
src/components/FlexCol.tsx (1)
FlexCol(16-46)src/components/Warning.tsx (1)
WarningBox(12-37)
src/providers/lightning.tsx (1)
src/lib/asp.ts (1)
sendOffChain(224-226)
src/screens/Apps/Lendasat/Index.tsx (2)
src/lib/asp.ts (2)
getReceivingAddresses(194-200)collaborativeExit(44-78)src/lib/address.ts (2)
isArkAddress(13-20)isBTCAddress(22-26)
src/lib/wallet.ts (2)
src/lib/types.ts (1)
Vtxo(100-100)src/providers/asp.tsx (1)
AspInfo(6-6)
src/screens/Init/Restore.tsx (4)
src/lib/logs.ts (1)
consoleError(40-44)src/lib/error.ts (1)
extractError(1-10)src/lib/constants.ts (1)
defaultPassword(7-7)src/lib/backup.ts (1)
BackupProvider(17-168)
src/screens/Init/Connect.tsx (2)
src/providers/wallet.tsx (1)
WalletContext(55-70)src/lib/logs.ts (1)
consoleError(40-44)
src/screens/Settings/Support.tsx (1)
src/lib/asp.ts (1)
getReceivingAddresses(194-200)
src/screens/Wallet/Unlock.tsx (5)
src/providers/navigation.tsx (1)
NavigationContext(169-173)src/lib/constants.ts (1)
defaultPassword(7-7)src/lib/storage.ts (2)
readWalletFromStorage(35-37)clearStorage(4-8)src/lib/logs.ts (1)
consoleError(40-44)src/lib/privateKey.ts (1)
getPrivateKey(40-44)
src/providers/wallet.tsx (7)
src/lib/types.ts (3)
Wallet(102-110)Tx(74-84)Vtxo(100-100)src/lib/logs.ts (1)
consoleError(40-44)src/providers/navigation.tsx (1)
NavigationContext(169-173)src/providers/flow.tsx (1)
FlowContext(92-107)src/lib/asp.ts (5)
getTxHistory(145-179)getVtxos(181-192)getBalance(139-143)renewCoins(270-273)settleVtxos(241-268)src/lib/explorers.ts (1)
getRestApiExplorerURL(34-36)src/lib/storage.ts (1)
clearStorage(4-8)
🪛 GitHub Actions: CI
src/App.tsx
[error] 1-1: Prettier formatting check failed for file src/App.tsx. Run 'prettier --write' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (19)
src/test/screens/mocks.ts (1)
84-84: LGTM!The mock for
initReadonlyWalletis consistent with the existing mock pattern and properly supports testing the readonly wallet initialization flow.src/test/e2e/nostr.test.ts (1)
86-86: LGTM!Adding
{ exact: true }to the 'Restore wallet' selector improves test precision by preventing false matches with similar text patterns, which is especially important with the new readonly wallet restoration flow.Also applies to: 209-209
src/components/ReadonlyWallet.tsx (1)
4-10: LGTM!The component correctly renders a warning message for readonly wallets and uses the proper grammatical form "transaction history" (singular).
src/lib/wallet.ts (1)
7-7: LGTM!The type change from
IWallettoIReadonlyWalletis semantically correct sincecalcNextRolloveronly performs read operations (getBoardingUtxos) without mutating wallet state.Also applies to: 21-21
src/lib/utxo.ts (1)
1-1: LGTM!The type change from
IWallettoIReadonlyWalletis semantically correct sincegetConfirmedAndNotExpiredUtxosonly performs read operations (getBoardingUtxos) without mutating wallet state.Also applies to: 18-22
src/providers/wallet.tsx (6)
35-35: LGTM!The
SvcWallettype with optionalwriteris a clean design that clearly separates read-only capabilities (always available viareader) from write capabilities (available only whenwriteris present).
220-232: LGTM!The
reloadWalletcallback correctly usessvcWallet.readerfor all read operations (getTxHistory,getVtxos,getBalance), which is appropriate for refreshing wallet state without requiring write permissions.
254-285: LGTM!The
svcWalleteffect properly:
- Polls wallet status via
svcWallet.reader.getStatus()- Conditionally calls
renewCoinsonly whensvcWallet.writeris available (Line 274)- Cleans up the polling interval and event listener on unmount
The pattern correctly enforces that write operations require the writer.
306-332: LGTM!Both initialization functions correctly:
initWalletcreates a full wallet with private key and setsisReadonly: falseinitReadonlyWalletcreates a readonly wallet with public key and setsisReadonly: trueThe separation is clear and consistent with the overall design.
334-369: LGTM!The wallet lifecycle methods correctly enforce the reader/writer pattern:
lockWalletandresetWalletusereader.clear()(read-side cleanup)settlePreconfirmedchecks forsvcWallet?.writerand useswriter(write operation)isLockedusesreader.getStatus()(read operation)
72-135: LGTMThe
initSvcWorkerWallethelper is well-designed with clean separation between private key (full wallet) and public key (readonly wallet) initialization, exponential backoff retry logic for service worker activation timeouts, and proper error handling.The SDK API usage is correct:
ReadonlySingleKey.fromPublicKey()accepts aUint8Arrayparameter, andhexToBytes()from @noble/hashes/utils returns exactly that type. The implementation properly converts the hex string publicKey to bytes before passing it to the SDK method.src/screens/Settings/Support.tsx (1)
34-34: LGTM!The change to
svcWallet.readeris consistent with the updatedgetReceivingAddressesAPI signature that expectsIReadonlyWallet, and correctly uses the reader for this read-only operation.src/screens/Apps/Lendasat/Index.tsx (3)
34-34: LGTM!Read operations correctly use
svcWallet.reader:
- Line 34:
getReceivingAddresses(svcWallet.reader)for fetching addresses- Line 110:
svcWallet.reader.identity.compressedPublicKey()for getting public keyAlso applies to: 110-111
69-104: LGTM!The
onSendToAddresshandler correctly enforces write permissions:
- Guards with
if (!svcWallet?.writer)to prevent operations in readonly mode- Uses
svcWallet.writer.sendBitcoinandcollaborativeExit(svcWallet.writer, ...)for write operations
142-168: LGTM!The signing operations correctly enforce write permissions:
- Both
onSignPsbt(Line 143) andonSignMessage(Line 154) guard withif (!svcWallet?.writer)- Use
svcWallet.writer.identity.sign()andsvcWallet.writer.identity.signMessage()for cryptographic operationsThis properly prevents signing in readonly mode.
src/screens/Wallet/Transaction.tsx (1)
26-26: LGTM!The changes correctly adapt the transaction screen for readonly wallet support by using
svcWallet.writerfor write operations and conditionally displaying the readonly warning.Also applies to: 70-77, 129-129
src/App.tsx (1)
83-83: LGTM!Adding
initInfo.publicKeyto the bailout condition correctly prevents navigation during readonly wallet setup.src/providers/lightning.tsx (1)
78-78: LGTM!Consistently using
svcWallet.writerfor wallet operations aligns with the readonly wallet architecture and ensures mutating operations require a writable wallet instance.Also applies to: 91-91, 164-164, 172-172
src/screens/Init/Restore.tsx (1)
79-93: LGTM!The updated flow correctly handles both readonly (publicKey) and full (privateKey) restoration paths, with appropriate navigation and backup restoration for each case.
| if (walletFromStorage?.isReadonly && walletFromStorage.pubkey) { | ||
| initReadonlyWallet(hexToBytes(walletFromStorage.pubkey)) | ||
| .then(() => navigate(Pages.Wallet)) | ||
| .catch((err) => { | ||
| consoleError(err, 'error initializing readonly wallet') | ||
| setStage('failed-publickey') | ||
| }) |
There was a problem hiding this comment.
Validate public key format before conversion.
Line 30 calls hexToBytes(walletFromStorage.pubkey) without validating that pubkey is valid hex. If the stored pubkey is malformed, hexToBytes will throw an uncaught error.
🔎 Proposed fix
const walletFromStorage = readWalletFromStorage()
if (walletFromStorage?.isReadonly && walletFromStorage.pubkey) {
+ // Validate pubkey is valid hex (64 characters, hex only)
+ if (!/^[0-9a-fA-F]{64}$/.test(walletFromStorage.pubkey)) {
+ consoleError('Invalid public key format in storage')
+ setStage('failed-publickey')
+ return
+ }
initReadonlyWallet(hexToBytes(walletFromStorage.pubkey))
.then(() => navigate(Pages.Wallet))
.catch((err) => {
consoleError(err, 'error initializing readonly wallet')
setStage('failed-publickey')
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (walletFromStorage?.isReadonly && walletFromStorage.pubkey) { | |
| initReadonlyWallet(hexToBytes(walletFromStorage.pubkey)) | |
| .then(() => navigate(Pages.Wallet)) | |
| .catch((err) => { | |
| consoleError(err, 'error initializing readonly wallet') | |
| setStage('failed-publickey') | |
| }) | |
| if (walletFromStorage?.isReadonly && walletFromStorage.pubkey) { | |
| // Validate pubkey is valid hex (64 characters, hex only) | |
| if (!/^[0-9a-fA-F]{64}$/.test(walletFromStorage.pubkey)) { | |
| consoleError('Invalid public key format in storage') | |
| setStage('failed-publickey') | |
| return | |
| } | |
| initReadonlyWallet(hexToBytes(walletFromStorage.pubkey)) | |
| .then(() => navigate(Pages.Wallet)) | |
| .catch((err) => { | |
| consoleError(err, 'error initializing readonly wallet') | |
| setStage('failed-publickey') | |
| }) |
🤖 Prompt for AI Agents
In src/screens/Wallet/Unlock.tsx around lines 29 to 35, the code calls
hexToBytes(walletFromStorage.pubkey) without validating that pubkey is
well-formed hex which can throw; before converting, validate that
walletFromStorage.pubkey is a non-empty string containing only valid hex
characters (and expected byte-length when decoded), and if invalid handle it the
same as other failures: log the error (consoleError) and
setStage('failed-publickey'); alternatively wrap hexToBytes(...) in a try/catch
to catch conversion errors, log the caught error and call
setStage('failed-publickey') instead of letting the exception propagate, then
only call initReadonlyWallet with the converted bytes when validation/conversion
succeeds.
|
@pietro909 feedback Change passwordBug: settings / advanced / change password is broken in read-only mode 
Redeem noteBug: settings / note freezes in loading state in read-only-mode 
Coin controlBug: the success message is always present (with nsec and npub) 
|
pietro909
force-pushed
the
readonly-wallet
branch
from
147ba5e to
c374887
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/screens/Apps/Lendasat/Index.tsx (2)
68-103: Improve error for readonly vs uninitialized wallet.
if (!svcWallet?.writer) throw Error('Wallet not initialized')conflates “readonly/locked (no writer)” with “not initialized”. Consider a clearer message whensvcWalletexists butwriteris absent.
30-39: RevertgetReceivingAddressessignature back toIWalletper design guidelines.The function signature has been widened to
IReadonlyWallet(line 194 insrc/lib/asp.ts), but it should remainIWallet-only. OnlycollaborativeExitshould accept flexible wallet types. ChangegetReceivingAddresses(wallet: IReadonlyWallet)back togetReceivingAddresses(wallet: IWallet), then adjust call sites in UI components (Lendasat, Settings, Lendaswap, Send, Receive) to passsvcWalletinstead ofsvcWallet.reader, or provide a compatible wrapper.src/screens/Settings/Vtxos.tsx (2)
100-110: Add error handling forgetInputsToSettleand include missing deps.This effect has no
.catch(...), so failures can become unhandled rejections and leave state inconsistent. Also it readswallet.thresholdMswithout listing it in deps.Proposed patch
useEffect(() => { if (!aspInfo || !svcWallet?.writer) return - getInputsToSettle(svcWallet.writer, wallet.thresholdMs).then(({ boardingUtxos, inputs, vtxos }) => { - setHasBoardingUtxosToSettle(boardingUtxos.length > 0) - setHasInputsToSettle(inputs.length > 0) - setHasVtxosToSettle(vtxos.length > 0) - const amount = inputs.reduce((a, v) => a + v.value, 0) || 0 - setAboveDust(amount > aspInfo.dust) - }) - }, [allUtxos, allVtxos, aspInfo, svcWallet]) + getInputsToSettle(svcWallet.writer, wallet.thresholdMs) + .then(({ boardingUtxos, inputs, vtxos }) => { + setHasBoardingUtxosToSettle(boardingUtxos.length > 0) + setHasInputsToSettle(inputs.length > 0) + setHasVtxosToSettle(vtxos.length > 0) + const amount = inputs.reduce((a, v) => a + v.value, 0) || 0 + setAboveDust(amount > aspInfo.dust) + }) + .catch((err) => { + setError(extractError(err)) + }) + }, [allUtxos, allVtxos, aspInfo, svcWallet, wallet.thresholdMs])
124-136:handleRolloverguard is inverted (breaks renewal for normal wallets).
if (!wallet.isReadonly) returnprevents rollover in the non-readonly case (the only case where writer signing should exist). This looks like a regression. Also,svcWallet.writer!should be guarded instead of asserted.Proposed patch
const handleRollover = async () => { - if (!wallet.isReadonly) return + if (wallet.isReadonly) return + if (!svcWallet.writer) { + setError('Wallet not initialized') + return + } try { setRollingover(true) - await settleVtxos(svcWallet.writer!, aspInfo.dust, wallet.thresholdMs) + await settleVtxos(svcWallet.writer, aspInfo.dust, wallet.thresholdMs) await reloadWallet() setRollingover(false) setSuccess(true) } catch (err) { setError(extractError(err)) setRollingover(false) } }src/screens/Init/Connect.tsx (1)
30-42: Init effect uses[]but readspublicKey/password/privateKey.If
initInfocan change after mount (even briefly), this effect won’t run. If it’s guaranteed immutable on this screen, consider adding a short comment disabling exhaustive-deps to document that assumption.
🤖 Fix all issues with AI agents
In @src/screens/Apps/Lendasat/Index.tsx:
- Around line 141-151: The onSignPsbt handler currently decodes the incoming
PSBT with hexToBytes and returns bytesToHex, but the Lendasat bridge uses
base64; update onSignPsbt to decode the incoming psbt string from base64
(replace hexToBytes(psbt) with a base64-to-bytes conversion) pass those bytes
into Transaction.fromPSBT, sign via svcWallet.writer.identity.sign(tx) as
before, then get the signed PSBT bytes (from toPSBT()) and encode the result to
base64 (replace bytesToHex(signedTxBytes) with bytes-to-base64). Ensure you use
or add a base64 <-> Uint8Array helper (or browser atob/Uint8Array equivalent) so
Transaction.fromPSBT and toPSBT continue to operate on byte arrays.
In @src/screens/Init/Connect.tsx:
- Around line 24-28: The effect that calls reloadWallet() should not perform
navigation because it can race with swap restoration; in the useEffect watching
svcWallet and initialized (which currently calls reloadWallet().then(() =>
navigate(Pages.Wallet))), remove the navigate(Pages.Wallet) call and instead let
navigation occur after restoreSwaps completes (or via a single coordinating
place that waits for both reloadWallet and restoreSwaps). Concretely, update the
effect containing reloadWallet to only call reloadWallet(), and move navigation
logic to the code path that runs when restoreSwaps finishes (or add a
combined-ready flag and navigate once that flag becomes true), referencing
reloadWallet, svcWallet, initialized, restoreSwaps, navigate and Pages.Wallet to
locate the spots to change.
In @src/screens/Init/Restore.tsx:
- Around line 106-115: Update the helper text under the Input named
'private-key' (label 'Public (npub) or Private key (nsec)') to mention that an
npub is safe to share and will restore the wallet in read-only mode; keep the
existing warning that nsec/private keys must be kept secret and reference the
readonly behavior already shown by the WarningBox when publicKey is present so
the copy is consistent with the label and ErrorMessage.
In @src/screens/Settings/Advanced.tsx:
- Around line 10-12: Rename the misspelled variable excludeInReaonlyMode to
excludeInReadonlyMode wherever it appears (e.g., in
src/screens/Settings/Advanced.tsx and src/screens/Settings/Menu.tsx): update the
declaration that uses useContext(WalletContext) and optionsUsingPrivKey, and
update every reference (such as the filter call building rows) to use the new
name so the code compiles and is consistent.
In @src/screens/Settings/Menu.tsx:
- Around line 13-22: Rename the misspelled variable excludeInReaonlyMode to
excludeInReadonlyMode and update all its usages in this file (and similarly fix
the same typo in src/screens/Settings/Advanced.tsx where the variable is
declared on line 11); specifically change the declaration const
excludeInReaonlyMode = wallet.isReadonly ? optionsUsingPrivKey : new Set() to
const excludeInReadonlyMode = wallet.isReadonly ? optionsUsingPrivKey : new
Set() and update calls like excludeInReaonlyMode.has(...) to
excludeInReadonlyMode.has(...).
In @src/screens/Settings/Password.tsx:
- Around line 83-84: The early return using wallet.isReadonly that renders only
ReadonlyWallet traps users; remove that early return and instead render
<ReadonlyWallet /> inside the existing screen layout so Header (with
back/navigation), Content and ButtonsOnBottom remain present. Locate the
conditional on wallet.isReadonly and replace the return with conditional JSX
(e.g., render <ReadonlyWallet /> within the screen's Content area and disable or
hide change-password-specific controls as needed) so users see the readonly
warning but still have the header/back navigation and bottom buttons.
In @src/screens/Settings/Vtxos.tsx:
- Line 292: The success banner is rendered unconditionally for non-readonly
wallets because the JSX uses only wallet.isReadonly; change the render condition
for the WarningBox in the Settings/Vtxos component to also check the success
state (e.g., render only when success is truthy and !wallet.isReadonly). Locate
the WarningBox usage in Vtxos.tsx and update its conditional to combine success
and wallet.isReadonly (e.g., success && !wallet.isReadonly) so the banner
appears only after a successful renewal.
In @src/screens/Wallet/Notes/Redeem.tsx:
- Line 44: The early return using "if (!svcWallet?.writer) return
<ReadonlyWallet />" removes the surrounding screen chrome and navigation;
instead, keep the screen layout and render ReadonlyWallet only inside the
Content area while disabling/hiding redeem actions and preventing redeem
handlers from running when svcWallet?.writer is falsy. Update the Redeem
component to remove the early return, place <ReadonlyWallet /> into the existing
Content/Body, and gate the redeem button handlers and ButtonsOnBottom (e.g.,
disable prop or conditional rendering) using svcWallet?.writer so the
back/header remains available but redeem functionality is blocked.
🧹 Nitpick comments (9)
src/screens/Wallet/Notes/Redeem.tsx (1)
50-50: Non-null assertion is safe but consider removing it.The
!assertion onsvcWallet.writeris protected by the guard on line 44. However, once the UX issue on line 44 is fixed, you can remove this assertion by adjusting the guard logic.src/providers/lightning.tsx (1)
165-184: Consider more specific error messaging for read-only mode.The error message "Lightning not initialized" is generic. Since read-only wallets cannot use Lightning features, consider providing a more specific error message when
svcWallet?.writeris unavailable due to read-only mode. However, this may be a low priority since the UI should prevent users from reaching this code path in read-only mode.💡 Suggested improvement
const payInvoice = async (pendingSwap: PendingSubmarineSwap): Promise<{ txid: string; preimage: string }> => { - if (!arkadeLightning || !svcWallet?.writer) throw new Error('Lightning not initialized') + if (!arkadeLightning) throw new Error('Lightning not initialized') + if (!svcWallet?.writer) throw new Error('Lightning payments are not available in read-only mode') if (!pendingSwap) throw new Error('No pending swap found')src/screens/Apps/Lendasat/Index.tsx (1)
48-67: Capabilities should reflect writer availability (readonly/locked safety).
capabilities()currently advertisessendBitcoin/signPsbtandark.canSendastrueeven ifsvcWallet.writeris missing; that can drive the iframe into actions that will always throw. Consider deriving these from!!svcWallet?.writer.Proposed patch
capabilities: () => { + const hasWriter = !!svcWallet?.writer return { bitcoin: { - signPsbt: true, - sendBitcoin: true, + signPsbt: hasWriter, + sendBitcoin: hasWriter, }, loanAssets: { supportedAssets: [], canReceive: false, canSend: false, }, nostr: { hasNpub: false, }, ark: { - canSend: true, + canSend: hasWriter, canReceive: true, }, } },Also applies to: 68-103
src/screens/Settings/Vtxos.tsx (1)
319-337: Bottom action gating should also consider writer availability.Even in non-readonly mode,
svcWallet.writermay be absent (locked/failed init). Consider adding&& !!svcWallet.writerto the gating to avoid showing a button that can’t work.src/screens/Wallet/Unlock.tsx (1)
50-52: Reset flow is reasonable; consider handling clearStorage failure.src/providers/wallet.tsx (4)
80-135: Consider validating public key format before initialization.The
initSvcWorkerWalletfunction has good retry logic with exponential backoff. However, whenpublicKeyis provided (line 105), it's passed directly tohexToByteswithout validation. Invalid hex strings could cause cryptic errors.🛡️ Add validation for public key format
Add validation before line 105:
if (publicKey) { + if (!/^[0-9a-fA-F]+$/.test(publicKey) || publicKey.length !== 66) { + throw new Error('Invalid public key format: expected 33-byte hex string') + } const reader = await ServiceWorkerReadonlyWallet.setup({ ...config, identity: ReadonlySingleKey.fromPublicKey(hexToBytes(publicKey)), }) return { reader } }
254-285: Effect dependency chain may cause excessive re-renders.The effect on line 254 depends on
handleServiceWorkerMessages(line 285), which itself depends on[svcWallet, reloadWallet](line 250). SincereloadWalletdepends on[svcWallet](line 232), this creates a dependency chain that could cause the effect to re-run more often than necessary.Consider extracting the message handling logic to avoid the nested dependency, or use
useRefto store the latestreloadWalletwithout triggering effect re-runs.♻️ Optimize effect dependencies
+ const reloadWalletRef = useRef(reloadWallet) + useEffect(() => { + reloadWalletRef.current = reloadWallet + }, [reloadWallet]) + const handleServiceWorkerMessages = useCallback( (event: MessageEvent) => { if (event.data && ['VTXO_UPDATE', 'UTXO_UPDATE'].includes(event.data.type)) { if (!svcWallet) { consoleError('Cannot handle a message when svcWallet is not defined') return } - reloadWallet() + reloadWalletRef.current() setTimeout(() => { - reloadWallet() + reloadWalletRef.current() }, 5000) } }, - [svcWallet, reloadWallet], + [svcWallet], )
258-261: Race condition delay may need adjustment.Line 260 introduces a 250ms delay "because there is a race condition where we may update the UI before some transactions are available." This hardcoded delay is brittle and may not be sufficient for slower networks or under heavy load.
Consider implementing a retry mechanism or checking transaction availability before updating the UI.
306-332: Consider initializing wallet state instead of spreading existing properties.Both
initWallet(line 317) andinitReadonlyWallet(line 331) spread the existingwalletobject when callingupdateWallet. This preserves properties from a previous wallet session (likethresholdMs,nextRollover, etc.) which may not be appropriate for a fresh initialization.Consider explicitly setting all wallet properties rather than spreading:
♻️ Initialize wallet with explicit properties
const initWallet = async (privateKey: Uint8Array) => { const arkServerUrl = aspInfo.url const network = aspInfo.network as NetworkName const esploraUrl = getRestApiExplorerURL(network) ?? '' const pubkey = hex.encode(secp.getPublicKey(privateKey)) updateConfig({ ...config, pubkey }) await setupSvcWorker({ privateKey: hex.encode(privateKey), arkServerUrl, esploraUrl, }) - updateWallet({ ...wallet, network, pubkey, isReadonly: false }) + updateWallet({ network, pubkey, isReadonly: false, nextRollover: 0 }) } const initReadonlyWallet = async (publicKey: Uint8Array) => { const pubkey = hex.encode(publicKey) const arkServerUrl = aspInfo.url const network = aspInfo.network as NetworkName const esploraUrl = getRestApiExplorerURL(network) ?? '' updateConfig({ ...config, pubkey }) await setupSvcWorker({ publicKey: pubkey, arkServerUrl, esploraUrl, }) - updateWallet({ ...wallet, network, pubkey, isReadonly: true }) + updateWallet({ network, pubkey, isReadonly: true, nextRollover: 0 }) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
src/App.tsxsrc/components/Keyboard.tsxsrc/components/ReadonlyWallet.tsxsrc/components/Refresher.tsxsrc/lib/asp.tssrc/lib/types.tssrc/lib/utxo.tssrc/lib/wallet.tssrc/providers/flow.tsxsrc/providers/lightning.tsxsrc/providers/options.tsxsrc/providers/wallet.tsxsrc/screens/Apps/Lendasat/Index.tsxsrc/screens/Apps/Lendaswap/Index.tsxsrc/screens/Init/Connect.tsxsrc/screens/Init/Restore.tsxsrc/screens/Init/Success.tsxsrc/screens/Settings/Advanced.tsxsrc/screens/Settings/Backup.tsxsrc/screens/Settings/Menu.tsxsrc/screens/Settings/Password.tsxsrc/screens/Settings/Support.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Index.tsxsrc/screens/Wallet/Notes/Redeem.tsxsrc/screens/Wallet/Receive/Amount.tsxsrc/screens/Wallet/Send/Details.tsxsrc/screens/Wallet/Send/Form.tsxsrc/screens/Wallet/Transaction.tsxsrc/screens/Wallet/Unlock.tsxsrc/test/e2e/init.test.tssrc/test/screens/mocks.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- src/providers/flow.tsx
- src/screens/Settings/Support.tsx
- src/test/e2e/init.test.ts
- src/test/screens/mocks.ts
- src/components/Keyboard.tsx
- src/lib/types.ts
- src/lib/utxo.ts
- src/components/ReadonlyWallet.tsx
- src/screens/Wallet/Send/Form.tsx
- src/screens/Apps/Lendaswap/Index.tsx
- src/screens/Wallet/Transaction.tsx
- src/screens/Settings/Backup.tsx
- src/screens/Wallet/Index.tsx
- src/screens/Wallet/Receive/Amount.tsx
- src/components/Refresher.tsx
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Init/Success.tsxsrc/screens/Settings/Menu.tsxsrc/screens/Init/Connect.tsxsrc/screens/Settings/Password.tsxsrc/screens/Wallet/Unlock.tsxsrc/screens/Settings/Advanced.tsxsrc/providers/wallet.tsxsrc/screens/Init/Restore.tsxsrc/App.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Init/Success.tsxsrc/screens/Settings/Menu.tsxsrc/screens/Init/Connect.tsxsrc/screens/Settings/Password.tsxsrc/screens/Settings/Vtxos.tsxsrc/providers/options.tsxsrc/screens/Wallet/Unlock.tsxsrc/screens/Settings/Advanced.tsxsrc/screens/Init/Restore.tsxsrc/App.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/providers/lightning.tsxsrc/screens/Settings/Menu.tsxsrc/screens/Init/Connect.tsxsrc/screens/Settings/Password.tsxsrc/screens/Apps/Lendasat/Index.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Unlock.tsxsrc/screens/Settings/Advanced.tsxsrc/screens/Wallet/Send/Details.tsxsrc/lib/asp.tssrc/providers/wallet.tsxsrc/screens/Wallet/Notes/Redeem.tsxsrc/App.tsxsrc/lib/wallet.ts
📚 Learning: 2025-07-07T14:41:35.991Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Applied to files:
src/providers/lightning.tsx
📚 Learning: 2025-06-30T18:23:34.163Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
Applied to files:
src/providers/lightning.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Send/Details.tsxsrc/providers/wallet.tsxsrc/App.tsx
📚 Learning: 2025-12-20T10:09:41.030Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 278
File: src/providers/annoucements.tsx:32-50
Timestamp: 2025-12-20T10:09:41.030Z
Learning: In src/providers/annoucements.tsx, the useEffect hook depends on [config] (not just [configLoaded]) because it needs to react to config changes after nostr restoration. The seen.current ref guard prevents showing multiple announcements per session, even though the effect may re-run on config updates.
Applied to files:
src/providers/wallet.tsx
🧬 Code graph analysis (11)
src/providers/lightning.tsx (1)
src/lib/asp.ts (1)
sendOffChain(224-226)
src/screens/Settings/Menu.tsx (3)
src/providers/wallet.tsx (1)
WalletContext(55-70)src/providers/options.tsx (2)
optionsUsingPrivKey(105-105)options(22-103)src/components/ReadonlyWallet.tsx (1)
ReadonlyWallet(4-10)
src/screens/Init/Connect.tsx (2)
src/providers/wallet.tsx (1)
WalletContext(55-70)src/lib/logs.ts (1)
consoleError(40-44)
src/screens/Settings/Password.tsx (4)
src/lib/privateKey.ts (1)
noUserDefinedPassword(65-67)src/lib/constants.ts (1)
defaultPassword(7-7)src/lib/logs.ts (1)
consoleError(40-44)src/components/ReadonlyWallet.tsx (1)
ReadonlyWallet(4-10)
src/screens/Settings/Vtxos.tsx (2)
src/lib/asp.ts (2)
getInputsToSettle(232-239)settleVtxos(241-268)src/components/ReadonlyWallet.tsx (1)
ReadonlyWallet(4-10)
src/screens/Settings/Advanced.tsx (2)
src/providers/wallet.tsx (1)
WalletContext(55-70)src/providers/options.tsx (2)
optionsUsingPrivKey(105-105)options(22-103)
src/screens/Wallet/Send/Details.tsx (1)
src/lib/asp.ts (2)
sendOffChain(224-226)collaborativeExitWithFees(80-123)
src/lib/asp.ts (1)
src/lib/types.ts (4)
Satoshis(41-41)Tx(75-85)Vtxo(101-101)Addresses(4-7)
src/screens/Wallet/Notes/Redeem.tsx (2)
src/components/ReadonlyWallet.tsx (1)
ReadonlyWallet(4-10)src/lib/asp.ts (1)
redeemNotes(202-222)
src/App.tsx (3)
src/components/Focusable.tsx (1)
Focusable(3-55)src/components/FlexCol.tsx (1)
FlexCol(16-46)src/icons/Apps.tsx (1)
AppsIcon(1-12)
src/lib/wallet.ts (2)
src/lib/types.ts (1)
Vtxo(101-101)src/providers/asp.tsx (1)
AspInfo(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (20)
src/screens/Init/Success.tsx (1)
17-17: LGTM! Readonly detection and messaging implemented correctly.The logic correctly identifies readonly mode by checking for a public key without a private key, and the conditional text rendering appropriately informs users about their readonly wallet status.
Also applies to: 19-25
src/App.tsx (2)
83-83: LGTM! Bailout condition extended correctly for readonly wallet setup.Adding
initInfo.publicKeyto the bailout condition ensures the app doesn't redirect during readonly wallet initialization, consistent with the existing private key and password checks.
159-168: LGTM! Apps tab correctly hidden in readonly mode.Hiding the Apps tab when
wallet.isReadonlyis true aligns with the PR objective of disabling functionality that requires signing operations. The conditional rendering is clean and preserves the tab structure for non-readonly wallets.src/screens/Wallet/Send/Details.tsx (2)
99-99: LGTM! Writer guard correctly prevents sending in readonly mode.The guard now checks for
svcWallet?.writerinstead of justsvcWallet, properly preventing send operations when only a reader is available.
102-102: LGTM! Writer correctly used for signing operations.Both
sendOffChainandcollaborativeExitWithFeesnow receivesvcWallet.writer, which is the appropriate wallet interface for operations requiring signatures. Based on learnings, these functions maintain their IWallet-only signatures as expected.Also applies to: 110-112
src/screens/Settings/Password.tsx (1)
30-34: Good improvement: Error logging added for password loading.Adding
.catch(consoleError)provides visibility into failures when checking for user-defined passwords. This error handling is appropriately scoped to thenoUserDefinedPassword()call and doesn't conflict with the existing authentication flow.src/providers/lightning.tsx (1)
80-112: LGTM: Lightning correctly requires writer access.The initialization guard now correctly checks for
svcWallet?.writer, ensuring Lightning features are unavailable in read-only mode. This aligns with the PR's goal of disabling functionality that requires signing capabilities.src/lib/wallet.ts (1)
7-7: LGTM: Appropriate use of IReadonlyWallet interface.The change from
IWallettoIReadonlyWalletis correct sincecalcNextRolloveronly performs read operations (accessing UTXOs and calculating rollover times) without requiring signing capabilities.Also applies to: 21-21
src/screens/Settings/Menu.tsx (1)
29-29: LGTM: ReadonlyWallet component provides good user feedback.The conditional rendering of the
ReadonlyWalletcomponent whenwallet.isReadonlyis true provides clear visual feedback to users about the wallet's read-only state and its limitations.src/providers/options.tsx (1)
105-105: The code is correct as written. The Reset option should not be included inoptionsUsingPrivKeybecause it does not access or require the private key. Unlike Password (which validatesoldPasswordviagetPrivateKey()), Notes, and Lock, theresetWallet()function only clears storage and wallet data without accessing any private key material. Reset requires only a confirmation checkbox, no authentication.Likely an incorrect or invalid review comment.
src/screens/Settings/Vtxos.tsx (1)
248-249: Readonly banner integration looks good.src/screens/Init/Restore.tsx (1)
106-110: Good: private-key input remains uncontrolled (security). Based on learnings, this avoids re-rendering sensitive keys back into the UI.src/screens/Wallet/Unlock.tsx (1)
26-48: No changes needed—the code is correct as written.The
walletFromStorage.pubkeyis stored as hex (not npub), sohexToBytes()is the appropriate conversion. The promise chain withnavigate()follows the same pattern used throughout the codebase without requiring cancellation guards.Likely an incorrect or invalid review comment.
src/lib/asp.ts (1)
1-1: LGTM! Appropriate broadening of API to support read-only wallets.The signature changes from
IWallettoIReadonlyWalletfor read-only operations (getBalance,getTxHistory,getVtxos,getReceivingAddresses, andcaptureSettleError) are correct. These functions only read wallet state and don't require signing capabilities. Write operations (collaborativeExit,redeemNotes,sendOnChain,settleVtxos, etc.) correctly maintain theirIWalletsignatures.Based on learnings, the previous guidance about maintaining IWallet-only signatures was specific to avoiding
ServiceWorkerWallettypes, notIReadonlyWallet. This change enables the read-only wallet feature while preserving type safety.Also applies to: 139-143, 145-179, 181-192, 194-200, 279-296
src/providers/wallet.tsx (6)
35-35: LGTM! Clean separation of read/write capabilities.The
SvcWallettype with mandatoryreaderand optionalwriterprovides a clear contract: read operations always work, write operations only available whenwriteris present. The updated context interface correctly exposesinitReadonlyWalletand the parameterlessreloadWallet.Also applies to: 37-53
144-148: LGTM! Consolidated state improves atomicity.Grouping
txs,vtxos, andbalanceinto a singlewalletStatesobject is a good refactor. This ensures these related values are updated together, reducing the risk of inconsistent UI state.Also applies to: 152-152
334-369: LGTM! Proper reader/writer separation in wallet operations.The wallet operation functions correctly distinguish between read and write operations:
lockWallet,resetWallet, andisLockedusesvcWallet.readerfor operations available to all walletssettlePreconfirmedchecks forsvcWallet?.writerbefore attempting write operations (line 348)This ensures read-only wallets can perform all read operations while write operations are properly gated.
220-232: Sequential execution is necessary due to indexer caching.The comment "running them in parallel can get stale data" correctly reflects a limitation in the underlying indexer. The implementation in lines 224-226 is appropriate. This is confirmed by the retry pattern at lines 244-247, which reloads the wallet again after 5 seconds to allow the indexer cache time to update—indicating this is an ongoing, acknowledged constraint. Parallelizing these calls with
Promise.allwould risk reintroducing stale data inconsistencies.
236-251: Remove unsupported regression claim; the setTimeout patterns are appropriate for eventual consistency handling.The setTimeout workarounds at lines 243-247 (5s delay) and line 260 (250ms delay) are legitimate eventual consistency fixes for service worker event handling and UI race conditions. However, the review claims these are insufficient to fix a regression where "transactions do not appear after onchain receive or redeem," but this regression is not documented or evidenced in the codebase. Without confirming the actual regression exists or understanding its failure mode, suggestions for exponential backoff or message-type-specific delays are speculative.
If a transaction visibility issue exists, verify and document it separately before proposing timing changes.
Likely an incorrect or invalid review comment.
14-21: Fix invalid @arkade-os/sdk version dependency.The project specifies
@arkade-os/sdkv0.3.10 in package.json, but this version does not exist in the npm registry. The latest published version is only 0.2.3, with GitHub releases showing up to v0.3.7/v0.3.5. Additionally, the imported typesServiceWorkerReadonlyWalletandReadonlySingleKeyare not documented in the official SDK documentation up to v0.3.x. Either use a published version (0.2.3 or verify if 0.3.7/0.3.5 is intended) or clarify if this is a private/monorepo dependency that requires local setup.⛔ Skipped due to learnings
Learnt from: bordalix Repo: arkade-os/wallet PR: 114 File: src/lib/asp.ts:0-0 Timestamp: 2025-06-30T18:33:29.839Z Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.Learnt from: bordalix Repo: arkade-os/wallet PR: 114 File: public/wallet-service-worker.mjs:6350-6353 Timestamp: 2025-06-30T18:23:34.163Z Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.Learnt from: bordalix Repo: arkade-os/wallet PR: 106 File: src/lib/bolt11.ts:153-155 Timestamp: 2025-07-07T14:41:35.991Z Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/screens/Init/Restore.tsx:
- Around line 78-92: The handleProceed logic navigates to InitSuccess
unconditionally in the .finally() of BackupProvider.restore; change it so
navigation only happens on successful restore and failures are surfaced: call
new BackupProvider({ seckey: privateKey! }).restore(updateConfig).then(() => {
navigate(Pages.InitSuccess) }).catch((err) => { consoleError(err, 'Error
restoring from nostr'); /* set an error state or show a UI error */
}).finally(() => { setRestoring(false) }); ensure setRestoring(true) still runs
before starting the restore and remove navigate(Pages.InitSuccess) from the
current .finally() to avoid showing success on failure.
In @src/test/e2e/backup.test.ts:
- Around line 52-68: Rename the test title in the test(...) call from "should be
able to get nspub without password" to "should be able to get npub without
password" and update the UI text matcher call page.getByText('copy to
clipboard') to match the actual button label (e.g., page.getByText('Copy to
clipboard') or use a case-insensitive regex like page.getByText(/copy to
clipboard/i)) so the assertion finds the correct element.
In @src/test/e2e/readonly-wallet.test.ts:
- Around line 52-65: Replace the misspelled locator in the e2e test so the
assertion checks the exact text; change the assertion that currently looks for
"Change passowrd" to use the correct spelling "Change password" and include the
exact: true option (i.e., update the getByText call in the
readonly-wallet.test.ts Advanced section to getByText('Change password', {
exact: true }) before asserting not.toBeVisible()).
- Around line 2-27: The test 'should receive onchain funds' uses bare exec() for
the faucet (and another send command elsewhere) so failures are not awaited;
replace those with a promisified exec (e.g., const execAsync = promisify(exec))
and await execAsync(...) in this test (and the paired send command) so the test
will surface command failures; after awaiting, check the returned stdout/stderr
or let execAsync reject on non-zero exit so the test fails fast if the
faucet/send command fails.
🧹 Nitpick comments (6)
src/test/e2e/init.test.ts (1)
20-28: Consider extracting the repeated onboarding navigation into a helper (reduces brittleness).
This sequence is duplicated across e2e tests/utilities; a helper likegoToOtherLoginOptions(page)would centralize UI text changes.src/test/e2e/backup.test.ts (1)
11-12: Make the “This is enough to restore your wallet.” assertion consistent (exact match in both tests).
Right now one uses{ exact: true }and the other doesn’t, which can hide copy regressions or cause flakiness if the string becomes a substring of a longer message.Also applies to: 34-35
src/screens/Settings/Menu.tsx (1)
13-22: Typo:excludeInReaonlyMode→excludeInReadonlyMode.
This appears in multiple files; renaming improves maintainability and avoids propagating the misspelling.src/screens/Settings/Advanced.tsx (1)
11-13: Same typo rename here too:excludeInReaonlyMode→excludeInReadonlyMode.src/test/e2e/readonly-wallet.test.ts (1)
11-27: Add a tx-list assertion to catch the regression you described (received tx not appearing).
After “Payment received!”, also assert the transaction list is no longer “No transactions yet” (or assert presence of a tx row/testid).Also applies to: 29-45
src/test/e2e/utils.ts (1)
128-142: ReplacewaitForTimeoutwith deterministic waits.
The fixed 500/1000ms sleeps will be flaky under load; prefer waiting for “Restore wallet” screen visibility, “Go to wallet” button, etc.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/screens/Init/Restore.tsxsrc/screens/Settings/Advanced.tsxsrc/screens/Settings/Menu.tsxsrc/test/e2e/backup.test.tssrc/test/e2e/init.test.tssrc/test/e2e/readonly-wallet.test.tssrc/test/e2e/utils.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Settings/Advanced.tsxsrc/test/e2e/readonly-wallet.test.tssrc/test/e2e/init.test.tssrc/screens/Settings/Menu.tsxsrc/test/e2e/backup.test.tssrc/test/e2e/utils.ts
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Settings/Advanced.tsxsrc/screens/Settings/Menu.tsxsrc/test/e2e/backup.test.tssrc/screens/Init/Restore.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Settings/Advanced.tsxsrc/screens/Settings/Menu.tsxsrc/test/e2e/backup.test.tssrc/screens/Init/Restore.tsx
🧬 Code graph analysis (5)
src/screens/Settings/Advanced.tsx (3)
src/providers/wallet.tsx (1)
WalletContext(55-70)src/providers/options.tsx (2)
optionsUsingPrivKey(105-105)options(22-103)src/components/ReadonlyWallet.tsx (1)
ReadonlyWallet(4-10)
src/test/e2e/readonly-wallet.test.ts (1)
src/test/e2e/utils.ts (5)
createWallet(3-14)resetAndRestoreReadonlyWallet(136-142)receiveOnchain(75-77)waitForPaymentReceived(157-159)receiveOffchain(79-81)
src/screens/Settings/Menu.tsx (3)
src/providers/wallet.tsx (1)
WalletContext(55-70)src/providers/options.tsx (2)
optionsUsingPrivKey(105-105)options(22-103)src/components/ReadonlyWallet.tsx (1)
ReadonlyWallet(4-10)
src/test/e2e/backup.test.ts (1)
src/test/e2e/utils.ts (1)
createWallet(3-14)
src/screens/Init/Restore.tsx (6)
src/lib/error.ts (1)
extractError(1-10)src/lib/constants.ts (1)
defaultPassword(7-7)src/lib/backup.ts (1)
BackupProvider(17-166)src/components/Input.tsx (1)
Input(13-40)src/components/Warning.tsx (1)
WarningBox(12-37)src/components/Text.tsx (1)
Text(25-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (5)
src/screens/Init/Restore.tsx (2)
106-114: Good: keep the key input uncontrolled (novalue) while expanding to npub/nsec.
This preserves the “don’t repopulate sensitive input on navigation” behavior while enabling the read-only path. Based on learnings, this is intentional and important.
20-23: The core technical concerns in this review are incorrect. The code is already correct.The
nip19.decode('npub...').datafor npub keys returns a hex string (confirmed in [email protected] spec), not a variable type. ThehexToBytes()call is correct.The import path
'@noble/hashes/utils.js'(already on line 21) is the required and only correct way to import from @noble/[email protected] (ESM-only release). This is not bundler-sensitive; it's the mandated path.The proposed fix that adds a type guard (
typeof data === 'string' ? ... : ...) is unnecessary—datais always a string for npub keys and always Uint8Array for nsec keys.Valid but minor concerns remain:
- Input not trimmed: whitespace in pasted keys will fail, though errors are caught and displayed.
- Private-key restore error handling: errors are only logged, not shown to user; navigation to success happens regardless (lines 86–90).
Likely an incorrect or invalid review comment.
src/screens/Settings/Menu.tsx (1)
29-33: Readonly banner placement is good (prevents confusing “missing options” UX).
Nice touch to show the reason before users hit filtered menus.src/test/e2e/utils.ts (2)
51-55: Good change:Receiveclick uses{ exact: true }(less ambiguity if there are multiple “Receive” strings).
110-126: Verify casing for “Other login options” (tests use multiple variants across files).
If the UI is “Other Login Options”, this will fail; consider targeting by role/name or normalizing to the exact UI string everywhere.Potential approach
- await page.getByText('Other login options').click() + await page.getByRole('button', { name: /Other Login Options/i }).click()
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/screens/Wallet/Notes/Redeem.tsx (1)
44-55: Critical: Unsafe access to writer causes freeze in readonly mode.The non-null assertion
!after optional chaining on line 48 is unsafe. In readonly mode,svcWallet.writerisundefined, and callingredeemNoteswithundefinedwill cause a runtime error or freeze—this likely explains the bug reported in the PR comments where the redeem flow freezes in readonly mode.Add an early return guard to prevent execution when writer is unavailable.
🛡️ Proposed fix: Add guard clause
const handleRedeem = async () => { + if (!svcWallet?.writer) { + setError('Cannot redeem notes in read-only mode') + return + } setError('') setRedeeming(true) try { - await redeemNotes(svcWallet?.writer!, [noteInfo.note]) + await redeemNotes(svcWallet.writer, [noteInfo.note]) navigate(Pages.NotesSuccess) } catch (err) { consoleError(err, 'error redeeming note') setError(extractError(err)) } setRedeeming(false) }src/screens/Init/Connect.tsx (2)
22-36: Init effect with[]deps can miss readonly init ifinitInfoisn’t ready on first mount.If
initInfo.publicKey/password/privateKeycan be populated after navigation (even briefly), this effect won’t rerun and the screen can hang. Consider adding the relevant deps + an idempotent guard.
38-54: Avoid a potential “Connecting…” dead-end; also awaitreloadWallet()and clearpublicKeyon proceed.When
initInfo.restoringis true,if (!arkadeLightning || !svcWallet) returncan leave users stuck forever if either never becomes available (or is intentionally absent in readonly mode). Also,reloadWallet()is async and currently not awaited, so the “refresh transaction history” comment may not hold. Finally,handleProceedshould clearpublicKeytoo to avoid stale init state.Proposed fix
useEffect(() => { if (!initialized) return if (!initInfo.restoring) return handleProceed() - if (!arkadeLightning || !svcWallet) return - reloadWallet() // side-effect to refresh transaction history - restoreSwaps() - .then((count) => count && consoleLog(`Restored ${count} swaps from network`)) - .catch((err) => consoleError(err, 'Error restoring swaps:')) - .finally(handleProceed) + ;(async () => { + try { + // refresh transaction history before leaving init + await reloadWallet() + + // restore swaps only when the required services are available + if (arkadeLightning && svcWallet) { + const count = await restoreSwaps() + if (count) consoleLog(`Restored ${count} swaps from network`) + } + } catch (err) { + consoleError(err, 'Error during post-init restore:') + } finally { + handleProceed() + } + })() }, [svcWallet, arkadeLightning, initialized, initInfo.restoring]) @@ const handleProceed = () => { - setInitInfo({ ...initInfo, password: undefined, privateKey: undefined }) + setInitInfo({ ...initInfo, password: undefined, privateKey: undefined, publicKey: undefined, restoring: false }) navigate(Pages.Wallet) }
🤖 Fix all issues with AI agents
In @src/screens/Init/Restore.tsx:
- Around line 21-23: InitInfo.publicKey is typed as Uint8Array | undefined but
initReadonlyWallet(pubkey: Uint8Array) requires a non-optional Uint8Array; add a
runtime check or assertion before calling initReadonlyWallet to guarantee a
defined value. Specifically, in the flow that retrieves or decodes the public
key (the variable named publicKey / InitInfo.publicKey), perform a
null/undefined guard (e.g., if (!publicKey) handle the error/return or throw) or
explicitly narrow the type before passing it to initReadonlyWallet;
alternatively restructure the logic so initReadonlyWallet is only called after
publicKey is confirmed present. Ensure the fix references the same symbols:
InitInfo.publicKey and initReadonlyWallet so callers never pass an undefined
value.
In @src/test/e2e/readonly-wallet.test.ts:
- Around line 22-26: The test currently fire-and-forgets shell calls using
exec(...) (e.g., the faucet call before waitForPaymentReceived and the other
occurrences around lines 39-44); change these to an awaited Promise-based call
(use util.promisify(exec) or child_process.execFile) and await the result with
sensible timeout/maxBuffer so failures throw and fail the test; ensure you await
the promisifiedExec(`nigiri faucet ${boardingAddress} 0.0001`) and
check/propagate errors (reject on non-zero exit) before calling
waitForPaymentReceived, and apply the same change to the other exec usages
mentioned.
🧹 Nitpick comments (4)
src/screens/Wallet/Notes/Redeem.tsx (1)
78-80: Consider hiding the redeem button entirely in readonly mode.The button is currently disabled in readonly mode, but showing it might confuse users. Consider hiding it entirely when
wallet.isReadonlyto provide clearer UX.♻️ Optional UX improvement
-<ButtonsOnBottom> - <Button onClick={handleRedeem} label={buttonLabel} disabled={redeeming || wallet.isReadonly} /> -</ButtonsOnBottom> +{!wallet.isReadonly && ( + <ButtonsOnBottom> + <Button onClick={handleRedeem} label={buttonLabel} disabled={redeeming} /> + </ButtonsOnBottom> +)}src/screens/Settings/Advanced.tsx (1)
11-13: Hiding privkey-dependent Advanced options: looks right, but also guard direct navigation.Filtering rows keeps “Change password / Notes / Lock” out of the menu in read-only mode, but users can still potentially deep-link into those routes—consider adding an explicit
wallet.isReadonlyguard in the corresponding screens as well (fail closed / redirect back).src/test/e2e/readonly-wallet.test.ts (1)
11-27: Add a tx-list assertion to cover the reported regression (tx not appearing).Right now these tests validate the “received” toast/message but not that the transaction actually shows up in history; adding an assertion in the list view would prevent the specific regression noted in the PR comments from slipping.
Also applies to: 29-45
src/screens/Init/Restore.tsx (1)
39-74: Small robustness/consistency tweaks: trim input + consistent “read-only” wording.Consider trimming
someKeybefore prefix checks/decoding, and standardizing copy to “read-only” (you currently use both “readonly” and “read-only”). Keeping the input uncontrolled is still the right call here (as per the prior security reasoning).Also applies to: 106-115
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/screens/Init/Connect.tsxsrc/screens/Init/Restore.tsxsrc/screens/Settings/Advanced.tsxsrc/screens/Settings/Menu.tsxsrc/screens/Settings/Password.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Notes/Redeem.tsxsrc/test/e2e/backup.test.tssrc/test/e2e/readonly-wallet.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/screens/Settings/Password.tsx
- src/screens/Settings/Vtxos.tsx
- src/test/e2e/backup.test.ts
- src/screens/Settings/Menu.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Init/Restore.tsxsrc/screens/Settings/Advanced.tsxsrc/screens/Init/Connect.tsxsrc/screens/Wallet/Notes/Redeem.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Init/Restore.tsxsrc/screens/Settings/Advanced.tsxsrc/test/e2e/readonly-wallet.test.tssrc/screens/Init/Connect.tsxsrc/screens/Wallet/Notes/Redeem.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Settings/Advanced.tsxsrc/test/e2e/readonly-wallet.test.tssrc/screens/Init/Connect.tsxsrc/screens/Wallet/Notes/Redeem.tsx
📚 Learning: 2025-12-20T10:09:41.030Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 278
File: src/providers/annoucements.tsx:32-50
Timestamp: 2025-12-20T10:09:41.030Z
Learning: In src/providers/annoucements.tsx, the useEffect hook depends on [config] (not just [configLoaded]) because it needs to react to config changes after nostr restoration. The seen.current ref guard prevents showing multiple announcements per session, even though the effect may re-run on config updates.
Applied to files:
src/screens/Init/Connect.tsx
🧬 Code graph analysis (5)
src/screens/Init/Restore.tsx (9)
public/wallet-service-worker.mjs (4)
e(1866-1866)e(5572-5572)e(7068-7068)e(10458-10458)src/lib/logs.ts (1)
consoleError(40-44)src/lib/error.ts (1)
extractError(1-10)src/lib/privateKey.ts (1)
setPrivateKey(46-54)src/lib/constants.ts (1)
defaultPassword(7-7)src/lib/backup.ts (1)
BackupProvider(17-166)src/components/Input.tsx (1)
Input(13-40)src/components/Warning.tsx (1)
WarningBox(12-37)src/components/Text.tsx (1)
Text(25-79)
src/screens/Settings/Advanced.tsx (4)
src/providers/wallet.tsx (1)
WalletContext(55-70)src/providers/options.tsx (2)
optionsUsingPrivKey(105-105)options(22-103)src/components/Content.tsx (1)
Content(9-16)src/components/ReadonlyWallet.tsx (1)
ReadonlyWallet(4-10)
src/test/e2e/readonly-wallet.test.ts (1)
src/test/e2e/utils.ts (5)
createWallet(3-14)resetAndRestoreReadonlyWallet(136-142)receiveOnchain(75-77)waitForPaymentReceived(157-159)receiveOffchain(79-81)
src/screens/Init/Connect.tsx (1)
src/providers/wallet.tsx (1)
WalletContext(55-70)
src/screens/Wallet/Notes/Redeem.tsx (3)
src/providers/wallet.tsx (1)
WalletContext(55-70)src/lib/asp.ts (1)
redeemNotes(202-222)src/components/ReadonlyWallet.tsx (1)
ReadonlyWallet(4-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (1)
src/screens/Settings/Advanced.tsx (1)
19-23: Readonly banner in Advanced is a good UX cue.The inline
ReadonlyWalletwarning is consistent with the intent and should reduce confusion while browsing settings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/providers/wallet.tsx (4)
72-109: Verify mutual exclusivity of privateKey and publicKey parameters.The
InitSvcWorkerWalletParamstype allows bothprivateKeyandpublicKeyto be provided, but the implementation (lines 95-109) assumes they are mutually exclusive. If both are provided, theprivateKeypath executes andpublicKeyis silently ignored.Consider making the mutual exclusivity explicit at the type level or adding a runtime check to throw an error if both are provided.
🔧 Proposed type-level fix using discriminated union
-type InitSvcWorkerWalletParams = { - arkServerUrl: string - esploraUrl: string - privateKey?: string - publicKey?: string - retryCount?: number - maxRetries?: number -} +type InitSvcWorkerWalletParams = { + arkServerUrl: string + esploraUrl: string + retryCount?: number + maxRetries?: number +} & ( + | { privateKey: string; publicKey?: never } + | { publicKey: string; privateKey?: never } +)
263-272: Consider increasing the status polling interval to reduce resource usage.The service worker status is polled every 1 second (line 263). For battery and performance optimization, consider increasing this interval (e.g., 5-10 seconds) unless real-time status updates are critical for the user experience.
297-300: Redundant timeout error check in setupSvcWorker.The timeout error check at lines 297-300 is redundant because
initSvcWorkerWalletalready throws this exact error message after exhausting retries (line 130). This catch block will never be reached for timeout errors.♻️ Simplify error handling
} catch (err) { console.error(err) - if (err instanceof Error && err.message.includes('Service worker activation timed out')) { - consoleError(err, 'Service worker activation failed') - return - } - // re-throw other errors throw err }
306-332: Potential stale state issue in initWallet and initReadonlyWallet.Both functions (lines 317 and 331) call
updateWallet({ ...wallet, ... })using the closure variablewallet, which may be stale if the component has re-rendered. This could overwrite concurrent updates to the wallet state.Consider using the functional update pattern to ensure you're working with the latest state:
♻️ Use functional update pattern
const initWallet = async (privateKey: Uint8Array) => { const arkServerUrl = aspInfo.url const network = aspInfo.network as NetworkName const esploraUrl = getRestApiExplorerURL(network) ?? '' const pubkey = hex.encode(secp.getPublicKey(privateKey)) updateConfig({ ...config, pubkey }) await setupSvcWorker({ privateKey: hex.encode(privateKey), arkServerUrl, esploraUrl, }) - updateWallet({ ...wallet, network, pubkey, isReadonly: false }) + updateWallet((prev) => ({ ...prev, network, pubkey, isReadonly: false })) } const initReadonlyWallet = async (publicKey: Uint8Array) => { const pubkey = hex.encode(publicKey) const arkServerUrl = aspInfo.url const network = aspInfo.network as NetworkName const esploraUrl = getRestApiExplorerURL(network) ?? '' updateConfig({ ...config, pubkey }) await setupSvcWorker({ publicKey: pubkey, arkServerUrl, esploraUrl, }) - updateWallet({ ...wallet, network, pubkey, isReadonly: true }) + updateWallet((prev) => ({ ...prev, network, pubkey, isReadonly: true })) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/providers/wallet.tsxsrc/screens/Init/Connect.tsx
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Init/Connect.tsxsrc/providers/wallet.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Init/Connect.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Init/Connect.tsxsrc/providers/wallet.tsx
📚 Learning: 2025-06-30T18:23:34.163Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
Applied to files:
src/providers/wallet.tsx
📚 Learning: 2025-12-20T10:09:41.030Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 278
File: src/providers/annoucements.tsx:32-50
Timestamp: 2025-12-20T10:09:41.030Z
Learning: In src/providers/annoucements.tsx, the useEffect hook depends on [config] (not just [configLoaded]) because it needs to react to config changes after nostr restoration. The seen.current ref guard prevents showing multiple announcements per session, even though the effect may re-run on config updates.
Applied to files:
src/providers/wallet.tsx
🧬 Code graph analysis (2)
src/screens/Init/Connect.tsx (3)
src/providers/wallet.tsx (1)
WalletContext(55-70)src/test/setup.mjs (1)
initialized(50-50)src/lib/logs.ts (2)
consoleError(40-44)consoleLog(35-38)
src/providers/wallet.tsx (3)
src/lib/types.ts (3)
Wallet(103-111)Tx(75-85)Vtxo(101-101)src/lib/wallet.ts (2)
calcBatchLifetimeMs(37-48)calcNextRollover(21-35)src/lib/asp.ts (3)
getTxHistory(145-179)getVtxos(181-192)getBalance(139-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (9)
src/providers/wallet.tsx (6)
80-135: LGTM! Well-implemented retry logic with exponential backoff.The retry mechanism correctly handles service worker activation timeouts with exponential backoff, clear error messages, and proper error propagation for non-timeout errors.
144-148: LGTM! Consolidated state improves performance.Grouping related wallet state (txs, vtxos, balance) into a single object reduces unnecessary re-renders when these values are updated together.
236-251: ****Re-reading the code, the early return at line 241 is correctly present. The message handler properly guards against undefined
svcWallet.
347-351: LGTM! settlePreconfirmed correctly requires a writable wallet.The function properly guards against read-only mode by checking for
svcWallet?.writerbefore attempting to settle VTXOs, which aligns with the PR objective of disabling send/settle functionality in read-only mode.
371-393: LGTM! Context provider correctly exposes all wallet functionality.The provider value correctly includes both the new
initReadonlyWalletfunction and updated state values, maintaining consistency with the expandedWalletContextPropsinterface.
236-251: Message handler silently ignores updates when svcWallet is undefined.Lines 239-242 log an error but don't return early when
svcWalletis undefined, allowing the function to continue toreloadWallet()which will fail since it guards on!svcWalletat line 221.The early return is missing after the error log.
🐛 Proposed fix to add early return
if (!svcWallet) { consoleError('Cannot handle a message when svcWallet is not defined') - return + return }Actually, looking at line 241, the return IS there. Let me re-check... Yes, the return statement is present at line 241. This is not an issue.
⛔ Skipped due to learnings
Learnt from: bordalix Repo: arkade-os/wallet PR: 114 File: src/lib/asp.ts:0-0 Timestamp: 2025-06-30T18:33:29.839Z Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.Learnt from: bordalix Repo: arkade-os/wallet PR: 114 File: public/wallet-service-worker.mjs:6350-6353 Timestamp: 2025-06-30T18:23:34.163Z Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.src/screens/Init/Connect.tsx (3)
18-18: LGTM! Correctly added dependencies for read-only wallet support.The destructuring adds
walletandinitReadonlyWalletfrom context andpublicKeyfrom initInfo, all of which are used in the updated initialization logic.Also applies to: 22-22
24-36: LGTM! Read-only initialization correctly takes precedence.The early return at line 29 ensures that when
publicKeyis present, the read-only path executes and the private key initialization is skipped. This aligns with the expected behavior for read-only wallet restoration.
38-46: LGTM! Restoration correctly skipped for read-only wallets.The logic properly prevents swap restoration in read-only mode (line 40), aligning with the PR objective to disable write operations. The dependency array correctly includes
wallet.isReadonlyto re-evaluate when this flag changes.Based on learnings, the effect depends on
wallet.isReadonlyin addition to other dependencies to properly guard restoration logic.
|
Thank you @bordalix Fixed:
|
|
Hey @pietro909 — this got approved by @louisinger back in January and has been sitting open for 2+ months. Intentionally on hold, or is it ready to merge? |
🔍 Arkana Review — Readonly WalletSolid feature. The 🐛 Bug: Inverted guard in
|
3 participants
Support Wallet restore via Public Key: it gives access to the transactions history in read-only mode.
Apps and anything requiring
IWallet.sendBitcoinorIWallet.settleisn't available.Some UI changes:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.