Feat/hub spoke pools - #559
Conversation
Spoke pools need supply and liquidation allowlists plus a per-market liquidation incentive. Mainline Comptroller backs the shared beacon on this and other chains and has no virtual functions, so the spoke gets its own implementation rather than a flag. This commit is the copy alone, so later commits show the feature diff by itself. The fork is byte-identical to its source apart from the solc metadata trailer, and carries the same storage layout.
- The spoke pool will not incentivise borrowing through Prime, so the integration is dead weight in a contract with 741 bytes of headroom against EIP-170. Removing it frees 1,097, leaving 1,838 for the allowlists and the per-market liquidation incentive. - The seven *Verify functions stay as no-ops instead of being deleted: ComptrollerInterface declares all of them, so omitting one leaves the contract abstract, and the vToken calls each against a comptroller with no fallback, so a missing function would revert the operation it belongs to. Selectors are unchanged. - Their parameters are unnamed so the build stays free of unused parameter warnings, which also means the @PARAM tags had to go, since solc validates those against the signature. - They are grouped in one section with the rationale stated once, rather than left between the pre-hooks where seven identical empty bodies read as noise. - The freed storage slot returns to __gap, so the contract occupies the same number of slots as the one it was forked from.
- Supply allowlist checks the account credited with the minted vTokens, not the payer. mintBehalf lets a third party fund a mint attributed to someone else, and metering the recipient is what bounds a market's supply. - Liquidation allowlist is pool wide rather than per market, because healAccount seizes across every market the borrower is in and so cannot attribute a seizure to a single one of them. - Each collateral market can carry its own liquidation incentive. The benchmark that routes an account between liquidateAccount and healAccount therefore becomes the sum of collateral value over incentive across markets, instead of one pool wide divisor. - liquidateAccount keeps the strict complement of healAccount's condition, so the two remain an exact partition and no account is accepted by both. - Every new setter carries its own ACM role string, so a governance proposal can grant one without granting the others. - Storage grows by five slots and the reserved gap shrinks to match, keeping the same slot footprint as the contract this was forked from.
- Values collateral at the low end of an asset's recent price window and debt at the high end while deviation protection is active, so a deviating print can only shrink borrowing capacity, never inflate it - Applies only where the collateral factor weights the position; the liquidation-threshold paths stay on spot because they route liquidations, and bounding them would distort eligibility - Replaces the snapshot's weight function pointer with an enum, because a function pointer cannot be compared and the price read has to know which weighting it is serving - Persists the price window on borrow and redeem so protection latches and its cooldown starts, instead of evaporating once the price returns - Optimizes this contract for size rather than gas: it no longer fits under EIP-170 at the default 200 optimizer runs, and the measured cost is under 1.5% on a borrow
- Deploys a dedicated implementation and beacon for the spoke pool - Keeps the pool out of poolConfig, or the standard scripts would deploy it first on the shared beacon and this script would skip it - Verifies constructor and initialize values before ownership transfer - Leaves oracle, pool listing and market config to the listing VIP
- Use the collateral market's liquidation incentive consistently in liquidateCalculateSeizeTokens and VToken._seize, so the protocol seize share and liquidator discount are calculated from the same value. - Make liquidationIncentiveMantissa() market-specific by resolving the incentive for msg.sender. - Apply the same market-specific incentive when validating VToken.setProtocolSeizeShare. - Add a floor of 1e18 + protocolSeizeShareMantissa in setMarketLiquidationIncentive to prevent configurations where the liquidator receives less collateral than the debt repaid. - Keep the pool-wide incentive internal as the default for markets without a custom incentive, initialized by PoolRegistry during registration. - Use effectiveLiquidationIncentive when reading the pool-wide/default value outside the pool market context. - Check that the market is listed before applying the new incentive bound. This ensures each market consistently uses its configured liquidation incentive and prevents protocol and liquidator seizure calculations from using conflicting incentive values.
- Cover allowlists, deviation bounded pricing, per market liquidation incentives and liquidation flows against real vTokens - Add hook coverage for caller guards, borrow caps, close factor, forced liquidation and the full action pause matrix - Cover market membership, rewards distributor wiring and max loop limits - Pin the storage layout against ComptrollerStorage, including the manually calculated trailing gap size - Assert ABI, events and role string parity with the shared implementation used by the lens and VIP calldata - Add UpgradedSpokeComptroller under contracts/test as the successor used by beacon upgrade tests - Skip the EIP 170 size assertion while the Yul optimizer is disabled, since the build is around 1.1 KB larger and is never deployed These tests focus on spoke specific changes and prevent regressions from the shared implementation.
- Split CollateralExceedsThreshold into CollateralCoversDebt and DebtExceedsClearableAmount, so healAccount and liquidateAccount stop reporting one selector for opposite conditions - Declare the fork's getters in SpokeComptrollerViewInterface, so integrators can read them without importing the implementation - Resolve the account's markets once per hook and pass the list to both the price and the protection state update - Accumulate maxClearableDebt only under the liquidation threshold weighting, the only one whose callers read it - Deploy the implementation without skipIfAlreadyDeployed and verify it as soon as it lands, so a source change is not silently skipped
- Cover listing, allowlists, bounded pricing, bad debt, low decimal markets and the supply flow against live bscmainnet state - Add the spoke adapter and its interfaces under contracts/test, so the suite runs the real integration path rather than a stand in - Record BSC's shanghai and cancun activation blocks in the fork config. Without them a recent fork runs the London EVM and every contract compiled for a later target reverts on PUSH0 or TSTORE
![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.
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.
A market listed with no incentive of its own falls back to the pool-wide value and carries the default 5% protocol seize share, so a floor of 1e18 leaves its liquidator with less collateral than the debt it repaid. Upstream stops at 1e18. The spoke now refuses anything below 1e18 plus that default share. - Add MIN_POOL_LIQUIDATION_INCENTIVE_MANTISSA and gate setLiquidationIncentive on it - Document the derivation and the case the constant does not cover - Rewrite the parity test on this bound as a deliberate divergence - Move the incentive floor tests to the new value - Park the pool-wide incentive above the market floor in the seize split test
- The package declares ISC and publishes contracts/ wholesale, so these four BUSL-1.1 headers shipped inside a permissively licensed package - BSD-3-Clause is what every other Solidity file in the repo carries; the BUSL headers arrived with the files rather than from a decision
- An isolated-pools VToken always returns NO_ERROR and reverts on failure, and no spoke pool lists a fee-on-transfer underlying, so the three VToken*Failed branches, the under-delivery check and the sub-one-vToken deposit guard had no coverage at all - Fakes the market and nothing else: the YieldGroup is minted from the implementation deployed on this chain and reached through depositResource / withdrawResource, so the adapter arrives by the same delegatecall the Hub's own deposit path takes - Extends the same treatment to the view branches no live market enters, including a zero total supply against a held balance, reserves above the whole backing, and a supply rate that overflows the uint64 return
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.
|
i dont see anything uncommon, but do have 2 points raising from ai, i checked make sense and seems inherited from existing codebase
|
|
Continuing the topic in group, we will need to deploy a new spoke pool registry and will need to update the addr in protocol share reserve as well. However, without any change to current PSR impl this will revert the existing isolated pool when sending out the revenue because of this check 
So we might need to update the logic of PSR a bit to unblock the existing isolated pool, tho its sunset at product level but lets at least make sure there is no unexpected revert onchain |
@fred-venus both points are correct and yes both are inherited from existing codebase 1. Pre-accrual liquidation snapshot
Fix: the accrue + 2. Fallback liquidation incentiveThe gap is real, but I would keep it out of the contract:
Worth noting the consequence there is liveness, not loss: if the invariant is violated the liquidator receives less than it repaid, so liquidations of that market stop rather than anything being drained. |
The batch liquidation entry point took its routing snapshot on stored balances and the last recorded price, then ran every order with the liquidity check skipped, so nothing re-tested eligibility on live state. healAccount already refreshed first, so the two paths decided on different views of the same position. - Pull the accrue plus updatePrice loop out of healAccount into a shared _refreshMarkets helper. - Call it from liquidateAccount before the snapshot. - Resolve the borrower's markets once and reuse the list for the trailing zero-borrow check. - Cover the ordering with a test that accrues past maxClearableDebt.
The registry is the directory every consumer iterates to answer which pools exist. A hub-funded pool whose supply, borrow and liquidation sides are each restricted to known accounts does not belong in the isolated-pools list, so it gets a registry of its own. - Add a deploy script for a second PoolRegistry instance, behind the chain's existing proxy admin, and construct the spoke implementation with it - Renumber the comptroller script and make it depend on the registry, so tag-selected runs pull that one in first - Grant the registry the six comptroller setters addPool and addMarket drive as the caller. The wildcard grants covering the isolated-pools registry name its address, so none of them carry over - Pin the ProtocolShareReserve trade in a fork suite. It holds one registry address, so pointing it here stops the existing isolated pools from taking income, and not pointing it here stops this pool - Widen maxStalePeriod on the main and pivot oracles in the fork fixture. The pinned block left about 77 seconds of budget, so which tests passed depended on how many transactions ran ahead of them
|
|
One more thing 
We probably want to poolLen as well to reflect DBO and per asset liquidation incentive |
also this Fixed: b261a7b |
- A spoke pool's bounded oracle, liquidation allowlist and per-market liquidation incentive had no place in the lens output, so every consumer read one as if it were an ordinary isolated pool - Probe for them with staticcall, so a comptroller that has no answer reports absence instead of reverting the whole read, and encode with abi.encodeCall so a signature change fails the build - Append the fields rather than reordering, so a decoder built against the old shape still reads the fields it knows - Archive the superseded revision as PoolLensR3 next to R1 and R2, and note in the deploy script that the record has to be archived before a redeploy overwrites it
enterMarkets reads msg.sender, so a router that supplies through mintBehalf enters itself instead of the supplier. That forces a first time supplier into two transactions. - Add enterMarketBehalf(address,address), restricted by AccessControlManager - Take a single market rather than an array, which keeps the contract under EIP-170 with 25 bytes to spare. The router loops instead. - Leave enterMarkets untouched, so self entry stays permissionless - Cover the new function in the membership, parity and role string tests
| liquidationIncentive: comptrollerViewInstance.liquidationIncentiveMantissa(), | ||
| minLiquidatableCollateral: comptrollerViewInstance.minLiquidatableCollateral() | ||
| minLiquidatableCollateral: comptrollerViewInstance.minLiquidatableCollateral(), | ||
| deviationBoundedOracle: _probeAddress( |
There was a problem hiding this comment.
is this to decide whether its isolated pools or spoke pools ?
i am thinking why dont we just redeploy a new PoolLens with clean implementation, right now we already have new poolRegistry, its natural to think we should have a poolLens as well.
So basically 2 sets of contracts in parallel so they dont confuse ppl
There was a problem hiding this comment.
also for VTokenBeacon lets deploy a new one, so that in the future if ever we do the change its not gonna impact existing isolated pools
There was a problem hiding this comment.
The lens isn't bound to a registry, it takes poolRegistryAddress as a call argument, and it's a plain non-proxy contract we version by redeploying (R1/R2 archived, this PR archives R3), so the separation already exists at the address level. But fair point on the confusion, I'll add a separate SpokePoolLens and leave PoolLens for isolated. That also drops the staticcall probing.
There was a problem hiding this comment.
Agreed, will deploy a separate VToken beacon for the spoke pool.
There was a problem hiding this comment.
- Spoke pools are listed in a registry of their own, so reading them gets its own lens rather than more fields on the shared one - Keeps the deployed PoolLens untouched: the spoke fields added to it are reverted, along with the revision that archived them, so the networks that carry no spoke pool need no redeploy - Carries the whole PoolLens surface too, under the same names and struct shapes wherever the shape is unchanged, so a consumer reads a spoke pool from one address - Declares markets and isForcedLiquidationEnabled on the spoke view interface, so the liquidation threshold is not dropped as trailing return data - Checks every mirrored read against PoolLens for the same pool, on a fixture that carries real bad debt and reward state
- upgradeTo moves every proxy behind a beacon in one call, so sharing VTokenBeacon would tie a VToken change for the spoke pool to every isolated market on the chain, and the other way round - The implementation is the same VToken with the same immutables, so the markets behave identically until an upgrade separates them - Point the fork fixture's markets at the new beacon, which is where the separation is decided: the beacon is inert until they use it - Extract the deploy scripts' verify helper so both spoke scripts share it
- 009-deploy-vtokens.ts cannot be reused: it iterates the shared pool config and points every market it builds at the shared VToken beacon, so the spoke markets need a script and a config of their own - keep that config out of globalConfig, otherwise 008 and 009 stand the pool up behind the shared beacons and claim its deployment names first - mirror the risk parameters of the isolated stablecoin markets on the same network, since the spoke pool restricts who may supply, borrow and liquidate rather than taking more risk per market - name each market VToken_<symbol> with the pool id as the symbol suffix, so a spoke market reads apart from an isolated one the way Comptroller_HubSpoke already does - read the comptroller, underlying and rate model back off the chain before the run reports success, since a market built against the wrong one can only be redeployed - add USDC to the bsctestnet token list, which carried no entry for it
|
4 participants
What
Adds
SpokeComptroller, a fork ofComptrollerfor hub-funded spoke pools, behind its own beacon. Two deploy scripts: the comptroller, and a secondPoolRegistryinstance this pool registers in instead of the isolated-pools one. No existing pool is affected.PoolLensis the only shared contract touched: four appended fields for the spoke-only state, with the superseded revision archived asPoolLensR3.Nothing is wired or listed by this PR. The oracle,
addPool, the ACM grants and the markets all belong to the listing VIP.Decisions
Comptroller.Comptrolleris shared by every pool in this repo, on this chain and others. Spoke-only policy does not belong in it, and a shared-beacon upgrade would put that policy on all live pools at once. The fork has its own beacon, so blast radius is zero.SpokeComptrollerInterface is ComptrollerInterface, so both comptrollers share one definition of the function surface and it cannot drift. Only the events and errors are declared in the spoke interface. Separate interface forSpokeComptrolleralso works*Verifyhooks kept as empty no-ops.VTokencalls all seven. Removing them would force a separateVTokenimplementation for this pool, so keeping them lets the pool reuse the sharedVTokenbeacon.runs: 30for this contract instead of moving code into an external library. With bounded pricing added, the contract no longer fits under EIP-170 at the default 200 runs. Measured:runs: 30costs +1,901 gas on a borrow (under 1.5%), while a linked library costs at least 2,600 gas for the coldDELEGATECALLalone, so it was both more expensive and a larger architectural change. The library stays the fallback if headroom ever runs out, since it frees kilobytes rather than bytes.ComptrollerStorage.PoolRegistryinstance of its own, not a row in the isolated-pools registry. That registry is the directory every consumer iterates to answer which pools exist, so a pool whose supply, borrow and liquidation sides are all restricted to known accounts would need a special case in each of them. A second instance also keeps the two products independently upgradeable and permissioned. The chain's existingDefaultProxyAdminis reused, so there is no second admin to track.poolConfig. The standard scripts iterate that list and would deploy this pool on the shared beacon under the same artifact name; this script would then skip it as already deployed, leaving a pool that looks correct but runs the wrong implementation.New behaviour
liquidateAccountandhealAccountenterMarketBehalf(address,address), so a router supplying throughmintBehalfenters the supplier rather than itself. ACM-gated, one market per call to stay under EIP-170.enterMarketsis untouched, so self entry stays permissionlessliquidateAccountrefreshes interest and prices before its routing snapshot, ashealAccountalready did, so the two do not decide on different views of the same positionPoolLensreports the spoke-only state: bounded oracle and liquidation allowlist per pool, liquidation incentive and supply allowlist per market. Probed bystaticcall, so a comptroller without them reports absence instead of reverting the read, and appended rather than reordered so an old decoder still worksKnown Gaps — Deferred
None of the items below are permissionless. Every one sits behind
onlyOwneror theAccessControlManager, so they are governance and configuration guardrails rather than directvulnerabilities. They are recorded here so the listing VIP and any future change to this contract
start from a known baseline.
1. DBO and Comptroller oracles are not cross-checked
DeviationBoundedOracle.RESILIENT_ORACLEis immutable, whileSpokeComptroller.oracleis settablethrough
setPriceOracle. Nothing verifies the two refer to the sameResilientOracle.If they ever diverge, borrowing power is priced through the bounded oracle while liquidation routing
and seize math are priced through the comptroller's own oracle, with no revert and no event to make
it visible.
Action: the listing VIP verifies the pairing at deployment. A future change could enforce it in
setPriceOracleandsetDeviationBoundedOracle, at a cost in contract size (see item 7).2. No maximum limit on liquidation incentives
Both
setLiquidationIncentiveandsetMarketLiquidationIncentiveenforce only a floor. Neitherrejects a value that is too high.
This is a risk-parameter decision and is intentionally left to governance, matching how the shared
Comptrollertreats it. One consequence is specific to this fork and worth being aware of whensetting the value: the per-market incentive divides into
maxClearableDebt, so an unusually highincentive on one collateral market shrinks that figure for every borrower holding it, which shifts
those accounts from
liquidateAccounttowardhealAccountand therefore toward bad debt.Action: none in code. Set per-market incentives deliberately.
3. Unset DBO oracle causes an unnamed revert
Borrow and redeem fail closed while
deviationBoundedOracleis unset, because_updateProtectionStatescalls into the zero address. The revert carries no named error, so thefailure is correct but opaque.
The window in which this is reachable is between proxy initialization and the listing VIP, and the
VIP sets the oracle before the pool serves any action.
Action: none. Failing closed is the intended behaviour; only the error message is unhelpful, and
only in a window nobody transacts in.
4. An empty liquidation allowlist can freeze liquidations
Enabling
isLiquidationAllowlistEnabledwith no approved liquidators blocks every seizure in thepool. This also covers
healAccount, so the keeper relied on to record bad debt has to beallowlisted too, not just the liquidators.
Action: the listing VIP adds every liquidator and bad-debt keeper before enabling the allowlist,
in the same transaction.
5.
PoolData.liquidationIncentivestill reports the pool-wide valueliquidationIncentiveMantissa()resolves againstmsg.sender, so a lens reading it receives thepool-wide value, and that is what
PoolData.liquidationIncentivecontinues to mean. Changing themeaning of an existing field would break current consumers.
Action: none. The per-market value is now reported as
VTokenMetadata.liquidationIncentiveMantissa, so a consumer reading a spoke pool takes that field.The on-chain seize amount was never affected:
liquidateCalculateSeizeTokensandVToken._seizeboth resolve the collateral market's own incentive.
6.
maxLoopsLimitneeds re-validationBorrow, redeem and transfer now walk an account's markets twice per call, once for
_updatePricesand once for_updateProtectionStates, on top of the reward-distributor loops. Thedeployed limit of 100 was chosen before the second walk existed, and neither oracle loop is bounded
by
_ensureMaxLoops.Action: measure the worst-case borrow against a realistic market count and adjust the deployed
value if the gas ceiling, rather than the configured limit, is what actually binds.
7. Contract size
SpokeComptrollercompiles to 24,551 bytes against the 24,576-byte EIP-170 limit, leaving 25bytes, at the
runs: 30override this contract already needs in order to fit. Any further changehas to be measured before it is written.
Moving the liquidity snapshot into an external library would free kilobytes rather than bytes, but it
has been left out for now to keep
SpokeComptrolleraligned with the ILComptrollerit was forkedfrom.
One related note: all four compiler blocks disable the Yul optimizer under
CI. Compiled that waythe contract measures 25,657 bytes, over the limit, while the production build is under it. Any size
check that runs in CI is therefore measuring a binary that is never deployed.
Listing vip important notes
enterMarketBehalf,setMarketLiquidationIncentive,setSupplyAllowlistEnabled,setAllowedSupplier,setLiquidationAllowlistEnabled,setAllowedLiquidator), plus the inheritedsetActionsPaused(address[],uint256[],bool)trap where the registered string deliberately disagrees with the real ABI (uint8[]). Deriving that one from the ABI gives you a grant that never matches.1.05e18, now enforced, so this one fails loudly.setTokenConfigruns with zero protection and no revert; the caching-flag pairing; theRESILIENT_ORACLEpairing.addMarket, with the YieldGroup granted in the same VIP.healAccounttoo.0-is-a-sentinel rule, and the ordering trap where lowering the pool-wide value after a market's seize share was raised is never re-checked.SpokePoolRegistry:acceptOwnership, since the deploy script only nominates, plus ACM grants in both directions. Existing wildcard grants name the isolated-pools registry's address, so none carry over andaddPoolreverts without new ones. The exact list is in the deploy script's closing comment.poolRegistryaddress. Pointing it atSpokePoolRegistrystops the existing isolated pools from taking income; not pointing it there stops this pool. The multi-registry change ships from another repo and has to be live first. Pinned intests/hardhat/Fork/HubSpoke/psrRegistryConflict.ts.enterMarketBehalf: grant the role only to a router that passes its own caller asaccount. Anything else can enter markets for anyone.