Refactor UniFFI standalone functions to enum/record methods#612
Refactor UniFFI standalone functions to enum/record methods#612praveenperera merged 10 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMany Rust free-function UniFFI exports were moved to type-bound methods and instance accessors; generated UniFFI bindings and platform call sites in Kotlin and Swift were updated accordingly (replacing helper functions/extensions with zero-arg methods or instance methods). No behavioral logic changes beyond API surface and call-site adaptations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR refactors ~20 standalone UniFFI FFI functions into Key points:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as iOS/Android App
participant Bootstrap as bootstrap.rs
participant KeyVerify as verify_database_key
participant DB as EncryptedBackend
participant FFI as UniFFI (Swift/Kotlin)
App->>Bootstrap: bootstrap()
Bootstrap->>Bootstrap: derive_encryption_key()
Bootstrap->>KeyVerify: verify_database_key(&db_path)
alt File does not exist
KeyVerify-->>Bootstrap: Ok(())
else V2 header present
KeyVerify->>KeyVerify: verify_header_tag(key, header)
alt Tag matches
KeyVerify-->>Bootstrap: Ok(())
else Tag mismatch
KeyVerify-->>Bootstrap: Err(DatabaseKeyMismatch)
Bootstrap-->>App: AppInitError::DatabaseKeyMismatch
end
else V1 header present
KeyVerify->>KeyVerify: verify_v1_database_key (read first non-zero block)
alt Decryption succeeds
KeyVerify-->>Bootstrap: Ok(())
else Decryption fails
KeyVerify-->>Bootstrap: Err(HeaderIntegrity → DatabaseKeyMismatch)
Bootstrap-->>App: AppInitError::DatabaseKeyMismatch
end
end
Bootstrap->>DB: open_or_create_database()
alt V1 header
DB->>DB: upgrade header V1→V2 in-place
end
DB-->>Bootstrap: Database handle
Bootstrap->>FFI: expose methods on types (e.g. route.stableHash(), fiatCurrency.emojiString())
Bootstrap-->>App: Ok(wallet_count)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/src/router.rs (1)
421-438:⚠️ Potential issue | 🟡 MinorTypo in user message string.
Line 427 contains a grammatical error: "you need must enter" should be "you must enter".
Proposed fix
- Self::Sign(_) => "For security purposes, you need must enter your TAPSIGNER PIN before you can sign a transaction".to_string(), + Self::Sign(_) => "For security purposes, you must enter your TAPSIGNER PIN before you can sign a transaction".to_string(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/router.rs` around lines 421 - 438, The user-facing string in AfterPinAction::user_message contains a typo ("you need must enter") for the Sign(_) arm; update that variant's message to read "you must enter your TAPSIGNER PIN before you can sign a transaction" (ensure change is made in the user_message method used by ffi_user_message as well).rust/src/signed_import.rs (1)
185-216:⚠️ Potential issue | 🟠 MajorRemove
#[uniffi::export] impl SignedTransactionOrPsbt— UniFFI does not support arbitrary methods on enum types.Lines 185–216 export instance methods on
SignedTransactionOrPsbt, but UniFFI explicitly treats enums as data types without method support. The#[uniffi::export] implsyntax is reserved for objects (#[derive(uniffi::Object)]), not enums. An open UniFFI issue confirms this feature is not implemented. You have already defined the proper pattern with free functions (signed_transaction_or_psbt_try_parse,signed_transaction_or_psbt_try_from_bytes, etc.); remove the impl block and expose the remaining accessors (tx_id,is_psbt,is_transaction,transaction,psbt) as free functions with the same naming convention, or wrap them in a helper object if you need method-style binding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/signed_import.rs` around lines 185 - 216, Remove the #[uniffi::export] impl SignedTransactionOrPsbt block — UniFFI does not support exporting enum instance methods — and replace those exported instance methods (ffi_tx_id, ffi_is_psbt, ffi_is_transaction, ffi_transaction, ffi_psbt) with free functions following the existing naming convention (e.g., signed_transaction_or_psbt_<action> or ffi-prefixed free functions) that take a SignedTransactionOrPsbt argument and internally call the existing methods tx_id(), is_psbt(), is_transaction(), transaction(), and psbt(); this preserves the same API surface for bindings without using a #[uniffi::export] impl on the enum.
🧹 Nitpick comments (1)
ios/Cove/Extention/NodeSelection+Ext.swift (1)
10-17: Consider cachingtoNode()result if both properties are accessed together.Both
urlandnamecalltoNode()independently, which may create redundant conversions if both properties are accessed in sequence. For most use cases this is negligible, but if performance becomes a concern, consider a single computednodeproperty or lazy caching.♻️ Optional: Single conversion approach
If both properties are commonly accessed together:
extension NodeSelection { private var node: Node { toNode() } var url: String { node.url } var name: String { node.name } }Note: This still calls
toNode()per access tonode. For true caching, a different pattern would be needed, but that may be overkill for this use case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Extention/NodeSelection`+Ext.swift around lines 10 - 17, Both url and name call toNode() separately; add a private computed helper to perform the conversion once and reuse it. Add a private var node: Node { toNode() } inside the NodeSelection extension and change url and name to return node.url and node.name respectively (keep in mind this computed helper still calls toNode() per access; implement a stored cache on the underlying type only if true persistent caching is required).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@rust/src/router.rs`:
- Around line 421-438: The user-facing string in AfterPinAction::user_message
contains a typo ("you need must enter") for the Sign(_) arm; update that
variant's message to read "you must enter your TAPSIGNER PIN before you can sign
a transaction" (ensure change is made in the user_message method used by
ffi_user_message as well).
In `@rust/src/signed_import.rs`:
- Around line 185-216: Remove the #[uniffi::export] impl SignedTransactionOrPsbt
block — UniFFI does not support exporting enum instance methods — and replace
those exported instance methods (ffi_tx_id, ffi_is_psbt, ffi_is_transaction,
ffi_transaction, ffi_psbt) with free functions following the existing naming
convention (e.g., signed_transaction_or_psbt_<action> or ffi-prefixed free
functions) that take a SignedTransactionOrPsbt argument and internally call the
existing methods tx_id(), is_psbt(), is_transaction(), transaction(), and
psbt(); this preserves the same API surface for bindings without using a
#[uniffi::export] impl on the enum.
---
Nitpick comments:
In `@ios/Cove/Extention/NodeSelection`+Ext.swift:
- Around line 10-17: Both url and name call toNode() separately; add a private
computed helper to perform the conversion once and reuse it. Add a private var
node: Node { toNode() } inside the NodeSelection extension and change url and
name to return node.url and node.name respectively (keep in mind this computed
helper still calls toNode() per access; implement a stored cache on the
underlying type only if true persistent caching is required).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67cb0249-0a49-4dfd-8424-32a5e578c66f
⛔ Files ignored due to path filters (2)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_types.swiftis excluded by!**/generated/**
📒 Files selected for processing (46)
android/app/src/main/java/org/bitcoinppl/cove/MainActivity.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/AppearanceSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/FiatCurrencySettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NetworkSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NodeSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/TapSignerFlow/TapSignerEnterPinView.ktandroid/app/src/main/java/org/bitcoinppl/cove/utils/BootstrapStepExt.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/types/cove_types.ktios/Cove/CoveApp.swiftios/Cove/Extention/Color+Ext.swiftios/Cove/Extention/ColorSchemeSelection+Ext.swiftios/Cove/Extention/Enum+Ext.swiftios/Cove/Extention/Error+Ext.swiftios/Cove/Extention/NodeSelection+Ext.swiftios/Cove/Extention/Routes+Ext.swiftios/Cove/Extention/SignedTransactionOrPsbt+Ext.swiftios/Cove/Extention/String+Ext.swiftios/Cove/Extention/StringOrData+Ext.swiftios/Cove/Extention/WalletAddressType+Ext.swiftios/Cove/Extention/WalletMetadata+Ext.swiftios/Cove/Flows/CoinControlFlow/UtxoListScreen.swiftios/Cove/Flows/CoinControlFlow/UtxoRowPreview.swiftios/Cove/Flows/SendFlow/Common/SendFlowUtxoCustomAmountSheetView.swiftios/Cove/Flows/SettingsFlow/SettingsPicker.swiftios/Cove/Flows/SettingsFlow/WalletSettings/WalletSettingsView.swiftios/Cove/Flows/TapSignerFlow/TapSignerConfirmPinView.swiftios/Cove/Flows/TapSignerFlow/TapSignerEnterPinView.swiftios/Cove/WalletManager.swiftios/CoveCore/Sources/CoveCore/CoveCore.swiftrust/crates/cove-types/src/color_scheme.rsrust/crates/cove-types/src/network.rsrust/crates/cove-types/src/utxo.rsrust/src/bootstrap.rsrust/src/fiat.rsrust/src/manager/coin_control_manager.rsrust/src/manager/wallet_manager.rsrust/src/mnemonic/ffi.rsrust/src/multi_format.rsrust/src/node_connect.rsrust/src/router.rsrust/src/signed_import.rsrust/src/tap_card.rsrust/src/tap_card/tap_signer_reader.rsrust/src/wallet.rsrust/src/wallet/metadata.rs
💤 Files with no reviewable changes (3)
- ios/Cove/Extention/ColorSchemeSelection+Ext.swift
- android/app/src/main/java/org/bitcoinppl/cove/utils/BootstrapStepExt.kt
- rust/crates/cove-types/src/network.rs
Convert standalone FFI functions to impl methods on their respective types where UniFFI supports it. This moves functions like fiatCurrencyEmoji() to FiatCurrency.emojiString(), hashRoute() to Route.hashValue(), walletMetadataIsEqual() to WalletMetadata.isEqual(), etc. Update Swift and Kotlin callers to use the new method syntax, remove wrapper extensions that are no longer needed, and regenerate bindings for both platforms.
Bump Android app versionCode to 16. Regenerate UniFFI-generated iOS bindings (ios/CoveCore/Sources/CoveCore/generated/*): rename/replace several functions to match updated Rust API (e.g. addressErrorToAlertState, signedTransactionOrPsbtTryFromBytes/FromNfcMessage/Parse, defaultWalletColors, walletMetadataPreview, allUnits/allNetworks/allColorSchemes), add createTransportErrorFromCode, and update the corresponding checksum constants. These are generated binding updates to keep the iOS API in sync with the Rust changes.
praveenperera
force-pushed
the
use-methods
branch
from
c47aa7e to
a36f614
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/Cove/Extention/SignedTransactionOrPsbt`+Ext.swift:
- Around line 12-21: The helper wrappers tryFromNfcMessage, tryParse and
tryFromBytes call non-existent symbols; update their call sites to match the
generated CoveCore API names (replace signedTransactionOrPsbtTryParseNfcMessage
with signedTransactionOrPsbtTryFromNfcMessage,
signedTransactionOrPsbtTryParseString with signedTransactionOrPsbtTryParse, and
signedTransactionOrPsbtTryParseBytes with signedTransactionOrPsbtTryFromBytes)
or alternatively regenerate the CoveCore bindings so the helper calls match;
ensure you update the calls inside the static methods tryFromNfcMessage,
tryParse and tryFromBytes accordingly.
In `@ios/CoveCore/Sources/CoveCore/CoveCore.swift`:
- Around line 208-211: The init in extension SendFlowAlertState is calling a
non-existent symbol sendFlowAlertStateFromAddressError; change that call to the
generated Swift bridge function addressErrorToAlertState(error:address:) (or
rename the Rust export to match if you control native bindings). Update the
initializer in SendFlowAlertState to call addressErrorToAlertState(error:
addressError, address: address) so it uses the existing Rust-exported symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a08c47c7-a433-488a-b596-05e58779c552
⛔ Files ignored due to path filters (2)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_types.swiftis excluded by!**/generated/**
📒 Files selected for processing (44)
android/app/src/main/java/org/bitcoinppl/cove/MainActivity.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/AppearanceSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/FiatCurrencySettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NetworkSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NodeSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/TapSignerFlow/TapSignerEnterPinView.ktandroid/app/src/main/java/org/bitcoinppl/cove/utils/BootstrapStepExt.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/types/cove_types.ktios/Cove/CoveApp.swiftios/Cove/Extention/Color+Ext.swiftios/Cove/Extention/ColorSchemeSelection+Ext.swiftios/Cove/Extention/Enum+Ext.swiftios/Cove/Extention/Error+Ext.swiftios/Cove/Extention/NodeSelection+Ext.swiftios/Cove/Extention/Routes+Ext.swiftios/Cove/Extention/SignedTransactionOrPsbt+Ext.swiftios/Cove/Extention/String+Ext.swiftios/Cove/Extention/StringOrData+Ext.swiftios/Cove/Extention/WalletAddressType+Ext.swiftios/Cove/Extention/WalletMetadata+Ext.swiftios/Cove/Flows/CoinControlFlow/UtxoListScreen.swiftios/Cove/Flows/CoinControlFlow/UtxoRowPreview.swiftios/Cove/Flows/SendFlow/Common/SendFlowUtxoCustomAmountSheetView.swiftios/Cove/Flows/SettingsFlow/SettingsPicker.swiftios/Cove/Flows/SettingsFlow/WalletSettings/WalletSettingsView.swiftios/Cove/Flows/TapSignerFlow/TapSignerConfirmPinView.swiftios/Cove/Flows/TapSignerFlow/TapSignerEnterPinView.swiftios/Cove/WalletManager.swiftios/CoveCore/Sources/CoveCore/CoveCore.swiftrust/crates/cove-types/src/color_scheme.rsrust/crates/cove-types/src/utxo.rsrust/src/bootstrap.rsrust/src/fiat.rsrust/src/manager/coin_control_manager.rsrust/src/manager/wallet_manager.rsrust/src/mnemonic/ffi.rsrust/src/multi_format.rsrust/src/node_connect.rsrust/src/router.rsrust/src/signed_import.rsrust/src/tap_card.rsrust/src/tap_card/tap_signer_reader.rsrust/src/wallet.rsrust/src/wallet/metadata.rs
💤 Files with no reviewable changes (2)
- android/app/src/main/java/org/bitcoinppl/cove/utils/BootstrapStepExt.kt
- ios/Cove/Extention/ColorSchemeSelection+Ext.swift
🚧 Files skipped from review as they are similar to previous changes (21)
- ios/Cove/WalletManager.swift
- rust/src/manager/wallet_manager.rs
- rust/src/manager/coin_control_manager.rs
- ios/Cove/CoveApp.swift
- android/app/src/main/java/org/bitcoinppl/cove/flows/TapSignerFlow/TapSignerEnterPinView.kt
- rust/crates/cove-types/src/utxo.rs
- android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NetworkSettingsScreen.kt
- ios/Cove/Flows/CoinControlFlow/UtxoListScreen.swift
- ios/Cove/Flows/TapSignerFlow/TapSignerEnterPinView.swift
- rust/src/mnemonic/ffi.rs
- ios/Cove/Flows/SettingsFlow/WalletSettings/WalletSettingsView.swift
- rust/src/wallet/metadata.rs
- ios/Cove/Flows/TapSignerFlow/TapSignerConfirmPinView.swift
- rust/src/router.rs
- ios/Cove/Extention/WalletAddressType+Ext.swift
- rust/src/signed_import.rs
- ios/Cove/Flows/CoinControlFlow/UtxoRowPreview.swift
- ios/Cove/Extention/Color+Ext.swift
- android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt
- rust/src/multi_format.rs
- ios/Cove/Extention/Error+Ext.swift
Corrects a grammatical error in AfterPinAction::Sign in rust/src/router.rs by replacing "you need must enter" with "you must enter" so the security prompt before signing a transaction reads clearly.
Rename Rust FFI function and fix Swift call sites that referenced wrong generated binding names after the standalone-to-methods refactor.
Detect database key mismatch early in bootstrap before the panic in Database::global(). When the keychain master key doesn't match the encrypted database (e.g. after backup/restore, Xcode reinstall, or certificate re-provisioning), return AppInitError::DatabaseKeyMismatch instead of panicking with "HMAC mismatch". - Add verify_database_key() for lightweight header-only HMAC check - Add DatabaseKeyMismatch variant to AppInitError - Show blocking error screen on iOS with recovery contact info - Add keychain master key lookup logging to diagnose future key loss - Add V1→V2 header auto-upgrade support
|
@greptile-apps re-review |
1 participant
Convert standalone FFI functions to impl methods on their respective types where UniFFI supports it. This moves functions like fiatCurrencyEmoji() to FiatCurrency.emojiString(), hashRoute() to Route.hashValue(), walletMetadataIsEqual() to WalletMetadata.isEqual(), etc. Update Swift and Kotlin callers to use the new method syntax, remove wrapper extensions that are no longer needed, and regenerate bindings for both platforms.
Summary by CodeRabbit
Refactor
Display Updates
Bug Fixes