The pinned branch for SystemZ - #1
Conversation
|
|
||
| // set builder deployed perpdex meta | ||
| { | ||
| let perp_dexs = info.perp_dexs().await?; |
There was a problem hiding this comment.
Unnecessary API call when no perp dexs requested
The info.perp_dexs().await? call is made unconditionally at initialization, even when interested_perp_dexs is empty. All the example binaries pass vec![] for this parameter. When the vector is empty, the loop at line 196 will skip all entries via continue, making the HTTP request completely unnecessary. This adds latency to ExchangeClient creation and can cause failures if the endpoint is unavailable, even when the user doesn't need any perp dex information.
| "mismatch spot base asset index. expect {base}. got {}", | ||
| base_info.index | ||
| ); | ||
| let quote_info = &spot_meta.tokens[quote as usize]; |
There was a problem hiding this comment.
Array access can panic on malformed API data
The array accesses at spot_meta.tokens[base as usize] and spot_meta.tokens[quote as usize] will panic with an index out of bounds error if the token indices in spot_info.tokens exceed the length of the spot_meta.tokens vector. The subsequent assertions (lines 127-131, 133-137) cannot catch malformed data because the panic occurs before they execute. If the Hyperliquid API ever returns a spot universe entry with invalid token indices, ExchangeClient::new will crash instead of returning a graceful error.
Add Noop action and create_noop function
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Off-by-one in bounds check causes potential panic
- Changed bounds check from
<to<=on lines 70 and 85 to correctly catch when token index equals the array length.
- Changed bounds check from
- ✅ Fixed: Duplicated logic across two bulk order creation functions
- Refactored create_bulk_order to delegate to create_bulk_order_with_nonce, eliminating the duplicated code.
- ✅ Fixed: Type change removes safety for WebSocket URL construction
- Added validation in InfoClient::new to ensure base_url starts with 'http' and is at least 4 characters long before WebSocket URL construction.
Or push these changes by commenting:
@cursor push 6ebad93516
Preview (6ebad93516)
diff --git a/src/exchange/cancel.rs b/src/exchange/cancel.rs
--- a/src/exchange/cancel.rs
+++ b/src/exchange/cancel.rs
@@ -24,4 +24,3 @@
pub asset: u32,
pub cloid: String,
}
-
diff --git a/src/exchange/exchange_client.rs b/src/exchange/exchange_client.rs
--- a/src/exchange/exchange_client.rs
+++ b/src/exchange/exchange_client.rs
@@ -67,7 +67,7 @@
let [base, quote] = spot_info.tokens;
// just skip if malformed token returned
- if spot_meta.tokens.len() < base as _ {
+ if spot_meta.tokens.len() <= base as _ {
eprintln!("Base token index out-of-bound: {base}");
continue;
}
@@ -82,7 +82,7 @@
}
// just skip if malformed token returned
- if spot_meta.tokens.len() < quote as _ {
+ if spot_meta.tokens.len() <= quote as _ {
eprintln!("Quote token index out-of-bound: {quote}");
continue;
}
@@ -359,34 +359,8 @@
wallet: Option<&LocalWallet>,
expires_after: Option<u64>,
) -> Result<ExchangePayload> {
- let wallet = wallet.unwrap_or(&self.wallet);
- let timestamp = next_nonce();
-
- let mut transformed_orders = Vec::new();
-
- for order in orders {
- transformed_orders.push(order.convert());
- }
-
- let action = Actions::Order(BulkOrder {
- orders: transformed_orders,
- grouping: "na".to_string(),
- });
- let connection_id = action.hash(timestamp, self.vault_address, expires_after)?;
- let action = serde_json::to_value(&action).map_err(|e| Error::JsonParse(e.to_string()))?;
-
- let is_mainnet = self.http_client.base_url == BaseUrl::Mainnet.get_url();
- let signature = sign_l1_action(wallet, connection_id, is_mainnet)?;
- let nonce = timestamp;
-
- let exchange_payload = ExchangePayload {
- action,
- signature,
- nonce,
- vault_address: self.vault_address,
- expires_after,
- };
- Ok(exchange_payload)
+ let (payload, _) = self.create_bulk_order_with_nonce(orders, wallet, expires_after)?;
+ Ok(payload)
}
pub fn create_bulk_order_with_nonce(
diff --git a/src/info/info_client.rs b/src/info/info_client.rs
--- a/src/info/info_client.rs
+++ b/src/info/info_client.rs
@@ -80,6 +80,12 @@
let client = client.unwrap_or_default();
let base_url = base_url.unwrap_or(BaseUrl::Mainnet.get_url());
+ if !base_url.starts_with("http") || base_url.len() < 4 {
+ return Err(Error::GenericParse(
+ "base_url must start with 'http' and be at least 4 characters".to_string(),
+ ));
+ }
+
Ok(InfoClient {
http_client: HttpClient { client, base_url },
ws_manager: None,
diff --git a/src/signature/create_signature.rs b/src/signature/create_signature.rs
--- a/src/signature/create_signature.rs
+++ b/src/signature/create_signature.rs
@@ -25,8 +25,17 @@
keccak256(x.encode()).into()
}
-pub(crate) fn sign_l1_action(wallet: &LocalWallet, connection_id: H256, is_mainnet: bool) -> Result<Signature> {
- sign_with_agent(wallet, EthChain::Localhost, if is_mainnet { "a" } else { "b" }, connection_id)
+pub(crate) fn sign_l1_action(
+ wallet: &LocalWallet,
+ connection_id: H256,
+ is_mainnet: bool,
+) -> Result<Signature> {
+ sign_with_agent(
+ wallet,
+ EthChain::Localhost,
+ if is_mainnet { "a" } else { "b" },
+ connection_id,
+ )
}
pub(crate) fn sign_usd_transfer_action(
diff --git a/src/ws/message_types.rs b/src/ws/message_types.rs
--- a/src/ws/message_types.rs
+++ b/src/ws/message_types.rs
@@ -23,10 +23,10 @@
#[derive(Deserialize, Clone, Debug)]
pub struct Candle {
- pub data: CandleData
+ pub data: CandleData,
}
#[derive(Deserialize, Clone, Debug)]
pub struct OrderUpdates {
- pub data: Vec<OrderUpdate>
\ No newline at end of file
-}
+ pub data: Vec<OrderUpdate>,
+}
diff --git a/src/ws/sub_structs.rs b/src/ws/sub_structs.rs
--- a/src/ws/sub_structs.rs
+++ b/src/ws/sub_structs.rs
@@ -87,12 +87,12 @@
#[derive(Deserialize, Clone, Debug)]
#[serde(rename_all = "camelCase")]
pub struct BasicOrder {
- pub coin: String,
- pub side: String,
- pub limit_px: String,
- pub sz: String,
- pub oid: u64,
- pub timestamp: u64,
- pub orig_sz: String,
- pub cloid: Option<String>,
+ pub coin: String,
+ pub side: String,
+ pub limit_px: String,
+ pub sz: String,
+ pub oid: u64,
+ pub timestamp: u64,
+ pub orig_sz: String,
+ pub cloid: Option<String>,
}This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
|
|
||
| let [base, quote] = spot_info.tokens; | ||
| // just skip if malformed token returned | ||
| if spot_meta.tokens.len() < base as _ { |
There was a problem hiding this comment.
Off-by-one in bounds check causes potential panic
High Severity
The bounds check spot_meta.tokens.len() < base as _ uses < instead of <=. When base equals tokens.len(), the guard is false (doesn't continue), so spot_meta.tokens[base as usize] on the next line panics with an index-out-of-bounds error. The same issue affects the quote check on the subsequent block.
Additional Locations (1)
| expires_after, | ||
| }; | ||
| Ok((exchange_payload, nonce)) | ||
| } |
There was a problem hiding this comment.
Duplicated logic across two bulk order creation functions
Low Severity
create_bulk_order and create_bulk_order_with_nonce have identical implementations — the only difference is that the latter additionally returns the nonce in a tuple. The duplicated logic increases maintenance burden and risks inconsistent bug fixes. create_bulk_order_with_nonce could delegate to create_bulk_order (or vice versa) instead of copying all the code.
Additional Locations (1)
| pub async fn new(client: Option<Client>, base_url: Option<String>) -> Result<InfoClient> { | ||
| let client = client.unwrap_or_default(); | ||
| let base_url = base_url.unwrap_or(BaseUrl::Mainnet).get_url(); | ||
| let base_url = base_url.unwrap_or(BaseUrl::Mainnet.get_url()); |
There was a problem hiding this comment.
Type change removes safety for WebSocket URL construction
Medium Severity
Changing InfoClient::new from accepting Option<BaseUrl> (a safe enum) to Option<String> removes the type-level guarantee that base_url starts with "http". The subscribe and unsubscribe methods construct a WebSocket URL via &self.http_client.base_url[4..], which will panic if the user-provided string is shorter than 4 bytes, or produce an incorrect URL if it doesn't start with "http". Previously the BaseUrl enum made this impossible.
Additional Locations (1)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Meta structs are internal to the module, no need for public re-export (avoids ambiguity with crate-root Meta/AssetMeta types) - Restore field doc comments on AssetMapping struct Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…set_mapping/ Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
The function `next_nonce()` always returns a unique nonce, by storing the current timestamp in a static variable and incrementing it before returning. However, this variable can lag during times of inactivity. Though there is a check that handles it by updating it with the current timestamp, it ends with returning the old nonce, which fails the current call. This fix makes sure that the current timestamp is returned when this check gets triggered.
…nonce upstream PR hyperliquid-dex#108 (prev commit) fixed the most acute symptom -- the 5-minute drift correction returning the pre-correction (stale) counter value, which caused the failed send we observed in production (X=07:38:03 vs Y=07:46:39, X was 11min behind wall clock). but PR hyperliquid-dex#108 still keeps the 5-minute threshold: between corrections, the counter drifts below wall clock, and if the exchange bounds nonces by wall-clock distance (HL's rejection pattern suggests a ~2.5min tolerance) calls will be rejected during that window. replace the fetch_add(1) + threshold scheme with a CAS loop that, on every call, sets CUR_NONCE = max(prev, now_ms) + 1. this guarantees: - strict monotonicity across threads (compare_exchange_weak) - nonce never below wall clock -- drift is structurally impossible - no 5-minute window of degraded nonces observed production failure ("Invalid nonce: nonce too low 1776584283874 < 1776584799101"): counter was 11min behind wall clock at the time of send. under this CAS loop the counter would have read max(prev, now_ms)+1 ~= 07:49:07.760, well above the exchange's 07:46:39 minimum -- order would have been accepted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fix: eliminate next_nonce drift causing 'nonce too low' rejections
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
HL's cancel and cancelByCloid actions accept an optional fast flag encoded as `f`. Add it to BulkCancel/BulkCancelCloid and thread `fast: bool` through create_bulk_cancel / create_bulk_cancel_by_cloid. f must be omitted when false (HL rejects actions hashed with f: false), so the field uses skip_serializing_if, keeping fast: false byte-identical to no flag in both the rmp hash and posted json. Existing cancel-hash signatures are unchanged; added a fast: true assertion to lock in the new behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>



Note
Medium Risk
Medium risk because it upgrades the HTTP stack (
reqwest/hyper/http) and changes public client APIs (base URL types and order/cancel asset identifiers) while adding new signed action payload formats (expires_after,ScheduleCancel,Noop).Overview
Updates networking dependencies by bumping
reqwestto0.12.4(and associatedhyper/http/h2crates), regeneratingCargo.lock.Refactors exchange/info client APIs to be asset-id based and dex-aware:
ClientOrderRequest/cancel requests now takeasset: u32,ExchangeClient::new/InfoClient::newtakebase_url: Option<String>,InfoClient::metaaccepts an optionaldex, and newspot_meta/perp_dexsendpoints plusDex/PerpDexMeta/SpotMetatypes are added.Adds
AssetMapping(built fromspot_meta+ per-dexmeta) and new helpers to build signed exchange payloads without sending (create_bulk_order*,create_bulk_cancel*), plus new actions/payload fields (expires_after,ScheduleCancel,Noop). Removes the legacy market maker module/example and updates all bins accordingly, adding a newmetadataexample.Written by Cursor Bugbot for commit 68f5afc. This will update automatically on new commits. Configure here.