Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions packages/rs-platform-wallet-ffi/src/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ pub unsafe extern "C" fn platform_wallet_manager_create_wallet_from_seed(
};

let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(manager_handle, |manager| {
runtime().block_on(manager.create_wallet_from_seed_bytes(network, seed, accounts))
runtime().block_on(manager.create_wallet_from_seed_bytes(network, seed, accounts, None))
});
let result = unwrap_option_or_return!(option);
let wallet = unwrap_result_or_return!(result);
Expand Down Expand Up @@ -139,7 +139,12 @@ pub unsafe extern "C" fn platform_wallet_manager_create_wallet_from_mnemonic(
};

let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(manager_handle, |manager| {
runtime().block_on(manager.create_wallet_from_mnemonic(mnemonic_str, network, accounts))
runtime().block_on(manager.create_wallet_from_mnemonic(
mnemonic_str,
network,
accounts,
None,
))
Comment on lines 103 to +147
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.

🟡 Suggestion: FFI hardcodes None, regressing first-run pre-funded mnemonic imports for iOS

Both platform_wallet_manager_create_wallet_from_seed (line 104) and platform_wallet_manager_create_wallet_from_mnemonic (line 146) unconditionally pass None for birth_height_override. Before this PR, ManagedWalletInfo was hardcoded to birth_height = 0 at registration, so an FFI caller's first-process SPV scan started at genesis and would surface coins funded before the wallet was registered. After this PR, None resolves to the current SPV header tip whenever SPV is up, so the same flow silently misses pre-existing UTXOs. This is exactly the failure mode the PR description cites as the motivation for the new override (bank-wallet pre-funded with testnet duffs), yet the override is unreachable from the FFI/Swift surface — which is the primary consumer of this crate. The pre-PR behaviour was already broken across restart (persisted birth_height was SPV-tip), so this is a tradeoff not a pure regression, but it leaves the stated use case unaddressable from iOS. Add a sibling *_with_birth_height FFI entry (or a birth_height_override: i64 with sentinel) so iOS callers can opt into Some(0) / Some(h). Not provided as a suggestion patch because exposing it cleanly requires an additional FFI export, not a one-line edit.

source: ['claude', 'codex']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-ffi/src/manager.rs`:
- [SUGGESTION] lines 103-147: FFI hardcodes `None`, regressing first-run pre-funded mnemonic imports for iOS
  Both `platform_wallet_manager_create_wallet_from_seed` (line 104) and `platform_wallet_manager_create_wallet_from_mnemonic` (line 146) unconditionally pass `None` for `birth_height_override`. Before this PR, `ManagedWalletInfo` was hardcoded to `birth_height = 0` at registration, so an FFI caller's first-process SPV scan started at genesis and would surface coins funded before the wallet was registered. After this PR, `None` resolves to the current SPV header tip whenever SPV is up, so the same flow silently misses pre-existing UTXOs. This is exactly the failure mode the PR description cites as the motivation for the new override (bank-wallet pre-funded with testnet duffs), yet the override is unreachable from the FFI/Swift surface — which is the primary consumer of this crate. The pre-PR behaviour was already broken across restart (persisted birth_height was SPV-tip), so this is a tradeoff not a pure regression, but it leaves the stated use case unaddressable from iOS. Add a sibling `*_with_birth_height` FFI entry (or a `birth_height_override: i64` with sentinel) so iOS callers can opt into `Some(0)` / `Some(h)`. Not provided as a `suggestion` patch because exposing it cleanly requires an additional FFI export, not a one-line edit.

Comment on lines 103 to +147
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.

🟡 Suggestion: FFI constructors hardcode None, so Swift cannot request the new recovery scan modes

Both exported constructors still pass birth_height_override = None into the Rust API, and the public Swift wrappers expose no parameter for Some(0) or Some(h). That would be a pure ergonomics gap if None still behaved exactly as before, but this PR changed the underlying semantics: before, wallet registration always seeded ManagedWalletInfo with 0; now None resolves to the live SPV tip whenever SPV is already running. Swift can start SPV before wallet creation via PlatformWalletManagerSPV.startSpv, so this path is reachable from public API. As a result, the main FFI consumer cannot force the full-history or known-height rescan that this feature was added to support, and the commit message's claim that FFI None preserves existing semantics is not true once SPV has started.

source: ['codex']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-ffi/src/manager.rs`:
- [SUGGESTION] lines 103-147: FFI constructors hardcode `None`, so Swift cannot request the new recovery scan modes
  Both exported constructors still pass `birth_height_override = None` into the Rust API, and the public Swift wrappers expose no parameter for `Some(0)` or `Some(h)`. That would be a pure ergonomics gap if `None` still behaved exactly as before, but this PR changed the underlying semantics: before, wallet registration always seeded `ManagedWalletInfo` with `0`; now `None` resolves to the live SPV tip whenever SPV is already running. Swift can start SPV before wallet creation via `PlatformWalletManagerSPV.startSpv`, so this path is reachable from public API. As a result, the main FFI consumer cannot force the full-history or known-height rescan that this feature was added to support, and the commit message's claim that FFI `None` preserves existing semantics is not true once SPV has started.

});
let result = unwrap_option_or_return!(option);
let wallet = unwrap_result_or_return!(result);
Expand Down
1 change: 1 addition & 0 deletions packages/rs-platform-wallet/examples/basic_usage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
Network::Testnet,
seed_bytes,
WalletAccountCreationOptions::Default,
None,
)
.await?;

