Skip to content

feat(platform)!: token marketplace listings (PV15) - #5529

Open
QuantumExplorer wants to merge 67 commits into
claude/token-market-escrow-contractfrom
claude/token-market-listings
Open

QuantumExplorer wants to merge 67 commits into
claude/token-market-escrow-contractfrom
claude/token-market-listings

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Oct 21, 2026 •

Copy link
Copy Markdown
Member

Basic explanation

What this does: Lets holders of a tradeable token sell it to each other on Platform. A seller lists an amount at a price (TokenPlaceAsk): the tokens leave their balance and wait in escrow. A buyer takes part or all of the listing (TokenFillAsk): the tokens go to the buyer and the credits go to the seller in the same block, so neither side has to trust the other. The seller can withdraw what is left at any time (TokenCancelOrder).

Value: A token can now have a real secondary market without an off-chain exchange or a trusted middleman. Order books are readable with the document queries and proofs every SDK already has, because each listing is an ask document of the token market contract.

Risks: Medium. Three new token transitions from protocol version 15. Before 15 each is refused unpaid at the batch's is_allowed gate, as software that cannot decode them refuses them, so no earlier version changes. From 15 the tests check that token supply stays balanced through listings, partial fills, sell-outs and cancels, and that a fill can never move more than the listing holds or cost the buyer more than their limit.

Issue being fixed or feature implemented

The third PR of the token marketplace stack (PV15), on top of #5526 (escrow trees and the token market contract). This one adds the sell side: listings. Offers (bids) come next.

What was done?

Transitions (TokenTransition variants 18 to 20, appended)

Transition Fields Effect
TokenPlaceAsk amount, price Seller balance to escrow, ask document created
TokenFillAsk orderId, minAmount, maxAmount, maxTotalPrice Escrow to buyer, buyer credits to seller, ask rewritten or deleted
TokenCancelOrder orderId, side Escrow back to the owner, order deleted

price is credits per whole token. The order id is derived from the token, the seller and the nonce, so a client knows it before placing.

The minimum fill and the decimals are the token's. A listing signs neither:

Before: TokenPlaceAsk { amount, price, decimals, minFill }
After:  TokenPlaceAsk { amount, price }
        marketplaceRules.tradeMode: { "Tradeable": { "minFill": 100 } }   (was "Tradeable")
Before: a holder cannot sell a token to another holder on Platform without trusting them.

After, a listing of 1,000 base units at 10 credits each, of a token whose minimum fill is 100:
  place                         seller 100,000 -> 99,000 tokens, 1,000 in escrow
  fill max 400, limit 4,000     buyer +400 tokens, -4,000 credits plus fee; seller +4,000 credits
  fill max 10,000               takes the 600 left; the listing closes and the seller
                                gets its storage back
On a fresh listing:
  fill min 950, max 950         refused: 950 would leave 50 open, under the minimum fill,
                                so the fill would take 900, under the buyer's 950

Rounding: each fill pays the change in the listing's running total, rounded up, so splitting never changes the total:

1.5 credits per base unit, two fills of 3:  5 + 4 = 9 = 6 x 1.5

Validation

  • Structure (dpp): amount and price in 1..=i64::MAX; a fill needs 1 <= min <= max <= i64::MAX. One function, validate_token_order_terms, checks a new order's terms, so an offer shares it; validate_token_order_terms_for_token checks what it needs of the token (below). New basic errors InvalidTokenOrderPriceError (10462), InvalidTokenOrderFillRangeError (10463) and InvalidTokenOrderValueError (10465); 10464 is unused (orders have no expiry).
  • Transformer (drive): reads the order document; not found, wrong token, self fill, fill range, price limit and not owned each bump the nonce with their error. A listing reads the token's configuration through token_configuration_or_position_error, so a listing of a token its contract does not define bumps the nonce with ContractHasNoTokensError or InvalidTokenPositionError instead of failing as an internal error, and a fill checks the same before reading the listing; the token base structure validation now reads the same helper.
  • Decimals: a listing records the token's decimals when it is placed (ask.decimals, from feat(platform)!: token marketplace escrow trees and token market contract (PV15) #5526) and every fill prices it at them, so an authorized conventions update cannot reprice an open listing. A fill reads the token's minimum fill, not its decimals.
  • Advanced structure (drive-abci, batch advanced_structure 2 from feat(platform)!: tradeable tokens (PV15) #5524, per-action structure validations 0): the rules that read nothing beyond the contract and the block run here, so check_tx refuses them too. A listing needs a tradeable token, an amount of at least the token's minimum fill (10463) and a worth of at most i64::MAX credits at the token's decimals (10465), all paid; a fill a token that is still tradeable.
  • State (drive-abci): not paused, seller not frozen, seller holds the amount; for a fill also the buyer not frozen unless the token allows transfers to frozen balances. A cancel needs nothing beyond ownership, so an order can always be withdrawn.
  • New state errors 40750 to 40756: TokenNotTradeableError (40750), TokenOrderNotFoundError (40751), TokenOrderFillAmountError (40753), TokenOrderPriceLimitError (40754), TokenOrderNotOwnedError (40755), TokenOrderSelfFillError (40756). 40752 is unused (orders have no expiry), and so is 40757 (the value limit is a structure check).
  • State error 40758 TokenOrderDecimalsMismatchError is defined but unused: orders name no decimals. The placement proof reads the decimals the order recorded.
  • The fill's cost counts in the batch's used balances, like a direct purchase, so fee validation checks the buyer can pay.
  • Group actions are refused for the three kinds (they have no action id).

Execution (drive): two new token ops, TokenPlaceInOrderEscrow and TokenReleaseFromOrderEscrow, over the escrow primitives of #5526; the ask document carries the seller's storage flags.

Gate: TokenTransition::is_marketplace_order(), an exhaustive classifier, drives the PV15 is_allowed refusal, so an order kind added later is gated without being listed.

Proofs: TokenPlaceAsk proves the ask document and checks its terms. The order id follows from the token, the signer and the nonce, so only this transition creates a document with it: one holding its terms proves the placement after partial fills too, with remaining from the amount down to 1, so a client asking for the proof after a fill still confirms it (a listing filled entirely is proved through its history record from #5531); TokenFillAsk the buyer's token balance; TokenCancelOrder the order's absence. Drive's verify feature now enables dpp/token-market-contract, so a verify-only build (wasm-drive-verify) has the contract the order proofs read: cargo tree -p wasm-drive-verify -e normal,features -i data-contracts lists data-contracts feature "token-market" with the change and not without it.

Events: TokenEvent::OrderPlaced, OrderFilled, OrderCancelled (appended, with JSON shapes). History documents come in a later PR, so these transitions do not write token history yet.

Bindings: wasm-dpp2 TokenPlaceAskTransition, TokenFillAskTransition, TokenCancelOrderTransition and the three event variants (every integer setter takes a BigInt through try_to_u64 and leaves the transition unchanged when it is out of range); wasm-dpp reports the kinds and points to wasm-dpp2; the platform wallet's signing summary shows each.

Docs: book data-model/token-marketplace.md (listings, rounding, rules, storage, proofs) and v15 notes 5 and 13.

How Has This Been Tested?

  • cargo test -p drive-abci --lib -- batch::tests::token::marketplace (16 tests, PV15 blocks):

    • listing moves tokens into escrow, the ask holds the terms, the proof verifies;
    • partial fill then sell-out: exact seller credits, buyer pays price plus fee, escrow and document follow, supply balanced, fill proof verifies;
    • running-total rounding across two fills;
    • cancel after a partial fill returns the rest, the absence proof verifies;
    • refusals: not tradeable (check_tx too), more than held, value too large (unpaid, check_tx too), self fill, price limit, min fill, not found, not owner, paused (cancel still works), buyer cannot pay.
  • cargo test -p drive-abci --lib -- token_marketplace_gate: each order kind refused before 15 under its name, allowed from 15.

  • Review round 2, in batch::tests::token::marketplace:

    • placement, a partial fill, a conventions update from 8 decimals to 2 and a second fill: the second fill costs 1,000 credits at the listing's 8 decimals (at 2 it would cost 1,000,000,000 and the buyer's limit would refuse it); a listing placed after the update records 2;
    • a listing of position 1 of a one-token contract and of DPNS (no tokens) is refused by check_tx and in a block, paid, with InvalidTokenPositionError and ContractHasNoTokensError;
    • a fill naming position 1 is refused the same way, before the listing is read;
    • a buyer short of the fee and a buyer short of the price are each refused with exactly one UnpaidConsensusError(IdentityInsufficientBalanceError), with their credits, nonce, token balance and the listing unchanged.
  • yarn workspace @dashevo/wasm-dpp2 exec mocha tests/unit/TokensTransitions.spec.ts (81 passing): each listing setter sets a value in range and throws on 2^64 and -1, keeping the previous value.

  • cargo test -p drive-abci --lib -- batch::tests::token (regressions in the existing token tests). Round 2: batch::tests::token --skip shielded (264 passed), batch::tests::token::marketplace (20).

  • cargo test -p dpp --features all_features --lib -- token_order consensus::codes state_error basic_error token_event marketplace_rules.

  • cargo test -p drive --lib -- state_transition_action::batch order_escrow verify::state_transition.

  • cargo test -p wasm-dpp2 --lib -- token_event (documented event fields match the serializer).

  • cargo clippy on dpp, drive, drive-abci, wasm-dpp, wasm-dpp2, platform-wallet, platform-version.

  • Review round 2, in batch::tests::token::marketplace:

    • should_refuse_a_listing_signed_at_other_decimals: a holder other than the conventions controller signs a listing at 8 decimals, the contract owner changes the token to 16, and the original listing is refused by check_tx and in a block with TokenOrderDecimalsMismatchError (40758), nothing escrowed; without the check it would execute;
    • should_refuse_filling_a_frozen_makers_listing_and_let_the_maker_cancel: a fill of a frozen maker's listing is refused with the maker's IdentityTokenAccountFrozenError, balances and escrow unchanged, and the frozen maker cancels and gets the escrow back;
    • should_refuse_a_frozen_buyers_fill_unless_the_token_allows_transfers_to_frozen_balances: refused with the buyer's IdentityTokenAccountFrozenError when the token disallows it, executed into the frozen balance when it allows it;
    • after a sell-out the placement proof fails with exactly ProofError::IncorrectProof;
    • wasm-dpp2 specs: decimals is required, settable within a byte, and refused outside it.
  • Review round 4 (the decimals part superseded in round 5): the wallet's listing row names the signed decimals (two listings differing only in them render differently); a listing of two whole tokens at i64::MAX credits at 0 decimals is refused unpaid with InvalidTokenOrderValueError (10465) by check_tx and in a block (dpp unit test at the boundary too); every marketplace proof round trip compares its root with the committed Drive root; a fill whose rounded running total does not change (0.5 credits per base unit, the second of three units) pays 0 credits, takes its unit, advances the nonce and charges its fee.

  • Round 5 (minimum fill and decimals from the token):

    • should_refuse_a_listing_of_less_than_the_tokens_minimum_fill: 99 at a minimum of 100 is refused with 10463 by check_tx and in a block; 100 is placed.
    • should_fill_open_listings_by_the_tokens_current_minimum_fill: the owner raises the minimum from 1 to 600 after a listing of 1,000; a fill of 500 is refused (TokenOrderFillAmountError naming 600), one of everything executes.
    • the value refusal (10465) is now paid, at the token's decimals;
    • dpp: the trade mode's JSON form and bincode tag, the minimum's bounds (0 and i64::MAX + 1 refused), fill_amount with the token's minimum.
    • cargo test -p drive-abci --lib -- tests::token::marketplace is_allowed contract_structure_error tests::token::non_transferable (71 passed), cargo test -p drive --lib -- order_escrow verify token (978), cargo test -p dpp --all-features --lib -- token_order token_place_ask token_configuration token_marketplace_rules token_configuration_item (227), cargo test -p platform-wallet --lib state_transition_summary.
    • Swift and Kotlin parse the trade mode's object form: the Kotlin TokenMaterializerTradeModeTest passes; the Swift DataContractParserTradeModeTests were updated but not run (no xcframework in this worktree).

In-place changes to shipped generations

  • TokenTradeMode::Tradeable gains its min_fill field in place. The variant is feat(platform)!: tradeable tokens (PV15) #5524's, admitted only from protocol version 15 (refused as not active before), so no stored contract or configuration update holds it; NotTradeable keeps its tag and its JSON form.

  • The token base transition action's advanced structure validation (structure_v0, every protocol version) now reads its rule through TokenBaseTransitionActionAccessorsV0::token_configuration_or_position_error. The helper holds the same condition and builds the same two errors with the same fields, so every result is unchanged; it moved so the listing transformer can refuse an unknown token with the same error.

  • The state transition proof paths: verify_state_transition_was_executed_with_proof v0, which its v1 dispatcher generation also calls, the execution path queries a node proves a transition with, and the proof guarantee classifier gain arms for TokenPlaceAsk, TokenFillAsk and TokenCancelOrder. Every arm for an existing kind is unchanged, so a proof of any transition an earlier protocol version admits verifies, or is refused, exactly as before. The new arms are reached only by the marketplace kinds, which the batch's is_allowed gate refuses before protocol version 15, so no earlier version can execute one for a client to verify. These paths are also not consensus: they build and check proofs for clients, and no node's state or result depends on them. A new verifier generation would only duplicate the dispatcher without changing what any version verifies.

  • The batch's other shipped v0 paths gain arms for TokenPlaceAsk, TokenFillAsk and TokenCancelOrder only: BatchTransition::validate_base_structure_v0 (each kind's structure validation), the batch transformer v0 (each kind's transformation), the batch state validation v0 (each kind's state validation), outputs_only_bundle_tag_v0 in the shielded-proof processor and the accepted token pool nullifiers of the state validation v0 (both list the three kinds among those that touch no shielded pool). No arm of an existing kind changes. The batch's is_allowed gate refuses the three kinds, unpaid, before protocol version 15, so on protocol versions 1 through 14 none of the new arms is reached and every block replays as before.

