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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0186e0500c
ℹ️ 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, built app executable hash and installed bundle identifier. iOS uses | ||
| `Debug`, `to.bitkit`, Paykit `0.1.0-rc72` and native LDK `0.7.0-rc.72`. Use normal Debug with | ||
| `Debug`, `to.bitkit`, Paykit `0.1.0-rc72` and native LDK `0.7.0-rc.74`. Use normal Debug with |
There was a problem hiding this comment.
Add a journey that exercises the force-close recovery
This is the only journey-related edit, but it merely updates the LDK version in an unrelated Pubky marketplace setup and does not exercise a force-closed channel with a pending HTLC or verify its eventual on-chain claim. Because the journey environment already provides Lightning channels plus regtest funds and block mining, leaving this solely as a manual test does not satisfy the repository's QA contract for this user-visible recovery fix; add or update a journey that proves the behavior.
AGENTS.md reference: AGENTS.md:L387-L391
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 abf9f44: 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 the ldk-node Swift package from 0.7.0-rc.72 to 0.7.0-rc.74 (exact version in the project, pin in Package.resolved) for the PSBT signing fix in synonymdev/ldk-node#125, plus the changelog fragment and the journey's version line. Reviewed abf9f44, 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, the revision Package.resolved pins; Package.swift at that commit points at the release's LDKNodeFFI.xcframework.zip with checksum ef0b7c49..., matching the asset's sha256 digest on the release.
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: validate green; Run Tests, Run Integration Tests and build-local still pending when reviewed.
Not built here: iOS does not build on Linux; this review is reasoned from the diff, the upstream compare and the release artifacts.
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 the test jobs were still running; the rc.74 tag also sits on the head of synonymdev/ldk-node#125, which is still open.
|
Replaced by #930: same change, on a branch named after the force-close claim fix. |
Twin: synonymdev/bitkit-android#1479
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
xcodebuild -resolvePackageDependencies: resolvesldk-nodeat0.7.0-rc.74;Package.resolvedpins the release tag's commit.