Fiet/update #8
Fiet/update #8
Conversation
There was a problem hiding this comment.
verityUrlMap seems to be why ExchangeMap exists.
Let's try keep CCXT agnostic to Verity please
Verity integration conducted inside of the Prover's CEX Broker Sidecar.
Furthermore, are there not existing function available to fetch account details for MEXC that we had to incorporate a new one?
| spotPrivateGetOrder(params?: {}): Promise<implicitReturnType>; | ||
| spotPrivateGetOpenOrders(params?: {}): Promise<implicitReturnType>; | ||
| spotPrivateGetAllOrders(params?: {}): Promise<implicitReturnType>; | ||
| spotPrivateGetAccount(params?: {}): Promise<implicitReturnType>; |
There was a problem hiding this comment.
Would this existing function not work?
Maybe run a check to determine what the TLS Proof outcome is for this function?
There was a problem hiding this comment.
i couldn't justify saving 100 times the data need and sending that to zkvm when an api exist with just what we need
https://www.mexc.com/api-docs/spot-v3/spot-account-trade#query-uid
ts/src/base/Exchange.ts
Outdated
| @@ -2838,6 +2886,7 @@ export default class Exchange { | |||
| this[property] = value | |||
| } | |||
| } | |||
| this.urlToMethodMap = mergeExchangeMaps(this.urlToMethodMap, userConfig.verityUrlMap) | |||
There was a problem hiding this comment.
What is userConfig.verityUrlMap ?
Is this set inside of the CEX Broker?
There was a problem hiding this comment.
verityUrlMap is a way to add to the urlToMethodMap so we dont have to redeploy if we want to support/add other Options like FetchAccountId with it corresponding Urls
There was a problem hiding this comment.
But in FetchAccountId, you're not using verityUrlMap ?
Additionally:
verityUrlMapseems to be whyExchangeMapexists.Let's try keep CCXT agnostic to Verity please
Verity integration conducted inside of the Prover's CEX Broker Sidecar.
There was a problem hiding this comment.
In case if we want to keep the opportunity to extend the map dynamically without redeploying the package, and meanwhile keep the package Verity agnostic, we can rename the verityUrlMap to something like extraExchangeMap.
But I think it's not enough to merge maps only without extending an exchange's interface which requires package redeployment anyway.
There was a problem hiding this comment.
Based on my review, there was no evidence explaijning what verityUrlMap is used for…
keep the package Verity agnostic
This is a correct objective for our CCXT package
There was a problem hiding this comment.
verityUrlMap could be renamed to extraExchangeMap.
verityUrlMap only extends the map dynamically without redeploying.
@victorshevtsov I am not sure what you mean by
But I think it's not enough to merge maps only without extending an exchange's interface which requires package redeployment anyway.
There was a problem hiding this comment.
Ok, maybe remove for now until we need something like this in the future.
Let's make an effort to remove features we create that are not used to minimise tech debt.
There was a problem hiding this comment.
Yes, you are right, verityUrlMap is never used, but introduced for a future use in case we need to update the map to call a specific API which currently not defined in the map, without redeploying the package.
WalkthroughThis PR bumps the library version from 0.0.12 to 0.0.13 and adds support for fetching account identifiers across multiple exchanges (MEXC, Binance, and Bybit) by introducing new spotPrivateGetUid API methods and routing corresponding endpoints to the fetchAccountId handler. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ts/src/base/Exchange.ts (1)
1308-1318: Binance account‑ID mapping: consider testnet parity and confirm endpointAdding
"https://api.binance.com/api/v5/user/query-api": "fetchAccountId"will correctly map mainnet UID queries tofetchAccountIdin the override flow. Two follow‑ups:
- Confirm this is the exact REST URL your Binance
fetchAccountIdimplementation uses (path, version, and host).- Decide if you also need a testnet mapping (e.g., a
testnet.binance.*host), as is done for many other Binance URLs in this map; if not, a short comment explaining why mainnet‑only is sufficient would help future readers.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
js/ccxt.d.tsjs/ccxt.jsjs/src/abstract/mexc.d.tsjs/src/base/Exchange.jsjs/src/mexc.jspackage.jsonts/ccxt.tsts/src/abstract/mexc.tsts/src/base/Exchange.tsts/src/mexc.ts
🧰 Additional context used
🧬 Code graph analysis (3)
ts/ccxt.ts (2)
js/ccxt.d.ts (1)
version(555-555)js/ccxt.js (1)
version(41-41)
js/ccxt.js (2)
js/ccxt.d.ts (1)
version(555-555)ts/ccxt.ts (1)
version(431-431)
js/ccxt.d.ts (2)
js/ccxt.js (1)
version(41-41)ts/ccxt.ts (1)
version(431-431)
🔇 Additional comments (14)
package.json (1)
3-3: LGTM! Version bump is clean.The version increment from 0.0.12 to 0.0.13 follows semantic versioning for a patch release.
js/ccxt.d.ts (1)
7-7: LGTM! Type declaration updated correctly.The version declaration is consistent with the version bump in package.json.
ts/ccxt.ts (1)
43-43: LGTM! Version constant updated consistently.The version is correctly synchronized with package.json and js/ccxt.d.ts, and properly assigned to
Exchange.ccxtVersionon line 45.js/src/mexc.js (1)
192-237: UID endpoint wiring into MEXC spot private GET looks structurally correctThe new
'uid': 1entry is consistent with surrounding spot private GET definitions and ccxt’s naming scheme (it will map cleanly tospotPrivateGetUid). No structural issues in theapidescription.Given that other account-related routes here use higher weights (
account,tradeFeeare10), please double‑check the documented rate‑limit weight and exact path name for this UID endpoint in the MEXC spot v3 docs to avoid throttling surprises.ts/src/abstract/mexc.ts (1)
25-35: Abstract interface update for UID endpoint is consistent with existing patternThe
spotPrivateGetUiddeclaration matches the established naming and typing of other spot private GET methods and lines up with the new'uid'entry injs/src/mexc.js. Interface surface looks good.If this file is generated, ensure the underlying generator/TS source is updated so the change is not lost on rebuild.
js/ccxt.js (1)
40-42: Version constant bump to 0.0.13 is fine—ensure all manifests are alignedThe version update and
Exchange.ccxtVersionassignment are consistent here.Please confirm that
js/ccxt.d.ts,ts/ccxt.ts, andpackage.jsonall expose0.0.13as well so published artifacts and typings stay in sync.js/src/abstract/mexc.d.ts (1)
23-23: UID endpoint declaration matches existing mexc interface conventions
spotPrivateGetUidfollows the same naming andimplicitReturnTypepattern as the otherspotPrivateGet*methods, so this cleanly exposes the new/api/v3/uidendpoint at the abstract layer.ts/src/mexc.ts (1)
191-199: UID endpoint mapping looks correct; just verify end‑to‑end wiringAdding
'uid': 1underspot.private.getcorrectly represents the documentedGET /api/v3/uidendpoint and keeps the rate‑limit weight consistent with MEXC’s spec. Assuming the baseExchangerouting now points this path atfetchAccountId, the mexc side of that flow should work as expected; worth a quick live/API test to confirm the response shape matches whatfetchAccountIdexpects.js/src/base/Exchange.js (3)
336-382: MEXC uid endpoint mapping looks correct
https://api.mexc.com/api/v3/uidis added under themexcmap and routed tofetchAccountId, somethodCalledwill resolve correctly for your override client. No functional issues spotted.Please double‑check against MEXC’s latest API docs that
/api/v3/uidis the exact path used by yourspotPrivateGetUidcall (including version and host) and that you don’t also need a separate testnet host mapped here.
1118-1305: Binance user/query-api correctly routed to fetchAccountIdThe new entry
"https://api.binance.com/api/v5/user/query-api": "fetchAccountId"fits the existingbinancemapping pattern and will allowfetch()to tag these calls withmethodCalled = 'fetchAccountId'. The prefix matching logic won’t conflict with other Binance URLs.Confirm from Binance’s API reference that:
api.binance.com/api/v5/user/query-apiis the endpoint yourfetchAccountIdimplementation actually hits, and- you don’t also invoke a testnet or alternative host variant that should be added to this map.
1530-1578: Bybit mainnet & testnet query-api mappings are consistentBoth mainnet and testnet Bybit URLs (
https://api.bybit.com/v5/user/query-apiandhttps://api-testnet.bybit.com/v5/user/query-api) are mapped tofetchAccountIdunder thebybitsection, matching how other Bybit endpoints are handled. This ensures the override metadata is populated for both environments.Verify that your Bybit
fetchAccountId/user/query-apicalls only ever use these two hosts and paths; if any regional or legacy hosts are in use, consider adding them here as well somethodCalledis set consistently.ts/src/base/Exchange.ts (3)
540-550: MEXC UID →fetchAccountIdmapping looks consistent; please verify URL usageThe new
"https://api.mexc.com/api/v3/uid": "fetchAccountId"entry aligns with the existingurlToMethodMappattern and will correctly surfacemethodCalled === "fetchAccountId"for overrides when this endpoint is hit. Please double‑check:
- That your MEXC implementation of
fetchAccountIdactually uses this exact URL (no trailing slash / different base host).- Whether you also need a testnet or futures variant mapped here, similar to other MEXC entries.
1738-1746: Bybit prod + testnetuser/query-api→fetchAccountIdmappings align with existing patternThe new Bybit mappings:
https://api.bybit.com/v5/user/query-apihttps://api-testnet.bybit.com/v5/user/query-apiboth routing to
"fetchAccountId"are consistent with the rest ofurlToMethodMapand ensure your override predicate/meta can distinguish account‑ID fetches for both environments.Just make sure these are the exact URLs your Bybit implementation calls and that there are no additional network variants you care about.
3158-3170: RefactoredmatchedEntry/methodCalledblock is behavior‑preservingThe reformatting to:
const matchedEntry: any = Object.entries(idMap).find(([prefix]) => path.startsWith(prefix) ); const methodCalled: string = matchedEntry?.[1] ?? "";keeps the previous semantics while improving readability;
methodCalledstill defaults to""when no prefix matches and logging remains guarded bythis.verbose.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.