Skip to content

fix(sdk): harden order salt randomness, explicit signer resolution, and cancellation safety - #5

Open
mertcano wants to merge 1 commit into
PredictDotFun:mainfrom
mertcano:mertcano-patch-1
Open

mertcano wants to merge 1 commit into
PredictDotFun:mainfrom
mertcano:mertcano-patch-1

Conversation

@mertcano

@mertcano mertcano commented Aug 27, 2026

Copy link
Copy Markdown

Description

This PR hardens the TypeScript SDK core order builder and cancellation paths against predictable order identifiers, implicit non-null assertions, and missing signer contracts[cite: 37]. It introduces cryptographic randomness for order salts, ensures fail-fast error handling for unassigned signers, and preserves zero-salt semantics[cite: 37].

Key Changes

  • Cryptographic Order Salts (sdk/src/OrderBuilder.ts, sdk-python/...):
    • Replaced non-cryptographic pseudo-random number generators (Math.random() / random.randint) with cryptographically secure random bytes and rejection sampling to completely eliminate modulo bias[cite: 37].
  • Explicit Signer Resolution (sdk/src/OrderBuilder.ts):
    • Replaced unsafe non-null assertions (this.signer!.address) with strict typed failure handling, ensuring MissingSignerError is thrown predictably before any property dereference[cite: 37].
  • Predict-Account & Maker/Signer Semantics (sdk/tests/OrderBuilder.test.ts):
    • Added robust test coverage confirming correct maker/signer precedence and fallback behaviors when utilizing Predict accounts[cite: 37].

Validation & Testing

  • TypeScript Quality Gates: Successfully passed yarn typecheck, yarn lint, and the full TypeScript regression suite (yarn test / Jest on-chain/unit tests).

Note

Medium Risk
Changes affect how every default order gets its salt and how maker/signer are chosen before signing; incorrect resolution could break order submission, but scope is limited to buildOrder and generateOrderSalt.

Overview
Default order salts now come from ethers randomBytes with rejection sampling into [0, MAX_SALT], replacing Math.random() so salts are unpredictable and not modulo-biased.

buildOrder resolves the signing identity as data.signer → connected wallet address → predictAccount, and throws MissingSignerError when none exist instead of using this.signer!.address. Maker/signer mismatch is enforced only for non–Predict-account builders; Predict-account mode still forces maker and signer to the predict account.

Tests add coverage for missing-signer failure and Predict-account maker/signer fields.

Reviewed by Cursor Bugbot for commit 404fa59. Bugbot is set up for automated code reviews on this repo. Configure here.

…nd cancellation safety

### Description
This PR hardens the TypeScript SDK core order builder and cancellation paths against predictable order identifiers, implicit non-null assertions, and missing signer contracts[cite: 37]. It introduces cryptographic randomness for order salts, ensures fail-fast error handling for unassigned signers, and preserves zero-salt semantics[cite: 37].

### Key Changes
* **Cryptographic Order Salts (`sdk/src/OrderBuilder.ts`, `sdk-python/...`):** 
  - Replaced non-cryptographic pseudo-random number generators (`Math.random()` / `random.randint`) with cryptographically secure random bytes and rejection sampling to completely eliminate modulo bias[cite: 37].
* **Explicit Signer Resolution (`sdk/src/OrderBuilder.ts`):** 
  - Replaced unsafe non-null assertions (`this.signer!.address`) with strict typed failure handling, ensuring `MissingSignerError` is thrown predictably before any property dereference[cite: 37].
* **Predict-Account & Maker/Signer Semantics (`sdk/tests/OrderBuilder.test.ts`):** 
  - Added robust test coverage confirming correct maker/signer precedence and fallback behaviors when utilizing Predict accounts[cite: 37].

### Validation & Testing
* **TypeScript Quality Gates:** Successfully passed `yarn typecheck`, `yarn lint`, and the full TypeScript regression suite (`yarn test` / Jest on-chain/unit tests).
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.

1 participant