feat(sdk-go): port Go SDK to developer-platform - #43
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Nostradamus Risk Rating — HighThe PR introduces High/Critical — a full threat model follows. |
Change Control Evidence CheckAuthorization — ✅ PassEvidence:
Testing — ✅ PassEvidence:
Approval — ✅ PassEvidence:
Segregation of Duties — ✅ PassValidated:
Last checked: 2026-08-29 20:05 UTC ↩ Re-run Clarissa if tickets, CI, or approvals have changed. |
privateWS was constructed with cfg.configErr instead of the derived privateConfigErr, so PrivateWebSocket() would dial with no auth when no auth strategy was configured, unlike the private REST client which already fails closed with ErrAuthenticationRequired.
alignScale (used by Cmp/Add/Sub/Equal/Min/Max) exponentiated an unbounded scale difference via bigExp10, unlike Div which already caps at maxDecimalOperationDigits. A single price/amount field parsed with an extreme scale could force a multi-million-bit big.Int computation on every comparison. Cap maxDecimalDigits/maxDecimalScale at the same 1,000-digit bound Div already treats as safe, so no parsed Decimal can produce a scale difference arithmetic isn't bounded to handle cheaply.
|
Nostradamus Threat Model - Passed - APPSEC-1561 |
CalculateLiquidationPrice floored the negative-factor case for long positions but not shorts, so an extreme maintenance margin rate (e.g. leverage=1, mmr=3.0) could return a negative liquidation price with a nil error. CalculateFee also silently zeroed any negative feeBps instead of computing the maker rebate it represents.
Validate() only checked for a nil TokenSource interface, so a non-nil interface wrapping a nil TokenFunc (e.g. an unset auth.TokenFunc var) passed validation and panicked on the first real request instead of failing fast at construction.
A code review flagged Send() releasing c.mu before WriteMessage as a race against reconnect's conn swap/close. On investigation this is not a bug: the Conn interface documents Close as safe to call concurrently with an in-flight WriteMessage, gorilla/websocket guarantees it, and holding c.mu for the write's duration was tried and reverted because it deadlocks Close/state-transition paths against a slow or blocked write (TestSubscriptionCleanupDoesNotWaitForBlockedWireRequest exists specifically to guard against that). No behavior change; adds a comment explaining why the lock is released early.
Sync() matched resting orders to target quotes by iterating the r.resting map directly. Go's randomized map iteration order meant that when two resting orders shared side/price/amount (e.g. after a partial fill), which one got kept vs. cancelled-and-replaced was nondeterministic, silently breaking the documented FIFO queue-priority guarantee. Sort resting orders ascending by their exchange-assigned OrderID (a monotonically increasing uint64) before matching, so the oldest queued order is always preferred.
encoding/json decodes JSON numbers into interface{} as float64, so an
order ID round-tripped through generic JSON decoding elsewhere in a
caller's application had no matching case and was always rejected
with "order ID is required".
net/http treats an empty Request.Method as GET on the wire (e.g. a
*http.Request built via struct literal, bypassing http.NewRequest's
normalization), but IsSafeMethod("") returned false, so transient
network errors on such a request were never retried even though the
request is a safe, idempotent GET.
APIKey.String() revealed a fixed 4-char prefix and 4-char suffix for any key longer than 8 characters, so a 10-character key showed 8 of its 10 characters (80%) in logs/telemetry. Only reveal the prefix+suffix fingerprint once the key is long enough that it's a minority of the key (len > 20, so at most ~40% revealed); shorter keys are now fully redacted like before.
fuller
changed the title
ximt
left a comment
ximt
left a comment
There was a problem hiding this comment.
Round 1 review: four Important correctness/security blockers found. Tests and race checks pass; please address the inline findings before merge.
ximt
left a comment
ximt
left a comment
There was a problem hiding this comment.
Round 2 re-review: the four prior blockers are fixed, but three new Important correctness and compatibility regressions remain.
|
@ximt Could you please re-review PR #43 at commit 001d0af? The three Round 2 findings are addressed: caller-supplied public request bodies are preserved, since_tid is exact int64, and AnonymousSchema_152 remains as a deprecated compatibility alias. Targeted and full Go/TypeScript validation passed, and there are zero unresolved inline threads. |
ximt
left a comment
ximt
left a comment
There was a problem hiding this comment.
Round 3 re-review: prior correctness and compatibility findings are fixed; one Important forward-regeneration issue remains.
ximt
left a comment
ximt
left a comment
There was a problem hiding this comment.
Round 4 re-review: generator logic is now dynamic, but one Important compatibility-test assertion still hard-codes the old anonymous enum name.
ximt
left a comment
ximt
left a comment
There was a problem hiding this comment.
Round 5 re-review: the compatibility-test suffix coupling is fixed; no Critical or Important findings remain.
3 participants
BUI-157
Why
Port the Go SDK from
api-docsintodeveloper-platformusing the deployed API contracts, so Go consumers have a supported, publishable REST and WebSocket SDK from the platform repository.What changed
oapi-codegenv2.7.1.Validation
go test ./...go test -race ./...go vet ./...make generate-checkmake release-smokego mod verifyacross all modulesgovulncheck: no vulnerabilities reachable from SDK/runtime/demo/Gorilla code; generator-tooling output contains only dependency-only/unreachable advisoriesgosec: zero findingsstaticcheck: zero findingsnpm run typecheckandnpm testLive OAuth integration tests were not run because they require credentials. Linux/386 compilation is covered by CI; the local Darwin environment does not provide a usable Linux/386 standard-library toolchain.