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. |
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d14b4c22a9
ℹ️ 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".
| `pubkyauth://signup_grant` URL naming that homeserver; invites and authorization URLs are secrets. | ||
| - Record the app commit, APK hash and installed package. Android uses `devDebug`, `to.bitkit.dev`, | ||
| Paykit `0.1.0-rc72` and native LDK `0.7.0-rc.72`. Use a normal build (`E2E=false`) or explicitly | ||
| Paykit `0.1.0-rc72` and native LDK `0.7.0-rc.74`. Use a normal build (`E2E=false`) or explicitly |
There was a problem hiding this comment.
Add a journey that exercises the HTLC claim
Changing only this version pin in the Paykit fallback does not exercise a force-close or an on-chain HTLC claim, so the listed journey cannot catch a regression in the behavior this dependency bump is meant to fix. The journey environment already provides Lightning channels and regtest block mining, so this scenario should be covered by an added or updated journey rather than only the manual-test list.
AGENTS.md reference: AGENTS.md:L109-L113
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The fix is a version pin; the behavior it restores is covered in synonymdev/ldk-node#125 by repro and integration tests that fail on 0.7.0-rc.72. A device journey would need an HTLC kept pending across a force-close and then mined past its expiry: the Capabilities table in journeys/README.md lists LSP force-close but no way to hold an HTLC (the LSP regtest API has no hold invoice), so per AGENTS.md this stays a manual test, now worded with the missing capability.
|
Pushed 8c26687: the changelog fragment now names only the force-close claim fix, since Bitkit does not start splices; the description and manual tests match. Answers the Codex threads on the fragment and the journey. |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Bumps ldk-node-android from 0.7.0-rc.72 to 0.7.0-rc.74 for the PSBT signing fix in synonymdev/ldk-node#125, plus the changelog fragment and the journey's version line. Reviewed 8c26687, full tier (dependency manifest).
What I checked, and 2 candidates I ruled out
Pin: tag v0.7.0-rc.74 in synonymdev/ldk-node resolves to 190a7d98b24aa0f4b337b21d9cee6fb8b2986679.
Upstream delta rc.72 to rc.74: 6 commits, d1bd9fc...190a7d9, touching crates/bdk-wallet-aggregate (signing), src/wallet/mod.rs, src/event.rs, version files and tests. No .udl or binding change, so the Kotlin and Swift API the app calls is unchanged.
Other references to rc.72: none left in the tree (grepped the branch head).
Ruled out
- API break in the bump: no UniFFI interface file changed between the two tags.
sign_owned_inputssignature change: it ispub(crate)inside ldk-node and gains aknown_prevoutsargument there only; nothing in the bindings exposes it.
CI: build, lint and detekt green; build-local still pending when reviewed. The build job resolved com.synonym:ldk-node-android:0.7.0-rc.74 from the registry.
Not built here: Bitkit Android branches are not built on this reviewer's box; this review is reasoned from the diff, the upstream compare and CI.
Merge confidence: 4/5, a dependency bump whose fixed path (on-chain HTLC claim after a force-close) has no test in this repo and rests on the manual test in the PR body; the rc.74 tag also sits on the head of synonymdev/ldk-node#125, which is still open.
|
Replaced by #1480: same change, on a branch named after the force-close claim fix. |
Twin: synonymdev/bitkit-ios#929
Refs: synonymdev/ldk-node#125
Description
0.7.0-rc.72to0.7.0-rc.74, so force-closed channels can claim their pending payments (HTLCs) on-chain again.0.7.0-rc.73and0.7.0-rc.74come from fix: let force-closed channels claim pending payments on-chain ldk-node#125 on top of0.7.0-rc.72(release).Design
N/A — no UI changes.
Preview
N/A — no user-visible changes.
QA Notes
Journeys
local-paykit-fallback.mdManual Tests
0.7.0-rc.72with an existing wallet and open channels → the node starts, channels reconnect, balances match — needs a wallet written by the previous build.Automated Checks
com.synonym:ldk-node-android:0.7.0-rc.74is published to the ldk-node Maven registry.