improve(InventoryClient): Align allocation percentage computations between Rebalancer and Relayer #1523
Conversation
…ns between Rebalancer and Relayer As part of a larger inventory client cleanup/refactor I noticed that there is possibly a bug in how we compute "allocation" percentages when (1) determining repayment chain for fills and (2) computing possible rebalances. For (2), we subtract the "token shortfall" from the numerator but in (2) we subtract it from both the numerator and the denominator. This is inconsistent and I don't think it makes sense. A shortfall represents a lack of tokens on a chain suggesting that we need that amount of tokens to fill a deposit. The rebalancer (i.e. function (2) above) it designed to rebalance inventory from L1 to L2 to fulfill these shortfalls. Therefore, it makes sense to me that in (2) the shortfall is subtracted from the numerator to compute the allocation percentage on the L2. The cumulative balance technically is the total inventory across all chains. This should intuitively include all outstanding transfers between L1 and L2 because this is still part of the inventory, its just in-flight between chains. A shortfall will lead to an outstanding transfer but it will also lead to a subtraction in the total inventory once the transfer finalizes and part of that new L2 balance is used to make a fill. However, if the shortfall is subtracted from the denominator in the rebalancer/(2) function, then the amount of tokens bridged over to the L2 will be insufficient to fulfill the shortfall. This PR makes the allocation % computation consistent between (1) and (2) by removing the shortfall subtraction from the denominator in (2).
nicholaspai
changed the title
nicholaspai
requested review from
mrice32,
pxrl and
james-a-morris
as code owners
There was a problem hiding this comment.
I've been over this a couple of times. I find the surrounding logic quite dense and not the easiest to follow, but I think the change is OK; it doesn't make sense to consider pending repayments on the candidate repayment chain without also considering "global" pending repayments across all chains.
There was a problem hiding this comment.
I think this mostly makes sense (and you've thought way more about this than I have :)).
Only part I'm struggling with is the last sentence in your description. Why should we intuitively assume that the shortfall will be removed from the L2 balance but will not be removed from global inventory if we're truly trying to compute the allocation after the relay?
So I think in all cases (1) and (2), shortfalls are tokens that will be removed from the cumulative balance and the chain balance where the shortfall exists.
I've reworded the PR description a bit. I think the key point I'm trying to make is not whether shortfalls should be subtracted from cumulative balance or not, but that the rebalancing and repayment chain choice computations should be consistent, right? Its important when rebalancing not to count shortfalls in the denominator, otherwise the rebalancer will never send inventory from L1 to an L2 in order to fill a large deposit that creates a shortfall. Therefore, shouldn't the repayment chain choice also use similar logic in a situation where there is a large unfilled deposit creating a shortfall? In this situation, the repayment chain should ideally be the chain with the shortfall so that the relayer can fill that deposit ASAP. |
Motivation
As part of a larger inventory client cleanup/refactor I noticed that there is possibly a bug in how we compute "allocation" percentages when (1) determining repayment chain for fills and (2) computing possible rebalances. This PR is therefore a prerequisite to any follow up refactors that reuse code to compute allocations for (1) and (2)
Problem description
For (2), we subtract the "token shortfall" from the numerator but in (1) we subtract it from both the numerator and the denominator. This is inconsistent which I think could lead to unexpected rebalancing/repayment behavior. I think its most important that the relayer and rebalancer use the same logic when computing how to rebalance inventory between all L2s. Choosing where to be repaid and rebalancing out of L1 inventory both rebalance inventory.
A shortfall represents a lack of tokens on a chain suggesting that we need that amount of tokens to fill a deposit.
The cumulative balance technically is the total inventory across all chains. This should intuitively include all outstanding transfers between L1 and L2 because this is still part of the inventory, its just in-flight between chains.
A shortfall will lead to an outstanding transfer but it will also lead to a subtraction in the total inventory once the transfer finalizes and part of that new L2 balance is used to make a fill.
However, if the shortfall is subtracted from the denominator in the rebalancer/(2) function, then the amount of tokens bridged over to the L2 will be insufficient to fulfill the shortfall. The whole point of the shortfall subtraction from the numerator (and not the denominator) in rebalancing is to force rebalances due to the L2 chain showing an "underallocation" when counting shortfalls.
Summary
This PR makes the allocation % computation consistent between (1) and (2) by removing the shortfall subtraction from the denominator in (1). The end result is that allocation computations for determining refund chain will be lower if there are short-falls, so we'll favor taking repayment on preferred chains slightly.