Repository navigation
Conversation
WalkthroughAdds an Escrow app: UI icon and screen, an iframe host with keep-alive and origin validation, an RPC message handler for wallet operations (signing, balance, funding, keep-alive), navigation/deeplink wiring, tests, and updated docs/type for a new environment variable. Changes
Sequence Diagram(s)sequenceDiagram
participant Iframe as Escrow Iframe
participant Host as Host App (ArkadeIframeHost)
participant Handler as RPC Handler (makeMessageHandler)
participant Wallet as Wallet Service
Iframe->>Host: postMessage(RPC request)
Host->>Host: validate origin & source
Host->>Handler: handle(InboundMessage)
Handler->>Wallet: invoke method (e.g., sign / getBalance / fund)
Wallet-->>Handler: result (signed PSBT / balance / txId)
Handler-->>Host: OutboundMessage (response)
Host-->>Iframe: postMessage(response)
alt Keep-alive flow
Host->>Iframe: postMessage(ARKADE_KEEP_ALIVE) every 2s until alive, then every 5s
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/Apps/Index.tsx (1)
95-140: Clean up unuseddeepLinkInfoand consider network-based Escrow gating
deepLinkInfofromFlowContextis currently unused (isEscrowEnabledis hard-coded totrue), which is triggering the static-analysis warning and adds noise.- Also, Escrow is rendered in the list on all networks, but
AppEscrowonly shows an iframe whenBASE_URLS[aspInfo.network]is non-null; on unsupported networks, clicking Escrow leads to a header-only screen.Consider either:
- Dropping
FlowContext/deepLinkInfohere and keeping Escrow always visible, or- Using a real condition (e.g., a feature flag or a network check mirroring
BASE_URLS) to controlisEscrowEnabledso the tile is hidden when the Escrow app is not actually available.Example if you just want it always on and to silence the warning:
-import { FlowContext } from '../../providers/flow' +// import { FlowContext } from '../../providers/flow' ... -export default function Apps() { - const { deepLinkInfo } = useContext(FlowContext) - const isEscrowEnabled = true // deepLinkInfo?.appId === 'escrow' +export default function Apps() { + const isEscrowEnabled = true
🧹 Nitpick comments (5)
src/providers/options.tsx (1)
53-57: Consider using a distinct icon for the Sign option.The
Signoption reusesLogsIcon, which is already used for theLogsoption (lines 48-52). This may confuse users when navigating settings. Consider creating or using a dedicated icon (e.g., a pen/signature icon) for better visual distinction.src/screens/Apps/Escrow/RpcHandler.ts (2)
208-211: Unreachable code after exhaustive switch.Line 211 is unreachable because the
switch (message.kind)covers both possible cases (ARKADE_KEEP_ALIVEandARKADE_RPC_REQUEST), and the innerswitch (method)is also exhaustive. TypeScript's control flow analysis should flag this. Consider removing the unreachable return or adding an explicitdefaultcase withnevertype assertion for compile-time exhaustiveness checking.case 'fund-address': { // ... existing code ... } + default: { + const _exhaustiveCheck: never = method + return { tag: 'failure', error: new Error(`Unknown method: ${_exhaustiveCheck}`) } + } } } + default: { + const _exhaustiveCheck: never = message + return { tag: 'failure', error: new Error('Unknown message kind') } + } } - return { tag: 'failure', error: new Error('Unknown message kind') } }
124-160: Consider consistent error handling for getter methods.The
get-x-public-key,get-ark-wallet-address, andget-ark-wallet-balancemethods don't have try-catch wrappers, unlikesign-login-challenge,sign-transaction, andfund-address. If any of these getter functions throw, the error will bubble up as an unhandled promise rejection rather than returning a structured{ tag: 'failure', error }result.src/screens/Apps/Escrow/Index.tsx (1)
17-26: Centralize the crypto hash configuration and align Escrow availability with network support
hashes.sha256 = sha256mutates global crypto-library configuration inside this screen. That’s easy to miss and couples the global signing behavior to whether this component module has been loaded.- Separately,
escrowAppUrlis only set whenBASE_URLS[aspInfo.network]is non-null, so on networks without an entry (e.g., where it’snull),AppEscrowrenders just the header and no iframe.Consider:
- Moving the
hashes.sha256 = sha256assignment to a single, shared initialization point (e.g., wherever the wallet / signing stack is first configured) so that all flows using the identity share the same, explicit hash configuration.- Optionally tying the Escrow tile visibility (in
Apps/Index.tsx) to the sameBASE_URLS/network logic so users don’t navigate into a header-only Escrow page on unsupported networks.Also applies to: 36-41, 72-81
src/screens/Apps/Escrow/ArkadeIframeHost.tsx (1)
10-27: Tighten the origin check to use equality instead ofstartsWithThe message handling pipeline (origin check +
event.source === iframeRef.current?.contentWindow+ shape validation) looks solid.Given that
allowedChildOriginsare already bare origins (as built inAppEscrow), you can simplify and harden the origin check:- if (!allowedChildOrigins.some((_) => _.startsWith(event.origin))) { + if (!allowedChildOrigins.includes(event.origin)) { console.error(`[wallet]: ignoring message from ${event.origin}`) return }This keeps the intent obvious and avoids any accidental prefix matches if
allowedChildOriginsever started to include paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/escrow.pngis excluded by!**/*.png
📒 Files selected for processing (11)
README.md(1 hunks)src/icons/Escrow.tsx(1 hunks)src/providers/navigation.tsx(4 hunks)src/providers/options.tsx(1 hunks)src/providers/wallet.tsx(1 hunks)src/screens/Apps/Escrow/ArkadeIframeHost.tsx(1 hunks)src/screens/Apps/Escrow/Index.tsx(1 hunks)src/screens/Apps/Escrow/RpcHandler.test.ts(1 hunks)src/screens/Apps/Escrow/RpcHandler.ts(1 hunks)src/screens/Apps/Index.tsx(3 hunks)src/vite-env.d.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/Apps/Escrow/RpcHandler.ts
🧬 Code graph analysis (6)
src/providers/options.tsx (1)
src/icons/Logs.tsx (1)
LogsIcon(1-10)
src/screens/Apps/Index.tsx (3)
src/providers/flow.tsx (1)
FlowContext(89-104)src/App.tsx (1)
App(36-164)src/icons/Escrow.tsx (1)
EscrowIcon(1-3)
src/screens/Apps/Escrow/RpcHandler.test.ts (1)
src/screens/Apps/Escrow/RpcHandler.ts (1)
makeMessageHandler(116-213)
src/screens/Apps/Escrow/Index.tsx (6)
src/providers/navigation.tsx (1)
NavigationContext(174-178)src/providers/wallet.tsx (1)
WalletContext(42-56)src/providers/flow.tsx (1)
FlowContext(89-104)src/providers/asp.tsx (1)
AspContext(15-20)src/screens/Apps/Escrow/RpcHandler.ts (1)
makeMessageHandler(116-213)src/screens/Apps/Escrow/ArkadeIframeHost.tsx (1)
ArkadeIframeHost(10-83)
src/screens/Apps/Escrow/RpcHandler.ts (1)
src/test/setup.mjs (2)
address(208-208)txid(96-96)
src/screens/Apps/Escrow/ArkadeIframeHost.tsx (1)
src/screens/Apps/Escrow/RpcHandler.ts (1)
MessageHandler(105-105)
🪛 GitHub Check: test
src/screens/Apps/Index.tsx
[warning] 96-96:
'deepLinkInfo' is assigned a value but never used
[warning] 96-96:
'deepLinkInfo' 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). (3)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
- GitHub Check: Cloudflare Pages: wallet-bitcoin
🔇 Additional comments (6)
src/vite-env.d.ts (1)
9-9: LGTM!The environment variable type declaration follows the established pattern and correctly marks the variable as optional.
README.md (1)
37-37: LGTM!The new
VITE_ARK_ESCROW_URLenvironment variable is properly documented.src/screens/Apps/Escrow/RpcHandler.ts (1)
101-116: Clean API design with tagged union Result type.The use of discriminated unions for message types and the tagged
Resulttype for success/failure handling is a solid pattern that enables type-safe message processing.src/icons/Escrow.tsx (1)
1-3: LGTM, but note the architectural inconsistency.The
/escrow.pngasset exists in the public directory. However, this component differs from other icon components in the codebase—all others use inline SVGs (e.g.,AddIcon,BackIcon,HomeIcon), while this uses an external PNG image. This approach may impact consistency and performance, as inline SVGs are bundled with the application while the PNG requires a separate network request.src/providers/wallet.tsx (1)
131-146: Escrow deeplink routing is consistent with existing appsThe new
'escrow'case in the deep-link switch cleanly routes toPages.AppEscrowand matches the pattern used for other apps (Boltz/Lenda*). No issues spotted.src/providers/navigation.tsx (1)
27-76: Navigation wiring forAppEscrowlooks good; just confirm enum renumbering is safe
- Import,
Pages.AppEscrowenum entry,pageTabmapping, andpageComponentswitch case all match the existing pattern for other Apps-tab screens.- Since
Pages.AppEscrowwas inserted in the middle of the enum, the numeric values of followingPagesmembers have shifted. This is harmless as long as you don’t serialize or share the raw numeric enum values outside this module (e.g., in storage or URLs).If you do persist
Pagessomewhere, it’s worth double-checking that you’re storing something stable (like names/strings) rather than the numeric enum value.Also applies to: 103-113
| it('handles fund-address', async () => { | ||
| ;(props.fundAddress as any).mockResolvedValue(undefined) | ||
| const handler = makeMessageHandler(props) | ||
| const res = await handler({ | ||
| kind: 'ARKADE_RPC_REQUEST', | ||
| id, | ||
| method: 'fund-address', | ||
| payload: { address: 'ark1qq', amount: 123 }, | ||
| }) | ||
| expect(props.fundAddress).toHaveBeenCalledWith('ark1qq', 123) | ||
| expect(res.tag).toBe('success') | ||
| if (res.tag === 'success') { | ||
| const out: any = res.result | ||
| expect(out.method).toBe('fund-address') | ||
| expect(out.payload).toEqual({}) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Align fund-address test with RpcHandler implementation (payload currently mismatched)
RpcHandler.ts returns a payload containing { txid } for method: 'fund-address', but the test here:
;(props.fundAddress as any).mockResolvedValue(undefined)
// ...
expect(out.payload).toEqual({})expects an empty payload. These semantics don’t match and will either break the test or obscure regressions.
If you want to keep the { txid } contract (which is more useful to callers), adjust the test like this:
- it('handles fund-address', async () => {
- ;(props.fundAddress as any).mockResolvedValue(undefined)
+ it('handles fund-address', async () => {
+ ;(props.fundAddress as any).mockResolvedValue('txid-123')
...
- expect(res.tag).toBe('success')
- if (res.tag === 'success') {
- const out: any = res.result
- expect(out.method).toBe('fund-address')
- expect(out.payload).toEqual({})
- }
+ expect(res.tag).toBe('success')
+ if (res.tag === 'success') {
+ const out: any = res.result
+ expect(out.method).toBe('fund-address')
+ expect(out.payload).toEqual({ txid: 'txid-123' })
+ }If instead you intend fund-address to be fire-and-forget with an empty payload, then RpcHandler.ts should be changed to return { payload: {} } for that method.
🤖 Prompt for AI Agents
In src/screens/Apps/Escrow/RpcHandler.test.ts around lines 150 to 166, the test
for 'fund-address' currently mocks props.fundAddress to resolve to undefined and
asserts an empty payload, but RpcHandler returns a payload { txid } for that
method; update the test to mock props.fundAddress to resolve to a txid string
(e.g. 'txid123') and assert that the response payload equals { txid: 'txid123' }
and that fundAddress was called with the expected args, so the test matches the
RpcHandler contract (alternatively, if you prefer fire-and-forget semantics
change RpcHandler to return { payload: {} } but keep tests consistent).
| type RpcFundAddressResponse = { | ||
| method: 'fund-address' | ||
| payload: {} | ||
| } |
There was a problem hiding this comment.
Type mismatch: RpcFundAddressResponse payload does not include txid.
The response type declares an empty payload object, but the implementation at lines 197-200 returns { txid }. This causes a type inconsistency.
type RpcFundAddressResponse = {
method: 'fund-address'
- payload: {}
+ payload: {
+ txid: string
+ }
}📝 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.
| type RpcFundAddressResponse = { | |
| method: 'fund-address' | |
| payload: {} | |
| } | |
| type RpcFundAddressResponse = { | |
| method: 'fund-address' | |
| payload: { | |
| txid: string | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/screens/Apps/Escrow/RpcHandler.ts around lines 76 to 79, the
RpcFundAddressResponse type currently defines payload as an empty object but the
implementation returns { txid }; update the type so payload includes txid (e.g.
payload: { txid: string }) to match the returned object, and adjust any callers
or tests that consume RpcFundAddressResponse if they expect a different shape.
| import { base64 } from '@scure/base' | ||
| import makeMessageHandler from './RpcHandler' | ||
|
|
||
| // Needed to sign the message, perhaps to be lifted up int App structure? |
* Fix showing password when server is unreachable * new server down e2e test
…s in restricted environments (#192) * Initial plan * Add JavaScript/JIT capability detection with informative error messages Co-authored-by: tiero <3596602+tiero@users.noreply.github.com> * Format code with prettier Co-authored-by: tiero <3596602+tiero@users.noreply.github.com> * rebuild service worker * refactor how blocking errors are shown * fix tests --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: tiero <3596602+tiero@users.noreply.github.com> Co-authored-by: João Bordalo <bordalix@users.noreply.github.com>
ecf94b7 to
27b5b11
Compare
Deploying wallet-mutinynet with
|
| Latest commit: |
27b5b11
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4f01356c.arkade-wallet.pages.dev |
| Branch Preview URL: | https://integrate-escrow-app.arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
27b5b11
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6212c1d8.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://integrate-escrow-app.wallet-bitcoin.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
README.md (1)
29-37: Remove stray periods after backticked env var names (docs rendering).
This was previously flagged; the periods are still present.-| `VITE_UTXO_MAX_AMOUNT`. | Overwrite the server's utxoMaxAmount | `VITE_UTXO_MAX_AMOUNT=-1` | -| `VITE_UTXO_MIN_AMOUNT`. | Overwrite the server's utxoMinAmount | `VITE_UTXO_MIN_AMOUNT=330` | -| `VITE_VTXO_MAX_AMOUNT`. | Overwrite the server's vtxoMaxAmount | `VITE_VTXO_MAX_AMOUNT=-1` | -| `VITE_VTXO_MIN_AMOUNT`. | Overwrite the server's vtxoMinAmount | `VITE_VTXO_MIN_AMOUNT=330` | +| `VITE_UTXO_MAX_AMOUNT` | Overwrite the server's utxoMaxAmount | `VITE_UTXO_MAX_AMOUNT=-1` | +| `VITE_UTXO_MIN_AMOUNT` | Overwrite the server's utxoMinAmount | `VITE_UTXO_MIN_AMOUNT=330` | +| `VITE_VTXO_MAX_AMOUNT` | Overwrite the server's vtxoMaxAmount | `VITE_VTXO_MAX_AMOUNT=-1` | +| `VITE_VTXO_MIN_AMOUNT` | Overwrite the server's vtxoMinAmount | `VITE_VTXO_MIN_AMOUNT=330` |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/escrow.pngis excluded by!**/*.png
📒 Files selected for processing (11)
README.md(1 hunks)src/icons/Escrow.tsx(1 hunks)src/providers/navigation.tsx(4 hunks)src/providers/options.tsx(1 hunks)src/providers/wallet.tsx(1 hunks)src/screens/Apps/Escrow/ArkadeIframeHost.tsx(1 hunks)src/screens/Apps/Escrow/Index.tsx(1 hunks)src/screens/Apps/Escrow/RpcHandler.test.ts(1 hunks)src/screens/Apps/Escrow/RpcHandler.ts(1 hunks)src/screens/Apps/Index.tsx(3 hunks)src/vite-env.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/providers/navigation.tsx
- src/screens/Apps/Escrow/RpcHandler.test.ts
- src/vite-env.d.ts
- src/icons/Escrow.tsx
- src/screens/Apps/Escrow/RpcHandler.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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.tsx
🧬 Code graph analysis (4)
src/providers/options.tsx (1)
src/icons/Logs.tsx (1)
LogsIcon(1-10)
src/screens/Apps/Escrow/Index.tsx (4)
src/providers/navigation.tsx (1)
NavigationContext(174-178)src/providers/wallet.tsx (1)
WalletContext(42-56)src/screens/Apps/Escrow/RpcHandler.ts (1)
makeMessageHandler(116-213)src/screens/Apps/Escrow/ArkadeIframeHost.tsx (1)
ArkadeIframeHost(10-83)
src/screens/Apps/Escrow/ArkadeIframeHost.tsx (2)
src/screens/Apps/Escrow/RpcHandler.ts (1)
MessageHandler(105-105)src/test/setup.mjs (1)
result(14-14)
src/screens/Apps/Index.tsx (3)
src/providers/flow.tsx (1)
FlowContext(89-104)src/App.tsx (1)
App(36-164)src/icons/Escrow.tsx (1)
EscrowIcon(1-3)
🪛 GitHub Check: test
src/screens/Apps/Index.tsx
[warning] 96-96:
'deepLinkInfo' is assigned a value but never used
[warning] 96-96:
'deepLinkInfo' 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 (3)
src/providers/options.tsx (1)
53-57: EnsureSettingsOptions.Signis fully wired + consider a dedicated icon (notLogsIcon).
IfSettingsOptions.Signis new, confirm enum + settings routing/rendering exists; otherwise tapping this entry will dead-end. Also, reusingLogsIconfor signing is likely confusing—worth adding aSignIcon(or similar) for clarity.src/providers/wallet.tsx (1)
131-146: Deep-link routing forappId: 'escrow'looks consistent.
Matches existingboltz/lendasat/lendaswaphandling and keeps wallet fallback behavior intact.src/screens/Apps/Escrow/Index.tsx (1)
2-5: Verify@noble/secp256k1hash wiring + PSBT signing assumptions.
hashes.sha256 = sha256and the PSBT signing flow (Transaction.fromPSBT(..., { allowUnknown: true }), plus signing checkpoints with[0]) are dependency/API-sensitive—worth confirming with the exact library versions in this repo that this is the intended/required setup and that checkpoint PSBTs always have the expected input structure.Also applies to: 17-19, 56-76
| const poll = useCallback(() => { | ||
| if (iframeRef.current?.contentWindow) { | ||
| iframeRef.current.contentWindow.postMessage({ kind: 'ARKADE_KEEP_ALIVE', timestamp: Date.now() }, childOrigin) | ||
| setTimeout(() => poll(), isAlive ? 5000 : 2000) | ||
| } | ||
| }, [isAlive, iframeRef, childOrigin]) | ||
|
|
There was a problem hiding this comment.
Fix timer/listener leaks and make origin validation strict (===).
As written, polling can continue after unmount, and the load listener can be registered multiple times. Also, origin checking via startsWith is brittle—prefer exact origin matching.
export const ArkadeIframeHost: React.FC<Props> = ({ src, allowedChildOrigins, messageHandler }) => {
const iframeRef = useRef<HTMLIFrameElement | null>(null)
const childOrigin = useMemo(() => new URL(src).origin, [src])
const [isAlive, setIsAlive] = useState(false)
+ const timeoutRef = useRef<number | null>(null)
+ const isAliveRef = useRef(false)
+
+ useEffect(() => {
+ isAliveRef.current = isAlive
+ }, [isAlive])
const poll = useCallback(() => {
if (iframeRef.current?.contentWindow) {
iframeRef.current.contentWindow.postMessage({ kind: 'ARKADE_KEEP_ALIVE', timestamp: Date.now() }, childOrigin)
- setTimeout(() => poll(), isAlive ? 5000 : 2000)
+ if (timeoutRef.current) window.clearTimeout(timeoutRef.current)
+ timeoutRef.current = window.setTimeout(() => poll(), isAliveRef.current ? 5000 : 2000)
}
- }, [isAlive, iframeRef, childOrigin])
+ }, [iframeRef, childOrigin])
useEffect(() => {
const onMessage = async (event: MessageEvent) => {
- if (!allowedChildOrigins.some((_) => _.startsWith(event.origin))) {
+ if (!allowedChildOrigins.some((allowed) => allowed === event.origin)) {
console.error(`[wallet]: ignoring message from ${event.origin}`)
return
}
@@
window.addEventListener('message', onMessage)
return () => {
window.removeEventListener('message', onMessage)
+ if (timeoutRef.current) window.clearTimeout(timeoutRef.current)
}
}, [messageHandler, isAlive, allowedChildOrigins])
- useEffect(() => {
- iframeRef.current?.addEventListener('load', () => {
- poll()
- })
- }, [poll])
+ const onLoad = useCallback(() => poll(), [poll])
@@
<iframe
ref={iframeRef}
title='Ark Escrow'
src={src}
+ onLoad={onLoad}
style={{ width: '100%', height: `100%`, border: 0, display: 'flex' }}
sandbox='allow-scripts allow-forms allow-same-origin'
allow='clipboard-read; clipboard-write'
/>Also applies to: 24-27, 67-71
🤖 Prompt for AI Agents
In src/screens/Apps/Escrow/ArkadeIframeHost.tsx around lines 15-21 (also apply
to 24-27 and 67-71): the poll timer and load event listener leak because
timeouts keep running after unmount and the load handler can be attached
multiple times; change origin checks from startsWith to strict ===, store the
timeout id and clear it on unmount/when polling stops, ensure poll is wrapped to
avoid re-registering (or use a ref to track mounted state) so poll stops when
unmounted, add the load listener only once (register in useEffect with an empty
dependency array or proper deps) and remove that listener in the effect cleanup,
and validate event.origin with === before processing postMessage.
| // Needed to sign the message, perhaps to be lifted up int App structure? | ||
| hashes.sha256 = sha256 | ||
|
|
||
| const BASE_URLS: Record<Network, string | null> = { | ||
| bitcoin: import.meta.env.VITE_ARK_ESCROW_URL ?? 'https://escrow.arkade.sh/client/', | ||
| mutinynet: import.meta.env.VITE_ARK_ESCROW_URL ?? 'https://api.escrow.mutinynet.arkade.sh/client/', | ||
| signet: null, | ||
| regtest: import.meta.env.VITE_ARK_ESCROW_URL ?? 'http://localhost:3002/client', | ||
| testnet: null, | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid stale escrowAppUrl + narrow allowedOrigins to the active iframe origin.
Right now, switching to a network with BASE_URLS[network] === null won’t clear escrowAppUrl, so the iframe may remain mounted with the previous URL. Also, consider only allowing the selected escrow origin rather than all known origins.
useEffect(() => {
- if (!aspInfo.network || !svcWallet) return
- const baseUrl = BASE_URLS[aspInfo.network as Network]
- if (!baseUrl) return // No escrow app for this network
- setEscrowAppUrl(baseUrl)
- }, [aspInfo, svcWallet])
+ if (!aspInfo.network || !svcWallet) {
+ setEscrowAppUrl(null)
+ return
+ }
+ const baseUrl = BASE_URLS[aspInfo.network as Network]
+ // No escrow app for this network
+ setEscrowAppUrl(baseUrl ?? null)
+ }, [aspInfo.network, svcWallet])
@@
- const allowedOrigins = Object.values(BASE_URLS)
- .filter((_) => _ !== null)
- .map((url) => new URL(url).origin)
+ const allowedOrigins = escrowAppUrl ? [new URL(escrowAppUrl).origin] : []
@@
- {escrowAppUrl !== null && (
+ {escrowAppUrl !== null && (
<ArkadeIframeHost src={escrowAppUrl} allowedChildOrigins={allowedOrigins} messageHandler={handlers} />
)}Also applies to: 36-42, 96-105
🤖 Prompt for AI Agents
In src/screens/Apps/Escrow/Index.tsx around lines 17-27 (also apply same changes
at 36-42 and 96-105): the component currently leaves escrowAppUrl set when
switching to a network whose BASE_URLS[network] is null and uses a broad
allowedOrigins list; update the logic so that when BASE_URLS[network] is null
you explicitly clear escrowAppUrl (e.g., set it to null/empty) to unmount the
iframe and prevent using a stale URL, and compute allowedOrigins dynamically
from the single active origin (only include the origin corresponding to the
selected network) instead of allowing all known origins so postMessage listeners
only accept messages from the mounted iframe origin.
| import EscrowIcon from '../../icons/Escrow' | ||
| import LendasatIcon from './Lendasat/LendasatIcon' | ||
| import LendaswapIcon from './Lendaswap/LendaswapIcon' | ||
| import { FlowContext } from '../../providers/flow' | ||
|
|
There was a problem hiding this comment.
Fix unused deepLinkInfo + remove hardcoded enablement (or drop the import).
Right now deepLinkInfo is read but never used, and Escrow is always enabled. Either use the intended gating or remove the dead code. Also prefer omitting link instead of link=''.
export default function Apps() {
- const { deepLinkInfo } = useContext(FlowContext)
- const isEscrowEnabled = true // deepLinkInfo?.appId === 'escrow'
+ const { deepLinkInfo } = useContext(FlowContext)
+ const isEscrowEnabled = deepLinkInfo?.appId === 'escrow'
@@
{isEscrowEnabled ? (
<App
name='Escrow'
icon={<EscrowIcon />}
desc='Escrow system on Ark'
- link=''
+ link={undefined}
page={Pages.AppEscrow}
live
/>
) : null}Also applies to: 95-140
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
The signing and fund-transfer surfaces introduced here accept instructions from an iframe with insufficient validation. Several issues can result in silent fund loss.
[CRITICAL] All network origins are trusted simultaneously in every build
src/screens/Apps/Escrow/Index.tsx:91-94
const allowedOrigins = Object.values(BASE_URLS)
.filter((_) => _ !== null)
.map((url) => new URL(url).origin)BASE_URLS always produces three non-null entries: https://escrow.arkade.sh, https://api.escrow.mutinynet.arkade.sh, and http://localhost:3002. All three end up in allowedChildOrigins regardless of the active network. On a mainnet wallet, any page served from http://localhost:3002 can send RPC messages that are accepted without question—triggering signArkTransaction or fundAddress silently.
Fix: pass only the origin corresponding to aspInfo.network, not all of them.
[CRITICAL] Origin comparison is directionally wrong
src/screens/Apps/Escrow/ArkadeIframeHost.tsx:30
if (!allowedChildOrigins.some((_) => _.startsWith(event.origin))) {This checks whether an allowed origin starts with the event origin, not the other way around. An attacker who controls a domain that is a string-prefix of an allowed domain (e.g. https://escrow.arkad for allowed https://escrow.arkade.sh) would pass the check. Use exact equality:
if (!allowedChildOrigins.includes(event.origin)) {[CRITICAL] signArkTransaction and fundAddress have zero user confirmation
src/screens/Apps/Escrow/Index.tsx:55-78
Both operations move money:
signArkTransactionsigns arbitrary PSBTs (main tx + checkpoints) delivered by the iframe.fundAddresscallssvcWallet.sendBitcoin({ address, amount })with iframe-supplied values.
Neither shows the user a confirmation dialog. A compromised or malicious escrow page (or any page that passes the broken origin check above) can drain the wallet silently. The Lendasat integration has the same gap but it relies on an external SDK with its own origin management. Escrow is rolling its own transport and must gate both operations behind a user-visible approval step before funds move.
[HIGH] Checkpoint always signed at index 0
src/screens/Apps/Escrow/Index.tsx:66
const signed = await svcWallet.identity.sign(
Transaction.fromPSBT(cp, { allowUnknown: true }), [0]
)Every checkpoint PSBT is signed at input index [0] unconditionally. If a checkpoint has wallet inputs at other indices they will be skipped, producing an incomplete signature that silently breaks the escrow protocol. The escrow app should specify which input indices need signing, or the wallet must inspect the PSBT to find its own inputs.
[HIGH] Main transaction signed without index constraint
src/screens/Apps/Escrow/Index.tsx:63
const signedTx = await svcWallet.identity.sign(tx)No input-index whitelist is passed. In a multi-party escrow PSBT this will sign every input that matches the wallet key, including counterparty inputs if the PSBT was crafted to include them.
[MEDIUM] RpcFundAddressResponse type does not include txid
src/screens/Apps/Escrow/RpcHandler.ts:73 (type), line ~187 (impl)
The type declares payload: {} but the implementation returns { kind: 'ARKADE_RPC_RESPONSE', id, method, payload: { txid } }. The escrow app cannot know a txid was returned because it is not typed. The test also asserts expect(out.payload).toEqual({}) using a mock that returns undefined, so the test silently passes with { txid: undefined }. Either surface txid in the type or remove it from the response; don't leave it untyped.
[MEDIUM] Polling creates multiple concurrent poll loops
src/screens/Apps/Escrow/ArkadeIframeHost.tsx:17-22
poll is a useCallback that calls setTimeout(() => poll(), ...). Each time isAlive changes, poll is recreated, but the previous setTimeout callback still holds a reference to the old poll closure and will continue firing, spawning a parallel poll chain. After a few isAlive transitions the page has multiple overlapping timers.
Fix: use a useRef to hold the timeout ID and cancel it on each re-render:
const timerRef = useRef<ReturnType<typeof setTimeout>>()
const poll = useCallback(() => {
clearTimeout(timerRef.current)
if (iframeRef.current?.contentWindow) {
iframeRef.current.contentWindow.postMessage(...)
timerRef.current = setTimeout(poll, isAlive ? 5000 : 2000)
}
}, [isAlive, childOrigin])[MEDIUM] load event listener accumulates without cleanup
src/screens/Apps/Escrow/ArkadeIframeHost.tsx:65-67
useEffect(() => {
iframeRef.current?.addEventListener('load', () => { poll() })
}, [poll])Every time poll changes a new load listener is added with no removal. After several isAlive flips the iframe has multiple load listeners, each starting an independent poll chain on the next navigation.
[LOW] Escrow is unconditionally visible in the Apps list
src/screens/Apps/Index.tsx:109
const isEscrowEnabled = true // deepLinkInfo?.appId === 'escrow'The commented-out gating logic suggests this was supposed to be behind a deep-link flag. As-is, every user on every network sees the Escrow entry. If the escrow service is not ready for all networks this will confuse users trying to use it on signet/testnet where BASE_URLS returns null.
[LOW] sandbox="allow-scripts allow-same-origin" on an env-overridable URL
src/screens/Apps/Escrow/ArkadeIframeHost.tsx:80
allow-same-origin + allow-scripts together in a sandbox removes the sandbox for same-origin frames (browsers treat it as "no sandbox"). src is overridable via VITE_ARK_ESCROW_URL. If a developer sets that to the same origin as the wallet (e.g. http://localhost:5173), the iframe has full DOM access. At minimum document this danger and consider whether allow-same-origin is actually required.
[LOW] package-lock.json added to a pnpm project
package.json declares "pnpm": ">=8" as the required package manager. Adding package-lock.json (npm format) alongside it creates two competing lockfiles. Danger already flagged this. This should be deleted or .gitignored; it was likely generated by accident.
[LOW] useMemo dependency list is stale / incorrect
src/screens/Apps/Escrow/Index.tsx:79-87
const handlers = useMemo(
() => makeMessageHandler({ getXOnlyPublicKey, ... }),
[svcWallet, navigate, setSendInfo],
)setSendInfo is imported from FlowContext but not used by any of the handler functions in this file. The actual closures (getXOnlyPublicKey, signArkTransaction, etc.) are re-created on every render and then captured by useMemo only when svcWallet changes—but since the functions are inline and not useCallback-wrapped, ESLint's exhaustive-deps rule would flag all of them as missing. The memo works coincidentally because svcWallet is the only meaningful dependency, but this is fragile. Use useCallback for each handler or move them outside the component.
Test gaps
RpcHandler.test.ts: no test forfund-addressreturning thetxidin the payload.- No tests for
ArkadeIframeHostorigin validation (the most critical security control). sign-transactiontest does not verify thatsignArkTransactionis called with the decoded payload rather than raw base64.- No test for the case where
getXOnlyPublicKeyreturnsnull.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.