Expand Down
62 changes: 48 additions & 14 deletions packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,23 @@ impl<P: PlatformWalletPersistence + 'static> PlatformWalletManager<P> {
/// [`parse_mnemonic_any_language`]). For passphrase-only flows or
/// out-of-band seed material, derive the seed externally and use
/// [`Self::create_wallet_from_seed_bytes`].
///
/// `birth_height_override` controls SPV's compact-filter scan
/// window for the new wallet. `None` (the default for fresh
/// wallets) seeds the birth height to SPV's current confirmed
/// header tip, so the scan window is `[H_now, ∞)` — anything
/// funded before init is invisible. `Some(0)` requests a full
/// historical scan from genesis (use sparingly — expensive on
/// long-lived chains, but required when an address may have
/// received funds before the wallet was first registered).
/// `Some(h)` pins the scan start to a specific block height,
/// useful when a known funding block is on record.
Comment on lines +60 to +69
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Align the None contract with the actual fallback.

None only resolves to the current confirmed tip when sync_progress() already has headers. If the wallet is created before SPV starts or before headers are available—which is exactly what the updated example and spv_sync test do—the unwrap_or(0) path still seeds genesis and triggers a historical scan. Please either document that fallback here or resolve None from a durable header tip so the public contract matches runtime behavior.

