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

chore(contracts-rfq): integration tests for legacy router [SLT-360] #3304

Merged
merged 13 commits into from
Oct 18, 2024

Conversation

ChiTimesChi
Copy link
Collaborator

@ChiTimesChi ChiTimesChi commented Oct 17, 2024

Description
This PR adds integration tests to ensure that the new FastBridge contract is compatible with the legacy FastBridgeRouter contract. The legacy router is currently used in the SDK, and doesn't support any of the new V2 features such as relayer exclusivity or Zaps. It does support the legacy transaction types, which are being tested in the added tests.

Summary by CodeRabbit

  • New Features

    • Introduced the FastBridgeRouterV2 contract for enhanced token bridging across blockchain networks.
    • Added interfaces and contracts for improved routing and liquidity management.
    • Implemented custom error handling for better transaction clarity.
    • Added a new script for filtering coverage reports.
  • Bug Fixes

    • Enhanced testing contracts to ensure proper function visibility and error handling.
  • Chores

    • Updated project configuration files for better dependency management and linter settings.
    • Installed the lcov tool for coverage reporting in the CI workflow.
    • Added a new configuration for Slither static analysis to focus on legacy code paths.

Copy link
Contributor

coderabbitai bot commented Oct 17, 2024

Walkthrough

The pull request introduces several modifications to the "sanguine" project, notably updating the package.json file to generalize the nohoist configuration for OpenZeppelin packages and adding a new dependency on @openzeppelin/contracts-4.5.0. Additionally, it creates new Solidity contracts and interfaces related to a fast bridge mechanism, along with utility libraries and error definitions. The changes also include updates to existing test contracts to enhance their functionality and structure.

Changes

File Path Change Summary
package.json Updated nohoist configuration for OpenZeppelin packages; added dependency on @openzeppelin/contracts-4.5.0.
packages/contracts-rfq/.solhintignore Added contracts/legacy/**/*.sol to ignore list.
packages/contracts-rfq/slither.config.json Introduced slither.config.json for Slither analysis, focusing on legacy code paths.
packages/contracts-rfq/contracts/legacy/rfq/FastBridgeRouterV2.sol Introduced FastBridgeRouterV2 contract with various functions and events for token bridging.
packages/contracts-rfq/contracts/legacy/rfq/interfaces/IFastBridge.sol Added IFastBridge interface with methods and events for bridge transactions.
packages/contracts-rfq/contracts/legacy/rfq/interfaces/IFastBridgeRouter.sol Introduced IFastBridgeRouter interface outlining cross-chain RFQ transaction methods.
packages/contracts-rfq/contracts/legacy/rfq/interfaces/ISwapQuoter.sol Added ISwapQuoter interface for token swap calculations.
packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol Introduced abstract DefaultRouter contract for Synapse Routers.
packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol Added DefaultAdapter contract for routing token swaps and liquidity actions.
packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultExtendedPool.sol Introduced IDefaultExtendedPool interface with methods for liquidity management.
packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultPool.sol Added IDefaultPool interface defining functions for token swaps.
packages/contracts-rfq/contracts/legacy/router/interfaces/IRouterAdapter.sol Introduced IRouterAdapter interface for token swapping.
packages/contracts-rfq/contracts/legacy/router/interfaces/IWETH9.sol Added IWETH9 interface for Ether deposit and withdrawal functions.
packages/contracts-rfq/contracts/legacy/router/libs/Errors.sol Introduced custom error definitions for smart contracts.
packages/contracts-rfq/contracts/legacy/router/libs/Structs.sol Added data structures and libraries for token swaps and liquidity pools.
packages/contracts-rfq/contracts/legacy/router/libs/UniversalToken.sol Introduced UniversalTokenLib for secure token transfer utilities.
packages/contracts-rfq/foundry.toml Added ignore = ["contracts/legacy/**/*.sol"] to formatter configuration.
packages/contracts-rfq/test/FastBridgeV2.Src.FBRV2.t.sol Introduced FastBridgeV2SrcFBRouterV2Test contract for testing FastBridgeRouterV2.
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol Updated visibility of functions in FastBridgeV2SrcTest contract, adding virtual modifiers.

Possibly related PRs

Suggested labels

M-contracts

Suggested reviewers

  • aureliusbtc
  • trajan0x
  • parodime

🐰 In the garden, changes bloom,
New contracts sprout, dispelling gloom.
With bridges built and tokens to share,
A hop of joy fills the air!
In code we trust, our paths align,
For every swap, a tale divine! 🌼


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 118103e and 26ee132.

📒 Files selected for processing (4)
  • .github/workflows/solidity.yml (3 hunks)
  • packages/contracts-rfq/package.json (2 hunks)
  • packages/contracts-rfq/slither.config.json (1 hunks)
  • packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • .github/workflows/solidity.yml
  • packages/contracts-rfq/package.json
  • packages/contracts-rfq/slither.config.json
🧰 Additional context used
📓 Learnings (1)
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (2)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-10-14T13:24:45.293Z
Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.Src.t.sol:919-993
Timestamp: 2024-10-14T14:48:01.520Z
Learning: In Solidity test files for FastBridgeV2 (e.g., `packages/contracts-rfq/test/FastBridgeV2.Src.t.sol`), code duplication in test functions is acceptable to maintain readability and maintainability, even if it contradicts DRY principles.
🔇 Additional comments (6)
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (6)

181-185: Approved: Enhanced extensibility with virtual keyword

The addition of the virtual keyword to this test function is a positive change. It allows for potential overriding in derived test contracts, which can be useful for extending or modifying test behavior in more specific scenarios. The core logic of the test remains intact, ensuring that the original functionality is still tested.


Line range hint 188-193: Approved: Increased flexibility with virtual modifier

The addition of the virtual keyword to this test function is a good improvement. It allows for potential overriding in derived test contracts, which can be beneficial for adapting the test to different scenarios or implementations. The core logic of the test remains unchanged, ensuring that the original functionality is still properly tested.


210-213: Approved: Enhanced test suite flexibility

The addition of the virtual keyword to this test function is a positive change. It allows for potential overriding in derived test contracts, which can be useful for adapting the test to different implementations or scenarios. The core logic of the test remains intact, ensuring that the original functionality is still properly tested.


Line range hint 233-237: Approved: Improved test suite extensibility

