Repository navigation
[simplex]: refuse decimals the asset registry contradicts, remove overfill protection - #1414
Conversation
Wizdave97
left a comment
There was a problem hiding this comment.
Verdict: changes needed before merge. The decimals cross-check is correct; the overfill removal needs one change.
The decimals check:
- The lookup is by chain plus lowercased address, not by symbol.
- The check runs after the
try/catch, so the registry fallback can't swallow it. - Every caller turns the throw into "skip this order".
- All 38 mainnet registry entries, plus BSC Chapel and Amoy, match on-chain
decimals(). - One unrelated registry bug: Gnosis Chiado USDC is registered as 6 decimals but returns 18 on chain. Simplex doesn't use that chain.
Must fix
- Rejecting
[simplex.overfillProtection]stops existing configs from booting (filler-toml.ts:308).- On main,
simplex init→ Fine-tune → "Overfill protection" writes the section uncommented (emit-toml.ts:111-117on main). That has shipped since #1051, andcarryPrefillExtraskeeps the section on laterinitruns. - After upgrading, those operators'
simplex runthrows, and under Docker's--restart unless-stoppedit crash-loops. simplex initcan't migrate the file either: it rejects the file and only offers "Start fresh?", which loses the prefilled answers.- Through the desktop app the reason isn't shown at all. The error happens before the UI socket opens, so the user only sees "exited before its UI became ready".
- The setting has done nothing since #964, so rejecting it adds no safety.
- Suggestion: warn and ignore it at boot, as is done for
[[pairs]], and delete the key wheninitloads the file, soemitFillerTomldrops it on the next write. - The
incheck also trips on{ overfillProtection: undefined }from library callers;!== undefinedis safer.
- On main,
Should fix
- A stale safety comment (ContractInteractionService.ts:302-303).
- It says a token the registry doesn't know is safe because "
FXFillerbounds its bid against the order's own output". That bound was removed in 27d82a4, andtargetOutputnow has no ceiling againstoutput.amount. - So a custom
[assets]token whose RPC answers 18 decimals for a 6-decimal token still bids 10¹² too much, limited only by wallet balance and funding venues. - The PR description is honest about this scope. Please correct the comment, and consider cross-checking
[assets]entries that declare decimals.
- It says a token the registry doesn't know is safe because "
Notes:
- A token that fails the check is re-read on every evaluation: about 2–4
eth_calls plus an error log line per order. That's acceptable; a short negative cache would only cut the log noise. getFeeTokenWithDecimalsstill caches its decimals without the check. The effect is limited to profit ranking and logs, because the cost and spread gates compare raw amounts.- The release should be a minor version bump that calls out the config change and the removed library and HTTP API.
Cross-PR: 7da2f5b removes the settlement hold give-back on the premise that #1421 removes holds. The new decimals throw is one more way settleFilledLimitOrder can fail after claimExecutedBid and leave reserved inflated. This PR shouldn't ship without #1421.
CI: the failing live job is unrelated. In the SDK, intentGatewayCancel.test.ts hit "insufficient funds for gas" because the test wallet ran dry, which #1420 addresses.
- A config that still carries [simplex.overfillProtection] is accepted, not rejected: validateConfig no longer throws, boot logs a warning that the section is ignored, and simplex init drops the key (assembleConfig also clears it) so the next write omits it. - Correct the getTokenDecimals comment that claimed FXFiller bounds a bid against the order's output; [assets] entries declare no decimals, so an unknown token is limited only by the wallet balance and funding venues. - Revise the note to match.
7da2f5b to
58eff72
Compare
- A config that still carries [simplex.overfillProtection] is accepted, not rejected: validateConfig no longer throws, boot logs a warning that the section is ignored, and simplex init drops the key (assembleConfig also clears it) so the next write omits it. - Correct the getTokenDecimals comment that claimed FXFiller bounds a bid against the order's output; [assets] entries declare no decimals, so an unknown token is limited only by the wallet balance and funding venues. - Revise the note to match.
58eff72 to
80b6ff0
Compare
- A config that still carries [simplex.overfillProtection] is accepted, not rejected: validateConfig no longer throws, boot logs a warning that the section is ignored, and simplex init drops the key (assembleConfig also clears it) so the next write omits it. - Correct the getTokenDecimals comment that claimed FXFiller bounds a bid against the order's output; [assets] entries declare no decimals, so an unknown token is limited only by the wallet balance and funding venues. - Revise the note to match.
80b6ff0 to
789296b
Compare
Wizdave97
left a comment
There was a problem hiding this comment.
Approved. Old configs that still contain [simplex.overfillProtection] now boot from every entry point, with one warning. simplex init and the dashboard rewrite both drop the key, and the stale comment is fixed.
Non-blocking follow-ups:
- PR description and commit title. The description still says the section "is rejected by
validateConfig" and lists rejection tests. The title of commit3ec6f5e4esays the same. Worth fixing before the squash. - Premature log line.
simplex initlogs "Dropped [simplex.overfillProtection]…" (cli/init/index.ts:108) before the operator has picked an action. If they choose "Start the filler with it as-is", nothing is actually dropped. - Merge order. Merge this together with #1421, because this PR drops the settlement hold release on the basis that #1421 removes holds.
- A config that still carries [simplex.overfillProtection] is accepted, not rejected: validateConfig no longer throws, boot logs a warning that the section is ignored, and simplex init drops the key (assembleConfig also clears it) so the next write omits it. - Correct the getTokenDecimals comment that claimed FXFiller bounds a bid against the order's output; [assets] entries declare no decimals, so an unknown token is limited only by the wallet balance and funding venues. - Revise the note to match.
789296b to
e940553
Compare
…at the ask plus maxOverfillBps
…fig that still sets them with a warning
- A config that still carries [simplex.overfillProtection] is accepted, not rejected: validateConfig no longer throws, boot logs a warning that the section is ignored, and simplex init drops the key (assembleConfig also clears it) so the next write omits it. - Correct the getTokenDecimals comment that claimed FXFiller bounds a bid against the order's output; [assets] entries declare no decimals, so an unknown token is limited only by the wallet balance and funding venues. - Revise the note to match.
e940553 to
2b095cb
Compare
Stacked on #1410 (stack #1417). Review only this PR's commits.
Merge this together with #1421. It drops the settlement hold release because #1421 removes limit-order holds. On its own, this PR's
settleFilledLimitOrderno longer gives holds back when settlement fails before it commits, which would leave a limit order'sreservedinflated.This PR fixes c18 (high, decimals half) from the 2026-10-15 simplex audit, and removes the overfill protection machinery.
Decimals cross-check (
ContractInteractionService.getTokenDecimals)getTokenDecimalsreaddecimals()once, from one endpoint of the plain fallback client, and cached it for the life of the process. The asset registry was only consulted when that read failed. One RPC answering 18 for a 6-decimal USDC inflated a bid by 10^12.… refusing to price it) and not cached, so that order is skipped. The check runs outside the read's error handler, so the registry fallback can't swallow it. Tokens the registry doesn't know are cached as before, and a failed read still falls back to the registry.[assets]entry, still takes the chain's answer.Overfill protection removed
[simplex.overfillProtection]is no longer read, and a config that still sets it is ignored with a warning rather than refused.simplex initwrote the section uncommented for existing operators, and refusing it would crash-loop their deployments.validateConfigaccepts the section in any form. Boot logs[simplex.overfillProtection] is ignored: overfill protection was removed …whensimplex.overfillProtection !== undefined, so a library caller passing{ overfillProtection: undefined }is not warned.simplex init: an update run does not write the section into the new file, and logsDropped [simplex.overfillProtection]once that file is written. "Start the filler with it as-is" leaves the file untouched, so boot's warning is what the operator sees.Simplex.isHalted(),Simplex.resetHalt(),SimplexStatus.halted,FillerConfigService.getMaxOverfillBps()/getMaxConsecutiveClamps(),FXFiller.isHalted()/resetHalt(),HaltControlandhaltControls.POST /api/reset-halt(now a 404), andhaltedin/api/status.Settlement hold release removed (
IntentFiller.settleFilledLimitOrder): the code that returned a claim's holds when settlement threw before its transaction committed is gone, because limit-order holds are being removed in #1421. This is why the two PRs merge together.Validation
Run in
sdk/packages/simplexat the PR head:write-step(2) andupdate-run-preservation(14) passed; the decimals,filler-toml-validateandboot-bundler-preflightsuites are in the runs below.testjob commands: simplexlintexit 0 (warnings only);test:data146 passed;test:unit267 passed;test:filler70 passed, 5 skipped.vitest --maxConcurrency=1 src/tests/core src/tests/data src/tests/orderbook src/tests/services): 745 passed, 4 skipped.tscshows the 14 errors already on main, none in changed files.validateConfigaccepts{},{ maxOverfillBps },{ maxConsecutiveClamps }and the example TOML with the section appended;undefined;halted, and/api/reset-haltis a 404.Not covered: no mainnet run.