fix(deps): remediate trading security alerts - #185
Conversation
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 07cc9c7d
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-28T21:28:37Z
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 07cc9c7d
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-28T21:48:12Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 2 (2 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 193.6s (2 bridge agents) |
| Total | 193.6s |
💰 Value — sound-with-nits
Swaps the archived ethers-based Hyperliquid client for the official Alloy SDK, bumps OpenTelemetry, and trims 15 stale RustSec ignores — coherent and well-scoped; only nits are a misleading PR body and forked git deps.
- What it does: Replaces the unmaintained
ethers-signers+hyperliquid0.2 crates withalloy::signers::local::PrivateKeySigner+hyperliquid_rust_sdk(pinned Tangle-org git rev) in trading-runtime/src/hyperliquid.rs:1-13; because the new SDK binds the vault address into the ExchangeClient at construction, the wrapper adds a per-account client cache (HashMap<Option, Arc> at hyperli - Goals it achieves: (1) Resolve the GitHub default-branch security alerts by removing the archived ethers-v2 Hyperliquid client and bumping OpenTelemetry/rustls-webpki/jsonwebtoken/ring to fixed releases — the alert class, not paperwork. (2) Pay down static-analysis config debt: the ignore lists had accumulated entries for advisories no longer in the resolved graph, the root audit.toml was an explicit duplicate of .c
- Assessment: Sound on its merits. The Hyperliquid rewrite is forced by the security goal — the only way off the archived ethers-v2
hyperliquidcrate is onto the official SDK, and that SDK's API requires the per-account ExchangeClient cache (vault is bound at ExchangeClient::new, not per-call). The wrapper keeps its own typed HlOrderType enum and translates into the SDK's stringly-typed TIF/TpSl at the bounda - Better / existing approach: none — this is the right approach. I checked the codebase for an existing Hyperliquid client wrapper to extend (grep'd HyperliquidClient|hyperliquid:: across .rs: 75 matches, all consume trading_runtime::hyperliquid:: via the existing HL_CLIENTS static cache in trading-http-api/src/routes/hyperliquid.rs:29). There is no second implementation to fold into. The per-account ExchangeClient cache is
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content
🎯 Usefulness — sound
Swaps an archived Ethers-based Hyperliquid SDK for the official Alloy-based successor with full public-API preservation, deeply integrated into production trading paths.
- Integration: HyperliquidClient is heavily used: 4 call sites across the HTTP API (hyperliquid.rs:29 static HL_CLIENTS registry, strategy.rs:222/315 execution helpers, execute.rs:3194), blueprint binary shutdown (main.rs:638/642), and E2E tests (hyperliquid_e2e.rs:15). The PR preserves every public method signature (new/testnet/place_order/place_bracket/cancel_order/set_leverage/get_account/get_mids/resolve_ass
- Fit with existing patterns: Excellent fit with the codebase's direction. The old
hyperliquidcrate was Ethers-based and archived; the newhyperliquid_rust_sdkuses Alloy, which the workspace already depends on (trading-runtime/Cargo.toml:10 —alloy = { version = "1", ... }). This consolidates on the existing crypto stack rather than introducing a competing one. The lazy-init pattern (OnceCell + RwLock HashMap) compleme - Real-world viability: Holds up well. Lazy init is actually better than eager for this system: paper/read-only bots (get_hl_read_client with HL_READ_DUMMY_KEY, hyperliquid.rs:107) won't open exchange connections until a real order is placed. Per-vault-address ExchangeClient caching is necessary because the new SDK bakes vault_address into ExchangeClient::new (unlike the old per-call passing) — the double-checked-locking
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 PR body says 'Changed files (2)' but the diff touches 20 files (-1880/+475 lines) [maintenance] ``
The PR summary lists only trading-blueprint-lib/tests/otlp_export.rs and trading-runtime/src/hyperliquid.rs, but git diff --shortstat shows 20 files changed including Cargo.lock (-1648 lines), 8 Cargo.toml manifests, deny.toml, .cargo/audit.toml, the deleted root audit.toml + audits/static-analysis-summary.md, .github/workflows/ci.yml, and audits/static-analysis-triage.md. A reviewer reading only the body would miss the workspace-wide dep bumps (sandbox-runtime rev 8ae2d24e→4a2e0f96, OpenTelemet
🟡 Two new forked git deps should be tracked for upstream retirement [maintenance] ``
deny.toml's [sources].allow-git now includes https://github.com/drewstone/eigensdk-rs.git and https://github.com/tangle-network/hyperliquid-rust-sdk.git (deny.toml:178-181). The triage doc (audits/static-analysis-triage.md) calls out the EigenSDK fork as 'submitted upstream as Layr-Labs/eigensdk-rs#591' but does NOT flag that hyperliquid_rust_sdk is a Tangle-org mirror rather than the upstream canonical repo — the PR body even calls it 'the official Alloy SDK' which is slightly misleading since
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
Summary
Alert impact
GitHub currently reports 12 default-branch alerts: 4 high, 5 medium, and 3 low.
After merge, the lockfile and manifests should resolve 7 alerts: OpenTelemetry SDK (2), rustls-webpki (3), jsonwebtoken (1), and ring (1).
Five alerts remain with explicit current evidence:
No ignore list was expanded.
The RustSec audit list shrank from 27 entries to 12, and cargo-deny now carries only active-graph exceptions.
Proof
cargo check --workspace --all-features: passedforge build: passed, 160 Solidity files compiledcargo fmt --all -- --check: passedcargo audit -D warnings: passed, 1,337 locked crates scannedcargo deny check --hide-inclusion-graph: advisories, bans, licenses, and sources passedgit merge-tree --write-tree origin/main HEAD: passedThe repository cannot currently complete one unfiltered all-target test command on
mainbecause unchanged tests contain four staleWorkflowRunRecordinitializers, one removed five-argumentTradeValidatorcall, and eight stale four-argumentVaultDeployerconstructor calls.Those files are unchanged by this PR and their remaining healthy targets were run separately.
Upstream work
4a2e0f9652c61f26f253a957ed05ad49e92397109dc8b253ebae65d8e6a07b074ea6806d7cf91189efcc7f926f21bd159cfa847186538a360064eacc