feat: Add BabylonSpoke - #1345
Conversation
![aave-code-review[bot]](https://avatars.githubusercontent.com/u/47617460?s=60&v=4){kind=small}
e1267b4 to
0cbf5a1
Compare
3af2dc9 to
e3b1a4c
Compare
| uint256 debtToCover, | ||
| bool receiveShares | ||
| ) external nonReentrant { | ||
| ) external virtual nonReentrant { |
There was a problem hiding this comment.
We're marking virtual only on the functions BabylonSpoke overrides (this one and _validateSetUsingAsCollateral), which is not consistent from the Spoke perspective. My preference would be to take the opportunity and mark all relevant functions virtual — opinions?
There was a problem hiding this comment.
im ok with all virtual tbh
| } | ||
|
|
||
| /// @dev Restricts users to at most one registered collateral. | ||
| function _validateSetUsingAsCollateral( |
There was a problem hiding this comment.
This caps the count but deliberately does not check the reserve against managedCollateralReserveId: a user whose single registered collateral is another reserve is unreachable by liquidationCall, which is only safe while every other reserve keeps a zero collateral factor.
This hook does not receive the reserve id, so enforcing the id here would need a reserveId parameter added to the canonical hook.
The alternative is overriding the external setUsingAsCollateral (made public virtual for super) — which leaves the canonical spoke bytecode the same
Question for reviewers: keep the count-only check, or enforce the managed reserve id via the external override? The override is proven bytecode-free for canonical instances, so it's purely a question of whether we want the stricter invariant.
There was a problem hiding this comment.
Should a full reserve for loop using positionStatus.isUsingAsCollateral() be good for this?
The detailed change might be a function _validateBabylonPositionStatus() where all other reserves (!= managedCollateralReserveId) are enforced 0. It might create a gas cost overhead but should be more intuitive.
There was a problem hiding this comment.
Implemented: BabylonSpoke now overrides setUsingAsCollateral (made public virtual on the canonical Spoke, verified byte-identical) and requires reserveId == managedCollateralReserveId when enabling, with a dedicated UnsupportedCollateralReserve error. No loop needed since only the managed id can ever be registered.
The count check in _validateSetUsingAsCollateral stays on top: it still matters after a config change, when a user may hold the previously managed reserve while registering the new one.
| uint256 debtReserveId = params.debtReserveIds[i]; | ||
| require(params.debtToCoverAmounts[i] > 0, ISpoke.InvalidDebtToCover()); | ||
| require(!reserves.get(debtReserveId).flags.paused(), ISpoke.ReservePaused()); | ||
| // quadratic duplicate scan; the list is bounded by the user's borrowed reserves |
There was a problem hiding this comment.
Duplicates are rejected with a quadratic scan: the list is bounded by the user's borrowed reserves, so N should be small, which is why I preferred this over a temporary bitmap (which needed either inline bit manipulation or new PositionStatusMap helpers). Flagging in case anyone prefers a different mechanism here.
| collateralAssetId: params.collateralAssetId, | ||
| collateralAssetUnit: MathUtils.uncheckedExp(10, params.collateralAssetDecimals), | ||
| collateralAssetPrice: IAaveOracle(params.oracle).getReservePrice(params.collateralReserveId), | ||
| liquidationBonus: LiquidationLogic.calculateLiquidationBonus({ |
There was a problem hiding this comment.
We compute lb only once, using the entry health factor, instead of recomputing HF after each debt reserve liquidated and recomputing lb. This should be fine, as lb will be the same or higher in this scenario, which means that we do not incentivise liquidaton splitting.
There was a problem hiding this comment.
yea and also bc we might be performing a liquidation when position has become healthy due to previous liqs
There was a problem hiding this comment.
also bc we might be performing a liquidation when position has become healthy due to previous liqs
yes although in this case you could update the lb calculation fn to return min lb is healthy
| params: liquidateDebtReservesParams | ||
| }); | ||
|
|
||
| emit IBabylonSpoke.BabylonLiquidationCallSummary({ |
There was a problem hiding this comment.
what do you think about the current event structure? one summary event + one event per each debt reserve.
| }); | ||
|
|
||
| return | ||
| collateralUserPosition.suppliedShares == 0 && |
There was a problem hiding this comment.
we don't check activeCollateralCount bc we restrict collateral count to 1 already in the BabylonSpoke.
There was a problem hiding this comment.
I think we can replace collateralUserPosition.suppliedShares == 0 with liquidateDebtReservesParams.suppliedShares == collateralSharesRemoved
| assetId: params.collateralAssetId, | ||
| sharesToLiquidate: liquidationAmounts.collateralSharesToLiquidate, | ||
| sharesToLiquidator: liquidationAmounts.collateralSharesToLiquidate, | ||
| liquidator: params.liquidator, |
There was a problem hiding this comment.
this means that we execute multiple collateral transfers to the liquidator. I preferred this option over either:
- passing address(this) here and then send the transfer at the end
- deferring removing collateral towards the end and removing in one go bc then we would repay debt before removing collateral, which we know is problematic.
we have a third approach, which is same as 2, but we also defer debt repayment such that we still remove coll before repaying debt, but that would increase complexity.
worth noting that 2 might not be problematic is the collateral has a share price of 1, but I don't want to make assumptions about the share price in this implementation (although we could ofc, if we want).
thoughts?
There was a problem hiding this comment.
From Babylon's point of view, if Aave does not enforce the price assumption, the adapter will have an extra layer of rounding to make sure that it receives enough collateral for a vault.
This though should not be an issue and the rounding should return an output exactly the same as the input, but seems to be addressed a lot more easily on the Spoke side.
There was a problem hiding this comment.
We cannot enforce the supply share price assumption in the spoke, as the hub can let other spokes borrow the same asset in theory
There was a problem hiding this comment.
agree, still believe it's safe to assume for this collateral (it's only on the hub bc we didnt want to build a custom hub for this collateral) but regardless the current option of multiple transfer seems fine to me (we're non reenetrent so there's no execution context handover and the underlying token does not have extra weird rounding on each transfer for allowance or balance)
There was a problem hiding this comment.
FYI Tests CI time goes up from 1h40 to 2h40
159e029 to
d215a49
Compare
d215a49 to
9702e1f
Compare
9702e1f to
6a1c832
Compare
avniculae
changed the title
6a1c832 to
3e2411d
Compare
3e2411d to
c5dd2f2
Compare
| .min(params.suppliedShares - totalCollateralSharesRemoved); | ||
| // no further repayments once the removal cap is consumed; the first repayment always runs, | ||
| // so debt can be liquidated even when the corresponding collateral amount to receive is zero | ||
| if (maxRemovableShares == 0 && i > 0) break; |
There was a problem hiding this comment.
wdyt about this condition?
| }); | ||
| } | ||
| if (deployInputs.spokeLabels.length > 0) { | ||
| if (deployInputs.spokeLabels.length > 0 || deployInputs.babylonSpokeLabels.length > 0) { |
There was a problem hiding this comment.
I think it is bc the roles for the standard spoke fns (that are inherited by Babylon spoke) should still be configured, wdyt?
| /// @param collateralAmountRemoved The amount of collateral removed, expressed in asset units. | ||
| /// @param collateralSharesLiquidated The amount of collateral shares liquidated. | ||
| event BabylonLiquidationCall( | ||
| uint256 indexed debtReserveId, |
There was a problem hiding this comment.
still fine i think to emit managedCollReserveId
|
|
||
| return | ||
| collateralUserPosition.suppliedShares == 0 && | ||
| userPositionStatus.nextBorrowing(params.reserveCount) != PositionStatusMap.NOT_FOUND; |
There was a problem hiding this comment.
just borrowCount > 1 is cheaper i think
| ) | ||
| .min(params.suppliedShares - totalCollateralSharesRemoved); | ||
| // no further repayments once the removal cap is consumed; the first repayment always runs, | ||
| // so debt can be liquidated even when the corresponding collateral amount to receive is zero |
There was a problem hiding this comment.
why do we want to allow the first repayment again sorry?
There was a problem hiding this comment.
for the same reason we allow no collateral to be liquidated while debt is being repaid. in an edge case scenario, the supply share price can be so high that one share is worth more than the entire debt. if we don't allow first repayment here, we would basically disallow 0 collateral liquidations
| sharesToLiquidate: liquidationAmounts.collateralSharesToLiquidate, | ||
| sharesToLiquidator: liquidationAmounts.collateralSharesToLiquidate, | ||
| liquidator: params.liquidator, | ||
| receiveShares: false |
There was a problem hiding this comment.
if we receive shares, anyone can bypass the liquidation manager (ie the adapter) and liquidate collateral up to any granularity
| // strict inequality is mandatory given rounding | ||
| if (params.debtToCover < premiumDebtRayToLiquidate.fromRayUp()) { |
There was a problem hiding this comment.
dont we lose out on a dust premium case (premium < 1 ray anyway is fine to discard?)
There was a problem hiding this comment.
I don't think we lose any case here, this is btw the same code as the canonical spoke. please think through it again and make sure it's correct, will do the same
| /// @notice Spoke variant for the Babylon integration: liquidations are restricted to a configured | ||
| /// liquidation manager and sized by a collateral cap instead of a target health factor. Users can | ||
| /// register at most one reserve as collateral. | ||
| abstract contract BabylonSpoke is IBabylonSpoke, Spoke { |
There was a problem hiding this comment.
should we override _validate config methods to ensure liquidationFee is zero and no additional coll
3 participants
Adds a
BabylonSpokevariant for the Babylon integration (vaultBTC collateral).Liquidations are restricted to a configured liquidation manager and sized by a collateral removal cap instead of the target health factor. One call repays multiple debt reserves: each pair repays up to the given amount (premium first, no dust validation, no target-HF sizing), removes collateral priced by the canonical bonus formula, and emits a per-pair event, followed by a summary event with the totals.
The removal cap is tracked in asset terms and converted into removable shares before each repayment, so the total removed collateral tracks the cap as closely as rounding allows at any supply share price. When the priced removal exceeds the cap, the repayment is resized to exactly consume it. The sizing and rounding maths are proven with z3 scripts in
tests/misc/z3/. Debt reserves the user no longer borrows are skipped, so front-running liquidations cannot block the call. The liquidation fee is never charged: the liquidator receives the full removed collateral, always in underlying assets.Users can register only the managed collateral reserve as collateral, one at a time. The manager and the managed collateral reserve are set through
updateBabylonLiquidationConfig, called directly by the executor rather than through the SpokeConfigurator. The config engine and the deployment engine support the new spoke, gated by the granularBABYLON_SPOKE_CONFIGURATOR_ROLE.BabylonLiquidationLogicmirrors the canonicalLiquidationLogicstructure and compiles without via-ir. CanonicalSpokeonly gainsvirtualmarkers, keeping canonical instances bytecode-unchanged.BabylonSpokeInstancecompiles at via-ir/450, the highest runs value that fits EIP-170 (121 bytes of margin).The liquidation suites run against a dedicated fourth spoke deployed through the engine (
BabylonBase), with the managed collateral non-borrowable and fee-free as in production. They mirror the canonical fuzz matrix (four shapes across nine config variants) and scenarios, and gas snapshots cover all user actions plus the liquidation paths.