Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2835 +/- ##
==========================================
+ Coverage 49.21% 51.64% +2.43%
==========================================
Files 112 113 +1
Lines 4844 4914 +70
Branches 1343 1361 +18
==========================================
+ Hits 2384 2538 +154
+ Misses 2456 2372 -84
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sparrowDom
left a comment
There was a problem hiding this comment.
Not done yet left comments inline
naddison36
left a comment
There was a problem hiding this comment.
I think we need to whitelist what strategies the RebalancerModule can automatically deposit/withdraw to/from. I'm mostly worried about AMO strategies being accidentally being called without a vault value checker.
sparrowDom
left a comment
There was a problem hiding this comment.
Left some comments inline: This code is much easier to follow now. Thanks it is a great improvement
| ]; | ||
|
|
||
| // Return the action amount, capping cross-chain moves at the bridge limit | ||
| const actionAmount = (a) => { |
There was a problem hiding this comment.
nit: maybe cappedAmount would be a better name
| // All withdrawals are same-chain: freed USDC lands in the vault immediately, | ||
| // so withdrawals and deposits can be batched into a single transaction. | ||
| await executeTx(() => | ||
| rebalancerModule.processWithdrawalsAndDeposits( |
There was a problem hiding this comment.
nit: the on-chain contract could have just the processWithdrawalsAndDeposits function exposed. And have empty arrays passed when there would be no deposits or no withdrawals.
There was a problem hiding this comment.
That's a good point, the safe module already behaves that way when you call that processWithdrawalsAndDeposits method. Will drop the other two methods
There was a problem hiding this comment.
It simplified the code even further, thank you: 4a0a253
| shortfall, | ||
| constraints | ||
| ) { | ||
| const totalRebalancable = strategies.reduce( |
There was a problem hiding this comment.
Should minDefaultStrategyBps be excluded from totalRebalancable?
There was a problem hiding this comment.
I believe that'll complicate the script further. If we are subtracting that here, we should also subtract it from the defaultStrategy's balance when we query it. Otherewise, if the default strategy has less than the amount we subtract here, it might end with us having a few more conditional statements (like subtracting it from other strategies). I'm not sure if that's gonna work
| /** | ||
| * Compute total capital minus reserved amounts (shortfall + minVaultBalance). | ||
| */ | ||
| function _computeDeployableCapital( |
There was a problem hiding this comment.
This code is much better readable from the last time. Great improvement 👍
contracts/utils/rebalancer.js
Outdated
|
|
||
| const sorted = [...strategies] | ||
| .filter((s) => s.address !== defaultStrategy.address) | ||
| .sort((a, b) => (targets[b.address].gt(targets[a.address]) ? 1 : -1)); |
There was a problem hiding this comment.
So the sort mechanics here is to sort by the amount to be deposited. Would it make sense to sort by the APY strategy is earning instead?
Otherwise we might always deduct from the strategy having the largest amount deposited which might also be the one earning the highest APY.
There was a problem hiding this comment.
I thought of doing it by APY at first. But then if we are allocating it by APY, the lower APY strategies will have less liquidity. So, it means that they may not have enough liquidity to fund from a single strategy. So, it'll involve multiple withdraws from lower APY strategy, which would only make it gas expensive
contracts/utils/rebalancer.js
Outdated
| .filter((a) => a.action === ACTION_DEPOSIT) | ||
| .sort((a, b) => b.apy - a.apy); | ||
|
|
||
| for (const c of deposits) { |
There was a problem hiding this comment.
nit: instead of c this could be named deposit
contracts/utils/rebalancer.js
Outdated
| } | ||
|
|
||
| const amt = c.delta.gt(budget) ? budget : c.delta; | ||
| budget = budget.sub(amt); |
There was a problem hiding this comment.
Shouldn't this budget subtraction happen after the additional if statement checks below that result in a continue? Budget is reduced even when the action can be invalidated by setting it to ACTION_NONE
| const hasApprovedWithdrawals = result.some( | ||
| (a) => a.action === ACTION_WITHDRAW | ||
| ); | ||
| if (!hasApprovedWithdrawals && shortfall.gt(0)) { |
There was a problem hiding this comment.
could it be that there are approved withdrawals that don't withdraw enough to cover the shortfall?
There was a problem hiding this comment.
Right now, the rebalancer cannot directly move between strategies. So, if it has to move between strategies, it has to withdraw to Vault first and then to the other strategy (either in a single run or across multiple runs). So, if there's a withdraw action, it'll always cover the withdrawal shortfall
naddison36
left a comment
There was a problem hiding this comment.
I think the biggest issue with the current approach is it doesn't take into account the impact of reallocating funds. The danger is it will reallocate based on the current net APY, after the reallocation the APYs change and the funds are reallocated back. We don't want to end up in a situation where funds are reallocated back and forth.
| const ACTION_NONE = "none"; | ||
|
|
||
| // Human-readable ABIs for contracts we interact with | ||
| const vaultAbi = [ |
There was a problem hiding this comment.
nit: this can be imported from an existing ABI file
const vaultAbi = require("../abi/vault.json");| "function withdrawalQueueMetadata() external view returns (tuple(uint128 queued, uint128 claimable, uint128 claimed, uint128 nextWithdrawalIndex))", | ||
| ]; | ||
|
|
||
| const strategyAbi = [ |
There was a problem hiding this comment.
nit: I think it would be cleaner if the cross chain strategy ABI was added to the ../abi/ folder. Then the following could be used
const strategyAbi = require("../abi/crossChainStrategy.json");| "function isTransferPending() external view returns (bool)", | ||
| ]; | ||
|
|
||
| const erc20Abi = [ |
There was a problem hiding this comment.
nit: this can be replaced with
const erc20Abi = require("../abi/erc20.json");| ]); | ||
|
|
||
| // Reserve any available vault balance for pending withdrawals | ||
| let shortfall = queueMeta.queued.sub(queueMeta.claimable); |
There was a problem hiding this comment.
this should be moved into its own util but no need to do as part of this PR.
I'll do it in a separate JS refactoring PR.
Leave it as is for now
contracts/utils/rebalancer.js
Outdated
| state.strategies.filter((s) => s.morphoVaultAddress) | ||
| ); | ||
|
|
||
| // Exclude strategies with suspiciously high APY |
There was a problem hiding this comment.
a nice feature. It'd be interesting to see historical APYs to know how high they can get. A 50% APY seems very high, especially if its an average over 6 hours.
I expect a spike to 50% for some number of blocks would be possible.
Is the 50% used from gut feel?
There was a problem hiding this comment.
Yea. The config for everything is just rough values, not final ones. We probably have to sync with everyone to decide on those constraints before we deploy
contracts/utils/rebalancer.js
Outdated
| } | ||
|
|
||
| /** | ||
| * Fetch APYs for multiple vaults in parallel. |
There was a problem hiding this comment.
This is a critical function. Let's make it very clear what APY we are getting
* Fetch a single vault's current net APY after fees from the Morpho GraphQL API.
* The APY is a weighted average based on the liquidity allocated in each market.
contracts/utils/rebalancer.md
Outdated
| | Field | Value | Meaning | | ||
| |-------|-------|---------| | ||
| | `minDefaultStrategyBps` | 2000 | Default strategy always gets ≥ 20 % of deployable capital | | ||
| | `maxPerStrategyBps` | 7000 | No single strategy gets > 70 % | |
There was a problem hiding this comment.
70% seems too low given we only have two strategies at the moment.
If Rebalancer was running now, 700k would be moved from Ethereum earning 4.23% to Base earning 3.64%.
Even if we had 5 Morpho strategies, if one was earning 5% and all the others were only earning 1%, would we really want 30% allocated to the vaults earning 1%?
I'd say the maxPerStrategyBps constraint is dropped and minVaultBalance is used to ensure not everything is allocated to a single vault.
Alternatively, increase maxPerStrategyBps to 90% or even 95%.
There was a problem hiding this comment.
Yep. Like I mentioned in the other comment, the values in the config file aren't finalised. Can change it
* Updated comments on what the Morpho APY is * Made it clear the strategies are only for Morpho * Fixed alignment of Allocations data in planBalance output
clement-ux
left a comment
There was a problem hiding this comment.
A few comments before this lands.
| rebalancerModule | ||
| .connect(stranger) | ||
| .allowStrategy("0x0000000000000000000000000000000000000099") | ||
| ).to.be.revertedWith("Caller is not the Safe"); |
There was a problem hiding this comment.
I think this assertion is wrong. Actual revert string in AbstractSafeModule.sol:16 is "Caller is not the safe contract" (lowercase s, plus contract). With waffle's substring matching this isn't a match either way, so the test should be failing — unless I'm missing something. Same thing on line 455. Line 345 has the right string.
clement-ux
left a comment
There was a problem hiding this comment.
Two defense-in-depth ideas to bound the blast radius if the off-chain script goes wrong (or the relayer key is compromised). Cheap to add now, much harder later.
| address[] memory assets = _toAddressArray(asset); | ||
| for (uint256 i = 0; i < _strategies.length; i++) { | ||
| if (_amounts[i] == 0) continue; | ||
| require(isAllowedStrategy[_strategies[i]], "Strategy not allowed"); |
There was a problem hiding this comment.
Worth adding a per-strategy max amount cap right next to this whitelist check. Something like:
mapping(address => uint256) public maxAmountPerStrategy;
function setMaxAmountPerStrategy(address strategy, uint256 max) external onlySafe {
maxAmountPerStrategy[strategy] = max;
emit MaxAmountSet(strategy, max);
}
// here in both _executeWithdrawals and _executeDeposits:
require(_amounts[i] <= maxAmountPerStrategy[_strategies[i]], "Amount exceeds max");Rationale: today nothing on-chain stops the operator from passing a number 10x bigger than the planner intended, whether from a script bug or a compromised relayer. A per-strategy cap (set by the Safe at allowStrategy time) caps the worst case at $X per call. Defaulting the value to 0 forces the Safe to set it explicitly — strategy is whitelisted but unusable until a cap is set, which is the safer fail mode.
Same check works for both withdrawals and deposits, one cap per strategy is enough.
There was a problem hiding this comment.
I do like the idea of having caps, to prevent a compromised stolen API key from doing too much damage. On the other hand I would prefer implementation where complexity is minimally increased and there is no maintenance overhead to caps (increasing / decreasing absolute configured values).
What if we set a general per rebalancer cap where the relabalancer is limited in the % of the TVL that it is allowed to atomatically move in a day? Pseudocode:
// This maps days to amount that has been moved in that day. Day can be just a normalized block number (block.number / 7200) - where 7200 is the amount of blocks per day.
mapping(uint256 => uint256) public amountMovedPerDay.
// amount of TVL allowed to rebalance per day e.g. 0.6 * 10e18
uint256 pctTVLAllowedToMove;
This is a somewhat simplified cap which sanity checks the rebalancer, so it is limited so that it can not re-locate more than 60% of the TVL daily
There was a problem hiding this comment.
Sharing some of comments from Discord yesterday. I do understand the idea of having a limit either at tx level or day level. But there're other things that can easily complicate in either approach. In both approaches,
- If we set the value too high, it essentially disables that protection (since rebalancer will always bypass it)
- If we set it too low, it'll prevent rebalancer from running as it is intended to (ex: if HyperEVM is giving 20% APY, we want max allocation in that strategy, if we can deposit only 500k or x% of the TVL there because of these limitations, it kinda defeats the purpose of the rebalancer). We are also planning to run the rebalancer more frequently, so the limit we configure might get used as soon as it resets, it'll be essentially same as running a rebalancer only one or two times a day. If the rebalancer takes two or more days to move funds to a higher-APY strategy, we are missing out on yield. Not to mention any huge pending withdrawal can also get affected because of this.
If we are unsure about the Rebalancer logic in production, we can just have it post the recommended actions on Discord (there's already an action for it) and do it manually for the test week
There was a problem hiding this comment.
Having said that, if you guys still feel strongly about having these limits, I can put up a PR
There was a problem hiding this comment.
I see your reservations. I do like the idea of having additional sanity check, but not that it would impede the normal operations. @shahthepro do you think there is a setting that is sane and would never be an obstacle for the rebalancer? say 200% of the TVL?
Ideally some value we never need to maintain and similar to Vault Value checker we have some peace of mind that there is an additional safe-guard where someone getting API keys to KMS can't just rebalance back and forth 40 times potentially draining user principal.
There was a problem hiding this comment.
say 200% of the TVL?
I think that's far more better. We can start with that limit and try to change it depending on how things work
There was a problem hiding this comment.
@clement-ux @sparrowDom I have added the daily limit: c9b9779
* Consider APY impact for deposits * Fix config issues * Bug fixes * Bug fix * bug fix * few more tweaks * Switch to using subsquid server * APY Impact solver * Fix available liquidity * Address CR comments
Code Changes
RebalancerModulewith methods to process deposits, withdrawals or both (in withdrawals -> deposits order). It will be replacing the existingAutoWithdrawalModuleplanRebalancethat prints the optimal allocations and recommended actionsExecuting Hardhat task
Pending Things
Monitoring: Discord NotificationsDone in d50c610