|
| 1 | +## Task |
| 2 | + |
| 3 | +Review this pull request thoroughly. Cover: |
| 4 | + |
| 5 | +### Correctness & Logic |
| 6 | + |
| 7 | +- Control flow, edge cases, and error propagation across both `advanced-wallet-manager` |
| 8 | + and `master-express` modes |
| 9 | +- mTLS certificate loading, validation, and fingerprint-checking paths |
| 10 | +- Key generation and signing correctness — no silent failures or swallowed errors |
| 11 | + |
| 12 | +### Cryptographic & Key Management Safety |
| 13 | + |
| 14 | +- **Never approve code that logs, serializes, or transmits private key material, |
| 15 | + mnemonics, or key shares in plaintext** — flag as Critical |
| 16 | +- MPC/DKG/DSG round sequencing: ensure protocol state machines advance in the correct |
| 17 | + order and cannot be replayed or skipped |
| 18 | +- EdDSA and ECDSA signing paths: verify that the correct key share type is used and |
| 19 | + that partial signatures are assembled correctly |
| 20 | +- Recovery paths: confirm that recovery operations use backup key material correctly |
| 21 | + and do not weaken the key custody model |
| 22 | + |
| 23 | +### Resiliency & External I/O |
| 24 | + |
| 25 | +- Key-provider client calls are treated as unreliable: timeouts, retries, and response |
| 26 | + validation must be present |
| 27 | +- HTTP/mTLS connections to the Advanced Wallet Manager from Master Express: verify |
| 28 | + error handling for network failures and unexpected status codes |
| 29 | +- No floats for coin amounts — all monetary/crypto math must use BigNumber, BigInt, |
| 30 | + or string arithmetic |
| 31 | + |
| 32 | +### Security |
| 33 | + |
| 34 | +- Input validation on all route handlers (`ValidationError` for bad input, not 500s) |
| 35 | +- No injection vectors (path traversal, command injection, prototype pollution) |
| 36 | +- Certificate paths and environment variables sourced only from trusted config, not |
| 37 | + request data |
| 38 | +- Secrets and credentials are never written to logs |
| 39 | + |
| 40 | +### API & Contract |
| 41 | + |
| 42 | +- Public endpoint behavior changes are backwards-compatible or explicitly documented |
| 43 | + as breaking |
| 44 | +- Response shapes conform to the `error` / `details` error format |
| 45 | +- Route params (`:coin`, `:walletId`, `:txRequestId`, `:shareType`) are validated |
| 46 | + before use |
| 47 | + |
| 48 | +### Test Coverage |
| 49 | + |
| 50 | +- Changed signing/key-management paths have unit tests covering happy path, error |
| 51 | + paths, and protocol edge cases |
| 52 | +- mTLS config changes are covered by integration-test scenarios where applicable |
| 53 | + |
| 54 | +### General |
| 55 | + |
| 56 | +- TypeScript strictness — no unsafe `any` casts or missing null checks in hot paths |
| 57 | +- Logging is meaningful and does not leak sensitive payload data |
| 58 | + |
| 59 | +## Context |
| 60 | + |
| 61 | +- Repository: ${{ github.repository }} |
| 62 | +- PR/Issue: ${{ github.event.issue.number || github.event.pull_request.number }} |
| 63 | +- This is a cryptographic signing server. Treat correctness and key-safety findings |
| 64 | + with maximum scrutiny — a bug here can result in loss of customer funds. |
0 commit comments