Breaking Changes

Consensus, from protocol version 15: three new token transitions, new consensus errors and new token events. Earlier protocol versions are unchanged: the new kinds are refused there before anything else runs.

Wire and API, all unreleased (protocol version 15):

  • TokenTradeMode::Tradeable is Tradeable { min_fill }; its JSON form is { "Tradeable": { "minFill": n } }, no longer the string "Tradeable". A downstream match needs Tradeable { .. }. wasm-dpp2's TokenTradeMode.Tradeable(minFill) takes the minimum and has a minFill getter.
  • TokenOrder loses min_fill; fill_amount takes the token's minimum fill.

Checklist:

  • I have performed a self-review of my own code
  • My Rust, FFI, WASM, Swift or Kotlin changes follow the coding conventions
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed
  • If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

QuantumExplorer and others added 3 commits October 21, 2026 22:36
Holders of a tradeable token can list it for sale (TokenPlaceAsk), buy part
or all of a listing (TokenFillAsk) and withdraw an open order
(TokenCancelOrder). Listed tokens wait in escrow; a fill moves them to the
buyer and the buyer's credits to the seller in the same transition, paying
the change in the listing's rounded-up running total.

Each kind is refused unpaid before protocol version 15 by the batch's
is_allowed gate, driven by the exhaustive TokenTransition::is_marketplace_order.
New basic errors 10462-10463 and state errors 40750-40757, token events
OrderPlaced/OrderFilled/OrderCancelled, proofs for each kind, wasm-dpp2
bindings, the wallet signing summary, the book and v15 note 7.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…market-listings

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A listing's tradeable token, value and expiry, and a fill's tradeable token,
read nothing beyond the contract the action carries and the block, so they
move from state validation to per-action structure validations 0 that the
batch's advanced structure 2 runs, where check_tx runs them too. Paused,
frozen and balance checks stay in state validation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

@thepastaclaw

thepastaclaw commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

⛔ Final review complete — 3 blocking finding(s) (commit b92488f) · triage: critical

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

📖 Book Preview built successfully.

Download the preview from the workflow artifacts.
To view locally: download the artifact, unzip, and open index.html.

Updated at 2026-10-31T13:14:25.804Z

QuantumExplorer and others added 3 commits October 22, 2026 00:05
…market-listings

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…market-listings

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
add_to_token_order_escrow_operations now takes the batch's earlier operations. A transition places one listing and its operations apply as their own batch, so the listing converter passes none.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Phase 1 + Phase 2

The marketplace flow is broadly coherent, but several newly introduced paths can either alter the economic meaning of an existing listing, reject user-controlled data as an internal error, or accept incomplete execution proofs. The verify-only feature configuration and WASM numeric setters also leave valid consumers exposed to broken proof verification or silently changed financial terms.

🔴 5 blocking | 🟡 1 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🔴 Blocking: Enable the token-market contract in Drive's verify feature
packages/rs-drive/Cargo.toml:152-160

The new verifier unconditionally loads the TokenMarket system contract, but Drive's verify feature enables dpp/system_contracts, dpp/token-history-contract, and dpp/withdrawals-contract without dpp/token-market-contract. In the production wasm-drive-verify dependency configuration, default features are disabled and only Drive's verify feature is enabled; the data-contracts crate therefore uses its ContractNotIncluded("token-market") implementation. Standalone verify-only consumers cannot verify valid PlaceAsk or CancelOrder proofs even though full/server builds work through broader feature unification. Add dpp/token-market-contract to Drive's verify feature and cover verification with the verify-only feature set.

source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This is a large, intricate change to consensus-critical token state transitions and platform funds movement, including escrow accounting, validation, serialization, and execution across DPP, Drive, and Drive ABCI.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-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/src/state_transition_action/batch/batched_transition/token_transition/token_fill_ask_transition_action/v0/transformer.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_fill_ask_transition_action/v0/transformer.rs:159-160: Preserve the listing pricing scale across convention changes
  Every fill derives the price scale from the token's current `decimals`, but `TokenConfigurationChangeItem::Conventions` can update that value after an order is placed and `TokenOrder` stores no placement-time scale. For example, an order priced at 100 credits with zero decimals charges 100 credits for its first base unit; after changing to two decimals, the next unit costs only `ceil(200 / 100) - ceil(100 / 100) = 1` credit. This lets an authorized convention update change the terms of an already-open listing without the maker signing new terms. Decreasing the decimals can also make a previously valid order exceed the representable credit bound, causing `fill_credits` to return an error that this `?` propagates as an internal failure. Persist the scale with the order or reject convention changes/fills that are incompatible with open orders, and test placement, partial fill, convention update, and subsequent fill.

