-
Notifications
You must be signed in to change notification settings - Fork 64
feat: mint + burn routing logic #1864
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: feat/mint-burn-bridge-routing
Are you sure you want to change the base?
feat: mint + burn routing logic #1864
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Left some remarks! Also some inline comments seem to be incorrect/outdated
const isLargeDeposit = depositAmountUsd > 1_000_000; // 1M USD | ||
|
||
// Check if eligible for Fast CCTP (Polygon, BSC, Solana) and deposit > 10K USD | ||
const fastCctpChains = [CHAIN_IDs.POLYGON, CHAIN_IDs.BSC, CHAIN_IDs.SOLANA]; |
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 part of the CCTP bridge strategy?
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.
Yes, for this use case we are already exporting CCTP_FILL_TIME_ESTIMATES
from the strategy. Maybe we can use that combined with a threshold.
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.
fast CCTP chains can be taken from here. Chains with "seconds" are considered fast
a8c41f2
to
671465d
Compare
if (bridgeStrategyData.isUtilizationHigh) { | ||
return getCctpBridgeStrategy(); | ||
} |
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.
when utilization is high CCTP strategy should be used only for USDC, right?
|
||
import { CrossSwap, CrossSwapQuotes, Token } from "../_dexes/types"; | ||
import { AppFee, CrossSwapType } from "../_dexes/utils"; | ||
import { Logger } from "@across-protocol/sdk/dist/types/relayFeeCalculator"; |
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 see we have some utils in api/_logger.ts
. Can we use getLogger
from there?
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.
Looking nice. Left some comments/questions.
return getAcrossBridgeStrategy(); | ||
} else { | ||
// Use OFT bridge if not CCTP | ||
return getCctpBridgeStrategy(); |
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.
for all these checks, should we check if the strategy we want to return is part of the supportedBridgeStrategies
list? That way we avoid returning here a strategy that was previously filtered out.
const isFastCctpChain = | ||
fastCctpChains.includes(inputToken.chainId) || | ||
fastCctpChains.includes(outputToken.chainId); |
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 only the origin chain matters here.
const utilizationThreshold = sdk.utils.fixedPointAdjustment.mul(80).div(100); // 80% | ||
|
||
// Calculate current utilization percentage | ||
const currentUtilization = _utilizedReserves |
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 utilizedReserves can be negative, would that have any impact in this calculation?
I see in other places we floored it to zero when that happens. See for example: https://github.com/across-protocol/frontend/blob/master/api/_utils.ts#L2709
}); | ||
|
||
// Safely return undefined if we can't fetch bridge strategy data | ||
return undefined; |
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 could instead return the expected JSON object but all fields set to false?
This would be a more clear return type than undefined.
const depositAmountUsd = parseFloat( | ||
ethers.utils.formatUnits(amountInInputTokenDecimals, inputToken.decimals) | ||
); | ||
const isInThreshold = depositAmountUsd <= 10_000; // 10K USD |
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.
Maybe we want to consider extracted hardcoded variables into a single place. This would help us later to make changes or understand why certain thresholds were set.
return getAcrossBridgeStrategy(); | ||
} else { | ||
// Use OFT bridge if not CCTP | ||
return getCctpBridgeStrategy(); |
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.
The comment here says that we should use OFT bridge if not CCTP but the return function indicates that we are using the cctp bridge strategy.
This PR implements the routing logic below. Changes here: a8c41f2
/limits
to return utilization data so that we don't need to refetch itlimits
with a standard amount to maximize cache hits