feat(android): init android SDK and expand uniffi coverage (T15) - #388
MrImmortal09 wants to merge 6 commits into
Conversation
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
This PR removes the checks for the js/react-native as the backend code ie. fedimint-client-uniffi is no longer maintained. The checks would be added as soon as js/react-native gets backed by rust/fedimint-sdk. |
|
Size Change: 0 B Total Size: 8.55 MB ℹ️ View Unchanged
|
4d24efe to
014b75a
Compare
9ab5d30 to
53d05b5
Compare
014b75a to
91aabfa
Compare
MrImmortal09
changed the title
|
@fedimint-bot review |
Review findingBlocking: React Native packages can still be published, but the release workflow no longer builds or attaches their required artifacts. In Before this PR, the workflow built RN Android/iOS artifacts, downloaded them into The same pattern exists in Suggested fix: either fully remove/ignore/mark private the RN packages and clear their pending changesets, or keep the old RN build + checksum + artifact upload path until the packages are intentionally unpublished/retired. NotesI did not find an issue in the new Android Kotlin artifact layout during this pass. The PR CI is green; I did not rerun the native/Gradle jobs locally. |
4a93493 to
327917d
Compare
MrImmortal09
changed the title
|
@fedimint-bot review |
Review resultNo new blocking findings from this pass. What I checked:
Notes:
|
|
@MrImmortal09 IMO this is jumping the gun a bit. If you recall, we had decided to first get the SDK complete and working before this. If we build the high-level on top of it already, the rust SDK changes will now involve the binding work as well, slowing down the Rust SDK plans. Nothing wrong with preparing already but I'd suggest making this work a WIP until we've the full SDK ready (i'm pretty confident we can get it done within the next few days). |
|
@zeenix I just thought that we should have something working on the go, so that we could test features against. |
Thanks, fwiw I also understand your motivation for doing this already. |
327917d to
4551bb4
Compare
4551bb4 to
554426a
Compare
MrImmortal09
changed the title
MrImmortal09
changed the title
MrImmortal09
changed the title
|
@fedimint-bot review |
| // The UniFFI views of `Operation<OnchainSendState>` and | ||
| // `Operation<OnchainReceiveState>`: `Operation<S>` is generic and UniFFI | ||
| // objects cannot be, so `crate::ffi::ffi_operation!` monomorphises | ||
| // one newtype object per state, forwarding every method to the real | ||
| // handle. See that macro's documentation in `ffi.rs`. |
There was a problem hiding this comment.
nitpick: please use the same line width limit (100 chars) for comments, as the rest of the code. Otherwise we unnecessarily have more LoC than needed. :)
| /// The UniFFI view of [`FederationInfo`], with `status` crossing as the | ||
| /// flattened projection above instead of the real [`FederationStatus`]. | ||
| /// Exported as `FederationInfo`, for the same reason. | ||
| #[cfg(feature = "uniffi")] | ||
| #[derive(Debug, uniffi::Record)] | ||
| #[uniffi(name = "FederationInfo")] | ||
| pub struct FfiFederationInfo { | ||
| /// See [`FederationInfo::id`]. | ||
| pub id: FederationId, | ||
| /// See [`FederationInfo::name`]. | ||
| pub name: Option<String>, | ||
| /// See [`FederationInfo::network`]. | ||
| pub network: Network, | ||
| /// See [`FederationInfo::status`]. | ||
| pub status: FfiFederationStatus, | ||
| } |
There was a problem hiding this comment.
Can we please put all the FFI-specific/only items into a separate module hierarchy?
There was a problem hiding this comment.
Yeah sure , moving them rn.
Review findingsI found two Android build blockers.
Checked:
|
| pub enum FfiFederationStatus { | ||
| /// See [`FederationStatus::Running`]. | ||
| Running, | ||
| /// See [`FederationStatus::Recovering`]. | ||
| Recovering, | ||
| /// See [`FederationStatus::Quarantined`]. | ||
| Quarantined { | ||
| /// [`Diagnostic::code`](crate::Diagnostic::code). | ||
| code: crate::ErrorCode, | ||
| /// [`Diagnostic::message`](crate::Diagnostic::message). | ||
| message: String, | ||
| }, | ||
| /// See [`FederationStatus::Closed`]. | ||
| Closed, | ||
| /// See [`FederationStatus::Forgetting`]. | ||
| Forgetting, | ||
| /// See [`FederationStatus::Forgotten`]. | ||
| Forgotten, | ||
| } |
There was a problem hiding this comment.
Do we really need to duplicate the whole type? 🤔
zeenix
left a comment
zeenix
left a comment
There was a problem hiding this comment.
Review by Codex on behalf of @zeenix. Reviewed 554426a against T15 and the existing comments.
Requesting changes for the three inline correctness findings. The existing Android build findings
also remain: migrate both modules from kotlinOptions.jvmTarget to compilerOptions, and align
CI's JDK 17 with the modules' Java 21 target.
Two T15 requirements still need completing:
- Move FFI-specific items into the requested module hierarchy. Keep common operation, subscriber
and quote adaptation independent of UniFFI so T14 can reuse it. The currentffi_operation!
hardwires UniFFI attributes, and the whole module is enabled only by theuniffifeature. - Add boundary conformance tests. This PR adds no behavioural tests; assembling the demo does not
check repeated/concurrent quote submission, subscriber completion and cancellation, diagnostic
preservation, or shutdown waking observers. Exercise those through generated bindings.
On the type/ownership questions, I audited all 23 new Rust types:
| Types | Count | Assessment |
|---|---|---|
| Concrete operation wrappers | 7 | Justified: UniFFI objects cannot be generic |
| Subscriber wrappers | 7 | Justified: concrete types and serialized mutable access |
| Result records | 4 | Justified: generic operations and nested object references |
| Ecash detail projections | 2 | Justified: preserve opaque Notes and Rust field types |
| Federation status/info projections | 2 | Avoidable; see below |
| QuoteClaim | 1 | Reasonable shared single-use enforcement |
For the duplication comment: export RawErrorDetails as a record, map DetailEnvelope to it
with uniffi::custom_type!, and export the existing Diagnostic, FederationStatus and
FederationInfo. This removes both federation projections while preserving structured details.
I compiled a minimal UniFFI 0.32 prototype and successfully generated Kotlin for this shape.
The complete T15 surface also needs the structured-details accessor on Error, which currently
exports only code() and reason().
The new Arc allocations for objects nested in records, Option and Vec are required by
UniFFI's conversion traits. As a small simplification, the three ffi_send parameters can take
&Quote: their bodies only borrow. This does not remove UniFFI's internal shared ownership.
I found no redundant new mutex.
Validation: cargo check --locked --features uniffi --tests passed, and I checked the failing
Android CI log. No end-to-end Kotlin tests were run.
| FederationStatus::Quarantined { diagnostic } => Self::Quarantined { | ||
| code: diagnostic.code, | ||
| message: diagnostic.message, | ||
| }, |
There was a problem hiding this comment.
Review by Codex on behalf of @zeenix.
[P2] Preserve the diagnostic details when exporting quarantine status
This conversion copies only code and message, discarding diagnostic.details. Kotlin
callers of stored_federations(), federation_status() and status subscriptions consequently
lose structured data such as the conflicting module generations, and unknown detail envelopes
cannot survive the boundary as the contract promises.
Please carry the optional RawErrorDetails envelope using DetailEnvelope::to_raw().
The raw-envelope conversion also offers a way to remove these duplicated status/info types;
see the review summary.
| withContext(Dispatchers.Main) { | ||
| lastInvoice = receive.invoice | ||
| lnReceiveCopy.isEnabled = true | ||
| watch(result, header) { receive.operation.updates().next() } |
There was a problem hiding this comment.
Review by Codex on behalf of @zeenix.
[P2] Reuse one subscription for the lifetime of this watcher
The lambda calls updates() again on every iteration of watch. Each fresh subscription
immediately yields the current state, including a terminal state. The watcher therefore keeps
polling and allocating subscribers instead of waiting for transitions, and it never observes
the final null that ends the loop.
Create the subscriber once before starting watch, reuse its next(), and close it when the
watcher finishes. The recovery, deposit and withdrawal watchers have the same problem.
| val state = operation.awaitFinal() | ||
| "Redeemed ${formatMsats(notes.value())}\n\n" + | ||
| "operation ${operation.id()}\nstate: ${describeState(state)}" |
There was a problem hiding this comment.
Review by Codex on behalf of @zeenix.
[P2] Only report redemption after a successful terminal state
awaitFinal() also returns EcashReceiveState.Failed normally: operation failures are
states, while exceptions represent failures to observe the operation. This currently displays
"Redeemed" even when the notes were not redeemed.
Match the returned state and display the success message only for Done; display the failure
reason for Failed.
554426a to
6adb620
Compare
|
On retiring the React Native lanes: please switch the tool instead of removing them. This PR repoints the Nix cross-compile from Concretely, in this PR rather than a follow-up:
T14 (the wasm binding) then builds on this PR: it adds the tool's Generated by Claude Fable 5.1. |
6adb620 to
2ef6807
Compare
|
Regards wasm support , we actually don't know how the exact changes that are required by ubrn for the react-native support ( jhugman/uniffi-bindgen-react-native#468 yet to be merged ), so for the reason I have not included any wasm releated changes here. |
|
@fedimint-bot review |
|
Review Finding
I did not find a second concrete runtime issue worth blocking on in the sampled Android/FFI paths before this compile blocker is resolved. |
2ef6807 to
90492ee
Compare
|
@fedimint-bot review |
Review resultNo new blocking finding from this pass. What changed since my last review:
Current CI state I saw:
I did not find a concrete additional runtime issue in the sampled FFI quote adapters, operation wrappers, generated-binding scripts, Android Gradle wiring, or demo lifecycle/threading paths. The broader maintainer review requesting T15 structure/test coverage changes still remains separate from this pass. Verification note: I attempted a local |
The React Native bindings were generated by uniffi-bindgen-react-native against rust/fedimint-client-uniffi. That generator is pinned to uniffi 0.31 and cannot read the metadata rust/fedimint-sdk emits under uniffi 0.32 — it walks the record with the wrong layout and fails partway through. So these lanes cannot be pointed at the new crate as they are, and keeping them running against the old one would only be testing a crate nothing is built on any more. Removed rather than disabled: a workflow that is skipped indefinitely still has to be read and reasoned about by everyone who touches CI, and the git history is the better record of how this worked. The React Native packages under js/ are untouched, so restoring these is a matter of reinstating the workflows once the generator catches up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds the android/ Gradle project (the fedimint-sdk library module that
becomes the AAR, plus a demo app), the cacheable Nix cross-compile that
produces its native libraries, and the scripts and CI that tie the two
together.
There is no hand-written Kotlin. The `#[uniffi::export]` blocks in
rust/fedimint-sdk hand out that crate's own types, so this SDK is a view
of that API rather than a copy that could drift from it. The surface is
the three Sdk methods that exist today — export_mnemonic, preview, join
— plus the constructor and the two value types they need.
Building it is two steps, and the split is the point:
1. the native library, cross-compiled for both ABIs through
nix/ffi.nix, which is expensive (rocksdb and aws-lc from C) and is
not specific to any one language; and
2. the bindings, which uniffi-bindgen reads out of the `.so` produced
by step 1 rather than out of the crate source, so the Kotlin cannot
describe a binary other than the one the device loads.
CI mirrors that exactly. android-native.yaml builds the `.so` and
uploads it; kotlin-sdk.yaml calls that workflow and generates against
the artifact. Keeping the costly half behind one callable workflow means
it is paid for once and a second binding generator added later starts
from the same binary. android-native.yaml has no pull_request trigger of
its own so that a PR touching both a Nix file and android/ cannot
cross-compile the same commit twice.
The Kotlin generation runs on plain cargo rather than Nix:
rust/uniffi-bindgen's only dependency is uniffi itself, which keeps that
whole job free of Nix, the NDK and any cross-compile toolchain.
`.#fedimint-uniffi-bindgen` builds the same binary but does not
currently work — crane's vendoring loses uniffi_bindgen's askama.toml.
nix-build.yml is replaced by android-native.yaml, which does the same
cache warming for the derivations this crate actually uses.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… shell libclang's unwrapped clang was shadowing the host compiler for host-target builds, and aws-lc-sys's bindgen feature was falling back to a CMake build the crates.io tarball can't satisfy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…to UniFFI Extends the opt-in `uniffi` feature (#384) from the three `Sdk` bootstrap methods to the whole per-federation surface: `Federation`'s own methods, its `ecash`/`lightning`/`onchain` facades, `AnyOperation` and the `Operation<S>` handles each facade returns, and `Sdk::recover` / `resume_recovery`. `ffi.rs` holds the two pieces of machinery shared across facades: `ffi_operation!`, which monomorphises one UniFFI object per concrete `Operation<S>` instantiation (the type is generic and a UniFFI object cannot be), and `QuoteClaim`, the single-use guard every quote type carries so a binding cannot submit the same quote twice — `send` takes its quote by value in plain Rust, which already rules that out at compile time, but a binding only ever holds a shared `Arc`. Bearer values stay off the wire as bare strings: `Notes` (ecash) crosses as a UniFFI object exactly like `InviteCode` and `Mnemonic` already do, so a generated Kotlin/Swift record can never print the token through its automatic `toString`. Records that hold notes by value in plain Rust (`EcashSendDetails`, `EcashReceiveDetails`) get an `Arc<Notes>`-holding projection exported under the real name instead, built through an exhaustively-destructured `From` impl so a field added to the Rust record fails to compile until its projection carries it too. `Sdk::federation` and `Sdk::federation_status` now return `Result` over FFI: both take a `FederationId` lifted from a plain string, and only a fallible export turns a malformed one into this crate's own `InvalidInput` rather than UniFFI's internal error. The renamed exports this needed (`FederationStatus`, `FederationInfo` for the two `Ffi*`-prefixed projections; snake_case for the handful that had drifted to camelCase) keep the generated surface consistent with the rest. nix/ffi.nix restores the macOS-only `libiconv` build input the earlier rewrite of `buildOne` dropped, needed for `cc -liconv` to resolve in a host build script's link step on macOS 14+. scripts/build-android-sdk.sh now calls nix-build-android-so.sh and generate-kotlin-bindings.sh — the same two scripts CI runs as android-native.yaml and kotlin-sdk.yaml — instead of duplicating their logic, so `just build-kotlin` can no longer drift from what CI does. Only the cargo-ndk `--local` path is its own code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replaces the step-numbered demo layout with one section per SDK call, the way js/examples/vite-core lays its own demo out: wallet status (open/restore/refresh, with a live balance from balanceUpdates()), join (preview/join/recover), generate and pay a Lightning invoice, send and redeem ecash, parse an invite code, generate a deposit address, send on-chain, and recent activity. Every operation-returning call now follows its own state to completion in its result line instead of reading it once. `onJoin` releases its old `Federation` handle only after a new one exists, and treats `ALREADY_JOINED` as the cue to look the running federation up by `sdk.federation(id)` rather than treat it as failure — which is also what makes reattaching after a restart possible. Amount fields convert sats to msats with an overflow check instead of wrapping. Every section reads its `EditText`s on the main thread before its `Dispatchers.IO` block runs. Exercised against a live mutinynet federation on an emulator: open, preview, join (including the already-joined path), invoice generation, and deposit address generation all completed successfully. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
90492ee to
bf8cc00
Compare
|
@fedimint-bot review again |
Review resultNo new blocking finding from this pass. Rechecked on current head
Current CI state when checked:
Caveat: I did not run the native cross-compile or Gradle build locally on this runner. The broader maintainer feedback about whether this PR should retire or preserve the React Native lanes remains a product/PR-scope decision separate from this pass. |
3 participants
Summary
Adds an Android SDK: the
android/Gradle project whose Kotlin bindings are generated fromrust/fedimint-sdk'suniffifeature, the cacheable Nix cross-compile that produces its native libraries, the CI that builds the native library once and generates bindings from that artifact, and a demo app that drives every exported call from one screen.Base branch note: this PR was originally stacked on
om/ffi-stage1. That branch's own commits (T4, T7, T11, T12) have since landed onmainindividually, so the branch is now rebased directly onto currentmain— no merge commits, every commit here is this PR's own.Details
There is no hand-written Kotlin. The
#[uniffi::export]blocks inrust/fedimint-sdkhand out that crate's own types (Sdk,Federation,Ecash,Lightning,Onchain,AnyOperation,Mnemonic,InviteCode,Notes, and the rest), so this SDK is a view of that API rather than a copy that could drift from it. The surface now covers the whole per-federation API: federation lifecycle (status, reopen, close, forget, shutdown), theecash/lightning/onchainfacades'quote→send/receivecalls, operation lookup and typed downcasts, seed recovery, activity history and metadata — not just the threeSdkbootstrap methods (createFedimintSdk,preview,join) the feature started with.Two pieces of shared machinery live in the new
ffi.rs:ffi_operation!monomorphises one UniFFI object per concreteOperation<S>instantiation, since the real type is generic and a UniFFI object cannot be. Invoked once per facade, next to the state type it wraps.QuoteClaimis the single-use guard every quote type (EcashQuote,LnQuote,OnchainQuote) carries. In plain Rust,sendtakes its quote by value, so a second attempt with the same quote is a compile error; a Kotlin/Swift binding only ever holds a sharedArc, so nothing else stops it from callingsendtwice.QuoteClaimturns that into a runtimeQuoteExpired(already_executed: true) on the second attempt, shared across all three facades instead of copied three times.Notes(ecash) crosses as a UniFFI object, the same wayInviteCodeandMnemonicalready do, rather than a bareString. Ecash notes are a bearer instrument — printing one hands over spendable value — and a generated Kotlin/Swift record prints every field through its automatictoString(). As an object, the token comes out only through an explicitdisplay(). The two Rust records that hold notes by value (EcashSendDetails,EcashReceiveDetails) keep their plain-Rust shape and get anArc<Notes>-holding projection exported under the same name, built through an exhaustively-destructuredFromimpl — no..— so a field added to the Rust record fails to compile here until the projection is updated too.Building it is still the same two Nix-then-bindgen steps as before, with the driving script now delegating to the same two scripts CI runs (
nix-build-android-so.sh,generate-kotlin-bindings.sh) instead of a separate copy of that logic, sojust build-kotlincannot silently diverge from whatandroid-native.yaml/kotlin-sdk.yamldo.The demo app (
android/app) now has one section per SDK call — wallet status with a live balance, join (preview/join/recover), generate and pay a Lightning invoice, send and redeem ecash, parse an invite code, generate a deposit address, send on-chain, and recent activity — mirroring the layout ofjs/examples/vite-core's own demo, so every exported call has somewhere to be exercised by hand against a real federation.Reviewing
The two places worth being opinionated about, carried over from the original review:
.so; bindings are generated outside it, so each generator is a cheap step over a shared artifact rather than a second derivation re-entering the cross-compile.uniffi-bindgen-react-nativeis pinned to uniffi 0.31 and cannot read this crate's 0.32 metadata. The React Native packages underjs/are untouched.New in this round:
QuoteClaim) sits at the FFI boundary rather than in the plain-Rustsendmethods, since the by-value signature already enforces it there. Worth checking that the three call sites (ecash/lightning/onchainffi_send) all claim before calling into the realsend, not after.Notesas an object vs. auniffi::custom_type!overString. The custom-type form is a few lines shorter but the token would still cross as a plain string, which a generated data class'stoString()would print. Went with the object for parity withInviteCode/Mnemonic.Ffi*detail records for ecash, whose only alternative was making the details themselves UniFFI objects (one accessor method per field, and inconsistent with every other facade's plain records).Testing
Verified locally on macOS (
nix develop .#android):cargo fmt --check,cargo clippy --locked --tests -- -D warningswith and without--features uniffi, andcargo doc --no-deps --features uniffiwith warnings as errors — all clean.cargo test --lib— 529 passing, with and without--features uniffi../scripts/build-android-sdk.sh— full Nix cross-compile (arm64-v8a+x86_64) and Kotlin generation.:app:installDebugonto a Pixel emulator, then exercised by hand against a live mutinynet federation (fed11qgq...— the same testnet federationjs/examples/vite-corepre-fills): open wallet (loads/generates the seed, reattaches to an already-joined federation), preview, join (including the already-joined → reattach path), generate a Lightning invoice, and generate a deposit address, all completed successfully with live-updating operation state.🤖 Generated with Claude Code