refactor: bridge strategy routing rules #1912
Conversation
ashwinrava
requested review from
NikolasHaimerl,
amateima,
dohaki,
gsteenkamp89,
melisaguevara,
mrice32,
nicholaspai and
pxrl
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
NikolasHaimerl
left a comment
NikolasHaimerl
left a comment
There was a problem hiding this comment.
The new routeStrategyForCctp looks much better, great change.
dohaki
left a comment
dohaki
left a comment
There was a problem hiding this comment.
Nice stuff! Left some questions
api/_bridges/cctp/utils/routing.ts
Outdated
| const ROUTING_RULES: RoutingRule[] = [ | ||
| { | ||
| name: "non-usdc-route", | ||
| priority: 1, |
There was a problem hiding this comment.
Can't we infer the priority based on the index in the array? Or would there be cases where we have same priority for a rule?
There was a problem hiding this comment.
Yeah good point. Thought it could be useful for readability but a top level comment would do.
Makes me think we can consider a field like group in the future though for better organization and less duplication. For example:
{
name: "fast-cctp-small-deposit",
shouldApply: (data) =>
data.isFastCctpEligible && !data.isInThreshold && !data.isLargeDeposit,
getStrategy: getCctpBridgeStrategy,
reason:
"Fast CCTP eligible chains (Polygon/BSC/Solana) use CCTP for medium deposits (>$10K, <$1M)",
},
{
name: "fast-cctp-threshold-or-large",
shouldApply: (data) =>
data.isFastCctpEligible && (data.isInThreshold || data.isLargeDeposit),
getStrategy: getAcrossBridgeStrategy,
reason:
"Fast CCTP chains use Across for very small (<$10K) or very large (>$1M) deposits",
},
We could have both of these be part of a "group" fast-cctp where the base condition is data.isFastCctpEligible and then rule 1: data.isInThreshold || data.isLargeDeposit and rule 2 (default): would return true. Will think about this.
| reason: string; | ||
| }; | ||
|
|
||
| const ROUTING_RULES: RoutingRule[] = [ |
There was a problem hiding this comment.
How would this work with a separate branch of rules? Like for sponsorship eligibility? Would we use just a different set of rules?
There was a problem hiding this comment.
Yeah exactly. I think it would be cleaner as a separate set. I am working on the PR for that now with some placeholders for the eligibility module.
The new set would be compatible since getStrategy: () => BridgeStrategy; still holds true.
ashwinrava
merged commit 293e5a6
into
epic/sponsored-bridging-to-hypercore
Following on from #1864, I want to refactor the routing rules in a way that makes it easy to add new rules. In the old approach, new rules could introduce more branching and duplication of conditions and was making the actual logic unreadable.
In this PR:
routeStrategyForCctpto use a decision table