In `packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_place_ask_transition/validate_structure/v0/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_place_ask_transition/validate_structure/v0/mod.rs:36-41: Reject expiresAt values that cannot be stored in the order document
  The structural validator bounds amount, price, and min_fill but accepts any `u64` expiry. The place transformer constructs `TokenOrder::new_document` before advanced structure validation, and `TokenOrder::properties()` converts `expires_at` to signed `i64`; values above `i64::MAX` therefore return `ProtocolError::Overflow` through the transformer rather than a deterministic consensus validation error. A signed transition with `expires_at = i64::MAX + 1` can consequently take the internal-error path instead of the normal paid refusal and nonce handling. Validate the signed storage-width bound here, while retaining the block-time comparison in advanced validation, and cover `i64::MAX` and `i64::MAX + 1`.

In `packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`:
- [BLOCKING] packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:1362-1367: Verify that a placed listing retains its full remaining amount
  PlaceAsk proof verification checks the order id, token, maker, price, amount, min_fill, and expiry, but not `order.remaining`. The transition's placement semantics require `remaining == amount`; without that check, a cryptographically valid proof of an order with the same terms but a smaller remaining amount is accepted as proof that the placement executed. Add `order.remaining != token_place_ask_transition.amount()` to the mismatch condition.

In `packages/rs-drive/Cargo.toml`:
- [BLOCKING] packages/rs-drive/Cargo.toml:152-160: Enable the token-market contract in Drive's verify feature
  The new verifier unconditionally loads the TokenMarket system contract, but Drive's `verify` feature enables `dpp/system_contracts`, `dpp/token-history-contract`, and `dpp/withdrawals-contract` without `dpp/token-market-contract`. In the production `wasm-drive-verify` dependency configuration, default features are disabled and only Drive's `verify` feature is enabled; the data-contracts crate therefore uses its `ContractNotIncluded("token-market")` implementation. Standalone verify-only consumers cannot verify valid PlaceAsk or CancelOrder proofs even though full/server builds work through broader feature unification. Add `dpp/token-market-contract` to Drive's `verify` feature and cover verification with the verify-only feature set.

In `packages/wasm-dpp2/src/state_transitions/batch/token_transitions/token_place_ask.rs`:
- [BLOCKING] packages/wasm-dpp2/src/state_transitions/batch/token_transitions/token_place_ask.rs:113-133: Validate BigInt ranges in marketplace setters before mutation
  The constructors convert marketplace integers with `try_to_u64`, but the new setters expose raw `u64`/`Option<u64>` WASM ABI parameters. Out-of-range JavaScript BigInts can therefore be converted by the ABI rather than rejected, silently changing the signed financial terms before the DPP validator sees them; for example, a value above `u64::MAX` can wrap to a different valid price, and the same issue affects amount, minFill, expiry, and the fill transition's minAmount, maxAmount, and maxTotalPrice setters. Existing wrappers in this crate accept `&js_sys::BigInt` and call `try_to_u64`, returning an error before mutating the transition. Apply that pattern to all new marketplace integer setters and add regression coverage that failed conversions leave the prior value unchanged.

In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/marketplace/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/marketplace/mod.rs:827-834: Assert the specific insufficient-funds rejection
  The test only asserts that the result is not `SuccessfulExecution`. It therefore also passes for an internal error, an unrelated validation error, `NotExecuted`, an empty result list, or multiple results. This does not establish that the fill's purchase credits are included in fee validation and that an unaffordable buyer receives the expected paid consensus rejection. Match exactly one expected insufficient-balance or fee error while retaining the balance and escrow assertions.

QuantumExplorer and others added 3 commits October 22, 2026 08:58
- A listing records the token's decimals when it is placed and every fill
  prices it at them, so a conventions update cannot reprice an open listing.
  Fills read no token configuration.
- An expiresAt above i64::MAX is refused by the structure validation with
  the new basic error InvalidTokenOrderExpiryError (10464), through one
  validate_token_order_terms shared with offers.
- A listing or a fill naming a token its contract does not define bumps the
  nonce with ContractHasNoTokensError or InvalidTokenPositionError instead
  of failing as an internal error; the token base structure validation reads
  the same helper (token_configuration_or_position_error).
- The PlaceAsk proof check also requires remaining == amount.
- Drive's verify feature enables dpp/token-market-contract, so a
  verify-only build has the contract the order proofs read.
- wasm-dpp2 listing setters convert BigInts with try_to_u64 and leave the
  transition unchanged when out of range.
- The cannot-pay test asserts the exact unpaid IdentityInsufficientBalanceError
  for a buyer short of the fee and one short of the price.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QuantumExplorer and others added 5 commits October 22, 2026 10:11
The PlaceAsk proof accepted only an untouched ask, so a client asking for
the proof after a fill could not confirm its placement. The order id
follows from the token, the signer and the nonce, so only this transition
creates an ask with it: one holding the transition's terms proves the
placement with any remaining from the amount down to 1.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

At head d4ab767, verification confirms one blocking financial-authorization issue and two targeted test-coverage suggestions. Five prior findings are fixed; the requirement that placement proofs retain the full original amount is withdrawn because legitimate partial fills preserve evidence of placement. This was a static review without builds or tests; current listed CI checks pass, but no Rust validation result is exposed in those checks.

🔴 1 blocking | 🟡 2 suggestion(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This is a large, intricate diff that changes consensus-critical token state transitions and directly moves tokens and credits through escrow, including validation, serialization, protocol-version gating, storage operations, and proof verification.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-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/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs:102-105: Bind the pricing scale into the seller's signed placement
  This lookup derives the listing's pricing denominator from mutable configuration at execution time. Neither TokenPlaceAskTransitionV0 nor its base transition signs expected decimals or a configuration version. A controller authorized to update conventions can therefore change another holder's pending sale without the holder's signature: 1,000 base units at a signed price of 1,000,000,000 yield 10,000 credits with 8 decimals, but increasing decimals to 16 before placement makes the entire listing cost only ceil(1000 * 1000000000 / 10^16) = 1 credit. Both configurations are valid. With the controller and seller being distinct identities, the configuration update does not invalidate the seller's nonce; the controller can subsequently buy the escrow and restore the conventions. Persisted decimals protect orders only after placement, and the buyer's maximum-price limit cannot protect the seller against this reduction. Include the seller's pricing scale in the signed DPP transition, validate any required configuration agreement before escrow creation, and compare the signed scale during placement-proof verification. Add an execution regression that signs as a holder distinct from the conventions controller, applies a conventions update, and then submits the original signed listing.

In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_fill_ask_transition_action/state_v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_fill_ask_transition_action/state_v0/mod.rs:81-89: Cover the marketplace's frozen-account branches
  The new marketplace execution suite never freezes either participant. It therefore does not exercise the distinction between checking the order's maker and checking the transaction's signer, or the exception allowing receipt into a frozen balance. Add cases through process_raw_state_transitions for a frozen maker, a frozen buyer with transfers to frozen balances disabled, and a frozen buyer with them enabled. On refusals, match IdentityTokenAccountFrozenError and the affected identity, and assert that escrow and token balances remain unchanged. Also verify that the frozen maker can still cancel and recover the remaining escrow. Existing transfer and freeze tests do not cover this marketplace-specific wiring.

In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/marketplace/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/marketplace/mod.rs:617: Assert the specific proof failure after a sell-out
  After the sell-out, this assertion accepts every verifier error rather than establishing that a valid absence proof cannot prove placement. The helper already panics on proof-generation failures, but unrelated verification errors such as UnknownContract or UnexpectedResultProof would still satisfy is_err(). The placement verifier explicitly returns ProofError::IncorrectProof when the expected listing is absent; match that variant to pin the intended rejection.

QuantumExplorer and others added 2 commits October 22, 2026 15:19
A listing's price is per whole token, so the token's decimals set what it
is worth, and they were read from the token's configuration when the listing
executed: whoever controls the token's conventions could change them between
the seller signing and execution and reprice the listing (1,000 base units
at 1,000,000,000 credits per whole token are 10,000 credits at 8 decimals,
1 credit at 16).

- TokenPlaceAskTransitionV0 gains a signed `decimals` (constructor, wasm-dpp2
  wrapper with a required option and checked setter).
- The listing's structure validation (batch advanced structure, so check_tx
  too) refuses it with the new TokenOrderDecimalsMismatchError (40758)
  unless they are the token's. The order records them, and the placement
  proof compares them.
- Tests: the attack, by a holder other than the conventions controller, is
  refused; fills of a frozen maker's listing and by a frozen buyer (unless
  the token allows transfers to frozen balances) are refused and the frozen
  maker can cancel; the sell-out proof failure is IncorrectProof.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QuantumExplorer added a commit that referenced this pull request Oct 22, 2026
As a listing does (#5529): TokenPlaceBidTransitionV0 gains a signed
`decimals` (constructor, wasm-dpp2 wrapper). The transformer refuses the
offer with TokenOrderDecimalsMismatchError (40758, check_tx too) unless they
are the token's, before the escrow is worked out from them, so whoever
controls the token's conventions cannot change them between signing and
execution and make the buyer escrow more (1,000 base units at 1,000,000,000
credits per whole token hold 10,000 credits at 8 decimals, 100,000,000 at
4). The order records them and the placement proof compares them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QuantumExplorer and others added 4 commits October 22, 2026 15:57
…its proof

A placement is never a group action and the node refuses group information on it, but the proof verifier does not run that validation: an executed listing re-sent with group information would verify. The PlaceAsk arm refuses it first.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QuantumExplorer added a commit that referenced this pull request Oct 22, 2026
…t sales' frozen-account roles

- The PlaceBid proof arm refuses group information first, as the PlaceAsk
  arm does (#5529): a placement is never a group action, and the verifier
  does not run the node's structure validation.
- Tests: an executed offer re-sent with group information is refused and
  the offer itself proves; a sale into an offer is refused naming the frozen
  seller whatever the token allows, and naming the frozen buyer unless the
  token allows transfers to frozen balances (then the tokens land in the
  frozen balance); a frozen buyer cancels and recovers its escrow.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d position

The proof verifier read a marketplace transition by its token id without
checking that the id is the one its contract and position derive, as the
node's structure validation requires. The genuine proof of an executed
listing verified the same request re-signed naming another position of a
two-token contract (or another contract) with the same token id, a request
the node refuses. Every marketplace transition (is_marketplace_order) now
refuses a token id its contract and position do not derive.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…igit number

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

The marketplace implementation covers escrow conservation, paid failures, proof roots and guarantees, bounds, frozen-account behavior, and same-block fills. Four in-scope blocking issues remain: consensus-critical order arithmetic and order-ID derivation bypass PlatformVersion dispatch, and a signed placement can be repriced by a token-conventions update before inclusion; the approval summary also omits the effective pricing scale. Kotlin SDK checks and the book preview remain pending, with no failed code checks in the supplied CI snapshot.

🔴 4 blocking | 🟡 1 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Preserve Tradeable minFill in mobile token models
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257

The contract JSON now carries the economically relevant minFill, but the Swift model intentionally reduces Tradeable to the variant name, and the Kotlin entity/materializer make the same choice. A persisted mobile token therefore cannot report the minimum fill that governs marketplace operations; consumers may display incomplete rules or construct fills that are predictably rejected. Persist the minimum-fill value in the mobile models with the required schema migration, or provide a canonical API that exposes it without discarding it during materialization.

source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)

4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This large, intricate diff changes consensus-critical token state transitions and validation plus escrowed token and credit funds movement across rs-dpp, rs-drive-abci, and rs-drive.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-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/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257: Preserve Tradeable minFill in mobile token models
  The contract JSON now carries the economically relevant `minFill`, but the Swift model intentionally reduces `Tradeable` to the variant name, and the Kotlin entity/materializer make the same choice. A persisted mobile token therefore cannot report the minimum fill that governs marketplace operations; consumers may display incomplete rules or construct fills that are predictably rejected. Persist the minimum-fill value in the mobile models with the required schema migration, or provide a canonical API that exposes it without discarding it during materialization.

In `packages/rs-dpp/src/tokens/token_order.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order.rs:93-141: Version token order pricing and fill arithmetic through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4372881399)
  The new `fill_amount`, `fill_credits`, and `order_value` helpers determine consensus-visible fill quantities, rounding, credit transfers, and representable-value bounds, but they accept no `PlatformVersion` and are absent from the DPP method-version tables. They are called from validation and the Drive transformation path, so changing rounding, minimum-fill handling, or high-decimal behavior later would change the behavior of the already-selected protocol-15 implementation instead of selecting a new generation. Add a frozen v0 implementation and PlatformVersion-selected dispatch, then thread the active version through every validation, transformation, fee, and proof caller.

