refactor(platform-wallet): extract shared contact request logic and use tracing#3206
refactor(platform-wallet): extract shared contact request logic and use tracing#3206thepastaclaw wants to merge 3 commits intodashpay:v3.1-devfrom
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
📝 Walkthrough🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-platform-wallet/src/platform_wallet_info/identity_discovery.rs`:
- Around line 82-89: The Err(e) arm currently increments consecutive_misses
(variable consecutive_misses) which treats query failures as empty slots;
instead do not count errors as misses—either propagate the error out from the
current function (replace the Err(e) branch with an early return/return Err(e)
or use the ? operator) or log and retry without modifying consecutive_misses;
keep the tracing::warn call but remove the consecutive_misses += 1 so query
failures don't stop the scan prematurely.
- Around line 68-76: The discovered list is being appended even when the
identity already exists; change the logic so you first check for existence using
identity_manager().identity(&identity_id) (to avoid cloning the entire
identities() map), and only call identity_manager_mut().add_identity(identity)?
and then discovered.push(identity_id) when the identity was actually added;
ensure discovered.push is moved inside that conditional that wraps
add_identity().
In
`@packages/rs-platform-wallet/src/platform_wallet_info/matured_transactions.rs`:
- Around line 82-83: The code in matured_transactions.rs currently hardcodes
identity_index = 0 when calling derive_identity_auth_key_hash(wallet,
self.network(), 0, 0), which prevents discovery of identities beyond the first
slot; change this to use the wallet's known identity candidate index or invoke
the gap-limit discovery flow instead of a fixed 0. Locate
derive_identity_auth_key_hash usage in matured_transactions.rs and replace the
literal 0 for identity_index with a value obtained from the wallet state (e.g.,
the candidate identity index accessor on the Wallet object) or call the new
discovery function (the gap-limit discovery entrypoint used elsewhere) to
resolve the correct identity_index before deriving the auth key hash. Ensure the
adjusted path handles errors the same way and preserves the existing network and
key_index (still 0) semantics.
In `@packages/rs-platform-wallet/src/platform_wallet_info/mod.rs`:
- Around line 202-204: Don't coerce missing timestamps to 0: when extracting
created_at_core_block_height and created_at from doc, treat missing values as an
error instead of using unwrap_or(0). Replace the unwrap_or(0) calls on
doc.created_at_core_block_height() and doc.created_at() with checks that
return/propagate an error (e.g., map to Err or use ok_or/ok_or_else) so the
caller can log and skip the malformed document; reference the symbols
created_at_core_block_height, created_at and doc in mod.rs to locate and update
the extraction logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef11c6c2-cdd5-4200-ab66-60d6f1d31c60
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
packages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/platform_wallet_info/identity_discovery.rspackages/rs-platform-wallet/src/platform_wallet_info/key_derivation.rspackages/rs-platform-wallet/src/platform_wallet_info/matured_transactions.rspackages/rs-platform-wallet/src/platform_wallet_info/mod.rspackages/rs-platform-wallet/src/platform_wallet_info/wallet_transaction_checker.rs
packages/rs-platform-wallet/src/platform_wallet_info/identity_discovery.rs
Outdated
Show resolved
Hide resolved
| Err(e) => { | ||
| tracing::warn!( | ||
| identity_index, | ||
| "Failed to query identity by public key hash: {}", | ||
| e | ||
| ); | ||
| consecutive_misses += 1; | ||
| } |
There was a problem hiding this comment.
Don't spend the gap budget on query failures.
A Platform/SDK error is not a confirmed empty slot. Incrementing consecutive_misses here can stop the scan early and skip identities later in the range. Either bubble the error up, or log/retry without counting it as a miss.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-platform-wallet/src/platform_wallet_info/identity_discovery.rs`
around lines 82 - 89, The Err(e) arm currently increments consecutive_misses
(variable consecutive_misses) which treats query failures as empty slots;
instead do not count errors as misses—either propagate the error out from the
current function (replace the Err(e) branch with an early return/return Err(e)
or use the ? operator) or log and retry without modifying consecutive_misses;
keep the tracing::warn call but remove the consecutive_misses += 1 so query
failures don't stop the scan prematurely.
There was a problem hiding this comment.
Fixed in 33b3da2 — removed the consecutive_misses += 1 from the error branch. Query failures now just log a warning without consuming gap budget.
| // Derive the first authentication key hash (identity_index 0, key_index 0) | ||
| let key_hash_array = derive_identity_auth_key_hash(wallet, self.network(), 0, 0)?; |
There was a problem hiding this comment.
This still only checks the first identity slot.
Every asset-lock follow-up derives identity_index = 0, so a wallet that registers a second identity will keep re-querying the first auth key and never discover the newer one. Since wallet_transaction_checker.rs triggers this path automatically, those identities stay invisible unless a separate discovery scan runs. Please hand this path off to the new gap-limit discovery flow, or derive the candidate index from wallet state instead of hardcoding slot 0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-platform-wallet/src/platform_wallet_info/matured_transactions.rs`
around lines 82 - 83, The code in matured_transactions.rs currently hardcodes
identity_index = 0 when calling derive_identity_auth_key_hash(wallet,
self.network(), 0, 0), which prevents discovery of identities beyond the first
slot; change this to use the wallet's known identity candidate index or invoke
the gap-limit discovery flow instead of a fixed 0. Locate
derive_identity_auth_key_hash usage in matured_transactions.rs and replace the
literal 0 for identity_index with a value obtained from the wallet state (e.g.,
the candidate identity index accessor on the Wallet object) or call the new
discovery function (the gap-limit discovery entrypoint used elsewhere) to
resolve the correct identity_index before deriving the auth key hash. Ensure the
adjusted path handles errors the same way and preserves the existing network and
key_index (still 0) semantics.
There was a problem hiding this comment.
This is intentional for now. The asset lock path (fetch_contact_requests_for_identities_after_asset_locks) is a quick check triggered when a new asset lock transaction is detected — it checks whether the primary identity (index 0) has been registered. The full gap-limit discovery scan in discover_identities() handles discovering additional identities across all indices.
Wiring the asset lock path into the gap-limit scan would add complexity that isn't needed yet (multi-identity wallets aren't common in practice). If/when that becomes a requirement, we can refactor this path to call discover_identities instead. For now, the separation keeps the two flows simple and distinct.
There was a problem hiding this comment.
@thepastaclaw, that makes sense — keeping the asset lock path as a lightweight primary-identity check and leaving full gap-limit discovery to discover_identities() is a clean separation of concerns. I'll note this design intent for future reviews.
(ﾐ ᵕ ꒳ ᵕ ﾐ)✨
✏️ Learnings added
Learnt from: thepastaclaw
Repo: dashpay/platform PR: 3206
File: packages/rs-platform-wallet/src/platform_wallet_info/matured_transactions.rs:82-83
Timestamp: 2026-03-06T13:10:32.252Z
Learning: In `packages/rs-platform-wallet/src/platform_wallet_info/matured_transactions.rs`, the hardcoded `identity_index = 0` in `fetch_contact_requests_for_identities_after_asset_locks` (via `derive_identity_auth_key_hash(wallet, self.network(), 0, 0)`) is intentional. This path is a quick check triggered when a new asset lock transaction is detected, targeting only the primary identity (index 0). Full gap-limit discovery across all identity indices is handled separately by `discover_identities()`. The separation is deliberate to keep both flows simple; multi-identity wallet support on the asset lock path can be added later by calling `discover_identities` if needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
thepastaclaw
force-pushed
the
feat/identity-discovery-scan
branch
from
33b3da2 to
cdfb886
Compare
Add DashSync-style identity discovery to PlatformWalletInfo that scans consecutive DIP-13 authentication key indices and queries Platform to find registered identities during wallet sync. New methods: - discover_identities: gap-limit scan using PublicKeyHash queries - discover_identities_with_contacts: same + fetches DashPay contacts Refactors shared key derivation and contact request parsing into reusable modules used by both discovery and asset lock processing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…se tracing - Extract duplicated contact request fetch+store loops from identity_discovery.rs and matured_transactions.rs into a shared fetch_and_store_contact_requests() helper method on PlatformWalletInfo - Replace all eprintln! calls with tracing::warn! for proper library logging (add tracing dependency) - Extract magic number 100 into DEFAULT_CONTACT_REQUEST_LIMIT constant - Log silently-swallowed contact request parse errors instead of ignoring them (aids debugging malformed documents)
- Only push to discovered list when identity is actually new (avoids reprocessing existing identities in discover_identities_with_contacts) - Use identity() instead of identities().contains_key() to avoid cloning the entire identities map - Don't increment consecutive_misses on query failures (a network/SDK error is not a confirmed empty slot and shouldn't consume gap budget) - Treat missing created_at and created_at_core_block_height as errors instead of silently coercing to 0, consistent with other required contact request fields
thepastaclaw
force-pushed
the
feat/identity-discovery-scan
branch
from
cdfb886 to
7e87f5b
Compare
2 participants
Summary
Follow-up refactor for #3188 (identity discovery scan). Addresses self-review findings:
fetch_and_store_contact_requests()helper eliminates ~60 lines of copy-paste betweenidentity_discovery.rsandmatured_transactions.rseprintln!withtracing::warn!— proper structured logging for a library crate100→DEFAULT_CONTACT_REQUEST_LIMITValidation
cargo check✅cargo clippy✅cargo fmt --check✅Summary by CodeRabbit
New Features
Improvements