-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
feat: Add support for mixed routes in the interface #4181
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
const MixedProtocolBadge = styled(ProtocolBadge)` | ||
width: 60px; | ||
` |
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.
This seems to work to prevent the badge text from overflowing and line breaking
@zhongeric can you rebase/merge main? |
} else { | ||
// TODO: Update with better estimates once we have interleaved routes. | ||
// fallback general gas estimation | ||
gas += V3_SWAP_BASE_GAS_ESTIMATE + route.pools.length * V3_SWAP_HOP_GAS_ESTIMATE | ||
} |
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.
Chose to keep an else
case as a fallback, not really needed tho
Do we want to query for mixedRoutes in the |
src/state/routing/slice.ts
Outdated
@@ -28,7 +28,6 @@ const protocols: Protocol[] = [Protocol.V2, Protocol.V3, Protocol.MIXED] | |||
|
|||
const DEFAULT_QUERY_PARAMS = { | |||
protocols: protocols.map((p) => p.toLowerCase()).join(','), | |||
forceMixedRoutes: true, |
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.
this part should be done in a separate PR (turning on the mixed routes at the API level)
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.
that way we know what we can minimally revert if issues arise
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.
makes sense, will cut
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.
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 all that PR needs to be is forceMixedRoutes: true
?
@@ -43,8 +45,19 @@ function guesstimateGas(trade: Trade<Currency, Currency, TradeType> | undefined) | |||
// V3 gas costs scale on initialized ticks being crossed, but we don't have that data here. | |||
// We bake in some tick crossings into the base 100k cost. | |||
gas += V3_SWAP_BASE_GAS_ESTIMATE + route.pools.length * V3_SWAP_HOP_GAS_ESTIMATE | |||
} else if (route.protocol === Protocol.MIXED) { | |||
const sections = partitionMixedRouteByProtocol(route as MixedRoute<Currency, Currency>) | |||
let acc = 0 |
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.
acc
is unnecssary - you should instead just have L53 and L55 use gas +=
to be more explicit about what you're adding up.
You should also use reduce
to make it more clear what is happening in the code:
gas += sections.reduce((gas, section) => {
if (section.every((pool) => pool instanceof Pool)) {
return gas + V3_SWAP_BASE_GAS_ESTIMATE + section.length * V3_SWAP_HOP_GAS_ESTIMATE
} else if (section.every((pool) => pool instanceof Pair)) {
return gas + V2_SWAP_BASE_GAS_ESTIMATE + (section.length - 1) * V2_SWAP_HOP_GAS_ESTIMATE
} else {
console.warn('Invalid section')
return gas
}
})
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 like the .reduce
src/state/routing/utils.ts
Outdated
} | ||
|
||
function isMixedRoute(route: (V3PoolInRoute | V2PoolInRoute)[]): route is (V3PoolInRoute | V2PoolInRoute)[] { | ||
/// Must have at least one V3 pool and one V2 pool |
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.
This isn't comprehensive. Should you also check that every route is either V3 or V2?
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 be resolved by new logic in getRouteProtocol
@@ -71,6 +79,13 @@ export function transformRoutesToTrade<TTradeType extends TradeType>( | |||
route | |||
?.filter((r): r is typeof route[0] & { routev3: NonNullable<typeof route[0]['routev3']> } => r.routev3 !== null) | |||
.map(({ routev3, inputAmount, outputAmount }) => ({ routev3, inputAmount, outputAmount })) ?? [], | |||
mixedRoutes: |
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.
Where is this used?
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.
used to build the InterfaceTrade
which is returned by useRoutingAPITrade()
and then in useBestTrade()
@@ -41,8 +42,15 @@ export function computeRoutes( | |||
} | |||
|
|||
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.
This requires a lot of looping that isn't necessary. Could you improve it by doing something like this?
getRouteProtocol(route: (V3PoolInRoute | V2PoolInRoute)[]): Protocol {
if (route.every((pool) => pool.type === 'v2-pool')) return Protocol.V2
if (route.every((pool) => pool.type === 'v3-pool')) return Protocol.V3
return Protocol.MIXED
}
You could optimize it more but this seems sufficient. The old isV2Route
-style functions were type guards, but the genericPoolPairParser
makes it so that you no longer need to cast to a type before parsing the route.
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 this is a lot more elegant. This also solves the comment above about the mixed routes not exactly being comprehensive
src/state/routing/utils.ts
Outdated
@@ -41,16 +41,21 @@ export function computeRoutes( | |||
throw new Error('Expected both amountIn and amountOut to be present') | |||
} | |||
|
|||
const routeProtocol: Protocol = getRouteProtocol(route) |
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.
rm : Protocol
, the typing is unneeded here. It can be inferred from the return value of getRouteProtocol
.
The new versions of the smart order router (SOR) and routing-api introduce support for a new kind of route called
MixedRoutes
. MixedRoutes are routes which have both V2 and V3 pools in them, ex:USDC -[V3]-> ETH -[V2]-> USDT
We only support returning these routes through the AutoRouter / routingAPI, and so we can pass in a new
protocols
queryParam of[Protocol.V2, Protocol.V3, Protocol.MIXED]
to direct the auto router to also look for and consider mixedRoutes. Client side, we should still only query for V3 routes using the client side SOR and/or the QuoterV2 fallback.We also need to edit the
Trade
object we create here to include the new mixed route, in order to generate the correct calldata to submit the swap with.We also show a new badge label of
V3 + V2
to denote the new mixed routes in the RoutingDiagram