search

synapsecns / sanguine

Synapse Monorepo
MIT License
43 stars 31 forks source link

refactor(contracts-rfq): reorganise imports #3409

Closed ChiTimesChi closed 1 week ago

ChiTimesChi commented 1 week ago

Description A clear and concise description of the features you're adding in this pull request.

Additional context Add any other context about the problem you're solving.

Metadata

Summary by CodeRabbit

Release Notes

coderabbitai[bot] commented 1 week ago

Walkthrough

The changes in this pull request involve modifications to import statements across multiple smart contract files in the contracts-rfq package. Key updates include activating previously commented imports, reordering existing import statements for clarity, and adding new parameters to certain functions in the FastBridgeV2 contract. Overall, the core functionality and logic of the contracts remain unchanged, with a focus on improving code organization and preparation for enhanced transaction handling.

Changes

File Change Summary
packages/contracts-rfq/contracts/Admin.sol Added active import of UniversalTokenLib.
packages/contracts-rfq/contracts/AdminV2.sol Reorganized import statements into categorized sections; no functional changes.
packages/contracts-rfq/contracts/FastBridge.sol Changed order of SafeERC20 and IERC20 imports; no functional changes.
packages/contracts-rfq/contracts/FastBridgeV2.sol Reorganized imports; updated function signatures to include new parameters and deprecate the refund function; enhanced validation in internal methods.
packages/contracts-rfq/contracts/libs/UniversalToken.sol Changed order of SafeERC20 and IERC20 imports; no functional changes.
packages/contracts-rfq/contracts/zaps/TokenZapV1.sol Reorganized imports for clarity; no functional changes.
packages/contracts-rfq/foundry.toml Added sort_imports = true configuration option.
packages/contracts-rfq/test/FastBridgeMock.sol Reordered import of IFastBridge; no functional changes.
packages/contracts-rfq/test/FastBridgeV2.Src.RefundV1.t.sol Reordered imports; no functional changes.
packages/contracts-rfq/test/FastBridgeV2.t.sol Activated import of IFastBridgeV2Errors and reordered other imports; no functional changes.
packages/contracts-rfq/test/MulticallTarget.t.sol Reordered imports; no functional changes.
packages/contracts-rfq/test/UniversalTokenLib.t.sol Reordered imports; no functional changes.
packages/contracts-rfq/test/integration/FastBridge.MulticallTarget.t.sol Reordered imports; no functional changes.
packages/contracts-rfq/test/integration/FastBridgeV2.MulticallTarget.t.sol Reordered imports; no functional changes.
packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Dst.t.sol Reordered imports; no functional changes.
packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Src.t.sol Reordered imports; no functional changes.
packages/contracts-rfq/test/integration/TokenZapV1.t.sol Activated import of VaultManyArguments; no functional changes.
packages/contracts-rfq/test/libs/ZapDataV1.t.sol Reordered imports; no functional changes.
packages/contracts-rfq/test/mocks/VaultMock.sol Changed order of SafeERC20 and IERC20 imports; no functional changes.
packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol Activated import of SimpleVaultMock; no functional changes.
packages/contracts-rfq/test/zaps/TokenZapV1.t.sol Reordered import of VaultManyArguments; no functional changes.

Possibly related PRs

Suggested labels

size/xs, needs-go-generate-services/rfq

Suggested reviewers

🐰 In the code we hop and play,
With imports shuffled, clear as day.
Functions stay true, logic intact,
Enhancing our contracts, that's a fact!
So let’s celebrate this tidy feat,
For every change makes our code neat! πŸ‡