The addition of the virtual keyword to this test function is a beneficial change. It allows for potential overriding in derived test contracts, which can be useful for adapting the test to different token implementations or edge cases. The core logic of the test remains unchanged, ensuring that the original functionality is still properly tested.


Line range hint 245-249: Approved: Enhanced test customization potential

The addition of the virtual keyword to this test function is a positive modification. It allows for potential overriding in derived test contracts, which can be beneficial for adapting the test to different sender scenarios or implementations. The core logic of the test remains intact, ensuring that the original functionality is still properly tested.


Line range hint 181-249: Summary: Improved test suite extensibility

The changes made to this test file consistently add the virtual keyword to several test functions. This modification enhances the overall extensibility of the test suite by allowing these functions to be overridden in derived test contracts. This approach provides the following benefits:

  1. Flexibility for future test adaptations
  2. Easier customization for specific edge cases or implementations
  3. Improved maintainability of the test suite

It's important to note that these changes do not alter the existing test logic, ensuring that the original test coverage remains intact. This is a positive improvement that aligns with best practices for creating modular and extensible test suites.


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>, please review it.
    • 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @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 using 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.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

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

cloudflare-workers-and-pages bot commented Oct 17, 2024

Deploying sanguine-fe with  Cloudflare Pages  Cloudflare Pages

Latest commit: 26ee132
Status: ✅  Deploy successful!
Preview URL: https://d1dc0074.sanguine-fe.pages.dev
Branch Preview URL: https://chore-rfq-legacy-router.sanguine-fe.pages.dev

View logs

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.29607%. Comparing base (bd6bd2d) to head (26ee132).
Report is 2 commits behind head on master.

Additional details and impacted files
@@                 Coverage Diff                  @@
##              master       #3304          +/-   ##
====================================================
+ Coverage   28.68684%   92.29607%   +63.60923%     
====================================================
  Files            184          60         -124     
  Lines          12131        1324       -10807     
  Branches         161          82          -79     
====================================================
- Hits            3480        1222        -2258     
+ Misses          8366          99        -8267     
+ Partials         285           3         -282     
Flag Coverage Δ
opbot ?
packages 90.44834% <ø> (ø)
rfq ?
solidity 98.65772% <ø> (+2.34900%) ⬆️

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.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Slither found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@ChiTimesChi ChiTimesChi changed the title chore(contracts-rfq): integration tests for legacy router chore(contracts-rfq): integration tests for legacy router [SLT-360] Oct 17, 2024
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: 18

🧹 Outside diff range and nitpick comments (21)
packages/contracts-rfq/contracts/legacy/router/interfaces/IWETH9.sol (1)

5-7: LGTM: Standard WETH9 function declarations with a minor suggestion.

The deposit and withdraw functions are correctly defined and match the standard WETH9 interface. They are appropriately marked as external, and deposit is correctly set as payable.

Consider adding a brief comment explaining the wad parameter in the withdraw function for improved clarity:

/// @param wad The amount of WETH to withdraw (in wei)
function withdraw(uint256 wad) external;
packages/contracts-rfq/contracts/legacy/router/libs/Errors.sol (2)

4-12: LGTM: Error definitions are well-structured and cover relevant scenarios.

The custom error definitions are clear, concise, and follow a consistent naming convention. They cover various potential issues that might occur in a router contract, which is good for error handling and debugging.

Consider adding parameters to some of the error definitions to provide more context when an error occurs. For example:

error DeadlineExceeded(uint256 deadline, uint256 currentTimestamp);
error InsufficientOutputAmount(uint256 expected, uint256 actual);
error TokenAddressMismatch(address expected, address actual);

This would allow for more informative error messages without significantly increasing gas costs.


1-12: Suggestions for further improvements:

  1. Consider adding a brief comment at the top of the file explaining its purpose and how these errors are intended to be used.

  2. You might want to group related errors together and separate them with blank lines for better readability. For example:

// Timing errors
error DeadlineExceeded();

// Amount errors
error InsufficientOutputAmount();
error MsgValueIncorrect();

// Pool errors
error PoolNotFound();

// Token errors
error TokenAddressMismatch();
error TokenNotContract();
error TokenNotETH();
error TokensIdentical();
  1. If this file is part of a larger system, consider adding a link to relevant documentation or other related contracts in a comment.

These suggestions aim to improve the maintainability and readability of the code.

packages/contracts-rfq/contracts/legacy/rfq/interfaces/ISwapQuoter.sol (2)

4-4: LGTM: Proper import statement. Consider adding a comment for clarity.

The import statement correctly uses the modern Solidity syntax and imports the necessary types. For improved readability, consider adding a brief comment explaining the purpose of these imported types in the context of this interface.

You could add a comment like this:

// Import necessary types for swap quotation
import {LimitedToken, SwapQuery} from "../../router/libs/Structs.sol";

7-12: LGTM: Well-structured function signature. Consider adding NatSpec documentation.

The getAmountOut function is well-defined with appropriate modifiers, clear parameter names, and a suitable return type. To further improve the code, consider adding NatSpec documentation to provide more details about the function's purpose, parameters, and return value.

You could add NatSpec documentation like this:

/// @notice Calculates the output amount for a token swap
/// @param tokenIn The input token details
/// @param tokenOut The address of the output token
/// @param amountIn The amount of input tokens
/// @return query The swap query result
function getAmountOut(
    LimitedToken memory tokenIn,
    address tokenOut,
    uint256 amountIn
) external view returns (SwapQuery memory query);
packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultPool.sol (2)

5-11: LGTM: Well-defined swap function with a minor suggestion.

The swap function is well-structured and includes important parameters for user protection (minimum output and deadline). The return value provides useful information about the swap result.

Consider adding NatSpec comments to provide more detailed documentation for the function and its parameters. This would improve the contract's readability and make it easier for developers to integrate with the interface.


1-20: Overall, well-designed interface that aligns with PR objectives.

The IDefaultPool interface is well-structured and provides the necessary functions for interacting with a default pool in the context of the legacy router. It includes functions for swapping tokens, calculating swap outcomes, and retrieving token addresses, which are essential for maintaining compatibility between the new FastBridge contract and the existing FastBridgeRouter contract.

The interface design supports the PR's objective of introducing integration tests to verify the compatibility of the new FastBridge contract with the existing FastBridgeRouter contract, particularly for legacy transaction types.

