chore: Refactor USDH Refiller to be more configurable #2800
Conversation
| } | ||
|
|
||
| // Default minimum is 10 USDH. USDH only exists on HyperEVM and has 6 decimals. | ||
| this.minUsdhRebalanceAmount = toBNWei(MIN_USDH_REBALANCE_AMOUNT ?? "10", 6); |
There was a problem hiding this comment.
I would maybe even prefer to move MIN_USDH_REBALANCE_AMOUNT to REFILL_BALANCES object level and we will be able to decide if we should check origin or destination balances based on this and not checkOriginChainBalance
wdyt?
There was a problem hiding this comment.
Yeah, that's not a bad idea 👍
dijanin-brat
requested review from
bmzig,
dohaki,
mrice32,
nicholaspai and
pxrl
as code owners
|
Before merging this PR, we should first update |
pxrl
left a comment
pxrl
left a comment
There was a problem hiding this comment.
A couple of minor questions - hopefully easy to resolve these so we can get this merged soon 👍
| const accountAddress = account.toNative(); | ||
|
|
||
| // Early exit check: If checking destination chain balance, verify it's below trigger | ||
| if (!checkOriginChainBalance && currentBalance.gt(triggerThreshold)) { |
There was a problem hiding this comment.
Could we simplify the checkOriginChainBalance logic by always checking it, and enforcing the minimum rebalance amount, but defaulting that amount to 0? Then we can increase that limit when we want to impose a minimum, and otherwise it'd effectively revert to always rebalancing.
There was a problem hiding this comment.
hmm, i don't see a big point for that one. We have 2 modes here:
- We will send everything we have on origin if the balance is greater than amount X we specified
- We will look for trigger amount on dst chain and if its below we want to refill it up to target amount
I don't really see anything meaningful in always rebalancing (specifically bcs this should be used either to top-up USDH relayer or to refill DB that we want to do manually)
There was a problem hiding this comment.
Ah, I understand now. I think we've ended in a situation where we've mixed two things:
- Rebalancing implementaiton of how to rebalance Arbitrum USDC -> HyperEVM USDH.
- Business logic that determines when we want to rebalance, of which we have two sub cases:
- Destination chain balance is under threshold.
- Origin chain balance is over threshold.
The 2nd one is what we're currently using, and the 1st one is what we're aiming to enable here.
Interesting, the "origin balance over threshold" one is reminiscent of what we do with the InventoryClient, where we pull back funds from Spoke to Hub when we're over balance.
In any case, the thing that triggers me a bit is the need to add an extra variable to unlock this case. I think the place we ultimately want to end up in, is that we separate the decision-making/business logic from the underlying implementation of how we rebalance.
Not sure if we can get there in the time horizon for this PR, though. wdyt?
There was a problem hiding this comment.
I agree with you 100% on this!
The thing is that currently, all rebalancing functions (even the ones that are not for USDH), have business logic inside of them. Eventually, i guess we want to end up having a business logic before we decide to call any rebalancing function so we can potentially exit early.
I wouldn't do that in this PR tho, it will create much bigger dif. We can do that as a follow-up
| minThreshold: formatUnits(this.config.minUsdhRebalanceAmount, decimals), | ||
| amountToTransfer: formatUnits(amountToTransfer, decimals), | ||
| }); | ||
| return null; |
There was a problem hiding this comment.
What if we returned { usdc, amountToTransfer: bnZero, accountAddress } in this case? it's arguably simpler wherever this is used, because the caller only needs to check for amountToTransfer.gt(bnZero).
There was a problem hiding this comment.
i am okey with that. Currently we are sending null so we can check if refillData exists, and if not we do an early exit. But we can change that to checking for amountToTransfer.gt(bnZero) here
Both approaches are fine with me tbh.
| const triggerThreshold = parseUnits(trigger.toString(), decimals); | ||
| const targetThreshold = parseUnits(target.toString(), decimals); | ||
| const accountAddress = account.toNative(); | ||
|
|
||
| // Early exit check: If checking destination chain balance, verify it's below trigger | ||
| if (!checkOriginChainBalance && currentBalance.gt(triggerThreshold)) { |
There was a problem hiding this comment.
If we expect trigger and target to have the same scale, and we're only checking them relative to one another, do we actually need to normalise them by decimals , or can we just compare them directly?
There was a problem hiding this comment.
We can compare them directly, but we are calculating how much should we transfer like this
const deficit = targetThreshold.sub(currentBalance);
So we will need targetThreshold normalized by decimals
| const accountAddress = account.toNative(); | ||
|
|
||
| // Early exit check: If checking destination chain balance, verify it's below trigger | ||
| if (!checkOriginChainBalance && currentBalance.gt(triggerThreshold)) { |
There was a problem hiding this comment.
Ah, I understand now. I think we've ended in a situation where we've mixed two things:
- Rebalancing implementaiton of how to rebalance Arbitrum USDC -> HyperEVM USDH.
- Business logic that determines when we want to rebalance, of which we have two sub cases:
- Destination chain balance is under threshold.
- Origin chain balance is over threshold.
The 2nd one is what we're currently using, and the 1st one is what we're aiming to enable here.
Interesting, the "origin balance over threshold" one is reminiscent of what we do with the InventoryClient, where we pull back funds from Spoke to Hub when we're over balance.
In any case, the thing that triggers me a bit is the need to add an extra variable to unlock this case. I think the place we ultimately want to end up in, is that we separate the decision-making/business logic from the underlying implementation of how we rebalance.
Not sure if we can get there in the time horizon for this PR, though. wdyt?
We want to make USDH Refiller more configurable so it can top-up address on HyperEVM with USDH based on balances on both origin and destination chains.