Also applies to: 91-96, 128-140

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs` around lines 60
- 69, The comment for birth_height_override is inaccurate: None currently only
resolves to SPV's confirmed tip when sync_progress() already has headers,
otherwise the code falls back to genesis (unwrap_or(0)); update behavior so the
public contract matches runtime behavior by either (a) changing the comment on
birth_height_override to state the real fallback (None -> use sync_progress()
tip if available, otherwise fall back to genesis), or (b) change the
implementation that resolves None to consult a durable/persisted header tip from
the SPV subsystem (e.g., call the persisted tip API instead of relying solely on
sync_progress()), then use that tip height as the default; update all places
referencing birth_height_override and sync_progress() accordingly (also fix
similar wording/behavior where birth_height_override is documented at the other
occurrences).

Comment on lines +60 to +69
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.

🟡 Suggestion: Public None contract omits the fallback to a genesis scan

The rustdoc says birth_height_override = None seeds the wallet from SPV's current confirmed tip, but register_wallet does not guarantee that. The actual resolver at lines 132-139 uses sync_progress().await...unwrap_or(0), so when SPV is not running yet or header access fails, None falls back to 0 and triggers a full historical scan from genesis. This is not just an internal detail: callers choose materially different sync cost and discovery behavior through this API, so the public contract needs to describe the offline-SPV fallback accurately.

💡 Suggested change
Suggested change
/// `birth_height_override` controls SPV's compact-filter scan
/// window for the new wallet. `None` (the default for fresh
/// wallets) seeds the birth height to SPV's current confirmed
/// header tip, so the scan window is `[H_now, ∞)` — anything
/// funded before init is invisible. `Some(0)` requests a full
/// historical scan from genesis (use sparingly — expensive on
/// long-lived chains, but required when an address may have
/// received funds before the wallet was first registered).
/// `Some(h)` pins the scan start to a specific block height,
/// useful when a known funding block is on record.
/// `birth_height_override` controls SPV's compact-filter scan
/// window for the new wallet. `None` prefers SPV's current
/// confirmed header tip so the scan window is `[H_now, ∞)`, but
/// falls back to `0` (full historical scan from genesis) when SPV
/// state is unavailable. `Some(0)` requests a full historical
/// scan from genesis (use sparingly — expensive on long-lived
/// chains, but required when an address may have received funds
/// before the wallet was first registered). `Some(h)` pins the
/// scan start to a specific block height, useful when a known
/// funding block is on record.

source: ['codex']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:
- [SUGGESTION] lines 60-69: Public `None` contract omits the fallback to a genesis scan
  The rustdoc says `birth_height_override = None` seeds the wallet from SPV's current confirmed tip, but `register_wallet` does not guarantee that. The actual resolver at lines 132-139 uses `sync_progress().await...unwrap_or(0)`, so when SPV is not running yet or header access fails, `None` falls back to `0` and triggers a full historical scan from genesis. This is not just an internal detail: callers choose materially different sync cost and discovery behavior through this API, so the public contract needs to describe the offline-SPV fallback accurately.

pub async fn create_wallet_from_mnemonic(
&self,
mnemonic_phrase: &str,
network: Network,
accounts: WalletAccountCreationOptions,
birth_height_override: Option<u32>,
) -> Result<Arc<PlatformWallet>, PlatformWalletError> {
let mnemonic = parse_mnemonic_any_language(mnemonic_phrase)
.map_err(|e| PlatformWalletError::WalletCreation(format!("Invalid mnemonic: {}", e)))?;
Expand All @@ -70,35 +82,64 @@ impl<P: PlatformWalletPersistence + 'static> PlatformWalletManager<P> {
e
))
})?;
self.register_wallet(wallet).await
self.register_wallet(wallet, birth_height_override).await
}

/// Create a PlatformWallet from raw seed bytes, initialize persisted
/// state, register it with the manager and return an `Arc` handle.
///
/// See [`Self::create_wallet_from_mnemonic`] for the
/// `birth_height_override` semantics. `None` keeps the
/// pre-existing behaviour (scan from current SPV tip forward);
/// `Some(h)` is for callers that need to see funding deposited
/// before the wallet was registered (e.g. a long-lived bank
/// address pre-funded with testnet duffs).
pub async fn create_wallet_from_seed_bytes(
&self,
network: Network,
seed_bytes: [u8; 64],
accounts: WalletAccountCreationOptions,
birth_height_override: Option<u32>,
) -> Result<Arc<PlatformWallet>, PlatformWalletError> {
let wallet = Wallet::from_seed_bytes(seed_bytes, network, accounts).map_err(|e| {
PlatformWalletError::WalletCreation(format!(
"Failed to create wallet from seed bytes: {}",
e
))
})?;
self.register_wallet(wallet).await
self.register_wallet(wallet, birth_height_override).await
}
Comment on lines 70 to 111
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.

💬 Nitpick: Breaking change to public Rust API not surfaced in PR title

create_wallet_from_mnemonic and create_wallet_from_seed_bytes are pub and gain a non-defaulted Option<u32> parameter — any external Rust consumer of packages/rs-platform-wallet needs to migrate. Per .github/workflows/pr.yml (conventional-commits enforcement) and the PR template's breaking-change checkbox, this should be feat(rs-platform-wallet)!: so the API break surfaces in generated changelogs. The C ABI is preserved by the FFI shim (which absorbs the parameter as None), so this is purely about Rust-crate consumers and changelog hygiene. Nitpick within an internal monorepo where all callers are updated in-PR, but worth correcting before merge.

source: ['claude']


/// Register a pre-built `Wallet` with the manager: insert into the
/// `WalletManager`, build a `PlatformWallet` handle, load persisted
/// state, and return an `Arc` to the managed wallet.
///
/// `birth_height_override` flows through to both the in-memory
/// `ManagedWalletInfo` sync checkpoint and the persisted
/// `WalletMetadataEntry` so the SPV scan window is consistent
/// across restarts. See [`Self::create_wallet_from_mnemonic`] for
/// the contract.
#[allow(clippy::type_complexity)]
async fn register_wallet(
&self,
wallet: Wallet,
birth_height_override: Option<u32>,
) -> Result<Arc<PlatformWallet>, PlatformWalletError> {
let wallet_info = ManagedWalletInfo::from_wallet(&wallet, 0);
// Birth height resolution: explicit override wins; otherwise
// fall back to SPV's confirmed header tip (default for fresh
// wallets — they only need to see funding from now on); 0 if
// SPV isn't running yet.
let birth_height: u32 = match birth_height_override {
Some(h) => h,
None => self
.spv_manager
.sync_progress()
.await
.and_then(|p| p.headers().ok().map(|h| h.tip_height()))
.unwrap_or(0),
};

let wallet_info = ManagedWalletInfo::from_wallet(&wallet, birth_height);

let balance = Arc::new(WalletBalance::new());

Expand Down Expand Up @@ -192,17 +233,10 @@ impl<P: PlatformWalletPersistence + 'static> PlatformWalletManager<P> {
// the persister is a best-effort channel, not a source of
// truth in steady state.

// Birth height = SPV's confirmed header tip if SPV is running,
// otherwise 0 (caller can bump it later when SPV catches up).
// 0 means "scan from genesis", which is safe-correct for
// fresh wallets.
let birth_height: u32 = self
.spv_manager
.sync_progress()
.await
.and_then(|p| p.headers().ok().map(|h| h.tip_height()))
.unwrap_or(0);

// `birth_height` was resolved at the top of `register_wallet`
// and seeded into `ManagedWalletInfo`; reuse it here so the
// persisted `WalletMetadataEntry` agrees with the in-memory
// sync checkpoint.
let mut registration_changeset = PlatformWalletChangeSet {
wallet_metadata: Some(WalletMetadataEntry {
network: self.sdk.network,
Expand Down
7 changes: 6 additions & 1 deletion packages/rs-platform-wallet/tests/spv_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,12 @@ async fn test_spv_sync_and_balance() {
let seed_bytes = mnemonic.to_seed("");

let platform_wallet = manager
.create_wallet_from_seed_bytes(network, seed_bytes, WalletAccountCreationOptions::Default)
.create_wallet_from_seed_bytes(
network,
seed_bytes,
WalletAccountCreationOptions::Default,
None,
)
.await
.expect("Failed to create platform wallet");

Expand Down
Loading