feat: add WithSigner for external/remote signing (KMS/HSM)#18
Open
developerAkX wants to merge 6 commits into
Open
feat: add WithSigner for external/remote signing (KMS/HSM)#18developerAkX wants to merge 6 commits into
developerAkX wants to merge 6 commits into
Conversation
Add a Signer callback option so callers can sign Hyperliquid actions without giving the SDK a raw private key. Adds the `Signer func(hashHex string) (*Signature, error)` type and `WithSigner` / `WithSignerAddress` options. When a signer is set, New() skips the PRIVATE_KEY env fallback, in-process wallet creation, and builder-fee auto-approval; requireWallet() is satisfied by a wallet OR a signer; buildSignSend() signs via the callback. The WithPrivateKey path is unchanged when no signer is set. Enables KMS/HSM/remote-signer setups where the key must never enter the SDK process. Adds package tests and a README section.
Author
|
For consistency's sake, I can add this feature in other languages too, like TypeScript, Rust, and Python. |
Port the Go external-signer seam to Python, Rust, and TypeScript with identical semantics. Add a dedicated SIGNER_FAILED/SignerError (distinct from venue SignatureError) with the underlying cause chained, and bound each signer call with a deadline/cancellation (context / tokio timeout / AbortSignal). Builder-fee auto-approval is skipped under a signer in all SDKs. Also add a per-order slippage helper to Go PlaceOrder. Tests and README docs for each SDK.
Contributor
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
… HTTP pipeline The previous commit wrapped the whole build→sign→send pipeline in a single context.WithTimeout, which unintentionally changed the wallet-only path: the build and send HTTP calls used to each get a fresh per-request budget (http.Client.Timeout) and now shared one deadline. Restore context.Background() for the HTTP calls and scope the Timeout-derived deadline to the external signer call only. This makes the wallet path byte-for-byte unchanged again and matches how the Rust (tokio::time::timeout) and TypeScript (AbortSignal) SDKs bound only the signer call. Update the now-inaccurate doc comments.
Author
|
bugbot run |
A user-supplied signer can return a nil/None/malformed signature without
erroring; the SDK then embedded it into the send payload, producing a
confusing venue-side rejection instead of a clear SDK-level error. Validate
the returned signature (non-nil, non-empty r/s, v in {27,28}) right after the
signer call in Go, Python, and TypeScript, raising SignerError (SIGNER_FAILED)
on failure so it never reaches the venue. Rust is already safe via its type
system (sign_hash returns Result<Signature> with a concrete Signature). Adds
tests for nil and malformed returns in all three dynamic SDKs.
Author
|
bugbot run |
The Go/Python/TS SDKs validate the signature returned by an external signer
(v in {27,28}, non-empty r/s), but Rust only relied on its type system, which
guarantees non-nil but not a valid value: an external HyperliquidSigner could
return v=0/1 (raw recovery id) or zero r/s and have it sent straight to the
venue as a confusing rejection. Add validate_signer_signature and apply it on
the external-signer path only (LocalSigner always produces a valid signature),
surfacing failures as Error::SignerError (SIGNER_FAILED). Adds a unit test.
Author
|
bugbot run |
The signer_deadline timeout was applied to all signers, so a timeout on the in-process LocalSigner path surfaced as SignerError (SIGNER_FAILED), misleading for the local key path and diverging from the Go/TS SDKs. Consolidate every external-signer-only concern (deadline, error remapping, signature validation) into a single sign_external() helper and branch on is_external_signer, leaving the LocalSigner path byte-for-byte unchanged and structurally unable to enter external-only logic. This matches the Go SDK's signer/wallet branch and removes the recurring class of bug where external-only behavior leaked into the local key path. Add regression tests for deadline timeout, deadline pass-through, returned-signature validation, and non-SignerError remapping.
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 068f3f1. Configure here.
Author
|
@yaanakbr all checks are passed. You can review it now bro |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an external-signer seam so callers can sign Hyperliquid actions without giving the SDK a raw private key. Backed by a small, additive change to
buildSignSend.Closes #17.
Motivation
Enables KMS/HSM/remote-signer architectures where the private key must never enter the SDK process (e.g. a separate hardened signing service). See #17 for the full rationale.
API
Behaviour
WithSigner/WithSignerAddressoptions added next toWithPrivateKey.New()skips thePRIVATE_KEYenv fallback, in-process wallet creation, and builder-fee auto-approve.requireWallet()is satisfied by a wallet or a signer.buildSignSend()signs via the callback when set, elseWallet.SignHash.Address()returns the signer address when there's no wallet.Backward compatibility
WithPrivateKeyis byte-for-byte unchanged when no signer is set. The twos.wallet.*dereferences (Address(),buildSignSend) are guarded, so there's no nil-panic on the signer path.Testing
hyperliquid/signer_test.go(stdlibtestingonly, no network).go build ./...,go vet ./...,go test ./hyperliquid/...all pass; the existing suite is unaffected.Notes
No new dependencies.
gofmtclean on changed files.Note
Medium Risk
Changes the build→sign→send path and trading initialization; mistakes could break signing or mis-attribute errors, but the local private-key path is intended to stay unchanged when no signer is configured.
Overview
Adds external signing across Go, Python, TypeScript, and Rust so trading can run without loading a private key into the SDK. Callers supply a signer callback/trait (plus agent address where there is no local wallet);
buildSignSendsigns the build hash externally, validates r/s/v, and maps failures toSIGNER_FAILED/SignerError(separate from venueSIGNATURE_INVALID). With a signer configured, the SDK skipsPRIVATE_KEY, in-process wallet creation, and builder-fee auto-approve; signing operations accept wallet or signer.Go also wires per-order slippage from
OrderBuilderintoPlaceOrder. README sections and tests cover the new paths (including signer timeouts and malformed signature handling).Reviewed by Cursor Bugbot for commit 068f3f1. Configure here.