Feat cache receive addresses with 5m timeout#666
Feat cache receive addresses with 5m timeout#666oyetanishq wants to merge 1 commit intobitcoinppl:masterfrom
5m timeout#666Conversation
|
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:
📝 WalkthroughWalkthroughAdds UniFFI-backed getReceiveAddress with a 5-minute receive-address cache and generation mutex in Rust; surfaces addressGeneratedTime through WalletManager across Android and iOS; UI components show a countdown and auto-refresh when the interval expires or forceNew is requested. Changes
Sequence DiagramsequenceDiagram
participant UI as UI Layer
participant WM as WalletManager (host)
participant Cache as GlobalCache/DB
participant Rust as Rust Backend/Actor
rect rgba(100,150,200,0.5)
Note over UI,WM: First request (no cache)
UI->>WM: getReceiveAddress(forceNew=false)
WM->>Cache: get_receive_address(wallet_id)
Cache-->>WM: None
WM->>Rust: next_address()/generate
Rust-->>WM: AddressInfoWithDerivation
WM->>Cache: set_receive_address(wallet_id, {address, generated_at, tx_count})
WM-->>UI: AddressInfo + addressGeneratedTime
UI->>UI: Start 5-minute countdown
end
rect rgba(150,200,100,0.5)
Note over UI,WM: Subsequent request within 5 min, tx_count unchanged
UI->>WM: getReceiveAddress(forceNew=false)
WM->>Cache: get_receive_address(wallet_id)
Cache-->>WM: Cached entry (age < 5m, tx_count same)
WM-->>UI: Cached AddressInfo + addressGeneratedTime
UI->>UI: Continue countdown
end
rect rgba(200,150,100,0.5)
Note over UI,WM: After timeout, tx change, or forceNew
UI->>WM: getReceiveAddress(forceNew=true/expired)
WM->>Cache: get_receive_address(wallet_id)
Cache-->>WM: Expired OR tx_count changed OR forceNew
WM->>Rust: next_address()/generate
Rust-->>WM: AddressInfoWithDerivation
WM->>Cache: set_receive_address(wallet_id, updated...)
WM-->>UI: New AddressInfo + new timestamp
UI->>UI: Reset countdown
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 introduces address caching to both iOS and Android Confidence Score: 4/5Safe to merge after fixing the hardcoded One P1 logic issue: iOS ios/Cove/WalletManager.swift — line 49 needs Important Files Changed
Sequence DiagramsequenceDiagram
participant V as ReceiveView / ReceiveAddressSheet
participant WM as WalletManager (cache)
participant R as rust.nextAddress()
participant T as Timer (1s tick)
V->>WM: getReceiveAddress(forceNew: false)
alt cache miss (null / expired / tx-count changed)
WM->>R: nextAddress()
R-->>WM: newAddress
WM->>WM: update cachedReceiveAddress,<br/>addressGeneratedTime, lastTxCountForAddress
end
WM-->>V: AddressInfoWithDerivation
loop every 1 second
T->>V: tick
V->>WM: read addressGeneratedTime
V->>V: compute remaining → update timeRemainingString
end
V->>WM: getReceiveAddress(forceNew: true)
WM->>R: nextAddress()
R-->>WM: newAddress
WM->>WM: update cache + addressGeneratedTime
WM-->>V: AddressInfoWithDerivation
note over V: LaunchedEffect(addressGeneratedTime)<br/>restarts countdown coroutine
Reviews (1): Last reviewed commit: "Feat cache receive addresses with `5m ti..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| LaunchedEffect(addressGeneratedTime) { | ||
| while(true) { | ||
| if (addressGeneratedTime == 0L) { | ||
| timeLeftStr = "" | ||
| } else { | ||
| val elapsed = System.currentTimeMillis() - addressGeneratedTime | ||
| val remaining = WalletManager.WaitTimeToGenerateNewAddress - elapsed | ||
|
|
||
| if (remaining > 0) { | ||
| val minutes = (remaining / 1000) / 60 | ||
| val seconds = (remaining / 1000) % 60 | ||
| timeLeftStr = String.format("Auto-refresh in %02d:%02d", minutes, seconds) | ||
| } else { | ||
| timeLeftStr = "Address Expired" | ||
| } | ||
| } | ||
| kotlinx.coroutines.delay(1000) | ||
| } | ||
| } |
There was a problem hiding this comment.
Once remaining <= 0, the loop continues firing every second, repeatedly assigning the same "Address Expired" string. After reaching the expired state the coroutine has no more useful work to do. Adding a break avoids the unnecessary work:
| LaunchedEffect(addressGeneratedTime) { | |
| while(true) { | |
| if (addressGeneratedTime == 0L) { | |
| timeLeftStr = "" | |
| } else { | |
| val elapsed = System.currentTimeMillis() - addressGeneratedTime | |
| val remaining = WalletManager.WaitTimeToGenerateNewAddress - elapsed | |
| if (remaining > 0) { | |
| val minutes = (remaining / 1000) / 60 | |
| val seconds = (remaining / 1000) % 60 | |
| timeLeftStr = String.format("Auto-refresh in %02d:%02d", minutes, seconds) | |
| } else { | |
| timeLeftStr = "Address Expired" | |
| } | |
| } | |
| kotlinx.coroutines.delay(1000) | |
| } | |
| } | |
| LaunchedEffect(addressGeneratedTime) { | |
| while (true) { | |
| if (addressGeneratedTime == 0L) { | |
| timeLeftStr = "" | |
| } else { | |
| val elapsed = System.currentTimeMillis() - addressGeneratedTime | |
| val remaining = WalletManager.WaitTimeToGenerateNewAddress - elapsed | |
| if (remaining > 0) { | |
| val minutes = (remaining / 1000) / 60 | |
| val seconds = (remaining / 1000) % 60 | |
| timeLeftStr = String.format("Auto-refresh in %02d:%02d", minutes, seconds) | |
| } else { | |
| timeLeftStr = "Address Expired" | |
| break | |
| } | |
| } | |
| kotlinx.coroutines.delay(1000) | |
| } | |
| } |
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
good start and thanks for the screenshots, We are only right now saving this in memory, so if the user exits and comes back, this will be lost. And a new address will be generated.So let's use the redb cache table. to save this in on on disk. So handle the save as much of the logic as possible in Rust. Read architecture.md. Also only show the countdown timer if it is within one minute of expiring and make that text uh read w but at some opacity.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt (1)
72-83:⚠️ Potential issue | 🟠 MajorWatch the wallet state that invalidates the cache.
This effect only reruns when
managerchanges. If the wallet's transaction count changes after the sheet is already visible,manager.getReceiveAddress(forceNew = false)is never called again, so the old address stays on screen even thoughWalletManager.ktlines 106-122 now consider that cache invalid.Possible fix
- LaunchedEffect(manager) { + LaunchedEffect(manager, manager.loadState) { try { isLoading = true addressInfo = manager.getReceiveAddress(forceNew = false)As per coding guidelines,
LaunchedEffect should be keyed to actual dependencies, not Unit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt` around lines 72 - 83, The LaunchedEffect that calls manager.getReceiveAddress(forceNew = false) is only keyed to manager, so it won't rerun when the wallet cache becomes invalid due to transaction/UTXO changes; update the effect's key to include the wallet state that invalidates the cache (e.g. the wallet/transactionCount/receiveAddressNonce or an exposed invalidate signal) so the effect reruns when that value changes, and ensure the manager.getReceiveAddress call and the isLoading/errorMessage/addressInfo state updates remain inside that keyed LaunchedEffect (referencing the existing LaunchedEffect block and manager.getReceiveAddress).
🧹 Nitpick comments (1)
ios/Cove/WalletManager.swift (1)
37-50: UsewaitTimeToGenerateNewAddressin the expiry check.Line 49 hard-codes
300even though the view readswaitTimeToGenerateNewAddress, so the cache policy and countdown will diverge the moment this timeout changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/WalletManager.swift` around lines 37 - 50, The expiry check in getReceiveAddress currently uses a hard-coded 300 seconds; update it to use the waitTimeToGenerateNewAddress property so the cache policy and UI countdown stay in sync. Locate getReceiveAddress and replace the timeElapsed calculation that references addressGeneratedTime (currently comparing > 300) to compare against waitTimeToGenerateNewAddress instead, ensuring types match (Double) and preserving the optional mapping behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt`:
- Around line 165-168: The String.format call in ReceiveAddressSheet (where
timeLeftStr is set) is using the platform default locale; change it to use an
explicit Locale.US by passing Locale.US as the first argument to String.format
to match the codebase convention (update the String.format(...) call in
ReceiveAddressSheet.kt where timeLeftStr is assigned).
In `@android/app/src/main/java/org/bitcoinppl/cove/WalletManager.kt`:
- Around line 106-122: Guard the cache check/update in
WalletManager.getReceiveAddress with a coroutine Mutex to prevent two coroutines
from both calling rust.nextAddress(); add a private val addressMutex = Mutex()
and wrap the logic that checks cachedReceiveAddress, computes
timeElapsed/txCountChanged, calls rust.nextAddress(), and updates
cachedReceiveAddress/addressGeneratedTime/lastTxCountForAddress inside
addressMutex.withLock { ... }; ensure you return the cached value after the lock
so only one coroutine can rotate the address and update the cache at a time.
In `@ios/Cove/Flows/SelectedWalletFlow/ReceiveView.swift`:
- Around line 146-165: The view currently only fetches an address on appearance;
add a reactive re-fetch when the wallet state changes by observing the manager's
load/transaction state and calling nextAddress() again so
manager.getReceiveAddress(forceNew:) runs when the cache is invalidated.
Concretely, attach an .onChange or .onReceive for manager.loadState (or the
published tx-count/loading property in WalletManager) and trigger Task { await
nextAddress() } (or equivalent) so the address updates while the view remains
open; keep the existing timer logic unchanged.
In `@ios/Cove/WalletManager.swift`:
- Around line 47-63: getReceiveAddress currently has a race where concurrent
callers can both observe a miss/expiry and call rust.nextAddress(), rotating the
address twice; serialize and dedupe generation by funneling the entire
check/generate/store flow through a single executor: introduce an in-flight
Task<> (e.g. inFlightReceiveAddressTask) or use a private actor/serial
DispatchQueue to ensure only one rust.nextAddress() runs at a time, have callers
await that shared Task when present, and update cachedReceiveAddress,
addressGeneratedTime, and lastTxCountForAddress only once the Task completes;
keep the existing checks (cachedReceiveAddress, forceNew, addressGeneratedTime,
currentTxCount vs lastTxCountForAddress) but make their decision and the
subsequent call to rust.nextAddress() atomic by using the new in-flight
Task/actor surrounding the getReceiveAddress flow.
---
Outside diff comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt`:
- Around line 72-83: The LaunchedEffect that calls
manager.getReceiveAddress(forceNew = false) is only keyed to manager, so it
won't rerun when the wallet cache becomes invalid due to transaction/UTXO
changes; update the effect's key to include the wallet state that invalidates
the cache (e.g. the wallet/transactionCount/receiveAddressNonce or an exposed
invalidate signal) so the effect reruns when that value changes, and ensure the
manager.getReceiveAddress call and the isLoading/errorMessage/addressInfo state
updates remain inside that keyed LaunchedEffect (referencing the existing
LaunchedEffect block and manager.getReceiveAddress).
---
Nitpick comments:
In `@ios/Cove/WalletManager.swift`:
- Around line 37-50: The expiry check in getReceiveAddress currently uses a
hard-coded 300 seconds; update it to use the waitTimeToGenerateNewAddress
property so the cache policy and UI countdown stay in sync. Locate
getReceiveAddress and replace the timeElapsed calculation that references
addressGeneratedTime (currently comparing > 300) to compare against
waitTimeToGenerateNewAddress instead, ensuring types match (Double) and
preserving the optional mapping behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d5b166d-af67-439c-9af7-f9c3c7ca1a2d
📒 Files selected for processing (4)
android/app/src/main/java/org/bitcoinppl/cove/WalletManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.ktios/Cove/Flows/SelectedWalletFlow/ReceiveView.swiftios/Cove/WalletManager.swift
| suspend fun getReceiveAddress(forceNew: Boolean = false): AddressInfoWithDerivation { | ||
| val now = System.currentTimeMillis() | ||
| val timeElapsed = (now - addressGeneratedTime) > WaitTimeToGenerateNewAddress | ||
| val txCountChanged = currentTxCount != lastTxCountForAddress | ||
|
|
||
| if (cachedReceiveAddress == null || forceNew || timeElapsed || txCountChanged) { | ||
| val newAddress = rust.nextAddress() | ||
| withContext(Dispatchers.Main) { | ||
| cachedReceiveAddress = newAddress | ||
| addressGeneratedTime = now | ||
| lastTxCountForAddress = currentTxCount | ||
| } | ||
| return newAddress | ||
| } | ||
|
|
||
| return cachedReceiveAddress!! | ||
| } |
There was a problem hiding this comment.
Guard the cache check/update with a mutex.
Two coroutines can hit this method together, both see the cache as missing/expired, and both call rust.nextAddress(). That breaks the reuse guarantee and can rotate the receive address twice before either caller stores cachedReceiveAddress.
Possible fix
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
+
class WalletManager :
WalletManagerReconciler,
Closeable {
+ private val receiveAddressMutex = Mutex()
+
suspend fun getReceiveAddress(forceNew: Boolean = false): AddressInfoWithDerivation {
- val now = System.currentTimeMillis()
- val timeElapsed = (now - addressGeneratedTime) > WaitTimeToGenerateNewAddress
- val txCountChanged = currentTxCount != lastTxCountForAddress
-
- if (cachedReceiveAddress == null || forceNew || timeElapsed || txCountChanged) {
- val newAddress = rust.nextAddress()
- withContext(Dispatchers.Main) {
- cachedReceiveAddress = newAddress
- addressGeneratedTime = now
- lastTxCountForAddress = currentTxCount
+ return receiveAddressMutex.withLock {
+ val now = System.currentTimeMillis()
+ val timeElapsed = (now - addressGeneratedTime) > WaitTimeToGenerateNewAddress
+ val txCountChanged = currentTxCount != lastTxCountForAddress
+
+ if (cachedReceiveAddress == null || forceNew || timeElapsed || txCountChanged) {
+ val newAddress = rust.nextAddress()
+ withContext(Dispatchers.Main) {
+ cachedReceiveAddress = newAddress
+ addressGeneratedTime = now
+ lastTxCountForAddress = currentTxCount
+ }
+ return@withLock newAddress
}
- return newAddress
- }
-
- return cachedReceiveAddress!!
+ cachedReceiveAddress!!
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| suspend fun getReceiveAddress(forceNew: Boolean = false): AddressInfoWithDerivation { | |
| val now = System.currentTimeMillis() | |
| val timeElapsed = (now - addressGeneratedTime) > WaitTimeToGenerateNewAddress | |
| val txCountChanged = currentTxCount != lastTxCountForAddress | |
| if (cachedReceiveAddress == null || forceNew || timeElapsed || txCountChanged) { | |
| val newAddress = rust.nextAddress() | |
| withContext(Dispatchers.Main) { | |
| cachedReceiveAddress = newAddress | |
| addressGeneratedTime = now | |
| lastTxCountForAddress = currentTxCount | |
| } | |
| return newAddress | |
| } | |
| return cachedReceiveAddress!! | |
| } | |
| suspend fun getReceiveAddress(forceNew: Boolean = false): AddressInfoWithDerivation { | |
| return receiveAddressMutex.withLock { | |
| val now = System.currentTimeMillis() | |
| val timeElapsed = (now - addressGeneratedTime) > WaitTimeToGenerateNewAddress | |
| val txCountChanged = currentTxCount != lastTxCountForAddress | |
| if (cachedReceiveAddress == null || forceNew || timeElapsed || txCountChanged) { | |
| val newAddress = rust.nextAddress() | |
| withContext(Dispatchers.Main) { | |
| cachedReceiveAddress = newAddress | |
| addressGeneratedTime = now | |
| lastTxCountForAddress = currentTxCount | |
| } | |
| return@withLock newAddress | |
| } | |
| cachedReceiveAddress!! | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/app/src/main/java/org/bitcoinppl/cove/WalletManager.kt` around lines
106 - 122, Guard the cache check/update in WalletManager.getReceiveAddress with
a coroutine Mutex to prevent two coroutines from both calling
rust.nextAddress(); add a private val addressMutex = Mutex() and wrap the logic
that checks cachedReceiveAddress, computes timeElapsed/txCountChanged, calls
rust.nextAddress(), and updates
cachedReceiveAddress/addressGeneratedTime/lastTxCountForAddress inside
addressMutex.withLock { ... }; ensure you return the cached value after the lock
so only one coroutine can rotate the address and update the cache at a time.
| func getReceiveAddress(forceNew: Bool = false) async throws -> AddressInfoWithDerivation { | ||
| let now = Date() | ||
| let timeElapsed = addressGeneratedTime.map { now.timeIntervalSince($0) > 300 } ?? true | ||
| let txCountChanged = currentTxCount != lastTxCountForAddress | ||
|
|
||
| if cachedReceiveAddress == nil || forceNew || timeElapsed || txCountChanged { | ||
| let newAddress = try await rust.nextAddress() | ||
| await MainActor.run { | ||
| self.cachedReceiveAddress = newAddress | ||
| self.addressGeneratedTime = now // Update exposed state | ||
| self.lastTxCountForAddress = self.currentTxCount | ||
| } | ||
| return newAddress | ||
| } | ||
|
|
||
| return cachedReceiveAddress! | ||
| } |
There was a problem hiding this comment.
Serialize receive-address generation.
This check/generate/store path is race-prone as written: two overlapping callers can both observe a miss/expiry here and each call rust.nextAddress(), which rotates twice and breaks the 5-minute reuse guarantee. Please funnel the whole decision/update flow through one executor and dedupe any in-flight generation task.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Cove/WalletManager.swift` around lines 47 - 63, getReceiveAddress
currently has a race where concurrent callers can both observe a miss/expiry and
call rust.nextAddress(), rotating the address twice; serialize and dedupe
generation by funneling the entire check/generate/store flow through a single
executor: introduce an in-flight Task<> (e.g. inFlightReceiveAddressTask) or use
a private actor/serial DispatchQueue to ensure only one rust.nextAddress() runs
at a time, have callers await that shared Task when present, and update
cachedReceiveAddress, addressGeneratedTime, and lastTxCountForAddress only once
the Task completes; keep the existing checks (cachedReceiveAddress, forceNew,
addressGeneratedTime, currentTxCount vs lastTxCountForAddress) but make their
decision and the subsequent call to rust.nextAddress() atomic by using the new
in-flight Task/actor surrounding the getReceiveAddress flow.
oyetanishq
force-pushed
the
5min-address
branch
from
ca8489c to
61bc758
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt (1)
168-168:⚠️ Potential issue | 🟡 MinorPass an explicit
Locale.UStoString.format.Detekt (
ImplicitDefaultLocale) still flags this line, and the rest of the codebase usesLocale.USforString.formatcalls. Also, please apply the mm:ss / full-window fix noted cross-platform onReceiveView.swifthere as well so Android and iOS stay in sync.🛠 Minimal fix for the locale warning
+import java.util.Locale ... - timeLeftStr = String.format("Auto-refresh in %02d", seconds) + timeLeftStr = String.format(Locale.US, "Auto-refresh in %02d", seconds)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt` at line 168, Update the String.format call in ReceiveAddressSheet (timeLeftStr assignment) to use an explicit Locale.US (String.format(Locale.US, ...)) and mirror the mm:ss vs full-window formatting behavior used in ReceiveView.swift: when remaining seconds are less than 60 format as mm:ss with zero-padding, otherwise use the full-window format used on iOS (e.g., a simple seconds or minutes-based display consistent with ReceiveView.swift). Modify the logic around timeLeftStr in ReceiveAddressSheet to choose the correct format branch and call String.format(Locale.US, ...) in both branches.
🧹 Nitpick comments (3)
rust/src/manager/wallet_manager.rs (1)
903-903: Extract the 5‑minute window into a shared constant.The value
5 * 60is duplicated here, inandroid/.../ReceiveAddressSheet.kt(5 * 60 * 1000L) andios/.../ReceiveView.swift(5 * 60). A single source of truth prevents the three from drifting; exposing it via a small#[uniffi::export]getter (or a constant in a types crate) lets both UIs read the same value the Rust cache enforces.+const RECEIVE_ADDRESS_CACHE_SECS: u64 = 5 * 60; ... - let time_elapsed = now.saturating_sub(cache.generated_at) > (5 * 60); + let time_elapsed = + now.saturating_sub(cache.generated_at) > RECEIVE_ADDRESS_CACHE_SECS;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager.rs` at line 903, The 5-minute TTL used to evaluate cache staleness (currently written as the literal expression `5 * 60` in wallet_manager.rs when calculating `time_elapsed` against `cache.generated_at`) must be extracted to a shared constant and exposed to consumers; add a named constant (e.g. CACHE_TTL_SECONDS) and replace the literal in the `time_elapsed` computation, then expose that value via an accessor annotated for uniffi (e.g. a `#[uniffi::export] fn cache_ttl_seconds() -> u64`) so Android/iOS can read the same TTL instead of duplicating `5 * 60`.rust/src/database/global_cache.rs (1)
74-109: Extract the per-wallet cache key into a helper.The
receive_address_{wallet_id}key is built identically inget_receive_addressandset_receive_address. Extracting a small helper avoids drift if the prefix ever changes, and the inlineformat!can use captured-identifier syntax.♻️ Proposed refactor
impl GlobalCacheTable { + fn receive_address_key(id: &WalletId) -> String { + let id_str: &str = id.as_ref(); + format!("receive_address_{id_str}") + } + pub fn get_receive_address(&self, id: &WalletId) -> Result<Option<ReceiveAddressCache>, Error> { let read_txn = self.db.begin_read().map_err_str(Error::DatabaseAccess)?; let table = read_txn.open_table(TABLE).map_err_str(Error::TableAccess)?; - - let id_str: &str = id.as_ref(); - let key = format!("receive_address_{}", id_str); + let key = Self::receive_address_key(id); let value = table .get(key.as_str()) .map_err_str(GlobalCacheTableError::Read)? .map(|value| value.value()); if let Some(GlobalCacheData::ReceiveAddress(data)) = value { Ok(Some(data)) } else { Ok(None) } } pub fn set_receive_address( &self, id: &WalletId, data: ReceiveAddressCache, ) -> Result<(), Error> { let write_txn = self.db.begin_write().map_err_str(Error::DatabaseAccess)?; { let mut table = write_txn.open_table(TABLE).map_err_str(Error::TableAccess)?; - let id_str: &str = id.as_ref(); - let key = format!("receive_address_{}", id_str); + let key = Self::receive_address_key(id); table .insert(key.as_str(), GlobalCacheData::ReceiveAddress(data)) .map_err_str(GlobalCacheTableError::Save)?; } write_txn.commit().map_err_str(Error::DatabaseAccess)?; Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/database/global_cache.rs` around lines 74 - 109, Both get_receive_address and set_receive_address build the same "receive_address_{wallet_id}" key; extract that into a small helper (e.g., fn receive_address_key(id: &WalletId) -> String or fn receive_address_key(id: &str) -> String) and use it from get_receive_address and set_receive_address instead of duplicating format! calls. Update both functions to call the helper and prefer the captured-identifier format (e.g., "receive_address_{id_str}") inside the helper so any future prefix change is centralized.rust/crates/cove-types/src/address.rs (1)
412-454: Avoid per-callStringallocations forkeychain.
AddressInfoShadow.keychain: Stringforces a heap allocation for every serialize/deserialize round-trip. Since the value is a closed set of two variants, a&'static str(for serialize) and matching on&str(for deserialize) avoids the allocation without changing the wire format.♻️ Proposed refactor
-#[derive(serde::Serialize, serde::Deserialize)] -struct AddressInfoShadow { - address: String, - index: u32, - keychain: String, -} +#[derive(serde::Serialize, serde::Deserialize)] +struct AddressInfoShadow<'a> { + address: String, + index: u32, + keychain: std::borrow::Cow<'a, str>, +} impl serde::Serialize for AddressInfo { fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> where S: serde::Serializer, { let shadow = AddressInfoShadow { address: self.0.address.to_string(), index: self.0.index, - keychain: match self.0.keychain { - bdk_wallet::KeychainKind::External => "External".to_string(), - bdk_wallet::KeychainKind::Internal => "Internal".to_string(), - }, + keychain: std::borrow::Cow::Borrowed(match self.0.keychain { + bdk_wallet::KeychainKind::External => "External", + bdk_wallet::KeychainKind::Internal => "Internal", + }), }; shadow.serialize(serializer) } } impl<'de> serde::Deserialize<'de> for AddressInfo { fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> where D: serde::Deserializer<'de>, { let shadow = AddressInfoShadow::deserialize(deserializer)?; let address = BdkAddress::from_str(&shadow.address) .map_err(serde::de::Error::custom)? .assume_checked(); - let keychain = match shadow.keychain.as_str() { + let keychain = match shadow.keychain.as_ref() { "External" => bdk_wallet::KeychainKind::External, "Internal" => bdk_wallet::KeychainKind::Internal, _ => return Err(serde::de::Error::custom("invalid keychain kind")), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-types/src/address.rs` around lines 412 - 454, The current AddressInfoShadow.keychain: String causes unnecessary heap allocations; fix by using a borrowed/static type: for serialization introduce a helper struct (e.g., AddressInfoShadowSer) with keychain: &'static str and construct it in impl serde::Serialize for AddressInfo (map KeychainKind to "External"/"Internal" &'static str), and for deserialization use a borrowed representation (e.g., AddressInfoShadowDe<'de> with keychain: std::borrow::Cow<'de, str> or &str) in impl<'de> serde::Deserialize for AddressInfo and match on the borrowed &str when mapping back to bdk_wallet::KeychainKind; update the Serialize/Deserialize impls to use these helpers instead of allocating a String for keychain.
🤖 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/Flows/SelectedWalletFlow/ReceiveView.swift`:
- Around line 39-49: The current logic sets timeRemainingString to empty when
remaining > 60 and shows seconds-only when <= 60, which contradicts the PR
screenshots; update the countdown in ReceiveView.swift (the block that sets
timeRemainingString using remaining) to always render a mm:ss string for the
full window (compute minutes = Int(remaining) / 60 and seconds = Int(remaining)
% 60 and use a String(format: "%02d:%02d", minutes, seconds) with a prefix like
"Auto-refresh in "), and keep existing behavior for expiration when remaining <=
0; apply the same change to the analogous code in ReceiveAddressSheet.kt so both
platforms show mm:ss throughout the five-minute period.
In `@rust/src/manager/wallet_manager.rs`:
- Around line 917-924: The cache write currently swallows errors at
db.global_cache.set_receive_address(&self.id, cache); change it to log failures
using the existing pattern (e.g., tap_err and error! with self.id and the error)
so write errors are visible; additionally, when switching address types in
switch_to_different_wallet_address_type invalidate/remove the receive-address
cache entry (the receive_address_{wallet_id} entry keyed by WalletId/self.id) so
get_receive_address cannot return a stale address from the old type if the entry
is younger than 5 minutes and tx_count is unchanged.
- Around line 897-898: Replace the expensive allocation in get_receive_address
that calls call!(self.actor.transactions()).await.len() by adding a new actor
API tx_count() (mirror the same filtering: sent_and_received() -> amount > zero
logic) which returns the count without building Transaction objects or sorting;
update get_receive_address to call tx_count(). Also stop silently dropping DB
errors at the cache write: change the call to
global_cache.set_receive_address(...) so failures are either logged (use
existing logger) or propagated (return Err) instead of let _ = ...; ensure you
reference global_cache.set_receive_address in the same function. Fix
stale-cache-on-address-type-switch by clearing or namespacing the
receive-address cache when switch_to_different_wallet_address_type() runs
(either call global_cache.clear_receive_address(wallet_id, address_type) or
include address_type in the cache key instead of the current
receive_address_{wallet_id} key). Finally, extract the magic TTL 5 * 60 into a
named constant (e.g., RECEIVE_ADDRESS_CACHE_TTL_SECS) and use it where the cache
TTL is set.
---
Duplicate comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt`:
- Line 168: Update the String.format call in ReceiveAddressSheet (timeLeftStr
assignment) to use an explicit Locale.US (String.format(Locale.US, ...)) and
mirror the mm:ss vs full-window formatting behavior used in ReceiveView.swift:
when remaining seconds are less than 60 format as mm:ss with zero-padding,
otherwise use the full-window format used on iOS (e.g., a simple seconds or
minutes-based display consistent with ReceiveView.swift). Modify the logic
around timeLeftStr in ReceiveAddressSheet to choose the correct format branch
and call String.format(Locale.US, ...) in both branches.
---
Nitpick comments:
In `@rust/crates/cove-types/src/address.rs`:
- Around line 412-454: The current AddressInfoShadow.keychain: String causes
unnecessary heap allocations; fix by using a borrowed/static type: for
serialization introduce a helper struct (e.g., AddressInfoShadowSer) with
keychain: &'static str and construct it in impl serde::Serialize for AddressInfo
(map KeychainKind to "External"/"Internal" &'static str), and for
deserialization use a borrowed representation (e.g., AddressInfoShadowDe<'de>
with keychain: std::borrow::Cow<'de, str> or &str) in impl<'de>
serde::Deserialize for AddressInfo and match on the borrowed &str when mapping
back to bdk_wallet::KeychainKind; update the Serialize/Deserialize impls to use
these helpers instead of allocating a String for keychain.
In `@rust/src/database/global_cache.rs`:
- Around line 74-109: Both get_receive_address and set_receive_address build the
same "receive_address_{wallet_id}" key; extract that into a small helper (e.g.,
fn receive_address_key(id: &WalletId) -> String or fn receive_address_key(id:
&str) -> String) and use it from get_receive_address and set_receive_address
instead of duplicating format! calls. Update both functions to call the helper
and prefer the captured-identifier format (e.g., "receive_address_{id_str}")
inside the helper so any future prefix change is centralized.
In `@rust/src/manager/wallet_manager.rs`:
- Line 903: The 5-minute TTL used to evaluate cache staleness (currently written
as the literal expression `5 * 60` in wallet_manager.rs when calculating
`time_elapsed` against `cache.generated_at`) must be extracted to a shared
constant and exposed to consumers; add a named constant (e.g. CACHE_TTL_SECONDS)
and replace the literal in the `time_elapsed` computation, then expose that
value via an accessor annotated for uniffi (e.g. a `#[uniffi::export] fn
cache_ttl_seconds() -> u64`) so Android/iOS can read the same TTL instead of
duplicating `5 * 60`.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df9f0cf6-358b-4161-b872-cf218caace9d
📒 Files selected for processing (7)
android/app/src/main/java/org/bitcoinppl/cove/WalletManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.ktios/Cove/Flows/SelectedWalletFlow/ReceiveView.swiftios/Cove/WalletManager.swiftrust/crates/cove-types/src/address.rsrust/src/database/global_cache.rsrust/src/manager/wallet_manager.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/Cove/WalletManager.swift
| let txns = call!(self.actor.transactions()).await.unwrap_or_default(); | ||
| let current_tx_count = txns.len(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the actor does not already expose a lightweight count, and inspect the cost of transactions().
ast-grep --pattern $'pub async fn transactions($_) -> $_ {
$$$
}'
rg -nP '\bfn\s+(tx_count|transaction_count|num_transactions)\s*\(' --type=rust -C2Repository: bitcoinppl/cove
Length of output: 1993
🏁 Script executed:
#!/bin/bash
# Inspect lines around 897-898 to see how current_tx_count is used
sed -n '890,930p' rust/src/manager/wallet_manager.rs
# Also check if there's any existing cheap tx_count or if transactions() result is cached elsewhere
rg -n 'current_tx_count\|tx_count\|transaction_count' rust/src/manager/wallet_manager.rs -A2 -B2Repository: bitcoinppl/cove
Length of output: 1611
🏁 Script executed:
#!/bin/bash
# Check if BDK transactions can be filtered by amount without creating Transaction objects
sed -n '291,313p' rust/src/manager/wallet_manager/actor.rs
# Check the silent error at line 923
sed -n '920,930p' rust/src/manager/wallet_manager.rs
# Search for switch_to_different_wallet_address_type to check cache invalidation
rg -n 'switch_to_different_wallet_address_type' rust/src/manager/wallet_manager.rs -A10 -B2
# Check the hardcoded 5*60 constant
rg -n '5\s*\*\s*60|300' rust/src/manager/wallet_manager.rsRepository: bitcoinppl/cove
Length of output: 1982
🏁 Script executed:
#!/bin/bash
# Check if switch_to_different_wallet_address_type clears receive_address cache
sed -n '1200,1230p' rust/src/manager/wallet_manager.rs
# Search for any cache clearing on address type change
rg -n 'receive_address|set_receive_address|clear.*cache' rust/src/manager/wallet_manager.rs | grep -i 'switch\|address_type\|invalidate'
# Check if address_type is part of cache key or stored separately
rg -n 'get_receive_address\|ReceiveAddressCache\|receive_address_' rust/src/database/ -A3 -B1 | head -60
# Check the return type of set_receive_address
rg -n 'fn set_receive_address' rust/src/database/ -A5Repository: bitcoinppl/cove
Length of output: 1841
🏁 Script executed:
#!/bin/bash
# Check cache key structure - is it just wallet_id or includes address_type?
rg -n 'receive_address_\|get_receive_address\|set_receive_address' rust/src/database/global_cache.rs -B2 -A10 | head -80
# Check what switch_descriptor_to_new_address_type does
rg -n 'switch_descriptor_to_new_address_type' rust/src/manager/wallet_manager/actor.rs -A20
# Check if there's any global_cache.clear or cache invalidation logic
rg -n 'clear.*receive_address\|invalidate.*cache\|remove.*receive_address' rust/src/Repository: bitcoinppl/cove
Length of output: 1175
🏁 Script executed:
#!/bin/bash
# Look at the global_cache implementation more carefully
cat rust/src/database/global_cache.rs | head -150
# Or search for the specific cache key format
rg -n 'receive_address_' rust/src/database/global_cache.rs -B5 -A5Repository: bitcoinppl/cove
Length of output: 5533
🏁 Script executed:
#!/bin/bash
# Check if address_type is stored in ReceiveAddressCache
rg -n 'struct ReceiveAddressCache' rust/src/database/global_cache.rs -A10
# Verify no cache clearing happens on address type switch
rg -n 'switch_descriptor_to_new_address_type\|remove.*receive_address\|clear.*cache' rust/src/manager/wallet_manager.rs | grep -A5 -B5 'switch_descriptor'
# Check if address_type information is preserved or if cache could be stale
rg -n 'address_type\|WalletAddressType' rust/src/manager/wallet_manager.rs | head -30Repository: bitcoinppl/cove
Length of output: 427
Avoid building the full Vec<Transaction> just to get a count; also handle the silent cache write error and stale cache on address-type switch.
Every call to get_receive_address runs call!(self.actor.transactions()).await followed by .len(). Per actor.rs:291, transactions() iterates BDK transactions, calls expensive sent_and_received() on each, wraps in Transaction::new(), filters by amount > zero, collects the full Vec, and sorts—only to discard it after one length check.
Consider exposing tx_count() on the actor that counts transactions with the same filtering (amount > zero) without allocating the full Transaction objects or sorting:
- let txns = call!(self.actor.transactions()).await.unwrap_or_default();
- let current_tx_count = txns.len();
+ let current_tx_count = call!(self.actor.tx_count()).await.unwrap_or(0);Two additional issues:
- Line 923 silently discards DB errors:
let _ = db.global_cache.set_receive_address(...)swallows write failures with no log. If the cache write fails, the next call will regenerate another address because the cache never persists. Log or propagate the error. - Cache is stale across address-type switches: The cache key is only
receive_address_{wallet_id}(line 79 ofglobal_cache.rs) with noaddress_typefield. Callingswitch_to_different_wallet_address_type()does not clear this cache. If you switch address types and callget_receive_address()within 5 minutes with the same transaction count, you receive the address from the previous address type. Clear the cache explicitly when address type changes.
Also extract the hardcoded 5 * 60 to a constant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/manager/wallet_manager.rs` around lines 897 - 898, Replace the
expensive allocation in get_receive_address that calls
call!(self.actor.transactions()).await.len() by adding a new actor API
tx_count() (mirror the same filtering: sent_and_received() -> amount > zero
logic) which returns the count without building Transaction objects or sorting;
update get_receive_address to call tx_count(). Also stop silently dropping DB
errors at the cache write: change the call to
global_cache.set_receive_address(...) so failures are either logged (use
existing logger) or propagated (return Err) instead of let _ = ...; ensure you
reference global_cache.set_receive_address in the same function. Fix
stale-cache-on-address-type-switch by clearing or namespacing the
receive-address cache when switch_to_different_wallet_address_type() runs
(either call global_cache.clear_receive_address(wallet_id, address_type) or
include address_type in the cache key instead of the current
receive_address_{wallet_id} key). Finally, extract the magic TTL 5 * 60 into a
named constant (e.g., RECEIVE_ADDRESS_CACHE_TTL_SECS) and use it where the cache
TTL is set.
| let cache = crate::database::global_cache::ReceiveAddressCache { | ||
| address_info: new_address.clone(), | ||
| generated_at: now, | ||
| tx_count: current_tx_count, | ||
| }; | ||
|
|
||
| let _ = db.global_cache.set_receive_address(&self.id, cache); | ||
| self.reconciler.send(Message::AddressGeneratedTime(now)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the WalletId does not change across switch_to_different_wallet_address_type
# (so the cached ReceiveAddressCache for the old address type persists).
ast-grep --pattern $'pub async fn switch_to_different_wallet_address_type($$$) -> $_ {
$$$
}'
rg -nP '\bset_receive_address\b|\bget_receive_address\b' --type=rust -C2Repository: bitcoinppl/cove
Length of output: 7028
🏁 Script executed:
cd rust && head -120 src/database/global_cache.rs | tail -50Repository: bitcoinppl/cove
Length of output: 1768
🏁 Script executed:
rg -n 'fn set_receive_address|fn get_receive_address' rust/src/database/global_cache.rs -A 15Repository: bitcoinppl/cove
Length of output: 1476
🏁 Script executed:
rg -n 'global_cache\.' rust/src/manager/wallet_manager.rs | grep -E 'delete|remove|invalidate|clear'Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
rg -n 'error!\|warn!\|debug!' rust/src/manager/wallet_manager.rs | head -20Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
rg -n 'let _ =' rust/src/manager/wallet_manager.rsRepository: bitcoinppl/cove
Length of output: 311
🏁 Script executed:
rg -n 'self.reconciler.send' rust/src/manager/wallet_manager.rs -B 2 -A 2Repository: bitcoinppl/cove
Length of output: 1873
🏁 Script executed:
head -50 rust/src/manager/wallet_manager.rs | grep -E 'use|log|error|warn'Repository: bitcoinppl/cove
Length of output: 470
🏁 Script executed:
rg -n 'tap_err' rust/src/manager/wallet_manager.rs -B 1 -A 3Repository: bitcoinppl/cove
Length of output: 350
🏁 Script executed:
rg -n 'Error::' rust/src/manager/wallet_manager.rs | grep -i database | head -10Repository: bitcoinppl/cove
Length of output: 355
🏁 Script executed:
rg -n 'ReceiveAddressCache' rust/src/database/global_cache.rs -B 2 -A 8Repository: bitcoinppl/cove
Length of output: 1562
🏁 Script executed:
rg -n 'struct ReceiveAddressCache\|pub struct ReceiveAddressCache' rust/ -A 5Repository: bitcoinppl/cove
Length of output: 41
Log cache-write failures; they silently break the 5‑minute reuse guarantee.
let _ = db.global_cache.set_receive_address(...) at line 923 drops any database error without logging. If the write fails, nothing is cached, and the next call to get_receive_address will skip the cache and rotate the address — defeating issue #386's reuse-window goal — with no indication of failure.
Use the established logging pattern (e.g., line 969–970):
let _ = db.global_cache.set_receive_address(&self.id, cache).tap_err(|error| {
error!("failed to cache receive address for {}: {error}", self.id);
});Additionally, the cache key receive_address_{wallet_id} persists when switch_to_different_wallet_address_type is called, since WalletId remains unchanged. If a cache entry is less than 5 minutes old and tx_count is unchanged after the switch, get_receive_address will return the previous address type's address. Invalidate this key (e.g., delete the entry) when switching address types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/manager/wallet_manager.rs` around lines 917 - 924, The cache write
currently swallows errors at db.global_cache.set_receive_address(&self.id,
cache); change it to log failures using the existing pattern (e.g., tap_err and
error! with self.id and the error) so write errors are visible; additionally,
when switching address types in switch_to_different_wallet_address_type
invalidate/remove the receive-address cache entry (the
receive_address_{wallet_id} entry keyed by WalletId/self.id) so
get_receive_address cannot return a stale address from the old type if the entry
is younger than 5 minutes and tx_count is unchanged.
61bc758 to
a3a9997
Compare
oyetanishq
changed the title
5m timeout and `tx count invalida…5m timeout
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt (1)
157-179:⚠️ Potential issue | 🟡 MinorCountdown still blanks above 60s and renders only seconds — doesn't match the PR screenshots.
Same issue as the iOS side: the screenshots show
Auto-refresh in 04:49/01:26(mm:ss) throughout the 5‑minute window, but this block emits an empty string whenremaining > 60_000Land, below that, formats as%02dseconds only. Render mm:ss for the full window to match the intended UX.♻️ Suggested fix
- if (remaining > 0) { - if (remaining <= 60_000L) { - val seconds = (remaining / 1000) - timeLeftStr = String.format("Auto-refresh in %02d", seconds) - } else { - timeLeftStr = "" - } - } else { + if (remaining > 0) { + val totalSeconds = remaining / 1000 + val minutes = totalSeconds / 60 + val seconds = totalSeconds % 60 + timeLeftStr = String.format( + Locale.US, + "Auto-refresh in %02d:%02d", + minutes, + seconds, + ) + } else { timeLeftStr = "Address Expired" break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt` around lines 157 - 179, The countdown currently emits an empty string when remaining > 60_000L and formats only seconds below that; update the LaunchedEffect block (using addressGeneratedTime, remaining, timeLeftStr) to always render mm:ss while remaining > 0: compute minutes = (remaining / 1000) / 60 and seconds = (remaining / 1000) % 60 and set timeLeftStr = String.format("Auto-refresh in %02d:%02d", minutes, seconds); keep the "Address Expired" branch when remaining <= 0 and retain the existing kotlinx.coroutines.delay(1000) loop logic.ios/Cove/Flows/SelectedWalletFlow/ReceiveView.swift (1)
39-49:⚠️ Potential issue | 🟡 MinorCountdown still only renders in the last 60s (not mm:ss across the full 5‑minute window).
The PR screenshots show
Auto-refresh in 01:26/04:49— these are mm:ss visible throughout the 5‑minute window. Current logic blanks the string wheneverremaining > 60and, below that threshold, formats as seconds only, so users see nothing for the first ~4 minutes and never a value above60.♻️ Suggested fix to match the screenshots
- if remaining > 0 { - if remaining <= 60 { - let seconds = Int(remaining) - timeRemainingString = String(format: "Auto-refresh in %02d", seconds) - } else { - timeRemainingString = "" - } - } else { + if remaining > 0 { + let total = Int(remaining) + let minutes = total / 60 + let seconds = total % 60 + timeRemainingString = String(format: "Auto-refresh in %02d:%02d", minutes, seconds) + } else { timeRemainingString = "Address Expired" break }Apply the equivalent change in
ReceiveAddressSheet.ktfor parity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SelectedWalletFlow/ReceiveView.swift` around lines 39 - 49, The countdown currently blanks timeRemainingString when remaining > 60 and only shows seconds when <= 60; change the logic in ReceiveView.swift to always render mm:ss for any remaining > 0: compute totalSeconds = Int(remaining), minutes = totalSeconds / 60, seconds = totalSeconds % 60 and set timeRemainingString = String(format: "Auto-refresh in %02d:%02d", minutes, seconds); keep the else branch to set "Address Expired" when remaining <= 0 and break; apply the equivalent change in ReceiveAddressSheet.kt where the countdown string is computed so both platforms show mm:ss across the full 5‑minute window.
🧹 Nitpick comments (2)
android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt (1)
157-179: Minor: idiomatic cleanup in the countdown loop.Small polish in the same block:
while(true)→while (true)(Kotlin style).kotlinx.coroutines.delay(1000)is fully qualified; addimport kotlinx.coroutines.delayand usedelay(1000).- When
addressGeneratedTime == 0L, considerreturn@LaunchedEffect(or letting the key change drive recomposition) instead of looping every second to assign an empty string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt` around lines 157 - 179, In the LaunchedEffect block for addressGeneratedTime update the countdown loop to be idiomatic: change the loop header to "while (true)" for Kotlin style, add an import for kotlinx.coroutines.delay and replace the fully-qualified kotlinx.coroutines.delay(1000) with delay(1000), and avoid spinning when addressGeneratedTime == 0L by using return@LaunchedEffect (or letting the key drive recomposition) instead of repeatedly setting timeLeftStr = "" in the loop; keep the existing expiry logic that sets timeLeftStr to "Address Expired" and breaks when remaining <= 0.rust/crates/cove-types/src/address.rs (1)
412-454: Custom serde forAddressInfo: network isn't persisted; consider simplifying the keychain encoding.A couple of small concerns with the new shadow-based (de)serialization:
BdkAddress::from_str(&shadow.address)?.assume_checked()on deserialize discards network validation. For the local cache use-case this is fine (we only ever cache addresses we just generated for the current wallet), but a corrupted/tampered DB could round-trip a wrong-network address silently. Consider either persisting the network alongside the string and callingrequire_network(...)on load, or asserting the address's implicit network matches the wallet's network where this cache is consumed.The stringly-typed
keychainfield can be replaced by deriving serde with a remote helper (or just a small#[repr(u8)]marker), which avoids the manual match and string allocation on every serialize:♻️ Optional simplification using a serde-friendly enum shadow
-#[derive(serde::Serialize, serde::Deserialize)] -struct AddressInfoShadow { - address: String, - index: u32, - keychain: String, -} +#[derive(serde::Serialize, serde::Deserialize)] +enum KeychainKindShadow { + External, + Internal, +} + +impl From<bdk_wallet::KeychainKind> for KeychainKindShadow { + fn from(k: bdk_wallet::KeychainKind) -> Self { + match k { + bdk_wallet::KeychainKind::External => Self::External, + bdk_wallet::KeychainKind::Internal => Self::Internal, + } + } +} + +impl From<KeychainKindShadow> for bdk_wallet::KeychainKind { + fn from(k: KeychainKindShadow) -> Self { + match k { + KeychainKindShadow::External => Self::External, + KeychainKindShadow::Internal => Self::Internal, + } + } +} + +#[derive(serde::Serialize, serde::Deserialize)] +struct AddressInfoShadow { + address: String, + index: u32, + keychain: KeychainKindShadow, +}This removes the
"invalid keychain kind"custom error path entirely and eliminates a per-serializeString::to_string()for the keychain.Also worth adding a serde round-trip unit test for
AddressInfo(andAddressInfoWithDerivation) so that a future change to the shadow format can't silently break cache compatibility with on-disk data from prior builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-types/src/address.rs` around lines 412 - 454, The custom AddressInfo (de)serializer currently drops address network validation and uses a stringly keychain; update AddressInfoShadow/serialization to persist and validate the address network (either add a network field and call BdkAddress::from_str(...).require_network(...) or assert the parsed address network matches the wallet network where this cache is consumed) and replace the string keychain field/matching with a serde-friendly enum or repr(u8) shadow type that maps to bdk_wallet::KeychainKind to avoid per-serialize allocations and the manual match/error branch; also add serde round-trip unit tests for AddressInfo and AddressInfoWithDerivation to catch future format regressions.
🤖 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/WalletManager.swift`:
- Around line 230-232: The switch in WalletManager.swift references the
addressGeneratedTime case that is missing from the generated
WalletManagerReconcileMessage enum; regenerate the UniFFI bindings so the Rust
variant Message::AddressGeneratedTime(u64) is exported and the Swift enum
WalletManagerReconcileMessage (in
ios/CoveCore/Sources/CoveCore/generated/cove.swift) includes the
addressGeneratedTime case, then rebuild the iOS target so the switch in
WalletManager.swift compiles against the updated bindings.
In `@rust/src/database/global_cache.rs`:
- Around line 92-109: Explain in a short comment inside set_receive_address why
it intentionally omits calling Updater::send_update(Update::DatabaseUpdated)
(point out that wallet_manager.rs sends Message::AddressGeneratedTime(now)
instead and that emitting DatabaseUpdated here would cause duplicate/spurious UI
refreshes and cache-invalidation races), and add a debug! log call in
set_receive_address similar to the one in set (include the WalletId and that the
receive address was stored) so behavior is consistent for troubleshooting
without sending the DatabaseUpdated notification.
- Around line 74-109: The global cache currently leaves
receive_address_{wallet_id} entries orphaned on wallet deletion and uses
non-idiomatic format calls; add a new method delete_receive_address(&self, id:
&WalletId) -> Result<(), Error> that opens a write txn, opens TABLE, removes the
key formatted as the same key used elsewhere, commits the txn, and return
Result, then call delete_receive_address from the wallet deletion flow (e.g.,
next to delete_wallet()) to ensure cleanup; also simplify the key construction
in get_receive_address and set_receive_address by using the inline format
capture (e.g., format!("receive_address_{id_str}") where id_str is the WalletId
as_ref()) so both places construct the identical key idiomatically.
---
Duplicate comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt`:
- Around line 157-179: The countdown currently emits an empty string when
remaining > 60_000L and formats only seconds below that; update the
LaunchedEffect block (using addressGeneratedTime, remaining, timeLeftStr) to
always render mm:ss while remaining > 0: compute minutes = (remaining / 1000) /
60 and seconds = (remaining / 1000) % 60 and set timeLeftStr =
String.format("Auto-refresh in %02d:%02d", minutes, seconds); keep the "Address
Expired" branch when remaining <= 0 and retain the existing
kotlinx.coroutines.delay(1000) loop logic.
In `@ios/Cove/Flows/SelectedWalletFlow/ReceiveView.swift`:
- Around line 39-49: The countdown currently blanks timeRemainingString when
remaining > 60 and only shows seconds when <= 60; change the logic in
ReceiveView.swift to always render mm:ss for any remaining > 0: compute
totalSeconds = Int(remaining), minutes = totalSeconds / 60, seconds =
totalSeconds % 60 and set timeRemainingString = String(format: "Auto-refresh in
%02d:%02d", minutes, seconds); keep the else branch to set "Address Expired"
when remaining <= 0 and break; apply the equivalent change in
ReceiveAddressSheet.kt where the countdown string is computed so both platforms
show mm:ss across the full 5‑minute window.
---
Nitpick comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt`:
- Around line 157-179: In the LaunchedEffect block for addressGeneratedTime
update the countdown loop to be idiomatic: change the loop header to "while
(true)" for Kotlin style, add an import for kotlinx.coroutines.delay and replace
the fully-qualified kotlinx.coroutines.delay(1000) with delay(1000), and avoid
spinning when addressGeneratedTime == 0L by using return@LaunchedEffect (or
letting the key drive recomposition) instead of repeatedly setting timeLeftStr =
"" in the loop; keep the existing expiry logic that sets timeLeftStr to "Address
Expired" and breaks when remaining <= 0.
In `@rust/crates/cove-types/src/address.rs`:
- Around line 412-454: The custom AddressInfo (de)serializer currently drops
address network validation and uses a stringly keychain; update
AddressInfoShadow/serialization to persist and validate the address network
(either add a network field and call
BdkAddress::from_str(...).require_network(...) or assert the parsed address
network matches the wallet network where this cache is consumed) and replace the
string keychain field/matching with a serde-friendly enum or repr(u8) shadow
type that maps to bdk_wallet::KeychainKind to avoid per-serialize allocations
and the manual match/error branch; also add serde round-trip unit tests for
AddressInfo and AddressInfoWithDerivation to catch future format regressions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5f59b7f8-5b5e-442f-a671-a57961e1ed60
📒 Files selected for processing (7)
android/app/src/main/java/org/bitcoinppl/cove/WalletManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.ktios/Cove/Flows/SelectedWalletFlow/ReceiveView.swiftios/Cove/WalletManager.swiftrust/crates/cove-types/src/address.rsrust/src/database/global_cache.rsrust/src/manager/wallet_manager.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- android/app/src/main/java/org/bitcoinppl/cove/WalletManager.kt
- rust/src/manager/wallet_manager.rs
| pub fn get_receive_address(&self, id: &WalletId) -> Result<Option<ReceiveAddressCache>, Error> { | ||
| let read_txn = self.db.begin_read().map_err_str(Error::DatabaseAccess)?; | ||
| let table = read_txn.open_table(TABLE).map_err_str(Error::TableAccess)?; | ||
|
|
||
| let id_str: &str = id.as_ref(); | ||
| let key = format!("receive_address_{}", id_str); | ||
|
|
||
| let value = table | ||
| .get(key.as_str()) | ||
| .map_err_str(GlobalCacheTableError::Read)? | ||
| .map(|value| value.value()); | ||
| if let Some(GlobalCacheData::ReceiveAddress(data)) = value { | ||
| Ok(Some(data)) | ||
| } else { | ||
| Ok(None) | ||
| } | ||
| } | ||
|
|
||
| pub fn set_receive_address( | ||
| &self, | ||
| id: &WalletId, | ||
| data: ReceiveAddressCache, | ||
| ) -> Result<(), Error> { | ||
| let write_txn = self.db.begin_write().map_err_str(Error::DatabaseAccess)?; | ||
| { | ||
| let mut table = write_txn.open_table(TABLE).map_err_str(Error::TableAccess)?; | ||
| let id_str: &str = id.as_ref(); | ||
| let key = format!("receive_address_{}", id_str); | ||
|
|
||
| table | ||
| .insert(key.as_str(), GlobalCacheData::ReceiveAddress(data)) | ||
| .map_err_str(GlobalCacheTableError::Save)?; | ||
| } | ||
| write_txn.commit().map_err_str(Error::DatabaseAccess)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for anything that deletes from the global_cache table, or that
# removes cache entries on wallet delete.
rg -nP --type=rust -C3 '\bglobal_cache\b'
rg -nP --type=rust -C3 '\breceive_address_'
rg -nP --type=rust -C3 '(delete|remove)_wallet\b'Repository: bitcoinppl/cove
Length of output: 11755
🏁 Script executed:
# Get the full implementation of delete_wallet
sed -n '946,1000p' rust/src/manager/wallet_manager.rsRepository: bitcoinppl/cove
Length of output: 2351
🏁 Script executed:
# Search for any deletion or cleanup methods in global_cache.rs
rg -nP --type=rust 'delete|remove|clear' rust/src/database/global_cache.rsRepository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Search for calls to delete_receive_address or any cleanup of global_cache on wallet delete
rg -nP --type=rust 'global_cache\.(delete|remove|clear)' rust/srcRepository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Check if there's a delete_receive_address method anywhere in the codebase
rg -nP --type=rust 'delete_receive_address'Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Get the full global_cache.rs file to see all methods
wc -l rust/src/database/global_cache.rsRepository: bitcoinppl/cove
Length of output: 96
🏁 Script executed:
# Show the complete global_cache.rs file
cat -n rust/src/database/global_cache.rsRepository: bitcoinppl/cove
Length of output: 6272
Add cache cleanup on wallet deletion and simplify format string inline.
Two issues identified:
-
Cache accumulation on wallet deletion: The
delete_wallet()flow cleans up the wallets table, keychain, and wallet-specific data, but does not clean up the correspondingreceive_address_{wallet_id}entry in the global cache. These rows will accumulate indefinitely. Either add adelete_receive_address(&WalletId)method and call it during wallet deletion, or ensure the wallet manager explicitly removes this cache entry. -
Inline format arguments for idiomatic Rust (lines 78-79 and 100-101):
Apply to both get_receive_address and set_receive_address
- let id_str: &str = id.as_ref();
- let key = format!("receive_address_{}", id_str);
+ let key = format!("receive_address_{}", id.as_ref());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/database/global_cache.rs` around lines 74 - 109, The global cache
currently leaves receive_address_{wallet_id} entries orphaned on wallet deletion
and uses non-idiomatic format calls; add a new method
delete_receive_address(&self, id: &WalletId) -> Result<(), Error> that opens a
write txn, opens TABLE, removes the key formatted as the same key used
elsewhere, commits the txn, and return Result, then call delete_receive_address
from the wallet deletion flow (e.g., next to delete_wallet()) to ensure cleanup;
also simplify the key construction in get_receive_address and
set_receive_address by using the inline format capture (e.g.,
format!("receive_address_{id_str}") where id_str is the WalletId as_ref()) so
both places construct the identical key idiomatically.
| pub fn set_receive_address( | ||
| &self, | ||
| id: &WalletId, | ||
| data: ReceiveAddressCache, | ||
| ) -> Result<(), Error> { | ||
| let write_txn = self.db.begin_write().map_err_str(Error::DatabaseAccess)?; | ||
| { | ||
| let mut table = write_txn.open_table(TABLE).map_err_str(Error::TableAccess)?; | ||
| let id_str: &str = id.as_ref(); | ||
| let key = format!("receive_address_{}", id_str); | ||
|
|
||
| table | ||
| .insert(key.as_str(), GlobalCacheData::ReceiveAddress(data)) | ||
| .map_err_str(GlobalCacheTableError::Save)?; | ||
| } | ||
| write_txn.commit().map_err_str(Error::DatabaseAccess)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the global_cache.rs file
find . -type f -name "global_cache.rs" | head -5Repository: bitcoinppl/cove
Length of output: 94
🏁 Script executed:
# Check the size and look at the relevant sections
wc -l rust/src/database/global_cache.rsRepository: bitcoinppl/cove
Length of output: 96
🏁 Script executed:
# Read the set_receive_address method and surrounding context
sed -n '90,115p' rust/src/database/global_cache.rsRepository: bitcoinppl/cove
Length of output: 982
🏁 Script executed:
# Read the generic set method at line 166
sed -n '160,190p' rust/src/database/global_cache.rsRepository: bitcoinppl/cove
Length of output: 341
🏁 Script executed:
# Search for wallet_manager.rs and where set_receive_address is called
fd -t f "wallet_manager.rs" && rg "set_receive_address" --type rustRepository: bitcoinppl/cove
Length of output: 349
🏁 Script executed:
# Look for Update::DatabaseUpdated usage in the codebase
rg "Update::DatabaseUpdated" --type rust -A 2 -B 2Repository: bitcoinppl/cove
Length of output: 2252
🏁 Script executed:
# Find the context in wallet_manager.rs where set_receive_address is called
rg "set_receive_address" -B 5 -A 5 rust/src/manager/wallet_manager.rsRepository: bitcoinppl/cove
Length of output: 356
🏁 Script executed:
# Search for Message::AddressGeneratedTime to verify the claim
rg "Message::AddressGeneratedTime" --type rust -B 3 -A 3Repository: bitcoinppl/cove
Length of output: 1014
🏁 Script executed:
# Check if there's any existing comment in global_cache.rs explaining the omission
sed -n '85,115p' rust/src/database/global_cache.rsRepository: bitcoinppl/cove
Length of output: 1126
🏁 Script executed:
# Look for any debug logs in set_receive_address and compare with set method
sed -n '92,109p' rust/src/database/global_cache.rs && echo "---" && sed -n '145,170p' rust/src/database/global_cache.rsRepository: bitcoinppl/cove
Length of output: 1539
Add a comment and debug log to explain why set_receive_address omits Update::DatabaseUpdated notification.
The method intentionally omits Updater::send_update(Update::DatabaseUpdated) that the generic set method emits, while the caller in wallet_manager.rs sends Message::AddressGeneratedTime(now) separately. However, the code lacks documentation of this design decision, and there is no debug log for parity with set. This inconsistency creates two risks:
- Future maintainers may unknowingly "fix" it by adding
Update::DatabaseUpdated, creating spurious UI refresh cascades. - Subscribers relying on
DatabaseUpdatedto invalidate their local cache view will be out-of-sync.
Add a comment explaining why the notification must be omitted, and add a debug! log analogous to set for consistency and troubleshooting:
Suggested diff
pub fn set_receive_address(
&self,
id: &WalletId,
data: ReceiveAddressCache,
) -> Result<(), Error> {
+ // NOTE: intentionally does not emit Update::DatabaseUpdated — the caller
+ // notifies via Message::AddressGeneratedTime and we want to avoid UI
+ // refresh cascades every time the receive-address cache is touched.
+ debug!("set receive address cache for wallet {}", id.as_ref());
let write_txn = self.db.begin_write().map_err_str(Error::DatabaseAccess)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/database/global_cache.rs` around lines 92 - 109, Explain in a short
comment inside set_receive_address why it intentionally omits calling
Updater::send_update(Update::DatabaseUpdated) (point out that wallet_manager.rs
sends Message::AddressGeneratedTime(now) instead and that emitting
DatabaseUpdated here would cause duplicate/spurious UI refreshes and
cache-invalidation races), and add a debug! log call in set_receive_address
similar to the one in set (include the WalletId and that the receive address was
stored) so behavior is consistent for troubleshooting without sending the
DatabaseUpdated notification.
a3a9997 to
18e8b39
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/manager/wallet_manager.rs (1)
927-934:⚠️ Potential issue | 🔴 CriticalRemove or deprecate
next_address(), or synchronize it with the cache.The method is still exported via
#[uniffi::method]and callsactor.next_address()directly, completely bypassing the cache infrastructure inget_receive_address(). The latter has proper cache management (mutex lock, 5-minute TTL, transaction count validation, persistence), while the former has none. If any caller invokesnext_address()directly, BDK will advance its internal revealed-address pointer while theReceiveAddressCacheremains stale. The nextget_receive_address()call within the 5-minute window would then return an outdated address and transaction count.Either route all callers through
get_receive_address(force_new: true)and remove or deprecatenext_address(), or havenext_address()also update the cache entry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager.rs` around lines 927 - 934, The exported next_address() currently calls actor.next_address() directly (bypassing ReceiveAddressCache) which will desync the cache; either remove/deprecate the uniffi method and route callers to get_receive_address(force_new: true) or make next_address() update the same cache path: acquire the ReceiveAddressCache mutex, call self.actor.next_address(), construct the AddressInfoWithDerivation, update/persist the cache entry and release the lock (mirroring get_receive_address() cache TTL and tx-count validation) so the cache stays consistent with BDK; update/remove the #[uniffi::method] annotation accordingly.
♻️ Duplicate comments (1)
rust/src/manager/wallet_manager.rs (1)
888-925:⚠️ Potential issue | 🟠 MajorSeveral previously-flagged issues still apply to this method.
Re-reviewing
get_receive_addressagainst the current code, the following concerns from earlier rounds are unresolved and still worth addressing before merge:
Silent DB error on cache write (line 921).
let _ = db.global_cache.set_receive_address(&self.id, cache);discards any persistence failure. If the write fails, the 5-minute reuse guarantee from#386is silently lost — the next call will regenerate and consume another address. Use the establishedtap_err+error!pattern already present elsewhere in this file (see line 967-969).Stale cache across address-type switches. The cache key in
global_cache.rsisreceive_address_{wallet_id}with no address-type component.switch_to_different_wallet_address_type(lines 1197-1259) mutates the descriptor but does not invalidate the cache. Iftx_countis unchanged and the entry is <5 minutes old,get_receive_addresswill return an address from the old descriptor type after the switch — a correctness bug. Either includeaddress_typein the cache key (per snippet atrust/src/database/global_cache.rs:92-109) or delete the entry on switch.Expensive tx-count computation (lines 897-898).
call!(self.actor.transactions()).awaititerates all BDK transactions, callssent_and_received()on each, allocatesTransactionobjects, filters, and sorts — only for.len(). This runs on every call including the cached path andforce_new=true. Consider exposing a cheaptx_count()on the actor that applies the sameamount > 0filter without allocating.Magic
5 * 60(line 902). Extract to a named constant such asRECEIVE_ADDRESS_CACHE_TTL_SECSso Rust and the platform countdown UIs share the same source of truth.Minor: the boolean
time_elapsedreads as “time has elapsed” but the condition actually means “cache is expired”. Renaming tois_expiredclarifies the subsequentif !time_elapsed && !tx_count_changedpredicate.🛠️ Sketch of the fixes
+const RECEIVE_ADDRESS_CACHE_TTL_SECS: u64 = 5 * 60; + pub async fn get_receive_address( &self, force_new: bool, ) -> Result<AddressInfoWithDerivation, Error> { let _guard = self.address_generation_lock.lock().await; let db = Database::global(); - let txns = call!(self.actor.transactions()).await.unwrap_or_default(); - let current_tx_count = txns.len(); + // Prefer a dedicated cheap count; falls back to 0 on actor error. + let current_tx_count = call!(self.actor.tx_count()).await.unwrap_or(0); if !force_new && let Ok(Some(cache)) = db.global_cache.get_receive_address(&self.id) { let now = jiff::Timestamp::now().as_second() as u64; - let time_elapsed = now.saturating_sub(cache.generated_at) > (5 * 60); - let tx_count_changed = current_tx_count != cache.tx_count; - - if !time_elapsed && !tx_count_changed { + let is_expired = + now.saturating_sub(cache.generated_at) > RECEIVE_ADDRESS_CACHE_TTL_SECS; + let tx_count_changed = current_tx_count != cache.tx_count; + + if !is_expired && !tx_count_changed { self.reconciler.send(Message::AddressGeneratedTime(cache.generated_at)); return Ok(cache.address_info); } } let new_address = call!(self.actor.next_address()).await.map_err_str(Error::NextAddressError)?; let now = jiff::Timestamp::now().as_second() as u64; let cache = crate::database::global_cache::ReceiveAddressCache { address_info: new_address.clone(), generated_at: now, tx_count: current_tx_count, }; - let _ = db.global_cache.set_receive_address(&self.id, cache); + let _ = db + .global_cache + .set_receive_address(&self.id, cache) + .tap_err(|error| error!("failed to cache receive address for {}: {error}", self.id)); self.reconciler.send(Message::AddressGeneratedTime(now)); Ok(new_address) }And in
switch_to_different_wallet_address_type, invalidate the entry before resetting the route, e.g.:let _ = db.global_cache.delete_receive_address(&self.id);(exact API name depending on what you add to
global_cache.rs).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager.rs` around lines 888 - 925, get_receive_address currently swallows DB write errors, can return stale addresses after an address-type switch, performs an expensive full transaction iteration to compute tx_count, and uses a magic TTL; fix by (1) replacing the `let _ = db.global_cache.set_receive_address(&self.id, cache);` call with the same tap_err + error! logging pattern used elsewhere so write failures are logged, (2) ensuring the receive-address cache key is address-type aware or explicitly deleting/invalidate the cached entry from global_cache in switch_to_different_wallet_address_type (e.g. call the new delete_receive_address API) so get_receive_address won't return an address from the old descriptor, (3) adding a cheap tx_count() method on the actor (or equivalent) and calling that instead of call!(self.actor.transactions()).await.len(), and (4) extracting the literal `5 * 60` into a named constant RECEIVE_ADDRESS_CACHE_TTL_SECS and renaming the boolean `time_elapsed` to `is_expired` to clarify semantics; update get_receive_address to use these changes and to still send Message::AddressGeneratedTime on both cached and new paths.
🧹 Nitpick comments (2)
android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt (2)
157-179: Minor: avoid busy-looping when no address has been generated, and importdelay.
- When
addressGeneratedTime == 0Lthe loop still wakes every second to re-assign"". A one-shot early return (as in the fix above) avoids that work.kotlinx.coroutines.delayis already transitively used here; prefer animport kotlinx.coroutines.delayso the call site is justdelay(1000).- Nit:
while(true)→while (true)to match Kotlin style elsewhere in the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt` around lines 157 - 179, Inside the LaunchedEffect block that updates timeLeftStr (LaunchedEffect(addressGeneratedTime)), avoid busy-looping by returning immediately when addressGeneratedTime == 0L instead of reassigning "" each second; change the loop header to Kotlin style while (true); add an explicit import kotlinx.coroutines.delay and replace kotlinx.coroutines.delay(1000) with delay(1000) so the call site is cleaner; ensure the expiration branch still sets timeLeftStr = "Address Expired" and breaks the loop as before.
308-319: Redundant 32.dp spacer when countdown is hidden.When
timeLeftStris empty (the entire first ~4 minutes, per current logic), you render two stacked 32.dp spacers with nothing between them, which visually pushes the Copy button down by 64.dp. If you keep the conditional render, consider collapsing the second spacer into the conditional branch, or (better, once the countdown is made always‑visible per the major comment) reserve a fixed-height slot to avoid layout shift when text transitions between states (""→MM:SS→Address Expired).♻️ Possible fix
- Spacer(modifier = Modifier.height(32.dp)) - - if (timeLeftStr.isNotEmpty() && !isLoading) { - Text( - text = timeLeftStr, - style = MaterialTheme.typography.bodySmall, - color = MaterialTheme.colorScheme.onSurfaceVariant.copy(alpha = 0.6f), - modifier = Modifier.padding(top = 4.dp) - ) - } - - Spacer(modifier = Modifier.height(32.dp)) + Spacer(modifier = Modifier.height(24.dp)) + Box(modifier = Modifier.height(20.dp), contentAlignment = Alignment.Center) { + if (timeLeftStr.isNotEmpty() && !isLoading) { + Text( + text = timeLeftStr, + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant.copy(alpha = 0.6f), + ) + } + } + Spacer(modifier = Modifier.height(24.dp))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt` around lines 308 - 319, ReceiveAddressSheet currently renders Spacer(modifier = Modifier.height(32.dp)) before and after the conditional that shows timeLeftStr, causing a 64.dp gap when timeLeftStr is empty; change the layout to reserve a fixed-height slot for the countdown instead of conditional spacing: replace the two separate 32.dp Spacers with a single container (e.g., a Box or Column with fixed height 32.dp or minHeight and the Text for timeLeftStr inside it), or move the trailing Spacer into the same if branch, referencing the ReceiveAddressSheet composable and the timeLeftStr / isLoading checks so the Copy button no longer jumps when the countdown appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt`:
- Around line 20397-20400: The generated Kotlin KDoc for getReceiveAddress
incorrectly claims the address is "persistently cached" — update the Rust doc
comment on the get_receive_address function (the source of generated docs) to
accurately describe the cache behaviour (e.g., "deduplicated and cached for up
to 5 minutes or until the wallet's tx count changes") so the generated
getReceiveAddress KDoc/Swift docs reflect the transient in-memory cache rather
than persistence.
---
Outside diff comments:
In `@rust/src/manager/wallet_manager.rs`:
- Around line 927-934: The exported next_address() currently calls
actor.next_address() directly (bypassing ReceiveAddressCache) which will desync
the cache; either remove/deprecate the uniffi method and route callers to
get_receive_address(force_new: true) or make next_address() update the same
cache path: acquire the ReceiveAddressCache mutex, call
self.actor.next_address(), construct the AddressInfoWithDerivation,
update/persist the cache entry and release the lock (mirroring
get_receive_address() cache TTL and tx-count validation) so the cache stays
consistent with BDK; update/remove the #[uniffi::method] annotation accordingly.
---
Duplicate comments:
In `@rust/src/manager/wallet_manager.rs`:
- Around line 888-925: get_receive_address currently swallows DB write errors,
can return stale addresses after an address-type switch, performs an expensive
full transaction iteration to compute tx_count, and uses a magic TTL; fix by (1)
replacing the `let _ = db.global_cache.set_receive_address(&self.id, cache);`
call with the same tap_err + error! logging pattern used elsewhere so write
failures are logged, (2) ensuring the receive-address cache key is address-type
aware or explicitly deleting/invalidate the cached entry from global_cache in
switch_to_different_wallet_address_type (e.g. call the new
delete_receive_address API) so get_receive_address won't return an address from
the old descriptor, (3) adding a cheap tx_count() method on the actor (or
equivalent) and calling that instead of
call!(self.actor.transactions()).await.len(), and (4) extracting the literal `5
* 60` into a named constant RECEIVE_ADDRESS_CACHE_TTL_SECS and renaming the
boolean `time_elapsed` to `is_expired` to clarify semantics; update
get_receive_address to use these changes and to still send
Message::AddressGeneratedTime on both cached and new paths.
---
Nitpick comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt`:
- Around line 157-179: Inside the LaunchedEffect block that updates timeLeftStr
(LaunchedEffect(addressGeneratedTime)), avoid busy-looping by returning
immediately when addressGeneratedTime == 0L instead of reassigning "" each
second; change the loop header to Kotlin style while (true); add an explicit
import kotlinx.coroutines.delay and replace kotlinx.coroutines.delay(1000) with
delay(1000) so the call site is cleaner; ensure the expiration branch still sets
timeLeftStr = "Address Expired" and breaks the loop as before.
- Around line 308-319: ReceiveAddressSheet currently renders Spacer(modifier =
Modifier.height(32.dp)) before and after the conditional that shows timeLeftStr,
causing a 64.dp gap when timeLeftStr is empty; change the layout to reserve a
fixed-height slot for the countdown instead of conditional spacing: replace the
two separate 32.dp Spacers with a single container (e.g., a Box or Column with
fixed height 32.dp or minHeight and the Text for timeLeftStr inside it), or move
the trailing Spacer into the same if branch, referencing the ReceiveAddressSheet
composable and the timeLeftStr / isLoading checks so the Copy button no longer
jumps when the countdown appears.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ccadd1a-989a-4823-9157-f340d943e544
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (8)
android/app/src/main/java/org/bitcoinppl/cove/WalletManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/Flows/SelectedWalletFlow/ReceiveView.swiftios/Cove/WalletManager.swiftrust/crates/cove-types/src/address.rsrust/src/database/global_cache.rsrust/src/manager/wallet_manager.rs
✅ Files skipped from review due to trivial changes (1)
- rust/src/database/global_cache.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rust/crates/cove-types/src/address.rs
- ios/Cove/Flows/SelectedWalletFlow/ReceiveView.swift
| /** | ||
| * Get the receive address for the wallet, deduplicated and persistently cached | ||
| */ | ||
| suspend fun `getReceiveAddress`(`forceNew`: kotlin.Boolean): AddressInfoWithDerivation |
There was a problem hiding this comment.
Minor: docstring wording "persistently cached" is misleading.
The KDoc (propagated from the Rust source's doc comment) says getReceiveAddress is "deduplicated and persistently cached", but per the PR description the cache is a 5‑minute in-memory window that is also invalidated on tx-count changes — not persistent across app restarts. Consider tightening the wording on the Rust side (e.g., "deduplicated and cached for up to 5 minutes or until the wallet's tx count changes") so the generated Kotlin/Swift docs match reality.
Note: the fix belongs in the Rust /// doc comment on get_receive_address, not in this generated file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt` around lines
20397 - 20400, The generated Kotlin KDoc for getReceiveAddress incorrectly
claims the address is "persistently cached" — update the Rust doc comment on the
get_receive_address function (the source of generated docs) to accurately
describe the cache behaviour (e.g., "deduplicated and cached for up to 5 minutes
or until the wallet's tx count changes") so the generated getReceiveAddress
KDoc/Swift docs reflect the transient in-memory cache rather than persistence.
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
Looking good @oyetanishq, 2 things
-
wallet_manager.rs:903-905: The new cache invalidation logic treats any transaction-count change as if the displayed receive address was used. Because both receive screens now call getReceiveAddress(false) whenever loadState changes, unrelated wallet activity such as initial sync progress or an outgoing payment can rotate the receive address even when the cached address is still unused. That causes the QR code/address to change unexpectedly and can burn through the unused-address window.
-
ReceiveView.swift:46-48: When the five-minute countdown reaches zero, the timer only sets "Address Expired" and exits. It never requests a fresh receive address. If the receive screen stays open past expiry and no loadState update happens, the user continues seeing and potentially copying a stale address even though the UI says it will auto-refresh. The Android countdown appears to have the same issue.
Also we want to make sure that IF the current address does get used we auto switch to the next address even if the timer is not up
oyetanishq
force-pushed
the
5min-address
branch
from
18e8b39 to
711b7a8
Compare
Updates:
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
rust/src/manager/wallet_manager.rs (2)
918-924:⚠️ Potential issue | 🟠 MajorSilent
let _ =on DB cache writes still defeats the 5‑minute reuse guarantee.Both
set_receive_addresscall sites (line 920 for the tx_count refresh and line 938 for the newly generated address) still discard errors. If the write fails, nothing is persisted and the next call rotates the address — exactly what issue#386is trying to prevent — with no log to diagnose. Please log withtap_err/error!consistent with line 984–986.🛡️ Suggested pattern
- let _ = db.global_cache.set_receive_address(&self.id, cache); + let _ = db.global_cache.set_receive_address(&self.id, cache).tap_err(|error| { + error!("failed to cache receive address for {}: {error}", self.id); + });Also applies to: 937-939
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager.rs` around lines 918 - 924, The DB cache write errors for set_receive_address are being discarded (using let _ = ...) in both the tx_count refresh path (updated_cache) and the newly generated address path; change these to handle and log errors instead—use tap_err or error! consistent with the existing pattern around lines 984–986 to report failures from db.global_cache.set_receive_address(&self.id, ...), so any persistence failure is logged (include context like self.id and which cache was written: updated_cache or the new address cache) before continuing.
1214-1276:⚠️ Potential issue | 🟠 MajorCache not invalidated on address-type switch — still open.
switch_to_different_wallet_address_typedoes not clearglobal_cache.get_receive_address(&self.id). Since the cache key is only theWalletId(unchanged across the switch) and the cache can remain valid for up to 5 minutes with unchangedtx_count, a subsequentget_receive_addresscall will return an address for the previous address type. Clear the cache in bothFoundAddressesFromJsonandFoundAddressesFromMnemonicbranches before redirecting the route.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager.rs` around lines 1214 - 1276, switch_to_different_wallet_address_type must invalidate the cached receive address so get_receive_address(&self.id) doesn't return the old-type address; add a call to clear the cached entry (e.g. global_cache.clear_receive_address(&self.id) or the equivalent invalidate/remove API) in both the DiscoveryState::FoundAddressesFromJson and DiscoveryState::FoundAddressesFromMnemonic branches just before calling FfiApp::global().load_and_reset_default_route(Route::SelectedWallet(id)); ensure you reference self.id and use the existing global_cache API to remove the cached receive address.
🧹 Nitpick comments (3)
ios/Cove/Flows/SelectedWalletFlow/ReceiveView.swift (1)
26-54: Timer logic looks correct; minor idle-poll nit.When
manager.addressGeneratedTimeisnil, the loop polls every second just to cleartimeRemainingStringand re-check. Since the.onChange(of: manager.addressGeneratedTime, initial: true)handler already restarts the timer when the value flips to non-nil, theelsebranch could simply break out of the loop rather than busy-waiting. Not a correctness issue — just unnecessary wake-ups.♻️ Optional simplification
- guard let generatedTime = manager.addressGeneratedTime else { - timeRemainingString = "" - try? await Task.sleep(for: .seconds(1)) - continue - } + guard let generatedTime = manager.addressGeneratedTime else { + timeRemainingString = "" + return // .onChange will restart the timer when it becomes non-nil + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SelectedWalletFlow/ReceiveView.swift` around lines 26 - 54, The loop in startTimer() busy-waits when manager.addressGeneratedTime is nil; instead of sleeping each second, exit the Task to avoid unnecessary wake-ups: when guard let generatedTime = manager.addressGeneratedTime fails, set timeRemainingString = "" (if needed) and break/return from the Task so the .onChange handler can restart startTimer() later; update timerTask handling accordingly and keep the existing behavior that when remaining <= 0 you set timeRemainingString = "Refreshing..." and call nextAddressSync().android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt (1)
158-180: Infinite loop whenaddressGeneratedTime == 0L— works, but wasteful.When the sentinel
0Lis observed, thewhile(true)loop clearstimeLeftStranddelay(1000)s in a tight poll. BecauseLaunchedEffectis keyed onaddressGeneratedTime, it will restart when the value transitions to non-zero, so a single clear-and-return would suffice instead of polling every second.♻️ Optional simplification
- LaunchedEffect(addressGeneratedTime) { - while(true) { - if (addressGeneratedTime == 0L) { - timeLeftStr = "" - } else { - ... - } - kotlinx.coroutines.delay(1000) - } - } + LaunchedEffect(addressGeneratedTime) { + if (addressGeneratedTime == 0L) { + timeLeftStr = "" + return@LaunchedEffect + } + while (true) { + ... + kotlinx.coroutines.delay(1000) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt` around lines 158 - 180, When addressGeneratedTime == 0L the coroutine should stop polling instead of looping; update the LaunchedEffect logic that contains the while(true) loop so that when addressGeneratedTime == 0L you set timeLeftStr = "" and exit the coroutine (return from the loop/block) rather than calling kotlinx.coroutines.delay(1000) repeatedly. Keep the existing behavior where non-zero addressGeneratedTime continues the countdown and when remaining <= 0 calls onCreateNewAddress() and breaks. Ensure you reference the same variables/functions (addressGeneratedTime, timeLeftStr, onCreateNewAddress, LaunchedEffect) so the early-return replaces the tight poll.rust/src/manager/wallet_manager.rs (1)
894-894: Lock ordering: mutex held across every await inside the function.
address_generation_lockis held throughself.actor.transactions(),self.actor.next_address(), and both DB writes. That's intentional for dedup, but any slow call (network-backed descriptor enumeration, DB contention) blocks all receive-address consumers, including the UI countdown on expiry. Consider releasing the guard after persisting and returning, and/or usingtry_lockwith a waiter coalescing strategy (sharedoneshotresult). Not blocking — just flagging for future resilience work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager.rs` at line 894, The address_generation_lock is held across await points (self.actor.transactions(), self.actor.next_address(), and subsequent DB writes), blocking other receivers; narrow the lock scope so it only surrounds the minimal dedup-critical region: acquire address_generation_lock before calling self.actor.transactions()/self.actor.next_address() to perform the in-memory dedup check, then explicitly drop/release the guard before performing the DB writes and any long awaits (persisting/returning) so the UI and other consumers aren’t blocked; alternatively, replace the exclusive lock with a try_lock plus a waiter-coalescing pattern (shared oneshot result) so callers that lose the lock can wait on a shared result instead of being blocked on the mutex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@rust/src/manager/wallet_manager.rs`:
- Around line 918-924: The DB cache write errors for set_receive_address are
being discarded (using let _ = ...) in both the tx_count refresh path
(updated_cache) and the newly generated address path; change these to handle and
log errors instead—use tap_err or error! consistent with the existing pattern
around lines 984–986 to report failures from
db.global_cache.set_receive_address(&self.id, ...), so any persistence failure
is logged (include context like self.id and which cache was written:
updated_cache or the new address cache) before continuing.
- Around line 1214-1276: switch_to_different_wallet_address_type must invalidate
the cached receive address so get_receive_address(&self.id) doesn't return the
old-type address; add a call to clear the cached entry (e.g.
global_cache.clear_receive_address(&self.id) or the equivalent invalidate/remove
API) in both the DiscoveryState::FoundAddressesFromJson and
DiscoveryState::FoundAddressesFromMnemonic branches just before calling
FfiApp::global().load_and_reset_default_route(Route::SelectedWallet(id)); ensure
you reference self.id and use the existing global_cache API to remove the cached
receive address.
---
Nitpick comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.kt`:
- Around line 158-180: When addressGeneratedTime == 0L the coroutine should stop
polling instead of looping; update the LaunchedEffect logic that contains the
while(true) loop so that when addressGeneratedTime == 0L you set timeLeftStr =
"" and exit the coroutine (return from the loop/block) rather than calling
kotlinx.coroutines.delay(1000) repeatedly. Keep the existing behavior where
non-zero addressGeneratedTime continues the countdown and when remaining <= 0
calls onCreateNewAddress() and breaks. Ensure you reference the same
variables/functions (addressGeneratedTime, timeLeftStr, onCreateNewAddress,
LaunchedEffect) so the early-return replaces the tight poll.
In `@ios/Cove/Flows/SelectedWalletFlow/ReceiveView.swift`:
- Around line 26-54: The loop in startTimer() busy-waits when
manager.addressGeneratedTime is nil; instead of sleeping each second, exit the
Task to avoid unnecessary wake-ups: when guard let generatedTime =
manager.addressGeneratedTime fails, set timeRemainingString = "" (if needed) and
break/return from the Task so the .onChange handler can restart startTimer()
later; update timerTask handling accordingly and keep the existing behavior that
when remaining <= 0 you set timeRemainingString = "Refreshing..." and call
nextAddressSync().
In `@rust/src/manager/wallet_manager.rs`:
- Line 894: The address_generation_lock is held across await points
(self.actor.transactions(), self.actor.next_address(), and subsequent DB
writes), blocking other receivers; narrow the lock scope so it only surrounds
the minimal dedup-critical region: acquire address_generation_lock before
calling self.actor.transactions()/self.actor.next_address() to perform the
in-memory dedup check, then explicitly drop/release the guard before performing
the DB writes and any long awaits (persisting/returning) so the UI and other
consumers aren’t blocked; alternatively, replace the exclusive lock with a
try_lock plus a waiter-coalescing pattern (shared oneshot result) so callers
that lose the lock can wait on a shared result instead of being blocked on the
mutex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c15bee69-45f7-4450-9e12-42ba5338c4bf
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (8)
android/app/src/main/java/org/bitcoinppl/cove/WalletManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/ReceiveAddressSheet.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/Flows/SelectedWalletFlow/ReceiveView.swiftios/Cove/WalletManager.swiftrust/crates/cove-types/src/address.rsrust/src/database/global_cache.rsrust/src/manager/wallet_manager.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- rust/src/database/global_cache.rs
- android/app/src/main/java/org/bitcoinppl/cove/WalletManager.kt
- rust/crates/cove-types/src/address.rs
|
heyy @praveenperera can you please review this pr!! |
| let mut address_was_used = false; | ||
| if current_tx_count != cache.tx_count { | ||
| let next_address = | ||
| call!(self.actor.next_address()).await.map_err_str(Error::NextAddressError)?; | ||
|
|
||
| // If BDK's next unused address index is greater than cached index | ||
| // It means cached address received funds and BDK moved the pointer | ||
| address_was_used = next_address.index() > cache.address_info.index(); | ||
| } |
There was a problem hiding this comment.
This validation path advances wallet state. When current_tx_count != cache.tx_count, calling self.actor.next_address() here is not a read-only check: wallet.get_next_address() reveals and persists a fresh external address whenever the gap window is not full.
That means an unrelated transaction-count change, like creating a send, can burn an unseen receive address even though this method still returns the cached address. This should use a non-mutating check instead of advancing wallet state during cache validation.
Look at peek_address
There was a problem hiding this comment.
Well looking at the function peek_address, I thought of another way to store address in cache, then check if the stored address is still available in unused addresses list, if yes then no need to update, otherwise get next_address.
| let new_address = | ||
| call!(self.actor.next_address()).await.map_err_str(Error::NextAddressError)?; | ||
| let now = jiff::Timestamp::now().as_second() as u64; |
There was a problem hiding this comment.
This can skip the immediate next receive address after the cached one is used. If current_tx_count != cache.tx_count, the earlier self.actor.next_address() call has already advanced BDK to the next valid receive address. Calling next_address() again here advances a second time, so the UI ends up showing the address after that and the immediate successor is never presented. This can happen as soon as a receive to the cached address increases the wallet’s transaction count before the sheet reloads.
There was a problem hiding this comment.
should we remove this tx_count??, ig its adding unnecessary complexity, and just check if address is present in unused addresses?
oyetanishq
force-pushed
the
5min-address
branch
2 times, most recently
from
54fc5df to
d964350
Compare
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
I found one cross-platform issue: reopening receive after the cached timestamp is stale can trigger both the initial address load and the timer refresh, revealing two addresses back-to-back.
| } | ||
| } else { | ||
| timeLeftStr = "Refreshing..." | ||
| onCreateNewAddress() |
There was a problem hiding this comment.
This timer can call onCreateNewAddress() while the initial LaunchedEffect(manager, manager.loadState) is still fetching the receive address. If addressGeneratedTime is stale from a previous sheet, the initial fetch may already refresh the expired cache, then this forced refresh reveals another address immediately afterward. That skips the first generated address and consumes gap-limit space on reopen. Gate the timer until the current address has loaded, or have the initial fetch handle expiration and start the countdown only after the refreshed timestamp arrives.
| } | ||
| } else { | ||
| timeRemainingString = "Refreshing..." | ||
| nextAddressSync() |
There was a problem hiding this comment.
This timer can fire before the initial .task { await nextAddress() } has completed when manager.addressGeneratedTime is stale from a previous receive sheet. The initial load already refreshes an expired cache via getReceiveAddress(forceNew: false); if this forced call queues behind it, the app reveals a second address immediately, skipping the first new receive address and unnecessarily consuming gap-limit space. Gate auto-refresh until an address has been loaded for the current sheet/timestamp, or let the initial load own expiry and only start the timer after the new generated time is published.
oyetanishq
force-pushed
the
5min-address
branch
from
d964350 to
fd1bf70
Compare
…tion` Feat cache receive address in redb table Fix clippy error - collapsed if statement Generate bindings for both platforms Fix update cache receive address when used, and timeout Replace next_address with next_peek_address Spelling mistake Check receive address in unused list, instead of peek Fix rust fmt error Fix trigger both inital load and refresh, revaling two addresses Fix android-compile error - initalLoadComplete value pass in preview
oyetanishq
force-pushed
the
5min-address
branch
from
fd1bf70 to
28a0a8e
Compare
2 participants
Summary
This commit introduces address caching to the WalletManager on both platforms:
5 minutesbefore a new one is requested.transaction count changes, ensuring a genuinely new address is shown if the current one receives pending or confirmed funds.manually force a new addressgeneration via the "Create New Address" button.Testing
Platform Coverage
Checklist
Summary by CodeRabbit