πŸ“œ Recent review details **Configuration used: .coderabbit.yaml** **Review profile: CHILL**
πŸ“₯ Commits Reviewing files that changed from the base of the PR and between 2e16814c9ec38477e722b838229a18505ddacb63 and 6695e64727180cc7b55bec36d25db757dd539ef3.
πŸ“’ Files selected for processing (21) * `packages/contracts-rfq/contracts/Admin.sol` (1 hunks) * `packages/contracts-rfq/contracts/AdminV2.sol` (1 hunks) * `packages/contracts-rfq/contracts/FastBridge.sol` (1 hunks) * `packages/contracts-rfq/contracts/FastBridgeV2.sol` (1 hunks) * `packages/contracts-rfq/contracts/libs/UniversalToken.sol` (1 hunks) * `packages/contracts-rfq/contracts/zaps/TokenZapV1.sol` (1 hunks) * `packages/contracts-rfq/foundry.toml` (1 hunks) * `packages/contracts-rfq/test/FastBridgeMock.sol` (1 hunks) * `packages/contracts-rfq/test/FastBridgeV2.Src.RefundV1.t.sol` (1 hunks) * `packages/contracts-rfq/test/FastBridgeV2.t.sol` (1 hunks) * `packages/contracts-rfq/test/MulticallTarget.t.sol` (1 hunks) * `packages/contracts-rfq/test/UniversalTokenLib.t.sol` (1 hunks) * `packages/contracts-rfq/test/integration/FastBridge.MulticallTarget.t.sol` (1 hunks) * `packages/contracts-rfq/test/integration/FastBridgeV2.MulticallTarget.t.sol` (1 hunks) * `packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Dst.t.sol` (1 hunks) * `packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Src.t.sol` (1 hunks) * `packages/contracts-rfq/test/integration/TokenZapV1.t.sol` (1 hunks) * `packages/contracts-rfq/test/libs/ZapDataV1.t.sol` (1 hunks) * `packages/contracts-rfq/test/mocks/VaultMock.sol` (1 hunks) * `packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol` (1 hunks) * `packages/contracts-rfq/test/zaps/TokenZapV1.t.sol` (1 hunks)
βœ… Files skipped from review due to trivial changes (16) * packages/contracts-rfq/contracts/Admin.sol * packages/contracts-rfq/contracts/AdminV2.sol * packages/contracts-rfq/contracts/FastBridge.sol * packages/contracts-rfq/contracts/libs/UniversalToken.sol * packages/contracts-rfq/test/FastBridgeMock.sol * packages/contracts-rfq/test/FastBridgeV2.Src.RefundV1.t.sol * packages/contracts-rfq/test/MulticallTarget.t.sol * packages/contracts-rfq/test/UniversalTokenLib.t.sol * packages/contracts-rfq/test/integration/FastBridge.MulticallTarget.t.sol * packages/contracts-rfq/test/integration/FastBridgeV2.MulticallTarget.t.sol * packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Dst.t.sol * packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Src.t.sol * packages/contracts-rfq/test/integration/TokenZapV1.t.sol * packages/contracts-rfq/test/libs/ZapDataV1.t.sol * packages/contracts-rfq/test/mocks/VaultMock.sol * packages/contracts-rfq/test/zaps/TokenZapV1.t.sol
🧰 Additional context used
πŸ““ Learnings (2)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1) ``` Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118 Timestamp: 2024-11-12T03:37:08.148Z 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.t.sol (1) ``` Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56 Timestamp: 2024-11-12T03:37:02.230Z Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code. ```
πŸ”‡ Additional comments (9)
packages/contracts-rfq/foundry.toml (1) `20-20`: **LGTM! The formatter configuration aligns with the PR objectives.** The addition of `sort_imports = true` will help maintain consistent import ordering across Solidity files, reducing manual effort and potential conflicts. Let's verify the impact on existing Solidity files:
βœ… Verification successful **Enabling `sort_imports = true` is safe and beneficial for code organization** The current import statements in the Solidity files follow a logical grouping pattern but could benefit from consistent ordering within each group. The formatter configuration will: - Sort imports alphabetically within their respective groups (external, internal) - Maintain the existing grouping structure (interfaces, libraries, contracts) - Help prevent merge conflicts from import reordering - Make the codebase more maintainable
🏁 Scripts executed The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Check the current import ordering in Solidity files # This will help understand the impact of enabling sort_imports # Find all Solidity files excluding legacy contracts fd -e sol -E 'contracts/legacy/**/*' --exec sh -c ' echo "\nFile: {}" # Extract import statements with line numbers rg "^import" -n "{}" ' ``` Length of output: 82541 --- Script: ```shell #!/bin/bash # Let's verify if there are any Solidity files in the packages/contracts-rfq directory # and check their current import ordering # Find all Solidity files in packages/contracts-rfq directory fd -e sol . packages/contracts-rfq/contracts/ --exec sh -c ' echo "\nFile: {}" # Extract import statements with line numbers rg "^import" -n "{}" ' ``` Length of output: 6382
packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol (2) `7-7`: **LGTM! Import statement properly activated** The SimpleVaultMock import is correctly formatted and matches its usage in the test contract. --- `7-7`: **Verify similar test files for consistency** Let's check if other test files in the suite have consistent import patterns for SimpleVaultMock.
βœ… Verification successful **Import pattern for SimpleVaultMock is consistent** The import pattern for SimpleVaultMock is consistent across the codebase. The file `TokenZapV1.GasBench.t.sol` is the only test file that imports and uses SimpleVaultMock, and it does so with the correct relative path from the mocks directory. No inconsistencies were found.
🏁 Scripts executed The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Check for SimpleVaultMock usage patterns across test files # Expected: Find other test files that might need similar import updates # Search for SimpleVaultMock usage in test files rg -t sol "SimpleVaultMock" "packages/contracts-rfq/test/" # Look for potentially commented imports rg -t sol "//.*SimpleVaultMock" "packages/contracts-rfq/test/" ``` Length of output: 175 --- Script: ```shell #!/bin/bash # Search for SimpleVaultMock usage in test files without file type restriction rg "SimpleVaultMock" "packages/contracts-rfq/test/" # Look for import statements of SimpleVaultMock rg "import.*SimpleVaultMock" "packages/contracts-rfq/test/" ``` Length of output: 843
packages/contracts-rfq/contracts/zaps/TokenZapV1.sol (3) `4-9`: **Well-structured import organization!** The new section headers provide clear separation between different types of imports, improving code readability and maintainability. --- `12-14`: **Clean import organization!** The grouping of related imports (IERC20 with SafeERC20) and separation of distinct utilities (Address) follows good practices and improves code clarity. --- `4-14`: **Verify import changes across the codebase** Let's ensure the import reorganization doesn't affect other contracts or tests.
βœ… Verification successful **Import changes are safe and properly organized** The reorganization of imports in TokenZapV1.sol is safe and follows a clear structure: - All files importing TokenZapV1 use the correct relative path - IZapRecipient and ZapDataV1 imports are properly referenced by their consumers - The import grouping (interfaces, internal, external) improves code readability without breaking any dependencies
🏁 Scripts executed The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Search for any files importing TokenZapV1 to verify the changes don't break existing imports rg -l "import.*TokenZapV1" --type solidity # Check for any potential circular dependencies rg -l "import.*\{.*\}" --type solidity | xargs -I {} rg "import" {} || true ``` Length of output: 182 --- Script: ```shell #!/bin/bash # Search for any files importing TokenZapV1 rg "import.*TokenZapV1" # Check for any files that import from the modified file's path rg "import.*zaps/TokenZapV1" # Look for files importing the specific interfaces and libs being reorganized rg "import.*IZapRecipient" rg "import.*ZapDataV1" # Find all Solidity files to analyze import patterns fd -e sol ``` Length of output: 20924
packages/contracts-rfq/test/FastBridgeV2.t.sol (2) `12-12`: **LGTM! Added missing interface import.** Added the import for IFastBridgeV2Errors interface which is required as the contract implements this interface. --- `18-18`: **Reordering of named imports.**
packages/contracts-rfq/contracts/FastBridgeV2.sol (1) `4-19`: **Effective grouping of import statements improves code organization** The import statements have been reorganized into distinct sections: **INTERFACES**, **INTERNAL IMPORTS**, and **EXTERNAL IMPORTS**. This clear categorization enhances code readability and maintainability by making dependencies more apparent and the structure easier to understand.

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 - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
πŸͺ§ Tips ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit , 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](https://docs.coderabbit.ai) for detailed information on how to use CodeRabbit. - Join our [Discord Community](http://discord.gg/coderabbit) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.
codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 41.80602%. Comparing base (748a3a2) to head (6695e64). Report is 2 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3409 +/- ## =================================================== - Coverage 42.36502% 41.80602% -0.55901% =================================================== Files 87 87 Lines 3019 2990 -29 Branches 82 82 =================================================== - Hits 1279 1250 -29 Misses 1737 1737 Partials 3 3 ``` | [Flag](https://app.codecov.io/gh/synapsecns/sanguine/pull/3409/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=synapsecns) | Coverage Ξ” | | |---|---|---| | [packages](https://app.codecov.io/gh/synapsecns/sanguine/pull/3409/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=synapsecns) | `90.44834% <ΓΈ> (ΓΈ)` | | | [solidity](https://app.codecov.io/gh/synapsecns/sanguine/pull/3409/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=synapsecns) | `98.76161% <ΓΈ> (-0.10203%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=synapsecns#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

🚨 Try these New Features:

cloudflare-workers-and-pages[bot] commented 1 week ago

Deploying sanguine-fe with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6695e64
Status: βœ…  Deploy successful!
Preview URL: https://6c85ccd3.sanguine-fe.pages.dev
Branch Preview URL: https://refactor-fbv2-sort-imports.sanguine-fe.pages.dev

View logs