feat(platform-wallet): add birth_height_override to wallet creation API#3636
feat(platform-wallet): add birth_height_override to wallet creation API#3636lklimek wants to merge 4 commits into
Conversation
…n API Both `create_wallet_from_mnemonic` and `create_wallet_from_seed_bytes` now take a `birth_height_override: Option<u32>` controlling the SPV compact-filter scan window for the new wallet: - `None` keeps the prior behaviour (seed birth height to SPV's current confirmed header tip — fine for fresh wallets that only need to see funding from now on). - `Some(0)` requests a full historical scan from genesis, required when an address may have received funds before registration. - `Some(h)` pins the scan to a specific block height. The override flows through `register_wallet` into both the in-memory `ManagedWalletInfo` checkpoint and the persisted `WalletMetadataEntry` so the SPV scan window is consistent across restarts. Previously those two carried independent values (in-memory hardcoded to 0, persisted seeded from SPV tip), which was incoherent. FFI bindings and the basic_usage example pass `None` to preserve existing semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rride The new birth_height_override parameter introduced in the previous commit requires existing call-sites to be updated. The integration test `spv_sync` now passes `None` to preserve prior behaviour (birth-seed from SPV header tip). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR extends wallet creation methods with an optional ChangesBirth Height Override Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lklimek
changed the title
|
✅ Review complete (commit e246dd3) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:
- Around line 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).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80ec229c-b1db-4ad9-be12-28b41bae6085
📒 Files selected for processing (4)
packages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet/examples/basic_usage.rspackages/rs-platform-wallet/src/manager/wallet_lifecycle.rspackages/rs-platform-wallet/tests/spv_sync.rs
| /// `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. |
There was a problem hiding this comment.
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).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Production change correctly threads birth_height_override: Option<u32> through register_wallet so in-memory ManagedWalletInfo and persisted WalletMetadataEntry.birth_height share a single resolved value, closing the prior in-memory-vs-persisted drift bug. Three valid concerns remain: the public docstring on None claims a [H_now, ∞) scan but unwrap_or(0) silently degrades to a genesis rescan when SPV isn't running yet; no test pins the lock-step invariant or the persist/reload path; and the FFI shim hardcodes None, which is a real first-run behavioral change for iOS clients (pre-PR in-memory birth_height was hardcoded to 0 = genesis scan on first launch, post-PR FFI clients get SPV-tip-forward and silently miss pre-funded coins).
I validated existing CodeRabbit comments separately in-thread instead of restating them here.
Reviewed commit: 59d49f9
🟡 2 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
🟡 suggestion: No test pins the in-memory / persisted lock-step or the restart-reload path
packages/rs-platform-wallet/tests/spv_sync.rs (lines 33-92)
The central invariant introduced by this PR is that one resolved birth_height flows into both ManagedWalletInfo (in-memory checkpoint) and the persisted WalletMetadataEntry, and survives reload. The only test change is spv_sync.rs passing None to compile against the new signature. RecordingPersister records only synced_height from core changesets, and its load() returns ClientStartState::default(), so the suite never asserts (a) that Some(h) ends up in the captured wallet_metadata.birth_height, (b) that the None branch resolves to the same value in both sinks, or (c) that the value round-trips through load_from_persistor into state.core_wallet.birth_height(). Extend RecordingPersister to capture changeset.wallet_metadata and add a focused test that calls create_wallet_from_seed_bytes(..., Some(42)) and asserts the recorded birth_height == 42 plus matching managed info; a second case with Some(0) and a third that feeds the captured state back through load() would lock the restart path the PR is explicitly fixing.
🤖 Prompt for all review comments 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.
In `packages/rs-platform-wallet/tests/spv_sync.rs`:
- [SUGGESTION] lines 33-92: No test pins the in-memory / persisted lock-step or the restart-reload path
The central invariant introduced by this PR is that one resolved `birth_height` flows into both `ManagedWalletInfo` (in-memory checkpoint) and the persisted `WalletMetadataEntry`, and survives reload. The only test change is `spv_sync.rs` passing `None` to compile against the new signature. `RecordingPersister` records only `synced_height` from core changesets, and its `load()` returns `ClientStartState::default()`, so the suite never asserts (a) that `Some(h)` ends up in the captured `wallet_metadata.birth_height`, (b) that the `None` branch resolves to the same value in both sinks, or (c) that the value round-trips through `load_from_persistor` into `state.core_wallet.birth_height()`. Extend `RecordingPersister` to capture `changeset.wallet_metadata` and add a focused test that calls `create_wallet_from_seed_bytes(..., Some(42))` and asserts the recorded `birth_height == 42` plus matching managed info; a second case with `Some(0)` and a third that feeds the captured state back through `load()` would lock the restart path the PR is explicitly fixing.
| @@ -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, | |||
| )) | |||
There was a problem hiding this comment.
🟡 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.
| @@ -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 | |||
| } | |||
There was a problem hiding this comment.
💬 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']
…wallet-birth-height-override
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The core fix is correct: register_wallet now resolves one birth_height and writes the same value into both ManagedWalletInfo and WalletMetadataEntry, which removes the previous in-memory vs persisted drift. Three follow-up issues remain at this head: the public Rust docs overstate what None guarantees, the FFI/Swift boundary still hides the new override while changing None semantics once SPV is already running, and there is still no focused test that locks the new birth-height invariant across persistence and reload.
Reviewed commit: e246dd3
🟡 3 suggestion(s)
1 additional finding
🟡 suggestion: The new birth-height persistence invariant still has no focused test
packages/rs-platform-wallet/tests/spv_sync.rs (lines 33-90)
This test helper records only whether a core changeset existed and its synced_height, and load() always returns ClientStartState::default(). That means the suite never asserts the behavior this PR is supposed to protect: the resolved birth_height written into ManagedWalletInfo must match WalletMetadataEntry.birth_height and survive a load_from_persistor() round-trip. Because the original bug was precisely a drift between in-memory and persisted birth height, leaving that invariant untested makes this regression easy to reintroduce during future refactors.
🤖 Prompt for all review comments 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.
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.
In `packages/rs-platform-wallet/tests/spv_sync.rs`:
- [SUGGESTION] lines 33-90: The new birth-height persistence invariant still has no focused test
This test helper records only whether a core changeset existed and its `synced_height`, and `load()` always returns `ClientStartState::default()`. That means the suite never asserts the behavior this PR is supposed to protect: the resolved `birth_height` written into `ManagedWalletInfo` must match `WalletMetadataEntry.birth_height` and survive a `load_from_persistor()` round-trip. Because the original bug was precisely a drift between in-memory and persisted birth height, leaving that invariant untested makes this regression easy to reintroduce during future refactors.
| /// `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. |
There was a problem hiding this comment.
🟡 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
| /// `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.
| @@ -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, | |||
| )) | |||
There was a problem hiding this comment.
🟡 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.
2 participants
Issue being fixed or feature implemented
Splits a self-contained production-code change out of #3549 (the large e2e framework PR) so it can be reviewed and merged on its own cadence.
The previous wallet-creation flow stored two independent birth heights for a new wallet:
ManagedWalletInfocheckpoint was hardcoded to 0.WalletMetadataEntrywas seeded from the SPV header tip.Those two could disagree after a restart, which made SPV compact-filter scans inconsistent across process lifecycles and could cause the scan window to miss blocks containing existing wallet funding.
What was done?
birth_height_override: Option<u32>tocreate_wallet_from_mnemonicandcreate_wallet_from_seed_bytesinmanager/wallet_lifecycle.rs.Some(h)=> pin the scan window to heighth(useSome(0)for a full historical scan from genesis);None=> seed from SPV header tip (prior behaviour).register_walletinto both the in-memoryManagedWalletInfoand the persistedWalletMetadataEntry, so the two stay in lock-step.platform-wallet-ffi/src/manager.rs) and thebasic_usageexample to passNone— preserves existing semantics on those surfaces.spv_syncintegration test caller (and apply rustfmt) to satisfy the new signature.How Has This Been Tested?
cargo check -p platform-wallet --tests --all-features— clean.cargo clippy -p platform-wallet -p platform-wallet-ffi --no-deps -- -D warnings— clean.cargo fmtclean on all files touched by this PR.Some(0)to force genesis rescan; regular test wallets passNone).Note: there is a pre-existing compile failure in
packages/rs-platform-wallet-ffi/tests/integration_tests.rsonv3.1-devthat callsplatform_wallet_info_create_from_mnemonicwith 4 args against a 3-arg signature. It is independent of this PR and reproduces on plainv3.1-dev.Breaking Changes
None on public FFI surface — the FFI helpers (
platform-wallet-ffi/src/manager.rs) absorb the new parameter and continue to expose the same C ABI. Direct Rust callers ofcreate_wallet_from_mnemonic/create_wallet_from_seed_bytesneed to add anOption<u32>argument; passingNonepreserves the previous behaviour.Checklist:
For repository code-owners and collaborators only
Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Release Notes