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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a53e17f8f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
 [Medium impact] The changes affect the Lightning transfer and receiving paths in the Android app. Fix the hardware retry bypass before merging so unsent funding cannot pay an unavailable Blocktank order. Findings
SummaryPoints blocked transfer entry points to Manual Setup through a new Transfer Unavailable sheet.
Acknowledged by Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Transfer entry point] --> B{Geographically blocked?}
B -->|Yes| C[Transfer Unavailable]
C --> D[Manual Setup]
B -->|No| E[Existing transfer flow]
E --> F[Confirm]
F --> G{Availability check}
G -->|Unavailable| C
G -->|Available| H[Fund order]
F --> I[Retained hardware transaction]
I -->|Retry skips check| H
Reviews (1) · Last reviewed commit: "fix: show the sunset sheet in every lang..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Replaces the geo-blocked copy and the hidden Transfer To Spending button with a Transfer Unavailable sheet that routes to Manual Setup, and re-checks the geo state in TransferViewModel before any Blocktank order is created or a held one is paid. Reviewed 5bc8c29, full tier, reasoned from the code and CI (the Bitkit Android branch is not built on this reviewer's machine).
What I checked, and 7 candidates I ruled out
Read in full: TransferUnavailableSheet.kt, LocationBlockScreen.kt, FundingScreen.kt, SheetIntro.kt, the four entry points and the effect collector in ContentView.kt, TransferViewModel from onTransferToSpendingConfirm through isTransferUnavailable and onTransferToSpendingHwConfirm, LightningRepo.start/stop/updateGeoBlockState, CoreService.isGeoBlocked, the new tests in TransferViewModelTest, LightningRepoTest, FundingScreenTest.
Call sites traced: blocktankRepo.createOrder (only getOrCreateSpendingOrder), navigateToTransferSpendingStart (all four now gated), ReceiveRoute.GeoBlock (CJIT button and ReceiveAdditionalLiquidityAction.GeoBlocked), R.string.lightning__funding__text_blocked* (no references left after the removal in every locale).
Strings: the five new keys are present in all 16 strings.xml.
CI: build (assemble + testDevDebugUnitTest), lint, detekt green at 5bc8c29; the e2e-tests-local suites were still running.
Ruled out
- Held order paid after the switch:
paySpendingConfirmOrderre-checks after the retained-attempt and paid-order branches, so an accepted broadcast still completes and a fresh pay stops; covered by the newTransferViewModelTestcase. - HW passphrase prompt before the sheet: the check now runs first, and
onHwPassphraseSubmitre-entersonTransferToSpendingHwConfirm, which re-checks;isBroadcastRetrykeeps a signed transfer's rebroadcast open. - Stale
isGeoBlockedat cold start opens the purchase flow: possible for the length of the geocheck request, but the confirm and HW confirm both refresh it beforecreateOrder, so no order is created. - Effect lost:
transferEffectshas no replay, but theRootNavHostcollector lives for the whole nav host, soOnTransferUnavailablealways has a subscriber when the confirm screen is up. - Geo state dropped on stop: both
stopbranches now carryisGeoBlockedover;wipeStoragestill resets it, and the nextstartrefreshes it. - Geocheck fails open (network error or non-403 returns false): unchanged by this PR, and Blocktank refusing the order is the backstop.
- Onboarding slide 1 still says "not currently provide Lightning services in your country" when geo-blocked: real copy left behind, but
onboarding__slide1_noteis untouched by this diff, so it is not this PR's defect.
Merge confidence: 4/5, the e2e suites had not finished at review time and the four ContentView entry-point branches are covered only by journey J1, not by a test.
Reviewer model: claude-opus-5-5
Reasoning effort: Unknown (not recorded in metadata this run can read)
|
Pushed 1fd64df:
|
|
Pushed 23dab9a, following Aldert's updated Blocktank-sunsetting frames in Figma (links in the description):
Design comparisons and new recordings are in the description. |
|
Pushed 9af8509: 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. |
a1412d5 to
a1c333c
Compare
|
Tested manually before #1696 (comment), will retest. |
|
Rebased onto master after #1699. Background Payments is repurposed for Paykit notifications rather than dropped (head f8641f4):
|
piotr-iohk
left a comment
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 a1c333c0. No earlier assessment baseline was available.
2 actionable findings — resolve or provide an evidence-backed rebuttal.
A failed or non-403 geocheck on confirm still pays a held Blocktank order: the new availability check treats that result as not blocked, then broadcasts. Removing Background Payments also leaves Keep Bitkit active in background stuck on for wallets that had already enabled it.
Compared with bitkit-ios#1127 at 1883357a for this flow only. The confirm guard has the same fail-open geocheck. iOS still exposes a notification toggle after dropping Blocktank registration. Earlier comments on this PR about a missing changelog fragment and a hardware broadcast retry skipping the check do not apply at this revision: the fragment is present, and the availability check runs before that retry.
Validation: this review did not execute unit tests or a device. The new TransferViewModelTest case was inspected; it stubs an already-blocked state and does not run a failed geocheck. Author-reported just compile, just test, and just lint on this head were not re-run here. Device testing: not performed in this review. Journey J1 evidence in the PR body is from 23dab9a, except the Settings row check noted for a1c333c.
Suggested additional test cases
- Android, dev build, geoblocking on, spending confirm already holding a Blocktank order and no broadcast. Block
https://api1.blocktank.to/api/geocheckor make it return a non-403 error, then swipe to transfer. Expected: the Transfer Unavailable sheet opens and no on-chain transaction is broadcast. - Android, a wallet that already has Keep Bitkit active in background enabled, updated to this build. Background the app. Expected: the persistent node notification stops, or Settings still offers a way to turn that behavior off.
Findings
- [MEDIUM] A failed geocheck still pays a held Blocktank order — inline at
app/src/main/java/to/bitkit/viewmodels/TransferViewModel.kt:485. - [LOW] Keep Bitkit active can no longer be turned off — inline at
app/src/main/java/to/bitkit/ui/settings/SettingsScreen.kt:370.
Reviewer model: grok-4.7
Reasoning effort: high
@piotr-iohk thanks, both findings are fixed. Since a1c333c (head 64e3637):
Could you re-check on the new head? |
|
Pushed 2dfbc86: the notification shown while "Keep Bitkit active in background" is on now reads "Bitkit is running in the background so you can receive payments and requests", translated in every locale. It previously named only Lightning payments, which no longer fits now that the setting covers Paykit payments and requests. |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review at 2dfbc86b of the changes since a1c333c0, using the inherited baseline review. This pass re-reviewed confirm-time geocheck failures, the Paykit repurposing of background payments, the string and unit-test updates in that delta, and the matching iOS behavior at c124633c. Inherited coverage: the Transfer Unavailable sheet with Learn More and Continue into Manual Setup, geo-blocked savings, spending, hardware, and funding entry points, Receive lightning location-block content, and geo refresh at node start with preservation when the node stops.
3 actionable findings — resolve or provide an evidence-backed rebuttal.
A failed or unexpected confirm-time geocheck is written into the shared geo flag, so savings, funding, and Receive lightning stay on the sunset path until the next geo refresh. Accepting a due subscription, or an allowance, can leave the sheet before the notification permission result is stored, so reminders and the keep-active service still see notifications as off. docs/timed-sheets.md still describes a Lightning-balance prompt; the sheet requires a Paykit subscription.
Validation was source inspection of 2dfbc86b. Unit tests, journeys, and device runs were not executed. TransferViewModelTest mocks isTransferBlocked() and does not cover a failed geocheck's effect on lightningState. iOS c124633c was compared only for this follow-up's geocheck and notification paths. Device testing: not performed in this review.
Suggested additional test cases
- Android, geoblocking enabled, geocheck forced to time out or return a non-403 status, spending confirm open for an allowed region. Swipe confirm. The Transfer Unavailable sheet opens and nothing is paid; after dismiss, Savings Transfer To Spending opens the transfer flow again without an app restart.
- Android 13+, notifications not granted, subscription accept starts a payment. Tap subscribe, leave the system permission dialog up until Send replaces the sheet, then tap Allow. Settings shows notifications on, and subscription reminders are scheduled, without opening the notifications settings screen.
- Android, notifications off, Paykit subscription present, Lightning balance zero. Open Home after the ask interval. The notification prompt appears. Repeat with a Lightning balance and no subscription: the prompt stays hidden.
Earlier findings reconciled at this revision: 4 verified fixed.
Findings
- [MEDIUM] Failed confirm geocheck sticks as geo-blocked — inline at
app/src/main/java/to/bitkit/repositories/LightningRepo.kt:675. - [MEDIUM] Subscription permission grant is dropped when the sheet closes — inline at
app/src/main/java/to/bitkit/ui/screens/subscriptions/SubscriptionsScreen.kt:739. - [LOW] Timed-sheet doc still requires a Lightning balance — inline at
app/src/main/java/to/bitkit/utils/timedsheets/sheets/NotificationsTimedSheet.kt:25.
Reviewer model: grok-4.7
Reasoning effort: high
Blocktank stopped selling channels for good, so there is no allowed region anymore. The first half holds (the sheet opens, nothing is paid); staying on the sunset path afterwards is intended. Pushed 1cda841:
|
|
Pushed 9e10dc3: 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. Accepting a subscription with notifications revoked asks for permission. The subscription-due reminder arrives in the background, and in the foreground the in-app sheet shows instead. The "Keep Bitkit active" notification shows on device; this capture predates its new copy.
✅ AllowanceA request from this phone within the iPhone'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 199,056 sats stayed claimable. Without the trusted peers the close broadcast on first launch and 197,998 sats came back after the timelock. That fix is in this PR.
|
Twin: synonymdev/bitkit-ios#1127
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, theinfo.nodesfetch and the "trusted peers" network-graph checks are gone.lightning__force_channels_skipped, removed in every locale). Node Info no longer names the Blocktank nodes from the hardcoded list, and the RN migration's peer recovery no longer filters them out.Learn More opens https://blog.bitkit.to/, where the announcement post appears once it is published.
Out of Scope
ui/screens/transfer,TransferViewModel,BlocktankRepo: 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
spending-sheet.mp4
funding-screen.mp4
receive-screen.mp4
Notifications, repurposed for Paykit, on c5f14db:
QA Notes
Setup
just build, regtest, staging LSP). Geoblocking is on by default and Blocktank's geocheck answers 403, so the app is geo-blocked with no extra setup../lsp POST /regtest/chain/deposit '{"address":"<savings address>","amountSat":100000}', then./lsp POST /regtest/chain/mine '{"count":3}'.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
NotificationsTimedSheetTest.kt— the prompt depends on Paykit subscriptionsWakeNodeWorkerTest.ktand the push-registration case inLightningRepoTest.kt— the wake worker and the registration are goneTransferViewModelTest.kt— a geo-blocked confirm swipe opens the sheet and sends nothing on-chainLightningRepoTest.kt— the geo state updates before the node is upFundingScreenTest.kt— while geo-blocked only Manual Setup respondsLightningServiceTest.kt— 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 pendingLightningRepoTest.kt— start disconnects idle peers and connects to no fixed peer; removed the cases for fetching Blocktank peers into the node setupjust compile,just test,just lintandcompileDevDebugAndroidTestKotlin— passSide 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 23dab9a, a dev build on a Pixel 9 emulator (Android 15) against the staging regtest LSP, with a fresh wallet funded with 100 000 sats on-chain:
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.