Repository navigation
feat!: support Platform protocol 14 with v5.1-dev SDK - #1015
Claudius-Maginificent wants to merge 103 commits into
Conversation
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>
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>
…igning keys Protocol version 14 lets an authentication key carry a lifetime budget and an expiry (IdentityPublicKey::V1) and be bound to a contract, a document type or a contract group. Choosing a signing key by purpose, security level and key type alone could pick a key Platform refuses. - model::identity_key_usability holds the pure rules, mirroring upstream platform-wallet key selection: skip disabled and expired keys, keep a contract-bound key to its contract (never on a transition outside a batch), never auto-pick a contract-group bound key, and prefer an unlimited key over a limited one. - Every automatic signing-key pick (identity transfer and withdrawal, contract register and update, add key, DPNS, document actions, token screens, DashPay) goes through it with the right signing scope. - Manual key choosers still offer limited, expired and out-of-bounds keys, tagged in the list, with a warning under the chooser saying what the network will do. - Key info shows the spending limit with what is left of it (new IdentityTask::FetchKeyRemainingBudgets over the getIdentityKeysRemaining Budgets query) and the expiry; the key list marks expired and limited keys. - Tests pin the selection rules, the manual-selection caveats, decoding of a real v0.9.3 identity blob (V0 keys, contactRequest bounds) and the round trip of V1 keys and contract-group bounds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Protocol version 14 adds a token distribution that pays every identity a fixed amount exactly once (TokenDistributionRules::V1). - View: token details state whether and how much each identity can claim. - Create: the token creator offers the distribution with a validated amount (1..=i64::MAX, model::token). Without it the rules stay version 0, the only format older networks accept; with it they become version 1. The option is gated on the connected protocol version (new FeatureGate::TokenOncePerIdentityDistribution) and the registration fee estimate includes its surcharge, priced from the connected network's fee table (AppContext::connected_platform_version). - Claim: the claim screen offers the distribution, pre-selects the only distribution a token has, and explains what the claim pays. - Platform has no query for "has this identity claimed", so DET keeps a per-identity hint in the app k/v store, written when a claim succeeds or is refused as already taken (new TaskError::TokenOncePerIdentityAlready Claimed). The claim screen shows when the claim was taken and disables claiming. - FeatureGate::IdentityKeyLimits is added alongside for the key limits UI. - Tests cover amount validation, V0/V1 rule building, stored token config round trips for both rule versions, the claim hint, the error mapping, the protocol gates and the creator refusing the option below protocol 14. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Protocol version 14 lets a document type charge a fee per action, split
between the contract owner and its moderators, and every transition paying
one must carry the fee agreement the user signed.
- model: `document_action_fee_quote` prices a declared fee at the current
fee multiplier and builds the agreement; the multiplier increase the user
accepts is the named constant
`ACTION_FEE_MULTIPLIER_INCREASE_TOLERANCE_PERCENT` (20%).
- documents screen: shows the contract fee next to the estimated fee and
asks for confirmation naming the total, the owner/moderator split and the
most that can be charged ("up to N DASH"); the confirmed agreement is the
one broadcast.
- documents screen: an optional token cost can be left out; the action then
pays its gas in credits.
- backend: the six mutating `DocumentTask` variants carry the agreement and
fall back to one priced at the cached multiplier for non-UI callers.
- platform info: restore the live `ExtendedEpochInfo::fetch_current` fetch
(dashpay/platform#4231 is in the pin), so fee agreements are signed
against the network's multiplier; a failed fetch keeps the last known one
and says so.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rrors Each new rejection DET flows can hit gets a dedicated `TaskError` variant with the SDK error as its source and a message saying what happened and what to do: - signing keys: expired, budget exhausted, budget exceeded, used outside its contract bounds, contract-bound key on a non-batch action; - document action fees: agreement missing, agreement mismatch, fee multiplier rose past the agreed tolerance; - contract moderation: identity banned, identity suspended (until when); - gas sponsorship: sponsor balance too low, payer not offered, mixed payers in one batch; - contract authoring: immutable property changed, deletable reference to a non-deletable type, moderator fees without moderation, invalid once-per-identity amount, pre-programmed distribution over the limit. Also note in the withdrawal history that a failed withdrawal's credits are not returned, and log an overflowing shielded balance total at warn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Protocol version 14 lets an authentication key below MASTER carry a total spending budget and an expiry, raised later by a key limits update. - model: `identity_key_limits` holds the pure rules Platform enforces (who may carry limits, non-zero budget, future expiry, raise-only updates, an expired key must be extended before it is only topped up) plus the choice of signer (MASTER, else an unlimited unbound CRITICAL key), so a refused - and still charged - request never leaves DET. - Add Key screen: optional spending limit and "expires after N days" for keys that may carry them; disabled with an explanation on networks without key limits. - Key Info screen: "Raise Limits" adds to the spending limit and/or extends the expiry, then shows the stored key and its new remaining budget. - backend: `IdentityTask::RaiseKeyLimits` computes absolute limits from the key as Platform holds it and calls `update_key_limits`; adding a key with limits is gated on protocol version 14 and validated first. - errors: typed variants for the key limits consensus rejections. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Document action fees collect in a contract's owner pot and moderators pot (protocol version 14); a recipient pays a pot out once per epoch. - Contracts screen: "Fee Pots" (shown where the network keeps them) lists both pots of a loaded contract with their last payout, and lets an identity that receives a pot claim it. - backend: `ContractTask::FetchContractFeePots` and `ClaimContractFees` (signed by a CRITICAL key not bound to a contract, chosen up front); the claimant's new balance is stored. - gate: `FeatureGate::ContractFeePots` on upstream's `CONTRACT_FEE_CLAIM_INITIAL_PROTOCOL_VERSION`. - errors: typed variants for claims that are not allowed, already made this epoch, or have nothing to pay. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n stories IDN-022..024 (key spending limits and expiry, raising them, signing-key choice), TOK-019 (once-per-identity distribution), DOC-010..012 (document action fee agreement, optional token cost, contract fee pots). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
sdk_error_user_message() does not exist; the SDK error mapping lives in the From<SdkError> impl. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
Automatic key selection preferred an unlimited key without asking whether this device holds its private half, so an identity imported with only a delegated limited key (the protocol version 14 application-key case) failed to sign although a usable key was there. Selection now mirrors upstream platform-wallet's signer-aware `usable_authentication_key`: qualifying unlimited keys first, then limited ones, and the first this device can sign with wins. Callers go through `QualifiedIdentity::signing_key_now`, which checks `can_sign_with`; availability never widens eligibility. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signing-key selection now requires the private half on this device; the test identity holds its keys, and a new case pins that a held limited key beats an unlimited key held elsewhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The chooser listed the copy of each key stored next to its private half. After a limits raise that copy is stale, so an extended key still showed as expired and a raised budget kept its old amount, and the stale copy was handed to the backend. `QualifiedIdentity::live_public_key` resolves a stored copy to the live key (same key, limits may differ); the chooser labels, warns about and selects the live key, and refreshes a stale selection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Protocol version 14 (contract bounds validation v2) admits AUTHENTICATION keys below MASTER bound to a contract or document type, with or without limits - the typical delegated application key. The Add Key screen only offered bound ENCRYPTION/DECRYPTION keys and hid the limits when bounds were on. - gate `ContractBoundAuthenticationKeys`, keyed on upstream's `validate_identity_public_key_contract_bounds` version (>= 2) of the connected platform version; - model `contract_bounds_allowed`; the backend refuses a bound authentication key where the network does not admit one; - Add Key offers AUTHENTICATION (CRITICAL/HIGH/MEDIUM) with bounds where admitted, and limits whenever the purpose and level allow them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With no agreement passed, the document tasks derived one from the cached fee multiplier and signed it, so any caller that skipped the confirmation would pay a contract fee the user never saw. `checked_action_fee_agreement` (model) now requires the caller's agreement whenever the document type charges a fee for the action, and refuses an agreement to another fee (the contract changed after confirmation) before anything is signed. Typed `ActionFeeAgreementError` via `TaskError::ActionFeeAgreement`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… exactly - A refusal because network fees rose past the agreed tolerance carries the multiplier the network charges now; the document task adopts it, so the retry quotes and agrees to the current fee instead of failing again on a stale one. The message says the fee was updated and to try again (the pointer to a non-existent "network status" control is gone). - The confirmation reads the pricing from the agreement, so a tiny multiplier-priced fee whose tolerance rounds away is no longer called fixed, and it is built from complete sentences with named placeholders (owner-only, moderators-only and split variants). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Record the hint only from a successful claim, whose result is proof-backed, at the block time of the proved claim document (the local clock when it carries none). An "already claimed" refusal is an unproven node error and upstream offers no proved claim-status query, so it no longer marks an entitlement as spent on this device. - The Claim screen warns that the identity may have already claimed but never disables Claim, and the user can dismiss the note. - `RegisterTokenContract` refuses version 1 distribution rules (a once-per-identity distribution) where the network does not accept them, as the authoritative backend check. - Tests for every hint branch and the backend gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…es exactly
- Messages for a changed immutable property, a non-deletable document
reference and moderator fees without moderation no longer interpolate
names decoded from the node's (unproven) consensus error; the names stay
in the variant for Debug and logs only, so a hostile node cannot inject
text into a trusted banner.
- Upstream names a failed token operation only by a `&'static str` label.
Every label it uses is mapped; an unknown one is logged at warn and kept
as `TokenOperationKind::Unrecognized { label }` (Debug only) instead of
being silently folded into a generic kind.
- Raising key limits for an identity the network does not hold reports
`IdentityMissingOnNetwork`, not "not found in your local wallet".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…shown Raising a key's limits widens its spending authority and is signed with the identity's master (or unlimited critical) key, but went out on one click, and the backend recomputed the new limits from the key as Platform held it - so a key raised elsewhere meanwhile got a total the user never saw. - `KeyLimitsRaise` (model) carries the limits the user saw, the absolute limits to set and the signing key. - Raise Limits opens a confirmation naming the resulting total spending limit and expiry and the signing key; confirming dispatches exactly that. - The backend sends it unchanged. If Platform's limits differ from the reviewed ones, nothing is signed: the live key replaces the local copy and the raise is refused (`LimitsChanged`), so the screen quotes again. The signing key must still be the reviewed one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
⛔ Final review complete — 2 blocking finding(s) (commit 6be15c9) · triage: critical · Phase 2 only (no Phase 1 for this repository) |
- The pots are read with the response metadata, whose epoch is the current one; a pot already paid out in it shows Claim disabled with an explanation instead of sending a paid refusal. - The Claim gating (recipients, selected identity, empty pot, epoch, claim in flight) is a pure `claim_blocked_reason`, covered by tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
Pins the key chooser's view: a stale stored copy of an expired key is resolved to the live, extended key before its caveats are computed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bound authentication keys (IDN-022), raise confirmation (IDN-023), signer-aware key choice (IDN-024), advisory once-per-identity note (TOK-019), fee retry and changed-fee handling (DOC-010), per-epoch fee pot claims (DOC-012). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…m#4764 merges Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The withdraw screen's Check Owner Key / Check Payout Address Key buttons (and the transfer screen's Manage Transfer Key) render exactly when this device cannot sign, so a signer-aware lookup left them dead. They now use QualifiedIdentity::matching_key_now, an informational lookup that ignores private-key availability. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (no Phase 1 for this repository)
The current head still has one blocking correctness issue: a successful key-limit update broadcasts a fee-paying transition but only persists the changed key, leaving the locally displayed identity balance stale. Three additional non-blocking issues remain around claim-hint persistence, typed error payloads, and protocol-version cache fallback. CI reports successful tests, Clippy, formatting, and pin checks; PR Hygiene is still pending.
🔴 1 blocking | 🟡 3 suggestion(s)
4 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Refresh or persist the identity balance after a successful limits raise
src/backend_task/identity/key_limits.rs:128-140
update_key_limits(...).await broadcasts a fee-paying identity transition, but the success path persists only the returned public key through persist_raised_key. The upstream operation returns the rewritten key rather than a refreshed identity balance, so the local identity record retains its pre-transition balance. KeyInfoScreen then reloads that stale record, which can overstate the credits available for later fee-paying operations. Persist the post-transition balance or fetch a proved identity/balance before reporting success.
source: gpt-6.1-sol (phase2-reviewer: general)
🟡 Suggestion: Do not silently discard a failed once-per-identity claim-hint write
src/backend_task/tokens/claim_tokens.rs:114-128
After a once-per-identity claim succeeds and is proof-backed, a failure from record_once_per_identity_claim is only logged while the task still returns the ordinary success result. The claim is irreversible, while this local record prevents the UI from offering the same claim again. Losing the record can cause a later session to offer another attempt that Platform refuses and may charge. Preserve the persistence failure in a typed success-with-warning result or a dedicated post-claim error that clearly says the claim succeeded but the local record was not saved.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
🟡 Suggestion: Keep node-supplied contract-authoring details in typed sources
src/backend_task/error.rs:2916-2951
DocumentImmutablePropertyChanged, ReferencedDocumentTypeNotDeletable, and DocumentActionFeesWithoutModeration copy node-supplied names and paths into String fields even though each variant already preserves the complete Box<SdkError> source and intentionally omits those values from Display. The duplicated untrusted strings weaken the typed TaskError boundary and can be accidentally exposed if formatting changes later. Remove the redundant fields unless callers need structured values; otherwise retain the diagnostic information only in the typed source.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
🟡 Suggestion: Do not serve a cached contract from the wrong protocol version
src/context/system_contracts.rs:62-85
SystemContractsCache::get accepts an exact requested PlatformVersion, but a failed reload logs the mismatch and returns the previously cached set. context_provider::resolve_data_contract uses this result during proof verification, so a protocol-14 request can receive protocol-13 system contracts if loading the requested version fails. Version-specific contract resolution should fail closed or propagate a typed load error instead of silently mixing protocol-version-specific schemas.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large cross-cutting diff changes cryptographic signing and key handling in wallet and identity code, peer-facing protocol serialization and verification, funds movement and fee operations, and storage migration compatibility, including files such as src/wallet_backend/det_signer.rs, src/backend_task/identity/key_limits.rs, src/backend_task/protocol_13_transitions.rs, and src/context/mod.rs. - Phase 1 reviewers: not run (disabled for this repository)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (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 `src/backend_task/identity/key_limits.rs`:
- [BLOCKING] src/backend_task/identity/key_limits.rs:128-140: Refresh or persist the identity balance after a successful limits raise
`update_key_limits(...).await` broadcasts a fee-paying identity transition, but the success path persists only the returned public key through `persist_raised_key`. The upstream operation returns the rewritten key rather than a refreshed identity balance, so the local identity record retains its pre-transition balance. `KeyInfoScreen` then reloads that stale record, which can overstate the credits available for later fee-paying operations. Persist the post-transition balance or fetch a proved identity/balance before reporting success.
In `src/backend_task/tokens/claim_tokens.rs`:
- [SUGGESTION] src/backend_task/tokens/claim_tokens.rs:114-128: Do not silently discard a failed once-per-identity claim-hint write
After a once-per-identity claim succeeds and is proof-backed, a failure from `record_once_per_identity_claim` is only logged while the task still returns the ordinary success result. The claim is irreversible, while this local record prevents the UI from offering the same claim again. Losing the record can cause a later session to offer another attempt that Platform refuses and may charge. Preserve the persistence failure in a typed success-with-warning result or a dedicated post-claim error that clearly says the claim succeeded but the local record was not saved.
In `src/backend_task/error.rs`:
- [SUGGESTION] src/backend_task/error.rs:2916-2951: Keep node-supplied contract-authoring details in typed sources
`DocumentImmutablePropertyChanged`, `ReferencedDocumentTypeNotDeletable`, and `DocumentActionFeesWithoutModeration` copy node-supplied names and paths into `String` fields even though each variant already preserves the complete `Box<SdkError>` source and intentionally omits those values from `Display`. The duplicated untrusted strings weaken the typed `TaskError` boundary and can be accidentally exposed if formatting changes later. Remove the redundant fields unless callers need structured values; otherwise retain the diagnostic information only in the typed source.
In `src/context/system_contracts.rs`:
- [SUGGESTION] src/context/system_contracts.rs:62-85: Do not serve a cached contract from the wrong protocol version
`SystemContractsCache::get` accepts an exact requested `PlatformVersion`, but a failed reload logs the mismatch and returns the previously cached set. `context_provider::resolve_data_contract` uses this result during proof verification, so a protocol-14 request can receive protocol-13 system contracts if loading the requested version fails. Version-specific contract resolution should fail closed or propagate a typed load error instead of silently mixing protocol-version-specific schemas.
A key limits update is a fee-paying transition, but its proof holds only the rewritten key, and the task stored only that key. The identity kept its pre-update balance, so Key Info and later fee checks overstated what the identity could spend. After an accepted raise the task now reads the identity's proved balance and stores it in the same record write as the key. If that read fails, the raise is still not reported as failed: the key is stored and the new KeyLimitsRaisedButBalanceNotRefreshed error tells the user to refresh the identity to see its balance. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ujJpB9DBhvtbjhYB3aG9Q
…cord After a successful, proof-backed once-per-identity claim, a failure to store the local claim record was only logged and the task reported a plain success. That record is what keeps the screen from offering the claim again, so losing it silently invited a second claim that Platform refuses and charges for. The failure now maps to the typed TokensClaimedButClaimNotRecorded error, which keeps the storage error as its source and tells the user the tokens were claimed and not to claim this token again with this identity. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ujJpB9DBhvtbjhYB3aG9Q
…urce DocumentImmutablePropertyChanged, ReferencedDocumentTypeNotDeletable and DocumentActionFeesWithoutModeration copied the node's document type name, property and reference path into String fields, although each variant already keeps the full SDK error as its source and no caller reads those fields. The String fields are gone; the unproven text stays only in the typed source, for logs and the details panel. The identifiers stay. The injection test now drives the real consensus-error conversion and checks the names reach the source but never the message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ujJpB9DBhvtbjhYB3aG9Q
SystemContractsCache::get answered a failed reload with the set cached for another protocol version. The proof verifier resolves system contracts at the version it verifies with, so a protocol 14 lookup could receive protocol 13 contracts and check a proof against the wrong schema. The new SystemContractsCache::try_get returns the set for exactly the requested version or the load error. The context provider uses it and reports a failed load as a data contract failure, so verification fails closed. The app's own reads keep the previous fallback, so the UI keeps working on its last loaded contracts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ujJpB9DBhvtbjhYB3aG9Q
|
Re the thepastaclaw review of
Each fix carries a regression test. 🤖 Co-authored by Claudius the Magnificent AI Agent |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (no Phase 1 for this repository)
The main protocol-14 implementation and all 28 prior findings are addressed, and the current test, clippy, and platform-pin checks pass. Two new correctness issues remain: numeric capability gates accept unknown future protocol versions, and concurrent epoch refreshes can overwrite newer fee and protocol state with stale data.
🔴 1 blocking | 🟡 1 suggestion(s)
2 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Fail closed for unknown future protocol versions in capability gates
src/context/feature_gate.rs:77-89
These capability checks compare the raw connected protocol number directly with activation thresholds. AppContext::adopt_epoch_info stores the protocol number from a proved epoch, while the pinned SDK deliberately ignores unknown protocol versions for its own ratchet and can still verify the response using the newest known verifier tables. If a valid response reports protocol 15, these >= 14 checks enable key-limit updates, once-per-identity distributions, fee-pot claims, and shielded operations even though this build has no protocol-15 definitions. The contract-bound and non-transferable checks already use PlatformVersion::get_optional and fail closed; the numeric gates need the same known-version check.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
🟡 Suggestion: Do not let an older epoch refresh overwrite newer protocol and fee state
src/backend_task/platform_info.rs:594-598
refresh_current_epoch is used by multiple asynchronous paths, including platform-information refreshes and document-action-fee refusals. A response proving an older epoch can complete after a newer response, but adopt_epoch_info unconditionally replaces both the fee multiplier and protocol version. This can regress the connected protocol from 14 to 13 and replace current fee estimates with stale values. Store the adopted epoch index and only update epoch-derived state when the fetched epoch is at least as new; protocol-version adoption should also remain monotonic.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large cross-cutting change that directly modifies cryptographic signing and key handling, peer-facing protocol deserialization and verification, funds movement, and storage migration paths across files such as src/wallet_backend/det_signer.rs, src/context_provider.rs, src/backend_task/identity/key_limits.rs, and migration-related code. - Phase 1 reviewers: not run (disabled for this repository)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (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 `src/context/feature_gate.rs`:
- [BLOCKING] src/context/feature_gate.rs:77-89: Fail closed for unknown future protocol versions in capability gates
These capability checks compare the raw connected protocol number directly with activation thresholds. `AppContext::adopt_epoch_info` stores the protocol number from a proved epoch, while the pinned SDK deliberately ignores unknown protocol versions for its own ratchet and can still verify the response using the newest known verifier tables. If a valid response reports protocol 15, these `>= 14` checks enable key-limit updates, once-per-identity distributions, fee-pot claims, and shielded operations even though this build has no protocol-15 definitions. The contract-bound and non-transferable checks already use `PlatformVersion::get_optional` and fail closed; the numeric gates need the same known-version check.
In `src/backend_task/platform_info.rs`:
- [SUGGESTION] src/backend_task/platform_info.rs:594-598: Do not let an older epoch refresh overwrite newer protocol and fee state
`refresh_current_epoch` is used by multiple asynchronous paths, including platform-information refreshes and document-action-fee refusals. A response proving an older epoch can complete after a newer response, but `adopt_epoch_info` unconditionally replaces both the fee multiplier and protocol version. This can regress the connected protocol from 14 to 13 and replace current fee estimates with stale values. Store the adopted epoch index and only update epoch-derived state when the fetched epoch is at least as new; protocol-version adoption should also remain monotonic.
…s not know The key limits, once-per-identity distribution, fee pot and shielded capabilities compared the connected protocol number with their activation version. A proved epoch can report a version newer than any this build defines, which passed every such check although nothing here knows that version's rules. The contract-bound and non-transferable capabilities already required a version this build defines. Every capability now reads the version through known_protocol_version, which is None before the version is fetched and for an unknown version, so all of them fail closed. Protocol 15, the newest version this pin defines, keeps them open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ujJpB9DBhvtbjhYB3aG9Q
Platform info refreshes and document action fee refusals both read the current epoch, concurrently. A response proving an older epoch could land after a newer one and put back its fee multiplier and protocol version, regressing the connected protocol from 14 to 13 and fee estimates to stale values. AppContext now records the index of the adopted epoch and adopts a proved epoch only if it is at least as new, under one lock that covers the check and both stores. The protocol version only moves up, also on the DPNS-fetch fallback path. Rebuilding the connection forgets the adopted epoch, since new endpoints may reach a network whose epochs restarted; each network already has its own AppContext. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ujJpB9DBhvtbjhYB3aG9Q
|
Re the thepastaclaw review of
🤖 Co-authored by Claudius the Magnificent AI Agent |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (no Phase 1 for this repository)
The connection rebuild resets only the adopted epoch index, while retaining the cached protocol version and fee multiplier and carrying the old protocol version into the replacement SDK. In-flight epoch requests against the old SDK can also adopt stale state after the rebuild, so a lower-protocol or reset network may temporarily expose unsupported operations or use stale fee data. All build, test, clippy, and platform-pin checks passed on this head; only the non-code PR Hygiene check remains in progress.
🔴 1 blocking
1 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Do not let an older epoch refresh overwrite newer protocol and fee state
src/context/mod.rs:1257-1275
Reinitializing the SDK carries self.sdk.load().protocol_version_number() into the replacement SDK and then calls forget_adopted_epoch(), but that method clears only the epoch index. The cached platform_protocol_version and fee multiplier therefore survive a connection rebuild. If the new endpoints serve protocol 13 or a reset devnet, protocol-14 capability gates can remain open, the replacement SDK can continue constructing and validating at the stale protocol version, and fee estimates can use the previous network's multiplier. There is also a cross-generation race: run_backend_task captures an Arc to the old SDK before awaiting refresh_current_epoch; if that response completes after the swap and epoch reset, it can become the first adoption for the new connection and install stale fee/protocol state. Reset all connection-derived protocol and fee state and initialize the replacement SDK from the configured network minimum, then associate epoch adoption with a connection generation or SDK identity so responses from the previous generation are rejected.
source: gpt-6.1-sol (phase2-reviewer: general, rust-quality)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, cross-cutting diff changes cryptographic signing and key handling, peer-facing protocol deserialization and verification, funds movement and fee-related operations, and wallet storage migrations in files including src/wallet_backend/det_signer.rs, src/model/qualified_identity/mod.rs, src/context/mod.rs, and src/backend_task/migration/*.rs. - Phase 1 reviewers: not run (disabled for this repository)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (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 `src/context/mod.rs`:
- [BLOCKING] src/context/mod.rs:1257-1275: Do not let an older epoch refresh overwrite newer protocol and fee state
Reinitializing the SDK carries `self.sdk.load().protocol_version_number()` into the replacement SDK and then calls `forget_adopted_epoch()`, but that method clears only the epoch index. The cached `platform_protocol_version` and fee multiplier therefore survive a connection rebuild. If the new endpoints serve protocol 13 or a reset devnet, protocol-14 capability gates can remain open, the replacement SDK can continue constructing and validating at the stale protocol version, and fee estimates can use the previous network's multiplier. There is also a cross-generation race: `run_backend_task` captures an `Arc` to the old SDK before awaiting `refresh_current_epoch`; if that response completes after the swap and epoch reset, it can become the first adoption for the new connection and install stale fee/protocol state. Reset all connection-derived protocol and fee state and initialize the replacement SDK from the configured network minimum, then associate epoch adoption with a connection generation or SDK identity so responses from the previous generation are rejected.
…fault fees Rebuilding the connection carried the old SDK's protocol version into the replacement and only forgot the adopted epoch index, so the cached protocol version and fee multiplier survived. New endpoints serving protocol 13 or a reset devnet kept protocol 14 capabilities open, the new SDK kept building at the old version, and fee estimates kept the old multiplier. An epoch read still in flight on the old SDK could also land after the rebuild and become the new connection's first adoption. A rebuild now starts like a cold start: the replacement SDK begins at the network minimum (the carried-version seed is removed), and reset_connection_state bumps a connection generation, forgets the adopted epoch, and puts the protocol version back to unknown and the fee multiplier to the default. Epoch reads capture the generation before they are sent and adopt only on the current SDK and generation; the DPNS-fetch fallback does the same. rebuild_connection, now used by the settings rebuild and the automatic DAPI refresh, reads the new connection's epoch right away, so capabilities reopen as soon as it proves its version. Also moves the reinit doc comment back onto reinit_core_client_and_sdk. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ujJpB9DBhvtbjhYB3aG9Q
|
Re the thepastaclaw review of
Regression tests cover the reset and a stale-generation read being dropped. 🤖 Co-authored by Claudius the Magnificent AI Agent |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (no Phase 1 for this repository)
The protocol-14 integration and the previously reported fixes are present at the reviewed head, and the code, test, clippy, and pin-guard checks reported by CI are successful. One blocking concurrency defect remains: connection rebuilding publishes the replacement SDK before clearing protocol and fee state, allowing backend tasks to observe a new SDK paired with stale connection metadata.
🔴 1 blocking
1 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Publish the replacement SDK only after resetting connection-scoped protocol state
src/context/mod.rs:1333-1341
reinit_core_client_and_sdk stores the replacement SDK and only then calls reset_connection_state. Backend tasks snapshot the SDK independently in run_backend_task, while capability gates and fee estimation read the separately cached protocol version and fee multiplier. A task can therefore start after self.sdk.store(...) but before reset_connection_state(), using the new network's SDK with the previous connection's protocol capabilities and fee state. For example, a protocol-14 capability can remain enabled briefly while the task operates through a replacement protocol-13 connection, and stale fee multipliers can be applied to new-connection work. The epoch generation checks do not make ordinary SDK snapshots and connection-scoped state atomic. Synchronize rebuilding with task snapshots, or introduce a shared generation/state snapshot that tasks validate before performing work; merely swapping the two calls would move the mismatch window rather than eliminate it.
source: gpt-6.1-sol (phase2-reviewer: general, rust-quality)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large cross-cutting diff changes cryptographic signing and key handling, peer-facing protocol deserialization and verification, funds-moving wallet operations, and storage migration behavior in files including src/wallet_backend/det_signer.rs, src/backend_task/identity/key_limits.rs, and src/context/mod.rs. - Phase 1 reviewers: not run (disabled for this repository)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (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 `src/context/mod.rs`:
- [BLOCKING] src/context/mod.rs:1333-1341: Publish the replacement SDK only after resetting connection-scoped protocol state
`reinit_core_client_and_sdk` stores the replacement SDK and only then calls `reset_connection_state`. Backend tasks snapshot the SDK independently in `run_backend_task`, while capability gates and fee estimation read the separately cached protocol version and fee multiplier. A task can therefore start after `self.sdk.store(...)` but before `reset_connection_state()`, using the new network's SDK with the previous connection's protocol capabilities and fee state. For example, a protocol-14 capability can remain enabled briefly while the task operates through a replacement protocol-13 connection, and stale fee multipliers can be applied to new-connection work. The epoch generation checks do not make ordinary SDK snapshots and connection-scoped state atomic. Synchronize rebuilding with task snapshots, or introduce a shared generation/state snapshot that tasks validate before performing work; merely swapping the two calls would move the mismatch window rather than eliminate it.
…s reset reinit_core_client_and_sdk stored the replacement SDK and only then reset the cached protocol version, fee multiplier and epoch generation. A task loading the SDK in between paired the new SDK with the old connection's protocol 14 capabilities and fee multiplier. Swapping the two steps alone would let an epoch read still running on the old SDK take the new generation and be adopted. replace_connection now writes the reset (generation bump, epoch, unknown protocol, default fee multiplier) and then publishes the new SDK, both under the epoch adoption lock. Whoever loads the new SDK therefore sees the reset state. Epoch reads take the generation and the current-SDK check together through connection_snapshot_for, so they see either the old pair, which adoption drops, or the new one. A task still holding the old SDK may see the reset state, which only closes capabilities and uses default fees. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ujJpB9DBhvtbjhYB3aG9Q
|
Re: Publish the replacement SDK only after resetting connection-scoped protocol state (
The reset is written before the new SDK is published, so any task that loads the new SDK is guaranteed to see the reset protocol version and fee multiplier. The only remaining combination is a task that already held the old SDK seeing the reset state: an unknown protocol closes every protocol capability, and the default fee multiplier is the value the new connection uses until its first proved epoch. Nothing is enabled or priced from stale data that way; it fails closed. Test: 🤖 Co-authored by Claudius the Magnificent AI Agent |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (no Phase 1 for this repository)
The final head fixes all 32 previously verified findings, and the reported Test Suite, Clippy, formatting, and platform-pin checks pass. Two new blocking correctness issues remain: a confirmed key-limit raise can be overwritten by legacy-recovery dispatch, and a stale epoch refresh can be reported as successful without updating the fee cache.
🔴 2 blocking
2 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: A confirmed key-limit raise can be dropped when legacy recovery dispatches in the same frame
src/ui/identity/keys/key_info_screen.rs:950-966
pending_raise.take() removes the confirmed raise and immediately produces a non-None action. The following legacy-recovery check or restore can also produce an action, but AppAction::BitOrAssign replaces an existing action instead of accumulating both tasks. When that happens, the recovery task is dispatched while the raise has already been removed from pending_raise; raise_in_flight remains true and its progress banner cannot be cleared because no raise result or error will arrive. The comment above this code only addresses the budget read versus recovery collision and does not prevent this newer raise-versus-recovery collision. Dispatch only one operation per frame, or retain the raise until it is actually returned as the dispatched action.
source: gpt-6.1-sol (phase2-reviewer: general, rust-quality)
🔴 Blocking: Do not report a fee refresh when the epoch was read from a stale connection
src/backend_task/document.rs:170-172
refresh_current_epoch returns Ok(epoch_info) even when the epoch was not adopted. It captures the connection generation and SDK identity before awaiting the request, then ignores the boolean result from adopt_epoch; a connection rebuild or a newer adopted epoch can therefore cause the cache update to be rejected while the caller still treats the refresh as successful. fee_refusal_after_refresh then returns DocumentActionFeeMultiplierRose, whose message tells the user that the contract fee has been updated, although the cached multiplier is unchanged. Make the refresh report whether adoption occurred, and only use the updated-fee error when the current connection actually adopted the epoch.
source: gpt-6.1-sol (phase2-reviewer: general, rust-quality)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, cross-cutting diff changes cryptographic signing and key handling, wallet and identity funds operations, peer-facing protocol responses, and storage migration behavior across files such as src/wallet_backend/det_signer.rs, src/model/qualified_identity/mod.rs, and src/context/mod.rs. - Phase 1 reviewers: not run (disabled for this repository)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (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 `src/ui/identity/keys/key_info_screen.rs`:
- [BLOCKING] src/ui/identity/keys/key_info_screen.rs:950-966: A confirmed key-limit raise can be dropped when legacy recovery dispatches in the same frame
`pending_raise.take()` removes the confirmed raise and immediately produces a non-None action. The following legacy-recovery check or restore can also produce an action, but `AppAction::BitOrAssign` replaces an existing action instead of accumulating both tasks. When that happens, the recovery task is dispatched while the raise has already been removed from `pending_raise`; `raise_in_flight` remains true and its progress banner cannot be cleared because no raise result or error will arrive. The comment above this code only addresses the budget read versus recovery collision and does not prevent this newer raise-versus-recovery collision. Dispatch only one operation per frame, or retain the raise until it is actually returned as the dispatched action.
In `src/backend_task/document.rs`:
- [BLOCKING] src/backend_task/document.rs:170-172: Do not report a fee refresh when the epoch was read from a stale connection
`refresh_current_epoch` returns `Ok(epoch_info)` even when the epoch was not adopted. It captures the connection generation and SDK identity before awaiting the request, then ignores the boolean result from `adopt_epoch`; a connection rebuild or a newer adopted epoch can therefore cause the cache update to be rejected while the caller still treats the refresh as successful. `fee_refusal_after_refresh` then returns `DocumentActionFeeMultiplierRose`, whose message tells the user that the contract fee has been updated, although the cached multiplier is unchanged. Make the refresh report whether adoption occurred, and only use the updated-fee error when the current connection actually adopted the epoch.
TL;DR: Updates Dash Evo Tool for Platform v5.1-dev (5.0.0-rc.8) while retaining the protocol 14 features in this PR and integrating the current development branch.
User story
As a wallet user, I want to keep my existing wallets and identities and use the features supported by my selected network safely.
Scenario
Mainnet (protocol 13) and testnet (protocol 14) answer identity discovery in different formats. With this update, the app asks each network in the format that network has proven it supports and verifies the signed answer, so identity lookup works on both.
Detailed discussion
Dependency and integration
dash-sdk,rs-sdk-trusted-context-provider,platform-wallet, andplatform-wallet-storagetogether to Platform v5.1-dev132ae31a1356d035cd8326f27c1109685090a71d(5.0.0-rc.8). InCargo.lockonly Platform packages move.v1.0-dev, including fix(ui): deliver each task's result to the screen that started it #1091 result ownership/routing and fix(identity): recover Home contacts after startup #1099 Home contacts recovery.dashcore::secp256k1re-export removed, new ECDSA key types). Signing output is byte-identical: DET keeps RFC 6979 deterministic signatures and does not switch to the new low-R signing path.Mainnet compatibility
Upstream #5515 (included in this pin) versions the non-unique identity lookup: the SDK sends the V0 request, answered with the protocol 13 two-proof
IdentityProvedResponse(field 2), until the network has proven a protocol version that serves V1.Verified live on 2026-10-23 with upstream's ignored harness
rs-sdk/tests/identity_discovery_live.rsat this exact pin, read-only, with full proof and quorum-signature verification (no relaxed checks, no funds, no writes):The harness takes quorum keys from the trusted HTTPS quorum service, while DET takes them from SPV; the proof and signature verifier is the same. DET's own end-to-end discovery on mainnet was not run.
Existing data
The upstream V019 migration adds an asset-lock operation identifier; prior V001–V018 migration sources are unchanged, and rc.8 changes no migration. Forward migration must preserve wallet, identity, key, protection, and history data. The previous V018 reader cannot open the upgraded storage: rollback requires a pre-upgrade copy. No in-place downgrade compatibility is claimed.
Profiles opened by #1028's side-branch dependency are a separate case: its Core transaction accounting migration differs from this pin's V019. The current bridge does not recognize that history; the upgrade is safely refused as incompatible.
Scope limits
0bf85881-633b-421d-b4c0-e0b4eee736dc.Validation
--all-features --all-targets -- -D warningsand pinned formatting pass; the dependency ancestry/migration-fingerprint guard passes.786584817fafc9b4d0c4fbaaebb38797f67a8678.🤖 Co-authored by Claudius the Magnificent AI Agent
PR Hygiene ·
6be15c9AGENTS.md,Cargo.lock,Cargo.tomland 154 more) — lklimekWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.