Skip to content
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(sdk-router)!: add support for FastBridgeRouterV2 #2957

Merged
merged 17 commits into from
Aug 5, 2024

Conversation

ChiTimesChi
Copy link
Collaborator

@ChiTimesChi ChiTimesChi commented Jul 30, 2024

Description
Adds support for the updated FastBridgeRouterV2 contract. This is a breaking change for the consumers previously using custom options like deadline or excludeCCTP. A single options object is now used instead:

/**
* Options for the bridgeQuote and allBridgeQuotes functions.
*
* @param deadline - Optional transaction deadline on the origin chain.
* @param excludedModules - Optional array of module names to exclude from the quote.
* @param originUserAddress - Optional address of the user on the origin chain. This parameter must be
* specified if a smart contract will initiate the bridge operation on behalf of the user.
*/
interface BridgeQuoteOptions {
deadline?: BigNumber
excludedModules?: string[]
originUserAddress?: string
}

export async function allBridgeQuotes(
this: SynapseSDK,
originChainId: number,
destChainId: number,
tokenIn: string,
tokenOut: string,
amountIn: BigintIsh,
options: BridgeQuoteOptions = {}
): Promise<BridgeQuote[]> {

export async function bridgeQuote(
this: SynapseSDK,
originChainId: number,
destChainId: number,
tokenIn: string,
tokenOut: string,
amountIn: BigintIsh,
options: BridgeQuoteOptions = {}
): Promise<BridgeQuote> {

Note: integrations that require a smart contract to work (such as a smart wallet like Safe, or a bridge aggregator contract) MUST provide the originUserAddress option, otherwise the origin transaction will revert.

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced structured parameters for bridge-related functions to enhance clarity and usability.
    • Added a required originUserAddress parameter for improved handling of smart contract interactions in bridge operations.
  • Documentation

    • Enhanced documentation to emphasize the necessity of originUserAddress when initiating bridge operations.

Copy link
Contributor

coderabbitai bot commented Jul 30, 2024

Walkthrough

The recent changes enhance the synapseSDK API by reorganizing function parameters into structured objects. Notable updates include the introduction of the originUserAddress for user-specific routing, improved handling of optional parameters, and the addition of an excludedModules array for streamlined exclusions. These modifications boost clarity, maintainability, and user experience, setting the stage for future feature expansions.

Changes

Files Changes Summary
packages/sdk-router/README.md Updated allBridgeQuotes to use an options object for deadline, excludedModules, and originUserAddress.
packages/sdk-router/src/operations/bridge.ts, packages/sdk-router/src/module/synapseModuleSet.ts, packages/sdk-router/src/rfq/fastBridgeRouterSet.ts Refactored methods to accept an options object for better parameter management, including optional originUserAddress.
packages/sdk-router/src/rfq/fastBridgeRouterSet.test.ts Introduced tests for createRFQDestQuery to validate behavior with and without originUserAddress.
packages/sdk-router/src/sdk.test.ts Modified test parameters to use an object for excludedModules, enhancing clarity in module exclusions.

Poem

🐇 In the code, I hop and play,
Adding options bright and gay.
User addresses now in line,
Exclusions clear, all is fine!
Hop, skip, and jump through the chain,
With each change, we grow and gain! 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Jul 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 25.71660%. Comparing base (e84f0f6) to head (1813785).
Report is 13 commits behind head on master.

Additional details and impacted files
@@                 Coverage Diff                 @@
##              master       #2957         +/-   ##
===================================================
+ Coverage   25.70802%   25.71660%   +0.00857%     
===================================================
  Files            771         771                 
  Lines          55613       55610          -3     
  Branches          80          82          +2     
===================================================
+ Hits           14297       14301          +4     
+ Misses         39830       39823          -7     
  Partials        1486        1486                 
Flag Coverage Δ
packages 90.55118% <100.00000%> (+0.06555%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChiTimesChi ChiTimesChi marked this pull request as draft July 30, 2024 09:40
@ChiTimesChi
Copy link
Collaborator Author

Converting this to draft until the FastBridgeRouterV2 is deployed

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

Support for the FastBridgeRouterV2 contract has been added, introducing a breaking change by consolidating bridge options into a single object.

  • README.md: Updated documentation to reflect the new options object for bridge operations, including originUserAddress.
  • src/module/synapseModuleSet.ts: Added originUserAddress parameter to getBridgeRoutes method.
  • src/operations/bridge.ts: Introduced BridgeQuoteOptions interface consolidating parameters like deadline and excludeCCTP.
  • src/rfq/fastBridgeRouterSet.ts: Enhanced getBridgeRoutes and createRFQDestQuery methods to support originUserAddress.
  • src/sdk.test.ts: Updated tests to accommodate the new options object for FastBridgeRouterV2.

5 file(s) reviewed, 12 comment(s)
Edit PR Review Bot Settings

packages/sdk-router/README.md Outdated Show resolved Hide resolved
packages/sdk-router/README.md Outdated Show resolved Hide resolved
packages/sdk-router/src/operations/bridge.ts Show resolved Hide resolved
packages/sdk-router/src/operations/bridge.ts Show resolved Hide resolved
packages/sdk-router/src/sdk.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (4)
packages/sdk-router/src/module/synapseModuleSet.ts (1)

Update function calls and signatures to match the new parameter

The function getBridgeRoutes has been updated to include an optional parameter originUserAddress. However, not all function calls and signatures have been updated to reflect this change. Please update the following instances:

  • packages/sdk-router/src/operations/bridge.ts:

    const routes = await moduleSet.getBridgeRoutes(
      originChainId,
      destChainId,
      tokenIn,
      tokenOut,
      amountIn,
      originUserAddress // Add this parameter
    );
  • packages/sdk-router/src/rfq/fastBridgeRouterSet.ts:

    public async getBridgeRoutes(
      originChainId: number,
      destChainId: number,
      tokenIn: string,
      tokenOut: string,
      amountIn: BigintIsh,
      originUserAddress?: string // Add this parameter
    ) {
      // Function implementation
    }
  • packages/sdk-router/src/router/routerSet.ts:

    public async getBridgeRoutes(
      originChainId: number,
      destChainId: number,
      tokenIn: string,
      tokenOut: string,
      amountIn: BigintIsh,
      originUserAddress?: string // Add this parameter
    ) {
      // Function implementation
    }
Analysis chain

Line range hint 101-111:
LGTM! But verify the function usage in the codebase.

The code changes are approved.

However, ensure that all function calls to getBridgeRoutes match the new signature.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `getBridgeRoutes` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type ts -A 5 $'getBridgeRoutes'

Length of output: 2094


Script:

#!/bin/bash
# Description: Verify that all calls to `getBridgeRoutes` in specific files match the new signature.

# Check in `packages/sdk-router/src/operations/bridge.ts`
echo "Checking in packages/sdk-router/src/operations/bridge.ts"
rg 'getBridgeRoutes' packages/sdk-router/src/operations/bridge.ts -A 3

# Check in `packages/sdk-router/src/rfq/fastBridgeRouterSet.ts`
echo "Checking in packages/sdk-router/src/rfq/fastBridgeRouterSet.ts"
rg 'getBridgeRoutes' packages/sdk-router/src/rfq/fastBridgeRouterSet.ts -A 3

# Check in `packages/sdk-router/src/router/routerSet.ts`
echo "Checking in packages/sdk-router/src/router/routerSet.ts"
rg 'getBridgeRoutes' packages/sdk-router/src/router/routerSet.ts -A 3

Length of output: 1033

packages/sdk-router/README.md (1)

93-93: Fix the grammatical issue.

Use a hyphen for the compound adjective "third-party."

- third party integration
+ third-party integration
Tools
LanguageTool

[uncategorized] ~93-~93: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ncludes smart wallets (like Safe), or a third party integration (like a bridge aggregator s...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

packages/sdk-router/src/rfq/fastBridgeRouterSet.ts (1)

96-97: Ensure all function calls to getBridgeRoutes match the new signature.

The following instances of getBridgeRoutes do not include the new optional parameter originUserAddress:

  • packages/sdk-router/src/operations/bridge.ts: lines 1-5
  • packages/sdk-router/src/router/routerSet.ts: lines 1-5
  • packages/sdk-router/src/module/synapseModuleSet.ts: lines 1-5

Please update these function calls to include the new optional parameter where necessary.

Analysis chain

LGTM! But verify the function usage in the codebase.

The code changes are approved.

However, ensure that all function calls to getBridgeRoutes match the new signature.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `getBridgeRoutes` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type ts -A 5 $'getBridgeRoutes'

Length of output: 2094

packages/sdk-router/src/operations/bridge.ts (1)

Update bridgeQuote function calls to match the new signature

The refactored bridgeQuote function now includes an options parameter. Ensure that all calls to bridgeQuote are updated to include this parameter.

  • File: packages/sdk-router/src/sdk.test.ts
    • Lines:
      • 105
      • 111
      • 117
      • 123
      • 129
      • 135
      • 141
      • 147
      • 153
      • 159
      • 165
      • 171
      • 177
      • 183
      • 189
      • 195
      • 201

Please update these function calls to include the options parameter.

Analysis chain

Line range hint 98-110:
LGTM! But verify the function usage in the codebase.

The refactor enhances readability and maintainability.

Ensure that all calls to bridgeQuote match the new signature.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `bridgeQuote` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type js -A 5 $'bridgeQuote'

Length of output: 31


Script:

#!/bin/bash
# Description: Verify all function calls to `bridgeQuote` match the new signature.

# Test: Search for the function usage in a more comprehensive way, including other possible contexts and aliases.
rg --type js 'bridgeQuote' -A 5
rg --type ts 'bridgeQuote' -A 5
ast-grep --lang typescript --pattern '$_.$bridgeQuote($$$)'

Length of output: 50575


Script:

#!/bin/bash
# Description: Verify that all calls to `bridgeQuote` match the new signature with the `options` parameter.

# Test: Search for the function usage and ensure the new signature is used.
rg --type js 'synapse\.bridgeQuote\(' -A 5
rg --type ts 'synapse\.bridgeQuote\(' -A 5

Length of output: 7012

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5dbb6e9 and f4a762f.

Files selected for processing (5)
  • packages/sdk-router/README.md (1 hunks)
  • packages/sdk-router/src/module/synapseModuleSet.ts (2 hunks)
  • packages/sdk-router/src/operations/bridge.ts (5 hunks)
  • packages/sdk-router/src/rfq/fastBridgeRouterSet.ts (3 hunks)
  • packages/sdk-router/src/sdk.test.ts (2 hunks)
Additional context used
LanguageTool
packages/sdk-router/README.md

[uncategorized] ~93-~93: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ncludes smart wallets (like Safe), or a third party integration (like a bridge aggregator s...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

GitHub Check: codecov/patch
packages/sdk-router/src/rfq/fastBridgeRouterSet.ts

[warning] 322-322: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L322
Added line #L322 was not covered by tests


[warning] 325-325: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L325
Added line #L325 was not covered by tests


[warning] 330-331: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L330-L331
Added lines #L330 - L331 were not covered by tests

Additional comments not posted (7)
packages/sdk-router/README.md (1)

80-89: LGTM!

The code changes are approved.

packages/sdk-router/src/rfq/fastBridgeRouterSet.ts (1)

316-332: LGTM! But verify the function usage in the codebase and add tests.

The code changes are approved.

However, ensure that all function calls to createRFQDestQuery match the new signature and add tests to cover the new logic.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Verification successful

LGTM! But verify the function usage in the codebase and add tests.

The code changes are approved.

However, ensure that all function calls to createRFQDestQuery match the new signature and add tests to cover the new logic.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `createRFQDestQuery` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type ts -A 5 $'createRFQDestQuery'

Length of output: 1025


Script:

#!/bin/bash
# Description: Verify all function calls to `createRFQDestQuery` match the new signature across the entire codebase.

# Test: Search for the function usage across the codebase. Expect: Only occurrences of the new signature.
rg --type ts -A 5 $'createRFQDestQuery'

Length of output: 1025

Tools
GitHub Check: codecov/patch

[warning] 322-322: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L322
Added line #L322 was not covered by tests


[warning] 325-325: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L325
Added line #L325 was not covered by tests


[warning] 330-331: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L330-L331
Added lines #L330 - L331 were not covered by tests

packages/sdk-router/src/operations/bridge.ts (3)

66-70: LGTM! Ensure documentation is updated.

The BridgeQuoteOptions interface consolidates optional parameters, improving code clarity.


59-65: LGTM!

The comments provide clear guidance on the usage of the new options.

Also applies to: 72-89, 118-133


Line range hint 142-170:
LGTM! But verify the function usage in the codebase.

The refactor enhances readability and maintainability.

Ensure that all calls to allBridgeQuotes match the new signature.

packages/sdk-router/src/sdk.test.ts (2)

525-527: LGTM!

The changes enhance clarity and extensibility by moving to a more descriptive object format.


566-568: LGTM!

The changes enhance clarity and extensibility by moving to a more descriptive object format.

Copy link

cloudflare-workers-and-pages bot commented Jul 30, 2024

Deploying sanguine-fe with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1813785
Status: ✅  Deploy successful!
Preview URL: https://130e3f1e.sanguine-fe.pages.dev
Branch Preview URL: https://sdk-fb-router-v2.sanguine-fe.pages.dev

View logs

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f4a762f and bcc5fc8.

Files selected for processing (3)
  • packages/sdk-router/src/rfq/fastBridgeRouterSet.test.ts (1 hunks)
  • packages/sdk-router/src/rfq/fastBridgeRouterSet.ts (3 hunks)
  • packages/sdk-router/src/sdk.test.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • packages/sdk-router/src/rfq/fastBridgeRouterSet.ts
  • packages/sdk-router/src/sdk.test.ts
Additional comments not posted (2)
packages/sdk-router/src/rfq/fastBridgeRouterSet.test.ts (2)

138-144: Test case for createRFQDestQuery without originUserAddress looks good.

The test case is well-structured and correctly validates the expected behavior of the function when originUserAddress is not provided.


146-156: Test case for createRFQDestQuery with originUserAddress looks good.

The test case is well-structured and correctly validates the expected behavior of the function when originUserAddress is provided.

@ChiTimesChi ChiTimesChi marked this pull request as ready for review July 30, 2024 11:16
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

Support for the FastBridgeRouterV2 contract has been added, introducing a breaking change by consolidating bridge options into a single object.

  • README.md: Updated documentation to reflect the new requirement for the originUserAddress parameter.
  • packages/sdk-router/src/constants/addresses.ts: Updated FastBridgeRouter contract address to 0xd50042193Db100FE0040005e00D5010000007e45.
  • packages/sdk-router/src/operations/bridge.ts: Introduced structured parameters for bridge-related functions, emphasizing the originUserAddress parameter.
  • packages/sdk-router/src/rfq/fastBridgeRouterSet.test.ts: Added tests for createRFQDestQuery method to handle originUserAddress.
  • packages/sdk-router/src/rfq/fastBridgeRouterSet.ts: Modified createRFQDestQuery method visibility to public static for better accessibility.

6 file(s) reviewed, 11 comment(s)
Edit PR Review Bot Settings

packages/sdk-router/README.md Show resolved Hide resolved
packages/sdk-router/README.md Outdated Show resolved Hide resolved
packages/sdk-router/src/constants/addresses.ts Outdated Show resolved Hide resolved
packages/sdk-router/src/operations/bridge.ts Outdated Show resolved Hide resolved
packages/sdk-router/src/operations/bridge.ts Show resolved Hide resolved
packages/sdk-router/src/sdk.test.ts Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
packages/sdk-router/README.md (1)

93-93: Fix hyphenation in the note.

Use a hyphen for "third-party integration" as it is a compound adjective.

- third party integration
+ third-party integration
Tools
LanguageTool

[uncategorized] ~93-~93: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ncludes smart wallets (like Safe), or a third party integration (like a bridge aggregator s...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between bcc5fc8 and 5912888.

Files selected for processing (3)
  • packages/sdk-router/README.md (1 hunks)
  • packages/sdk-router/src/constants/addresses.ts (1 hunks)
  • packages/sdk-router/src/operations/bridge.ts (5 hunks)
Files skipped from review as they are similar to previous changes (1)
  • packages/sdk-router/src/operations/bridge.ts
Additional context used
LanguageTool
packages/sdk-router/README.md

[uncategorized] ~93-~93: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ncludes smart wallets (like Safe), or a third party integration (like a bridge aggregator s...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

Additional comments not posted (3)
packages/sdk-router/src/constants/addresses.ts (1)

60-60: LGTM! Address format is correct.

The FAST_BRIDGE_ROUTER_ADDRESS has been updated correctly.

packages/sdk-router/README.md (2)

80-89: LGTM! The parameter structure is clear and well-documented.

The updated function call to synapseSDK.allBridgeQuotes now uses an object for parameters, enhancing clarity and extensibility. The mandatory originUserAddress is correctly highlighted.


93-93: LGTM! The note is clear and correctly formatted.

The note correctly emphasizes the necessity of providing the originUserAddress for smart contract integrations.

Tools
LanguageTool

[uncategorized] ~93-~93: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ncludes smart wallets (like Safe), or a third party integration (like a bridge aggregator s...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

@ChiTimesChi ChiTimesChi marked this pull request as draft July 30, 2024 15:07
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
packages/sdk-router/README.md (1)

113-113: Use a hyphen for the compound adjective.

To improve readability, use a hyphen in "third-party integration."

- third party integration
+ third-party integration
Tools
LanguageTool

[uncategorized] ~113-~113: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ncludes smart wallets (like Safe), or a third party integration (like a bridge aggregator s...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5912888 and fb8475a.

Files selected for processing (1)
  • packages/sdk-router/README.md (2 hunks)
Additional context used
LanguageTool
packages/sdk-router/README.md

[uncategorized] ~113-~113: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ncludes smart wallets (like Safe), or a third party integration (like a bridge aggregator s...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

Additional comments not posted (2)
packages/sdk-router/README.md (2)

100-109: Ensure correct usage of the new options object.

The updated allBridgeQuotes function now requires an object with deadline, excludedModules, and originUserAddress. The example provided is clear and accurately reflects the new structure.


113-113: Clarify the importance of originUserAddress.

The note correctly highlights the necessity of providing the originUserAddress when a smart contract initiates the bridge operation. Ensure this information is prominently displayed to avoid transaction reverts.

Tools
LanguageTool

[uncategorized] ~113-~113: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ncludes smart wallets (like Safe), or a third party integration (like a bridge aggregator s...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

@ChiTimesChi
Copy link
Collaborator Author

The typescript tests are expected to pass once the FastBridgeRouterV2 is redeployed and this PR is updated with the correct address

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
packages/sdk-router/README.md (1)

113-113: Emphasize the importance of originUserAddress and correct hyphenation.

The note effectively highlights the critical requirement for smart contract integrations. Correct the hyphenation for clarity.

- ...a third party integration (like a bridge aggregator smart contract).
+ ...a third-party integration (like a bridge aggregator smart contract).
Tools
LanguageTool

[uncategorized] ~113-~113: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ncludes smart wallets (like Safe), or a third party integration (like a bridge aggregator s...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between fb8475a and d3562f2.

Files selected for processing (2)
  • packages/sdk-router/README.md (2 hunks)
  • packages/sdk-router/src/constants/addresses.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • packages/sdk-router/src/constants/addresses.ts
Additional context used
LanguageTool
packages/sdk-router/README.md

[uncategorized] ~113-~113: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ncludes smart wallets (like Safe), or a third party integration (like a bridge aggregator s...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

Additional comments not posted (1)
packages/sdk-router/README.md (1)

100-109: Improved parameter structure for allBridgeQuotes function.

The new structured object for options enhances clarity and maintainability. Ensure that the originUserAddress is correctly populated to avoid transaction reverts when a smart contract initiates the bridge operation.

Copy link

codecov bot commented Aug 2, 2024

Bundle Report

Changes will decrease total bundle size by 7.86MB ⬇️

Bundle name Size Change
widget-esm-cjs 275.25kB 0 bytes
synapse-interface-server-cjs 1.38MB 634 bytes ⬆️
sdk-router-@synapsecns/sdk-router-esm 254.83kB 2.51kB ⬆️
sdk-router-@synapsecns/sdk-router-cjs 117.19kB 404.76kB ⬇️
synapse-interface-edge-server-array-push 83 bytes 0 bytes
synapse-interface-client-array-push (removed) 7.46MB ⬇️

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d3562f2 and 6be5abf.

Files selected for processing (1)
  • packages/sdk-router/src/operations/bridge.ts (6 hunks)
Files skipped from review as they are similar to previous changes (1)
  • packages/sdk-router/src/operations/bridge.ts

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (2)
packages/sdk-router/README.md (2)

117-117: Fix grammatical issue.

Replace "amount of tokens" with "number of tokens."

-  the highest amount of tokens on the destination chain.
+  the highest number of tokens on the destination chain.
Tools
LanguageTool

[uncategorized] ~117-~117: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...th the first quote offering the highest amount of tokens on the destination chain. - A...

(AMOUNTOF_TO_NUMBEROF)


102-111: Ensure originUserAddress is correctly populated in all allBridgeQuotes function calls.

The originUserAddress parameter is crucial for smart contract integrations. Ensure it is correctly populated in all calls to allBridgeQuotes to avoid transaction reverts.

  • Files to update:
    • packages/widget/src/state/slices/bridgeQuote/hooks.ts
    • packages/synapse-interface/pages/state-managed-bridge/index.tsx
    • packages/rest-api/src/app.ts
    • packages/sdk-router/src/sdk.test.ts
    • packages/sdk-router/src/operations/bridge.ts
Analysis chain

Ensure originUserAddress is correctly populated.

The originUserAddress parameter is crucial for smart contract integrations. Ensure it is correctly populated to avoid transaction reverts.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `allBridgeQuotes` include the `originUserAddress` parameter.

# Test: Search for the function usage. Expect: Occurrences of the new structured object.
rg --type ts -A 5 $'allBridgeQuotes'

Length of output: 7573

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6be5abf and d458ac1.

Files selected for processing (1)
  • packages/sdk-router/README.md (2 hunks)
Additional context used
LanguageTool
packages/sdk-router/README.md

[uncategorized] ~117-~117: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...th the first quote offering the highest amount of tokens on the destination chain. - A...

(AMOUNTOF_TO_NUMBEROF)

Additional comments not posted (1)
packages/sdk-router/README.md (1)

115-115: LGTM!

The note clearly highlights the importance of providing the originUserAddress for smart contract integrations.

@ChiTimesChi ChiTimesChi marked this pull request as ready for review August 2, 2024 14:53
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

Support for the FastBridgeRouterV2 contract has been added, introducing a breaking change by consolidating bridge options into a single object.

  • README.md: Updated documentation to reflect the new requirement for the originUserAddress parameter.
  • packages/sdk-router/src/constants/addresses.ts: Updated FastBridgeRouter contract address to 0xd50042193Db100FE0040005e00D5010000007e45.
  • packages/sdk-router/src/operations/bridge.ts: Introduced structured parameters for bridge-related functions, emphasizing the originUserAddress parameter.
  • packages/sdk-router/src/rfq/fastBridgeRouterSet.test.ts: Added tests for createRFQDestQuery method to handle originUserAddress.
  • packages/sdk-router/src/rfq/fastBridgeRouterSet.ts: Modified createRFQDestQuery method visibility to public static for better accessibility.

3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

- **Important:** It is crucial to provide the `originUserAddress` when a smart contract will initiate the bridge operation on behalf of the user. This requirement applies to smart wallets (e.g., Safe) and third-party integrations (such as bridge aggregator smart contracts).
- For native gas tokens (e.g., ETH on Ethereum/Arbitrum, AVAX on Avalanche, etc.), use the specialized address `0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE` instead of the token address for either input or output tokens.
- The returned list is sorted by the `maxAmountOut` field in descending order, with the first quote offering the highest amount of tokens on the destination chain.
- All quotes in the list are provided without any slippage applied. To add slippage to the quotes, use the `applyBridgeSlippage` function.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets change the language to "without any slippage settings applied" saying "without any slippage" doesnt fully communicate the point imo

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1813785

| SynapseBridge | All[^1] | `0x7E7A0e201FD38d3ADAA9523Da6C109a07118C96a` |
| SynapseBridge | Blast | `0x0000000000365b1d5B142732CF4d33BcddED21Fc` |
| SynapseCCTP | All | `0xd5a597d6e7ddf373a92C8f477DAAA673b0902F48` |
| SynapseRFQ | All | `0x00cD000000003f7F682BE4813200893d4e690000` |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are the fastbridge routers all at the same address even though the bridge contract is different on blast and linea?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the FastBridgeRouter has the same address regardless of FastBridge address

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

Support for the FastBridgeRouterV2 contract has been added, introducing a breaking change by consolidating bridge options into a single object.

  • README.md: Updated documentation to reflect the new requirement for the originUserAddress parameter.
  • packages/sdk-router/src/operations/bridge.ts: Introduced structured parameters for bridge-related functions, emphasizing the originUserAddress parameter.
  • packages/sdk-router/src/rfq/fastBridgeRouterSet.ts: Modified createRFQDestQuery method visibility to public static for better accessibility.
  • packages/sdk-router/src/rfq/fastBridgeRouterSet.test.ts: Added tests for createRFQDestQuery method to handle originUserAddress.

2 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
packages/sdk-router/README.md (1)

117-117: Replace "amount" with "number".

"Tokens" is a countable noun, so "number of tokens" is more appropriate than "amount of tokens".

- the highest amount of tokens on the destination chain.
+ the highest number of tokens on the destination chain.
Tools
LanguageTool

[uncategorized] ~117-~117: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...th the first quote offering the highest amount of tokens on the destination chain. - A...

(AMOUNTOF_TO_NUMBEROF)

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d458ac1 and 1813785.

Files selected for processing (2)
  • packages/sdk-router/README.md (3 hunks)
  • packages/sdk-router/src/operations/bridge.ts (6 hunks)
Files skipped from review as they are similar to previous changes (1)
  • packages/sdk-router/src/operations/bridge.ts
Additional context used
LanguageTool
packages/sdk-router/README.md

[uncategorized] ~117-~117: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...th the first quote offering the highest amount of tokens on the destination chain. - A...

(AMOUNTOF_TO_NUMBEROF)

Additional comments not posted (7)
packages/sdk-router/README.md (7)

7-7: No action required.

This line is part of the context and does not require any changes.


49-61: No action required.

These lines provide context for the bridging functionality and do not require any changes.


102-111: Updated usage of allBridgeQuotes function looks good.

The changes correctly reflect the new structure for passing options to the function. Ensure that originUserAddress is correctly populated to avoid transaction reverts.


115-120: Important notes on allBridgeQuotes function usage look good.

The notes correctly emphasize the importance of providing the originUserAddress and other details about the returned quotes.

Tools
LanguageTool

[uncategorized] ~117-~117: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...th the first quote offering the highest amount of tokens on the destination chain. - A...

(AMOUNTOF_TO_NUMBEROF)


122-123: No action required.

These lines provide useful information about the relationship between the bridgeQuote and allBridgeQuotes methods.


49-70: No action required.

These lines provide context for the bridging functionality and do not require any changes.


49-61: No action required.

These lines provide context for the bridging functionality and do not require any changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants