feat(hl-react): update hyperliquid api coverage - #103
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Code ReviewOverviewThis PR expands The changes are tightly scoped and well-structured. A few things are worth checking before merge. Potential Bugs
The README explicitly documents
The updated README body names Code QualitySorting convention — the set uses case-sensitive ASCII ordering, so uppercase letters sort before their lowercase counterparts ( Test coverage — the Public API SurfaceFive hooks are newly exported as named entry points in
Peer Dep RangeChanging from Summary
Overall a clean PR — just address the two missing-methods questions and decide on the export-stability story before merging. |
Code ReviewOverviewThis PR expands the Hyperliquid API coverage in Positives
Issues1.
|
Code Review — feat(hl-react): update hyperliquid api coverageOverviewThis PR expands What's Good
Issues & Suggestions1. interface PackageJson {
exports: Record<string, string>;
}Node's interface PackageJson {
exports: Record<string, string | Record<string, string>>;
}The downstream 2. Redundant test — The test added just below the existing it("routes representative L1 methods to trading client", () => {
const expectedL1Methods: ExchangeMethod[] = ["agentEnableDexAbstraction", "setDisplayName"];
...
});
3. Comment in // All other ExchangeClient methods are L1 actions and intentionally use the
// agent trading client. Examples include setDisplayName and agentEnableDexAbstraction
// agent-side actions in the SDK, which call executeL1Action rather than // <-- "agent-side actions" restates line 2
// executeUserSignedAction."Examples include setDisplayName and agentEnableDexAbstraction" is self-contained. "agent-side actions in the SDK" on the line below re-states it. Trim to: // All other ExchangeClient methods are L1 actions routed to the trading client.
// They call executeL1Action in the SDK, not executeUserSignedAction.4. Peer-dep range in README vs
If the workspace root pins to 5. Hardcoded relative path count in test const PACKAGE_DIR = resolve(TEST_DIR, "../../../../../packages/hl-react");Five Summary
Overall a clean, well-scoped PR. The |
|
test comment - please ignore |
|
PR Review: feat(hl-react): update hyperliquid api coverage Overview: This PR expands the hl-react package to cover newer Hyperliquid SDK 0.32.2 methods — adding 7 user-signed exchange actions to the registry, exposing 5 previously-internal hooks as named subpath exports, and adding a new test file that validates export-to-source-file integrity. Documentation is thorough and accurate. What looks good:
Issues / suggestions:
Function declarations are hoisted so this works at runtime, but it breaks the top-to-bottom reading flow of the module. Move the helper above the const assignments at lines 15-19 of hl-react-exports.test.ts.
useInfo and useSubscription existed before this PR. If either were accidentally removed from package.json, the 'exposes direct hook subpaths' test would still pass because it only asserts on the 5 new entries. Either add the pre-existing exports to the toMatchObject snapshot, or rename the test to clarify scope (e.g. 'exposes newly-added hook subpaths').
The project style guide allows a comment only when the WHY is non-obvious. The SDK call-path distinction (executeL1Action vs executeUserSignedAction) qualifies — but two lines is one too many. Collapse to a single line:
Summary: The core change (registry + exports + docs) is correct and well-tested. Two actionable asks before merge:
The comment trimming and peer-dep check are minor. Overall a clean, well-scoped PR. |
PR Review: feat(hl-react): update hyperliquid api coverageOverview: This PR expands the hl-react package to cover newer Hyperliquid SDK 0.32.2 methods — adding 7 user-signed exchange actions to the registry, exposing 5 previously-internal hooks as named subpath exports, and adding a new test file that validates export-to-source-file integrity. Documentation is thorough and accurate. What looks good
Issues / suggestions1. Function declarations are hoisted so this works at runtime, but it breaks the top-to-bottom reading flow of the module. Move the helper above the 2. Second test only covers the 5 newly-added exports
3. Two-line comment where one would do (code-style rule) The project style guide allows a comment only when the WHY is non-obvious. The SDK call-path distinction ( 4. Peer dependency range is intentionally broad — confirm no runtime availability gaps
SummaryThe core change (registry + exports + docs) is correct and well-tested. Two actionable asks before merge:
The comment trimming and peer-dep check are minor. Overall a clean, well-scoped PR. |
No description provided.