fix: address Lukasz review comments from PR #564#583
Merged
PastaPastaPasta merged 3 commits intoFeb 17, 2026
Merged
Conversation
1. CRITICAL: Fix BitOrAssign dropping tasks in fetch_unresolved_profiles()
- Changed from action |= AppAction::BackendTask(task) loop (which only
kept the last task) to collecting all tasks into a Vec and dispatching
via AppAction::BackendTasks with Concurrent execution mode.
2. HIGH: Remove dummy [0x02; 33] pubkey in load_payment_history()
- Changed PaymentRecord.to_address from Address to Option<Address>
- Historical records loaded from DB now use None instead of generating
a fake-but-valid P2PKH address from a dummy public key.
3. HIGH: Fix i64 as u64 wrapping for amounts (payments.rs and dashpay.rs)
- Added bounds checking with warning logs for negative amounts,
clamping to 0 instead of silently wrapping.
4. MEDIUM: Eliminate duplicate payment history logic in dashpay.rs
- LoadPaymentHistory task handler now calls the shared
payments::load_payment_history() and converts results, instead of
reimplementing the same logic with different return types.
5. MEDIUM: Fix N+1 database queries in resolve_names_from_local_cache()
- Pre-load all contacts once before the loop and build a HashMap
for O(1) lookups instead of calling load_dashpay_contacts() and
load_dashpay_profile() per request inside the loop.
6. MEDIUM: Fix created_at: None breaking filters in contacts_list.rs
- Contacts from DashPayContactsWithInfo results now get current
timestamp as fallback instead of None, so they work with
'Recent' filter and 'Date added' sort.
7. LOW: Fix 'failed' as failure reason in payment status
- Changed PaymentStatus::Failed to use descriptive 'Transaction failed'
string instead of echoing the literal status column value 'failed'.
8. LOW: Fix i64 as u64 for timestamps in payments.rs
- Added bounds checking with warning for negative timestamps,
consistent with the amount fix.
9. LOW: Both i64 as u64 locations fixed (dashpay.rs duplicate removed
entirely by fix dashpay#4, payments.rs fixed by fixes dashpay#3/dashpay#8).
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
PastaPastaPasta
merged commit 2c81c86
into
dashpay:feat/dashpay-contacts-improvements
3 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses all 9 review comments from @lklimek on PR #564.
Changes
1. CRITICAL —
BitOrAssigndrops tasks (contact_requests.rs)fetch_unresolved_profiles()usedaction |= AppAction::BackendTask(task)in a loop, butBitOrAssignforAppActionsimply replacesselfwithrhs— only the last profile fetch would fire. Fixed by collecting all tasks into aVecand usingAppAction::BackendTasks(..., Concurrent).2. HIGH — Dummy
[0x02; 33]pubkey (payments.rs)load_payment_history()created a fake-but-valid P2PKH address from a dummy compressed public key. ChangedPaymentRecord.to_addresstoOption<Address>and useNonefor historical records loaded from the database.3. HIGH —
i64 as u64wrapping for amounts (payments.rs)sp.amount as u64silently wraps negative values. Added bounds checking withtracing::warn!for negative amounts, clamping to 0.4. MEDIUM — Duplicate payment history logic (
dashpay.rs)DashPayTask::LoadPaymentHistoryhandler had its own inline implementation duplicatingpayments::load_payment_history(). Refactored to call the shared function and convert results.5. MEDIUM — N+1 database queries (
contact_requests.rs)resolve_names_from_local_cache()calledload_dashpay_contacts()per-request inside the loop. Fixed by pre-loading all contacts once into aHashMapfor O(1) lookups.6. MEDIUM —
created_at: Nonebreaks filters (contacts_list.rs)Contacts from
DashPayContactsWithInforesults gotcreated_at: None, breaking "Recent" filter and "Date added" sort. Fixed by using current timestamp as fallback.7. LOW —
"failed"as failure reason (payments.rs)PaymentStatus::Failed(sp.status.clone())stored the literal string "failed". Changed to use descriptive "Transaction failed" message.8. LOW —
i64 as u64for timestamps (payments.rs)Same wrapping risk as #3 for
sp.created_at as u64. Fixed consistently with bounds checking.9. LOW —
i64 as u64in dashpay.rsEliminated entirely by fix #4 (duplicate code removed).