To further improve the interface and its integration:

  1. Consider adding events for important actions like successful swaps. This would enhance the contract's observability and make it easier to track activities off-chain.
  2. If not already implemented elsewhere, consider adding functions to query the number of tokens in the pool and their respective indices. This would make the interface more self-contained and easier to use.
packages/contracts-rfq/foundry.toml (1)

Line range hint 1-62: Consider improving file organization and documentation

While the current configuration file is comprehensive, consider the following suggestions to enhance its maintainability and readability:

  1. Add comments to explain the purpose of each major section (e.g., [profile.default], [rpc_endpoints], [etherscan]).
  2. Group related settings together, such as moving all testnet configurations under a separate section.
  3. Consider using environment variables for sensitive information like API keys, if not already done.
  4. Add a brief comment explaining the rationale behind ignoring legacy contracts in the formatting section.

These improvements would make the configuration more self-documenting and easier to maintain in the long run.

packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultExtendedPool.sol (1)

20-20: Avoid using decorative Unicode characters in comments for compatibility.

Using decorative Unicode characters in comments, such as the line separators, can cause issues in code editors or environments that do not support these characters properly. It's recommended to use standard ASCII characters for better compatibility.

Consider replacing the decorative line with standard ASCII characters:

-    // ═══════════════════════════════════════════════════ VIEWS ═══════════════════════════════════════════════════════
+    // ==================================================== VIEWS ====================================================
packages/contracts-rfq/contracts/legacy/router/interfaces/IRouterAdapter.sol (2)

12-12: Correct grammatical error in documentation.

"Swap deadline and slippage is checked outside of this contract." should be "Swap deadline and slippage are checked outside of this contract."


19-25: Consider documenting the expected format of rawParams.

The rawParams parameter is defined as bytes calldata rawParams for additional swap parameters. To facilitate correct implementation, consider providing more details or examples of the expected structure or encoding of rawParams, perhaps through an interface or documentation reference.

packages/contracts-rfq/contracts/legacy/router/libs/UniversalToken.sol (1)

39-39: Use Professional Language in Code Comments

The comment // ETH Chad doesn't require your approval may not be appropriate for a professional codebase. Consider rephrasing it to maintain a professional tone.

You can rephrase it as:

-// ETH Chad doesn't require your approval
+// ETH does not require approval
packages/contracts-rfq/contracts/legacy/rfq/interfaces/IFastBridge.sol (3)

4-4: Address the TODO comment regarding dependency import

There is a TODO comment indicating that the interface should be pulled from the sanguine repository, which requires publishing and adjusting the Solidity pragma. Please address this to ensure the interface uses the centralized and up-to-date contract.

Would you like assistance in updating the import and adjusting the Solidity version pragma?


6-19: Add NatSpec comments to structs for better documentation

Consider adding NatSpec comments to the BridgeTransaction and BridgeProof structs to enhance code readability and provide comprehensive documentation.

Example:

+    /// @notice Represents the details of a bridge transaction.
     struct BridgeTransaction {
         uint32 originChainId;
         uint32 destChainId;
         address originSender; // user (origin)
         address destRecipient; // user (dest)
         address originToken;
         address destToken;
         uint256 originAmount; // amount in on origin bridge less originFeeAmount
         uint256 destAmount;
         uint256 originFeeAmount;
         bool sendChainGas;
         uint256 deadline;
         uint256 nonce;
     }

Proceed similarly for the BridgeProof struct.


28-40: Add NatSpec comments to events for enhanced clarity

Adding NatSpec comments to the events will help developers understand the purpose and usage of each event.

Example:

+    /// @notice Emitted when a bridge request is initiated.
     event BridgeRequested(bytes32 transactionId, address sender, bytes request);
+    /// @notice Emitted when a bridge transaction is relayed by a relayer.
     event BridgeRelayed(
         bytes32 transactionId,
         address relayer,
         address to,
         address token,
         uint256 amount,
         uint256 chainGasAmount
     );

Consider adding comments for all events.

packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol (3)

234-241: Remove Unused Function _getPoolTokens

The function _getPoolTokens is not used anywhere in the contract. Unused code can increase maintenance overhead and may introduce confusion. Consider removing this function to keep the codebase clean and maintainable.

Apply this diff to remove the unused function:

-    /// @dev Returns the tokens in the given pool.
-    function _getPoolTokens(address pool) internal view returns (address[] memory tokens) {
-        uint256 numTokens = _getPoolNumTokens(pool);
-        tokens = new address[](numTokens);
-        for (uint8 i = 0; i < numTokens; ++i) {
-            // This will not revert because we already know the number of tokens in the pool
-            tokens[i] = IDefaultPool(pool).getToken(i);
-        }
-    }
🧰 Tools
🪛 GitHub Check: Slither

