Conversation
dijanin-brat
requested review from
bmzig,
dohaki,
mrice32,
nicholaspai and
pxrl
as code owners
pxrl
left a comment
pxrl
left a comment
There was a problem hiding this comment.
Looks good - only minor comments 👍
| const err = _err.toString(); | ||
| if (KNOWN_BINANCE_ERROR_REASONS.some((errorReason) => err.includes(errorReason)) && nRetries < maxRetries) { | ||
| const err = stringifyBinanceError(_err); | ||
| const stringError = _err.toString(); |
There was a problem hiding this comment.
Some errors we can read with _err.toString and for others we need to have more granular parsing. That is why we have both variations here so we cover both cases.
|
@codex Review. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57412d4196
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (BINANCE_EMPTY_RESPONSE_ERROR_PATTERNS.some((r) => err.includes(r) || stringError.includes(r))) { | ||
| return []; |
There was a problem hiding this comment.
This change treats proxy/HTTP/parse failures from withdrawHistory as an empty successful response, which makes callers interpret an API outage as "no prior withdrawals." In binanceFinalizer, amountToFinalize is computed from deposits minus withdrawals, so if this endpoint fails while deposits/balances still load, the bot can re-withdraw already-finalized deposits and overpay recipients (limited only by current Binance balance). This should propagate an error (or explicitly mark the run degraded) instead of silently returning [].
Useful? React with 👍 / 👎.
| ); | ||
| routesWithCoins.push(route); | ||
| }); | ||
| this.availableRoutes = routesWithCoins; |
There was a problem hiding this comment.
Pruning this.availableRoutes here can leave the Binance adapter with zero supported routes, but the rebalancer client still iterates its original rebalanceRoutes and invokes Binance getEstimatedCost/initializeRebalance on those routes. Because both methods assert supportsRoute, a transient accountCoins outage now leads to Route is not supported exceptions during rebalance evaluation rather than gracefully skipping Binance routes. The client-visible route set must be synchronized or calls should be gated by adapter.supportsRoute(route).
Useful? React with 👍 / 👎.
2 participants
No description provided.