fix(dashpay): fix DPNS username resolution in contact requests#723
fix(dashpay): fix DPNS username resolution in contact requests#723thepastaclaw wants to merge 4 commits into
Conversation
The resolve_username_to_identity function had three issues that caused 'Add Contact by Username' to fail silently: 1. Missing normalizedParentDomainName where clause - DPNS queries require both normalizedParentDomainName and normalizedLabel for the domain document type index. Without it, the query returns empty results even for existing usernames. 2. Used document.owner_id() instead of records.identity - The DPNS domain document's owner may differ from the identity the name resolves to. The correct field is records.identity. 3. No homograph-safe normalization - The label was only lowercased but not run through convert_to_homograph_safe_chars, which is required for consistent DPNS lookups. Additionally, replaced the redundant DPNS contract fetch with the app_context's cached dpns_contract, consistent with the rest of the codebase. Closes dashpay#687
|
@coderabbitai review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughresolve_username_to_identity now accepts Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "Add Contact UI"
participant Task as "Backend Task"
participant App as "AppContext (dpns_contract)"
participant DPNS as "DPNS Service"
participant IDSvc as "Identity Service"
UI->>Task: send_contact_request_with_proof(username)
Task->>App: resolve_username_to_identity(app_context, sdk, username)
App->>DPNS: queryDocuments(filter: normalizedParentDomainName=="dash", normalizedLabel)
alt transient DPNS error
DPNS-->>App: error (e.g., "height is outdated"/"try another server")
App->>DPNS: queryDocuments (retry up to MAX_RETRIES)
end
DPNS-->>App: DPNS document (records.identity)
App->>IDSvc: fetchIdentity(records.identity)
alt transient identity fetch error
IDSvc-->>App: error (transient)
App->>IDSvc: fetchIdentity (retry up to MAX_RETRIES)
end
IDSvc-->>App: Identity
App-->>Task: Identity
Task-->>UI: success / mapped NetworkError (platform-sync)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
✅ Actions performedReview triggered.
|
|
Added retry logic for transient "height is outdated" / "try another server" errors (up to 3 retries on both DPNS query and identity fetch). Also added UI-side mapping so these errors show a user-friendly message and the Retry button. This subsumes the fix from tracker-529 (same issue, different manifestation). @coderabbitai review |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/backend_task/dashpay/contact_requests.rs (1)
185-205:⚠️ Potential issue | 🟡 MinorThe retry fix only covers usernames right now.
The
.dashpaths now benefit from the new retry logic, but the direct identity-ID branch still does a one-shotIdentity::fetch. Transient"height is outdated"/"try another server"failures will still surface immediately for the other supported input mode, so this reliability fix is only applied to half of the flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/dashpay/contact_requests.rs` around lines 185 - 205, The identity-ID branch currently does a one-shot Identity::fetch(sdk, to_id) whereas the username paths use the new retry logic; update the to_identity resolution so Identity::fetch is retried on transient RPC errors (e.g., messages containing "height is outdated" or "try another server") similar to resolve_username_to_identity's retry/backoff behavior. Locate the match arm that handles Identifier::from_string_try_encodings and replace the direct Identity::fetch call with the same retry wrapper or loop used for usernames (or extract the retry logic into a helper and call it here), preserving the existing error mapping (map_err/map_ok and not-found handling) so failures after retries still produce the same formatted errors.
🧹 Nitpick comments (1)
src/backend_task/dashpay/contact_requests.rs (1)
507-511: Avoid extending the stringly typed error contract here.These new branches add more backend error text that
AddContactScreenhas to classify by substring. Please surface a typedTaskErrorwith a DashPay-specific variant instead of moreStringcases, otherwise small wording changes will keep breaking the UI mapping.As per coding guidelines, "Backend tasks should return
Result<T, TaskError>fromsrc/backend_task/error.rs. When adding new backend error types, add a#[from]variant toTaskErrorrather than converting toString."Also applies to: 559-563, 621-627
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/dashpay/contact_requests.rs` around lines 507 - 511, Change the function signature of resolve_username_to_identity to return Result<Identity, TaskError> (import TaskError from src/backend_task/error.rs), stop returning raw String errors and instead return a typed DashPay-specific TaskError variant; add a new DashPayError type (or similar) and add a #[from] variant to TaskError like DashPay(#[from] DashPayError) so you can convert concrete DashPay errors into TaskError easily, replace all Err(String::from(...)) in resolve_username_to_identity (and the other similar branches noted) with Err(DashPayError::new(...).into()) or Err(DashPayErrorVariant.into()), and update callers to handle TaskError accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/dashpay/add_contact_screen.rs`:
- Around line 656-662: The branch in AddContactScreen constructs
DashPayError::NetworkError with a specific reason string but
DashPayError::user_message() currently ignores NetworkError.reason and returns a
generic message; update the error handling so the UI can show the curated reason
by either (a) modifying DashPayError::user_message() to return
NetworkError.reason when present (preserve generic fallback), or (b) introducing
a new enum variant (e.g., DashPayError::PlatformOutOfSync or
DashPayError::RecoverableError) used by AddContactScreen and handled in
user_message() to return the specific out-of-sync text; change the code paths
that construct NetworkError in AddContactScreen to use the new variant if you
choose option (b).
---
Outside diff comments:
In `@src/backend_task/dashpay/contact_requests.rs`:
- Around line 185-205: The identity-ID branch currently does a one-shot
Identity::fetch(sdk, to_id) whereas the username paths use the new retry logic;
update the to_identity resolution so Identity::fetch is retried on transient RPC
errors (e.g., messages containing "height is outdated" or "try another server")
similar to resolve_username_to_identity's retry/backoff behavior. Locate the
match arm that handles Identifier::from_string_try_encodings and replace the
direct Identity::fetch call with the same retry wrapper or loop used for
usernames (or extract the retry logic into a helper and call it here),
preserving the existing error mapping (map_err/map_ok and not-found handling) so
failures after retries still produce the same formatted errors.
---
Nitpick comments:
In `@src/backend_task/dashpay/contact_requests.rs`:
- Around line 507-511: Change the function signature of
resolve_username_to_identity to return Result<Identity, TaskError> (import
TaskError from src/backend_task/error.rs), stop returning raw String errors and
instead return a typed DashPay-specific TaskError variant; add a new
DashPayError type (or similar) and add a #[from] variant to TaskError like
DashPay(#[from] DashPayError) so you can convert concrete DashPay errors into
TaskError easily, replace all Err(String::from(...)) in
resolve_username_to_identity (and the other similar branches noted) with
Err(DashPayError::new(...).into()) or Err(DashPayErrorVariant.into()), and
update callers to handle TaskError accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fad7b015-1281-43b9-a9ff-8ca1ad2d391c
📒 Files selected for processing (2)
src/backend_task/dashpay/contact_requests.rssrc/ui/dashpay/add_contact_screen.rs
- Add retry logic for transient platform errors in the identity-ID branch of send_contact_request_with_proof, matching the existing retry behavior in resolve_username_to_identity - Update DashPayError::user_message() to surface curated platform out-of-sync reasons from NetworkError instead of always showing generic network text
|
The two actionable findings from CodeRabbit's review (retry logic missing on the identity-ID branch, and The remaining nitpick about switching from @coderabbitai review |
|
Would you like me to open a follow-up GitHub issue to track the typed error refactor for the DashPay backend error flow? I'll kick off the incremental review of the latest changes now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend_task/dashpay/contact_requests.rs`:
- Around line 207-224: The retry branches that currently check err.contains("try
another server") and err.contains("height is outdated") should also check for
the SDK/server wording "temporarily out of sync" so transient errors are treated
consistently with DashPayError::user_message(); update the conditional in the
identity-fetch retry block (references: retries, MAX_RETRIES, to_username_or_id,
e) to include err.contains("temporarily out of sync") and mirror the same change
in the other two retry blocks noted (the blocks around the other occurrences at
the regions you flagged), ensuring the same retry/increment/continue behavior
and the same final Err(...) message when exhausted.
- Around line 542-546: Replace the current naive split that truncates on the
first '.' by explicitly accepting only an optional ".dash" suffix: check if the
input string `username` ends_with(".dash") and if so set `name` =
username.strip_suffix(".dash").unwrap(); otherwise, if `username` contains any
'.' return an error; finally ensure `name` is non-empty and contains no '.'
before proceeding (update the error message used in the existing ok_or_else to
reflect invalid dotted usernames). This uses the existing `username` and `name`
identifiers so you can locate the logic and safely reject malformed dotted
inputs instead of truncating them.
In `@src/backend_task/dashpay/errors.rs`:
- Around line 145-155: The DashPayError::NetworkError arm currently returns the
raw backend `reason` when it contains platform-sync phrases; change it to return
a single curated user-facing string instead of `reason.clone()` so the Retry
copy doesn't leak SDK/transport details. In the match arm that checks
`reason.contains("temporarily out of sync") || reason.contains("height is
outdated") || reason.contains("try another server")`, replace returning
`reason.clone()` with a fixed message like "Wallet data temporarily out of sync,
please try again shortly." and keep the existing generic fallback for all other
cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c9ecccfe-580c-42be-91c1-97bddda62060
📒 Files selected for processing (2)
src/backend_task/dashpay/contact_requests.rssrc/backend_task/dashpay/errors.rs
- Extract is_transient_platform_sync_error() helper for consistent transient error detection across all retry paths, including 'temporarily out of sync' alongside existing checks - Reject dotted usernames instead of silently truncating them: only accept 'alice' or 'alice.dash', reject 'alice.test' etc. - Return fixed user-facing message in NetworkError::user_message() instead of leaking raw SDK/transport reason strings
|
Addressed all 3 findings from the second CodeRabbit review:
@coderabbitai review |
thepastaclaw
force-pushed
the
fix/add-contact-username-resolution
branch
from
fdc4bca to
1873064
Compare
|
Third round of CodeRabbit feedback addressed:
@coderabbitai review |
|
Kicking off the incremental review now. ✅ Actions performedReview triggered.
|
|
Kicking off the incremental review now. ✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
DPNS username resolution fix is correct — the core issue (missing normalizedParentDomainName in compound-index query) is properly addressed. Also fixes identity extraction, adds homograph normalization, and adds retry logic. Two maintainability suggestions: retry logic is copy-pasted 3 times (extract a helper), and the invalid-username error message still references the old .dash-only rule.
Reviewed commit: 1873064
🟡 2 suggestion(s)
1 additional finding
🟡 suggestion: Error message still describes old .dash-only rule
src/backend_task/dashpay/errors.rs (line 172)
resolve_username_to_identity() now accepts both bare labels and fully qualified names, but InvalidUsername::user_message() still tells users usernames must end with .dash. The error text should be updated to match the new parsing rule: accept 'alice' or 'alice.dash', reject other dotted forms.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/backend_task/dashpay/contact_requests.rs`:
- [SUGGESTION] lines 207-232: Retry logic copy-pasted 3 times with no backoff
The same retry loop pattern (loop + is_transient_platform_sync_error check + increment + continue) is duplicated at lines 207-232, 575-596, and 633-659. Extract a generic async retry helper to reduce maintenance drift risk.
In `src/backend_task/dashpay/errors.rs`:
- [SUGGESTION] line 172: Error message still describes old .dash-only rule
resolve_username_to_identity() now accepts both bare labels and fully qualified names, but InvalidUsername::user_message() still tells users usernames must end with .dash. The error text should be updated to match the new parsing rule: accept 'alice' or 'alice.dash', reject other dotted forms.
| loop { | ||
| match Identity::fetch(sdk, to_id).await { | ||
| Ok(Some(identity)) => break identity, | ||
| Ok(None) => { | ||
| return Err(format!("Identity {} not found", to_username_or_id)); | ||
| } | ||
| Err(e) => { | ||
| let err = e.to_string(); | ||
| if is_transient_platform_sync_error(&err) && retries < MAX_RETRIES { | ||
| retries += 1; | ||
| tracing::warn!( | ||
| "Retrying identity fetch for '{}' (attempt {}/{}): {}", | ||
| to_username_or_id, | ||
| retries, | ||
| MAX_RETRIES, | ||
| e | ||
| ); | ||
| continue; | ||
| } | ||
| if is_transient_platform_sync_error(&err) { | ||
| return Err("Platform servers are temporarily out of sync. Please try again in a moment.".to_string()); | ||
| } | ||
| return Err(format!("Failed to fetch identity: {}", e)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Retry logic copy-pasted 3 times with no backoff
The same retry loop pattern (loop + is_transient_platform_sync_error check + increment + continue) is duplicated at lines 207-232, 575-596, and 633-659. Extract a generic async retry helper to reduce maintenance drift risk.
source: ['claude-general']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/backend_task/dashpay/contact_requests.rs`:
- [SUGGESTION] lines 207-232: Retry logic copy-pasted 3 times with no backoff
The same retry loop pattern (loop + is_transient_platform_sync_error check + increment + continue) is duplicated at lines 207-232, 575-596, and 633-659. Extract a generic async retry helper to reduce maintenance drift risk.
|
Superseded by #810 |
2 participants
Issue
Fixes #687
Description
Adding a contact by username silently fails — the DPNS lookup always returns empty results.
Root Cause
resolve_username_to_identitywas missing the requirednormalizedParentDomainNamewhere clause in the DPNS document query. DPNS uses a compound index on(normalizedParentDomainName, normalizedLabel)— querying with onlynormalizedLabelreturns empty results silently.Fix
normalizedParentDomainName = "dash"where clause — this is the primary fix that makes the DPNS query actually match the document type indexdocument.owner_id()torecords.identity— the DPNS document'srecords.identityfield is the authoritative identity reference, which may differ from the document ownerconvert_to_homograph_safe_charsnormalization — consistent with how DPNS registration normalizes labelsdpns_contractfromapp_contextinstead of fetching it from the network on every lookupTesting
cargo check✅cargo clippy✅Summary by CodeRabbit
Refactor
Bug Fixes / Reliability