Repository navigation
fix(platform-wallet): select available signing keys across identity operations - #4764
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (26)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe wallet adds shared signer-aware key selectors. Identity, document, DPNS, token, transfer, and withdrawal flows now verify that selected keys are available to the supplied signer and release wallet locks before signer callbacks. ChangesSigner-aware identity key selection
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant WalletFlow
participant IdentityWallet
participant available_signing_key
participant Identity
participant Signer
WalletFlow->>IdentityWallet: start external-signer operation
IdentityWallet->>Identity: clone identity and release wallet lock
IdentityWallet->>available_signing_key: provide identity, signer, and key policy
available_signing_key->>Identity: inspect eligible public keys
available_signing_key->>Signer: call can_sign_with
Signer-->>available_signing_key: report availability
available_signing_key-->>IdentityWallet: return selected key
IdentityWallet-->>WalletFlow: continue operation with selected key
Merge Risk: ⚪ Minimal · up to The signer-aware key-selection changes have no identified merge-blocking issue. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4764 +/- ##
============================================
- Coverage 79.28% 79.01% -0.28%
============================================
Files 2846 2849 +3
Lines 405218 408754 +3536
============================================
+ Hits 321268 322961 +1693
- Misses 83950 85793 +1843
🚀 New features to boost your workflow:
|
lklimek
added
the
ready for final review
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The PR correctly filters DashPay profile signing keys through the supplied external signer's availability capability while preserving the existing eligibility rules and deterministic key ordering. The focused tests cover the intended mixed-key and unavailable-key scenarios. Three non-blocking issues remain: the manually added changelog section conflicts with the repository's generated changelog flow, edited rustdoc retains stale links, and signer callbacks now execute while the wallet manager read guard is held.
🟡 1 suggestion(s) | 💬 2 nitpick(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
💬 Nitpick: Updated documentation retains links to deleted profile methods
packages/rs-platform-wallet/src/wallet/identity/network/profile.rs:132-141
The edited documentation still references Self::create_profile, and the update method's documentation previously referenced Self::update_profile, but those methods do not exist in this implementation. These links produce unresolved intra-doc-link warnings and make the documentation misleading. Link to the corresponding external-signer method or describe the shared behavior without referencing deleted methods.
source: glm-5.3-flash (phase1-reviewer: general, architecture-layering, rust-quality, security-auditor)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: architecture-layering); reviewer 3: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 4: glm-5.3-flash (agent: phase1-reviewer, role: security-auditor); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The contained change modifies DashPay profile signing-key selection and availability handling in profile signing paths, directly affecting cryptographic key handling and signature authorization behavior. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort max); agentphase1-reviewer,glm-5.3-flash— architecture-layering (completed, effort max); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed, effort max); agentphase1-reviewer,glm-5.3-flash— security-auditor (completed, effort max); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 100% left, weekly 53% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 11% left, 5h 100% left) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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 `CHANGELOG.md`:
- [NITPICK] CHANGELOG.md:1-6: Manually added Unreleased section will remain stale after changelog generation
The repository's `scripts/release/generate_changelog.js` generates a new version section and prepends it to the existing `CHANGELOG.md`; it does not consume or remove an `## [Unreleased]` section. This PR adds the first such section manually, so the next release will leave this block below the generated release section while the conventional commit also generates its own entry, producing a stale and potentially duplicated changelog entry. Remove the manual section and rely on the established release generator.
In `packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:
- [NITPICK] packages/rs-platform-wallet/src/wallet/identity/network/profile.rs:132-141: Updated documentation retains links to deleted profile methods
The edited documentation still references `Self::create_profile`, and the update method's documentation previously referenced `Self::update_profile`, but those methods do not exist in this implementation. These links produce unresolved intra-doc-link warnings and make the documentation misleading. Link to the corresponding external-signer method or describe the shared behavior without referencing deleted methods.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/profile.rs:194-201: Signer availability callback runs while holding the wallet manager read guard
Both external-signer profile methods call `profile_signing_key(&managed.identity, signer)` inside the `wallet_manager.read().await` guard. The new selector synchronously invokes `signer.can_sign_with`, and FFI implementations may call host keystore code. This creates a re-entrancy and lock-order hazard: a callback that blocks on work requiring the wallet manager write lock can deadlock or unnecessarily hold the wallet read lock during host code. Collect or clone the eligible profile-key candidates while holding the guard, release the guard, and then evaluate signer availability over those candidates.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Sibling external-signer write paths still select keys the signer cannot use — NOT_ACTIONABLE: The cited contact-request, contact-info, document, DPNS, contract, token, and identity-update paths are pre-existing behavior outside the PR's stated profile-only scope; this PR neither introduces nor worsens those paths.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
llbartekll
left a comment
llbartekll
left a comment
There was a problem hiding this comment.
LGTM. The selector is a faithful copy of get_first_public_key_matching (BTreeMap values() keeps key-ID order, !is_disabled() matches allow_disabled = false) plus the can_sign_with filter, and the two new tests pin both orderings and the empty case. CI green, patch fully covered.
One non-blocking nit: the repo's changelog is generated at release time by scripts/release/generate_changelog.js (conventional-changelog) from commit titles, and CHANGELOG.md has never carried a manual ## [Unreleased] block. Suggest dropping that hunk so the next release doesn't end up with a duplicated/stale entry sitting under the generated section.
On the lock-hazard comment from the bot: I don't think it blocks. The FFI contract documents can_sign_with as synchronous and cheap, and the Kotlin implementation only touches the keystore/DAO, never the wallet manager, so there's no re-entrancy into the guard. Worth keeping in mind if a future host signer ever needs wallet state though.
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Profile signer key should not be a special case. In all cases we should use a key that we can sign with.
lklimek
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
The PR’s wallet signer-selection changes are correctly scoped and preserve explicit-key behavior, but the accompanying platform-state persistence changes introduce several in-scope defects. Existing version-0 Drive-ABCI implementations were modified while historical platform versions still select them, and the new auxiliary-state storage path can panic on GroveDB errors. Additional non-blocking issues affect validator-set integrity checks, cache mutex handling, and reporting success after local persistence fails.
🔴 4 blocking | 🟡 3 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Known-contract cache APIs panic when the mutex is poisoned
packages/rs-sdk-trusted-context-provider/src/provider.rs:248-274
The new known-contract cache methods use Mutex::lock().unwrap(). If a panic occurs while a cache guard is held, later proof-verification calls panic again instead of recovering the cache or returning a typed provider error. The surrounding quorum-cache code already defines a recoverable poisoning policy; apply the same policy to these new cache accesses or convert lock failure into an appropriate provider error.
source: gpt-6-astra (phase2-reviewer: rust-quality)
🟡 Suggestion: Successful credit operations swallow persistence failures
packages/rs-platform-wallet/src/wallet/identity/network/transfer.rs:124-145
After the on-chain transfer succeeds, a failure from persister.store(...) is logged and the method still returns Ok(()). The in-memory balance has changed, but a relaunch can restore the old persisted balance, leaving the wallet with stale local state and potentially causing subsequent transitions to be built from an inconsistent snapshot. Return a typed persistence error or provide an explicit reconciliation outcome instead of reporting complete success after durable-state persistence fails. The same behavior is present in the newly added withdrawal and address-transfer paths.
source: gpt-6-astra (phase2-reviewer: rust-quality)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The large cross-cutting diff changes platform-wallet signing-key selection and availability checks across identity, document, token, credit-transfer, and withdrawal operations, directly affecting signatures and funds movement. - Phase 1 reviewers: not run (skipped for throughput: 18 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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 `packages/rs-drive-abci/src/execution/storage/store_platform_state/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/storage/store_platform_state/v0/mod.rs:15-54: Protocol-v0 platform-state storage generation was edited in place
The shipped `store_platform_state_v0` implementation now conditionally skips the full platform-state record and writes a recent-state auxiliary record, while the corresponding v0 fetch implementation consumes that record. Historical `PlatformVersion` snapshots still select version 0 for this method, so replaying or operating those protocol versions now uses behavior different from the originally shipped generation. Preserve the old v0 implementation and add a new versioned generation, dispatcher arm, and platform-version selection for the new behavior; retain compatibility coverage for historical versions.
In `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs:135-151: Masternode-list v0 behavior was changed without a versioned generation
`update_state_masternode_list_v0` now returns early for payment-only Core diffs and uses tracked mutation helpers instead of the original full update path. The dispatcher and method tables still expose this as version 0, including historical platform snapshots. Because this method updates persisted masternode and validator state, editing v0 changes historical replay behavior. Move the new behavior into a v1 implementation, add the dispatcher arm, select it only in the unreleased platform snapshot, and leave v0 unchanged.
In `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_quorum_info/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_quorum_info/v0/mod.rs:120-141: Quorum-update v0 behavior was changed without a versioned generation
The existing quorum-update v0 implementation now computes removals through a new path, skips sorting when no validator set changed, and returns early when quorum indexes appear unchanged. The dispatcher still selects version 0 for all historical platform versions. Validator sets and validating quorums are persisted platform state, so this in-place change violates the frozen-generation rule and can alter historical replay. Preserve v0 and put the optimized behavior in a new version selected only by the unreleased snapshot.
In `packages/rs-drive/src/drive/platform_state/store_platform_state_entry_bytes/v0/mod.rs`:
- [BLOCKING] packages/rs-drive/src/drive/platform_state/store_platform_state_entry_bytes/v0/mod.rs:14-17: Platform-state auxiliary storage unwraps GroveDB errors
The new writer calls `put_aux(...).unwrap()` before mapping the inner result. If the outer GroveDB operation returns an error, block execution panics instead of propagating the error through the method’s `Result<(), Error>` API. The same unwrap pattern is used by the recent-state writer and the corresponding fetch/delete helpers. Propagate both result layers explicitly so storage failures remain typed errors rather than chain-halting panics.
In `packages/rs-drive-abci/src/platform_types/platform_state/platform_state_for_saving/v2/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/platform_types/platform_state/platform_state_for_saving/v2/mod.rs:190-207: Validate that a validator-set entry matches its aux key
Masternode entries are checked against the hash encoded in their auxiliary-storage key, but validator-set entries are deserialized and inserted without checking that `ValidatorSet::quorum_hash()` equals the key-derived quorum hash. A corrupted or mismatched entry can therefore reconstruct a map whose key identifies one quorum while its value identifies another. Reject mismatched validator-set entries as corrupted before insertion, mirroring the masternode integrity check.
In `packages/rs-sdk-trusted-context-provider/src/provider.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/src/provider.rs:248-274: Known-contract cache APIs panic when the mutex is poisoned
The new known-contract cache methods use `Mutex::lock().unwrap()`. If a panic occurs while a cache guard is held, later proof-verification calls panic again instead of recovering the cache or returning a typed provider error. The surrounding quorum-cache code already defines a recoverable poisoning policy; apply the same policy to these new cache accesses or convert lock failure into an appropriate provider error.
In `packages/rs-platform-wallet/src/wallet/identity/network/transfer.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/transfer.rs:124-145: Successful credit operations swallow persistence failures
After the on-chain transfer succeeds, a failure from `persister.store(...)` is logged and the method still returns `Ok(())`. The in-memory balance has changed, but a relaunch can restore the old persisted balance, leaving the wallet with stale local state and potentially causing subsequent transitions to be built from an inconsistent snapshot. Return a typed persistence error or provide an explicit reconciliation outcome instead of reporting complete success after durable-state persistence fails. The same behavior is present in the newly added withdrawal and address-transfer paths.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Credit-key availability workaround leaves the DPP default selector incorrect for other callers — NOT_ACTIONABLE: The PR is explicitly scoped to platform-wallet operations and deliberately selects an available credit key in the wallet before invoking the SDK; its description states that public APIs and versioned DPP code remain unchanged. Direct DPP or unrelated SDK callers retaining their existing first-match behavior is outside this PR’s stated wallet-only change and is not a regression introduced by the patch.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-09-17T11:52:50.006Z |
Check external signer capability while selecting an eligible profile key, so an unavailable earlier key cannot shadow an available ECDSA key. Keep signing errors terminal and retain existing eligibility restrictions. Co-Authored-By: Codex GPT-6 <noreply@openai.com>
…perations Share signer-aware selection across wallet write paths and select credit keys before invoking the SDK. Preserve operation policies, explicit keys and withdrawal purpose priority, and release wallet manager guards before host availability callbacks. Remove the manual changelog block and stale rustdoc. Co-Authored-By: Codex GPT-6 <noreply@openai.com>
ce285c4 to
027f82d
Compare
…election Distinguish missing eligible identity keys from eligible keys the signer cannot use, preserving the existing FFI code 31 recovery guidance. Retain withdrawal purpose fallback and explicit key selections. Cover DPNS set-price preflight and real wallet-to-FFI error conversion, including missing-key controls and selection policy regressions. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Codex <noreply@openai.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete base-to-head diff at 2d2c5cb and found no remaining in-scope defects. The SDK centralization suggestion would expand the explicitly wallet-scoped change, while the persistence and disabled-key observations concern unchanged behavior. Independent validation passed all 253 wallet identity-network tests, both FFI signing-preflight tests, formatting checks, and git diff --check; the worktree remains clean.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: architecture-layering); reviewer 3: gemini-3.8-flash-high (agent: phase1-reviewer, role: ffi-engineer); reviewer 4: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 5: gemini-3.8-flash-high (agent: phase1-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 13: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 14: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 15: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The diff introduces intricate, cross-operation signing-key selection in packages/rs-platform-wallet/src/wallet/identity/network/signing_key.rs and changes credit transfer and withdrawal key selection, directly affecting signature/key handling and funds movement with eligibility, signer availability, explicit-key preservation, and TRANSFER-before-OWNER fallback rules. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— architecture-layering (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— ffi-engineer (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— security-auditor (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 100% left, 5h 100% left - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Out-of-scope follow-up suggestions (3)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Share credit auto-selection with the SDK's optional-key path — Out of scope. The SDK methods still delegate omitted keys to DPP's first-key selectors, but those SDK implementations are unchanged from the base. The PR explicitly implements selection in platform-wallet before calling the SDK, and the wallet now supplies Some(selected_key). Moving the policy into the SDK would improve sibling clients but is not required for this PR's stated wallet behavior to work.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
- Share automatic credit-key selection at the SDK boundary — Duplicate of the SDK centralization observation and outside this PR's stated scope. Source inspection confirms that direct SDK, WASM, and SDK-FFI optional-key paths retain their pre-existing behavior; the current change neither introduces nor worsens that limitation.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
- Centralize signer-aware key selection in lower-layer SDK or DPP transition builder helpers — Out of scope. The optional-key limitation is real but pre-existing in unchanged SDK/DPP code. Broadening the implementation to all SDK consumers is adjacent work, not a prerequisite for the wallet-specific fix.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
* refactor(dashpay): publish profiles through platform-wallet Delegate profile create and update to the managing wallet with the existing identity signer. Validate inputs and report unsupported field clearing and wallet-less writes explicitly. Preserve profile metadata during refresh. Document upstream HASH160 limitations and add focused unit coverage plus an opt-in testnet create/replace test using a wallet-derived HIGH key. Co-Authored-By: Codex <noreply@openai.com> * fix(dashpay): address profile publication review feedback Reject avatar failures before profile submission, preflight the managing identity's supported signing keys, and retry failed timestamp writes on profile reads without resubmitting paid transitions. Co-Authored-By: Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> * fix(network): report cancelled startup as stopped Prioritize cancellation when startup is also ready, and clear the startup result after cancellation tears down the registered backend. Co-Authored-By: OpenAI Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> * fix(dashpay): preserve profile timestamps on refresh Initialize missing timestamps under the save lock, preserving persisted dates and pending repairs even when storage fails. Cache avatars downloaded for profile updates so subsequent views reuse the validated image. Validation: 43 profile tests and 3 network-switch tests pass; cargo fmt --all and cargo clippy --all-features --lib -- -D warnings pass. Co-Authored-By: GPT-6 <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> * fix(dashpay): accept HASH160 profile authentication keys Prepare DET for platform-wallet HASH160 profile signing in Platform #4653. Keep all Platform dependency revisions unchanged. Extend policy tests and add an ignored profile create/replace E2E case with derived HASH160 keys. Co-Authored-By: GPT-6 <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> * fix(dashpay): pin Platform with HASH160 profile signing support Adopt v4.2-dev at 7f809377 and adapt checked shielded balances, versioned reward calculations, and failed withdrawal status handling. Update the profile documentation to reflect available HASH160 support. Co-Authored-By: OpenAI GPT-6 <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> * fix(dashpay): use signer-aware upstream profile key selection Pin Platform to the fix in dashpay/platform#4764 so an unavailable HASH160 key cannot shadow an available ECDSA key during profile writes. Add mixed-key create/replace E2E coverage and document the dependency. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> * fix(dashpay): validate available profile keys and clarify withdrawal history Check eligible managed-identity keys against the supplied signer's placements before publishing, preserving the actionable error when none is available. Cover unavailable, ineligible, and mixed-key placements in profile tests. Use neutral withdrawal history labels and document failed terminal results. Co-Authored-By: Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> * chore(deps): bump platform pin to latest platform#4764 commit Update dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, and platform-wallet-storage git revs (plus Cargo.lock) from f73f5d6098a739d29a1cebc2f7941e062ee8a517 to dc946fa8af68584c927b4062b1e8909486413ed0, the current head of dashpay/platform#4764 (fix/dashpay-available-signer, still open). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Codex <noreply@openai.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
⚠️ DEGRADED — Re-review — Final validation — Phase 1 + Phase 2
⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models:gpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped athigheffort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.
Wallet signer-aware key selection is correctly centralized with eligibility, ordering, and guard-release discipline preserved across all touched operations. No in-scope consensus, fee, proof, or FFI defects were found in the wallet-only diff at this head.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: architecture-layering); reviewer 3: gemini-3.8-flash-high (agent: phase1-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 8: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: ffi-engineer); reviewer 9: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 10: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); reviewer 11: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 12: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 13: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: ffi-engineer); reviewer 14: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 15: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)
- Degraded mode:
gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-18T05:22:01Z); stand-insgpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor; Phase 1 effort capped athigh - Triage:
criticalbymuse-spark-1.3-contributor(standing in forgpt-6-astra) (effort low) — Large cross-cutting change adding signer-aware key selection in packages/rs-platform-wallet/src/wallet/identity/network/signing_key.rs that directly controls which identity keys sign funds-moving transfer/withdrawal/token operations. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— architecture-layering (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— ffi-engineer (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort high); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(zai below 15% reserve: 5h 99% left, weekly 14% left) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — final-verifier; agentastra-verifier - Phase 2 reviewers:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — architecture-layering (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — ffi-engineer (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — rust-quality (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — security-auditor (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — architecture-layering (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — ffi-engineer (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — rust-quality (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
Bots are done — your move: address QuantumExplorer requested changes, then post |
github-actions
Bot
added
bot-review-skipped
…able-signer # Conflicts: # packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs # packages/rs-platform-wallet/src/wallet/identity/network/profile.rs
github-actions
Bot
added
waiting-bots
…together Selection tests exercised the two filters in isolation. Combine them so an ineligible-but-available key can't mask an unavailable eligible one, and an unavailable-but-ineligible key can't turn None into a signer error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keep the selection semantics while cutting the diff roughly in half: - share `first_available` / `require_available` between the generic, credit and contract-bound selectors; return `dash_sdk::Error` so call sites drop their `.map_err(dash_sdk::Error::from)` - expose the selector as an `Identity` extension method mirroring `get_first_public_key_matching`, keeping call-site layout (and the original error messages) close to the base branch - replace per-module mock signers with one `KeyFilter` test signer; the lock check becomes a `lock_checking_signer` filter - fold overlapping selector tests; move the credit-operation signer-unavailable check from the FFI crate into platform-wallet - restore the external-signer profile rustdoc and trim history comments Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The v4.3-dev merge dropped the import, breaking set_balance calls (E0599). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hpay/platform into fix/dashpay-available-signer
lklimek
left a comment
lklimek
left a comment
There was a problem hiding this comment.
LGTM /self-reviewed
* chore(deps)!: bump dashpay/platform to v4.3-dev (4d1db8a5) Pin dash-sdk, rs-sdk-trusted-context-provider, platform-wallet and platform-wallet-storage to dashpay/platform#4764 head 4d1db8a5, which contains the full v4.3-dev branch plus the available-signer key selection fix DET relies on for HASH160 DashPay profile keys. This commit alone does not compile: protocol version 14 API changes are adapted in follow-up commits on this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: adapt to protocol version 14 upstream types Make DET compile against dashpay/platform v4.3-dev without hiding the new variants behind catch-alls: - same_key compares identity public keys of either version through one identity tuple. disabled_at, total_budget and expires_at are state that Platform moves (disable, IdentityKeyLimitsUpdate); whether a key has a budget or an expiry is fixed for its life and still identifies it. A V1 key without limits equals its V0 wire form. - Key info shows contract-group bounds. - PlatformWalletError::TokenOperationFailed is classified through the normal SDK error mapping; an otherwise unclassified failure becomes the new TaskError::TokenOperationFailed, which names the operation. - StateTransitionCreationOptions carries no action fee agreement from the signing-override helper; document transitions add their own. - Tests and fixtures cover the new enum variants. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(deps): re-pin dashpay/platform to 21d70b3c 21d70b3c restores the IdentitySettersV0 import that the v4.3-dev merge in dashpay/platform#4764 dropped; 4d1db8a5 did not compile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(deps): re-pin dashpay/platform to c5c4d316 c5c4d316 is the head of dashpay/platform#4764 (fix/dashpay-available-signer): 21d70b3c plus signer-aware identity key selection in platform-wallet (277b5356, 088499f4). The pin still sits on an unmerged PR branch; re-pin to v4.3-dev once #4764 merges. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(deps): mark the platform pin as temporary until dashpay/platform#4764 merges Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci(migration): point platform pin guard at dashpay/platform#4764 branch The platform pin sits on fix/dashpay-available-signer (dashpay/platform#4764), which carries all of v4.3-dev plus the available-signer fix DET depends on, so it is not yet an ancestor of any release line. The wallet-storage migration fingerprint is unchanged (18 files). Temporary: once #4764 merges, re-pin to a commit on v4.3-dev, set the target branch to v4.3-dev and rerun --update. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(context): pin DET's local platform version to protocol 13 The platform pin knows protocol 14, so `PlatformVersion::latest()` is now PLATFORM_V14, while mainnet and testnet run protocol 13. DET must not build or validate protocol-14 structures yet. - Add `DET_PLATFORM_VERSION` (PLATFORM_V13), the single source for `default_platform_version`: `AppContext::platform_version()` (validation, building, signing), the SDK's initial version seed and therefore system-contract loading. - Platform info loaded the Withdrawals contract and computed the daily withdrawal limit at `latest()`; both now use `DET_PLATFORM_VERSION`, and a test proves the limit is protocol 13's flat 2000 Dash, not protocol 14's relative one. - Update the platform#4231 TODO (the fix is in the pin now) and the devnet seeding TODO; rename the v12 seeding test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor: reduce protocol 14 type adaptation to the minimum DET targets protocol 13, so the new upstream variants only need the simplest correct handling to compile, not protocol-14 semantics. - `PlatformWalletError::TokenOperationFailed` is upstream's typed successor of `TokenError` with the same message; map it through the same generic `WalletBackend` arm. Drop `TokenOperationKind`, `TaskError::TokenOperationFailed` and `from_token_operation_failure`. - `same_key` keeps the original exhaustive V0 comparison. V1 keys (protocol 14 only) compare strictly apart from `disabled_at`, and a V1 key never equals a V0 one, since upstream writes a limit-less key as V0. The limit-raise semantics of `IdentityKeyLimitsUpdate` are left for protocol 14 support. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(deps): re-pin dashpay/platform to v4.3-dev (9f7ed169) dashpay/platform#4764 merged as 9f7ed169, whose tree is identical to the previous pin c5c4d316, so no code changes follow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci(migration): point platform pin guard back at v4.3-dev Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
TL;DR: Wallet operations select an eligible identity key that the supplied signer can use, so an unavailable earlier key no longer prevents signing with an available one.
User story
As a wallet user with only some of an identity's keys available, I want supported operations to use an available key without changing a key I explicitly selected.
Scenario
An identity has two eligible keys, but only the second is available to the signer. Automatic selection should use the second key. If the caller explicitly selects the unavailable key, the operation should fail without substituting another key.
Detailed discussion
Issue being fixed or feature implemented
Follow-up to #4653. Automatic identity-key selection in platform-wallet must account for signer availability across operations, including the credit operations whose default selection is delegated to DPP.
What was done?
profileandcontactInfowrites use the contract-bounds and key-limits awareusable_authentication_keyfrom feat(sdk)!: key limits on every client: wasm-dpp2, platform-wallet, FFI, Kotlin and Swift #4811, now also signer-aware: keys bound elsewhere, disabled or expired are ineligible, unlimited keys are preferred over limited ones, and a key the signer cannot use is skipped.None, existing not-found errors) from "eligible keys exist but the signer can use none" (signer-unavailable error, FFI code 31).How Has This Been Tested?
cargo nextest run -p platform-wallet -p platform-wallet-ffi— 1561 passed, 6 skipped.cargo clippy -p platform-wallet -p platform-wallet-ffi --all-targets --all-features -- -D warningsandcargo fmt --all -- --check.Breaking Changes
None.
Checklist
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes
PR Hygiene ·
c5c4d31/skip-botsproceeds without the ones not yet reported/self-reviewedonce the bots are doners-platform-wallet(packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs,packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs,packages/rs-platform-wallet/src/wallet/identity/network/contract.rsand 24 more) — HashEngineering or ZocoLini or llbartekll or romchornyiWhen every box is checked the
PR Hygienecheck passes and this can merge.