feat(morpho-sdk): support BluePublicAllocator reallocations - #919
feat(morpho-sdk): support BluePublicAllocator reallocations#919prd-carapulse[bot] wants to merge 35 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd445eceaa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
295ab89 to
a20a817
Compare
a20a817 to
0121d44
Compare
6ae7ded to
11e1acc
Compare
11e1acc to
15112a8
Compare
…nto hermes/public-allocator-v2-sdk
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 781059ef9d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
0xbulma
left a comment
There was a problem hiding this comment.
Automated review — /facets:pr-review-local (8 parallel lenses)
Scope: 101 file(s), 71a60b23..781059ef · Findings: 26 (0 critical, 4 high, 16 medium, 6 low)
Lenses: correctness · error-handling · docs · tests · simplification · performance · web3 · release-integrity — 0 agent failures, 0 findings dropped by the scope filter.
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 4 |
| Medium | 16 |
| Low | 6 |
What to look at first
- The two
.changesetfindings block a release as-is — both argue this is amajor, not aminor: a requiredreallocationPenaltyAssetsfield lands on four exported action interfaces, andVaultV2MorphoMarketV1AdapterV2.ids()is retyped to a readonly tuple with no compatibility alias. maxPenaltydefaults to unbounded — the only real money-movement concern. Default discovery will plan against a vault whose curator setpenaltyup to WAD, and the resulting approval is invisible in the wallet (unlike the V1 fee, which lands intx.value).vaultV2BlueReallocationData.tscarries 9 of the 26 findings, concentrated in the per-probe deep clone in the cap-fit binary search.
Each comment carries the lens that raised it; where more than one lens is listed, they found it independently — that cross-agent agreement is the strongest real-vs-false signal here.
Posted as event=COMMENT — not an approval, and not a change request.
|
|
||
| while (lower < upper) { | ||
| const assets = (lower + upper + 1n) / 2n; | ||
| const postState = data.cloneWithPublicReallocation({ |
There was a problem hiding this comment.
[HIGH] (confidence: 90%) · performance, correctness
WHAT: The cap-fit binary search calls data.cloneWithPublicReallocation(...) once per probe, and that helper starts with this.clone() (line 1157) which deep-copies the ENTIRE snapshot — every Market, every AccrualVaultV2 (including nested AccrualVault + every AccrualPosition for MorphoVaultV1 adapters), every allocation record and every config map — then runs setMarket up to twice, each of which rebuilds every adapter view of every vault. upper is seeded from reallocation.assets, so for realistic token amounts (1e12–1e21 base units) the search runs ~40–70 probes, i.e. 40–70 full-state deep clones per raw candidate. That is multiplied by the number of raw candidates per vault (idle + adapters x source markets), by the number of vaults, and again by every iteration of the enclosing while (true) greedy loop (line 783), which recomputes all candidates from scratch after each accepted reallocation. getPublicReallocationLiquidity and getAvailableLiquidityToUtilization run the same loop just to sum assets, so a read-only liquidity metric pays the full cost. This is a synchronous, main-thread-blocking planner.
FIX: make the probe predicate cheap instead of cloning the world — the search only reads postState.getAllocation(vault, targetIds[i]), postVault._totalAssets and postState.firstTotalAssets[vault], so extract a private simulateCapFit(reallocation, assets) that copies only the affected vault's allocation records plus the source/target markets and returns those values, leaving cloneWithPublicReallocation for the accepted amounts only. Additionally seed upper from VaultV2Utils.allocationHeadroom(...) so the search starts from an analytic bound rather than the full asset amount.
| data.getPublicAllocatorConfig(vaultAddress); | ||
| if ( | ||
| !isAddressEqual(publicAllocatorConfig.vault, vaultAddress) || | ||
| (options.maxPenalty != null && |
There was a problem hiding this comment.
[HIGH] (confidence: 75%) · web3
WHAT: maxPenalty is opt-in and defaults to no limit (options.maxPenalty != null && publicAllocatorConfig.penalty > options.maxPenalty), and the only other bound is reallocation.penalty > MathLib.WAD in validateVaultV2BlueReallocations (helpers/validate.ts:446). So the default discovery path will plan BluePublicAllocator calls for a vault whose curator has set penalty anywhere up to WAD, and the entity then silently emits a loan-token approval for the sum of ceil(assets x penalty / WAD) — up to 100% of the reallocated amount — which the user signs as an ERC-20 approval buried inside the bundle (unlike the V1 fee, which surfaces in tx.value and is therefore visible in the wallet). A curator who raises setPenalty before the user's tx lands extracts that amount as a donation to their own vault. Every other user-cost parameter in this SDK is hard-capped (MAX_SLIPPAGE_TOLERANCE = 10%, enforced by validateSlippageTolerance); this one is not.
FIX: give VaultV2BluePublicAllocatorOptions.maxPenalty a conservative non-null default (e.g. a new exported MAX_REALLOCATION_PENALTY constant in helpers/constant.ts) so undiscovered high-penalty vaults are skipped by default, and additionally reject penalty above that ceiling in validateVaultV2BlueReallocations with InputExceedsMaxError so hand-built plans cannot bypass the planner's filter.
| publicAllocatorConfig.penalty > options.maxPenalty) | ||
| ) | ||
| return; | ||
| const activeAdapters = data.activeAdapters[vaultAddress]; |
There was a problem hiding this comment.
[LOW] (confidence: 74%) · error-handling
WHAT: const activeAdapters = data.activeAdapters[vaultAddress]; if (activeAdapters == null) return; silently drops the vault when the snapshot has no activeAdapters entry, unlike every other missing-state lookup on this class (getMarket, getVault, getAllocation, getPublicAllocatorConfig, getMarketPublicAllocatorConfig), which all throw a typed UnknownReallocation*Error. Because InputVaultV2BlueReallocationData.activeAdapters is an optional public input, a hand-built snapshot that omits it yields an empty plan with no diagnostic at all — not even a swallowed typed error.
FIX: add a getActiveAdapters(vault) accessor that throws a typed error (reuse UnknownReallocationVaultError or add a dedicated class) when the entry is absent, and reserve an empty Set for the 'fetched, none active' case, so the distinction survives once the surrounding _try is narrowed.
| adapter.shares, | ||
| ); | ||
|
|
||
| return adapter; |
There was a problem hiding this comment.
[LOW] (confidence: 60%) · correctness
WHAT: cloneAdapter falls through to return adapter; for any IAccrualVaultV2Adapter that is not one of the three known classes, so an integrator-supplied adapter object is shared by reference across every snapshot produced by clone() / cloneWithPublicReallocation() — breaking the class's documented contract that "Constructor inputs are cloned. Every simulated reallocation returns a new instance."
FIX: either shallow-copy the unknown adapter (e.g. via Object.assign(Object.create(Object.getPrototypeOf(adapter)), adapter)) before returning it, or reject unsupported adapter types with a typed error so the aliasing cannot happen silently.
jinmel
left a comment
There was a problem hiding this comment.
The patch contains breaking public API changes released under minor versions, leaves the new deployment registry entries incomplete, and silently mishandles negative penalty limits. These issues should be fixed before merging.
| (options.maxPenalty != null && | ||
| publicAllocatorConfig.penalty > options.maxPenalty) |
There was a problem hiding this comment.
[P2] Reject negative maximum penalties
When maxPenalty is negative, every nonnegative allocator penalty exceeds it, so this branch silently filters out all eligible vaults and can turn otherwise valid discovery or planning requests into empty results or an insufficient-liquidity error. Validate this option up front with NegativeInputError, as prescribed for nonnegative values by types/AGENTS.md.
Stack
Why
Blue flows currently support shared-liquidity reallocations only through PublicAllocator V1. Vault V2 integrations need to invoke BluePublicAllocator market-to-market and idle-to-market reallocations before borrow, collateral-plus-borrow, loan-asset withdraw, and refinance operations without inventing a deployment address or duplicating the target market in consumer inputs.
What changed
VaultReallocationV1 callers and introduced an additiveBlueReallocationunion.reallocateandallocateFromIdle, plus dispatcher and bundle value accounting.skipRevert: falsein high-level flows.API shape
The target
MarketParamsare always derived from the enclosing Blue action. No registry address is assumed because BluePublicAllocator has no registered deployment.Verification
pnpm --filter @morpho-org/morpho-sdk test --run src/bundler/actions.test.tsfailed with 2 missing BluePublicAllocator encoder tests; dispatcher/value RED later failed with 3 expected missing-dispatch failures.pnpm --filter @morpho-org/morpho-sdk buildpassed.pnpm lint:cipassed (Biome 634 files, JSDoc self-check 41/41, address lint).git diff --checkpassed.Dependent audit
wdk-protocol-lending-morpho-evmandliquidity-sdk-viemdirectly depend on morpho-sdk, but neither requires source or peer-range changes for this additive API; no dependent bump is required.Requested by: <@U03L0SC1JUR>