[warning] 234-241: Dead-code
DefaultAdapter._getPoolTokens(address) (contracts/legacy/router/adapters/DefaultAdapter.sol#234-241) is never used and should be removed


245-257: Remove Unused Function _getPoolSwapQuote

The function _getPoolSwapQuote is not utilized within the contract. Maintaining unused code can lead to unnecessary complexity. Removing it will streamline the codebase.

Apply this diff to remove the unused function:

-    /// @dev Returns the quote for a swap through the given pool.
-    /// Note: will return 0 on invalid swaps.
-    function _getPoolSwapQuote(
-        address pool,
-        uint8 tokenIndexFrom,
-        uint8 tokenIndexTo,
-        uint256 amountIn
-    ) internal view returns (uint256 amountOut) {
-        try IDefaultPool(pool).calculateSwap(tokenIndexFrom, tokenIndexTo, amountIn) returns (uint256 dy) {
-            amountOut = dy;
-        } catch {
-            // Return 0 instead of reverting
-            amountOut = 0;
-        }
-    }
🧰 Tools
🪛 GitHub Check: Slither

[warning] 245-257: Dead-code
DefaultAdapter._getPoolSwapQuote(address,uint8,uint8,uint256) (contracts/legacy/router/adapters/DefaultAdapter.sol#245-257) is never used and should be removed


29-29: Improve Test Coverage for Key Functions

Several lines and functions within the DefaultAdapter contract are not covered by tests, according to the code coverage report. Ensuring that all critical paths are tested is essential for contract reliability and security.

Consider adding tests for the following areas:

  • The main adapterSwap workflow (lines 29, 59, 72)
  • Parameter validation in _checkParams (line 83)
  • Edge cases involving ETH and WETH wrapping/unwrapping (lines 49, 66, 100, 102, 104)
  • Different execution paths in _performPoolAction and associated functions (_swap, _addLiquidity, _removeLiquidity) (lines 136, 139, 141, 143, 145, 148)

Increasing test coverage will help identify potential issues early and ensure the contract behaves as expected under various scenarios.

Also applies to: 46-46, 49-49, 53-53, 59-59, 66-66, 72-72, 83-83, 100-100, 102-102, 104-104, 120-120, 122-122, 136-136, 139-139, 141-141, 143-143, 145-145, 148-148

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 29-29: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L29
Added line #L29 was not covered by tests

packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol (3)

22-54: Increase test coverage for _doSwap function

According to the code coverage report, some lines within the _doSwap function (lines 32-34, 37, 41-42, 50) are not covered by tests. To ensure reliability and prevent regressions, please consider adding unit tests that cover these code paths.

🧰 Tools
🪛 GitHub Check: Slither

[warning] 22-54: Unused return
DefaultRouter._doSwap(address,address,uint256,SwapQuery) (contracts/legacy/router/DefaultRouter.sol#22-54) ignores return value by IRouterAdapter(routerAdapter).adapterSwap{value: msg.value}({recipient:recipient,tokenIn:tokenIn,amountIn:amountIn,tokenOut:tokenOut,rawParams:query.rawParams}) (contracts/legacy/router/DefaultRouter.sol#42-48)


[notice] 22-54: Block timestamp
DefaultRouter._doSwap(address,address,uint256,SwapQuery) (contracts/legacy/router/DefaultRouter.sol#22-54) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > query.deadline (contracts/legacy/router/DefaultRouter.sol#30)

🪛 GitHub Check: codecov/patch

[warning] 32-34: packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol#L32-L34
Added lines #L32 - L34 were not covered by tests


[warning] 37-37: packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol#L37
Added line #L37 was not covered by tests


[warning] 41-42: packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol#L41-L42
Added lines #L41 - L42 were not covered by tests


[warning] 50-50: packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol#L50
Added line #L50 was not covered by tests


41-50: Consider reentrancy protection when interacting with external adapters

The _doSwap function makes an external call to adapterSwap on lines 42-48. Since this involves an external contract, there is a potential risk of reentrancy attacks. Even though you're calculating amountOut using balance differences, consider adding reentrancy protection using OpenZeppelin's ReentrancyGuard or ensure that the logic is reentrancy-safe.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 41-42: packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol#L41-L42
Added lines #L41 - L42 were not covered by tests


[warning] 50-50: packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol#L50
Added line #L50 was not covered by tests


63-71: Ensure correct handling of tokens with transfer fees

In the _pullToken function (lines 63-71), the amount of tokens pulled is calculated by measuring the balance difference before and after the safeTransferFrom call. While this accounts for tokens with transfer fees or deflationary tokens, ensure that this behavior is intended and properly documented. Users might expect the full amount to be transferred, so clarifying this can prevent confusion.

Would you like assistance in updating the documentation or code comments to clarify this behavior?

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 6880ddd and 02b7871.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (19)
  • package.json (1 hunks)
  • packages/contracts-rfq/.solhintignore (1 hunks)
  • packages/contracts-rfq/contracts/legacy/rfq/FastBridgeRouterV2.sol (1 hunks)
  • packages/contracts-rfq/contracts/legacy/rfq/interfaces/IFastBridge.sol (1 hunks)
  • packages/contracts-rfq/contracts/legacy/rfq/interfaces/IFastBridgeRouter.sol (1 hunks)
  • packages/contracts-rfq/contracts/legacy/rfq/interfaces/ISwapQuoter.sol (1 hunks)
  • packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol (1 hunks)
  • packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol (1 hunks)
  • packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultExtendedPool.sol (1 hunks)
  • packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultPool.sol (1 hunks)
  • packages/contracts-rfq/contracts/legacy/router/interfaces/IRouterAdapter.sol (1 hunks)
  • packages/contracts-rfq/contracts/legacy/router/interfaces/IWETH9.sol (1 hunks)
  • packages/contracts-rfq/contracts/legacy/router/libs/Errors.sol (1 hunks)
  • packages/contracts-rfq/contracts/legacy/router/libs/Structs.sol (1 hunks)
  • packages/contracts-rfq/contracts/legacy/router/libs/UniversalToken.sol (1 hunks)
  • packages/contracts-rfq/foundry.toml (1 hunks)
  • packages/contracts-rfq/package.json (1 hunks)
  • packages/contracts-rfq/test/FastBridgeV2.Src.FBRV2.t.sol (1 hunks)
  • packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (4 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/contracts-rfq/.solhintignore
🧰 Additional context used
📓 Learnings (4)
packages/contracts-rfq/contracts/legacy/rfq/FastBridgeRouterV2.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-10-14T14:45:57.214Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
packages/contracts-rfq/contracts/legacy/rfq/interfaces/IFastBridge.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-10-14T14:45:57.214Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
packages/contracts-rfq/test/FastBridgeV2.Src.FBRV2.t.sol (3)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.Src.t.sol:919-993
Timestamp: 2024-10-14T14:48:01.520Z
Learning: In Solidity test files for FastBridgeV2 (e.g., `packages/contracts-rfq/test/FastBridgeV2.Src.t.sol`), code duplication in test functions is acceptable to maintain readability and maintainability, even if it contradicts DRY principles.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-10-14T13:24:45.293Z
Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-10-14T13:24:45.293Z
Learning: In Solidity test contracts under the `test` directory, such as `FastBridgeV2.t.sol`, contracts are not deployed, so gas consumption is not a concern.
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (2)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-10-14T13:24:45.293Z
Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.Src.t.sol:919-993
Timestamp: 2024-10-14T14:48:01.520Z
Learning: In Solidity test files for FastBridgeV2 (e.g., `packages/contracts-rfq/test/FastBridgeV2.Src.t.sol`), code duplication in test functions is acceptable to maintain readability and maintainability, even if it contradicts DRY principles.
🪛 GitHub Check: Slither
packages/contracts-rfq/contracts/legacy/rfq/FastBridgeRouterV2.sol

[warning] 2-2: Different pragma directives are used
4 different versions of Solidity are used:
- Version constraint 0.8.17 is used by:
-0.8.17 (contracts/legacy/rfq/FastBridgeRouterV2.sol#2)
-0.8.17 (contracts/legacy/router/DefaultRouter.sol#2)
-0.8.17 (contracts/legacy/router/adapters/DefaultAdapter.sol#2)
-0.8.17 (contracts/legacy/router/interfaces/IDefaultExtendedPool.sol#2)
-0.8.17 (contracts/legacy/router/interfaces/IDefaultPool.sol#2)
-0.8.17 (contracts/legacy/router/interfaces/IRouterAdapter.sol#2)
-0.8.17 (contracts/legacy/router/interfaces/IWETH9.sol#2)
-0.8.17 (contracts/legacy/router/libs/Errors.sol#2)
-0.8.17 (contracts/legacy/router/libs/UniversalToken.sol#2)
- Version constraint ^0.8.0 is used by:
-^0.8.0 (contracts/legacy/rfq/interfaces/IFastBridge.sol#2)
-^0.8.0 (contracts/legacy/rfq/interfaces/IFastBridgeRouter.sol#2)
-^0.8.0 (contracts/legacy/rfq/interfaces/ISwapQuoter.sol#2)
-^0.8.0 (node_modules/@openzeppelin/contracts-4.5.0/access/Ownable.sol#4)
-^0.8.0 (node_modules/@openzeppelin/contracts-4.5.0/token/ERC20/IERC20.sol#4)
-^0.8.0 (node_modules/@openzeppelin/contracts-4.5.0/token/ERC20/utils/SafeERC20.sol#4)
-^0.8.0 (node_modules/@openzeppelin/contracts-4.5.0/utils/Context.sol#4)
- Version constraint >=0.8.13 is used by:
->=0.8.13 (contracts/legacy/router/libs/Structs.sol#2)
- Version constraint ^0.8.1 is used by:
-^0.8.1 (node_modules/@openzeppelin/contracts-4.5.0/utils/Address.sol#4)


[warning] 2-2: Incorrect versions of Solidity
Version constraint 0.8.17 contains known severe issues (https://solidity.readthedocs.io/en/latest/bugs.html)
- VerbatimInvalidDeduplication
- FullInlinerNonExpressionSplitArgumentEvaluationOrder
- MissingSideEffectsOnSelectorAccess.
It is used by:
- 0.8.17 (contracts/legacy/rfq/FastBridgeRouterV2.sol#2)
- 0.8.17 (contracts/legacy/router/DefaultRouter.sol#2)
- 0.8.17 (contracts/legacy/router/adapters/DefaultAdapter.sol#2)
- 0.8.17 (contracts/legacy/router/interfaces/IDefaultExtendedPool.sol#2)
- 0.8.17 (contracts/legacy/router/interfaces/IDefaultPool.sol#2)
- 0.8.17 (contracts/legacy/router/interfaces/IRouterAdapter.sol#2)
- 0.8.17 (contracts/legacy/router/interfaces/IWETH9.sol#2)
- 0.8.17 (contracts/legacy/router/libs/Errors.sol#2)
- 0.8.17 (contracts/legacy/router/libs/UniversalToken.sol#2)


[notice] 39-39: Missing zero address validation
FastBridgeRouterV2.setFastBridge(address).fastBridge_ (contracts/legacy/rfq/FastBridgeRouterV2.sol#39) lacks a zero-check on :
- fastBridge = fastBridge_ (contracts/legacy/rfq/FastBridgeRouterV2.sol#40)


[notice] 45-45: Missing zero address validation
FastBridgeRouterV2.setSwapQuoter(address).swapQuoter_ (contracts/legacy/rfq/FastBridgeRouterV2.sol#45) lacks a zero-check on :
- swapQuoter = swapQuoter_ (contracts/legacy/rfq/FastBridgeRouterV2.sol#46)


[notice] 51-92: Block timestamp
FastBridgeRouterV2.bridge(address,uint256,address,uint256,SwapQuery,SwapQuery) (contracts/legacy/rfq/FastBridgeRouterV2.sol#51-92) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > originQuery.deadline (contracts/legacy/rfq/FastBridgeRouterV2.sol#70)


[notice] 95-113: Calls inside a loop
FastBridgeRouterV2.getOriginAmountOut(address,address[],uint256) (contracts/legacy/rfq/FastBridgeRouterV2.sol#95-113) has external calls inside a loop: originQueries[i] = ISwapQuoter(swapQuoter).getAmountOut(LimitedToken({actionMask:ActionLib.allActions(),token:tokenIn}),rfqTokens[i],amountIn) (contracts/legacy/rfq/FastBridgeRouterV2.sol#103-107)


[warning] 118-135: Assembly usage
FastBridgeRouterV2._getOriginSender(bytes) (contracts/legacy/rfq/FastBridgeRouterV2.sol#118-135) uses assembly
- INLINE ASM (contracts/legacy/rfq/FastBridgeRouterV2.sol#123-128)

packages/contracts-rfq/contracts/legacy/rfq/interfaces/IFastBridge.sol

[warning] 2-2: Incorrect versions of Solidity
Version constraint ^0.8.0 contains known severe issues (https://solidity.readthedocs.io/en/latest/bugs.html)
- FullInlinerNonExpressionSplitArgumentEvaluationOrder
- MissingSideEffectsOnSelectorAccess
- AbiReencodingHeadOverflowWithStaticArrayCleanup
- DirtyBytesArrayToStorage
- DataLocationChangeInInternalOverride
- NestedCalldataArrayAbiReencodingSizeValidation
- SignedImmutables
- ABIDecodeTwoDimensionalArrayMemory
- KeccakCaching.
It is used by:
- ^0.8.0 (contracts/legacy/rfq/interfaces/IFastBridge.sol#2)
- ^0.8.0 (contracts/legacy/rfq/interfaces/IFastBridgeRouter.sol#2)
- ^0.8.0 (contracts/legacy/rfq/interfaces/ISwapQuoter.sol#2)
- ^0.8.0 (node_modules/@openzeppelin/contracts-4.5.0/access/Ownable.sol#4)
- ^0.8.0 (node_modules/@openzeppelin/contracts-4.5.0/token/ERC20/IERC20.sol#4)
- ^0.8.0 (node_modules/@openzeppelin/contracts-4.5.0/token/ERC20/utils/SafeERC20.sol#4)
- ^0.8.0 (node_modules/@openzeppelin/contracts-4.5.0/utils/Context.sol#4)

packages/contracts-rfq/contracts/legacy/rfq/interfaces/IFastBridgeRouter.sol

[warning] 71-71: Conformance to Solidity naming conventions
Function IFastBridgeRouter.GAS_REBATE_FLAG() (contracts/legacy/rfq/interfaces/IFastBridgeRouter.sol#71) is not in mixedCase

packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol

[warning] 22-54: Unused return
DefaultRouter._doSwap(address,address,uint256,SwapQuery) (contracts/legacy/router/DefaultRouter.sol#22-54) ignores return value by IRouterAdapter(routerAdapter).adapterSwap{value: msg.value}({recipient:recipient,tokenIn:tokenIn,amountIn:amountIn,tokenOut:tokenOut,rawParams:query.rawParams}) (contracts/legacy/router/DefaultRouter.sol#42-48)


[notice] 22-54: Block timestamp
DefaultRouter._doSwap(address,address,uint256,SwapQuery) (contracts/legacy/router/DefaultRouter.sol#22-54) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > query.deadline (contracts/legacy/router/DefaultRouter.sol#30)

packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol

[warning] 159-175: Unused return
DefaultAdapter._swap(address,DefaultParams,uint256,address) (contracts/legacy/router/adapters/DefaultAdapter.sol#159-175) ignores return value by IDefaultPool(pool).swap({tokenIndexFrom:params.tokenIndexFrom,tokenIndexTo:params.tokenIndexTo,dx:amountIn,minDy:0,deadline:type()(uint256).max}) (contracts/legacy/router/adapters/DefaultAdapter.sol#168-174)


[warning] 179-193: Unused return
DefaultAdapter._addLiquidity(address,DefaultParams,uint256,address) (contracts/legacy/router/adapters/DefaultAdapter.sol#179-193) ignores return value by IDefaultExtendedPool(pool).addLiquidity({amounts:amounts,minToMint:0,deadline:type()(uint256).max}) (contracts/legacy/router/adapters/DefaultAdapter.sol#192)


[warning] 197-212: Unused return
DefaultAdapter._removeLiquidity(address,DefaultParams,uint256,address) (contracts/legacy/router/adapters/DefaultAdapter.sol#197-212) ignores return value by IDefaultExtendedPool(pool).removeLiquidityOneToken({tokenAmount:amountIn,tokenIndex:params.tokenIndexTo,minAmount:0,deadline:type()(uint256).max}) (contracts/legacy/router/adapters/DefaultAdapter.sol#206-211)


[warning] 217-219: Unused return
DefaultAdapter._getPoolLPToken(address) (contracts/legacy/router/adapters/DefaultAdapter.sol#217-219) ignores return value by (None,None,None,None,None,None,lpToken) = IDefaultExtendedPool(pool).swapStorage() (contracts/legacy/router/adapters/DefaultAdapter.sol#218)


[warning] 222-231: Unused return
DefaultAdapter._getPoolNumTokens(address) (contracts/legacy/router/adapters/DefaultAdapter.sol#222-231) ignores return value by IDefaultPool(pool).getToken(index) (contracts/legacy/router/adapters/DefaultAdapter.sol#225-229)


[warning] 234-241: Dead-code
DefaultAdapter._getPoolTokens(address) (contracts/legacy/router/adapters/DefaultAdapter.sol#234-241) is never used and should be removed


[warning] 245-257: Dead-code
DefaultAdapter._getPoolSwapQuote(address,uint8,uint8,uint256) (contracts/legacy/router/adapters/DefaultAdapter.sol#245-257) is never used and should be removed


[failure] 262-266: Functions that send Ether to arbitrary destinations
DefaultAdapter._wrapETH(address,uint256) (contracts/legacy/router/adapters/DefaultAdapter.sol#262-266) sends eth to arbitrary user
Dangerous calls:
- IWETH9(weth).deposit{value: amount}() (contracts/legacy/router/adapters/DefaultAdapter.sol#265)

packages/contracts-rfq/contracts/legacy/router/libs/Structs.sol

[warning] 2-2: Incorrect versions of Solidity
Version constraint >=0.8.13 contains known severe issues (https://solidity.readthedocs.io/en/latest/bugs.html)
- VerbatimInvalidDeduplication
- FullInlinerNonExpressionSplitArgumentEvaluationOrder
- MissingSideEffectsOnSelectorAccess
- StorageWriteRemovalBeforeConditionalTermination
- AbiReencodingHeadOverflowWithStaticArrayCleanup
- DirtyBytesArrayToStorage
- InlineAssemblyMemorySideEffects
- DataLocationChangeInInternalOverride
- NestedCalldataArrayAbiReencodingSizeValidation.
It is used by:
- >=0.8.13 (contracts/legacy/router/libs/Structs.sol#2)

packages/contracts-rfq/contracts/legacy/router/libs/UniversalToken.sol

[warning] 15-30: Low-level calls
Low level call in UniversalTokenLib.universalTransfer(address,address,uint256) (contracts/legacy/router/libs/UniversalToken.sol#15-30):
- (success,None) = to.call{value: value}() (contracts/legacy/router/libs/UniversalToken.sol#25)

🪛 GitHub Check: codecov/patch
packages/contracts-rfq/contracts/legacy/rfq/FastBridgeRouterV2.sol

[warning] 46-47: packages/contracts-rfq/contracts/legacy/rfq/FastBridgeRouterV2.sol#L46-L47
Added lines #L46 - L47 were not covered by tests


[warning] 65-65: packages/contracts-rfq/contracts/legacy/rfq/FastBridgeRouterV2.sol#L65
Added line #L65 was not covered by tests


[warning] 71-71: packages/contracts-rfq/contracts/legacy/rfq/FastBridgeRouterV2.sol#L71
Added line #L71 was not covered by tests


[warning] 74-74: packages/contracts-rfq/contracts/legacy/rfq/FastBridgeRouterV2.sol#L74
Added line #L74 was not covered by tests


[warning] 100-103: packages/contracts-rfq/contracts/legacy/rfq/FastBridgeRouterV2.sol#L100-L103
Added lines #L100 - L103 were not covered by tests


[warning] 110-110: packages/contracts-rfq/contracts/legacy/rfq/FastBridgeRouterV2.sol#L110
Added line #L110 was not covered by tests


[warning] 133-133: packages/contracts-rfq/contracts/legacy/rfq/FastBridgeRouterV2.sol#L133
Added line #L133 was not covered by tests

packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol

[warning] 32-34: packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol#L32-L34
Added lines #L32 - L34 were not covered by tests


[warning] 37-37: packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol#L37
Added line #L37 was not covered by tests


[warning] 41-42: packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol#L41-L42
Added lines #L41 - L42 were not covered by tests


[warning] 50-50: packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol#L50
Added line #L50 was not covered by tests

packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol

[warning] 29-29: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L29
Added line #L29 was not covered by tests


[warning] 46-46: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L46
Added line #L46 was not covered by tests


[warning] 49-49: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L49
Added line #L49 was not covered by tests


[warning] 53-53: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L53
Added line #L53 was not covered by tests


[warning] 59-59: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L59
Added line #L59 was not covered by tests


[warning] 66-66: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L66
Added line #L66 was not covered by tests


[warning] 72-72: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L72
Added line #L72 was not covered by tests


[warning] 83-83: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L83
Added line #L83 was not covered by tests


[warning] 100-100: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L100
Added line #L100 was not covered by tests


[warning] 102-102: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L102
Added line #L102 was not covered by tests


[warning] 104-104: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L104
Added line #L104 was not covered by tests


[warning] 120-120: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L120
Added line #L120 was not covered by tests


[warning] 122-122: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L122
Added line #L122 was not covered by tests


[warning] 136-136: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L136
Added line #L136 was not covered by tests


[warning] 139-139: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L139
Added line #L139 was not covered by tests


[warning] 141-141: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L141
Added line #L141 was not covered by tests


[warning] 143-143: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L143
Added line #L143 was not covered by tests


[warning] 145-145: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L145
Added line #L145 was not covered by tests


[warning] 148-148: packages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol#L148
Added line #L148 was not covered by tests

🔇 Additional comments (30)
packages/contracts-rfq/contracts/legacy/router/interfaces/IWETH9.sol (3)

1-2: LGTM: Appropriate license and Solidity version.

The MIT license is suitable for open-source projects, and Solidity version 0.8.17 is a good choice as it includes important safety features and optimizations.


4-4: LGTM: Well-defined interface for WETH9.

The interface IWETH9 is correctly defined and follows proper naming conventions. It represents the standard interface for Wrapped Ether (WETH9) used in most Ethereum projects.


1-8: Overall assessment: Well-implemented WETH9 interface.

This file introduces a standard IWETH9 interface, which is crucial for interacting with Wrapped Ether in Ethereum-based projects. The interface is correctly defined and includes the essential deposit and withdraw functions. This implementation aligns well with the PR objectives, as it will likely be used in the context of the FastBridge and FastBridgeRouter contracts for handling ETH-WETH conversions in the legacy router integration tests.

packages/contracts-rfq/contracts/legacy/router/libs/Errors.sol (1)

1-2: LGTM: File structure and pragma statement are appropriate.

The SPDX license identifier and Solidity version pragma are correctly placed and specified. Using a fixed Solidity version (0.8.17) is a good practice for library files to ensure consistent behavior across different environments.

packages/contracts-rfq/contracts/legacy/rfq/interfaces/ISwapQuoter.sol (2)

1-2: LGTM: Proper license and Solidity version specified.

The file correctly starts with the MIT license SPDX identifier and specifies a recent Solidity version (^0.8.0), which is good for security and access to newer language features.


6-6: LGTM: Interface declaration follows best practices.

The interface ISwapQuoter is well-named, clearly indicating its purpose and following the common naming convention for interfaces (prefixed with 'I').

packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultPool.sol (4)

1-2: LGTM: Appropriate license and Solidity version.

The file header correctly specifies the MIT license and uses a recent Solidity version (0.8.17), which is suitable for this contract.


4-4: LGTM: Well-named interface.

The interface name IDefaultPool follows the standard naming convention and clearly indicates its purpose as an interface for a default pool.


13-17: LGTM: Well-defined calculateSwap function.

The calculateSwap function is correctly implemented as a view function, allowing users to estimate swap outcomes without executing a transaction. Its parameters and return value are consistent with the swap function, which is good for usability.


19-19: LGTM: Concise and clear getToken function.

The getToken function is well-defined, allowing retrieval of token addresses by their index in the pool. Its simplicity and view modifier are appropriate for this purpose.

packages/contracts-rfq/package.json (1)

11-11: Verify the necessity of multiple OpenZeppelin contract versions

The addition of @openzeppelin/contracts-4.5.0 alongside the existing @openzeppelin/contracts@5.0.1 is unusual and may lead to potential version conflicts or confusion in the codebase.

  1. Can you clarify why both versions are necessary?
  2. Are there specific components in the legacy router that require the older 4.5.0 version?
  3. Have you considered the potential risks of maintaining two versions, such as increased bundle size or inconsistent behavior?

If this is indeed necessary for compatibility reasons, consider adding a comment in the package.json file explaining the rationale behind using both versions to help future maintainers understand this decision.

To ensure this change doesn't introduce unintended consequences, please run the following script to check for any conflicting usage of OpenZeppelin contracts:

This will help identify any potential issues with mixing different versions of OpenZeppelin contracts in your Solidity files.

packages/contracts-rfq/foundry.toml (1)

15-15: LGTM: Ignoring legacy contracts from formatting

The addition of ignore = ["contracts/legacy/**/*.sol"] in the [fmt] section is a good practice. This configuration ensures that legacy contracts are not affected by automatic formatting, which helps maintain compatibility and reduces the risk of unintended changes to existing code.

This change aligns well with the PR objectives of ensuring compatibility between new and legacy components, as it prevents potential formatting-related issues in legacy contracts.

package.json (1)

15-15: Approved: Generalized nohoist configuration for OpenZeppelin packages

The change to use "**/@openzeppelin/*" in the nohoist configuration is a good optimization. It simplifies the configuration and ensures all OpenZeppelin packages are treated consistently.

However, this change might affect how OpenZeppelin packages are hoisted in the project's workspace. Please verify that this doesn't cause any unexpected behavior in the project's dependencies. You can run the following command to check the project's dependency tree:

This will show how these packages are resolved in the project structure after the nohoist change.

packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (6)

179-183: Approved: Enhanced extensibility for test function.

The change from public to public virtual for the test_bridge_token_revert_approvedZero function is a good improvement. This modification allows the function to be overridden in derived contracts, enhancing the extensibility of the test suite without altering the core test logic.


Line range hint 186-191: Approved: Consistent extensibility improvement.

The modification of test_bridge_token_revert_approvedNotEnough from public to public virtual is consistent with the previous change. This alteration maintains the test logic while allowing for potential overrides in derived contracts, further enhancing the test suite's flexibility.


208-211: Approved: Consistent pattern of extensibility improvements.

The change of test_bridge_eth_revert_zeroMsgValue to public virtual follows the established pattern in this update. This modification maintains consistency across the test suite, allowing for potential customization in derived contracts while preserving the original test logic.


Line range hint 231-235: Approved: Aligned with extensibility improvement strategy.

The modification of test_bridge_revert_zeroOriginToken to public virtual is in line with the overall strategy of this update. This change maintains the integrity of the test while enabling potential customization in derived contracts, contributing to a more flexible and extensible test suite.


Line range hint 243-247: Approved: Completes consistent extensibility improvements.

The change of test_bridge_revert_zeroSender to public virtual completes the pattern of modifications in this update. This consistent approach across multiple test functions enhances the overall extensibility of the test suite, allowing for potential customizations in derived contracts while maintaining the integrity of the original tests.


Line range hint 179-247: Summary: Enhanced test suite extensibility.

The modifications in this file consistently change several test function signatures from public to public virtual. This pattern of changes improves the extensibility of the test suite by allowing these functions to be overridden in derived contracts. The core test logic remains unchanged, preserving the integrity of the existing tests while providing flexibility for future customizations or extensions.

These changes are well-structured and aligned with best practices for creating maintainable and extensible test suites in Solidity. The consistency of the modifications across multiple functions indicates a thoughtful approach to improving the overall design of the test contract.

packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultExtendedPool.sol (1)

6-45: Interface IDefaultExtendedPool is well-defined and consistent with Solidity conventions.

The interface extends IDefaultPool appropriately, and the function declarations are clear and correctly specified. The use of external, view, and the return types are correctly applied.

packages/contracts-rfq/contracts/legacy/router/interfaces/IRouterAdapter.sol (2)

6-8: Clarify the definition of ETH_ADDRESS.

The documentation refers to ETH_ADDRESS to represent native ETH. Ensure that ETH_ADDRESS is consistently defined and accessible, perhaps by importing it from a common constants contract or library, to avoid confusion and enhance maintainability.


1-26: Overall, the interface is well-defined and follows Solidity best practices.

The IRouterAdapter interface is structured properly with comprehensive documentation, which will aid implementers in correctly adhering to the expected behaviors.

packages/contracts-rfq/test/FastBridgeV2.Src.FBRV2.t.sol (3)

9-14: Contract definition and error declarations look good

The contract FastBridgeV2SrcFBRouterV2Test correctly extends FastBridgeV2SrcTest. The public variable router and the custom errors FastBridgeRouterV2__OriginSenderNotSpecified and TokenNotContract are appropriately declared.


15-23: setUp function is properly overridden

The setUp function initializes the test environment by deploying the FastBridgeRouterV2 contract, setting the fast bridge address, and approving token transfers for userA and userB. The implementation aligns with the testing requirements.


25-49: bridge function override is implemented correctly

The overridden bridge function leverages the FastBridgeRouterV2 contract, accurately constructs the destQueryParams, and sets up the SwapQuery parameters. The function correctly simulates the bridging logic for testing purposes.

packages/contracts-rfq/contracts/legacy/rfq/interfaces/IFastBridgeRouter.sol (1)

1-70: LGTM

The interface and function declarations adhere to Solidity best practices.

Also applies to: 72-78

packages/contracts-rfq/contracts/legacy/router/libs/Structs.sol (3)

6-46: Well-Structured Definitions of Token and Pool Structs

The various structs (BridgeToken, IndexedToken, LimitedToken, PoolToken, Pool) are well-defined and documented, enhancing code clarity and maintainability.


78-96: Appropriate Use of SwapQueryLib

The SwapQueryLib provides useful utility functions for managing SwapQuery objects. The implementation is sound and adheres to best practices.


112-143: Effective Implementation of Action Enum and ActionLib

The Action enumeration and associated ActionLib library provide a flexible system for managing actions using bitmasking techniques. This is an efficient approach for action management and allows for easy expansion of actions in the future.

packages/contracts-rfq/contracts/legacy/router/DefaultRouter.sol (1)

42-48: ⚠️ Potential issue

Handle the ignored return value from adapterSwap

The call to adapterSwap on lines 42-48 ignores its return value. If adapterSwap returns a value, it should be captured and handled appropriately. If it does not return a value, consider updating the IRouterAdapter interface to reflect this to avoid confusion.

Run the following script to check if adapterSwap returns a value in IRouterAdapter:

@github-actions github-actions bot added the M-ci Module: CI label Oct 17, 2024
@ChiTimesChi
Copy link
Collaborator Author

@coderabbitai resolve

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 and nitpick comments (1)
.github/workflows/solidity.yml (1)

225-229: LGTM! Consider adding a comment for clarity.

The addition of this step to filter coverage reports is a good improvement. It allows for more accurate coverage reporting by excluding irrelevant files on a per-package basis.

Consider adding a brief comment explaining the purpose of this step, for example:

# Filter coverage report to exclude irrelevant files (e.g., tests, legacy code)
- name: Apply filters to coverage report
  if: ${{ matrix.package != 'solidity-devops' }}
  working-directory: './packages/${{matrix.package}}'
  run: npm run coverage:filter --if-present

This will help other developers understand the purpose of this step at a glance.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 844e353 and baedb58.

📒 Files selected for processing (2)
  • .github/workflows/solidity.yml (1 hunks)
  • packages/contracts-rfq/package.json (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/contracts-rfq/package.json
🧰 Additional context used

Copy link
Contributor

coderabbitai bot commented Oct 17, 2024

✅ Actions performed

Comments resolved.

@ChiTimesChi ChiTimesChi merged commit c209f6b into master Oct 18, 2024
57 checks passed
@ChiTimesChi ChiTimesChi deleted the chore/rfq-legacy-router branch October 18, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant