-
Notifications
You must be signed in to change notification settings - Fork 25
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
Synapse SDK Plugin (Draft) #106
Synapse SDK Plugin (Draft) #106
Conversation
|
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 looks like you're definitely going in the right direction! There might be some peculiarities as you enter testing but I don't forsee any major hiccups.
Two things to flag:
- if at all possible for the
tokens.ts
andTokens.ts
file if those values are available via an API, or a published repo the plugin will be a lot more stable relying on that source. - Make sure that nothing breaks if any of the optional values on
BridgeActionParams
are passed in as undefined.
Thanks so much for taking the time to draft a plugin for our system!
packages/synapse/src/Synapse.ts
Outdated
} | ||
|
||
//Need to edit based on CCTP domains | ||
export const chainDomainToID = (id: number): number => { |
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 helper doesn't look like it covers all of the supported chain IDs, do only some chains have these domains?
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.
Only transactions routed through the "SynapseCCTP bridge" will have the parameter "chainDomain" thus we need to convert that into a chainID for the rest of the application
packages/synapse/src/Synapse.ts
Outdated
|
||
const destinationDomain = chainDomainToID(destinationChainId) | ||
|
||
if (contractAddress && Object.values(cctpMapping).includes(contractAddress)) { |
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.
Okay so basically the domain is only defined on some chains and this covers that?
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 are two different bridge contracts -- this is a case to handle when a transaction is routed through the "SynapseCCTP bridge"
packages/synapse/src/Synapse.ts
Outdated
const { | ||
sourceChainId, | ||
destinationChainId, | ||
contractAddress, |
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.
contractAddress
isn't always defined but it does look like the filter will work even if it's not.
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's some tweaks I'm going to make but I'm going to get it in first then add the changeset for it to publish when I make my changes.
[METIS_CHAIN_ID]: '0x06Fea8513FF03a0d3f61324da709D4cf06F42A5c', | ||
[MOONBEAM_CHAIN_ID]: '0x84A420459cd31C3c34583F67E0f0fB191067D32f', | ||
[AURORA_CHAIN_ID]: '0xaeD5b25BE1c3163c907a471082640450F928DDFE' | ||
export const CHAIN_TO_ROUTER: { [chainId: number]: string } = { |
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.
Is it necessary to use a lookup here vs just a single const
?
@@ -50,10 +48,39 @@ export const CHAIN_TO_CONTRACT: { [chainId: number]: string } = { | |||
[AVALANCHE_CHAIN_ID]: '0xfB2Bfc368a7edfD51aa2cbEC513ad50edEa74E84', | |||
} | |||
|
|||
export const SYNAPSE_CCTP_ROUTER: Record< any, string> = { |
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.
Same question here - could we use a const
instead?
This is a first attempt at creating the questsdk for the Synapse plugin. There is support for the original synapse bridge contracts as well as an outline for supporting the SynapseCCTP contracts. This version still needs robust testing support.