In `packages/rs-dpp/src/tokens/token_order_side.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order_side.rs:74-92: Version the consensus order-ID derivation through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4371726120)
  `TokenOrderSide::order_id` directly selects `Document::generate_document_id_v0`, and `order_id_preimage_size` fixes the associated hash-cost calculation without accepting a platform version or consulting a version table. The result is the persisted order-document key, escrow key, proof path, and marketplace identifier. A future change to the preimage or derivation would therefore require editing the behavior in place and could make nodes derive different storage locations for the same placement. Add a PlatformVersion-selected order-ID generation with an explicit v0 implementation and thread it through preimage sizing, fee derivation, placement, proof, query, and event paths.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs:95-120: Bind the pricing scale into the seller's signed placement
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4347888980)
  The placement transition signs only `amount` and `price`, while the transformer reads `decimals` from mutable token conventions immediately before creating the order document. A seller can sign while the token has 8 decimals, then an authorized conventions update can change it to 16 before inclusion; the same signed price is then interpreted at the new scale, changing the seller's economics without another signature. For example, 1,000 base units at 1,000,000,000 credits per whole token changes from 10,000 credits at 8 decimals to 1 credit at 16 decimals. Carry the intended scale in the signed transition and reject a mismatch before escrow creation, or otherwise make convention changes invalidate pending placements.

In `packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs:600-604: Include the signed pricing scale in listing approval summaries
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4352824559)
  The approval summary renders only `price: <value> credits per whole token`, while the current transition carries no signed scale and placement resolves the scale from mutable token state. Consequently, otherwise identical signing requests can result in materially different listing economics after a conventions update, with no approval-screen detail exposing that ambiguity. Bind the scale into the signed placement and render it in the summary, or show the exact trusted placement-time scale together with an execution-time agreement check.

QuantumExplorer and others added 2 commits October 27, 2026 18:13
…market-listings

v5.1-dev's DataContractCompiledMaxSizeExceededError takes BasicError
discriminant 207, so the token order errors follow at 208 to 210 (their
codes are unchanged). The listings note is now v15 note 11, and the
generated decimals it names note 9.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

The marketplace escrow, proof, fee, and activation-gate paths are substantially covered, and the available CI checks for this head succeeded. However, the current design still derives a pending listing's pricing scale from mutable token configuration after signing, omits that scale from approval summaries, and leaves the new consensus order identity and fill arithmetic outside platform-version dispatch. These are in-scope blocking issues for a protocol feature that changes committed order state and user-authorized economics.

🔴 4 blocking | 🟡 1 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Preserve Tradeable minFill in mobile token models
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257

The Swift parser and TokenTradeMode intentionally reduce {"Tradeable":{"minFill":...}} to the bare Tradeable case and do not persist minFill; the Kotlin materializer and entity make the same lossy conversion. minFill is a live token rule used by every marketplace fill, so mobile state cannot faithfully display or round-trip the contract's trading terms and may construct fills that the platform rejects. Preserve the value in a u64-safe representation or retain the original marketplace-rules payload, with the corresponding persistence migration.

source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality)

4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-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: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This is a large, intricate change that modifies consensus-critical protocol-version dispatch and validation, token state transitions involving escrowed funds, serialization, storage operations, and cross-language bindings.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-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/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257: Preserve Tradeable minFill in mobile token models
  The Swift parser and `TokenTradeMode` intentionally reduce `{"Tradeable":{"minFill":...}}` to the bare `Tradeable` case and do not persist `minFill`; the Kotlin materializer and entity make the same lossy conversion. `minFill` is a live token rule used by every marketplace fill, so mobile state cannot faithfully display or round-trip the contract's trading terms and may construct fills that the platform rejects. Preserve the value in a u64-safe representation or retain the original marketplace-rules payload, with the corresponding persistence migration.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs:98-100: Bind the pricing scale into the seller's signed placement
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4347888980)
  `TokenPlaceAskTransitionV0` signs only `amount` and `price`, while placement reads `conventions().decimals()` from mutable token configuration immediately before creating the order. A conventions controller can therefore change the scale after the seller signs but before placement executes; for example, a listing signed at 8 decimals can execute at 16 decimals and represent a materially lower credit price without a new seller signature. Persisting decimals in the order protects only after placement and does not protect the pending signed transition. Carry the pricing scale, or an equivalent configuration commitment, in the signed DPP transition and reject a mismatch before escrow creation; propagate the signed value through proof verification and summaries.

In `packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs:600-604: Include the signed pricing scale in listing approval summaries
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4352824559)
  The approval summary renders only `price: ... credits per whole token`, but the transition has no signed decimals field and the execution path resolves the scale from mutable token metadata at placement time. Consequently, an approval UI cannot distinguish listings with the same numeric price but materially different economic meaning, nor show the scale that will be applied if configuration changes before placement. Make the pricing scale a signed transition term and render that exact signed value in the summary rather than deriving it from mutable metadata.

In `packages/rs-dpp/src/tokens/token_order.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order.rs:93-141: Version token order pricing and fill arithmetic through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4372881399)
  `fill_amount`, `fill_credits`, and `order_value` determine consensus-visible fill selection, rounding, escrow value, and credit movement, but are direct unversioned methods. They are called by the Drive transformation and therefore affect committed balances and order state. A future correction to minimum-fill handling, decimal rounding, or representable-value behavior would require changing the same implementation in place instead of selecting a new generation while preserving PV15 replay semantics. Add an explicit token-order arithmetic version to the DPP/platform-version tables, dispatch an explicit v0 implementation, and thread the selected platform version through Drive, proof, and event callers.

