-
Notifications
You must be signed in to change notification settings - Fork 102
chore: Refactor USDH Refiller to be more configurable #2800
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's not a bad idea 👍
|
Before merging this PR, we should first update |
src/refiller/Refiller.ts
Outdated
| this.logger.debug({ | ||
| at: "Refiller#refillUsdh", | ||
| message: "Checking destination chain (HyperEVM) USDH balance", | ||
| destinationChainBalance: formatUnits(currentBalance, decimals), | ||
| triggerThreshold: formatUnits(triggerThreshold, decimals), | ||
| targetThreshold: formatUnits(targetThreshold, decimals), | ||
| shouldRefill, | ||
| }); | ||
|
|
||
| // Early exit if destination chain balance is above trigger | ||
| if (!shouldRefill) { | ||
| this.logger.debug({ | ||
| at: "Refiller#refillUsdh", | ||
| message: "Destination chain balance above trigger, skipping transfer", | ||
| destinationChainBalance: formatUnits(currentBalance, decimals), | ||
| triggerThreshold: formatUnits(triggerThreshold, decimals), | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it probably only makes sense to emit one of these messages, not both (i.e. in the case of !shouldRefill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pxrl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor questions - hopefully easy to resolve these so we can get this merged soon 👍
| message: "Rebalanced Arbitrum USDC to HyperEVM USDH", | ||
| nonMulticall: true, | ||
| mrkdwn: `Sent ${formatUnits(amountToTransfer, decimals)} USDC from Arbitrum to HyperEVM.`, | ||
| const originChainBalance = await usdc.balanceOf(this.baseSignerAddress.toNative()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be accountAddress instead? (Based on line ~410 it seems like they should be interchangeable). Does it make sense for us to assert equality on accountAddress and this.baseSignerAddress.toNative() ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check for USDC balance on Arbitrum for signer address (address from where we are sending funds) and that can be different them accountAddress (destination address where we are sending funds)
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
| minThreshold: formatUnits(this.config.minUsdhRebalanceAmount, decimals), | ||
| amountToTransfer: formatUnits(amountToTransfer, decimals), | ||
| }); | ||
| return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.