Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
 |
![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). |
CI:
|
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Settled the Checked the spec against the branch. The step order is receive unconfirmed, CPFP boost, restore from seed, reinstall, The e2e PR adds
|
|
Correction on the e2e link: the companion PR is now synonymdev/bitkit-e2e-tests#231. Same commit, on a branch named |
|
CI is green on 24ab459. The rerun resolved the e2e branch to Merge order: #231 first (or together), otherwise this PR is green only while the matching e2e branch exists. |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no HIGH/MEDIUM findings at head 24ab459b9. (Posted as a comment because GitHub does not allow approving your own PR.)
Inline: LOW — Device clock off by more than an hour keeps every confirmed-only receive silent
Checked: Traced both entry paths at the PR head: LightningRepo invokes the service handler sequentially and only then emits to AppViewModel, so in the background the service claims first and posts one notification while AppViewModel's later call hits isPresentationClaimed and skips; in the foreground the service's canPresent (App.currentActivity == null) fails and AppViewModel claims, so one sheet. Dedupe across process death does not depend on the in-memory presentationClaims set: markOnchainActivityAsSeen writes seenAt into the Core activity DB via present -> recordPresentation, and CoreService.shouldShowReceivedSheet returns false on seenAt != null, so a mempool-presented tx that confirms after a restart (foreground or background path) resolves to Skip; ldk-node calls eventHandled before onEvent so events are not replayed either. Sends, CPFP children and channel-funding/order payments carry a negative net amount_sats (ldk-node get_tx_details = received - sent) and exit on amountSats <= 0 before any activity write; coop-close and sweep transactions run the same findClosedChannelForTransaction / isTransfer / channelId checks as the existing mempool path, with lightningService.balances read live from node.listBalances(). ldk-node rc.66 updates the payment store before returning wallet events, so handleOnchainTransactionConfirmed finds the payment and creates the activity before shouldShowReceivedSheet runs. Restore, migration and the one-hour block-timestamp guard are evaluated before any state is written, and the greptile thread on the duplicate confirmed update is resolved by applyConfirmationIfMissing. iOS twin synonymdev/bitkit-ios#588 is open.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no HIGH/MEDIUM findings at head 2a6b9dd68. (Posted as a comment because GitHub does not allow approving your own PR.)
Inline: LOW — grep -A3 "pkg=to.bitkit.dev" never prints the notification title the next step verifies; LOW — Cross-platform row says not ported, but the iOS twin fixes the same behaviour without the journeys
Checked: Round 3 (2a6b9dd, new journeys): Round 3 delta is journeys only (three onchain-receive journeys, the suite README and two rows in journeys/README.md). Every testTag the journeys name exists at head: Receive (TabBar.kt:156), ReceiveScreen (ReceiveSheet.kt:144), ShowDetails and ReceiveOnchainAddress (ReceiveQrScreen.kt:427/658), ReceivedTransaction and ReceivedTransactionButton (NewTransactionSheet.kt:161/194), MoneyText (Money.kt). The preconditions are reachable with repo tooling: ./lsp at the root wraps the blocktank-api script, deposit returns the txid the log grep needs, Env.walletSyncIntervalSecs is 10, and LightningService logs every event as LDK event fired: <json> at debug level under the APP tag with the event class name as type. The staging regtest tip timestamp was checked against the wall clock (about 5 minutes apart), so the one-hour MAX_CONFIRMED_ONLY_AGE guard does not make the confirmed-only journeys false-fail today. The background flow matches the code: App.currentActivity clears on stop, LightningNodeService claims and posts Payment Received / Received %s and caches the sheet, AppViewModel's later call skips on the claim, and ContentView ON_START calls consumePaymentReceivedInBackground, so tapping the auto-cancel notification opens the received sheet once. The mempool-then-confirmed journey would catch a second sheet if the txid claim or seenAt dedupe regressed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no blocking HIGH/MEDIUM findings at head c67782eb6. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Round 4 (e2be5d9, c67782e): both journey LOWs are fixed. Round 4 covers two journey-only commits that answer my earlier LOWs. e2be5d9: the new grep matches what AOSP NotificationRecord prints inside the extras block under --noredact (android.title=String (Payment Received), android.text=String (Received ...)). The receive notification is posted through Context.pushNotification (Notifications.kt:84-86) with plain Strings from ReceivedNotificationContent.build (R.string.notification__received__title = "Payment Received", notification__received__body_amount = "Received %s"), so the extras class is String and both patterns hit. The foreground-service notification uses app_name ("Bitkit Regtest" on dev, build.gradle.kts:205) and notification__service__body, so it matches neither pattern and the title count is exact. c67782e: the cross-platform row now says the port is pending in synonymdev/bitkit-ios#588 and records both adaptations (restore-flag replay guard instead of the one-hour window, no foreground-service counterpart for the background notification journey), which is what I asked for. No new findings.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ✅ Approve
Review: diff 13 files.
Findings:
N/A
Audit:
Audited - no findings.
Coverage:
QA: journeys and manual tests await green CI checks
Reviewed by claude-opus-5-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)
|
@jvsena42 ci failed |
|
Looked at it: infra, not the change. The That shard has failed the same way on several PRs in this batch this afternoon. Re-running the failed jobs now. The real e2e issue this PR did cause is already handled separately in synonymdev/bitkit-e2e-tests#231, which dismisses the received sheet in |
2 participants
Fixes #797
Related to #1275
E2E companion: synonymdev/bitkit-e2e-tests#231 (same branch name, so CI here runs against it)
This PR shows the received sheet and notification for onchain deposits that the wallet first sees already confirmed, without a prior mempool event.
Description
Out of Scope
NotifyPaymentReceivedHandler: a confirmed-only deposit found more than one hour after its block (for example, the app was killed while it confirmed and reopened later) stays silent, while a deposit still in the mempool at reopen always shows. A replay discriminator that does not depend on recency, such as a persisted flag set after the first post-restore full scan, is left for a follow-up.AppViewModel: other receive side effects keyed to the mempool event, such as closing an open Receive sheet and the Paykit contact attachment and private address rotation, still run only onOnchainTransactionReceived. On device the Savings address did rotate after a confirmed-only receive.OnchainTransactionReceived.LightningNodeService: after a process restart, a recent confirmation of a tx already seen through its mempool event still runs the handler's short retry delay (about 1.4 s) before skipping.@boost_1 - Can do CPFPin the local e2e suite restores from seed and then mines, so the boosted receive becomes a confirmed-only receive and the sheet coversHeaderMenu. The sheet is correct there, so the spec dismisses it: test: dismiss confirmed-only receive sheet in boost_1 bitkit-e2e-tests#231.NotifyPaymentReceivedHandler: the one-hour window is measured against the device clock, so a device whose clock is off by more than an hour never shows a confirmed-only receive, and ldk-node emitsOnchainTransactionConfirmedonce, so it cannot be recovered later. The mempool path is unaffected. A clock-independent reference, such as the timestamp of the tip block from the sync that produced the event, is left for the same follow-up as the replay discriminator above.Design
Reuses the existing received sheet with no visual change: Payment received event
Preview
The wallet's primary display is USD, so fiat leads on device; the button label is chosen at random. Both are existing behaviour.
Deposit and mine in one command while on Home; the received sheet appears after the next sync:
confirmed-only-foreground.mp4
QA Notes
Manual Tests
OnchainTransactionReceivedfor the txid.regression:Home → deposit to a Savings address without mining: received sheet shows; dismiss it.regression:Restore a wallet with old onchain deposits: no received sheets or notifications for them.The device run used the
journeys/onchain-receivesuite, committed in this PR atjourneys/onchain-receive/(confirmed-only-received-sheet.xml,mempool-then-confirmed-single-sheet.xml,confirmed-only-background-notification.xml, plus thesuite
README.mdand thejourneys/README.mdrows). The same diff inline:journeys/onchain-receive
Automated Checks
NotifyPaymentReceivedHandlerTest.ktcovers a recent confirmed-only receive showing the sheet and the notification, timestamps inside and outside the one-hour window in both directions, received-then-confirmed showing once, negative amounts, the restore and migration skips, and the confirmed activity update being applied once and skipped when it is already stored.NotifyPaymentReceivedHandlerTest.ktmoves the existing onchain cases to the new handler dependencies.LightningNodeServiceTest.ktcovers a background confirmed event reaching the handler and posting exactly one notification, and a handler skip posting none.AppViewModelSendFlowTest.ktcovers the confirmed event updating the activity and then showing the sheet, and no handler call during a migration.just compile,just test,just lint.