In `packages/rs-dpp/src/tokens/token_order_side.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order_side.rs:74-86: Version the consensus order-ID derivation through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4371726120)
  `TokenOrderSide::order_id` directly calls `Document::generate_document_id_v0` without accepting a `PlatformVersion` or consulting a token-order version slot. Its result is the committed order-document key, escrow key, proof lookup key, and marketplace identifier; `order_id_preimage_size` also feeds the placement hash fee. The `_v0` suffix names the current implementation but does not provide protocol dispatch, so a future preimage or derivation change cannot coexist safely with the generation that created historical order state. Add an order-ID generation selected from the platform-version tables and thread it through preimage sizing, fee calculation, Drive, proof, and event call sites.

…market-listings

v5.1-dev's YesNoVotePollNotAvailableForVotingError takes StateError
discriminant 176, so the marketplace's state errors follow at 177 to 183
(their codes are unchanged). The listings note is now v15 note 13, and the
generated decimals it names note 11.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

The marketplace implementation has strong coverage for escrow accounting, proof roots, fee metering, validation, and boundary behavior. Four in-scope blocking issues remain: consensus order arithmetic and order-ID derivation bypass the platform-version tables, and a mutable conventions update can change the economics of a seller's signed but not-yet-placed listing; the approval summary also omits the pricing scale. Mobile persistence additionally discards the authoritative Tradeable minimum fill. The CI snapshot contains no failed code checks.

🔴 4 blocking | 🟡 1 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Preserve Tradeable minFill in mobile token models
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257

The parser now accepts the externally tagged {"Tradeable":{"minFill":...}} form, but TokenTradeMode stores only the variant name and explicitly discards minFill. PersistentToken therefore cannot distinguish tradeable tokens with different minimum fills after restoration. Kotlin's TokenMaterializer and TokenEntity have the same lossy representation. Preserve the numeric minimum through both mobile parsers and persistence models, with a schema migration or compatible nullable field for existing records; the mobile layer should marshal the value rather than reimplement protocol validation.

source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality, security-auditor); gpt-6-astra (phase2-reviewer: architecture-layering, ffi-engineer)

4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-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: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This large, intricate diff changes consensus-critical token validation and state transitions that move tokens and credits through escrow, including protocol-version dispatch, storage operations, proofs, and cross-language serialization.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort xhigh); agent phase2-reviewer
  • Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257: Preserve Tradeable minFill in mobile token models
  The parser now accepts the externally tagged `{"Tradeable":{"minFill":...}}` form, but TokenTradeMode stores only the variant name and explicitly discards minFill. PersistentToken therefore cannot distinguish tradeable tokens with different minimum fills after restoration. Kotlin's TokenMaterializer and TokenEntity have the same lossy representation. Preserve the numeric minimum through both mobile parsers and persistence models, with a schema migration or compatible nullable field for existing records; the mobile layer should marshal the value rather than reimplement protocol validation.

In `packages/rs-dpp/src/tokens/token_order.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order.rs:84-141: Version token order pricing and fill arithmetic through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4372881399)
  TokenOrder::fill_amount, fill_credits, and order_value implement consensus-visible marketplace behavior: minimum-fill selection, decimal scaling, rounding direction, high-decimal fallback behavior, and the representable order-value bound. These helpers accept no PlatformVersion and are not selected by a DPP token-method table, even though they are called by validation and Drive transformation paths. A later correction to these rules would require changing the only implementation used when replaying PV15 state instead of selecting a new generation. Add a DPP token-order method version with an explicit v0 implementation and thread the active platform version through all callers.

In `packages/rs-dpp/src/tokens/token_order_side.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order_side.rs:68-92: Version the consensus order-ID derivation through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4371726120)
  TokenOrderSide::order_id directly calls Document::generate_document_id_v0 without a PlatformVersion argument or a DPP token-method version slot. Its output is the committed order-document key, escrow key, proof lookup key, and marketplace identifier. The `_v0` suffix documents the current algorithm but does not select it from the protocol version tables, so a future order-ID change cannot coexist with the PV15 derivation through normal version dispatch. Add an explicit order-ID generation slot and carry the selected generation through preimage sizing, fee derivation, Drive transformation, proof queries, and event construction.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs:95-121: Bind the pricing scale into the seller's signed placement
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4347888980)
  TokenPlaceAskTransitionV0 signs only amount and price, but the transformer reads the token's current conventions decimals after signature validation and stores that value in the order. A conventions controller distinct from the seller can change decimals while the signed placement is pending, causing the same seller signature to create a listing with different base-unit economics. For example, a price of 1,000,000,000 credits per whole token and 1,000 units represents 10,000 credits at 8 decimals but approximately 1 credit at 16 decimals. Recording decimals in the order protects only listings after placement; it does not protect the pending authorization. Carry the intended scale or an equivalent immutable configuration commitment in the signed transition, validate it against the token before escrow creation, and use that signed value in placement proof handling.

In `packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs:600-604: Include the signed pricing scale in listing approval summaries
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4352824559)
  The PlaceAsk approval row renders only `price: ... credits per whole token`, while the transition carries no scale and the transformer derives it from mutable token conventions at execution time. The FFI projection forwards this details string, so the mobile approval surface has no independent authoritative scale with which to interpret or distinguish otherwise identical listings. An approver can therefore approve a numeric price without seeing the term that determines its base-unit value. Once the scale is bound to the transition, render that signed value in this summary and cover listings with different scales; displaying mutable metadata alone would not solve the authorization problem.

QuantumExplorer and others added 2 commits October 30, 2026 13:13
v5.1-dev's consume note took 11, so the generated decimals note is 12,
the marketplace state 13 and the listings 14.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

The marketplace implementation still has a blocking authorization flaw: placement derives the pricing scale from mutable token conventions after the seller signs, so a conventions update can silently change the listing's economic meaning. The new order arithmetic and order-ID derivation also bypass the repository's required platform-version dispatch, while mobile persistence loses minFill and Kotlin accepts unknown trade-mode discriminators. CI shows the Kotlin SDK job queued; completed policy, book, explorer, and semantic checks passed.

🔴 4 blocking | 🟡 2 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Preserve Tradeable minFill in mobile token models
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257

The Swift parser reduces {"Tradeable":{"minFill":...}} to the bare Tradeable enum case, and PersistentToken stores only the raw mode string. The Kotlin materializer follows the same lossy representation. After reload, Tradeable tokens with different minimum fills are indistinguishable, so mobile consumers cannot display or reason about the rule that controls valid listings and fills. Preserve the exact minimum as an associated value or separate persisted field, with the corresponding persistence/schema handling.

source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality, security-auditor)

4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-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: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This is a large, intricate change to consensus-critical token state transitions that moves tokens and credits through escrow, changes protocol-version dispatch and serialization, and modifies persisted order storage and proof paths.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-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/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257: Preserve Tradeable minFill in mobile token models
  The Swift parser reduces `{"Tradeable":{"minFill":...}}` to the bare `Tradeable` enum case, and `PersistentToken` stores only the raw mode string. The Kotlin materializer follows the same lossy representation. After reload, `Tradeable` tokens with different minimum fills are indistinguishable, so mobile consumers cannot display or reason about the rule that controls valid listings and fills. Preserve the exact minimum as an associated value or separate persisted field, with the corresponding persistence/schema handling.

In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/tokens/TokenMaterializer.kt`:
- [SUGGESTION] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/tokens/TokenMaterializer.kt:211-220: Reject unknown trade-mode discriminators in the Kotlin parser
  `tradeMode` returns any bare string and any object's single key, rather than restricting the result to `NotTradeable` and `Tradeable`. The Kotlin token UI treats every value other than `NotTradeable` as tradeable, so malformed or future unsupported modes can be presented as an active marketplace mode. Parse only the known discriminators and return `null` for unknown values, matching Swift's fail-closed `TokenTradeMode(rawValue:)` behavior.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs:95-121: Bind the pricing scale into the seller's signed placement
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4347888980)
  `TokenPlaceAskTransitionV0` carries and signs only `amount` and `price`, while this transformer reads `base_action.token_configuration()?.conventions().decimals()` at placement time and stores that value in the order. A seller can sign a listing while the token has 8 decimals, then an authorized conventions controller can change it to 16 before inclusion; the same signed price is then interpreted at a different scale without invalidating the seller's nonce or signature. Persisting decimals protects an order only after placement and does not protect the pending authorization. Add the pricing scale or an equivalent configuration version to the signed transition, reject a mismatch before escrow creation, and verify the bound term in placement proofs.

In `packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs:600-604: Include the signed pricing scale in listing approval summaries
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4352824559)
  The approval summary renders only `price: ... credits per whole token`, but the current transition contains no signed scale and the placement transformer derives the scale from mutable token configuration. Consequently, two economically different pending listings can produce the same approval text, and the user cannot identify the scale whose change can reprice the listing before placement. Bind the scale to the Rust transition and render that authoritative signed value in the summary rather than reconstructing it from token metadata.

In `packages/rs-dpp/src/tokens/token_order.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order.rs:84-141: Version token order pricing and fill arithmetic through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4372881399)
  `fill_amount`, `fill_credits`, and `order_value` determine consensus-visible fill selection, rounding, credit transfers, and order admissibility, but are ordinary unversioned methods with no `PlatformVersion` parameter, dispatcher, or DPP token-method slot. A later change to rounding, decimal handling, or minimum-fill behavior would alter replay and accounting for an already-selected protocol generation. Add a frozen v0 implementation and dispatcher selected by the protocol-version tables, then thread the selected generation through validation and Drive call sites.

In `packages/rs-dpp/src/tokens/token_order_side.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order_side.rs:68-92: Version the consensus order-ID derivation through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4371726120)
  `TokenOrderSide::order_id` directly calls `Document::generate_document_id_v0` and has no `PlatformVersion` argument or token-method version slot. Its result is the committed order-document key, escrow key, and proof path, while `order_id_preimage_size` and the associated fee calculation depend on the same unversioned preimage. A future order-ID change would therefore have no protocol-selected generation for storage and proof compatibility. Add an explicit order-ID v0 dispatcher and version slot, and pass the selected generation through order-ID derivation, fee sizing, Drive, and proof call sites.

QuantumExplorer and others added 4 commits October 30, 2026 16:48
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eable

TokenMaterializer read any bare string or any tagged object's single key
as the trade mode, and the token screen treats every mode other than
NotTradeable as tradeable, so a malformed or newer mode showed as
tradeable. Only NotTradeable and Tradeable are accepted now; any other
name parses to null, which the materializer stores as NotTradeable, as
Swift's TokenTradeMode(rawValue:) refuses it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

The marketplace implementation has strong escrow, proof, fee, validation, and boundary-test coverage, and the Kotlin parser now fails closed for unknown trade modes. However, the current head still allows a conventions update to reprice a seller's signed pending listing, leaves the approval summary unable to show an authoritative pricing scale, and introduces consensus order arithmetic and order-ID derivation without platform-version dispatch. CI has no failed code checks in the snapshot, although compatible runner selection remains queued.

