Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
talosmachina
left a comment
There was a problem hiding this comment.
0 findings. Replaces the geo-blocked transfer screens with the Transfer Unavailable sheet (Continue to Manual Setup), re-checks the geo state before a swipe or hardware confirm creates or reuses a Blocktank order, and keeps saving the transfer record of an order that was already paid. Reviewed 44ad53b, full tier, reasoned from the code: iOS does not build on this reviewer's Linux host, so nothing was compiled or run here.
Merge confidence: 4/5. Unit tests cover the swipe and the paid-record paths, but Run Tests, Run Integration Tests and build-local were still pending at review time, and the sheet flows rest on the author's recordings and J1.
What I checked, and 5 candidates I ruled out
Read in full: TransferViewModel.swift (order selection, hardware confirm, broadcast retry, failure handling), TransferService.swift, GeoService.swift, TransferUnavailableSheet.swift, SheetIntro.swift, FundingOptions.swift, the three wallet screens, ReceiveCjitGeoBlocked.swift.
Entry points traced: every navigation to .spendingIntro, .spendingAmount, .spendingIntroHw and .spendingAmountHw (Savings, Spending and Hardware wallet screens, FundingOptions, SpendingIntroView). Suggestions and LightningConnectionsView reach these only through .fundingOptions, which is gated. Both blocktank.createOrder callers (SpendingConfirm, SpendingHwSign) go through currentOrder, which now runs ensureTransferAvailable before reusing or creating an order.
createTransfer callers: fundPaidOrder and restoreAcceptedTransfer always pass a funding txid, so the narrowed geo guard lets recovery of a paid order through. The manual-channel and close paths pass no lspOrderId and never hit the guard.
Checks run here: node scripts/validate-translations.js (0 errors), git diff --check (clean). CI: SwiftFormat, validate and detect-changes green; tests and build pending.
Ruled out
- Hardware retry stuck while geo-blocked: a pending signed broadcast now throws at
ensureTransferAvailableand leaves the Sign screen's buttons disabled. This is the stated intent of44ad53b, since the retry would pay an unpaid order. The only case that loses anything is an uncertain broadcast that had already reached the network before the switch, which needs a signing timeout to straddle the sunset. - Geocheck failure lets a swipe through:
checkGeoStatusfalls back to not blocked on a network error, so the swipe goes on tocreateOrder, which Blocktank refuses server-side. This is the existing fallback, not new behaviour. - E2E builds:
GEO=falseand noCHECK_GEOBLOCKleaveisTransferUnavailablefalse. The test-only initializers default to{ false }, so existing transfer tests are unaffected. - Sheet over an active sheet: every
showSheet(.transferUnavailable)caller is a navigation screen, never inside another sheet. Receive rendersTransferUnavailableContentinline. - Spending wallet button now shown while blocked: it opens the sheet, which is the intent.
Reviewer model: claude-opus-5-5
Reasoning effort: Unknown (the review run's accessible metadata does not record it)
|
Pushed 89c527f, following Aldert's updated Blocktank-sunsetting frames in Figma (links in the description):
Design comparisons and new recordings are in the description. |
|
Pushed 2061c33: Learn More, on the sheet and on the Funding screen, now opens https://blog.bitkit.to/, where the sunset article appears once it is published, so the link needs no further change. The recordings show the earlier bitkit.to target. |
9d5e276 to
1883357
Compare
|
Pushed 8a79eb0:
|
|
Pushed 7d93fb2: Blocktank's nodes are no longer special.
The PR body lists the side effects checked. |
|
QA: ✅ Transfer Unavailable sheetSavings › Transfer To Spending opens the sheet. Learn More opens blog.bitkit.to and Continue opens Manual Setup.
✅ Manual setup still worksA channel to the staging Blocktank node opens with no service fee, confirms, and routes Lightning payments both ways between the two phones.
✅ Receive with a channelReceive › Spending shows a normal invoice. The content for a wallet with no inbound capacity wasn't reachable with channels open, so it isn't covered.
✅ Notifications for PaykitSettings › Notifications shows the Paykit copy with the "Open Bitkit to review payment" preview. On a fresh install, accepting a subscription asks for notification permission. With the app open, the subscription-due reminder shows the in-app sheet, plus a duplicate banner tracked in #1136.
✅ AllowanceA request from the Pixel within this phone's $5 cap is paid automatically. One over it (10,000 sats, about $8) asks for approval, then pays. ✅ Transfer to Savings with BlocktankThe cooperative close failed because LND's closing fee (141 sats) was below LDK's minimum (182), so it fell back to a force close. ldk-node never broadcast that close, because the config trusted Blocktank's node to broadcast it, and 87,020 sats stayed claimable. Without the trusted peers the close broadcast on first launch and 86,896 sats came back after the timelock. That fix is in this PR.
|
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Reviewed the full PR diff against its merge base, at 7d93fb2f.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
The unavailable sheet, disabled funding rows, paid-order recovery, local force-close broadcast, and Paykit notification prompts match the change as written. Compared with bitkit-android#1696 at 9e10dc31, those flows agree. That twin's on-chain recovery still pays a created Blocktank order as well; this was a comparison of the shared flow, not a review of that PR.
Run Tests passed on this commit. The new swipe and paid-record tests do not cover a retained unpaid funding attempt. This review did not run tests or a device.
Device testing: not performed in this review.
Suggested additional test cases:
- iOS. Setup: debug regtest build so the geocheck returns 403, with a node-wallet Blocktank funding attempt left pending, rejected, or unknown, and that order still created and unexpired. Action: open Send, or the pending payment, and confirm Retry original. Expected: no transaction is broadcast and the order stays unpaid.
Findings
- [MEDIUM] Unpaid Blocktank funding can still be retried — inline at
Bitkit/ViewModels/TransferViewModel.swift:421.
Reviewer model: grok-4.7
Reasoning effort: high
|
Pushed 77b01a4: Retry original on a pending, rejected or unknown Blocktank funding attempt now stops at the geocheck. Unless it says allowed, the Transfer Unavailable sheet opens and nothing is broadcast. This answers the unpaid-order retry finding in #1127 (comment). |









Twin: synonymdev/bitkit-android#1696
This PR replaces the "not available in your country" screens with the Blocktank-sunset design, which sends users to Manual Setup, and stops the transfer confirm from paying a Blocktank order now that Blocktank no longer sells channels.
Description
Blocktank stopped selling Lightning channels on 26 Oct. Its geocheck now answers 403 for everyone, so the app treats every user as geo-blocked.
trustedPeers0conf,trustedPeersNoReserve), so the wallet broadcasts and fee-bumps its own force close with Blocktank. Before, LDK left that broadcast to Blocktank, whose node no longer acts, so a force-closed Blocktank channel kept its funds stuck in "claimable on channel close". The hardcoded Blocktank node list (Env.trustedLnPeers), the Blocktankinfopeer fetch and the trusted-peer section of the network-graph dump are gone.lightning__force_channels_skipped, removed in every locale). The RN migration's peer recovery no longer filters the Blocktank nodes out.Learn More opens https://blog.bitkit.to/, where the announcement post appears once it is published.
Out of Scope
Views/Transfer,TransferViewModel,BlocktankService: deleting the purchase screens and the order and CJIT code.Design
Bitkit Wallet › Bitkit - Final Review v63 › Blocktank sunsetting:
Preview
Figma (left) next to the app (right):
Each entry point on a geo-blocked regtest wallet (2x):
savings-sheet.mp4
funding-screen.mp4
receive-screen.mp4
Notifications, repurposed for Paykit, on c124633:
QA Notes
Setup
./lsp POST /regtest/chain/deposit '{"address":"<savings address>","amountSat":100000}', then./lsp POST /regtest/chain/mine '{"count":3}'from a bitkit-android checkout.Journeys
transfer-unavailable-sheet.xml— the wallet screens' Transfer To Spending buttons open the sheet, Learn More opens bitkit.to, Continue opens Manual Setup, the Funding screen leads with Manual Setup with the other options disabled, and Receive's Lightning screen shows the sheet's contentManual Tests
Automated Checks
TransferViewModelTests.swift— a geo-blocked swipe pays no held order and creates no new one, and opens the sheetTransferServiceActivityTests.swift— a paid order still saves its transfer record while geo-blockedLightningServiceConfigTests.swift— the node config trusts no peer for zero-conf or force-close broadcasts; only stored peers with no channel and no claimable balance count as idle, and none while a sweep is pending. The removed startup connect is covered by deleting the function, not by a test.ChannelPurchaseFlow.swift(integration) — connects to the LSP node from Blocktank info instead of the removed trusted-peer connectunit-tests.ymlat 7d93fb2 on an iPhone 17 simulator (iOS 26.5) — pass, excepttestCustomFeeWalletSwitchSurvivesFeeScreenNavigation, the known flake fix: prepare the wallet switch once on send review #1098 fixesunit-tests.ymlon an iPhone 17 simulator (iOS 26.5) — pass, excepttestCustomFeeWalletSwitchSurvivesFeeScreenNavigationinSendConfirmationLifecycleTests.swift, a known flake that fix: prepare the wallet switch once on send review #1098 fixesSide effects checked
perChannelReserveSatsstays 1, so each open Blocktank channel now holds 1 sat of on-chain reserve. Spendable balance is effectively unchanged.trustedPeers0confonly gates accepting new unconfirmed inbound channels, so channels already open are unaffected.disconnectremoves the peer from the store.Evidence
J1 ran on 55b8a47, a Debug build on an iPhone 17 simulator (iOS 26.5) against the staging regtest LSP, with a fresh wallet funded with 100 000 sats on-chain. 89c527f only renames the Funding screen's title to "Spending Balance".
Gaps
Models used
claude-opus-5-5(reasoning:Not exposed)claude-opus-5-5(reasoning:Not exposed)claude-sonnet-5-5(reasoning:Not exposed)Metadata note: the session records name the models but do not expose their reasoning settings.