improve: Add utility functions for inventory equivalence remapping and Binance deposit matching#3108
Conversation
…inance deposit matching, and floatToBN fix - TokenUtils: getInventoryEquivalentL1TokenAddress, getInventoryBalanceContributorTokens, isL2OnlyEquivalentToken - RunningBalanceUtils: getLatestRunningBalances extracted for shared use - BinanceUtils: getOutstandingBinanceDeposits for cross-L2 deposit matching, export BinanceDeposit type - BNUtils: Fix floatToBN MAX_SAFE_INTEGER overflow via string manipulation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| * @param bundleDataApproxClient BundleDataApproxClient for upcoming deposits/refunds. | ||
| * @returns Dictionary keyed by chain ID of running balance results. | ||
| */ | ||
| export async function getLatestRunningBalances( |
There was a problem hiding this comment.
Note, this is copied from InventoryClient._getLatestRunningBalances
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7d85415a5
ℹ️ 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".
| const upcomingDeposits = bundleDataApproxClient.getUpcomingDeposits(chainId, l1Token); | ||
| const upcomingRefunds = bundleDataApproxClient.getUpcomingRefunds(chainId, l1Token); | ||
|
|
||
| // Updated running balance is last known running balance minus deposits plus upcoming refunds. | ||
| const latestRunningBalance = lastValidatedRunningBalance.sub(upcomingDeposits).add(upcomingRefunds); |
There was a problem hiding this comment.
Convert upcoming amounts to L1 decimals before balance math
lastValidatedRunningBalance is in hub/L1 token units, but BundleDataApproxClient.getUpcomingDeposits and getUpcomingRefunds return amounts in the spoke token’s native decimals (see the existing conversion logic in InventoryClient._getLatestRunningBalances). Subtracting/adding these values directly here mis-scales running balances whenever L2 and L1 decimals differ, which can cause incorrect excess-balance calculations and repayment-chain selection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a good callout, however this function isn't used in any clients in this PR and the downstream PR (#3109) fixes this and makes sure that the BundleDataApproxClient functions return values in L1 units so I don't think this is necessary
|
@codex please respond to all unresolved comment threads |
|
To use Codex here, create an environment for this repo. |
| assert( | ||
| withdrawals.every((withdrawal) => withdrawal.network === BINANCE_NETWORKS[CHAIN_IDs.MAINNET]), | ||
| "Withdrawals must be for the Mainnet network" | ||
| ); |
There was a problem hiding this comment.
Do we intend to constrain this to mainnet, or just t ensure that each withdrawal is for the same chainId?
Constraining to mainnet is something we'd like to avoid moving forward, since ideally we can withdraw on any chain.
There was a problem hiding this comment.
I don't think this function works unless all withdrawals are to the same chain, otherwise how can we differentiate between deposits that are supposed to be withdrawn to mainnet vs another chain?
There was a problem hiding this comment.
Yeah, but do we require that it's always a withdrawal to mainnet? Could we instead just verify that all withdrawal networks are equal, rather than that they are all mainnet?
src/utils/BNUtils.ts
Outdated
| const bnAmount = BigNumber.from(Math.round(scaledAmount)); | ||
| // Convert to a fixed-point decimal string to avoid scientific notation (e.g. "1.234e-9") | ||
| // that JavaScript produces for very small or very large numbers. | ||
| const strFloat = typeof float === "string" ? float : float.toFixed(20).replace(/0+$/, "").replace(/\.$/, ""); |
There was a problem hiding this comment.
Is 20 decimal places significant in this context? i.e. does node switch to scientific notation beyond that, or something?
There was a problem hiding this comment.
i ended up rewriting floatToBN with the help of codex here 2d70db5
Co-authored-by: Paul <108695806+pxrl@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c874c1f7e6
ℹ️ 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".
src/utils/BNUtils.ts
Outdated
| // that JavaScript produces for very small or very large numbers. toFixed(20) is used because | ||
| // JavaScript's Number has ~15-17 significant digits of precision, so 20 decimal places captures | ||
| // everything the float can represent. Trailing zeros are stripped since they don't affect the value. | ||
| const strFloat = typeof float === "string" ? float : float.toFixed(20).replace(/0+$/, "").replace(/\.$/, ""); |
There was a problem hiding this comment.
Parse numeric amounts without IEEE-754 artifact expansion
Converting number inputs with toFixed(20) bakes in binary floating-point artifacts (for example 0.3 becomes 0.29999999999999998890), and the subsequent decimal rescaling turns that artifact into token units; this makes floatToBN(0.3, 18) produce 299999999999999988 instead of 300000000000000000. Because Binance amount paths pass number values into floatToBN, this regression can under/over-count bridge amounts and cause exact-amount reconciliation failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Number(0.3) is already 0.29999999999999998890... in IEEE 754 — the imprecision exists before floatToBN is called. The old code had the same issue (it just happened to round correctly for simple cases via Math.round). For exact amounts, callers should pass strings (which bypass toFixed entirely). The Binance API returns numeric floats so some precision loss is inherent at the source.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d70db5d8d
ℹ️ 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".
Summary
New utility functions that support the larger inventory accounting refactor (split from #3073).
getInventoryEquivalentL1TokenAddress,getInventoryBalanceContributorTokens,isL2OnlyEquivalentToken— generic resolution of L2-only tokens (pathUSD, USDH) to their inventory-equivalent L1 token viaTOKEN_EQUIVALENCE_REMAPPINGgetLatestRunningBalances— extracted from InventoryClient for shared use by InventoryClient and MonitorgetOutstandingBinanceDeposits— pure function for matching Binance deposits against withdrawals across multiple L2s, handling batched withdrawals correctly. ExportsBinanceDeposittypefloatToBNoverflow when input has many decimal digits (e.g.5654.8610313399695 * 10^13 > MAX_SAFE_INTEGER). Also handles scientific notation for very small numbersUpstream dependencies
None — merges directly to master.
Downstream PRs
Test plan
🤖 Generated with Claude Code