🔴 4 blocking | 🟡 1 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Preserve Tradeable minFill in mobile token models
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257

The Rust wire shape now carries Tradeable { minFill }, but the Swift model and parser collapse it to the bare Tradeable discriminator and explicitly discard minFill; the Kotlin materializer does the same. Mobile callers therefore cannot distinguish valid token configurations with different minimum fills or present the rule governing whether a partial fill will be accepted. Preserve the authoritative minimum in the mobile persistence models while keeping validation and fill decisions in Rust.

source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality, security-auditor)

4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This large, intricate diff changes consensus-critical token state transitions and escrowed token/credit movement across DPP, Drive, and rs-drive-abci, including validation, serialization, version gates, and proof execution paths.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-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/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257: Preserve Tradeable minFill in mobile token models
  The Rust wire shape now carries `Tradeable { minFill }`, but the Swift model and parser collapse it to the bare `Tradeable` discriminator and explicitly discard `minFill`; the Kotlin materializer does the same. Mobile callers therefore cannot distinguish valid token configurations with different minimum fills or present the rule governing whether a partial fill will be accepted. Preserve the authoritative minimum in the mobile persistence models while keeping validation and fill decisions in Rust.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs:98-120: Bind the pricing scale into the seller's signed placement
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4347888980)
  The transition signs only amount and price, while placement reads `conventions().decimals()` from mutable token configuration and stores that value in the order. A conventions controller can therefore change the scale after the seller signs but before inclusion: the same numeric price can represent materially different credits without invalidating the seller's signature or nonce. Persisting decimals protects fills after placement, but it does not protect this pending-placement window. Add the expected scale or an equivalent configuration commitment to the signed transition, reject a mismatch before escrow creation, and carry the authoritative value through proof and approval-summary handling.

In `packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs:600-604: Include the signed pricing scale in listing approval summaries
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4352824559)
  The listing summary introduced by this PR renders only `price: <n> credits per whole token`, while the transition carries no signed scale and placement resolves the scale from mutable token configuration. A user approving the request cannot distinguish economically different listings or know which scale the authorization will use if conventions change before inclusion. Once the scale is bound into the transition, render that same authoritative value in the Rust summary rather than expecting Swift or Kotlin callers to infer it from mutable contract state.

In `packages/rs-dpp/src/tokens/token_order.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order.rs:93-141: Version token order pricing and fill arithmetic through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4372881399)
  `fill_amount`, `fill_credits`, and `order_value` determine consensus-visible fill selection, rounding, credits transferred, and placement validity, but they accept no `PlatformVersion` and are not selected by a DPP token-method table. A later change to decimal handling, rounding, minimum-fill behavior, or overflow rules would otherwise require editing these consensus helpers in place and could not select the PV15 behavior during replay. Add a versioned token-order method slot and dispatcher, retain the current implementation as the PV15 generation, and thread the active platform version through all consensus callers.

In `packages/rs-dpp/src/tokens/token_order_side.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order_side.rs:74-92: Version the consensus order-ID derivation through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4371726120)
  `order_id` determines the committed order-document key, escrow key, proof path, and future marketplace event identifier, but it directly calls `Document::generate_document_id_v0` and its preimage-size helper has no active platform-version selector. A future change to the order-ID preimage or hash derivation could not coexist with replay of the current generation through the platform-version tables. Add an explicit v0 order-ID dispatcher and matching versioned preimage-size/fee path, then pass the selected generation through placement, proof, query, and event call sites.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

The marketplace implementation has addressed most previously reported fee, proof, validation, mobile parsing, and test-coverage issues, and the available relevant CI check succeeded. Five issues remain: two consensus-critical helpers bypass platform-version dispatch, placement derives its pricing scale from mutable token state after signing, the wallet approval summary omits that scale, and mobile persistence discards Tradeable.minFill.

🔴 4 blocking | 🟡 1 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Preserve Tradeable minFill in mobile token models
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257

The Rust contract representation now emits Tradeable as an object containing minFill, but the Swift model explicitly discards that value and persists only the discriminator. The Kotlin TokenEntity and TokenMaterializer make the same lossy projection. Consequently, mobile consumers cannot distinguish tokens with different minimum-fill rules, even though minFill affects whether marketplace fills are accepted. Persist the minimum in a UInt64-safe representation, with appropriate defaults and migration handling for older NotTradeable rows.

source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)

4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-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: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This large, intricate diff changes consensus-critical token state transitions and validation, escrowed token and credit movement, order storage and proofs, protocol-version dispatch, and cross-language serialization paths.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-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/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257: Preserve Tradeable minFill in mobile token models
  The Rust contract representation now emits Tradeable as an object containing minFill, but the Swift model explicitly discards that value and persists only the discriminator. The Kotlin TokenEntity and TokenMaterializer make the same lossy projection. Consequently, mobile consumers cannot distinguish tokens with different minimum-fill rules, even though minFill affects whether marketplace fills are accepted. Persist the minimum in a UInt64-safe representation, with appropriate defaults and migration handling for older NotTradeable rows.

In `packages/rs-dpp/src/tokens/token_order.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order.rs:84-141: Version token order pricing and fill arithmetic through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4372881399)
  TokenOrder::fill_amount, fill_credits, and order_value determine consensus-visible fill quantities, credit transfers, rounding, decimal overflow behavior, and listing admission. They take no PlatformVersion and have no corresponding DPP method-version slot, so a future protocol correction would require editing the only implementation in place or changing callers without an explicit generation boundary. The PV15 is_allowed gate does not solve this for later protocol versions. Add explicit versioned order-arithmetic implementations selected by the platform-version tables and thread the selected version through validation, transformation, proof, and operation callers.

In `packages/rs-dpp/src/tokens/token_order_side.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order_side.rs:74-92: Version the consensus order-ID derivation through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4371726120)
  TokenOrderSide::order_id directly derives the committed order-document and escrow key through Document::generate_document_id_v0, while order_id_preimage_size assumes the same fixed derivation. This identifier is used by placement, fills, proofs, and marketplace storage, so changing its preimage or hash domain in a later protocol generation would require changing consensus-visible key selection. Existing helpers that happen to pin v0 do not exempt this newly introduced marketplace key from the repository rule that consensus-critical behavior is selected through platform-version tables. Add an explicit order-ID generation slot and use the selected generation consistently for derivation, fee sizing, Drive lookups, and proof paths.

In `packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs:600-604: Include the signed pricing scale in listing approval summaries
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4352824559)
  The approval summary renders only the numeric price, while TokenPlaceAsk signs no pricing scale and the placement transformer obtains decimals from mutable token conventions at execution time. The displayed text therefore cannot communicate the denominator that gives the price its economic meaning, and otherwise identical seller approvals can be executed at materially different values after a conventions update. Bind the scale into the signed placement and render that exact signed value in the Rust summary and its FFI projections.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs:95-100: Bind the pricing scale into the seller's signed placement
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4347888980)
  TokenPlaceAskTransitionV0 signs only amount and price, but this transformer reads decimals from the current token conventions after the seller has signed. A conventions controller can update the token from, for example, 8 to 16 decimals before the seller's transition is included, changing the credit value represented by the same signed numeric price without changing the seller's nonce or signature. Persisting placement-time decimals protects only orders after creation; it does not protect the pending authorization. Carry the scale in the signed transition, validate it against the token configuration before escrow creation, and use the same value in the order and placement proof.

…laude/token-market-listings

Decision proposals' consensus errors reached v5.1-dev first, so they keep the
next discriminants (BasicError 208-209, StateError 177) and the token order
errors follow them (BasicError 210-212, StateError 178-184). The listings note
becomes v15 note 16, citing generatedFrom as note 14.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

The PV15 marketplace implementation has correct escrow, proof, fee, and activation-gate coverage, but five in-scope issues remain. The signed placement still derives its pricing scale from mutable token state, the approval summary omits that material term, and newly introduced order arithmetic and order-ID derivation bypass platform-version dispatch. The supplied CI snapshot has no failing checks, but it does not report the Rust marketplace checks described in the PR.

🔴 4 blocking | 🟡 1 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Preserve Tradeable minFill in mobile token models
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257

The parser recognizes the nested {"Tradeable":{"minFill":...}} form but reduces it to the raw Tradeable discriminator, and the persisted token model stores no minimum-fill value. After reload, mobile consumers cannot distinguish tokens with different consensus minimums, so they cannot accurately display marketplace rules or avoid constructing fills the platform will reject. Preserve minFill alongside the mode in the Swift and Kotlin persistence models, with the corresponding schema migration.

source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality)

4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This is a large, intricate change to consensus-critical token state transitions and escrowed funds movement across validation, execution, serialization, storage, and proof paths, including protocol version 15 activation.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-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/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257: Preserve Tradeable minFill in mobile token models
  The parser recognizes the nested `{"Tradeable":{"minFill":...}}` form but reduces it to the raw `Tradeable` discriminator, and the persisted token model stores no minimum-fill value. After reload, mobile consumers cannot distinguish tokens with different consensus minimums, so they cannot accurately display marketplace rules or avoid constructing fills the platform will reject. Preserve `minFill` alongside the mode in the Swift and Kotlin persistence models, with the corresponding schema migration.

