test(bstock): drive the fork suite off the live deployment and wiring - #704
test(bstock): drive the fork suite off the live deployment and wiring#704Debugger022 wants to merge 1 commit into
Conversation
The suite pointed at a superseded liquidator at a block predating its deployment, and configured its own router allowlist, so it could not catch a missing or rolled-back setRouter. It now attaches to the live proxy and asserts the mainnet allowlist instead. Adds flash+two-hop and flash+native-BNB coverage (both previously untested despite flash being the recommended shape for non-USDT debt), plus opt-in LIVE_AMM=1 tests that prove the KyberSwap and OpenOcean adapters execute against their real routers.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
|
vancegpt-bot
left a comment
vancegpt-bot
left a comment
There was a problem hiding this comment.
Approving. Test-only change, and I verified the on-chain premises it now rests on rather than taking the comments' word for them.
Every factual claim in the new comments checks out against bscmainnet:
| Claim | Check |
|---|---|
DEPLOYED_LIQ is the live proxy |
matches deployments/bscmainnet/BStockLiquidator.json on this branch |
| proxy deployed at 111096720 | no code at 111096719, 5481 bytes at 111096720 |
| router wiring landed at 111264556 | isRouter false for PCS and Kyber at 111264555, true at 111264556 |
all four hop-2 targets allowlisted at FORK_BLOCK |
PancakeSwap V2, KyberSwap, OpenOcean and the VAI PSM all true at 111264600, and still true at head |
So the before allowlist assertions hold at the pinned block, and dropping the fresh-proxy deploys in the Liquid Mesh and VAI scenarios is safe — the deployed implementation carries both routerSpender and VAI support.
Driving the suite off the live deployment and the live wiring is the right direction: a test that allowlists its own routers proves the contract works against a configuration nobody operates.
Four notes, none blocking.
1. The green checks on this PR do not cover this file. ci.yaml has no fork job — the suite needs FORKED_NETWORK and an archive RPC, so Test / Lint / Docgen passing says nothing about BStockLiquidatorFork.ts. My approval rests on reading the diff and verifying the chain state above, not on a run: I do not have this repo's dependencies installed and did not execute the suite. Worth someone posting the local run output, since nothing in CI will ever tell you it broke.
2. The pinned block neutralises the stated reason for asserting the allowlist.
a test that sets up its own allowlist cannot catch a missing or rolled-back
setRouter, which is exactly the misconfiguration that bricks liquidations
True at head, but at FORK_BLOCK = 111264600 the state is frozen: a setRouter(..., false) on mainnet tomorrow leaves this assertion passing forever, because the fork never sees it. As written, the check can only catch a rollback that happened before block 111264600 — i.e. never. The regression the comment describes is caught only by the at-head paths (LIVE_AMM=1, verify-lm-fork.ts). Either scope the comment to those, or have something at-head assert the allowlist on a schedule; the pinned assertion is really just a precondition guard for the fixture, which is still worth having for the clear error message.
3. process.env.AMM_PROVIDER is set and never restored (BStockLiquidatorFork.ts, in the live-aggregator loop). Both iterations write it and nothing clears it, so under LIVE_AMM=1 the last provider leaks into anything that runs after in the same process. Harmless in CI, where the block is skipped, but it is the kind of cross-test leak that is confusing to debug the one time it matters. A try/finally or passing the provider into getAmmSwap explicitly would settle it.
4. Two passthroughs left behind. deployVaiLiquidator() is now return liq; and the Liquid Mesh case opens with const liqLm = liq;. Both were doing real work before and now only preserve their call sites. Fine to keep for diff legibility, worth inlining whenever this file is next touched.
For the record on scope: I read the diff and checked chain state; I did not run the suite, and the LIVE_AMM=1 paths are unverifiable from here since they depend on live third-party APIs.
3 participants
What changed
0x5974Badab6911a78Ba15229045514C2C1bD42343at block 111264600.New tests
LIVE_AMM=1. These check the router each one returns still matches what we allowlisted.How to run