-
Notifications
You must be signed in to change notification settings - Fork 18
feat: Add CCTP V2 support on Linea #958
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
@@ -170,7 +171,7 @@ export function chainIsCCTPEnabled(chainId: number): boolean { | |||
* @returns True if chainId requires manual L1 -> L2 finalization, otherwise false. | |||
*/ | |||
export function chainRequiresL1ToL2Finalization(chainId: number): boolean { | |||
return chainIsCCTPEnabled(chainId) || chainIsLinea(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.
This comment is mostly tangential to the actual change here, but I think this function lacks granularity around the token/bridge that's used, and we should consider changing that in a follow-up commit.
The existing logic means that we use effort trying to finalise deposits destined for Polygon, Optimism and Arbitrum, just because they have CCTP enabled. In practice we probably only want to do that for actual native USDC deposits. This would also mean we'd need to stub off an OFT finaliser for Arbitrum USDT0 even though it's not required.
In general I think this logic probably belongs as a method on the bridge adapter - wdyt @bmzig ?
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 I'm missing the point here, but doesn't this function only get used by the finalizer? The bridge adapters are configured per-bridge, and it looks like we only use this function to add some extra addresses to check when finalizing L1->L2 deposits https://github.com/across-protocol/relayer/blob/177420f58a66f11900206bb0eff916c53449b4d4/src/finalizer/index.ts#L240.
To be clear, it seems like we would only then be running the cctpFinalizer on those chains (since that is the defined L1->L2 finalizer), so we wouldn't check other tokens.
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 I agree this is only used by the finalizer
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.
Also we won't need this function anymore following the relayer PR accompanying this SDK PR: https://github.com/across-protocol/relayer/pull/2165/files#diff-d8c1bd620fbb6212fbcc45c4aed169001b34a3fb5dbe3731902fc9af923fab7d
So I'll remove this function
No description provided.