In `packages/rs-dpp/src/tokens/token_order.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order.rs:84-141: Version token order pricing and fill arithmetic through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4372881399)
  `fill_amount`, `fill_credits`, and `order_value` determine consensus-visible fill selection, rounding, overflow behavior, and the order-value bound, but they take no `PlatformVersion` and have no token-method version slot. They are called by the marketplace transformation and validation paths, so editing this implementation later would change the behavior of already-open PV15 orders for every protocol generation rather than allowing a later generation to select new arithmetic. Add an explicit versioned v0 implementation and dispatch it through the DPP token version tables, threading the active platform version through all callers.

In `packages/rs-dpp/src/tokens/token_order_side.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order_side.rs:74-92: Version the consensus order-ID derivation through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4371726120)
  `TokenOrderSide::order_id` directly calls `Document::generate_document_id_v0`, and `order_id_preimage_size` hard-codes the matching preimage without a `PlatformVersion` or token-method version slot. This identifier is the committed order-document key, escrow key, proof path, and future marketplace-event identity. A future change to the preimage or derivation cannot preserve the existing order-key behavior through a protocol-version boundary. Add an explicit versioned order-ID dispatcher and thread it through preimage sizing, fee calculation, placement, lookup, proof, and event call sites.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs:95-100: Bind the pricing scale into the seller's signed placement
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4347888980)
  `TokenPlaceAskTransitionV0` signs only `amount` and `price`, while the transformer reads `conventions().decimals()` from mutable token state immediately before constructing the order. A conventions controller can change a token from 8 to 16 decimals after a seller signs but before inclusion; the same signed numeric price then represents materially different economics, and the resulting escrowed listing executes under terms the seller did not authorize. Carry the intended scale or configuration version in the canonical signed transition, reject a mismatch before escrow creation, and use that same signed value in the order and placement proof.

In `packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/state_transition_summary.rs:600-604: Include the signed pricing scale in listing approval summaries
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4352824559)
  The PlaceAsk approval row currently renders only `price: <value> credits per whole token`. Because the transition has no signed scale and placement resolves decimals from mutable state, materially different pricing interpretations can produce the same approval details. Once the placement fix binds the scale to the signed transition, render that exact signed scale in this summary rather than reconstructing it from token metadata; otherwise the wallet cannot show the user the economic term being authorized.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

Verified the four distinct reviewer findings and revalidated all 31 prior findings against head 26337eb. Three blockers remain: execution-time configuration can alter a pending seller authorization's proceeds, and the shared order arithmetic and order-ID derivation lack the required table-selected version boundaries; mobile models also discard minFill. This was static-only verification: all currently reported CI checks pass, including Kotlin build/tests, but the check list provides no Rust/WASM build or test results.

🔴 3 blocking | 🟡 1 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Preserve Tradeable minFill in mobile token models
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257

DPP now emits Tradeable with a minFill payload, but Swift's DataContractParser and PersistentToken retain only the mode discriminator; Kotlin's TokenMaterializer and TokenEntity make the same projection. Consequently, configurations with minFill 1 and minFill 1000 become indistinguishable through the structured persisted token API, although they impose different placement and fill requirements. Raw contract JSON remains available, but consumers must reparse it to recover this new configuration value. Preserve the authoritative minimum alongside the mode using lossless integer handling and additive persistence changes where required. Cover numeric and large decimal-string JSON forms; leave protocol validation and fill decisions in Rust rather than duplicating them in mobile code.

source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality)

3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The large, intricate diff changes consensus validation and funds movement, notably in token_fill_ask_transition_action/v0/transformer.rs and action_convert_to_operations/batch/token/token_fill_ask_transition.rs, which calculate fills and settle escrowed tokens against buyer and seller credits.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-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/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257: Preserve Tradeable minFill in mobile token models
  DPP now emits Tradeable with a minFill payload, but Swift's DataContractParser and PersistentToken retain only the mode discriminator; Kotlin's TokenMaterializer and TokenEntity make the same projection. Consequently, configurations with minFill 1 and minFill 1000 become indistinguishable through the structured persisted token API, although they impose different placement and fill requirements. Raw contract JSON remains available, but consumers must reparse it to recover this new configuration value. Preserve the authoritative minimum alongside the mode using lossless integer handling and additive persistence changes where required. Cover numeric and large decimal-string JSON forms; leave protocol validation and fill decisions in Rust rather than duplicating them in mobile code.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs:98-99: Bind the pricing scale into the seller's signed placement
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4347888980)
  TokenPlaceAskTransitionV0 signs amount and price but no pricing scale, configuration commitment, or minimum proceeds. This lookup supplies the denominator from execution-time conventions. For a token whose conventions controller is distinct from the holder, that controller can update decimals from 8 to 16 before the holder's pending placement executes: 1,000 base units at a signed price of 1,000,000,000 credits per whole token then sell for 1 credit instead of 10,000. Both scales are valid, the controller's update does not consume the holder's nonce, and the placement's minimum-fill and value checks accept these terms. The controller can buy the listing and restore the conventions. The generatedFrom property and stored decimals authenticate and preserve the scale chosen at placement, not the proceeds the holder intended to authorize. The signed-decimals field, agreement check, and distinct-controller regression described in your earlier reply are absent at this head. Restore a signed expected scale or equivalent minimum-proceeds protection, enforce it before escrow creation, propagate it through proof comparison and bindings, and cover a conventions update occurring after signing but before placement.

In `packages/rs-dpp/src/tokens/token_order.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order.rs:118-137: Version token order pricing and fill arithmetic through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4372881399)
  fill_amount, fill_credits, and order_value select consensus-visible matching, rounding, payment, and placement-validity rules without accepting PlatformVersion or selecting a generation. Drive's fill transformer and DPP's contextual order-terms validator call these shared implementations directly; DPPTokenVersions contains no selectors for them. The book's versioned-behaviour rules require table-selected methods for shared behavioral helpers, including an explicit initial implementation, rather than relying on callers to preserve an unversioned implementation. The PV15 activation gate establishes that this is not a present pre-PV15 replay regression, but it does not satisfy that architectural boundary. Add frozen v0 implementations and versioned entry points, thread the active PlatformVersion through callers, and select them from a PV15-owned token-version snapshot while leaving the snapshot used by PV14 unchanged.

In `packages/rs-dpp/src/tokens/token_order_side.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order_side.rs:74-92: Version the consensus order-ID derivation through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4371726120)
  The direct generate_document_id_v0 call pins the document hash routine, but the marketplace-specific construction here still independently chooses the token ID, maker, side name, and nonce encoding, with an unversioned companion determining the charged preimage size. These choices supply committed document and escrow keys, execution-proof query paths, verifier expectations, and event identifiers. The existing historical-ID examples establish present determinism, not an exception to the book's shared-helper versioning rule. Add an order-ID selector under DPPTokenVersions and an explicit v0 recipe, keeping derivation and preimage sizing tied to that selection. A change limited to the placement transformers would be incomplete: state_transition_execution_path_queries, placement-proof verification, and TokenTransition event construction also call order_id independently and need the same active version. This is a required versioning boundary, not a claim that current nodes calculate different IDs.

…to claude/token-market-listings

Private document types' consensus errors reached v5.1-dev first, so
DocumentTypePrivateError keeps BasicError 210 and
ReferencedDocumentTypePrivateError StateError 178; the token order errors
follow (BasicError 211-213, StateError 179-185). The listings note becomes
v15 note 17, citing generatedFrom as note 15.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

Verified the findings and all 31 prior finding identities against head 7fb2733. Three blockers remain: pending listings lack seller-authorized pricing-scale protection, and the new order arithmetic and ID derivation lack the required versioned-method boundaries. This was static verification only; current reported CI checks pass, but they provide no Rust marketplace or Rust/WASM build-and-test results.

🔴 3 blocking

3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This large, intricate diff directly introduces consensus-validated funds movement through escrow and atomic token-for-credit settlement in packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/token/token_fill_ask_transition.rs and related placement and cancellation operations.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-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/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs:98-99: Bind the pricing scale into the seller's signed placement
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4347888980)
  The earlier signature-bound fix is absent from this head: TokenPlaceAskTransitionV0 contains only base, amount and price, and advanced structure performs no signed-scale agreement check. A conventions controller distinct from the holder can change the denominator of an already-signed sale without consuming the holder's nonce. For example, 1,000 base units at a signed price of 1,000,000,000 credits per whole token yield 10,000 credits at 8 decimals, but only 1 credit if the controller changes decimals to 16 before placement. Both pass the current amount, price, minimum-fill and total-value bounds. The controller can then buy the escrow and restore the conventions. The documented execution-time semantics and generatedFrom property freeze the scale after placement; neither establishes the seller's authorization for that scale. Restore a signed expected scale or equivalent minimum-proceeds commitment in DPP, enforce it before escrow creation, and propagate it through constructors, approval summaries and placement-proof checks. Add a processor regression with distinct controller and holder identities and a conventions update between signing and submission.

In `packages/rs-dpp/src/tokens/token_order.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order.rs:93-112: Version token order pricing and fill arithmetic through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4372881399)
  fill_amount, fill_credits and order_value implement consensus policy: they select accepted fill quantities, credit transfers and placement refusals. None accepts PlatformVersion or has a DPP token-method selector, and the rounding and high-decimal behavior lives in shared unversioned functions. The PV15 activation gate makes these calculations unreachable for earlier protocols, but it does not supply the required versioned-method boundary for this new consensus behavior. This is a repository versioning-rule violation, not evidence that identical current binaries disagree. Put the coherent arithmetic in an explicit frozen v0 selected through DPPTokenVersions, and pass the active platform_version through validation and transformation callers. Include dispatch and unsupported-version coverage so future arithmetic generations can coexist with PV15 replay.

