2024-09-26 FE Release #3155
2024-09-26 FE Release #3155
Conversation
- use `forge fmt` as formatter in Solidity extension
* fix swaptxinfo function * Updates test coverage command * migrating to using token addresses instead of symbols * fix linting errors * fixing swaptxinfocontroller * new tests and new functionality --------- Co-authored-by: abtestingalpha <abtestingalpha@gmail.com>
Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
* merging rfq indexer into monorepo * nuke .env * fix commands * fix package name * test coverage script * rough pass at docs and some linting and fixes yarn * Upgrades wagmi & rainbowkit * indxer * Adds invisible but used packages * +recent-invalid-fills [SLT-188] * Moves wagmi to root * new endpoints and clean up linting --------- Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com> Co-authored-by: abtestingalpha <abtestingalpha@gmail.com> Co-authored-by: parodime <jordan@protochainresearch.com>
* Adds /destinationTokens route * ZeroAddress & NativeGasAddress * Adds test for native gas tokens * Checksums incoming token address params
* boba pause * only boba to txns
Defi-Moses
requested review from
trajan0x,
ChiTimesChi,
bigboydiamonds,
abtestingalpha and
aureliusbtc
as code owners
WalkthroughThis pull request introduces significant updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsError running LanguageTool: LanguageTool error: Unknown error 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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
javascript
Defi-Moses
temporarily deployed
to
github-pages
Deploying sanguine-fe with
|
| Latest commit: |
ebf102c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://44a9f189.sanguine-fe.pages.dev |
| Branch Preview URL: | https://master.sanguine-fe.pages.dev |
abtestingalpha
temporarily deployed
to
github-pages
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/synapse-interface/CHANGELOG.md (2)
22-27: LGTM with a minor suggestion: Feature entry is informative, but heading level could be adjusted.The changelog entry for version 0.39.0 provides good information about the new feature, including a reference to the issue number and commit hash. This level of detail is appropriate for a feature release.
However, to maintain consistency with the markdown structure, consider changing the "Features" subheading from h3 (###) to h2 (##). This will ensure proper nesting of headings throughout the document.
- ### Features + ## Features🧰 Tools
🪛 Markdownlint
25-25: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
Line range hint
1-83: LGTM with a minor suggestion: Overall changelog structure and formatting are good.The changelog follows best practices and maintains a consistent format throughout:
- Version numbers are properly linked to compare views.
- Entries are clear and concise.
- Changes are categorized when necessary (e.g., Features, Bug Fixes).
To further improve the document:
- Ensure consistent use of heading levels. When using subheadings for categories (like Features or Bug Fixes), maintain h2 (##) throughout the document.
- Consider adding a brief description of the change type for version bumps (e.g., "Patch update" or "Dependency updates") to provide more context.
These minor adjustments will enhance the readability and consistency of the changelog.
🧰 Tools
🪛 Markdownlint
25-25: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- packages/contracts-rfq/CHANGELOG.md (1 hunks)
- packages/contracts-rfq/package.json (1 hunks)
- packages/synapse-interface/CHANGELOG.md (1 hunks)
- packages/synapse-interface/README.md (1 hunks)
- packages/synapse-interface/package.json (4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/synapse-interface/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/synapse-interface/package.json
🧰 Additional context used
🪛 Markdownlint
packages/contracts-rfq/CHANGELOG.md
52-52: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
85-85: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
packages/synapse-interface/CHANGELOG.md
25-25: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
🔇 Additional comments (12)
packages/contracts-rfq/package.json (5)
8-8: Verify the new contract file structure.The change from "src/.sol" to "contracts/**/.sol" indicates a move towards a more standard and flexible project structure for Solidity contracts. This is a good practice, especially for larger projects.
Please ensure that this change aligns with the actual file structure in the repository. Run the following script to verify:
#!/bin/bash # Description: Verify the new contract file structure echo "Listing Solidity files in the 'contracts' directory:" fd -e sol . contracts echo -e "\nChecking for any Solidity files in the 'src' directory (should be empty):" fd -e sol . src echo -e "\nListing all Solidity files in the repository:" fd -e sol .If the output doesn't match the expected structure, please update the "files" field in package.json accordingly.
14-15: Dependency updates and removals require attention.
The "@synapsecns/solidity-devops" package has been updated from "^0.4.4" to "^0.4.6". This minor update may introduce new features or bug fixes.
The removal of "prettier" and "prettier-plugin-solidity" suggests a change in the code formatting strategy. Ensure that this aligns with the team's decision and update any related documentation or CI/CD processes.
The "solhint" update from "^3.3.6" to "5.0.3" is a major version bump, which may introduce breaking changes or new linting rules.
Please ensure the following:
- Review the changelog for "@synapsecns/solidity-devops" to understand the changes in the new version.
- Update any CI/CD processes or documentation related to code formatting, given the removal of prettier.
- Review the solhint changelog for version 5.0.3 and update your solhint configuration file if necessary.
#!/bin/bash # Description: Check for prettier configuration and solhint version # Check for prettier configuration echo "Checking for prettier configuration files:" fd -H -I -tf '\.prettierrc(\.js|\.json|\.yml|\.yaml)?$' # Check solhint version in node_modules echo -e "\nChecking installed solhint version:" if [ -d "node_modules/solhint" ]; then cat node_modules/solhint/package.json | jq '.version' else echo "solhint is not installed in node_modules" fi # Check for solhint configuration echo -e "\nChecking for solhint configuration files:" fd -H -I -tf '\.solhint(rc|\.json)?$'
23-25: Script updates reflect changes in linting and formatting strategy.The changes to the scripts section indicate a shift in the project's linting and formatting approach:
- The
lintscript now usesforge fmtinstead of prettier, and includes solhint for linting.- New
lint:checkandci:lintscripts have been added for more granular control over the linting process.- The
solhintandsolhint:checkscripts have been updated with new paths and a maximum warning limit.These changes align with the removal of prettier from the dependencies and the update to solhint.
Please ensure the following:
- Verify that
forge fmtprovides the desired formatting results.- Update any CI/CD pipelines to use the new
ci:lintscript.- Review the solhint configuration to ensure it aligns with the new script options.
#!/bin/bash # Description: Verify forge installation and solhint configuration # Check forge installation echo "Checking forge installation:" if command -v forge &> /dev/null; then forge --version else echo "forge is not installed or not in PATH" fi # Check solhint configuration echo -e "\nChecking solhint configuration:" if [ -f .solhint.json ]; then cat .solhint.json elif [ -f .solhintrc ]; then cat .solhintrc else echo "No solhint configuration file found" fiAlso applies to: 27-28
18-18:⚠️ Potential issueEmpty build script may lead to confusion.
The
buildscript is currently empty ("build": " "). This could potentially cause issues if any processes depend on this script.Please clarify:
- Is the build process handled elsewhere, making this script unnecessary?
- If a build process is needed, what should this script contain?
- If the script is truly not needed, consider removing it entirely rather than leaving it empty.
To check if there are any other build-related scripts or if this empty script is referenced elsewhere, run:
#!/bin/bash # Description: Check for build-related scripts and references echo "All scripts containing 'build':" jq '.scripts | to_entries[] | select(.key | contains("build")) | .key + ": " + .value' package.json echo -e "\nChecking for references to 'npm run build' in the repository:" rg "npm run build"
2-4: Significant package metadata updates require additional actions.The package metadata changes look good and indicate important shifts in the project:
- The package name change to "@synapsecns/contracts-rfq" suggests a move towards a more organized, scoped package structure.
- The license change to MIT is significant, indicating a shift towards open-source.
- The version bump from 0.3.0 to 0.5.5 is substantial.
Please ensure the following:
- Update all references to the old package name "FastBridge" throughout the codebase.
- Add or update the LICENSE file in the root directory with the MIT license text.
- Update the CHANGELOG.md file to reflect the changes introduced in versions 0.3.1 through 0.5.5.
packages/contracts-rfq/CHANGELOG.md (5)
6-9: LGTM: Version 0.5.5 entry is correct and consistent.The changelog entry for version 0.5.5 is properly formatted and correctly indicates a version bump. The compare URL uses the correct package name.
14-17: LGTM: Version 0.5.4 entry is correct and consistent.The changelog entry for version 0.5.4 is properly formatted and correctly indicates a version bump. The compare URL uses the correct package name.
22-25: LGTM: Version 0.5.3 entry is correct and consistent.The changelog entry for version 0.5.3 is properly formatted and correctly indicates a version bump. The compare URL uses the correct package name.
30-33: LGTM: Version 0.5.2 entry is correct and consistent.The changelog entry for version 0.5.2 is properly formatted and correctly indicates a version bump. The compare URL uses the correct package name.
38-43: LGTM: Version 0.5.1 entry is well-documented and consistent.The changelog entry for version 0.5.1 is properly formatted and documents the bug fix for limiting solhint warnings. The entry adheres to conventional commit guidelines and includes links to the relevant issue (SLT-245) and pull request (#3182).
packages/synapse-interface/CHANGELOG.md (2)
6-8: LGTM: Version bump entry is correct and follows the established format.The changelog entry for version 0.39.2 is properly formatted and clearly indicates that it's a version bump only, with no functional changes.
14-16: LGTM: Version bump entry is correct and follows the established format.The changelog entry for version 0.39.1 is properly formatted and clearly indicates that it's a version bump only, with no functional changes.
| ## [0.4.1](https://github.com/synapsecns/sanguine/compare/FastBridge@0.4.0...FastBridge@0.4.1) (2024-09-23) | ||
|
|
||
| **Note:** Version bump only for package FastBridge |
There was a problem hiding this comment.
Update package name in compare URL for consistency.
The changelog entry for version 0.4.1 correctly indicates that it's only a version bump. However, there's still an inconsistency in the package name used in the compare URL.
Update the compare URL to use the correct package name:
-## [0.4.1](https://github.com/synapsecns/sanguine/compare/FastBridge@0.4.0...FastBridge@0.4.1) (2024-09-23)
+## [0.4.1](https://github.com/synapsecns/sanguine/compare/@synapsecns/contracts-rfq@0.4.0...@synapsecns/contracts-rfq@0.4.1) (2024-09-23)This change ensures consistency with the package name used in other entries and prevents potential confusion.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## [0.4.1](https://github.com/synapsecns/sanguine/compare/FastBridge@0.4.0...FastBridge@0.4.1) (2024-09-23) | |
| **Note:** Version bump only for package FastBridge | |
| ## [0.4.1](https://github.com/synapsecns/sanguine/compare/@synapsecns/contracts-rfq@0.4.0...@synapsecns/contracts-rfq@0.4.1) (2024-09-23) | |
| **Note:** Version bump only for package FastBridge |
| # [0.4.0](https://github.com/synapsecns/sanguine/compare/FastBridge@0.3.0...FastBridge@0.4.0) (2024-09-20) | ||
|
|
||
|
|
||
| ### Features | ||
|
|
||
| * **contracts-rfq:** relay/prove/claim with different address [SLT-130] ([#3138](https://github.com/synapsecns/sanguine/issues/3138)) ([23f6c4c](https://github.com/synapsecns/sanguine/commit/23f6c4c652743c5ca7a184ad730ce19af3600a9c)) |
There was a problem hiding this comment.
Update package name in compare URL and approve feature documentation.
The changelog entry for version 0.4.0 is well-documented, including a clear link to the issue (SLT-130) and pull request (#3138) for the new feature. However, there's an inconsistency in the package name used in the compare URL.
Please update the compare URL to use the correct package name:
-# [0.4.0](https://github.com/synapsecns/sanguine/compare/FastBridge@0.3.0...FastBridge@0.4.0) (2024-09-20)
+# [0.4.0](https://github.com/synapsecns/sanguine/compare/@synapsecns/contracts-rfq@0.3.0...@synapsecns/contracts-rfq@0.4.0) (2024-09-20)This change ensures consistency with the package name used in other entries and prevents potential confusion.
The documentation of the feature itself is approved.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # [0.4.0](https://github.com/synapsecns/sanguine/compare/FastBridge@0.3.0...FastBridge@0.4.0) (2024-09-20) | |
| ### Features | |
| * **contracts-rfq:** relay/prove/claim with different address [SLT-130] ([#3138](https://github.com/synapsecns/sanguine/issues/3138)) ([23f6c4c](https://github.com/synapsecns/sanguine/commit/23f6c4c652743c5ca7a184ad730ce19af3600a9c)) | |
| # [0.4.0](https://github.com/synapsecns/sanguine/compare/@synapsecns/contracts-rfq@0.3.0...@synapsecns/contracts-rfq@0.4.0) (2024-09-20) | |
| ### Features | |
| * **contracts-rfq:** relay/prove/claim with different address [SLT-130] ([#3138](https://github.com/synapsecns/sanguine/issues/3138)) ([23f6c4c](https://github.com/synapsecns/sanguine/commit/23f6c4c652743c5ca7a184ad730ce19af3600a9c)) |
🧰 Tools
🪛 Markdownlint
85-85: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
| # 0.5.0 (2024-09-24) | ||
|
|
||
|
|
||
| ### Bug Fixes | ||
|
|
||
| * adjust to [#2658](https://github.com/synapsecns/sanguine/issues/2658) breaking changes ([#2688](https://github.com/synapsecns/sanguine/issues/2688)) ([7533f70](https://github.com/synapsecns/sanguine/commit/7533f70f4bc88f334af26a009c81c9d581d3997f)) | ||
| * **contracts-rfq:** CI workflows [SLT-245] ([#3178](https://github.com/synapsecns/sanguine/issues/3178)) ([74b620e](https://github.com/synapsecns/sanguine/commit/74b620e4c928be8d0dbb422708376d167db7848d)) | ||
|
|
||
|
|
||
| ### Features | ||
|
|
||
| * **contracts-rfq:** Deploy new RFQ Contracts ([#2255](https://github.com/synapsecns/sanguine/issues/2255)) ([b3a51e2](https://github.com/synapsecns/sanguine/commit/b3a51e28c037a93fb62fd064c3c2df5e901bd79d)) | ||
| * **contracts-rfq:** Multicall target abstraction [SLT-134] ([#3078](https://github.com/synapsecns/sanguine/issues/3078)) ([100324f](https://github.com/synapsecns/sanguine/commit/100324f269f77f73fc10913d0162676f5f918996)) | ||
| * **contracts-rfq:** relay/prove/claim with different address [SLT-130] ([#3138](https://github.com/synapsecns/sanguine/issues/3138)) ([23f6c4c](https://github.com/synapsecns/sanguine/commit/23f6c4c652743c5ca7a184ad730ce19af3600a9c)) | ||
| * **rfq:** Deploy RFQ to Base ([#2397](https://github.com/synapsecns/sanguine/issues/2397)) ([b73538e](https://github.com/synapsecns/sanguine/commit/b73538e93dfe6ecc0ce6806e75d4d1c59328c7b3)) | ||
|
|
||
|
|
||
| ### Reverts | ||
|
|
||
| * Revert "update bl" ([3693110](https://github.com/synapsecns/sanguine/commit/3693110e0f9df6177935bbbfba5444df62b11866)) |
There was a problem hiding this comment.
Fix heading levels in version 0.5.0 entry.
The version 0.5.0 entry is well-documented with clear links to issues and pull requests for each change. However, there's still an inconsistency in the heading levels for "Bug Fixes", "Features", and "Reverts".
To maintain consistency, please update the heading levels as follows:
# 0.5.0 (2024-09-24)
-### Bug Fixes
+## Bug Fixes
[...]
-### Features
+## Features
[...]
-### Reverts
+## RevertsThis change will ensure that all subheadings within the version entry are at the same level (h2).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 0.5.0 (2024-09-24) | |
| ### Bug Fixes | |
| * adjust to [#2658](https://github.com/synapsecns/sanguine/issues/2658) breaking changes ([#2688](https://github.com/synapsecns/sanguine/issues/2688)) ([7533f70](https://github.com/synapsecns/sanguine/commit/7533f70f4bc88f334af26a009c81c9d581d3997f)) | |
| * **contracts-rfq:** CI workflows [SLT-245] ([#3178](https://github.com/synapsecns/sanguine/issues/3178)) ([74b620e](https://github.com/synapsecns/sanguine/commit/74b620e4c928be8d0dbb422708376d167db7848d)) | |
| ### Features | |
| * **contracts-rfq:** Deploy new RFQ Contracts ([#2255](https://github.com/synapsecns/sanguine/issues/2255)) ([b3a51e2](https://github.com/synapsecns/sanguine/commit/b3a51e28c037a93fb62fd064c3c2df5e901bd79d)) | |
| * **contracts-rfq:** Multicall target abstraction [SLT-134] ([#3078](https://github.com/synapsecns/sanguine/issues/3078)) ([100324f](https://github.com/synapsecns/sanguine/commit/100324f269f77f73fc10913d0162676f5f918996)) | |
| * **contracts-rfq:** relay/prove/claim with different address [SLT-130] ([#3138](https://github.com/synapsecns/sanguine/issues/3138)) ([23f6c4c](https://github.com/synapsecns/sanguine/commit/23f6c4c652743c5ca7a184ad730ce19af3600a9c)) | |
| * **rfq:** Deploy RFQ to Base ([#2397](https://github.com/synapsecns/sanguine/issues/2397)) ([b73538e](https://github.com/synapsecns/sanguine/commit/b73538e93dfe6ecc0ce6806e75d4d1c59328c7b3)) | |
| ### Reverts | |
| * Revert "update bl" ([3693110](https://github.com/synapsecns/sanguine/commit/3693110e0f9df6177935bbbfba5444df62b11866)) | |
| # 0.5.0 (2024-09-24) | |
| ## Bug Fixes | |
| * adjust to [#2658](https://github.com/synapsecns/sanguine/issues/2658) breaking changes ([#2688](https://github.com/synapsecns/sanguine/issues/2688)) ([7533f70](https://github.com/synapsecns/sanguine/commit/7533f70f4bc88f334af26a009c81c9d581d3997f)) | |
| * **contracts-rfq:** CI workflows [SLT-245] ([#3178](https://github.com/synapsecns/sanguine/issues/3178)) ([74b620e](https://github.com/synapsecns/sanguine/commit/74b620e4c928be8d0dbb422708376d167db7848d)) | |
| ## Features | |
| * **contracts-rfq:** Deploy new RFQ Contracts ([#2255](https://github.com/synapsecns/sanguine/issues/2255)) ([b3a51e2](https://github.com/synapsecns/sanguine/commit/b3a51e28c037a93fb62fd064c3c2df5e901bd79d)) | |
| * **contracts-rfq:** Multicall target abstraction [SLT-134] ([#3078](https://github.com/synapsecns/sanguine/issues/3078)) ([100324f](https://github.com/synapsecns/sanguine/commit/100324f269f77f73fc10913d0162676f5f918996)) | |
| * **contracts-rfq:** relay/prove/claim with different address [SLT-130] ([#3138](https://github.com/synapsecns/sanguine/issues/3138)) ([23f6c4c](https://github.com/synapsecns/sanguine/commit/23f6c4c652743c5ca7a184ad730ce19af3600a9c)) | |
| * **rfq:** Deploy RFQ to Base ([#2397](https://github.com/synapsecns/sanguine/issues/2397)) ([b73538e](https://github.com/synapsecns/sanguine/commit/b73538e93dfe6ecc0ce6806e75d4d1c59328c7b3)) | |
| ## Reverts | |
| * Revert "update bl" ([3693110](https://github.com/synapsecns/sanguine/commit/3693110e0f9df6177935bbbfba5444df62b11866)) |
🧰 Tools
🪛 Markdownlint
52-52: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
* Txn transaction refund tracking * Update store to support tracking * Query FastBridge contract for `bridgeStatuses` to find refund status * Track bridge transaction `bridgeQuote.routerAddress` in store * Fetch FastBridge contract address when only provided router address * add translations
* no empty sender/to addrs [SLT-184] * enable tests * code reformat
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (23)
packages/contracts-rfq/package.json (1)
1-30: Summary of changes and remaining concernsThis update to
package.jsonintroduces significant changes to the project structure, dependencies, and development practices:
- Package metadata updated (name, version, license).
- Project structure changed from "src" to "contracts" directory.
- Dependencies updated, with prettier removed and solhint upgraded.
- Linting and formatting processes updated to use forge and solhint.
- Build script left empty.
While these changes generally improve the project setup, there are a few remaining concerns:
- Ensure LICENSE and CHANGELOG.md files are added to reflect the license change and version update.
- Verify that the new file structure correctly includes all necessary Solidity files.
- Address the empty build script to prevent potential build inconsistencies.
- Update any CI/CD pipelines or documentation to reflect the new linting and formatting processes.
Please review and address these concerns to ensure a smooth transition to the new project setup.
Consider the following architectural improvements:
- If this package is intended to be used as a dependency in other projects, ensure that the
"private": truefield is removed or set tofalse.- Consider adding a
"prepare"script that runsforge buildto ensure the contracts are compiled before the package is published or installed.- If there are any generated artifacts that should be included in the package (e.g., ABI files), make sure to add them to the
"files"field.packages/synapse-interface/components/_Transaction/helpers/calculateEstimatedTimeStatus.ts (3)
24-24: LGTM: New check for refund eligibility after 4 hours.The addition of
isCheckTxForRefundis a good implementation for checking if a transaction has been running for more than 4 hours, potentially indicating the need for a refund check.Consider renaming the variable to
shouldCheckTxForRefundfor consistency with other boolean variables in the function that start with "is" or "should".- const isCheckTxForRefund = elapsedTime > fourHoursInSeconds + const shouldCheckTxForRefund = elapsedTime > fourHoursInSeconds
38-38: LGTM: New refund check flag added to return object.The addition of
isCheckTxForRefundto the return object is correct and consistent with the existing structure.Consider updating the function's JSDoc comment to include information about this new return value. This will help maintain clear documentation for future developers. For example:
/** * Calculates remaining time based on given initial, current, and estimated times. * * @param currentTime - The current time, as a unix timestamp. * @param initialTime - The initial time, as a unix timestamp. * @param estimatedTime - The estimated duration to calculate, in seconds. * @returns An object containing various time calculations and status flags, including: * - targetTime: The target completion time. * - elapsedTime: The time elapsed since the initial time. * - remainingTime: The estimated remaining time. * - delayedTime: The amount of delay if the estimated time is exceeded. * - delayedTimeInMin: The amount of delay in minutes. * - isEstimatedTimeReached: Whether the estimated time has been reached. * - isCheckTxStatus: Whether to check the transaction status. * - isCheckTxForRevert: Whether to check if the transaction has been reverted. * - isCheckTxForRefund: Whether to check if the transaction is eligible for a refund (after 4 hours). */
Line range hint
1-40: Summary: Good implementation of refund check for long-running transactions.The changes introduce a new feature to check for transactions that have been running for more than 4 hours, potentially requiring a refund. This is a valuable addition to the
calculateEstimatedTimeStatusfunction, enhancing its capability to handle various transaction scenarios.The implementation is clean, consistent with the existing code structure, and doesn't introduce any apparent issues. The suggestions for minor improvements in variable naming and documentation will further enhance the code quality and maintainability.
Consider the following to further improve the function:
If the 4-hour threshold for refund checks is likely to change or be different for various scenarios, consider making it a parameter of the function rather than a hard-coded constant.
Evaluate if there's a need for additional unit tests to cover this new functionality, ensuring the refund check behaves correctly under different time conditions.
packages/synapse-interface/components/_Transaction/components/TransactionSupport.tsx (2)
16-18: Simplified 'reverted' status checkThe simplified condition for the 'reverted' status is good. It's more direct and aligns well with the new prop type.
For consistency with other status checks, consider using parentheses:
- {status === 'reverted' && ( + {(status === 'reverted') && ( <div>{t('Transaction reverted, funds returned')}</div> )}
20-25: New status messages improve user feedbackThe addition of specific messages for 'refunded' and 'pending' statuses is a good improvement. It provides clearer feedback to users about their transaction state.
Consider adding a message for the 'completed' status as well, for consistency:
{(status === 'completed') && ( <div>{t('Transaction completed successfully')}</div> )}Also, for consistency with the other status checks, consider using parentheses:
- {status === 'refunded' && ( + {(status === 'refunded') && ( <div>{t('Transaction refunded, funds returned')}</div> )} - {status === 'pending' && <div>{t("What's taking so long?")}</div>} + {(status === 'pending') && <div>{t("What's taking so long?")}</div>}packages/synapse-interface/components/_Transaction/components/TimeRemaining.tsx (3)
15-15: LGTM! Consider using TypeScript enum for status.The addition of 'refunded' to the status prop types is consistent with the new functionality and maintains backwards compatibility.
Consider using a TypeScript enum for the status prop to improve type safety and maintainability. For example:
enum TransactionStatus { Pending = 'pending', Completed = 'completed', Reverted = 'reverted', Refunded = 'refunded' } // Then update the prop type: status: TransactionStatusThis change would make it easier to manage status types across the application and provide better autocomplete support.
39-45: LGTM! Consider extracting common status rendering logic.The new conditional block for the 'refunded' status is well-implemented and consistent with the existing code structure.
To improve code reusability and maintainability, consider extracting the common logic for rendering status messages into a separate function. For example:
const renderStatusMessage = (status: string, message: string) => ( <span className="flex items-center space-x-1 text-sm"> <ExclamationIcon className="w-4 h-4" /> <span>{t(message)}</span> </span> ); // Then use it like this: if (status === 'reverted') { return renderStatusMessage('reverted', 'Reverted'); } if (status === 'refunded') { return renderStatusMessage('refunded', 'Refunded'); }This refactoring would reduce code duplication and make it easier to add new status types in the future.
Line range hint
1-58: Overall assessment: Changes are well-implemented and consistent.The addition of the 'refunded' status to the
TimeRemainingcomponent is well-executed and maintains consistency with the existing code structure. The changes enhance the component's functionality without introducing breaking changes.To further improve the component:
- Consider using TypeScript enums for status types to enhance type safety.
- Extract common status rendering logic to a separate function to improve code reusability.
These suggestions would make the code more maintainable and scalable for future additions.
packages/synapse-interface/utils/hooks/use_TransactionsListener.ts (1)
Line range hint
1-67: Consider updating dependent code that consumes transaction data.While the addition of
routerAddressis a localized change in this file, it may have implications for other parts of the application that consume transaction data.Please ensure that:
- Any components or hooks that display or process transaction details are updated to handle the new
routerAddressproperty.- Any TypeScript interfaces or types defining transaction objects are updated to include
routerAddress.- If there are any API endpoints or database schemas related to transactions, they are updated to accommodate this new field.
This will help maintain consistency across the application and prevent potential runtime errors or missing data issues.
packages/synapse-interface/components/_Transaction/helpers/useBridgeTxUpdater.ts (3)
31-32: Update JSDoc to include the new parameter.The addition of
isTxRefundedparameter is correct and consistent with the existing structure. However, the JSDoc comment above the function should be updated to include this new parameter.Please update the JSDoc comment to include:
* @param {boolean} isTxRefunded - Whether the bridge transaction was refunded, queried from the appropriate source.
54-60: LGTM: New effect for handling refunded transactions.The new useEffect for handling refunded transactions is well-implemented and consistent with the existing pattern. It correctly checks both the
isTxRefundedstate and the current transaction status before dispatching the action.Consider adding
storedTx.statusto the effect's dependency array to ensure it runs if the transaction status changes:}, [isTxRefunded, storedTx.status])This change would make the effect more robust against potential race conditions where the transaction status might change from another source.
Issue Found: UI Components Do Not Handle 'Refunded' Transaction State
The current UI implementation does not handle the 'refunded' transaction state. To ensure users receive appropriate feedback when a transaction is refunded, please update the following components:
TransactionButton.tsx_Transactions.tsxTransactionStatus.tsxEnsure that these components correctly display and manage the 'refunded' status to provide a seamless user experience.
🔗 Analysis chain
Line range hint
1-79: Consider additional error handling and UI updates for refund cases.The implementation of the refund handling mechanism is well-integrated and consistent with existing patterns. However, there are a few additional considerations:
Error Handling: Consider adding try-catch blocks around the dispatch calls to handle any potential errors during the refund process.
Logging: It might be beneficial to add some logging when a refund occurs, especially if you have a centralized logging system.
UI Updates: Ensure that any UI components displaying transaction status are updated to handle and display the new 'refunded' state appropriately.
To verify the UI updates, you can run the following command to check for any components that might need updating:
This will help identify components that might need to be updated to handle the new 'refunded' state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for components that render transaction status rg --type typescript --type tsx 'transaction.*status|tx.*status' packages/synapse-interface/componentsLength of output: 139
Script:
#!/bin/bash # Search for components that render transaction status in .ts and .tsx files rg 'transaction.*status|tx.*status' packages/synapse-interface/components --glob '*.ts' --glob '*.tsx'Length of output: 808
Script:
#!/bin/bash # Search for the term 'refunded' in the identified transaction status components rg 'refunded' packages/synapse-interface/components/buttons/TransactionButton.tsx rg 'refunded' packages/synapse-interface/components/_Transaction/_Transactions.tsx rg 'refunded' packages/synapse-interface/components/_Transaction/components/TransactionStatus.tsx rg 'refunded' packages/synapse-interface/components/_Transaction/helpers/useTxRefundStatus.ts rg 'refunded' packages/synapse-interface/components/_Transaction/helpers/useBridgeTxStatus.tsLength of output: 450
packages/synapse-interface/components/_Transaction/_Transactions.tsx (1)
59-59: LGTM! Consider destructuring for consistency.The addition of the
routerAddressprop is correct and aligns with the expected changes. Good job on including this new piece of information for the transaction details.For consistency with other props, consider destructuring
routerAddressfrom thetxobject along with other properties at the beginning of the mapping function. This would make the code more readable and maintain a consistent style. Here's a suggested change:sortedTransactions.slice(0, 5).map((tx: _TransactionDetails) => { + const { routerAddress, timestamp, originTxHash, bridgeModuleName, estimatedTime, kappa, status } = tx; const originChain = CHAINS_BY_ID[tx.originChain?.id] const originToken = _(ALL_TOKENS).find( (token) => token.routeSymbol === tx.originToken?.routeSymbol ) // ... rest of the code ... return ( <_Transaction key={tx.timestamp} // ... other props ... - routerAddress={tx.routerAddress} + routerAddress={routerAddress} // ... rest of the props ... /> ) })This change would make the code more consistent and easier to maintain in the future.
packages/synapse-interface/slices/_transactions/reducer.ts (2)
17-17: LGTM! Consider adding JSDoc comments for clarity.The additions to the
_TransactionDetailsinterface enhance transaction tracking capabilities. TherouterAddressproperty allows for identifying the specific router used in a transaction, while the 'refunded' status provides a more comprehensive set of transaction outcomes.Consider adding JSDoc comments to these new/modified properties for better code documentation:
/** The address of the router used for this transaction */ routerAddress: string; /** The current status of the transaction */ status: 'pending' | 'completed' | 'reverted' | 'refunded';Also applies to: 21-21
91-103: LGTM! Consider using object property shorthand for consistency.The
refundTransactionreducer function is well-implemented and follows the existing pattern of other transaction status update functions. It correctly updates the transaction status to 'refunded' based on theoriginTxHash.For consistency with other reducer functions, consider using object property shorthand:
refundTransaction: ( state, action: PayloadAction<{ originTxHash: string }> ) => { const { originTxHash } = action.payload; const txIndex = state.transactions.findIndex( (tx) => tx.originTxHash === originTxHash ); if (txIndex !== -1) { state.transactions[txIndex] = { ...state.transactions[txIndex], status: 'refunded' }; } },This change maintains consistency with how other properties are updated in similar reducer functions.
packages/synapse-interface/package.json (2)
35-35: New dependency @popperjs/core added.The addition of @popperjs/core suggests new UI features or improvements, likely for tooltips or popovers. Consider updating to the latest version (currently 2.11.8) for potential bug fixes and improvements.
- "@popperjs/core": "^2.11.5", + "@popperjs/core": "^2.11.8",
111-111: New devDependency autoprefixer added.The addition of autoprefixer as a devDependency is a good practice for ensuring CSS compatibility across different browsers. Consider updating to the latest version (currently 10.4.19) for potential improvements and bug fixes.
- "autoprefixer": "^10.4.7", + "autoprefixer": "^10.4.19",packages/contracts-rfq/CHANGELOG.md (1)
46-51: Adjust heading level for consistency.The content of the 0.5.1 entry is correct and well-documented. However, the heading level for "Bug Fixes" should be adjusted for consistency with other entries.
Please update the heading level as follows:
-### Bug Fixes +## Bug FixesThis change will ensure consistency across all entries in the changelog.
packages/synapse-interface/constants/abis/fastBridgeRouter.json (2)
69-160: bridge function is comprehensive but complex.This function handles cross-chain token bridging with origin and destination swaps. The complexity of the
SwapQuerystruct and multiple parameters increases the risk of errors. Consider breaking this function into smaller, more manageable parts if possible.Consider refactoring the
bridgefunction to improve readability and maintainability. You could split it into separate functions for origin and destination swaps, reducing the number of parameters in each function call.
1-387: Overall, the FastBridgeRouter ABI is well-structured but complex.The ABI defines a comprehensive set of functions, events, and errors for a fast bridge router contract. It includes features for token swapping, cross-chain bridging, and contract management. The use of custom errors and events is commendable for gas efficiency and transparency.
However, there are areas that could benefit from improvement:
The
bridgefunction is particularly complex, with multiple nested structs as parameters. Consider refactoring this into smaller, more manageable functions.Ensure that proper input validation and access control are implemented in the contract, especially for critical functions like
setFastBridgeandsetSwapQuoter.Given the complexity of the contract, extensive testing and possibly a formal audit would be highly recommended to ensure its security and correctness.
Consider breaking down complex functions into smaller, more focused ones to improve readability and maintainability. This could also help in writing more targeted and comprehensive tests for each piece of functionality.
packages/synapse-interface/messages/ar.json (1)
311-311: New status message translation added correctly with a minor suggestion.The new entry "Transaction refunded, funds returned" has been accurately translated to Arabic and appropriately placed within the "Time" section. The translation effectively conveys the meaning of the original phrase.
For consistency with the "Refunded" status, consider using the same verb form:
-"Transaction refunded, funds returned": "تم استرداد المعاملة، وتمت إعادة الأموال" +"Transaction refunded, funds returned": "تم استرداد المعاملة، وتم إعادة الأموال"This minor change maintains consistency in verb usage across related status messages.
packages/synapse-interface/constants/abis/fastBridge.json (1)
1-851: Overall assessment: Well-structured but requires security enhancementsThe FastBridge contract ABI demonstrates a comprehensive implementation of a cross-chain bridge with clearly defined functions, events, and error handling. However, several areas could benefit from improvement:
- Enhance security measures in core functions like
bridgeandrelay.- Implement additional safeguards for administrative functions.
- Improve event definitions for better off-chain monitoring.
- Refactor error definitions for consistency and clarity.
It's crucial to conduct a thorough security audit and consider implementing the suggested improvements before deploying this contract to a production environment. Additionally, ensure that extensive testing is performed, including edge cases and potential attack scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (27)
- packages/contracts-rfq/CHANGELOG.md (1 hunks)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (1 hunks)
- packages/contracts-rfq/package.json (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1 hunks)
- packages/synapse-interface/CHANGELOG.md (1 hunks)
- packages/synapse-interface/components/_Transaction/_Transaction.tsx (6 hunks)
- packages/synapse-interface/components/_Transaction/_Transactions.tsx (1 hunks)
- packages/synapse-interface/components/_Transaction/components/AnimatedProgressBar.tsx (2 hunks)
- packages/synapse-interface/components/_Transaction/components/TimeRemaining.tsx (2 hunks)
- packages/synapse-interface/components/_Transaction/components/TransactionSupport.tsx (1 hunks)
- packages/synapse-interface/components/_Transaction/helpers/calculateEstimatedTimeStatus.ts (2 hunks)
- packages/synapse-interface/components/_Transaction/helpers/useBridgeTxUpdater.ts (3 hunks)
- packages/synapse-interface/components/_Transaction/helpers/useTxRefundStatus.ts (1 hunks)
- packages/synapse-interface/constants/abis/fastBridge.json (1 hunks)
- packages/synapse-interface/constants/abis/fastBridgeRouter.json (1 hunks)
- packages/synapse-interface/messages/ar.json (2 hunks)
- packages/synapse-interface/messages/en-US.json (2 hunks)
- packages/synapse-interface/messages/es.json (2 hunks)
- packages/synapse-interface/messages/fr.json (2 hunks)
- packages/synapse-interface/messages/jp.json (2 hunks)
- packages/synapse-interface/messages/tr.json (2 hunks)
- packages/synapse-interface/messages/zh-CN.json (2 hunks)
- packages/synapse-interface/package.json (4 hunks)
- packages/synapse-interface/pages/state-managed-bridge/index.tsx (7 hunks)
- packages/synapse-interface/slices/_transactions/reducer.ts (3 hunks)
- packages/synapse-interface/slices/transactions/actions.ts (1 hunks)
- packages/synapse-interface/utils/hooks/use_TransactionsListener.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/contracts-rfq/contracts/FastBridgeV2.sol
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol
- packages/synapse-interface/messages/en-US.json
- packages/synapse-interface/messages/es.json
- packages/synapse-interface/messages/fr.json
- packages/synapse-interface/messages/jp.json
- packages/synapse-interface/messages/tr.json
- packages/synapse-interface/messages/zh-CN.json
- packages/synapse-interface/pages/state-managed-bridge/index.tsx
🧰 Additional context used
🪛 Markdownlint
packages/contracts-rfq/CHANGELOG.md
60-60: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
93-93: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
packages/synapse-interface/CHANGELOG.md
9-9: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
36-36: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
🔇 Additional comments (29)
packages/contracts-rfq/package.json (1)
14-15: Review dependency updates and removals
- "@synapsecns/solidity-devops" updated from "^0.4.4" to "^0.4.6".
- "solhint" updated from "^3.3.6" to "5.0.3" (major version update).
- "prettier" and "prettier-plugin-solidity" dependencies removed.
These changes have the following implications:
- The solhint major version update may introduce breaking changes. Ensure all solhint rules are still compatible and adjust your solhint configuration if necessary.
- Removal of prettier dependencies suggests a change in code formatting approach. Verify that this aligns with your team's coding standards and update any CI/CD pipelines that may have relied on prettier.
✅ Verification successful
Solhint Dependency Removal Verified
solhintis no longer listed inpackage.jsondependencies or devDependencies.solhintis not installed globally or locally.- No references to
solhintfound inpackage.jsonscripts or CI/CD configurations.- ESLint is present as an alternative linting tool.
.solhint.jsonconfiguration files remain and should be removed ifsolhintis no longer in use.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for solhint configuration and usage # Check for solhint configuration file if [ -f .solhint.json ]; then echo "solhint configuration file exists" else echo "Warning: solhint configuration file is missing" fi # Check for usage of prettier in scripts if grep -q "prettier" package.json; then echo "Warning: 'prettier' is still referenced in package.json scripts" else echo "No references to 'prettier' found in package.json scripts" fiLength of output: 439
Script:
#!/bin/bash # Description: Verify solhint installation and configuration # Check if solhint is listed in package.json dependencies or devDependencies if grep -q '"solhint":' package.json; then echo "solhint is listed in package.json." else echo "Warning: solhint is not listed in package.json dependencies." fi # Verify if solhint is installed in node_modules if [ -x "$(command -v solhint)" ]; then echo "solhint is installed globally." else if [ -d "node_modules/solhint" ]; then echo "solhint is installed locally in node_modules." else echo "Warning: solhint is not installed in node_modules." fi fi # Search for any solhint configuration files if find . -type f \( -name ".solhint.json" -o -name "solhint.config.js" -o -name ".solhintrc" \) | grep .; then echo "Solhint configuration file found." else echo "Warning: No solhint configuration file found." fiLength of output: 647
Script:
#!/bin/bash # Description: Verify references to solhint in package.json scripts and CI/CD configurations # Check for solhint references in package.json scripts echo "Checking for solhint references in package.json scripts..." grep '"scripts": {[^}]*solhint' package.json && echo "solhint is referenced in package.json scripts." || echo "No solhint references found in package.json scripts." # Search for solhint references in CI/CD configuration files echo "Searching for solhint references in CI/CD configuration files..." if find . -type f \( -name ".github/workflows/*.yml" -o -name ".gitlab-ci.yml" \) -exec grep -H "solhint" {} \; ; then echo "solhint is referenced in CI/CD configuration files." else echo "No solhint references found in CI/CD configuration files." fi # Check for alternative linting tools in package.json echo "Checking for alternative linting tools in package.json..." grep '"eslint":' package.json && echo "ESLint is present as an alternative linting tool." || echo "No alternative linting tools found."Length of output: 943
Script:
#!/bin/bash # Description: Extract solhint usage in CI/CD configuration files # Search for solhint commands in GitHub Actions workflows echo "Extracting solhint commands from GitHub Actions workflows..." grep -H "solhint" .github/workflows/*.yml || echo "No solhint commands found in GitHub Actions workflows." # Search for solhint commands in GitLab CI configurations echo "Extracting solhint commands from GitLab CI configurations..." grep -H "solhint" .gitlab-ci.yml || echo "No solhint commands found in GitLab CI configurations." # Summarize findings echo "Solhint usage extraction completed."Length of output: 1191
packages/synapse-interface/components/_Transaction/helpers/calculateEstimatedTimeStatus.ts (1)
19-19: LGTM: Well-defined constant for 4 hours in seconds.The addition of
fourHoursInSecondsconstant is a good practice. It improves code readability and maintainability by giving a clear name to the magic number 14400.packages/synapse-interface/components/_Transaction/components/TransactionSupport.tsx (1)
4-8: Improved type safety for thestatuspropThe change from a generic
stringtype to a union of specific string literals ('pending' | 'completed' | 'reverted' | 'refunded') for thestatusprop is a good improvement. This enhances type safety and makes the component's API more self-documenting.packages/synapse-interface/components/_Transaction/helpers/useBridgeTxUpdater.ts (1)
8-8: LGTM: New import for refund functionality.The addition of
refundTransactionimport is consistent with the new refund handling feature being implemented.packages/synapse-interface/slices/_transactions/reducer.ts (1)
116-116: LGTM! Exporting the new action is correct.The addition of
refundTransactionto the list of exported actions is correct and necessary. This allows the new refund functionality to be used in other parts of the application, maintaining consistency with how other actions are exported and used.packages/synapse-interface/components/_Transaction/components/AnimatedProgressBar.tsx (1)
8-8: JSDoc comment updated correctly.The JSDoc comment for the
statusparameter has been properly updated to include the new 'refunded' status. This change maintains consistency between the documentation and the actual code implementation.packages/synapse-interface/package.json (3)
3-3: Version bump looks good.The version has been updated from 0.38.3 to 0.40.0, which is appropriate for introducing new features or significant changes. This adheres to semantic versioning principles.
36-36: Dependency @rainbow-me/rainbowkit updated.The update from ^2.1.2 to ^2.1.6 is a patch version change, likely containing bug fixes or minor improvements. This update follows good practices for keeping dependencies current.
49-50: @Wagmi dependencies updated and wagmi removed.The updates to @wagmi/connectors (^5.0.0 to ^5.1.11) and @wagmi/core (^2.10.5 to ^2.13.5) are appropriate minor version changes. The removal of the wagmi dependency appears to be intentional, possibly consolidating functionality into @wagmi/core and @wagmi/connectors.
Please verify that the removal of the wagmi dependency doesn't negatively impact the project's functionality. Run the following script to check for any remaining wagmi imports:
packages/contracts-rfq/CHANGELOG.md (1)
6-33: LGTM: New version entries are consistent and well-formatted.The new version entries (0.5.6, 0.5.5, 0.5.4, and 0.5.3) are correctly formatted and consistent. They all use the correct package name
@synapsecns/contracts-rfqand indicate that they are version bumps only.packages/synapse-interface/constants/abis/fastBridgeRouter.json (9)
1-12: Constructor looks good.The constructor is well-defined, taking an
owner_parameter of typeaddress. This is a standard practice for setting up initial ownership of the contract.
14-16: Receive function is properly defined.The contract includes a
receivefunction, allowing it to accept Ether transfers. This is important for contracts that need to handle native token transactions.
17-29: GAS_REBATE_FLAG function looks correct.This view function returns a
bytes1value, likely used as a flag for gas rebate functionality. Ensure this flag is used consistently throughout the contract logic.
161-173: Standard Ownable pattern functions are present.The contract includes standard functions for the Ownable pattern:
owner,renounceOwnership, andtransferOwnership. These are important for proper access control.Also applies to: 230-242, 243-249, 289-301
174-229: getOriginAmountOut function looks well-defined.This view function calculates the expected output amount for a given input token and amount. The use of dynamic arrays (
address[]) forrfqTokensis appropriate for handling multiple token options.
302-346: Events are well-defined for important state changes.The ABI includes three events:
FastBridgeSet: Logs when the fast bridge address is updated.OwnershipTransferred: Standard event for ownership changes.SwapQuoterSet: Logs when the swap quoter address is updated.These events provide good transparency for critical contract changes.
347-386: Custom errors are well-defined and cover important scenarios.The ABI includes 8 custom error types, which is a gas-efficient approach for handling reverts. These errors cover important scenarios such as:
- Deadline exceeded
- Insufficient output amount
- Incorrect message value
- Pool not found
- Token address mismatch
- Token not being a contract
- Token not being ETH
- Identical tokens
This comprehensive set of errors should help in providing clear feedback when operations fail.
30-68: adapterSwap function seems well-structured.This payable function handles token swaps through an adapter. It takes appropriate parameters for the swap operation. However, consider adding input validation to ensure the integrity of the swap parameters.
250-262: Setter and getter functions for fastBridge and swapQuoter are present.These functions allow for updating and retrieving the addresses of the fastBridge and swapQuoter contracts. Ensure that proper access control is implemented in the contract to restrict who can call the setter functions.
Also applies to: 263-275, 276-288
packages/synapse-interface/messages/ar.json (2)
61-61: New translation added correctly.The new entry "Confirm new quote" has been accurately translated to Arabic and appropriately placed within the "Bridge" section.
309-309: New status translation added correctly.The new entry "Refunded" has been accurately translated to Arabic and appropriately placed within the "Time" section, which likely contains various transaction status messages.
packages/synapse-interface/constants/abis/fastBridge.json (3)
1-8: Constructor implementation looks good.The constructor takes an
_owneraddress parameter, which is a standard practice for setting up initial administrative control of the contract.
394-434: Enhance security measures for administrative functions.The administrative functions (
setChainGasAmount,setProtocolFeeRate,sweepProtocolFees) are critical for contract management. Consider implementing the following security enhancements:
- Ensure all administrative functions are protected by appropriate access control (e.g., onlyOwner or specific roles).
- Implement a time-lock mechanism for sensitive parameter changes to allow users to react to upcoming changes.
- Add event emissions for all administrative actions to maintain a clear audit trail.
- Consider implementing upper and lower bounds for settable parameters to prevent potential misconfigurations.
To verify the implementation of these security measures, you can run the following script:
79-122: Ensure proper access control and input validation in thebridgefunction.The
bridgefunction is a critical entry point for initiating cross-chain transfers. Consider the following security aspects:
- Ensure that the
paramsstruct is properly validated to prevent potential exploits.- Implement checks for minimum and maximum transfer amounts to prevent dust attacks or excessive transfers.
- Consider adding a mechanism to pause the bridge in case of emergencies.
To verify the implementation of these security measures, you can run the following script:
packages/synapse-interface/CHANGELOG.md (2)
6-11: LGTM: Latest version entry is well-formatted and informative.The changelog entry for version 0.40.0 is clear and follows a consistent format. It provides useful information about the new feature for refunding RFQ transactions, including the associated issue number and commit hash.
🧰 Tools
🪛 Markdownlint
9-9: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
Line range hint
1-93: Consider the heading structure, but maintain consistency.The static analysis tool suggests using h2 headings instead of h3 for the version numbers. However, the current structure is consistent throughout the file and follows a common changelog format. Changing the heading levels might disrupt the readability of the document.
Recommendation: Keep the current heading structure to maintain consistency with the established format of the changelog.
🧰 Tools
🪛 Markdownlint
9-9: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
36-36: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
packages/synapse-interface/components/_Transaction/_Transaction.tsx (3)
39-39: Verify that all possible 'status' values are handled correctly in the component.The
statusprop now includes'refunded'(line 39). Ensure that all conditional logic in the component correctly handles the new'refunded'status, such as inshowTransactionSupport(line 127) and other places wherestatusis used.Also applies to: 127-127
120-121: Confirm thatuseBridgeTxUpdateracceptsisTxRefundedas a parameter.The
isTxRefundedvariable is added as a parameter touseBridgeTxUpdater(line 121). Verify that theuseBridgeTxUpdaterhook is updated to accept this new parameter and handles it appropriately.
202-202: EnsureisTxRefundedis properly handled in the conditional rendering.In the conditional rendering for the menu item text (line 202),
isTxReverted || isTxRefundedis used to determine the text displayed. Ensure thatisTxRefundedis defined and that this logic correctly reflects the intended behavior when a transaction is refunded.
| "name": "@synapsecns/contracts-rfq", | ||
| "license": "MIT", | ||
| "version": "0.5.6", |
There was a problem hiding this comment.
💡 Codebase verification
CHANGELOG.md is missing
- A
CHANGELOG.mdfile is required to document the changes for version0.5.6.
🔗 Analysis chain
Verify package metadata changes and required files
The package metadata has been significantly updated:
- Name changed to "@synapsecns/contracts-rfq"
- License changed to MIT
- Version bumped to 0.5.6
These changes require additional actions:
- Ensure a LICENSE file with the MIT license text exists in the repository root.
- Add a CHANGELOG.md file detailing the changes for version 0.5.6.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for LICENSE and CHANGELOG.md files
# Check for LICENSE file
if [ -f LICENSE ]; then
echo "LICENSE file exists"
if grep -q "MIT License" LICENSE; then
echo "LICENSE file contains MIT license"
else
echo "LICENSE file does not contain MIT license"
fi
else
echo "LICENSE file is missing"
fi
# Check for CHANGELOG.md file
if [ -f CHANGELOG.md ]; then
echo "CHANGELOG.md file exists"
if grep -q "## \[0.5.6\]" CHANGELOG.md; then
echo "CHANGELOG.md contains entry for version 0.5.6"
else
echo "CHANGELOG.md does not contain entry for version 0.5.6"
fi
else
echo "CHANGELOG.md file is missing"
fi
Length of output: 271
| "solhint": "solhint '{contracts,script,test}/**/*.sol' --fix --noPrompt --max-warnings 3", | ||
| "solhint:check": "solhint '{contracts,script,test}/**/*.sol' --max-warnings 3" |
There was a problem hiding this comment.
💡 Codebase verification
Solhint is missing from project dependencies
The solhint package is referenced in the package.json scripts but is not listed under dependencies or devDependencies. This omission can cause the solhint command to fail during execution, as it is not installed in the project environment.
Recommended actions:
- Add
solhintto thedevDependenciesinpackage.json. - Run
npm install solhint --save-devto install the package. - Ensure all team members are updated with the new dependency.
🔗 Analysis chain
Review updates to solhint scripts
The solhint scripts have been updated:
- The "solhint" script now includes more directories (contracts, script, test) and uses the --fix flag.
- A new "solhint:check" script has been added for checking without fixing.
These changes have the following implications:
- The linting process now covers a broader range of files, improving code quality across the project.
- The --fix flag in the "solhint" script will automatically fix some issues, which may lead to unexpected changes if not carefully reviewed.
- The new "solhint:check" script allows for linting checks without modifying files, useful for CI/CD processes.
Ensure that:
- All team members are aware of the expanded scope of linting and the auto-fix feature.
- Code reviews include checking the changes made by the auto-fix feature.
- CI/CD pipelines are updated to use the appropriate solhint script (fixing or checking) based on the stage of development.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify solhint configuration and coverage
# Check solhint configuration
if [ -f .solhint.json ]; then
echo "solhint configuration found"
jq . .solhint.json
else
echo "Warning: .solhint.json configuration file not found"
fi
# Check solhint coverage
echo "\nChecking solhint coverage:"
solhint '{contracts,script,test}/**/*.sol' --max-warnings 3
Length of output: 439
Script:
#!/bin/bash
# Description: Check if solhint is listed in dependencies or devDependencies
if jq -e '.dependencies.solhint or .devDependencies.solhint' package.json > /dev/null; then
echo "solhint is listed as a dependency or devDependency."
else
echo "solhint is not listed in dependencies or devDependencies."
fi
Length of output: 197
| "lint": "forge fmt && npm run solhint", | ||
| "lint:check": "forge fmt --check && npm run solhint:check", | ||
| "ci:lint": "npm run lint:check", |
There was a problem hiding this comment.
💡 Codebase verification
Missing forge dependency and dependencies not installed
forgeis used in the lint scripts but is not listed inpackage.json. Please addforgeto the project's dependencies or devDependencies.- The
node_modulesdirectory is missing, indicating that dependencies may not be installed. Ensure to runnpm installto install all required dependencies.
🔗 Analysis chain
Review updates to linting scripts
The linting scripts have been updated:
- "lint" now uses
forge fmtfor formatting andnpm run solhintfor linting. - A new "lint:check" script has been added for checking without modifying files.
- "ci:lint" now uses the new "lint:check" script.
These changes align with the removal of prettier dependencies and shift towards using forge for formatting. Ensure that:
- All team members are aware of the new linting process.
- CI/CD pipelines are updated to use the new "ci:lint" script.
- The
forge fmtcommand produces results consistent with your team's coding standards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify forge installation and formatting consistency
# Check if forge is installed
if command -v forge &> /dev/null; then
echo "forge is installed"
else
echo "Warning: forge is not installed"
fi
# Check formatting consistency
echo "Checking formatting consistency:"
forge fmt --check 2>&1 | grep "Formatted"
if [ $? -eq 0 ]; then
echo "Warning: Some files are not formatted according to forge fmt"
else
echo "All files are formatted consistently"
fi
Length of output: 439
Script:
#!/bin/bash
# Description: Verify solhint installation
# Check if solhint is installed
if command -v solhint &> /dev/null; then
echo "solhint is installed"
else
echo "Warning: solhint is not installed"
fi
Length of output: 97
Script:
#!/bin/bash
# Description: Check for .solhint.json configuration file
if [ -f .solhint.json ]; then
echo ".solhint.json configuration file exists"
else
echo "Warning: .solhint.json configuration file not found"
fi
Length of output: 139
Script:
#!/bin/bash
# Description: Corrected script to verify forge installation and formatting consistency
# Check if forge is installed
if command -v forge &> /dev/null; then
echo "forge is installed"
else
echo "Warning: forge is not installed"
fi
# Check formatting consistency
echo "Checking formatting consistency:"
forge fmt --check 2>&1 | grep "Formatted"
if [ $? -eq 0 ]; then
echo "Warning: Some files are not formatted according to forge fmt"
else
echo "All files are formatted consistently"
fi
Length of output: 304
Script:
#!/bin/bash
# Description: Check if solhint and forge are listed in package.json
# Search for solhint in dependencies and devDependencies
grep -E '"solhint"\s*:\s*' packages/contracts-rfq/package.json && echo "solhint is listed in package.json" || echo "Warning: solhint is not listed in package.json"
# Search for forge in dependencies and devDependencies
grep -E '"forge"\s*:\s*' packages/contracts-rfq/package.json && echo "forge is listed in package.json" || echo "Warning: forge is not listed in package.json"
Length of output: 421
Script:
#!/bin/bash
# Description: Check if node_modules exists to verify if npm install has been run
if [ -d "node_modules" ]; then
echo "node_modules directory exists. Dependencies are installed."
else
echo "Warning: node_modules directory does not exist. Dependencies may not be installed."
fi
Length of output: 200
| "description": "FastBridge contracts.", | ||
| "private": true, | ||
| "files": [ | ||
| "src/*.sol" | ||
| "contracts/**/*.sol" |
There was a problem hiding this comment.
💡 Codebase verification
Expand the Solidity file pattern to include all contract directories
The current file pattern "contracts/**/*.sol" does not capture Solidity files located outside the contracts directory. To ensure all Solidity contracts are included, consider updating the "files" field in package.json to encompass additional directories. For example:
"contracts/**/*.sol""agents/testutil/agentstestcontract/contract/**/*.sol""ethergo/examples/contracttests/counter/**/*.sol""services/explorer/contracts/**/*.sol"
Alternatively, you can use a more inclusive pattern like "**/*.sol" to cover all Solidity files across the repository, ensuring no contracts are missed.
🔗 Analysis chain
Verify new contract file structure
The change from "src/.sol" to "contracts/**/.sol" indicates a restructuring of the project's Solidity files. Let's verify if this new pattern correctly captures all Solidity contracts in the project.
If the output shows Solidity files outside the 'contracts' directory, consider updating the "files" field to ensure all relevant contracts are included, e.g., "files": ["contracts/**/*.sol", "packages/**/contracts/**/*.sol"].
Re-running Verification Without solhint
The previous verification script encountered errors due to the missing solhint command and a syntax issue. Let's attempt to verify the Solidity file structure without using solhint.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new contract file structure
echo "Listing all Solidity files in the repository:"
fd -e sol .
echo "\nVerifying if all Solidity files are under 'contracts' directory:"
if fd -e sol . | grep -vq "^contracts/"; then
echo "Warning: Some Solidity files are not under the 'contracts' directory"
else
echo "All Solidity files are under the 'contracts' directory"
fi
Length of output: 439
Script:
#!/bin/bash
# Description: Verify that all Solidity files are under the 'contracts' directory
echo "Listing all Solidity files in the repository:"
fd -e sol .
echo -e "\nVerifying if all Solidity files are under the 'contracts' directory:"
if fd -e sol . | grep -vq "^contracts/"; then
echo "Warning: Some Solidity files are not under the 'contracts' directory"
else
echo "All Solidity files are under the 'contracts' directory"
fi
Length of output: 15244
| "prettier-plugin-solidity": "^1.0.0-beta.19", | ||
| "solhint": "^3.3.6" | ||
| "@synapsecns/solidity-devops": "^0.4.6", | ||
| "solhint": "5.0.3" | ||
| }, | ||
| "scripts": { | ||
| "build": " ", |
There was a problem hiding this comment.
💡 Codebase verification
Empty build script causes inconsistencies and broken references
The build script in package.json is currently empty ("build": " " while build:fe is defined. Additionally, there are multiple references to npm run build in the repository, which rely on the empty script. This could lead to build failures and confusion.
Please address the following:
- Define the
buildscript if it is intended to handle the primary build process. - Remove the
buildscript if it is no longer needed and ensure all references tonpm run buildare updated to use the appropriate script, such asnpm run build:fe. - Update documentation and examples to reflect the correct build commands to prevent confusion.
🔗 Analysis chain
Empty build script may lead to build inconsistencies
The build script is currently empty ("build": " "). This could potentially cause issues if any processes depend on this script. Please address the following:
- If the build process is handled elsewhere, consider removing this script entirely to avoid confusion.
- If a build process is needed, define the appropriate commands in this script.
- Ensure that any CI/CD pipelines or documentation referring to
npm run buildare updated accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for build-related scripts and references
echo "All scripts containing 'build':"
jq '.scripts | to_entries[] | select(.key | contains("build")) | .key + ": " + .value' package.json
echo "\nChecking for references to 'npm run build' in the repository:"
rg "npm run build" .
Length of output: 439
Script:
#!/bin/bash
# Description: Verify the presence and usage of the 'build' script
# List all scripts containing 'build' in package.json
echo "Listing all build-related scripts in package.json:"
jq '.scripts | to_entries[] | select(.key | contains("build")) | .key + ": " + .value' package.json
# Search for references to 'npm run build' in the repository
echo -e "\nSearching for references to 'npm run build' in the codebase:"
rg "npm run build" .
Length of output: 938
| const bridgeContract = await getRFQBridgeContract( | ||
| routerAddress, | ||
| chain?.id | ||
| ) | ||
|
|
There was a problem hiding this comment.
Handle undefined bridgeContract before proceeding
The getRFQBridgeContract function may return undefined if it fails. Before calling checkRFQTxBridgeStatus, ensure that bridgeContract is defined to prevent errors.
Apply this diff to handle undefined bridgeContract:
const bridgeContract = await getRFQBridgeContract(
routerAddress,
chain.id
)
+ if (!bridgeContract) {
+ console.error('Failed to retrieve bridge contract')
+ return
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const bridgeContract = await getRFQBridgeContract( | |
| routerAddress, | |
| chain?.id | |
| ) | |
| const bridgeContract = await getRFQBridgeContract( | |
| routerAddress, | |
| chain?.id | |
| ) | |
| if (!bridgeContract) { | |
| console.error('Failed to retrieve bridge contract') | |
| return | |
| } | |
| if (status === BridgeStatus.REFUNDED) { | ||
| setIsRefunded(true) | ||
| } |
There was a problem hiding this comment.
Ensure isRefunded reflects the current status
Currently, isRefunded is only set to true when status === BridgeStatus.REFUNDED, but it is never reset to false if the status changes. To keep isRefunded in sync with the actual status, consider updating it based on the status value.
Apply this diff to update isRefunded accordingly:
- if (status === BridgeStatus.REFUNDED) {
- setIsRefunded(true)
- }
+ setIsRefunded(status === BridgeStatus.REFUNDED)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (status === BridgeStatus.REFUNDED) { | |
| setIsRefunded(true) | |
| } | |
| setIsRefunded(status === BridgeStatus.REFUNDED) |
| const getTxRefundStatus = async () => { | ||
| try { | ||
| const bridgeContract = await getRFQBridgeContract( | ||
| routerAddress, | ||
| chain?.id | ||
| ) |
There was a problem hiding this comment.
Add null checks for txId and chain.id before proceeding
The function getTxRefundStatus does not verify if txId and chain.id are defined before using them. If either is undefined, it could lead to runtime errors when making contract calls. Please add checks to ensure they are valid.
Apply this diff to add null checks:
const getTxRefundStatus = async () => {
+ if (!txId || !chain?.id) {
+ console.error('Invalid txId or chain.id')
+ return
+ }
try {
const bridgeContract = await getRFQBridgeContract(
routerAddress,
chain.id
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getTxRefundStatus = async () => { | |
| try { | |
| const bridgeContract = await getRFQBridgeContract( | |
| routerAddress, | |
| chain?.id | |
| ) | |
| const getTxRefundStatus = async () => { | |
| if (!txId || !chain?.id) { | |
| console.error('Invalid txId or chain.id') | |
| return | |
| } | |
| try { | |
| const bridgeContract = await getRFQBridgeContract( | |
| routerAddress, | |
| chain.id | |
| ) |
| @@ -30,11 +31,12 @@ interface _TransactionProps { | |||
| destinationToken: Token | |||
| originTxHash: string | |||
| bridgeModuleName: string | |||
| routerAddress: string | |||
There was a problem hiding this comment.
Ensure 'routerAddress' is always defined or handle it as optional.
The routerAddress prop is added as a required string in the _TransactionProps interface (line 34) and used in the component (line 54). It is passed to useTxRefundStatus (line 107) cast as Address. If there are scenarios where routerAddress might be undefined or null, consider making it optional in the interface and adding checks before using it to prevent potential runtime errors.
Also applies to: 54-54, 107-107
| const isTxRefunded = useTxRefundStatus( | ||
| kappa, | ||
| routerAddress as Address, | ||
| originChain, | ||
| isCheckTxForRefund && | ||
| status === 'pending' && | ||
| bridgeModuleName === 'SynapseRFQ' | ||
| ) |
There was a problem hiding this comment.
Handle potential undefined values for 'kappa' and 'routerAddress' in useTxRefundStatus.
The kappa parameter is optional in _TransactionProps, but it is being used in useTxRefundStatus (line 106) without a null check. Similarly, if routerAddress can be undefined, casting it directly to Address may cause issues. Consider adding checks or default values to ensure these values are defined before use.
7f458db: synapse-interface preview link
Summary by CodeRabbit
Summary by CodeRabbit
New Features
FastBridgeV2contract for seamless token bridging across blockchain networks.Bug Fixes
contracts-rfqmodule.Tests
FastBridgeV2contract, covering various bridging scenarios and ensuring robust functionality.a5791e7: synapse-interface preview link
42e847b: synapse-interface preview link
c1ef693: synapse-interface preview link
b662ba5: synapse-interface preview link
1b13015: synapse-interface preview link
23e79e2: synapse-interface preview link
b610347: synapse-interface preview link
739a35a: synapse-interface preview link
4ac58af: synapse-interface preview link
a289e40: synapse-interface preview link
164aecc: synapse-interface preview link
d818405: synapse-interface preview link
45250a8: synapse-interface preview link
92858b2: synapse-interface preview link
bff7bab: synapse-interface preview link
afe22e7: synapse-interface preview link
21b3f9d: synapse-interface preview link
cba3a2b: synapse-interface preview link
a4c1f04: synapse-interface preview link
7148d44: synapse-interface preview link
24036a9: synapse-interface preview link
083b446: synapse-interface preview link
e82d507: synapse-interface preview link
41df495: synapse-interface preview link
ab65616: synapse-interface preview link
52beb9a: synapse-interface preview link
692f9e0: synapse-interface preview link
c77713e: synapse-interface preview link
ee185b6: synapse-interface preview link
ee185b6: explorer-ui preview link
76e3db7: explorer-ui preview link
76e3db7: synapse-interface preview link
271bbed: synapse-interface preview link
271bbed: explorer-ui preview link
a037434: synapse-interface preview link
a037434: explorer-ui preview link
b6b4c10: explorer-ui preview link
b6b4c10: synapse-interface preview link