feat: add DEFAULT_GAP_LIMIT core default + accessor#110
Conversation
Subtask 3 of #107. The gap limit of 20 was repeated as a literal across the watcher and account-info paths, and each mobile platform also hardcodes gapLimit: 20. Introduce a single `DEFAULT_GAP_LIMIT` constant (= 20, matching BIP44 and BDK's stop-gap floor), use it everywhere the literal appeared, and expose `get_default_gap_limit()` via UniFFI so platforms read one source of truth instead of hardcoding it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c0a0c54a6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Addresses Codex review on #110: the new UniFFI accessor was Rust-only. Regenerated the checked-in Swift and Python bindings so consumers can call get_default_gap_limit(). (iOS xcframework / Android Kotlin / dylib are rebuilt at release via build.sh, per repo convention.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ben-kaufman
left a comment
ben-kaufman
left a comment
There was a problem hiding this comment.
Looks like the Python binding artifacts are out of sync. bitkitcore.py now references uniffi_bitkitcore_fn_func_get_default_gap_limit, but the bundled libbitkitcore.dylib does not export that symbol, so importing bitkitcore from the checked-in package fails with a missing dlsym symbol.
Could we regenerate the Python native artifact too, or leave the generated Python wrapper out until the dylib is updated?
…fault_gap_limit Addresses @ben-kaufman review: the committed Python libbitkitcore.dylib (and the iOS/Android artifacts) did not export get_default_gap_limit, so importing the packaged bindings failed with a missing symbol. Regenerated everything from this Rust head via build.sh all (Swift + bitkitcoreFFI.h + XCFramework, Kotlin + jniLibs .so, Python wrapper + libbitkitcore.dylib). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@ben-kaufman regenerated all platform bindings and native artifacts from the Rust head (3704081), not just the Python wrapper, so the dylib/XCFramework/jniLibs all export the new symbol now. Confirmed present in the committed |
ben-kaufman
left a comment
ben-kaufman
left a comment
There was a problem hiding this comment.
Verified the regenerated artifacts are in sync now and the Python package imports successfully. Looks good.
# Conflicts: # Package.swift # bindings/android/lib/src/main/jniLibs/arm64-v8a/libbitkitcore.so # bindings/android/lib/src/main/jniLibs/armeabi-v7a/libbitkitcore.so # bindings/android/lib/src/main/jniLibs/x86/libbitkitcore.so # bindings/android/lib/src/main/jniLibs/x86_64/libbitkitcore.so # bindings/ios/BitkitCore.xcframework.zip # bindings/ios/BitkitCore.xcframework/ios-arm64-simulator/libbitkitcore.a # bindings/ios/BitkitCore.xcframework/ios-arm64/libbitkitcore.a # bindings/python/bitkitcore/libbitkitcore.dylib # src/lib.rs
|
LGTM, thank you guys 🥇 |
3 participants
Summary
Subtask 3 of #107. The address gap limit of
20is duplicated as a bare literal across core's watcher and account-info paths, and each mobile platform also hardcodesgapLimit: 20inHwWalletManagerand its watcher params. This makes it a core default with one source of truth.What changed
pub const DEFAULT_GAP_LIMIT: u32 = 20in the onchain module, documented (20 = BIP44 standard and BDK's hardcoded Electrum stop-gap floor).20literals with the constant:listener.rs—gap_limit.unwrap_or(...)and the.max(...)sync floor.implementation.rs—get_account_info'sgap_limit.unwrap_or(...).#[uniffi::export] pub fn get_default_gap_limit() -> u32(mirrorsget_default_wallet_id), so platforms read the default instead of hardcoding20.Behaviour is unchanged — the effective default was already 20; this just removes the duplication and exposes it.
Tests
cargo test default_gap_limit_is_twenty— asserts the constant and that the exported accessor returns it.🤖 Generated with Claude Code