Skip to content

Prepare 0.8.0-rc2 with USDT recovery APIs - #168

Merged
ben-kaufman merged 2 commits into
masterfrom
chore/release-0.8.0-rc2
Oct 8, 2026
Merged

ben-kaufman merged 2 commits into
masterfrom
chore/release-0.8.0-rc2

Conversation

@ben-kaufman

Copy link
Copy Markdown
Collaborator

Prepares 0.8.0-rc2 from master, including the merged Paykit proof and Orchestra APIs and the final bridge delivery/refund validation fixes from #167.

This also adds the USDT recovery APIs already included in 0.8.0-rc1 and required by the mobile apps, but absent from master. UsdtBackup acknowledges an application-encrypted remote snapshot before a signed payment is submitted or rebroadcast. Export/restore preserves payment IDs and signed operations, merges atomically for the same account, and reconciles restored outcomes with the chain.

Synchronizes the Cargo, Android, and SwiftPM versions; regenerates the Swift interfaces; and updates SwiftPM to the rebuilt device/simulator XCFramework checksum. The optional bridge endpoint remains optional. No dependency upgrades or data migrations.

Validation:

  • USDT suite: 83 passed; one fork-only test skipped. Includes backup failures, restart recovery, atomic restore, stale snapshots, and bridge tracking.
  • Rust formatting and Clippy checked; no new Clippy diagnostics compared with master.
  • Swift recovery/constructor and existing record smoke tests passed against the static host library; device and simulator XCFramework built. The local Xcode linker rejected the host dylib with a LINKEDIT alignment error, so the smoke tests used the static archive.
  • Kotlin bindings regenerated with the repository-pinned generator; release bindings and existing Kotlin smoke fixture compiled. ktlint diagnostics inspected.
  • Shell scripts, Cargo, Gradle, and SwiftPM manifests validated. Full Android native packaging is covered by the PR's Android Bindgen CI.

This PR prepares the release; it does not publish a tag/package or change the apps' dependency pins. The release ZIP must match the checksum committed here.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ✅ Approve

Review: diff 16 files.

Findings:
1 inline (1 MEDIUM)

QA:
Tests wait for the author’s automated validation reported in the description; no manual test or journey was requested.


Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

Comment thread src/modules/usdt/tests/backup.rs
ovitrif
ovitrif previously approved these changes Oct 7, 2026

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAck

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ✅ Approve

Reaudit: diff 1 file.
No new findings; the rest is in the review.

QA:
I ran no tests because the description only reports automated validation already run by the author and requests no reviewer manual test or journey.


Reviewed by gpt-6.1-sol-medium via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

@coreyphillips

Copy link
Copy Markdown
Collaborator

Two independent reviews.

needs changing before merge

  • Restore can assign one refund transaction to two payments (src/modules/usdt/backup.rs:249). Restoring a terminal BridgeRefunded record inserts it without running the refund ownership query used by update_delivery. Merge a local database containing refunded payment A with a pruned snapshot containing payment B with the same orchestra.refund_tx: both remain refunded and polling stops, so one on-chain credit is counted twice. I confirmed the no-plan restore path persists the transfer unchanged, while validation checks only payment IDs and operation hashes.

worth doing, does not block

  • Backup callback throwing a non-UsdtError panics inside Rust (src/modules/usdt/errors.rs:4). If the app's UsdtBackup.persist throws anything other than UsdtError (for example a URLError or a Kotlin IOException), the Rust side panics. Callers get an internal panic error instead of BackupUnavailable. UsdtError (src/modules/usdt/errors.rs) does not implement From<uniffi::UnexpectedUniFFICallbackError>. In uniffi_core 0.29.4, LiftReturn for Result<R, E>::handle_callback_unexpected_error panics when that conversion is missing (ffi_converter_traits.rs:396 and the convert_unexpected_error! path). The generated Swift (uniffiTraitInterfaceCallAsyncWithError, bitkitcore.swift:28662) reports any non-UsdtError throw as CALL_UNEXPECTED_ERROR. The panic happens at wallet.rs:237 in send and wallet.rs:477 in recover_pending, while the operation lock is held. RustFuture catches it and the tokio mutex does not poison, so the payment stays pending locally and nothing is broadcast. The cost is a confusing error and a Rust panic for what should be an ordinary "backup offline" condition. Fix: add impl From<uniffi::UnexpectedUniFFICallbackError> for UsdtError that maps to BackupUnavailable, or document that implementations must throw UsdtError.BackupUnavailable. I confirmed this by reading the uniffi_core source and the generated Swift. I did not run it end to end from Swift.
  • Every refresh of an unmined pending payment re-uploads the full snapshot (src/modules/usdt/wallet.rs:477). recover_pending (wallet.rs:476-478) calls self.backup.persist(self.export_backup()?) on every refresh_transfers while the nonce is unconsumed and the operation is unexpired. That covers every poll before mining. export_backup serializes every row in usdt_transfers, including all incoming history and retained signed plans, so each poll does a full encrypted remote upload under the wallet operation lock. If the backup is unchanged since the last acknowledged persist, it does not need to be resent. That is fine for an RC, but apps that poll frequently will see repeated uploads and lock hold time that grows with history size. Possible fix: skip the persist when the snapshot hash matches the last acknowledged one.
  • Hash casing bypasses duplicate operation validation (src/modules/usdt/backup.rs:100). Backup validation compares operation hashes as case-sensitive strings, but later parses them as B256, which accepts mixed-case hex. Changing only the casing lets one signed operation appear under two payment IDs. The non-unique, case-sensitive database index then accepts both records, and recovery can show both as confirmed from the same receipt.

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAck pls check Corey's suggestions

@ben-kaufman
ben-kaufman merged commit 06234a8 into master Oct 8, 2026
6 checks passed
@ben-kaufman
ben-kaufman deleted the chore/release-0.8.0-rc2 branch October 8, 2026 14:42
@ben-kaufman

Copy link
Copy Markdown
Collaborator Author

Thanks, addressed all four points in #169. This PR merged while I was validating the fixes, so they are in a focused follow-up against current master:

  • Restore now uses the same refund-ownership check as delivery updates, inside the merge transaction. Conflicts roll back the entire restore, including pruned terminal records.
  • Operation and refund hashes are canonicalized before comparison. Live refund processing stores canonical hashes too, so casing or an omitted 0x cannot create a second identity.
  • Unexpected Swift/Kotlin backup errors return BackupUnavailable instead of causing a Rust panic.
  • An unchanged snapshot is uploaded once after acknowledgement per wallet session. Failed saves, changed snapshots and reopening the wallet still require a successful backup before broadcast.

Validation: 85 USDT tests passed, one opt-in fork test ignored; format checks passed and Clippy introduced no diagnostics. Coverage includes atomic restore conflicts, hash encodings, the UniFFI callback error path, failed acknowledgements, retries, reopening and subsequent payments. The device/simulator XCFramework and its SwiftPM checksum have been rebuilt; generated Swift interfaces are unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants