Repository navigation
UI bug fix import wallet - #475
Conversation
- Update SortRow to derive button state dynamically from Rust - Add sort state read to trigger Compose recomposition - Make sort property public with private set in CoinControlManager - Add enabled parameter to Continue button - Remove hardcoded arrow directions - now derived from ListSortDirection
Add wrapper functions to dispatch setWalletMetadata to background threads, preventing potential UI jank from database I/O: - iOS: setWalletMetadata dispatches to rustBridge queue - Android: persistWalletMetadata dispatches to ioScope - Android: Add ioScope for reconcile threading (matches iOS pattern) UI state updates remain on main thread, only the DB write moves.
Match iOS reconcile pattern: run FFI callbacks on background thread before dispatching to main thread for UI updates. This prevents potential ANR issues from blocking the main thread during reconcile. Managers updated: - SendFlowManager - CoinControlManager - AppManager - AuthManager - PendingWalletManager - ImportWalletManager
- Add guard against invalid maxWidthPx (0 or infinity) - Initialize optimalSize to max instead of min to prevent returning minimum font size when constraints aren't ready - Use finer 0.1sp steps instead of 0.5sp for less over-shrinking - Add all style dependencies to remember keys - Remove aggressive character-based pre-reduction in BalanceAutoSizeText, letting binary search find optimal size directly
- Add tight text styling to Typography with PlatformTextStyle and LineHeightStyle to remove extra font padding - Improve AutoSizeText binary search precision and add guard for invalid width - Revert BalanceWidget spacing to 6.dp now that typography matches iOS
Directly modify metadata instead of calling dispatch to prevent potential deadlock if this function is ever called from within a dispatch handler.
Add observer for selectedUnit changes that notifies SendFlowManager when unit changes, mirroring iOS .onChange behavior. Without this, the dropdown would update walletMetadata but the Rust backend was never notified to recalculate displayed amounts.
83a4d49 to
2035faa
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Greptile OverviewGreptile SummaryThis PR addresses UI bugs related to wallet import and improves perceived performance across the application. The changes focus on three main areas: Backend Performance Improvements:
iOS Wallet Import Keyboard Fix:
Fiat Balance Loading UX:
Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant UI as iOS/Android UI
participant WM as WalletManager
participant Wallet as Wallet (BDK)
participant FiatClient as FiatClient
participant DB as Database Cache
Note over UI,DB: Wallet Load Flow (Instant Cached Data)
UI->>WM: Load Wallet
WM->>Wallet: try_load_persisted()
Wallet-->>WM: wallet instance
WM->>Wallet: balance() (cached)
Wallet-->>WM: Balance
WM->>Wallet: transactions() (cached)
Wallet-->>WM: Vec<Transaction>
WM->>UI: Send cached balance & transactions
Note over UI: UI shows data immediately
WM->>WM: Start wallet scan (async)
Note over UI,DB: Fiat Balance Display Flow
UI->>WM: Display fiat balance
WM->>FiatClient: value_in_currency_cached()
FiatClient->>FiatClient: Check in-memory cache
alt Cache hit
FiatClient-->>WM: Some(fiat_value)
WM-->>UI: Display fiat amount
else Cache miss (in-memory)
FiatClient->>DB: get_prices()
alt DB cache hit
DB-->>FiatClient: Some(prices)
FiatClient-->>WM: Some(fiat_value)
WM-->>UI: Display fiat amount
else No cache
FiatClient-->>WM: None
WM-->>UI: Show ProgressView
end
end
Note over UI,DB: Price Update Flow
FiatClient->>FiatClient: Background fetch (60s interval)
FiatClient->>FiatClient: Retry with backon
FiatClient-->>UI: FiatPricesChanged event
UI->>WM: updateFiatBalance()
WM->>UI: Update displayed amounts
Note over UI,DB: Import Wallet Keyboard Flow
UI->>UI: User enters word in TextField
UI->>UI: Press Return key
UI->>UI: TextFieldReturnHandler.onReturn()
UI->>UI: Validate word
alt Valid word
UI->>UI: Set state to valid
UI->>UI: Move focus to next field
Note over UI: Keyboard stays visible
else Invalid word
UI->>UI: Set state to invalid
Note over UI: Stay on current field
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
ios/Cove/Components/KeyboardAccessory/KeyboardAccessoryHost.swift (1)
51-57: Naming inconsistency: observer name doesn't match notification type.
keyboardDidShowObserversubscribes tokeyboardWillShowNotification. Consider renaming tokeyboardWillShowObserverfor consistency and to avoid confusion.- private var keyboardDidShowObserver: NSObjectProtocol? + private var keyboardWillShowObserver: NSObjectProtocol?And update the corresponding references in
init()anddeinit.ios/Cove/Flows/SelectedWalletFlow/WalletBalanceHeaderView.swift (1)
177-181: Minor typo in commentThe comment on Line 178 has a small typo:
"pased"→"passed".- // if fiatBalance was pased in explicitly, don't update it, only for previews + // if fiatBalance was passed in explicitly, don't update it, only for previewsios/Cove/Flows/SelectedWalletFlow/SelectedWalletContainer.swift (1)
78-79: Verify that silent error handling is intentional.Scan failures are currently only logged without user-facing feedback. Please confirm this is the intended behavior—perhaps the UI's loading state or other mechanisms already surface wallet scan issues to users.
ios/Cove/Extention/KeyboardObserver.swift (2)
5-13: Observer token properties look good; consider excluding them from observationThe use of
@Observable+@MainActorandnonisolated(unsafe)for the notification tokens is a reasonable pattern given deinit isolation. If you don’t needshowObserver/hideObserverto participate in the observation graph, consider annotating them with@ObservationIgnoredto make the intent explicit and keep the observable surface limited to UI-relevant state (keyboardIsShowing/keyboardHeight).
16-29: Keyboard show/hide handling is correct; you may want to sync with system animation timingThe show/hide observers correctly hop back to the main actor and update both
keyboardIsShowingandkeyboardHeightwith animation. If you want tighter visual parity with the system keyboard animation (especially across devices and iPad configurations), you could pull the duration (and optionally curve) fromnotification.userInfoinstead of hard‑coding0.25, and reuse that value in both the show and hide branches.Also applies to: 32-45
rust/src/wallet.rs (1)
514-533: Consider optimization and deterministic sorting.The current implementation maps all transactions before filtering, which computes
sent_and_receivedfor transactions that may be discarded. Additionally,sort_unstable_bymay produce non-deterministic ordering for transactions with equal amounts.Consider this refactor for better efficiency and deterministic ordering:
pub fn transactions(&self) -> Vec<crate::transaction::Transaction> { use crate::transaction::{Amount, Transaction}; let zero = Amount::ZERO; let mut transactions = self .bdk .transactions() .map(|tx| { let sent_and_received = self.bdk.sent_and_received(&tx.tx_node.tx).into(); (tx, sent_and_received) }) + // Filter early to avoid unnecessary Transaction::new calls + .filter(|(_, sent_and_received)| sent_and_received.amount() > zero) .map(|(tx, sent_and_received)| Transaction::new(&self.id, sent_and_received, tx)) - .filter(|tx| tx.sent_and_received().amount() > zero) .collect::<Vec<Transaction>>(); - transactions.sort_unstable_by(|a, b| a.cmp(b).reverse()); + // Use stable sort for deterministic ordering + transactions.sort_by(|a, b| a.cmp(b).reverse()); transactions }rust/src/app.rs (1)
522-539: Consider the 120-second retry delay for production.The fiat price initialization now uses a
ConstantBuilderwith a 120-second delay between retries (up to 5 attempts, totaling ~10 minutes). This seems excessive for an app initialization path. If the first few attempts fail, users may experience a degraded experience for an extended period.Consider using an exponential backoff similar to other retry logic in the codebase:
- let init_result = (|| crate::fiat::client::init_prices()) - .retry( - ConstantBuilder::default() - .with_delay(Duration::from_secs(120)) - .with_max_times(5), - ) - .notify(|err, _| warn!("unable to init prices: {err}, trying again")) - .await; + let init_result = (|| crate::fiat::client::init_prices()) + .retry( + backon::ExponentialBuilder::default() + .with_min_delay(Duration::from_secs(5)) + .with_max_delay(Duration::from_secs(60)) + .with_max_times(5), + ) + .notify(|err, _| warn!("unable to init prices: {err}, trying again")) + .await;This would retry faster initially (5s, 10s, 20s, 40s, 60s) while still backing off on persistent failures.
rust/src/fiat/client.rs (2)
126-131: Consider logging database errors instead of silently ignoring them.The
if let Ok(Some(prices))pattern silently discards any database errors. Consider logging errors to aid debugging:- // fallback to database cache - if let Ok(Some(prices)) = Database::global().global_cache.get_prices() { + // fallback to database cache + match Database::global().global_cache.get_prices() { + Ok(Some(prices)) => { debug!("loaded cached prices from database"); PRICES.swap(Arc::new(Some(prices))); return Some(prices); + } + Ok(None) => {} + Err(e) => { + warn!("failed to load cached prices from database: {e:?}"); + } }
244-262: Minor redundancy: Double HARD_LIMIT check.The function checks
HARD_LIMITat line 248, then callsget_or_fetch_prices()which checks it again at line 142. This is harmless but creates slight redundancy. Consider if the outer check is still valuable or could be removed.Otherwise, the retry configuration with 5 attempts and 1s max delay is appropriate for a background refresh operation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt(8 hunks)ios/Cove.xcodeproj/project.pbxproj(8 hunks)ios/Cove.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved(2 hunks)ios/Cove/AppManager.swift(3 hunks)ios/Cove/Components/KeyboardAccessory/KeyboardAccessoryHost.swift(2 hunks)ios/Cove/Extention/KeyboardObserver.swift(1 hunks)ios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletImportCard.swift(4 hunks)ios/Cove/Flows/SelectedWalletFlow/SelectedWalletContainer.swift(1 hunks)ios/Cove/Flows/SelectedWalletFlow/WalletBalanceHeaderView.swift(3 hunks)ios/Cove/WalletManager.swift(4 hunks)rust/Cargo.toml(1 hunks)rust/src/app.rs(4 hunks)rust/src/fee_client.rs(2 hunks)rust/src/fiat/client.rs(6 hunks)rust/src/manager/wallet_manager.rs(2 hunks)rust/src/router.rs(3 hunks)rust/src/wallet.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
rust/**/*.rs
⚙️ CodeRabbit configuration file
rust/**/*.rs: - Verify correct functionality and logic consistency.
- Check for idiomatic Rust usage and performance best practices.
- Suggest improvements to error handling, unwraps, and Result handling.
- Check for any potential memory leaks or unsafe code.
- Check for any potential mutex deadlocks.
- Check for potential security issues, make sure Bitcoin wallets are handled securely.
- Identify spelling mistakes in comments, string literals, and documentation
Files:
rust/src/wallet.rsrust/src/router.rsrust/src/fee_client.rsrust/src/app.rsrust/src/fiat/client.rsrust/src/manager/wallet_manager.rs
ios/Cove/**/*.swift
⚙️ CodeRabbit configuration file
ios/Cove/**/*.swift: - Review SwiftUI view code for proper layout, best practices
- Identify spelling mistakes in comments, string literals, and documentation
- Ignore generated bindings code in ios/CoveCore/Sources/CoveCore/generated/**
Files:
ios/Cove/Flows/SelectedWalletFlow/SelectedWalletContainer.swiftios/Cove/Components/KeyboardAccessory/KeyboardAccessoryHost.swiftios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletImportCard.swiftios/Cove/AppManager.swiftios/Cove/Flows/SelectedWalletFlow/WalletBalanceHeaderView.swiftios/Cove/WalletManager.swiftios/Cove/Extention/KeyboardObserver.swift
🧬 Code graph analysis (6)
rust/src/wallet.rs (2)
rust/src/transaction/ffi.rs (2)
sent_and_received(189-191)sent_and_received(212-214)rust/src/transaction.rs (2)
sent_and_received(111-116)new(67-109)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (5)
android/app/src/main/java/org/bitcoinppl/cove_core/types/cove_types.kt (17)
callWithHandle(2377-2398)callWithHandle(2727-2748)callWithHandle(3011-3032)callWithHandle(3327-3348)callWithHandle(3630-3651)callWithHandle(4000-4021)callWithHandle(4282-4303)callWithHandle(4780-4801)callWithHandle(5055-5076)callWithHandle(5379-5400)callWithHandle(5724-5745)callWithHandle(6027-6048)callWithHandle(6401-6422)callWithHandle(6646-6667)callWithHandle(6994-7015)callWithHandle(7334-7355)uniffiRustCall(273-275)android/app/src/main/java/org/bitcoinppl/cove_core/nfc/cove_nfc.kt (1)
uniffiRustCall(273-275)android/app/src/main/java/org/bitcoinppl/cove_core/ur/cove_ur.kt (1)
uniffiRustCall(273-275)android/app/src/main/java/uniffi/cove_ur/cove_ur.kt (1)
uniffiRustCall(273-275)android/app/src/main/java/org/bitcoinppl/cove_core/device/cove_device.kt (1)
uniffiRustCall(273-275)
ios/Cove/AppManager.swift (2)
ios/Cove/WalletManager.swift (1)
updateFiatBalance(140-142)android/app/src/main/java/org/bitcoinppl/cove/WalletManager.kt (1)
updateFiatBalance(194-206)
rust/src/app.rs (1)
rust/src/fiat/client.rs (2)
init_prices(198-230)prices(115-135)
rust/src/fiat/client.rs (2)
rust/src/app.rs (6)
new(42-44)new(80-116)prices(124-126)prices(471-473)global(119-121)global(253-255)rust/src/database.rs (2)
new(52-54)global(87-91)
ios/Cove/Flows/SelectedWalletFlow/WalletBalanceHeaderView.swift (2)
ios/CoveCore/Sources/CoveCore/generated/cove.swift (4)
displayFiatAmount(7293-7301)displayFiatAmount(7899-7907)amount(7258-7264)amount(9115-9121)ios/Cove/WalletManager.swift (1)
amountFmtUnit(118-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: swiftformat
- GitHub Check: clippy
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: compile-ios
- GitHub Check: compile-android
- GitHub Check: ktlint
🔇 Additional comments (30)
ios/Cove/Components/KeyboardAccessory/KeyboardAccessoryHost.swift (2)
89-104: Well-structured keyboard reattachment logic.The approach of clearing the accessory before re-setting it ensures a clean state on each focus change. Using
keyboardWillHideto track detachment andkeyboardWillShowto reattach handles the focus-transition edge cases correctly.
106-116: Observer cleanup is correct.All three observers are properly removed in
deinit, preventing potential crashes or memory leaks.android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (4)
2339-2340: Verify FFI signature is consistent with Rust side changes.The external function signature now returns
RustBuffer.ByValuewith anuniffi_out_errparameter, indicating a shift from async future-based FFI to synchronous error-returning calls. Ensure this matches the corresponding Rust function signature change (amount_in_fiat).
18125-18137: Implementation pattern is correct.The synchronous implementation correctly uses the
callWithHandle { uniffiRustCall { ... } }pattern:
callWithHandleensures object lifetime safety via atomic call counter increment/decrement (per relevant snippets).uniffiRustCallprovides inline error handling viaUniffiRustCallStatus.FfiConverterOptionalDouble.lift()properly converts theRustBuffer.ByValueresult.The nesting and error-handling flow align with patterns in other generated FFI files (e.g.,
cove_nfc.kt,cove_device.kt).
42518-42545:FfiConverterOptionalDoubleimplementation is correct.The converter properly encodes/decodes Rust
Option<f64>as a single-byte discriminant followed by optional data:
- read(): Discriminant byte 0 = null, non-zero = unwrap and read Double
- allocationSize(): 1 byte discriminant + (Double size if non-null)
- write(): 0 for null, 1 + Double value for non-null
This matches the standard FFI pattern for optional types and is consistent with existing converters in the file (e.g.,
FfiConverterOptionalLong).
17890-17893: Function signature is already synchronous with proper call site compatibility.The current code shows
amountInFiatis defined as a regular synchronous function (fun 'amountInFiat'('amount': Amount): kotlin.Double?), and the single call site inWalletTransactionsScreen.kt(line 822) is already using it correctly without.await()and properly handling the null return value via try-catch. No.await()patterns exist in the codebase for this function, and no breaking changes are evident in the current state.Likely an incorrect or invalid review comment.
ios/Cove/Flows/SelectedWalletFlow/WalletBalanceHeaderView.swift (3)
23-37: Primary balance view builder change looks solidThe new
primaryBalanceViewcleanly handles sensitivity masking, fiat vs BTC switching, and introduces a proper loading state viaProgressViewinstead of empty strings. This is a clear improvement in readability and UX.
39-54: Secondary balance view logic correctly mirrors primary with fiat fallback
secondaryBalanceViewcorrectly shows masked content when hidden, fiat-as-secondary when BTC is primary (with a loading spinner), and BTC/sats when fiat is primary. The split between primary/secondary roles is clear and matches the intended toggle behavior.
80-85: Good use ofcontentTransition(.numericText())around the new viewsApplying
.contentTransition(.numericText())to the containers usingprimaryBalanceViewandsecondaryBalanceViewis appropriate for smooth numeric updates as balances change, and works well with the conditionalText/ProgressViewproduced by the view builders.Also applies to: 90-94
ios/Cove/Flows/SelectedWalletFlow/SelectedWalletContainer.swift (1)
73-82: LGTM! Immediate scan improves UX.The removal of artificial delays and immediate invocation of
startWalletScan()is a solid improvement. Starting the scan right away with cached data first makes the UI feel more responsive. Theif let managercheck appropriately handles cases where manager initialization might have failed.ios/Cove/Extention/KeyboardObserver.swift (1)
48-51: Observer cleanup indeinitis implemented correctlyStoring the
NSObjectProtocoltokens and removing them indeinitavoids leaks and stray callbacks fromNotificationCenterafterKeyboardObserveris deallocated. This matches the block‑based observer API’s lifecycle expectations.ios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletImportCard.swift (3)
302-312: Potential stale capture ofallEnteredWordsin closure.The closure
{ [self] in submitFocusField() }is created once whenreturnHandler == nil. This capturesallEnteredWords(a let constant) at creation time. WhensubmitFocusField()later callsautocomplete.isValidWord(word: text, allWords: allEnteredWords)andautocomplete.nextFieldNumber(...), it uses the stale captured value rather than the current array.Consider restructuring to avoid capturing view constants, or verify that stale
allEnteredWordsdoesn't affect validation correctness.
14-56: TextFieldReturnHandler delegate forwarding looks correct.The delegate wrapper properly uses
weak varfororiginalDelegateto avoid retain cycles, and forwards the most commonly usedUITextFieldDelegatemethods. One minor omission istextFieldDidEndEditing(_:reason:)(iOS 10+), but the simplertextFieldDidEndEditing(_:)is forwarded, which should suffice for most use cases.
238-270: Validation logic before advancing looks good.The updated
submitFocusField()correctly validates the word before advancing focus: empty input stays in typing state, invalid words are marked invalid without advancing, and valid words advance to the next field. This prevents navigation to subsequent fields until the current word is valid.ios/Cove.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
22-30: SwiftUIIntrospect dependency added correctly.The package pin for
swiftui-introspectat version 26.0.0 is properly configured with the correct repository URL and revision hash.ios/Cove.xcodeproj/project.pbxproj (2)
10-10: SwiftUIIntrospect package integration is correct.The project file correctly adds SwiftUIIntrospect as a remote Swift package dependency with build file references, framework linkage, and package product dependencies properly configured.
Also applies to: 58-58, 108-108, 142-142, 431-438, 458-462
309-309: Project version bump to 49 is appropriate.Version increment from 48 to 49 reflects the changes in this PR.
Also applies to: 359-359
rust/src/fee_client.rs (1)
153-160: LGTM! Retry logic successfully migrated to backon.The migration from tryhard to backon preserves the same retry behavior (exponential backoff with 10ms-5s delays, max 20 attempts). The implementation is clean and consistent with the broader PR migration strategy.
ios/Cove/AppManager.swift (2)
4-4: Good practice: centralized delay constant.Extracting the delay value to a named constant improves maintainability and makes the intent clearer.
244-244: LGTM! Synchronous fiat balance update on price changes.The added call to
updateFiatBalance()ensures the wallet fiat balance is refreshed when prices change, improving UI consistency. This aligns with the broader shift to synchronous fiat balance updates in the PR.rust/src/router.rs (1)
3-4: Verify the reduced delay values don't cause UI issues.The PR consolidates delays to a single constant (good practice), but significantly reduces them from 500-800ms to 250ms. This 50-68% reduction might cause UI flickering, loading screen artifacts, or race conditions if the original delays were intentional buffers for async operations.
Verify through testing that:
- Loading screens display long enough to avoid flicker
- Route transitions complete cleanly without race conditions
- User experience remains smooth with the shorter delays
If any issues arise, consider increasing
LOAD_AND_RESET_DELAY_MSor making it configurable per use case.Also applies to: 232-232, 306-306, 310-310
rust/src/app.rs (1)
410-410: LGTM! Consistent use of centralized delay constant.Using
LOAD_AND_RESET_DELAY_MSinstead of a hardcoded value improves maintainability and consistency across the codebase.ios/Cove/WalletManager.swift (1)
40-40: LGTM! Consistent async-to-sync migration for fiat balance.The conversion of
updateFiatBalance()from async to synchronous is implemented consistently across all call sites. The new implementation usesrust.amountInFiat()which relies on cached fiat prices, eliminating the need for async/await.Verify that
rust.amountInFiat()never blocks the main thread for an extended period. If the cached prices are unavailable or the computation is complex, this could cause UI freezes. Based on the Rust code review, it appears to usevalue_in_currency_cached(), which should be fast, but confirming this in testing is prudent.Also applies to: 52-52, 140-142, 146-149, 178-178
rust/src/manager/wallet_manager.rs (2)
248-254: Excellent UX improvement: early cached data notification.Sending cached balance and transactions immediately before spawning the actor provides faster initial UI feedback. This aligns well with the broader shift to cached data patterns in the PR.
509-514: LGTM! Simplified synchronous fiat conversion API.The new
amount_in_fiat()method is cleaner than the previous async approach:
- Returns
Option<f64>instead ofResult<f64, Error>(simpler error handling)- Uses cached prices via
value_in_currency_cached()(no async overhead)- Clear semantics:
Nonemeans no cached prices availableThis is a breaking API change, but the migration appears complete across the codebase (Swift side updated to call this method).
rust/src/fiat/client.rs (5)
21-25: LGTM! Clear and well-documented constants.The refresh interval and hard limit constants are appropriately named and documented. The values (60s background refresh, 30s hard limit) provide reasonable rate limiting for the price API.
82-89: LGTM!Clean and simplified construction. The
new_with_urlhelper for testing is appropriately marked as#[allow(dead_code)].
91-97: LGTM!Clean implementation with proper
Optionreturn type for cached-only lookups. The calculationbtc * price as f64is straightforward and correct.
202-209: LGTM! Good retry configuration for initialization.The exponential backoff with 20 retries and max delay of 5s is appropriate for app initialization where we want to be persistent in getting initial prices. The fallback to database cache on failure provides good resilience.
381-384: LGTM!Simple and correct FFI-exported comparison function. The
Arcsmart pointers are correctly dereferenced for value comparison via the derivedPartialEqimplementation.
| async-trait = "0.1.81" | ||
| futures = "0.3.30" | ||
| tryhard = "0.5" | ||
| backon = "1.3" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check backon crate information from crates.io
curl -s https://crates.io/api/v1/crates/backon | jq '.crate | {max_version, newest_version, name}'
# Also get all recent versions to see the version history
curl -s https://crates.io/api/v1/crates/backon/versions | jq '.versions[] | {num, created_at}' | head -30Repository: bitcoinppl/cove
Length of output: 632
🏁 Script executed:
#!/bin/bash
# Check if there are any known security advisories for backon
# Using rust security advisory database via cargo-audit approach
pip install requests
python3 <<'EOF'
import requests
import json
# Try to get advisory information from rustsec database
response = requests.get("https://api.rustsec.org/osv/query?package=backon")
if response.status_code == 200:
data = response.json()
print("Advisories found:")
print(json.dumps(data, indent=2))
else:
print(f"No advisories returned (status: {response.status_code})")
EOFRepository: bitcoinppl/cove
Length of output: 145
Update backon dependency to a more recent version.
Version 1.3 is valid but outdated—version 1.3.0 was released in November 2024. The latest available version is 1.6.0 (October 2025), with several intermediate releases (1.4.0, 1.4.1, 1.5.0, 1.5.1, 1.5.2) containing bug fixes and improvements. Consider updating to 1.6.0 or at least 1.5.x to benefit from recent updates.
🤖 Prompt for AI Agents
In rust/Cargo.toml around line 143, the backon dependency is pinned to "1.3"
which is outdated; update the version to a newer release (preferably "1.6.0", or
at minimum a 1.5.x series such as "1.5.2") by changing the version string in
Cargo.toml, then run cargo update (and cargo build/test) to refresh Cargo.lock
and verify there are no breaking changes or new warnings; if your project
requires strict semver, consider using a caret version (^1.6 or ^1.5) to allow
patch/minor updates.
| if let Some(prices) = PRICES.load().as_ref() { | ||
| let now_secs = Timestamp::now().as_second() as u64; | ||
| if now_secs - prices.fetched_at < self.wait_before_new_prices { | ||
| if now_secs - prices.fetched_at < HARD_LIMIT { | ||
| return Ok(*prices); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential underflow if fetched_at is in the future due to clock skew.
If prices.fetched_at > now_secs (possible with clock drift or NTP adjustments), the subtraction will underflow since both are u64. Consider using saturating_sub or checked_sub:
if let Some(prices) = PRICES.load().as_ref() {
let now_secs = Timestamp::now().as_second() as u64;
- if now_secs - prices.fetched_at < HARD_LIMIT {
+ if now_secs.saturating_sub(prices.fetched_at) < HARD_LIMIT {
return Ok(*prices);
}
}The same pattern appears at lines 118-119 and 247-248.
🤖 Prompt for AI Agents
In rust/src/fiat/client.rs around lines 140-145 (also applies to 118-119 and
247-248), the subtraction now_secs - prices.fetched_at can underflow if
fetched_at is in the future; update the check to use a non-panicking safe
subtraction (e.g., now_secs.saturating_sub(prices.fetched_at) < HARD_LIMIT or
use checked_sub and treat a None as stale) so future timestamps don’t underflow
and stale logic remains correct, and apply the same fix at the other two
locations.
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Chores
✏️ Tip: You can customize this high-level summary in your review settings.