In `packages/rs-dpp/src/tokens/token_order_side.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order_side.rs:74-85: Version the consensus order-ID derivation through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4371726120)
  generate_document_id_v0 freezes the inner document hash recipe, but this new helper independently owns the marketplace preimage policy: token/maker inputs, side domain and nonce encoding. That policy determines committed document and escrow keys and is shared by placement, proof-query, proof-verification and event paths without a generation or table selector. The cited legacy ID helpers demonstrate deterministic precedent, but they do not exempt a newly introduced consensus method from the project's versioned-dispatch requirement. Add an order-ID method version under DPPTokenVersions with an explicit frozen v0 and unsupported-version handling. Use the same selection for preimage sizing and metering, and thread platform_version through all ID consumers rather than only the placement transformers. This is a version-ownership blocker, not a claim of a present hash collision or cross-node disagreement.
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.

  • Cover the signed-width boundaries for order amount and price — FIXED: token_place_ask_transition/validate_structure/mod.rs contains should_refuse_an_amount_or_price_outside_a_signed_8_byte_value, exercising 0, 1, i64::MAX and i64::MAX + 1 independently for both fields through the versioned validator and matching the specific errors. The processor suite also checks unpaid rejection of an oversized amount and unchanged state.
    • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

The marketplace execution paths correctly preserve order-time decimals after placement, account for hashing and document reads, and include substantial regression coverage. However, the current placement still derives its pricing scale from mutable conventions before escrow creation, allowing a controller to reprice another holder's pending signed listing; the new consensus arithmetic and order-ID helpers also lack the required PlatformVersion-selected generations. Mobile projections additionally discard the authoritative minFill payload, and no Rust build or test result is present in the supplied CI snapshot.

🔴 3 blocking | 🟡 1 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Preserve Tradeable minFill in mobile token models
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257

The Swift parser and Kotlin materializer recognize the new tagged Tradeable representation but retain only the discriminator, explicitly discarding minFill. Consequently, tokens with minimum fills of 1 and 1,000 are indistinguishable through the typed persisted token projections, even though the Rust rule changes listing and fill validity. The full serialized contract remains available, so this is not a consensus or data-loss blocker, but mobile consumers cannot expose or use the authoritative minimum through the typed model without reparsing raw JSON. Preserve the canonical unsigned value, including the decimal-string representation for values beyond JavaScript-safe numeric ranges, while keeping validation and fill decisions in Rust.

source: muse-spark-1.3-contributor (phase1-reviewer: rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality); gpt-6-astra (phase2-reviewer: general, architecture-layering, ffi-engineer)

3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 10: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This large, cross-layer change directly adds consensus validation and atomic token/credit movement in packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/token/token_fill_ask_transition.rs and the marketplace transition validators in rs-drive-abci.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort xhigh); agent phase2-reviewer
  • Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Types/TokenTypes.swift:247-257: Preserve Tradeable minFill in mobile token models
  The Swift parser and Kotlin materializer recognize the new tagged Tradeable representation but retain only the discriminator, explicitly discarding minFill. Consequently, tokens with minimum fills of 1 and 1,000 are indistinguishable through the typed persisted token projections, even though the Rust rule changes listing and fill validity. The full serialized contract remains available, so this is not a consensus or data-loss blocker, but mobile consumers cannot expose or use the authoritative minimum through the typed model without reparsing raw JSON. Preserve the canonical unsigned value, including the decimal-string representation for values beyond JavaScript-safe numeric ranges, while keeping validation and fill decisions in Rust.

In `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs:98-99: Bind the pricing scale into the seller's signed placement
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4347888980)
  The placement transition signs only amount and price, while these lines obtain the pricing denominator from mutable token conventions at execution time. A seller can sign 1,000 base units at 1,000,000,000 credits per whole token while the token has 8 decimals, then a distinct conventions controller can change the token to 16 decimals before placement; the same signed bytes will create a listing worth only 1 credit instead of 10,000. Persisting the execution-time decimals protects later fills but does not protect the seller's pending authorization. Bind a seller-authorized scale, configuration commitment, or minimum proceeds to the unreleased placement generation and reject an incompatible placement before escrow creation. The regression must use distinct seller and conventions-controller identities and update conventions between signing and placement.

In `packages/rs-dpp/src/tokens/token_order.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order.rs:93-143: Version token order pricing and fill arithmetic through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4372881399)
  fill_amount, fill_credits, and order_value directly determine accepted fill quantities, transferred credits, and whether a listing is admissible, but they remain shared implementations with no PlatformVersion parameter or DPPTokenVersions selector. Their rounding, minimum-fill adjustment, cumulative-total behavior, and >=39-decimal handling are therefore outside the required table-selected consensus boundary. Versioning the surrounding transition or transformer does not freeze these shared bodies for future protocol generations. Add explicit v0 implementations selected through DPP token-method tables, thread the active PlatformVersion through placement validation and fill transformation, and exercise the public dispatcher in tests.

In `packages/rs-dpp/src/tokens/token_order_side.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order_side.rs:74-92: Version the consensus order-ID derivation through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4371726120)
  TokenOrderSide::order_id directly assembles the marketplace-specific preimage and calls Document::generate_document_id_v0 without accepting PlatformVersion or consulting a DPP token-method slot. The document-ID suffix freezes only the underlying hash primitive; it does not version the order-specific side name, token/maker inputs, nonce encoding, or the matching preimage-size calculation. This helper determines committed order and escrow keys and is reused by placement, execution-path queries, proof verification, and event construction. Add an order-ID generation slot with an explicit v0 implementation and keep derivation, metering, proof paths, and event callers on the same selected generation.
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.

  • Cover the signed-width boundaries for order amount and price — REFUTED: the current DPP source contains should_refuse_an_amount_or_price_outside_a_signed_8_byte_value in packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_place_ask_transition/validate_structure/mod.rs:64-96. That table exercises 0, 1, i64::MAX, and i64::MAX + 1 independently for amount and price and matches the specific InvalidTokenAmountError and InvalidTokenOrderPriceError variants; the processor-path amount overflow regression is additional coverage.
    • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

Verified the combined reviewer claims and all 31 prior findings against b92488f. Three blockers remain: mutable conventions can reprice a pending seller authorization, and the shared order arithmetic and identifier derivation lack the required version-table dispatch. This was static verification only; current PR checks pass, but they provide no Rust build or test results.

🔴 3 blocking

3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The large, intricate diff directly changes consensus validation and funds movement through token_fill_ask_transition_action/state_v0/mod.rs and token_fill_ask_transition_action/v0/transformer.rs, implementing escrow settlement and buyer-to-seller credit transfers.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-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/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_place_ask_transition_action/v0/transformer.rs:98-99: Bind the pricing scale into the seller's signed placement
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4347888980)
  TokenPlaceAskTransitionV0 signs amount and price but no pricing scale or minimum proceeds; this lookup supplies the denominator from mutable conventions. A holder can sign 1,000 base units at 1,000,000,000 credits per whole token with 8 decimals, expecting 10,000 credits. A distinct conventions controller can update the token to 16 decimals before that placement executes, making the unchanged listing cost only 1 credit, fill it, and restore the conventions. Configuration updates use the controller's identity-contract nonce, so they do not invalidate the holder's pending authorization. The generatedFrom field records the scale selected at execution and protects subsequent fills, but it does not constrain that initial selection. Restore a seller-signed expected scale, configuration commitment, or minimum-proceeds bound; enforce it before escrow creation and propagate the commitment through placement proofs and approval summaries. Restore the processor regression with the holder distinct from the conventions controller.

In `packages/rs-dpp/src/tokens/token_order.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order.rs:93-118: Version token order pricing and fill arithmetic through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4372881399)
  fill_amount, fill_credits, and order_value implement consensus behavior: accepted fill quantities, transferred credits, rounding, and the placement-value bound. They accept no PlatformVersion and have no selectors in DPPTokenVersions. Their versioned callers therefore select only the surrounding validation or transformation, not these shared algorithms. The book's versioned-behavior rules require shared behavior to have its own table-selected implementation rather than relying on each caller to preserve an ordinary public helper. The PV15 activation gate prevents a historical marketplace regression today, but does not satisfy that dispatch requirement. Add explicit v0 implementations selected through the token-version tables, thread the active version through the terms validator and fill transformer, and cover dispatch rejection alongside the existing arithmetic boundary tests.

In `packages/rs-dpp/src/tokens/token_order_side.rs`:
- [BLOCKING] packages/rs-dpp/src/tokens/token_order_side.rs:74-92: Version the consensus order-ID derivation through platform-version tables
  (existing thread: https://github.com/dashpay/platform/pull/5529#discussion_r4371726120)
  Document::generate_document_id_v0 pins the inner hash implementation, but order_id still defines the marketplace-specific token/maker/side/nonce preimage without a version-table selector. order_id_preimage_size separately defines its metered size. This shared derivation determines committed document and escrow keys and is called by placement, execution-proof queries, the verifier body shared by verifier generations 0 and 1, and event construction. The book's shared-helper rule requires a table-selected generation for this behavior; older unversioned identifier helpers do not establish an exception for new code. Current nodes compute the same identifier, so this is a protocol-versioning violation rather than demonstrated present nondeterminism. Add a DPP token-method selector and explicit v0 implementation, and route derivation and matching fee sizing through it at every producer and verifier call site—not only the placement transformers.
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.

  • Remove the unused order-decimals mismatch error — The variant is unused outside registration, bindings, and tests, but it is appended and does not change existing consensus-error discriminants or codes. No cited architecture rule requires an appended public error to have an immediate producer. Removing it is optional cleanup, not a separate correctness issue; restoring the missing signed-scale validation identified by the authorization blocker can also give this error its intended producer.
    • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants