fix(dashpay): DPNS normalization, contact resolution, key type, and privateData fixes#810
Conversation
The profile search used `to_lowercase()` to normalize the query, but DPNS stores normalizedLabel using homograph-safe conversion (o→0, i/l→1 plus lowercase). Searching for "supertestingnameabc123" would never match because the on-chain label is "supertest1ngnameabc123". Use `convert_to_homograph_safe_chars()` from the SDK, matching the same normalization used by `Sdk::search_dpns_names()`. Also strip `.dash` suffix if present. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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 DPNS normalization utilities and applies them across username resolution, profile search, and identity lookup; enforces a 16-byte minimum plaintext padding for DashPay privateData before encryption and validates encrypted field sizes; expands acceptable auth key types for document create/replace; updates error variant for contact-info validation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
…solution Same root cause as the profile search fix: resolve_username_to_identity() used to_lowercase() instead of convert_to_homograph_safe_chars() when querying normalizedLabel. Names containing i, l, or o would fail with UsernameResolutionFailed. Also unify profile search to use dpp::util::strings import path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
refs #687 |
- Trim input in resolve_username_to_identity (contact requests) - Trim and strip .dash suffix in load_identity_by_dpns_name Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…me resolution resolve_username_to_identity() queried normalizedLabel without the required normalizedParentDomainName == "dash" filter. The DPNS index requires both fields — without the parent domain, the query matched no documents and always returned UsernameResolutionFailed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
create_or_update_contact_info() restricted signing keys to ECDSA_SECP256K1, but many Platform identities use BLS12_381 keys. This caused "MissingAuthenticationKey" when rejecting contact requests (which creates a contactInfo document), while accepting worked because it uses KeyType::all_key_types(). Accept any key type — Platform accepts all for document state transitions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
changed the title
|
✅ Review complete (commit cd90c21) |
There was a problem hiding this comment.
Pull request overview
This PR fixes DashPay/DPNS username lookup behaviors by aligning client-side normalization and DPNS query filters with how DPNS stores and indexes domain documents, and by relaxing overly strict key-type selection for DashPay contactInfo signing.
Changes:
- Switch DPNS lookups (profile search, identity load by name, username resolution) from plain
to_lowercase()to DPNS homograph-safe normalization, and trim/strip.dashsuffix. - Fix “Add Contact” username resolution by adding the required
normalizedParentDomainName == "dash"filter. - Allow any authentication key type (BLS/ECDSA/EDDSA) when selecting a signing key for contactInfo updates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/backend_task/identity/load_identity_by_dpns_name.rs | Trims and strips .dash before homograph-safe normalization for DPNS identity lookup. |
| src/backend_task/dashpay/profile.rs | Updates DPNS profile search normalization to use homograph-safe conversion and strip .dash. |
| src/backend_task/dashpay/contact_requests.rs | Fixes username-to-identity resolution by trimming input, adding normalizedParentDomainName == "dash", and using homograph-safe normalization. |
| src/backend_task/dashpay/contact_info.rs | Expands signing key selection to allow all key types for contactInfo document transitions. |
Comments suppressed due to low confidence (1)
src/backend_task/dashpay/contact_requests.rs:527
username.split('.').next()can never returnNone, so theInvalidUsernamebranch is effectively dead code. More importantly, this will silently accept inputs likealice.test/alice.test.dashand resolve them asalice.dash(everything after the first dot is ignored), which can lead to resolving/sending a contact request to an unintended identity. Consider validating that the label is non-empty and that any suffix is exactly.dash(ideally case-insensitive), otherwise returnInvalidUsername.
let name = username.split('.').next().ok_or_else(|| {
TaskError::DashPay(DashPayError::InvalidUsername {
username: username.to_string(),
})
})?;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
strip_suffix(".dash") is case-sensitive — inputs like "Alice.DASH" or
"alice.Dash" would not have the suffix removed, causing lookup failures.
Use case-insensitive check before stripping.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
send_contact_request_with_proof() used ends_with(".dash") which is
case-sensitive. "Alice.DASH" would fall through to the ID parser,
fail, then get ".dash" appended again → "Alice.DASH.dash".
Also trim whitespace from the input.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/backend_task/dashpay/contact_requests.rs:526
- This parsing accepts empty labels (e.g., input ".dash" or " .dash"), because
split('.').next()returnsSome(\"\"). That leads to a DPNS query for an emptynormalizedLabeland ultimatelyUsernameResolutionFailedinstead ofInvalidUsername. Add an explicit check thatnameis non-empty (and potentially reject unexpected extra dots if not supported) before building the query.
let username = username.trim();
// Parse username (e.g., "alice.dash" -> "alice")
let name = username.split('.').next().ok_or_else(|| {
TaskError::DashPay(DashPayError::InvalidUsername {
username: username.to_string(),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Centralize the "trim → strip .dash suffix → homograph-safe normalize" pipeline into normalize_dpns_label(), strip_dash_suffix(), and has_dash_suffix() helpers. All 4 DPNS-by-name lookup sites now use the shared helper instead of inline logic. Includes 7 unit tests covering suffix stripping, case-insensitivity, whitespace trimming, and homograph character mapping. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/model/dpns.rs (1)
41-93: Good test coverage; consider adding edge cases for empty/whitespace inputs.The tests comprehensively cover suffix stripping, case-insensitivity, whitespace trimming, and homograph mappings. For completeness, consider adding tests for empty strings and whitespace-only inputs to document expected behavior.
🧪 Optional: Additional edge case tests
#[test] fn strip_suffix_cases() { assert_eq!(strip_dash_suffix("alice.dash"), "alice"); assert_eq!(strip_dash_suffix("alice.DASH"), "alice"); assert_eq!(strip_dash_suffix("alice"), "alice"); assert_eq!(strip_dash_suffix("a.dash"), "a"); // valid: label "a" with .dash suffix } + + #[test] + fn normalize_empty_and_whitespace() { + assert_eq!(normalize_dpns_label(""), ""); + assert_eq!(normalize_dpns_label(" "), ""); + } + + #[test] + fn has_suffix_whitespace_handling() { + // has_dash_suffix trims internally + assert!(has_dash_suffix(" alice.dash ")); + assert!(!has_dash_suffix(" ")); // whitespace-only + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/dpns.rs` around lines 41 - 93, Add tests for empty-string and whitespace-only inputs to cover edge cases for normalize_dpns_label, has_dash_suffix, and strip_dash_suffix: assert expected outputs when given "" and strings like " " (e.g., normalize_dpns_label("") and normalize_dpns_label(" "), has_dash_suffix("") and has_dash_suffix(" "), strip_dash_suffix("") and strip_dash_suffix(" ")), ensuring consistent behavior (document expected return values such as empty string or trimmed result) so the test module includes these cases alongside existing ones.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/model/dpns.rs`:
- Around line 41-93: Add tests for empty-string and whitespace-only inputs to
cover edge cases for normalize_dpns_label, has_dash_suffix, and
strip_dash_suffix: assert expected outputs when given "" and strings like " "
(e.g., normalize_dpns_label("") and normalize_dpns_label(" "),
has_dash_suffix("") and has_dash_suffix(" "), strip_dash_suffix("") and
strip_dash_suffix(" ")), ensuring consistent behavior (document expected
return values such as empty string or trimmed result) so the test module
includes these cases alongside existing ones.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 596fab88-4363-40de-ab68-8c948c1c04d2
📒 Files selected for processing (5)
src/backend_task/dashpay/contact_requests.rssrc/backend_task/dashpay/profile.rssrc/backend_task/identity/load_identity_by_dpns_name.rssrc/model/dpns.rssrc/model/mod.rs
✅ Files skipped from review due to trivial changes (1)
- src/model/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/backend_task/identity/load_identity_by_dpns_name.rs
- src/backend_task/dashpay/contact_requests.rs
The DashPay contract requires privateData to be 48-2048 bytes (minItems: 48). When rejecting a contact request with no nickname, no note, and no accepted accounts, the serialized data was only 8 bytes → 32 bytes after AES-CBC encryption (16 IV + 16 ciphertext). Pad plaintext to 17 bytes minimum so encrypted output is at least 48 bytes (16 IV + 32 ciphertext). The trailing zero padding is harmless — the deserializer reads length-prefixed fields and ignores trailing bytes. Accept was unaffected because it creates a contactRequest document (no privateData field), not a contactInfo document. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…size Replace zero-padding with random bytes to avoid leaking plaintext length to observers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
First padding byte is 0x00 so deserializers can detect where real data ends and padding begins. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
changed the title
- Use DASH_SUFFIX constant + eq_ignore_ascii_case instead of byte slicing in dpns helpers (safe for non-ASCII UTF-8 inputs) - Remove unnecessary normalized.clone() in contact_requests.rs - Fix MIN_PLAINTEXT_SIZE: 16 bytes suffice (PKCS7 pads block-aligned input to 32 → 48 with IV) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/backend_task/dashpay/contact_info.rs (2)
411-422: Consider a shared helper for DashPay signing-key selection.This now mirrors similar
get_first_public_key_matching(Purpose::AUTHENTICATION, …)logic insrc/backend_task/dashpay/contact_requests.rs:634-644andsrc/backend_task/dashpay/profile.rs:120-129. A helper would keep the security-level / key-type policy intentional and reduce drift across document create/replace flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/dashpay/contact_info.rs` around lines 411 - 422, Create a shared helper (e.g., get_dashpay_signing_key or select_dashpay_signing_key) that encapsulates the repeated call to identity.identity.get_first_public_key_matching with Purpose::AUTHENTICATION, the SecurityLevel set {CRITICAL, HIGH, MEDIUM}, and KeyType::all_key_types(), and replace the duplicated inline logic in contact_info (signing_key), contact_requests, and profile with calls to this helper so the security-level/key-type policy is centralized and consistent across create/replace flows.
42-46: Keep the minimum-size workaround in the encryption layer.
serialize()is public, and it now injects random bytes to satisfy an encryption-specific size invariant. Consider moving this padding intoencrypt_private_data()(or a small helper next to it) and covering the 15/16-byte boundary there, so the serializer stays canonical while the 48-byte contract invariant stays protected.Also applies to: 82-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/dashpay/contact_info.rs` around lines 42 - 46, The serializer currently injects random bytes to meet an encryption-size invariant (MIN_PLAINTEXT_SIZE) — move that padding logic out of serialize() and into the encryption layer: modify encrypt_private_data() (or add a small helper next to it) to enforce the 15/16-byte boundary and ensure the plaintext passed to AES-CBC is padded to at least MIN_PLAINTEXT_SIZE before encryption; remove any random-byte injection from serialize() so it remains canonical, and update/remove the related constants/usages around MIN_PLAINTEXT_SIZE and the code referenced in lines 82-95 accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/backend_task/dashpay/contact_info.rs`:
- Around line 411-422: Create a shared helper (e.g., get_dashpay_signing_key or
select_dashpay_signing_key) that encapsulates the repeated call to
identity.identity.get_first_public_key_matching with Purpose::AUTHENTICATION,
the SecurityLevel set {CRITICAL, HIGH, MEDIUM}, and KeyType::all_key_types(),
and replace the duplicated inline logic in contact_info (signing_key),
contact_requests, and profile with calls to this helper so the
security-level/key-type policy is centralized and consistent across
create/replace flows.
- Around line 42-46: The serializer currently injects random bytes to meet an
encryption-size invariant (MIN_PLAINTEXT_SIZE) — move that padding logic out of
serialize() and into the encryption layer: modify encrypt_private_data() (or add
a small helper next to it) to enforce the 15/16-byte boundary and ensure the
plaintext passed to AES-CBC is padded to at least MIN_PLAINTEXT_SIZE before
encryption; remove any random-byte injection from serialize() so it remains
canonical, and update/remove the related constants/usages around
MIN_PLAINTEXT_SIZE and the code referenced in lines 82-95 accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d29ddbcd-f939-4e88-b2ba-227fca33a631
📒 Files selected for processing (3)
src/backend_task/dashpay/contact_info.rssrc/backend_task/dashpay/contact_requests.rssrc/model/dpns.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/model/dpns.rs
- src/backend_task/dashpay/contact_requests.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #810 introduces DPNS normalization helpers, DashPay contact info encryption, and key type flexibility. Six findings verified against source; three are blocking. The convergent UTF-8 panic finding is confirmed STILL PRESENT — the fix commit cb4652e only replaced magic numbers with a constant but did not add is_char_boundary guards. Two additional medium-severity issues: rootEncryptionKeyIndex stores the wrong key ID, and a hardcoded DPNS contract ID breaks non-mainnet usage.
Reviewed commit: a9c0762
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 3 nitpick(s)
4 additional findings
🟡 suggestion: rootEncryptionKeyIndex written from authentication signing key, not encryption root key
src/backend_task/dashpay/contact_info.rs (line 1)
Line 435 writes signing_key.id() into rootEncryptionKeyIndex. The signing_key is an AUTHENTICATION-purpose key (lines 413-424), but rootEncryptionKeyIndex per DIP-15 should reference the identity key used as the root of the encryption key derivation tree. The read path at line 353 ignores this value (_root_idx), so self-decryption works, but other DashPay clients (e.g., mobile wallets) that rely on this field to find the correct encryption key will fail to decrypt contactInfo.
🟡 suggestion: Hardcoded DPNS contract ID breaks non-mainnet contact requests
src/backend_task/dashpay/contact_requests.rs (line 1)
resolve_username_to_identity() hardcodes the mainnet DPNS contract ID (GWRSAVFMjXx8HpQFaNJMqBV7MBgMK4br5UESsB4S31Ec) and re-fetches it from the network, while every other DPNS lookup uses app_context.dpns_contract (which is network-aware). On testnet/devnet, this hardcoded ID won't match, causing contact request username resolution to fail.
💬 nitpick: Sentinel-based padding in ContactInfoPrivateData lacks corresponding deserializer
src/backend_task/dashpay/contact_info.rs (line 1)
The serializer pushes a 0x00 sentinel byte before random padding (lines 88-94), but ContactInfoPrivateData has no deserialize() method. decrypt_private_data() exists (line 281) and is currently #[allow(dead_code)]. The sentinel design is correct for the current format (0x00 after the accepted_accounts section unambiguously marks padding), but without a deserializer, the correctness of the sentinel protocol is unverifiable.
💬 nitpick: Devnet/regtest SPV silently proceeds with no peers when core_host is unconfigured
src/spv/manager.rs (line 1)
With NetworkConfig fields now Optional, a devnet config without core_host parses successfully but primary_peer_socket() returns None (line 1398 does config.core_host.as_deref()?). The devnet branch at line 1346-1349 silently skips peer addition, leaving SPV with no peers and no DNS seeds — it will never sync. Previously, missing core_host would fail config parsing entirely. This is a design trade-off (app starts vs config error), not a strict regression.
🤖 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/model/dpns.rs`:
- [BLOCKING] line 1: UTF-8 panic in strip_dash_suffix and has_dash_suffix (CONVERGENT — NOT FIXED)
Both functions slice strings at byte offsets (input.len() - DASH_SUFFIX.len()) without checking is_char_boundary(). For inputs like "tëstab" (7 bytes), the split point (2) falls inside the 2-byte ë character → panic. Fix commit cb4652e8 claims to address this but only extracted a constant; the byte-offset slicing is identical. DPNS labels are ASCII-only on-chain, but these functions run on raw user input BEFORE platform validation, so any multi-byte character in search/contact fields triggers an unrecoverable crash.
In `src/backend_task/dashpay/contact_info.rs`:
- [SUGGESTION] line 1: rootEncryptionKeyIndex written from authentication signing key, not encryption root key
Line 435 writes `signing_key.id()` into `rootEncryptionKeyIndex`. The signing_key is an AUTHENTICATION-purpose key (lines 413-424), but rootEncryptionKeyIndex per DIP-15 should reference the identity key used as the root of the encryption key derivation tree. The read path at line 353 ignores this value (`_root_idx`), so self-decryption works, but other DashPay clients (e.g., mobile wallets) that rely on this field to find the correct encryption key will fail to decrypt contactInfo.
In `src/backend_task/dashpay/contact_requests.rs`:
- [SUGGESTION] line 1: Hardcoded DPNS contract ID breaks non-mainnet contact requests
resolve_username_to_identity() hardcodes the mainnet DPNS contract ID (GWRSAVFMjXx8HpQFaNJMqBV7MBgMK4br5UESsB4S31Ec) and re-fetches it from the network, while every other DPNS lookup uses app_context.dpns_contract (which is network-aware). On testnet/devnet, this hardcoded ID won't match, causing contact request username resolution to fail.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean fix commit addressing prior review findings. The DASH_SUFFIX constant eliminates the magic number and makes the byte-offset slicing self-documenting. The UTF-8 safety concern is resolved in practice: .dash is always ASCII, so input.len() - DASH_SUFFIX.len() always lands on a char boundary regardless of multi-byte characters in the label. MIN_PLAINTEXT_SIZE corrected from 17 to 16. Good additional test coverage for the .dash edge case. The remaining prior findings (rootEncryptionKeyIndex, hardcoded DPNS contract ID, sentinel padding) are not addressed in this push.
Reviewed commit: cb4652e
- Replace direct byte-index slicing with .get() + .is_some_and() to prevent panics on non-ASCII inputs - Make strip_dash_suffix() trim whitespace (matching has_dash_suffix) - Remove redundant trim in normalize_dpns_label since strip_dash_suffix now handles it - Add non-ASCII test case Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Excellent follow-up fixing all DPNS-related findings from the prior review. Byte-index slicing replaced with safe .get() + .is_some_and() pattern — eliminates the UTF-8 panic risk entirely. API asymmetry between has_dash_suffix and strip_dash_suffix resolved by adding trim to strip_dash_suffix. Non-ASCII test case added. Clean merge of v1.0-dev (includes #813 SPV test fix). The remaining prior findings (rootEncryptionKeyIndex, hardcoded DPNS contract ID, sentinel padding) are not in scope for this fix commit.
Reviewed commit: d7fea39
…etwork calls Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…me resolution resolve_username_to_identity() had two issues: - Used a hardcoded DPNS contract ID and fetched it from network on every call, instead of using the cached contract from AppContext - Used document.owner_id() instead of records.identity to extract the identity ID, which returns the wrong identity after name transfers Now matches the approach already used in load_identity_by_dpns_name(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
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)
191-216:⚠️ Potential issue | 🟡 MinorValidate the stripped label before the DPNS lookup.
After the trim, an empty string or malformed suffixed input like
alice .dashoralice.other.dashstill falls through toresolve_username_to_identity, which turns a local format error into a network lookup miss. Please usecrate::model::dpns::strip_dash_suffix()here to reject empty labels, extra dots, or embedded whitespace before the DPNS call.🤖 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 191 - 216, After trimming to_username_or_id, validate the DPNS label before any network lookup by calling crate::model::dpns::strip_dash_suffix(&to_username_or_id) when has_dash_suffix(&to_username_or_id) is true (and also when you fall back to treating the input as a username without suffix); ensure the returned stripped label is non-empty, has no extra dots or embedded whitespace (reject labels like "alice .dash" or "alice.other.dash") and return TaskError::DashPay::InvalidUsername if validation fails, otherwise pass the validated stripped label to resolve_username_to_identity(app_context, sdk, &validated_label). Use the same validation path before calling resolve_username_to_identity in the fallback branch so local format errors are rejected before any DPNS network lookup.
🧹 Nitpick comments (1)
src/backend_task/dashpay/contact_info.rs (1)
82-95: Consider keepingserialize()deterministic.This adds randomness to
ContactInfoPrivateData::serialize(), so the same logical payload no longer encodes the same way when it is shorter than 16 bytes. If the padding is only there to satisfy the encrypted-size contract, moving it intoencrypt_private_data()or a dedicatedserialize_for_encryption()helper would keep the model serialization stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/dashpay/contact_info.rs` around lines 82 - 95, The current ContactInfoPrivateData::serialize() injects randomness by adding random padding when bytes.len() < MIN_PLAINTEXT_SIZE; remove that padding from ContactInfoPrivateData::serialize() so serialization is deterministic and instead perform padding at encryption time (e.g., in encrypt_private_data() or a new serialize_for_encryption() helper): ensure the helper/encrypt_private_data() adds the 0x00 sentinel byte then fills remaining bytes with cryptographic randomness to reach MIN_PLAINTEXT_SIZE before encrypting, leaving ContactInfoPrivateData::serialize() to produce a stable byte representation.
🤖 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_info.rs`:
- Around line 411-421: The current validation in
validate_contact_info_field_sizes only checks encrypted blob sizes and misses
one-byte length/count limits used by serialize(); add checks to validate the raw
nickname length, raw note length, and account count are <= 255 (fit in u8)
before serializing so they cannot wrap and later misparse in the deserializer
(see serialize and the parser used in contacts.rs); if any exceed 255, return
the existing TaskError::DashPay::ContactInfoValidationFailed with appropriate
errors in the same format as other validation failures.
In `@src/backend_task/dashpay/contact_requests.rs`:
- Around line 587-594: The current ok_or_else creates
TaskError::DashPay(DashPayError::InvalidDocument { reason: format!(...) }) which
embeds the internal `records.identity` field name and a user-facing string in a
String payload; change this to use a typed DashPayError variant that does not
hold a user-visible String (e.g., add or use a variant like
InvalidDocumentMissingRecordsIdentity or InvalidDocument{ details:
DocumentDetails } without a String message) and remove the formatted sentence
from the enum. In the call site (the closure producing the error), construct the
new DashPayError variant with machine-readable details (attach the offending
DPNS document or minimal structured info) and surface the user-facing message
via the enum's #[error("...")] attribute; additionally, when reporting to the
banner/UX, attach the low-level document details via
BannerHandle::with_details(...) rather than embedding them into the user
message. Ensure you update any match arms that create or pattern-match
TaskError::DashPay/DashPayError::InvalidDocument accordingly.
In `@src/backend_task/dashpay/errors.rs`:
- Around line 174-176: The ContactInfoValidationFailed error variant is not
wired into the legacy helpers so user_message() falls back to a generic text and
requires_user_action() doesn't mark it actionable; update the legacy error
mapping to return the specific guidance "Contact info is too large to save. Try
shortening your nickname or note." (or the existing variant message) for
ContactInfoValidationFailed and ensure requires_user_action() treats
ContactInfoValidationFailed as true so callers get the user-actionable
classification; search for ContactInfoValidationFailed, user_message(), and
requires_user_action() to locate the mapping code and add the new branch.
---
Outside diff comments:
In `@src/backend_task/dashpay/contact_requests.rs`:
- Around line 191-216: After trimming to_username_or_id, validate the DPNS label
before any network lookup by calling
crate::model::dpns::strip_dash_suffix(&to_username_or_id) when
has_dash_suffix(&to_username_or_id) is true (and also when you fall back to
treating the input as a username without suffix); ensure the returned stripped
label is non-empty, has no extra dots or embedded whitespace (reject labels like
"alice .dash" or "alice.other.dash") and return
TaskError::DashPay::InvalidUsername if validation fails, otherwise pass the
validated stripped label to resolve_username_to_identity(app_context, sdk,
&validated_label). Use the same validation path before calling
resolve_username_to_identity in the fallback branch so local format errors are
rejected before any DPNS network lookup.
---
Nitpick comments:
In `@src/backend_task/dashpay/contact_info.rs`:
- Around line 82-95: The current ContactInfoPrivateData::serialize() injects
randomness by adding random padding when bytes.len() < MIN_PLAINTEXT_SIZE;
remove that padding from ContactInfoPrivateData::serialize() so serialization is
deterministic and instead perform padding at encryption time (e.g., in
encrypt_private_data() or a new serialize_for_encryption() helper): ensure the
helper/encrypt_private_data() adds the 0x00 sentinel byte then fills remaining
bytes with cryptographic randomness to reach MIN_PLAINTEXT_SIZE before
encrypting, leaving ContactInfoPrivateData::serialize() to produce a stable byte
representation.
🪄 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: b9e0e156-6a97-4f65-bddd-f03703274442
📒 Files selected for processing (4)
src/backend_task/dashpay/contact_info.rssrc/backend_task/dashpay/contact_requests.rssrc/backend_task/dashpay/errors.rssrc/model/dpns.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I found one new blocking issue in the current SHA: profile search still resolves DPNS hits through owner_id() even though this PR correctly switches direct username resolution to records.identity, so transferred names can still surface the wrong profile in search results. I also validated the live CodeRabbit thread: the raw u8 length/count overflow in contact-info serialization and the missing legacy error-helper wiring are both real, while the InvalidDocument { reason } complaint is not — the formatted reason is internal detail on a variant whose user-facing text already comes from the enum-level display string.
I validated existing CodeRabbit comments separately in-thread instead of restating them here.
Reviewed commit: 142a042
🔴 1 blocking
1 additional finding
🔴 blocking: Profile search still resolves transferred names through `owner_id()`
src/backend_task/dashpay/profile.rs (lines 486-490)
search_profiles() now normalizes the input correctly, but it still derives the profile identity from document.owner_id() when turning a DPNS domain result into a user profile. This PR explicitly fixes the same ownership mismatch elsewhere by switching direct username resolution to records.identity, because transferred DPNS names can diverge from the original document owner. Leaving search_profiles() on owner_id() means a transferred username can still return the previous owner's profile from search even after add-contact/name lookup was fixed.
💡 Suggested change
Use the DPNS document's `records.identity` field here too (ideally through a shared helper), and only fall back to `owner_id()` if the field is absent.
🤖 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/profile.rs`:
- [BLOCKING] lines 486-490: Profile search still resolves transferred names through `owner_id()`
`search_profiles()` now normalizes the input correctly, but it still derives the profile identity from `document.owner_id()` when turning a DPNS domain result into a user profile. This PR explicitly fixes the same ownership mismatch elsewhere by switching direct username resolution to `records.identity`, because transferred DPNS names can diverge from the original document owner. Leaving `search_profiles()` on `owner_id()` means a transferred username can still return the previous owner's profile from search even after add-contact/name lookup was fixed.
…dash check Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codifies the pattern: model owns format validation, backend enforces it plus stateful checks, UI borrows model functions for early feedback. Prevents UI layers from reimplementing validation logic (as happened with the case-sensitive .dash suffix check). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Platform rejects self-contact requests with code 40500 ("Identity must
not be equal to owner id"). Catch this early with a clear message:
"You cannot send a contact request to yourself."
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I validated the current SHA and the main correctness issue from the prior review is still present: search_profiles() still resolves DPNS hits through owner_id() and normalizedLabel, so transferred names can still return and display the wrong profile. The contact-info serialization overflow and the missing ContactInfoValidationFailed helper wiring also remain real in the current code, and both match live CodeRabbit comments. I did not keep the multi-label .dash validation concern because it is lower-confidence and not needed for the review outcome.
I validated existing CodeRabbit comments separately in-thread instead of restating them here.
Reviewed commit: 5cc5046
🔴 1 blocking
1 additional finding
🔴 blocking: Profile search still resolves transferred DPNS names through `owner_id()`
src/backend_task/dashpay/profile.rs (lines 486-493)
search_profiles() still derives its result identity from document.owner_id() and its display name from normalizedLabel. For transferred DPNS names, the authoritative current identity lives in records.identity, not the document owner, and the human-facing label should come from the original label field rather than the normalized homograph-safe one. The rest of this PR already switched direct username/contact resolution to records.identity, so profile search still disagrees with the corrected behavior and can show the wrong person's profile for a transferred name.
💡 Suggested change
Use the DPNS document's `records.identity` field here too (ideally through a shared helper), and render the user-facing name from `label` rather than `normalizedLabel`.
🤖 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/profile.rs`:
- [BLOCKING] lines 486-493: Profile search still resolves transferred DPNS names through `owner_id()`
`search_profiles()` still derives its result identity from `document.owner_id()` and its display name from `normalizedLabel`. For transferred DPNS names, the authoritative current identity lives in `records.identity`, not the document owner, and the human-facing label should come from the original `label` field rather than the normalized homograph-safe one. The rest of this PR already switched direct username/contact resolution to `records.identity`, so profile search still disagrees with the corrected behavior and can show the wrong person's profile for a transferred name.
Adds bounds checks before casting field lengths to u8 in ContactInfoPrivateData::serialize(). Wires ContactInfoValidationFailed and CannotContactSelf into user_message() and requires_user_action() legacy helpers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The follow-up fixes from the prior review are present, but one DPNS search path still uses the wrong fields when turning domain documents into user-facing search results. search_profiles() remains inconsistent with the transfer-aware resolution logic added elsewhere in this SHA, so the PR still has one correctness issue that should block approval.
Reviewed commit: cd90c21
🔴 1 blocking
1 additional finding
🔴 blocking: Profile search still resolves DPNS results through `owner_id()` and `normalizedLabel`
src/backend_task/dashpay/profile.rs (lines 486-493)
search_profiles() still converts each DPNS domain document into a result by reading document.owner_id() and normalizedLabel. In this same SHA, the other DPNS resolution paths explicitly switched to records.identity because that is the authoritative current identity after a name transfer, and they use label for display because normalizedLabel is only the homograph-safe lookup key. Leaving this path on the old fields means a transferred name can still load the previous owner's profile, and even non-transferred names can be rendered as the normalized slug instead of the user-facing username.
🤖 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/profile.rs`:
- [BLOCKING] lines 486-493: Profile search still resolves DPNS results through `owner_id()` and `normalizedLabel`
`search_profiles()` still converts each DPNS domain document into a result by reading `document.owner_id()` and `normalizedLabel`. In this same SHA, the other DPNS resolution paths explicitly switched to `records.identity` because that is the authoritative current identity after a name transfer, and they use `label` for display because `normalizedLabel` is only the homograph-safe lookup key. Leaving this path on the old fields means a transferred name can still load the previous owner's profile, and even non-transferred names can be rendered as the normalized slug instead of the user-facing username.
Summary
to_lowercase()but DPNS storesnormalizedLabelusing homograph-safe conversion (o→0,i/l→1). Any name containing these characters would never match.normalizedParentDomainName == "dash"compound filter in DPNS query, plus wrong normalization. Also usedowner_id()instead ofrecords.identity(wrong after name transfers).create_or_update_contact_info()only acceptedECDSA_SECP256K1signing keys. Many Platform identities use BLS keys. Verified against DIP-0015: no key type restriction for contactInfo documents.privateData48-2048 bytes. Reject with empty nickname/note produced only 32 bytes. Padded with 0x00 sentinel + random bytes..dashsuffix handling —strip_suffix(".dash")andends_with(".dash")were case-sensitive."Alice.DASH"would fail.model::dpnshelper withnormalize_dpns_label(),strip_dash_suffix(),has_dash_suffix(),validate_dpns_input(). All DPNS-by-name lookup sites use the shared helpers. 9 unit tests.model::dpns::validate_dpns_input()instead of implementing their own.dashsuffix checks. Fixes case-sensitive check in Add Contact screen.resolve_username_to_identity()had a hardcoded Base58 contract ID and fetched the contract from network on every call. Now uses the cachedapp_context.dpns_contract."alice.foo"(non-.dashdomain) now returnInvalidUsernameimmediately instead of making a wasted DPNS query.validate_contact_info_field_sizes()after encryption to catch oversized data before broadcasting.Fixes
Fixes #687
Supersedes #723
Changes by File
src/model/dpns.rssrc/model/mod.rsdpnsmodulesrc/backend_task/dashpay/profile.rsnormalize_dpns_label()for searchsrc/backend_task/dashpay/contact_requests.rsnormalize_dpns_label()+has_dash_suffix()+validate_dpns_input(); addnormalizedParentDomainNamefilter; userecords.identityinstead ofowner_id(); use cached DPNS contract; reject self-contactsrc/backend_task/identity/load_identity_by_dpns_name.rsnormalize_dpns_label()src/backend_task/dashpay/contact_info.rssrc/backend_task/dashpay/errors.rsCannotContactSelfandContactInfoValidationFailedvariantssrc/ui/dashpay/add_contact_screen.rsvalidate_dpns_input()instead of case-sensitiveends_with(".dash")CLAUDE.mdRoot Causes
DPNS Normalization
DPNS normalizes labels with
convert_to_homograph_safe_chars()(o→0, i/l→1 + lowercase). All lookup sites were using plainto_lowercase().Missing DPNS Index Filter
resolve_username_to_identity()queriednormalizedLabelwithout the requirednormalizedParentDomainName == "dash"compound filter — the DPNS index needs both fields.Wrong Identity Reference
resolve_username_to_identity()useddocument.owner_id()to get the identity from a DPNS document. After a name transfer,owner_id()still points to the original registrant.records.identityis the authoritative reference.Hardcoded DPNS Contract
resolve_username_to_identity()hardcoded the DPNS contract ID and fetched it from network on every call, whileAppContextalready caches it at startup.Key Type Restriction
create_or_update_contact_info()restricted signing keys to ECDSA_SECP256K1. The DashPay contract (DIP-0015) has no key type constraint for contactInfo. Platform accepts all key types.privateData Minimum Size
When rejecting a contact (no nickname, no note), serialized privateData was 8 bytes → 32 bytes after AES-CBC. Contract requires ≥48 bytes.
Case-Sensitive UI Validation
add_contact_screen.rsusedends_with(".dash")(case-sensitive) to validate usernames. Inputs likeolivia22.DASHwere rejected before reaching the backend's correct case-insensitive check.Test plan
cargo clippy --all-features --all-targets -- -D warningsclean🤖 Co-authored by Claudius the Magnificent AI Agent