FE Release 3-21 #2341
FE Release 3-21 #2341
Conversation
Used for summarization Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
* Skip failing RFQ tracking tests (older txs not tracked) * Put future TODOs re:RFQ tracking
Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
aureliusbtc
requested review from
trajan0x,
ChiTimesChi,
bigboydiamonds,
abtestingalpha and
Defi-Moses
as code owners
WalkthroughThis update encompasses a variety of changes including expanding base branches for reviews, updating Go and GitHub Actions configurations, adjusting package versions and dependencies, introducing new functionalities across components, and enhancing support for legacy transactions and maintenance events. Changes
Related issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
M-ci
Deploying sanguine-fe with
|
| Latest commit: |
970d5a9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cf6fcb26.sanguine-fe.pages.dev |
| Branch Preview URL: | https://master.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: .coderabbit.yaml
Files selected for processing (22)
- .coderabbit.yaml (1 hunks)
- .github/workflows/go.yml (1 hunks)
- .github/workflows/lint.yml (1 hunks)
- package.json (1 hunks)
- packages/contracts-core/CHANGELOG.md (1 hunks)
- packages/contracts-core/package.json (3 hunks)
- packages/rest-api/CHANGELOG.md (1 hunks)
- packages/rest-api/package.json (2 hunks)
- packages/sdk-router/CHANGELOG.md (1 hunks)
- packages/sdk-router/package.json (1 hunks)
- packages/sdk-router/src/rfq/fastBridgeRouter.ts (2 hunks)
- packages/sdk-router/src/sdk.test.ts (2 hunks)
- packages/synapse-interface/CHANGELOG.md (1 hunks)
- packages/synapse-interface/components/Maintenance/Events/example/EcotoneForkUpgrade.tsx (5 hunks)
- packages/synapse-interface/components/Maintenance/Events/template/MaintenanceEvent.tsx (1 hunks)
- packages/synapse-interface/package.json (2 hunks)
- packages/synapse-interface/pages/index.tsx (2 hunks)
- packages/synapse-interface/pages/state-managed-bridge/index.tsx (4 hunks)
- packages/synapse-interface/public/blacklist.json (1 hunks)
- packages/widget/CHANGELOG.md (1 hunks)
- packages/widget/package.json (2 hunks)
- yarn.lock (6 hunks)
Files skipped from review due to trivial changes (2)
- packages/sdk-router/package.json
- packages/synapse-interface/CHANGELOG.md
Additional comments: 37
.coderabbit.yaml (1)
- 13-19: The addition of new base branches for reviews is a good practice, ensuring flexibility in the review process across different types of branches. Well done!
packages/rest-api/package.json (2)
- 3-3: The version update for the
rest-apipackage is correctly following semantic versioning. Good practice!- 26-26: The dependency
@synapsecns/sdk-routerhas been updated. Please ensure compatibility with the new version.packages/synapse-interface/pages/index.tsx (1)
- 6-6: The replacement of
EcotoneForkUpgradeBannerwithMaintenanceBanneris a clear improvement for handling maintenance events. Ensure the new component functions as expected in the UI.packages/contracts-core/package.json (3)
- 3-3: The version update for the
@synapsecns/contracts-corepackage follows semantic versioning. Good practice!- 3-3: Noticed the removal of several formatting and linting dependencies. Ensure this aligns with the project's development environment strategy.
- 3-3:
solidity-coveragehas been updated. Please verify the new version maintains or improves test coverage quality.packages/widget/package.json (2)
- 4-4: The version update for the
packages/widgetpackage correctly follows semantic versioning. Good practice!- 69-69: The dependency
@synapsecns/sdk-routerhas been updated. Please ensure compatibility with the new version..github/workflows/lint.yml (1)
- 31-31: Adding blank lines for readability is a good practice in workflow configurations. Well done!
package.json (1)
- 35-35: The update to the
collectscript streamlines the coverage collection process. Ensure the new script functions as expected.packages/synapse-interface/package.json (2)
- 3-3: The version bump from
0.10.6to0.11.1is noted and appears appropriate for the changes described.- 37-37: The update of the
@synapsecns/sdk-routerdependency to version0.3.29is appropriate and suggests the integration of new features or bug fixes from thesdk-routerpackage.packages/synapse-interface/components/Maintenance/Events/template/MaintenanceEvent.tsx (3)
- 11-23: The addition of comments and constants for maintenance times is well-documented and correctly implemented, enhancing the maintainability and clarity of the maintenance event handling.
- 25-46: The
MaintenanceBannercomponent is correctly implemented, making effective use of existing components and hooks to manage the display of maintenance messages.- 49-103: The
MaintenanceWarningMessagefunction and theuseMaintenanceCountdownProgresshook are well-implemented, using conditional logic and React best practices to manage maintenance-related UI elements.packages/synapse-interface/components/Maintenance/Events/example/EcotoneForkUpgrade.tsx (4)
- 19-30: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3-27]
The updated import statements and constants for the Ecotone Fork Upgrade event times are correctly implemented, ensuring consistency and clarity in handling the event.
- 40-46: The
EcotoneForkUpgradeBannercomponent is correctly implemented, effectively using theAnnouncementBannercomponent to communicate details about the Ecotone Fork Upgrade.- 63-74: The
EcotoneForkWarningMessagefunction is well-implemented, using conditional logic to display a warning message for specific chains during the Ecotone Fork upgrade.- 108-125: The
useEcotoneForkCountdownProgresshook is correctly implemented, effectively managing the countdown and operational status of the bridge during the Ecotone Fork upgrade.packages/contracts-core/CHANGELOG.md (1)
- 6-8: The documentation of the version bump from
1.0.27to1.0.28in the CHANGELOG is clear and follows best practices, indicating it's a version bump only.packages/widget/CHANGELOG.md (2)
- 6-9: The entry for version
0.1.9correctly follows the pattern of previous entries by noting it as a version bump without additional changes. This consistency is good for readability and understanding the project's history.- 3-16: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [81-86]
The feature addition in version
0.1.0is clearly documented, including a brief description, issue link, and commit hash. This level of detail is excellent for transparency and traceability. However, consider clarifying the feature description to enhance understanding for readers not deeply familiar with the project.Consider expanding the description of the
useBridgeSelectionshelper to briefly explain its purpose and impact on consumer selection awareness.packages/rest-api/CHANGELOG.md (1)
- 6-9: The addition to the CHANGELOG.md for version 1.0.57 follows the established format and correctly documents the version bump with no functional modifications. This change is consistent with the project's documentation practices.
packages/sdk-router/CHANGELOG.md (1)
- 6-9: The version bump entry for 0.3.29 is correctly documented, adhering to the conventional commits and changelog format. The provided comparison link is appropriately formatted. Good job maintaining the consistency and clarity in the changelog.
packages/synapse-interface/public/blacklist.json (1)
- 430-432: The addition of new addresses to the blacklist is noted. However, it's important to ensure that these addresses are not duplicates of existing ones in the list. A manual check might be sufficient for now, but as the list grows, consider implementing a script or automated process to verify the uniqueness of new entries before they are added.
Additionally, managing a large blacklist in a JSON file might become cumbersome over time. It could be beneficial to explore more scalable and maintainable solutions, such as storing these addresses in a database or using a dedicated service for managing blacklists.
packages/synapse-interface/pages/state-managed-bridge/index.tsx (4)
- 89-91: The import statements for
MaintenanceWarningMessageanduseMaintenanceCountdownProgresshave been updated to reflect the new focus on maintenance events handling. This change aligns with the PR's objective to enhance functionality related to maintenance events. Ensure that the removed components and hooks (EcotoneForkWarningMessageanduseEcotoneForkCountdownProgress) are no longer used elsewhere in the project to avoid any unresolved references.- 525-528: The usage of
useMaintenanceCountdownProgresshook to manage the state related to maintenance events is a good practice. It encapsulates the logic for determining if maintenance is pending and if the current chain is disabled, which can improve the maintainability of the code. However, ensure that the logic withinuseMaintenanceCountdownProgressis thoroughly tested, especially since it likely interacts with external data sources or APIs to determine the maintenance state.- 567-567: Displaying the
MaintenanceCountdownProgressBardirectly within theCardcomponent ensures that users are immediately aware of any ongoing or upcoming maintenance events. This is a user-friendly approach to communicate important system states. However, consider the visual impact of this addition on the overall UI design and user experience, especially in scenarios where no maintenance is scheduled.- 605-605: The conditional rendering of
MaintenanceWarningMessagebased onisMaintenancePendingis a clear and effective way to alert users about maintenance events. This approach ensures that the warning message is only shown when necessary, which is good for maintaining a clean user interface. However, ensure that theisMaintenancePendingstate accurately reflects the actual maintenance status to avoid any confusion for the users..github/workflows/go.yml (6)
- 332-332: The Go version is updated to
1.21.xin thelintjob. This change is consistent with the updates in thetestandbuildjobs. However, it's important to verify that all Go code in the repository is compatible with Go1.21.xto avoid potential build or runtime issues.- 329-335: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [24-24]
There's a TODO comment about potentially reducing the
fetch-depthfor the checkout action. Reducingfetch-depthcan speed up the checkout process, especially for repositories with a large history. Consider evaluating the impact of reducingfetch-depthto a smaller value like500or1000to improve CI run times.
- 329-335: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [26-26]
Another TODO comment questions the necessity of using
submodules: 'recursive'. If your project does not use Git submodules, you can remove this option to slightly speed up the checkout process. If submodules are used, ensure they are necessary for the CI workflow.
- 332-332: In the
lintjob, the Go version is explicitly set to1.21.x. This is a good practice to ensure that the linting process uses the same Go version as the build and test processes. Consistency across these processes helps avoid discrepancies that could arise from differences in language features or behavior between versions.- 329-335: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [186-186]
The caching strategy for Go modules is well-implemented across the
testandbuildjobs. Using a cache key that includes the hash of**/go.sumfiles is a best practice that ensures the cache is invalidated when dependencies change. This approach optimizes build times while ensuring that the latest dependencies are used.
- 329-335: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [172-172]
The use of a Docker service for MariaDB in the
testjob is a good practice for integration testing. It ensures that tests run against a consistent database environment. The configuration for health checks and environment variables is appropriately set up to support the testing environment.packages/sdk-router/src/sdk.test.ts (1)
- 1190-1190: The tests for
SynapseRFQare skipped. It's important to ensure that all functionalities are covered by tests to maintain code quality and catch potential issues early.Please verify the reason for skipping these tests. If the functionality is not yet implemented or external dependencies are not available, consider adding a comment explaining the reason. Otherwise, it would be beneficial to implement these tests to cover all code paths.
packages/widget/CHANGELOG.md
Outdated
| All notable changes to this project will be documented in this file. | ||
| See [Conventional Commits](https://conventionalcommits.org) for commit guidelines. | ||
|
|
||
| ## [0.1.9](https://github.com/synapsecns/sanguine/compare/@synapsecns/widget@0.1.8...@synapsecns/widget@0.1.9) (2024-03-21) | ||
|
|
||
| **Note:** Version bump only for package @synapsecns/widget | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| ## [0.1.8](https://github.com/synapsecns/sanguine/compare/@synapsecns/widget@0.1.7...@synapsecns/widget@0.1.8) (2024-03-18) | ||
|
|
||
| **Note:** Version bump only for package @synapsecns/widget |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [22-27]
The bug fix entry for version 0.1.7 is well-documented, including a brief description, issue link, and commit hash. This provides a clear and traceable history of changes, which is beneficial for maintainers and users alike. However, ensure that "urls" is written in uppercase ("URLs") for consistency with common technical terminology.
- updated image urls
+ updated image URLs| @@ -98,6 +98,7 @@ export class FastBridgeRouter implements SynapseModule { | |||
| * @inheritdoc SynapseModule.getSynapseTxId | |||
| */ | |||
| public async getSynapseTxId(txHash: string): Promise<string> { | |||
| // TODO: this should support older instances of FastBridge to track legacy txs | |||
There was a problem hiding this comment.
The comment regarding supporting older instances of FastBridge for tracking legacy transactions is clear and appropriately placed. Consider creating a tracking issue for this TODO item to ensure it's addressed in future updates.
Would you like me to open a GitHub issue to track this task?
| @@ -114,6 +115,7 @@ | |||
| * @inheritdoc SynapseModule.getBridgeTxStatus | |||
| */ | |||
| public async getBridgeTxStatus(synapseTxId: string): Promise<boolean> { | |||
| // TODO: this should support older instances of FastBridge to track legacy txs | |||
There was a problem hiding this comment.
The comment about supporting older instances of FastBridge for tracking legacy transactions is consistent and clear. Consider creating a tracking issue for this TODO item to ensure it's addressed in future updates.
Would you like me to open a GitHub issue to track this task?
| - uses: actions/upload-artifact@v4 | ||
| if: steps.yaml-lint.outcome == 'success' | ||
| with: | ||
| name: yamllint-logfile | ||
| path: ${{ steps.yaml-lint.outputs.logfile }} | ||
|
|
||
|
|
There was a problem hiding this comment.
The TODO comment to reinstate lint is a good reminder for future improvements. Ensure this task is tracked appropriately.
Would you like me to open a GitHub issue to track the reinstatement of lint checks?
Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- packages/synapse-interface/CHANGELOG.md (1 hunks)
- packages/synapse-interface/package.json (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/synapse-interface/CHANGELOG.md
- packages/synapse-interface/package.json
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (4)
- contrib/promexporter/config/config.go (1 hunks)
- packages/contracts-core/CHANGELOG.md (1 hunks)
- packages/contracts-core/package.json (3 hunks)
- yarn.lock (7 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/contracts-core/package.json
- yarn.lock
Additional comments: 2
contrib/promexporter/config/config.go (1)
- 107-115: The addition of a new
SubmitterChecksentry with address0xdc927bd56cf9dfc2e3779c7e3d6d28da1c219969and namerfq2is noted. Ensure that this new entry is correctly integrated into the system and that any necessary updates to related components or documentation have been made.packages/contracts-core/CHANGELOG.md (1)
- 6-9: The version bump to
1.0.29for@synapsecns/contracts-coreis documented. It's important to ensure that all related documentation and version references throughout the project are updated to reflect this change.Verification successful
The occurrences of "@synapsecns/contracts-core@1.0.28" found in the
CHANGELOG.mdare appropriate and serve as historical documentation for the version changes. There's no need to update these references as they correctly document the transition from version 1.0.28 to 1.0.29. Therefore, the review comment is addressed correctly in the context of maintaining accurate version history in the changelog.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all references to the old version have been updated rg "@synapsecns/contracts-core@1.0.28" || echo "All references to the old version have been updated."Length of output: 398
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- packages/synapse-interface/package.json (2 hunks)
- yarn.lock (19 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/synapse-interface/package.json
There was a problem hiding this comment.
Review Status
Actionable comments generated: 12
Configuration used: .coderabbit.yaml
Files selected for processing (48)
- .gitmodules (1 hunks)
- packages/synapse-interface/CHANGELOG.md (1 hunks)
- packages/synapse-interface/package.json (2 hunks)
- services/cctp-relayer/config/chain.go (3 hunks)
- services/cctp-relayer/config/config.go (3 hunks)
- services/cctp-relayer/contracts/messagetransmitter/doc.go (1 hunks)
- services/cctp-relayer/contracts/messagetransmitter/generate.go (1 hunks)
- services/cctp-relayer/contracts/messagetransmitter/messagetransmitter.metadata.go (1 hunks)
- services/cctp-relayer/contracts/messagetransmitter/topics.go (1 hunks)
- services/cctp-relayer/contracts/tokenmessenger/doc.go (1 hunks)
- services/cctp-relayer/contracts/tokenmessenger/generate.go (1 hunks)
- services/cctp-relayer/contracts/tokenmessenger/tokenmessenger.metadata.go (1 hunks)
- services/cctp-relayer/contracts/tokenmessenger/topics.go (1 hunks)
- services/cctp-relayer/db/relayer_db.go (1 hunks)
- services/cctp-relayer/db/sql/base/message.go (1 hunks)
- services/cctp-relayer/external/evm-cctp-contracts (1 hunks)
- services/cctp-relayer/relayer/chains.go (1 hunks)
- services/cctp-relayer/relayer/circle.go (1 hunks)
- services/cctp-relayer/relayer/export_test.go (2 hunks)
- services/cctp-relayer/relayer/handler.go (1 hunks)
- services/cctp-relayer/relayer/relayer.go (11 hunks)
- services/cctp-relayer/relayer/relayer_test.go (3 hunks)
- services/cctp-relayer/relayer/suite_test.go (1 hunks)
- services/cctp-relayer/relayer/synapse.go (1 hunks)
- services/cctp-relayer/relayer/synapse_test.go (1 hunks)
- services/cctp-relayer/relayer/util.go (1 hunks)
- services/cctp-relayer/relayer/util_test.go (1 hunks)
- services/cctp-relayer/testutil/typecast.go (1 hunks)
- services/cctp-relayer/types/messagetype_string.go (1 hunks)
- services/cctp-relayer/types/type.go (1 hunks)
- services/rfq/e2e/setup_test.go (3 hunks)
- services/rfq/relayer/cmd/cmd.go (1 hunks)
- services/rfq/relayer/cmd/commands.go (1 hunks)
- services/rfq/relayer/inventory/circle.go (1 hunks)
- services/rfq/relayer/inventory/manager.go (7 hunks)
- services/rfq/relayer/inventory/rebalance.go (2 hunks)
- services/rfq/relayer/inventory/synapse.go (1 hunks)
- services/rfq/relayer/relconfig/config.go (2 hunks)
- services/rfq/relayer/relconfig/config_test.go (4 hunks)
- services/rfq/relayer/relconfig/enum.go (1 hunks)
- services/rfq/relayer/relconfig/getters.go (3 hunks)
- services/rfq/relayer/relconfig/rebalancemethod_string.go (1 hunks)
- services/rfq/relayer/reldb/base/model.go (2 hunks)
- services/rfq/relayer/reldb/base/model_test.go (1 hunks)
- services/rfq/relayer/reldb/base/rebalance.go (3 hunks)
- services/rfq/relayer/reldb/db.go (5 hunks)
- services/rfq/relayer/reldb/db_test.go (2 hunks)
- services/rfq/relayer/service/relayer.go (7 hunks)
Files skipped from review due to trivial changes (9)
- services/cctp-relayer/contracts/messagetransmitter/doc.go
- services/cctp-relayer/contracts/messagetransmitter/generate.go
- services/cctp-relayer/contracts/tokenmessenger/doc.go
- services/cctp-relayer/contracts/tokenmessenger/generate.go
- services/cctp-relayer/external/evm-cctp-contracts
- services/cctp-relayer/testutil/typecast.go
- services/cctp-relayer/types/messagetype_string.go
- services/rfq/relayer/cmd/cmd.go
- services/rfq/relayer/reldb/base/model_test.go
Files skipped from review as they are similar to previous changes (2)
- packages/synapse-interface/CHANGELOG.md
- packages/synapse-interface/package.json
Additional comments: 79
services/cctp-relayer/types/type.go (1)
- 3-13: The addition of
MessageTypeenumeration and its constantsSynapseMessageTypeandCircleMessageTypeare correctly implemented. Usingiota + 1for enumeration starting from 1 is a good practice to avoid uninitialized values. The use ofgo:generatefor generating string methods is also appropriate.services/rfq/relayer/inventory/rebalance.go (1)
- 2-8: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-14]
The
RebalanceDatastruct andRebalanceManagerinterface are well-defined and focus on the core functionality of rebalancing. The use ofbig.Intfor handling large numbers in theAmountfield is appropriate. The interface methodsStartandExecuteare clearly defined, indicating a clean separation of concerns.services/rfq/relayer/relconfig/enum.go (1)
- 11-14: The addition of
RebalanceMethodSynapseCCTPandRebalanceMethodCircleCCTPconstants extends the rebalance methods functionality appropriately. Usingiotafor enumeration andgo:generatefor generating string methods follows Go best practices.services/cctp-relayer/contracts/tokenmessenger/tokenmessenger.metadata.go (1)
- 1-25: The use of
go:embedfor including contract metadata and the unmarshalling process into theContractsmap are correctly implemented. The error handling strategy usingpanicin theinitfunction is appropriate for failing fast if critical resources are not loaded correctly.services/cctp-relayer/contracts/messagetransmitter/messagetransmitter.metadata.go (1)
- 1-25: The use of
go:embedfor including contract metadata and the unmarshalling process into theContractsmap are correctly implemented, following the same best practices as seen in thetokenmessenger.metadata.gofile. The error handling strategy usingpanicin theinitfunction is appropriate for critical resource loading.services/cctp-relayer/relayer/handler.go (1)
- 1-16: The
CCTPHandlerinterface is well-defined, with clear method signatures for handling logs, processing sent events, and submitting receive messages related to CCTP contracts. This indicates a clean separation of concerns and a focus on specific functionalities.services/cctp-relayer/contracts/tokenmessenger/topics.go (1)
- 1-26: The initialization of
DepositForBurnTopicusing theTokenMessengercontract ABI is correctly implemented. The use ofpanicfor error handling ensures that the application fails fast if critical resources are not correctly initialized, which is appropriate in this context.services/rfq/relayer/relconfig/rebalancemethod_string.go (1)
- 9-22: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-19]
The generated code correctly reflects the changes to the
RebalanceMethodconstants, including the addition ofRebalanceMethodSynapseCCTPandRebalanceMethodCircleCCTP. The use of thestringertool for generating string methods for enums is a good practice in Go.services/cctp-relayer/contracts/messagetransmitter/topics.go (1)
- 10-28: Using
panicfor error handling in theinitfunction can lead to the termination of the entire application if an error occurs during initialization. Consider handling errors more gracefully or ensure that the use ofpanicis well-documented and justified, given its impact.services/cctp-relayer/relayer/util_test.go (1)
- 11-18: The test function
TestParseDestDomainis well-structured and correctly uses assertions. However, consider adding a comment about the potential fragility of using real transaction data in tests, as this can make the tests less reliable if the data changes or behaves differently in different environments.services/rfq/relayer/cmd/commands.go (1)
- 6-6: Ensure that the newly added import
github.com/synapsecns/sanguine/coreis used within the file. Unused imports should be removed to avoid confusion and unnecessary dependencies.Verification successful
Given the warning about the pattern and the lack of output, it's necessary to refine the search approach to verify the usage of the import statement more effectively. Let's perform a broader search to find any usage of the
github.com/synapsecns/sanguine/coreimport across the Go files in the repository.
The output from the broader search for the usage of the import statement
github.com/synapsecns/sanguine/coreacross Go files in the repository confirms that this import is indeed used in multiple files, including the specific file under review (services/rfq/relayer/cmd/commands.go). This evidence verifies that the import statement is not unused and therefore, the initial concern about potentially unnecessary dependencies due to an unused import is addressed.Given this context, the import statement
github.com/synapsecns/sanguine/coreis correctly used within the fileservices/rfq/relayer/cmd/commands.goamong others, aligning with best practices for managing imports in Go projects.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of the newly added import. ast-grep --lang go --pattern $'import $$$ { $_, "github.com/synapsecns/sanguine/core", $$$ }'Length of output: 439
Script:
#!/bin/bash # Broaden the search for the import statement usage. rg 'github.com/synapsecns/sanguine/core' --type goLength of output: 56639
.gitmodules (1)
- 23-25: The addition of the new submodule
evm-cctp-contractsis correctly formatted. Ensure that the URLhttps://github.com/circlefin/evm-cctp-contracts.gitis correct and that the submodule is necessary for the project.Verification successful
The submodule URL
https://github.com/circlefin/evm-cctp-contracts.gitis correct, though it redirects tohttps://github.com/circlefin/evm-cctp-contracts, which is expected behavior for GitHub URLs when accessed through a web client. This confirms the submodule's URL is accessible and correctly specified in the.gitmodulesfile.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the submodule URL is accessible. curl --head https://github.com/circlefin/evm-cctp-contracts.gitLength of output: 1307
services/cctp-relayer/db/relayer_db.go (1)
- 22-23: The addition of the
GetMessageByHashmethod to theCCTPRelayerDBReaderinterface is appropriate. Consider adding documentation for this method to explain its purpose and usage, enhancing code readability and maintainability.services/cctp-relayer/relayer/export_test.go (2)
- 17-18: The modification to the
HandleLogfunction to return a booleanshouldProcessis a useful enhancement that provides more control over the processing flow. This change aligns well with common Go idioms.- 30-34: The modification to the
StoreCircleRequestFulfilledfunction to handle a different type assertion and return an error message if the assertion fails is a robustness improvement. Usingfmt.Errorffor error messaging aligns with Go best practices.services/cctp-relayer/config/chain.go (1)
- 16-17: The addition of the
TokenMessengerAddressfield toChainConfigand the correspondingGetTokenMessengerAddressmethod is appropriate. Consider adding documentation for the new field and method to explain their purpose and usage, enhancing code readability and maintainability.services/cctp-relayer/relayer/relayer_test.go (1)
- 2-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The
TestFetchAttestationfunction is well-structured and correctly tests the attestation fetching functionality. Consider adding more test cases to cover different scenarios, such as failure cases or different attestation contents, to ensure comprehensive test coverage.services/cctp-relayer/relayer/chains.go (1)
- 5-5: Consider moving this file to a common package as suggested by the TODO comment, especially if these chain IDs and domain mappings are used across multiple packages. This will improve code reusability and maintainability.
services/cctp-relayer/relayer/util.go (1)
- 61-72: The error handling in
GetMessageTransmitterAddressis well-implemented, wrapping the underlying error with context. This is a good practice as it aids in debugging by providing more information about where the error occurred.services/rfq/relayer/reldb/db_test.go (1)
- 36-93: The test
TestStoreAndUpdateRebalanceis comprehensive, covering the entire lifecycle of a rebalance from initiation to completion. It checks for pending rebalances, stores a new rebalance, updates it to pending, and finally to completed, validating the state at each step. This ensures the rebalance functionality works as expected.services/rfq/relayer/reldb/base/rebalance.go (2)
- 22-58: The refactoring of
UpdateRebalanceStatustoUpdateRebalancewith the addedupdateIDparameter allows for more flexible updates to the rebalance records. This change, along with the conditional logic to update the rebalance ID based on theupdateIDflag, enhances the functionality and adaptability of the rebalance update process.- 104-122: The addition of
GetRebalanceByIDis a valuable enhancement, providing a way to retrieve rebalance records by their ID. This function is crucial for operations that need to query specific rebalance actions, improving the system's ability to monitor and manage rebalances.services/cctp-relayer/config/config.go (3)
- 27-38: The addition of
CCTPType,ScribePort, andScribeURLfields to theConfigstruct is a significant enhancement, allowing for more granular configuration of the CCTP relayer. This change enables the specification of the CCTP method and the configuration of the scribe server, which are essential for the relayer's operation.- 101-114: The
GetCCTPTypemethod provides a clean and straightforward way to retrieve the CCTP method based on the configuration. The use of a switch statement for handling different CCTP types and the default case for handling unspecified types ensures that the method is robust and flexible.- 116-124: The
GetChainConfigmethod is a useful addition, allowing for the retrieval of chain-specific configurations based on the chain ID. This method enhances the flexibility of the configuration system by enabling the dynamic selection of chain configurations at runtime.services/cctp-relayer/db/sql/base/message.go (1)
- 142-155: The addition of the
GetMessageByHashfunction is a valuable enhancement, providing a way to retrieve messages based on their hash. This functionality is crucial for operations that need to query specific messages, improving the system's ability to monitor and manage message flows.services/rfq/relayer/relconfig/config.go (3)
- 15-15: The import of
cctpConfigfromservices/cctp-relayer/configis a good practice, as it allows for the integration of CCTP relayer configuration directly into the RFQ relayer configuration. This promotes modularity and reusability of configuration structures.- 52-53: The inclusion of
CCTPRelayerConfigin theConfigstruct is a significant enhancement, enabling the RFQ relayer to be configured with CCTP relayer settings directly. This change facilitates tighter integration between the RFQ and CCTP relayers, enhancing the overall functionality.- 60-64: Renaming
CCTPAddresstoSynapseCCTPAddressand addingTokenMessengerAddressimproves clarity and specificity in the configuration. This change makes it easier to understand the purpose of each address and ensures that the configuration accurately reflects the system's architecture.services/rfq/relayer/reldb/db.go (4)
- 27-29: The change in the function signature from
UpdateRebalanceStatustoUpdateRebalancewith additional parameters (rebalance Rebalance, updateID bool) is a significant improvement. It provides more flexibility in updating rebalance records, allowing for a more comprehensive update operation that can include changing the rebalance ID.- 44-45: The addition of
GetRebalanceByIDto theReaderinterface is a valuable enhancement. It provides a standardized way to retrieve rebalance records by ID, improving the system's ability to query specific rebalance actions. This method is essential for monitoring and managing rebalances effectively.- 62-63: Introducing the
ErrNoRebalanceForIDerror variable is a good practice. It standardizes the error handling for cases where a rebalance record cannot be found by its ID, improving the clarity and consistency of error messages throughout the system.- 168-168: Using a pointer to a string for
RebalanceIDin theRebalancestruct is a thoughtful design choice. It allows for the distinction between an unsetRebalanceIDand an empty string, providing more nuanced control over rebalance record handling.services/cctp-relayer/relayer/suite_test.go (1)
- 201-201: The addition of
CCTPType: "synapse"in the configuration setup is clear and aligns with the PR's objectives to enhance functionality. Ensure that the integration and usage of this new configuration are verified across the system, especially in components that rely on theGetTestConfigfunction.Verification successful
The review comment is verified by the new context. The addition of
CCTPType: "synapse"in the configuration setup is consistent with the system's design to support different CCTP transaction types. The usage ofCCTPTypeacross configuration, testing, and service implementation files confirms its integration and relevance across the system.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of CCTPType across the system rg "CCTPType" --type goLength of output: 3487
services/rfq/relayer/reldb/base/model.go (3)
- 140-151: The changes to handle
RebalanceIDusing a pointer and the introduction ofhashToNullStringforOriginTxHashandDestTxHashare well-implemented. These changes improve the handling of nullable fields and enhance the code's readability and maintainability.- 153-161: The introduction of the
emptyHashvariable and thehashToNullStringfunction is a good practice for handling empty or null hashes in a consistent manner. This approach reduces the risk of errors and improves code clarity.- 219-234: The
ToRebalancemethod efficiently converts a database object to areldb.Rebalanceobject. Ensure that error handling is robust, especially when converting strings to big integers, as this can be a source of runtime errors if the string is not a valid representation of a number.services/rfq/relayer/inventory/synapse.go (4)
- 23-39: The
rebalanceManagerSynapseCCTPstruct is well-defined, encapsulating all necessary components for managing rebalances in the Synapse CCTP context. Ensure that all fields are used effectively throughout the manager's methods and that the struct's design aligns with the overall architecture.- 42-52: The constructor
newRebalanceManagerSynapseCCTPis correctly setting up therebalanceManagerSynapseCCTPinstance. It's good practice to validate any inputs or configurations that could affect the manager's operation to prevent runtime issues.Verification successful
Given the information from the script output, it's clear that the constructor
newRebalanceManagerSynapseCCTPis defined and used within the codebase. However, the output does not directly confirm whether the configurations passed to the constructor are validated. To accurately verify the presence of input validation, a deeper inspection of the constructor's implementation and its invocation is necessary. Since the script output alone does not provide this level of detail, further verification is required.
The verification process reveals that the constructor
newRebalanceManagerSynapseCCTPand its usage do not include explicit validation of the configurations passed to it. This observation supports the suggestion made in the review comment that validating inputs or configurations could be beneficial to prevent runtime issues. Therefore, the review comment is correct in highlighting the absence of validation as a potential area for improvement.* 133-178: The `Execute` method for performing rebalances appears to be correctly implemented, with appropriate error handling and transaction submission logic. However, ensure that the `TODO` comments regarding inspecting certain parameters are addressed before finalizing the implementation to avoid potential issues.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all configurations passed to the constructor are validated rg "newRebalanceManagerSynapseCCTP" --type goLength of output: 490
Script:
#!/bin/bash # Inspect the constructor definition for validation logic rg "newRebalanceManagerSynapseCCTP" -A 20 --type go # Inspect the usage of the constructor for any validation before its call rg "newRebalanceManagerSynapseCCTP" -B 3 -A 3 --type goLength of output: 4730
Verification successful
The verification process confirms the presence of unresolved
TODOcomments in theExecutemethod, as mentioned in the review comment. These comments are related to inspecting parameters passed to theSendCircleTokenfunction, which should be addressed before finalizing the implementation.* 181-267: The `listen` method's implementation for handling CCTP events is comprehensive, including error handling and logging. However, ensure that the logging framework (`logger.Warnf`) is correctly set up and that all error paths are adequately logged for troubleshooting.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for unresolved TODOs in the Execute method rg "TODO" services/rfq/relayer/inventory/synapse.goLength of output: 111
Verification successful
The verification process confirms that the logging framework is correctly set up and utilized within the
listenmethod for handling CCTP events, including comprehensive error handling and logging. This aligns with the initial review comment, ensuring that all error paths are adequately logged for troubleshooting purposes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the setup of the logging framework used in the listen method rg "logger.Warnf" --type goLength of output: 11054
services/cctp-relayer/relayer/synapse_test.go (2)
- 59-118: The
TestStoreCircleRequestFulfilledtest checks the storage of a circle request fulfilled event. It's important to also test scenarios where the event fails to fulfill or encounters errors during processing to ensure comprehensive coverage.- 120-155: The
TestSubmitReceiveCircleTokentest validates the submission of a receive circle token request. Consider adding tests for invalid or malformed messages to ensure the system gracefully handles such cases.Verification successful
The review comment suggesting the addition of tests for invalid or malformed messages in the context of
TestSubmitReceiveCircleTokenis supported by the script output, which indicates that no such tests currently exist. This makes the suggestion for enhancing test coverage relevant and actionable.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests handling invalid or malformed messages in TestSubmitReceiveCircleToken rg "TestSubmitReceiveCircleToken" --type goLength of output: 149
services/rfq/relayer/service/relayer.go (4)
- 19-22: The addition of new packages for CCTP relayer functionality is noted. Ensure that these packages are properly versioned and maintained to avoid future compatibility issues.
- 40-40: The update to the
clientreference to use the new CCTP-related package is appropriate. Verify that all instances whereclientis used have been updated accordingly to prevent runtime errors.Verification successful
The verification process confirms that the update to the
clientreference to use the new CCTP-related package has been appropriately handled. All instances whereclientis used have been updated accordingly, with no evidence of outdated references that could lead to runtime errors.* 223-229: Introducing the `startCCTPRelayer` method within the `Start` function is a significant change. It's crucial to ensure that error handling is consistent and that any failures in starting the CCTP relayer do not halt the entire application unless intended. Consider adding more detailed logging around this process for easier debugging. * 257-289: The `startCCTPRelayer` method's implementation appears to be well-structured and follows good error handling practices. However, ensure that the database connection and the creation of the CCTP relayer are thoroughly tested, especially in failure scenarios, to prevent any unexpected behavior in production.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any outdated references to the old client package. rg --type go "omniClient.[^NewOmnirpcClient]" services/rfq/relayer/service/Length of output: 1204
services/rfq/relayer/relconfig/config_test.go (2)
- 22-23: The addition of
SynapseCCTPAddressandTokenMessengerAddressfields to theChainConfigstruct is noted. Ensure that these new fields are documented and that their usage is consistent across the codebase.Also applies to: 40-41, 60-61
- 99-111: The updates to the test functions to reflect the new field names in
ChainConfigare appropriate. It's good practice to ensure that all related configurations and their usages are updated accordingly. Additionally, consider adding new test cases if the logic related to these new fields introduces new pathways or conditions.Also applies to: 113-124
services/cctp-relayer/relayer/synapse.go (3)
- 59-91: The method
HandleLogcorrectly processes different types of log topics related to CCTP events. Ensure that the handling of unknown topics (line 89) is intentional and that it won't suppress important errors or warnings that should be addressed.- 198-267: The
SubmitReceiveMessagemethod performs critical operations, including submitting transactions. Ensure that error handling and transaction submission logic are thoroughly tested, especially in scenarios where network or contract issues might arise.- 269-339: The handling of the
CircleRequestFulfilledevent is crucial for marking messages as complete. Verify that the logic for fetching and updating message states is robust and accounts for potential edge cases, such as missing messages or database errors.services/cctp-relayer/relayer/circle.go (4)
- 39-68: Consider refactoring the initialization logic within
NewCircleCCTPHandler, specifically the setup ofboundMessageTransmittersand the signer configuration, into separate functions. This can improve readability and maintainability by separating concerns and making the main function more concise.- 75-78: The comment on line 75 suggests reconsidering the necessity of the check for
len(log.Topics) == 0. If this situation is indeed impossible or already handled upstream, removing this check could simplify the function.- 230-230: The use of
//nolint:nilnilon line 230 suggests suppressing a linter warning about returningnilfor both the message and error. It's important to verify the necessity of this directive and consider addressing the underlying issue it's suppressing, such as by providing more informative error handling or return values.- 316-316: When no message is found (line 316), consider enhancing the error handling to provide more informative feedback or logging. This could help in diagnosing issues and understanding the context in which messages are missing.
services/rfq/e2e/setup_test.go (1)
- 250-258: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [253-268]
The renaming of
CCTPAddressfields toSynapseCCTPAddressand the update of therebalanceMethodstring value from "cctp" to "synapsecctp" are clear and correctly implemented. However, ensure that these changes are reflected across all usages in the codebase to maintain consistency and prevent any potential issues due to the renaming.Verification successful
The verification process confirms that the renaming of
CCTPAddressfields toSynapseCCTPAddressand the update of therebalanceMethodstring value from "cctp" to "synapsecctp" have been correctly implemented and are used consistently across the codebase. No issues were found related to these changes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the renamed fields are consistently used across the codebase. rg --type go "CCTPAddress" # Expecting no results for the old field name. # Verify the new string value for rebalanceMethod is used consistently. rg --type go "synapsecctp" # Expecting results showing consistent usage of the new string value.Length of output: 4249
services/cctp-relayer/relayer/relayer.go (6)
- 55-56: The introduction of
cctpHandlerfor interacting with CCTP contracts is a significant improvement. It encapsulates CCTP-related logic, making theCCTPRelayerstruct cleaner and more focused on its responsibilities. Ensure thatcctpHandleris properly implemented and tested to handle the various CCTP contract interactions efficiently.- 118-136: The refactoring to handle different CCTP types in the
NewCCTPRelayerfunction is well-structured and improves the code's readability and maintainability. However, ensure that all CCTP types are adequately supported and that the error handling is robust to prevent runtime issues.- 282-282: Using
cctpHandler.SubmitReceiveMessageto process messages is a clean approach. It abstracts the complexity of submitting messages to the CCTP contract. Ensure that error handling withinSubmitReceiveMessageis comprehensive to gracefully handle any issues that may arise during message submission.- 298-324: The switch statement for handling different CCTP types during event streaming is a good practice. It allows for easy extension and maintenance of the codebase as new CCTP types are introduced. However, ensure that the logic within each case is thoroughly tested, especially the interaction with external services like gRPC clients and the OmniRPC client.
- 422-427: The handling of logs within
streamLogsusingcctpHandler.HandleLogis a significant improvement, as it delegates the responsibility of log processing to a dedicated handler. However, the commented-out return statement for error handling (// return err) should be revisited. Decide whether to enable it for stricter error handling or remove it if the current warning log is deemed sufficient.Consider enabling the commented-out return statement for error handling to ensure that errors during log handling are not silently ignored. This could improve the robustness of the relayer service by preventing it from processing logs incorrectly due to unhandled errors.
- 449-449: The use of
cctpHandler.FetchAndProcessSentEventinprocessAPIRequeststo fetch and process events based on transaction hashes is a clean and modular approach. It keeps the API request processing logic simple and focused. Ensure thatFetchAndProcessSentEventincludes comprehensive error handling and logging to troubleshoot issues effectively.services/rfq/relayer/inventory/circle.go (8)
- 26-47: The struct
rebalanceManagerCircleCCTPis well-defined with clear field names and types. However, consider documenting the purpose of each field and the struct itself for better maintainability and understanding for new developers.- 49-61: The constructor function
newRebalanceManagerCircleCCTPis correctly initializing therebalanceManagerCircleCCTPstruct. It's good practice to return an interface type if possible, to enhance modularity and facilitate testing.- 98-137: The method
initContractsis well-structured and handles errors appropriately. Consider adding more detailed logging or comments explaining the purpose of each step for maintainability.- 140-186: The method
initListenersis correctly initializing listeners for both messenger and transmitter contracts. Similar toinitContracts, consider adding more detailed comments or logging for clarity.- 189-253: The method
Executeperforms the rebalance operation. It's crucial to ensure that all external calls, especially those interacting with smart contracts, are thoroughly tested to prevent unintended consequences. Consider adding more comprehensive error handling and validation of inputs.- 257-285: The method
listenDepositForBurnlistens for specific blockchain events. Ensure that the event filtering logic (log.Topics[0] != tokenmessenger.DepositForBurnTopic) is robust and correctly handles all expected cases.Verification successful
To effectively verify the robustness of the event filtering logic in the
listenDepositForBurnmethod, particularly the conditionlog.Topics[0] != tokenmessenger.DepositForBurnTopic, I will need to examine the definition oftokenmessenger.DepositForBurnTopicand any related documentation or comments in the code. This will help determine if the topic correctly represents theDepositForBurnevent and if the filtering logic is appropriately designed to handle all expected cases.
The verification process has confirmed that the
listenDepositForBurnmethod usestokenmessenger.DepositForBurnTopicto filter forDepositForBurnevents from blockchain logs. The definition ofDepositForBurnTopicinservices/cctp-relayer/contracts/tokenmessenger/topics.gois based on theTokenMessengercontract's ABI, ensuring that it accurately represents theDepositForBurnevent topic. This, along with the sanity check to ensureDepositForBurnTopicis not nil, supports the robustness of the event filtering logic as implemented.Given the information available and the context provided, the event filtering logic appears to be appropriately designed for its intended purpose. However, the absolute robustness of this logic would depend on the uniqueness and correct identification of the
DepositForBurnevent's topic, which is assumed to be correctly handled based on the current verification.* 320-368: The method `handleDepositForBurn` processes specific blockchain events. It's important to ensure that the logic for updating the database based on these events is correct and resilient to potential data inconsistencies. * 371-408: The method `handleMessageReceived` is similar to `handleDepositForBurn` in its purpose. Ensure that its logic for handling events and updating the database is thoroughly tested and validated.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Placeholder for a script to verify event filtering logic. echo "Verify event filtering logic."Length of output: 68
Script:
#!/bin/bash # Search for the definition of tokenmessenger.DepositForBurnTopic and any related comments or documentation. rg "DepositForBurnTopic" --context 5Length of output: 2729
services/rfq/relayer/relconfig/getters.go (3)
- 105-117: Renaming
GetCCTPAddresstoGetSynapseCCTPAddressimproves clarity by specifying the type of CCTP address. This change aligns with best practices for naming, making the codebase more understandable.- 119-129: The addition of
GetTokenMessengerAddressfunction is a good practice, as it encapsulates the logic for retrieving the TokenMessenger address, making the code more modular and maintainable.- 420-429: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [411-426]
The introduction of new rebalance methods
RebalanceMethodSynapseCCTPandRebalanceMethodCircleCCTPin theGetRebalanceMethodfunction enhances the flexibility of the rebalancing logic. Ensure that all parts of the codebase that depend on rebalance methods are updated accordingly.Verification successful
The verification process has confirmed that the new rebalance methods
RebalanceMethodSynapseCCTPandRebalanceMethodCircleCCTPare integrated and referenced in multiple parts of the codebase, including configuration and inventory management. This suggests that the necessary updates to accommodate these new methods have been made, aligning with the original review comment's guidance.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Placeholder for a script to verify the usage of new rebalance methods across the codebase. echo "Verify usage of new rebalance methods."Length of output: 86
Script:
#!/bin/bash # Search for references to the new rebalance methods in the codebase. rg "RebalanceMethodSynapseCCTP" || echo "No references found for RebalanceMethodSynapseCCTP." rg "RebalanceMethodCircleCCTP" || echo "No references found for RebalanceMethodCircleCCTP."Length of output: 1296
services/rfq/relayer/inventory/manager.go (5)
- 140-140: The addition of the
Allowancesfield in theTokenMetadatastruct introduces a mapping of contracts to their respective allowances. This is a significant enhancement for managing token allowances more efficiently. However, ensure that the initialization of this map is handled correctly to avoid nil map assignment errors.- 148-152: The introduction of new constants
contractRFQ,contractSynapseCCTP, andcontractTokenMessengeris a good practice for maintaining readability and avoiding the use of magic numbers. Ensure these constants are used consistently throughout the codebase.- 297-325: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [288-322]
The
ApproveAllTokensmethod now includes logic to handle approvals for different contracts based on the allowances map. This is a crucial update for managing token approvals more dynamically. However, ensure that error handling is robust, especially in scenarios where contract addresses might not be retrievable or the approval transaction fails.
- 586-594: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [575-591]
Initializing the
Allowancesmap with zero values for each contract type is a good practice to ensure that the map is ready for use. However, consider adding a comment to explain the rationale behind initializing these values to zero, especially for developers who might be unfamiliar with the context.+ // Initialize allowances with zero values to ensure they are ready for use for _, contract := range []spendableContract{contractRFQ, contractSynapseCCTP, contractTokenMessenger} { rtoken.Allowances[contract] = new(big.Int) }
- 607-620: The logic for fetching and setting allowances for different contracts during token initialization is well-implemented. However, ensure that error handling is robust for scenarios where contract addresses might not be retrievable. Additionally, consider optimizing the code to avoid duplication in fetching allowances for different contracts.
| case relconfig.RebalanceMethodSynapseCCTP: | ||
| rebalanceManagers[method] = newRebalanceManagerSynapseCCTP(cfg, handler, clientFetcher, txSubmitter, relayer, db) | ||
| case relconfig.RebalanceMethodCircleCCTP: | ||
| rebalanceManagers[method] = newRebalanceManagerCircleCCTP(cfg, handler, clientFetcher, txSubmitter, relayer, db) |
There was a problem hiding this comment.
The creation of new rebalance manager instances based on the rebalance method is a clean approach to handling different rebalancing strategies. However, consider adding a default case in the switch statement to handle unexpected rebalance methods gracefully.
default:
+ logger.Warnf("Unsupported rebalance method: %s", method)
+ continueCommittable 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.
| case relconfig.RebalanceMethodSynapseCCTP: | |
| rebalanceManagers[method] = newRebalanceManagerSynapseCCTP(cfg, handler, clientFetcher, txSubmitter, relayer, db) | |
| case relconfig.RebalanceMethodCircleCCTP: | |
| rebalanceManagers[method] = newRebalanceManagerCircleCCTP(cfg, handler, clientFetcher, txSubmitter, relayer, db) | |
| case relconfig.RebalanceMethodSynapseCCTP: | |
| rebalanceManagers[method] = newRebalanceManagerSynapseCCTP(cfg, handler, clientFetcher, txSubmitter, relayer, db) | |
| case relconfig.RebalanceMethodCircleCCTP: | |
| rebalanceManagers[method] = newRebalanceManagerCircleCCTP(cfg, handler, clientFetcher, txSubmitter, relayer, db) | |
| default: | |
| logger.Warnf("Unsupported rebalance method: %s", method) | |
| continue |
| // TODO: consider pulling from scribe? | ||
| // TODO: this function is particularly prone to silent errors |
There was a problem hiding this comment.
The TODO comments on lines 127-128 highlight areas for potential improvement. It's important to address these comments to ensure that silent errors are properly handled and to consider integrating with a "scribe" for fetching information, if applicable.
Would you like assistance in addressing these TODO items or further discussion on their implications?
| return fmt.Errorf("no contract found for chain %d", msg.DestChainID) | ||
| } | ||
|
|
||
| //TODO: check if already executed on destination? |
There was a problem hiding this comment.
The TODO comment on line 166 suggests checking if the message has already been executed on the destination. Implementing this check could prevent potential double submissions and ensure idempotency.
Would you like assistance in implementing this check or further discussion on its implications?
| return messageSentEvent.Message, nil | ||
| } | ||
| } | ||
| return nil, fmt.Errorf("no depositForBurn event found") |
There was a problem hiding this comment.
The error message on line 357, "no depositForBurn event found," seems to be a copy-paste error. It would be more accurate to say something like "no message sent event found" to reflect the operation being performed in this context.
- return nil, fmt.Errorf("no depositForBurn event found")
+ return nil, fmt.Errorf("no message sent event found")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.
| return nil, fmt.Errorf("no depositForBurn event found") | |
| return nil, fmt.Errorf("no message sent event found") |
| func (s *synapseCCTPHandler) FetchAndProcessSentEvent(parentCtx context.Context, txhash common.Hash, originChain uint32) (msg *relayTypes.Message, err error) { | ||
| ctx, span := s.handler.Tracer().Start(parentCtx, "FetchAndProcessSentEvent", trace.WithAttributes( | ||
| attribute.String(metrics.TxHash, txhash.String()), | ||
| attribute.Int(metrics.ChainID, int(originChain)), | ||
| )) | ||
|
|
||
| defer func() { | ||
| metrics.EndSpanWithErr(span, err) | ||
| }() | ||
|
|
||
| // check if message already exists before we do anything | ||
| msg, err = s.db.GetMessageByOriginHash(ctx, txhash) | ||
| // if we already have the message, we can just return it | ||
| if err == nil { | ||
| return msg, nil | ||
| } | ||
| if !errors.Is(err, gorm.ErrRecordNotFound) { | ||
| return nil, fmt.Errorf("could not get message by origin hash: %w", err) | ||
| } | ||
|
|
||
| ethClient, err := s.omniRPCClient.GetConfirmationsClient(ctx, int(originChain), 1) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain client: %w", err) | ||
| } | ||
|
|
||
| // TODO: consider pulling from scribe? | ||
| // TODO: this function is particularly prone to silent errors | ||
| receipt, err := ethClient.TransactionReceipt(ctx, txhash) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get transaction receipt: %w", err) | ||
| } | ||
|
|
||
| // From this receipt, we expect two different logs: | ||
| // - `messageSentEvent` gives us the raw bytes of the CCTP message | ||
| // - `circleRequestSentEvent` gives us auxiliary data for SynapseCCTP | ||
| var messageSentEvent *messagetransmitter.MessageTransmitterMessageSent | ||
| var circleRequestSentEvent *cctp.SynapseCCTPEventsCircleRequestSent | ||
|
|
||
| for _, log := range receipt.Logs { | ||
| // this should never happen | ||
| if len(log.Topics) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| switch log.Topics[0] { | ||
| case cctp.CircleRequestSentTopic: | ||
| // TODO: do we need to make sure log.Address matches our log.Address? | ||
| eventParser, err := cctp.NewSynapseCCTPEvents(log.Address, ethClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create event parser: %w", err) | ||
| } | ||
|
|
||
| circleRequestSentEvent, err = eventParser.ParseCircleRequestSent(*log) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not parse circle request sent: %w", err) | ||
| } | ||
| case messagetransmitter.MessageSentTopic: | ||
| eventParser, err := messagetransmitter.NewMessageTransmitterFilterer(log.Address, ethClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create event parser: %w", err) | ||
| } | ||
|
|
||
| messageSentEvent, err = eventParser.ParseMessageSent(*log) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not parse message sent: %w", err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if messageSentEvent == nil { | ||
| return nil, fmt.Errorf("no message sent event found") | ||
| } | ||
|
|
||
| if circleRequestSentEvent == nil { | ||
| return nil, fmt.Errorf("no circle request sent event found") | ||
| } | ||
|
|
||
| rawMsg := relayTypes.Message{ | ||
| OriginTxHash: txhash.String(), | ||
| OriginChainID: originChain, | ||
| DestChainID: uint32(circleRequestSentEvent.ChainId.Int64()), | ||
| Message: messageSentEvent.Message, | ||
| MessageHash: crypto.Keccak256Hash(messageSentEvent.Message).String(), | ||
| RequestID: common.Bytes2Hex(circleRequestSentEvent.RequestID[:]), | ||
|
|
||
| //Attestation: //comes from the api | ||
| RequestVersion: circleRequestSentEvent.RequestVersion, | ||
| FormattedRequest: circleRequestSentEvent.FormattedRequest, | ||
| BlockNumber: uint64(receipt.BlockNumber.Int64()), | ||
| } | ||
|
|
||
| // Store the requested message. | ||
| rawMsg.State = relayTypes.Pending | ||
| err = s.db.StoreMessage(ctx, rawMsg) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not store pending message: %w", err) | ||
| } | ||
|
|
||
| return &rawMsg, nil | ||
| } |
There was a problem hiding this comment.
The FetchAndProcessSentEvent method is complex and handles multiple steps, including fetching transaction receipts and parsing events. Consider refactoring this method to improve readability and maintainability, possibly by breaking it down into smaller, more focused functions.
- func (s *synapseCCTPHandler) FetchAndProcessSentEvent(parentCtx context.Context, txhash common.Hash, originChain uint32) (msg *relayTypes.Message, err error) {
+ // Proposed refactor: Break down into smaller functions for clarity.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.
| // fetchAndStoreCircleRequestSent handles the CircleRequestSent event. | |
| // | |
| //nolint:cyclop | |
| func (s *synapseCCTPHandler) FetchAndProcessSentEvent(parentCtx context.Context, txhash common.Hash, originChain uint32) (msg *relayTypes.Message, err error) { | |
| ctx, span := s.handler.Tracer().Start(parentCtx, "FetchAndProcessSentEvent", trace.WithAttributes( | |
| attribute.String(metrics.TxHash, txhash.String()), | |
| attribute.Int(metrics.ChainID, int(originChain)), | |
| )) | |
| defer func() { | |
| metrics.EndSpanWithErr(span, err) | |
| }() | |
| // check if message already exists before we do anything | |
| msg, err = s.db.GetMessageByOriginHash(ctx, txhash) | |
| // if we already have the message, we can just return it | |
| if err == nil { | |
| return msg, nil | |
| } | |
| if !errors.Is(err, gorm.ErrRecordNotFound) { | |
| return nil, fmt.Errorf("could not get message by origin hash: %w", err) | |
| } | |
| ethClient, err := s.omniRPCClient.GetConfirmationsClient(ctx, int(originChain), 1) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get chain client: %w", err) | |
| } | |
| // TODO: consider pulling from scribe? | |
| // TODO: this function is particularly prone to silent errors | |
| receipt, err := ethClient.TransactionReceipt(ctx, txhash) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get transaction receipt: %w", err) | |
| } | |
| // From this receipt, we expect two different logs: | |
| // - `messageSentEvent` gives us the raw bytes of the CCTP message | |
| // - `circleRequestSentEvent` gives us auxiliary data for SynapseCCTP | |
| var messageSentEvent *messagetransmitter.MessageTransmitterMessageSent | |
| var circleRequestSentEvent *cctp.SynapseCCTPEventsCircleRequestSent | |
| for _, log := range receipt.Logs { | |
| // this should never happen | |
| if len(log.Topics) == 0 { | |
| continue | |
| } | |
| switch log.Topics[0] { | |
| case cctp.CircleRequestSentTopic: | |
| // TODO: do we need to make sure log.Address matches our log.Address? | |
| eventParser, err := cctp.NewSynapseCCTPEvents(log.Address, ethClient) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not create event parser: %w", err) | |
| } | |
| circleRequestSentEvent, err = eventParser.ParseCircleRequestSent(*log) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not parse circle request sent: %w", err) | |
| } | |
| case messagetransmitter.MessageSentTopic: | |
| eventParser, err := messagetransmitter.NewMessageTransmitterFilterer(log.Address, ethClient) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not create event parser: %w", err) | |
| } | |
| messageSentEvent, err = eventParser.ParseMessageSent(*log) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not parse message sent: %w", err) | |
| } | |
| } | |
| } | |
| if messageSentEvent == nil { | |
| return nil, fmt.Errorf("no message sent event found") | |
| } | |
| if circleRequestSentEvent == nil { | |
| return nil, fmt.Errorf("no circle request sent event found") | |
| } | |
| rawMsg := relayTypes.Message{ | |
| OriginTxHash: txhash.String(), | |
| OriginChainID: originChain, | |
| DestChainID: uint32(circleRequestSentEvent.ChainId.Int64()), | |
| Message: messageSentEvent.Message, | |
| MessageHash: crypto.Keccak256Hash(messageSentEvent.Message).String(), | |
| RequestID: common.Bytes2Hex(circleRequestSentEvent.RequestID[:]), | |
| //Attestation: //comes from the api | |
| RequestVersion: circleRequestSentEvent.RequestVersion, | |
| FormattedRequest: circleRequestSentEvent.FormattedRequest, | |
| BlockNumber: uint64(receipt.BlockNumber.Int64()), | |
| } | |
| // Store the requested message. | |
| rawMsg.State = relayTypes.Pending | |
| err = s.db.StoreMessage(ctx, rawMsg) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not store pending message: %w", err) | |
| } | |
| return &rawMsg, nil | |
| } | |
| // Proposed refactor: Break down into smaller functions for clarity. |
| func indexUint32(memView []byte, index int, bytes uint8) (uint32, error) { | ||
| if int(bytes)+index > len(memView) || bytes > 4 { | ||
| return 0, errors.New("invalid index or byte length") | ||
| } | ||
|
|
||
| // Ensure we are not trying to parse more than 4 bytes for a uint32. | ||
| if bytes > 4 { | ||
| return 0, errors.New("byte length too large for uint32") |
There was a problem hiding this comment.
The check for bytes > 4 is redundant and appears twice (lines 26 and 31). Since uint32 can only hold up to 4 bytes, this condition will always be true when bytes > 4, making the second check unnecessary. Consider removing the duplicate check to simplify the code.
- if int(bytes)+index > len(memView) || bytes > 4 {
+ if int(bytes)+index > len(memView) {
return 0, errors.New("invalid index or byte length")
}
- // Ensure we are not trying to parse more than 4 bytes for a uint32.
- if bytes > 4 {
- return 0, errors.New("byte length too large for uint32")
- }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.
| func indexUint32(memView []byte, index int, bytes uint8) (uint32, error) { | |
| if int(bytes)+index > len(memView) || bytes > 4 { | |
| return 0, errors.New("invalid index or byte length") | |
| } | |
| // Ensure we are not trying to parse more than 4 bytes for a uint32. | |
| if bytes > 4 { | |
| return 0, errors.New("byte length too large for uint32") | |
| func indexUint32(memView []byte, index int, bytes uint8) (uint32, error) { | |
| if int(bytes)+index > len(memView) { | |
| return 0, errors.New("invalid index or byte length") | |
| } |
| func (c *rebalanceManagerCircleCCTP) Start(ctx context.Context) (err error) { | ||
| err = c.initContracts(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("could not initialize contracts: %w", err) | ||
| } | ||
|
|
||
| err = c.initListeners(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("could not initialize listeners: %w", err) | ||
| } | ||
|
|
||
| g, _ := errgroup.WithContext(ctx) | ||
| for cid := range c.cfg.Chains { | ||
| // capture func literal | ||
| chainID := cid | ||
| ethClient, err := c.chainClient.GetClient(ctx, big.NewInt(int64(chainID))) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get chain client: %w", err) | ||
| } | ||
| g.Go(func() error { | ||
| return c.listenDepositForBurn(ctx, chainID, ethClient) | ||
| }) | ||
| g.Go(func() error { | ||
| return c.listenMessageReceived(ctx, chainID, ethClient) | ||
| }) | ||
| } | ||
|
|
||
| err = g.Wait() | ||
| if err != nil { | ||
| return fmt.Errorf("error listening to contract: %w", err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
The method Start correctly initializes contracts and listeners, and handles errors appropriately. However, consider extracting the inner loop's logic into a separate method to reduce complexity and improve readability.
| func (c *rebalanceManagerCircleCCTP) listenMessageReceived(parentCtx context.Context, chainID int, ethClient client.EVM) (err error) { | ||
| listener, ok := c.transmitterListeners[chainID] | ||
| if !ok { | ||
| return fmt.Errorf("could not find listener for chain %d", chainID) | ||
| } | ||
|
|
||
| err = listener.Listen(parentCtx, func(parentCtx context.Context, log types.Log) (err error) { | ||
| ctx, span := c.handler.Tracer().Start(parentCtx, "rebalance.listenMessageReceived", trace.WithAttributes( | ||
| attribute.Int(metrics.ChainID, chainID), | ||
| )) | ||
| defer func(err error) { | ||
| metrics.EndSpanWithErr(span, err) | ||
| }(err) | ||
|
|
||
| if log.Topics[0] != messagetransmitter.MessageReceivedTopic { | ||
| logger.Warnf("unknown event on MessageTransmitter: %s", log.Topics[0]) | ||
| return nil | ||
| } | ||
|
|
||
| err = c.handleMessageReceived(ctx, log, chainID, ethClient) | ||
| if err != nil { | ||
| return fmt.Errorf("could not handle MessageReceived: %w", err) | ||
| } | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("could not listen for MessageReceived events: %w", err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Similar to listenDepositForBurn, the method listenMessageReceived should have its event filtering logic verified for robustness. Additionally, consider abstracting common logic between event listeners to reduce duplication.
| func (s *CCTPRelayerSuite) TestHandleCircleRequestSent() { | ||
| // setup | ||
| originChain := s.testBackends[0] | ||
| destChain := s.testBackends[1] | ||
| _, originSynapseCCTP := s.deployManager.GetSynapseCCTP(s.GetTestContext(), originChain) | ||
| _, originMockUsdc := s.deployManager.GetMockMintBurnTokenType(s.GetTestContext(), originChain) | ||
|
|
||
| // create a new relayer | ||
| mockAPI := attestation.NewMockCircleAPI() | ||
| omniRPCClient := omniClient.NewOmnirpcClient(s.testOmnirpc, s.metricsHandler, omniClient.WithCaptureReqRes()) | ||
| relay, err := relayer.NewCCTPRelayer(s.GetTestContext(), s.GetTestConfig(), s.testStore, s.GetTestScribe(), omniRPCClient, s.metricsHandler, mockAPI) | ||
| s.Nil(err) | ||
|
|
||
| // mint token | ||
| opts := originChain.GetTxContext(s.GetTestContext(), nil) | ||
| amount := big.NewInt(1000000000000000000) | ||
| tx, err := originMockUsdc.MintPublic(opts.TransactOpts, opts.From, amount) | ||
| s.Nil(err) | ||
| originChain.WaitForConfirmation(s.GetTestContext(), tx) | ||
|
|
||
| // approve token | ||
| tx, err = originMockUsdc.Approve(opts.TransactOpts, originSynapseCCTP.Address(), amount) | ||
| s.Nil(err) | ||
| originChain.WaitForConfirmation(s.GetTestContext(), tx) | ||
|
|
||
| // send token | ||
| tx, err = originSynapseCCTP.SendCircleToken(opts.TransactOpts, opts.From, big.NewInt(int64(destChain.GetChainID())), originMockUsdc.Address(), amount, 0, []byte{}) | ||
| s.Nil(err) | ||
| originChain.WaitForConfirmation(s.GetTestContext(), tx) | ||
|
|
||
| // handle send request | ||
| msg, err := relay.FetchAndProcessSentEvent(s.GetTestContext(), tx.Hash(), uint32(originChain.GetChainID())) | ||
| s.Nil(err) | ||
| s.Equal(msg.OriginTxHash, tx.Hash().String()) | ||
| s.Equal(msg.State, relayTypes.Pending) | ||
|
|
||
| // verify that the request is stored in the db | ||
| var storedMsg relayTypes.Message | ||
| err = s.testStore.DB().Where("origin_tx_hash = ?", msg.OriginTxHash).First(&storedMsg).Error | ||
| s.Nil(err) | ||
| s.Equal(*msg, storedMsg) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The detailed review of the TestHandleCircleRequestSent function confirms that it primarily covers the happy path scenario, focusing on the successful execution of operations. There is no explicit coverage of edge cases or error paths, such as failures in token minting, approval, sending, or issues with storing the request in the database. This observation suggests that while the test is well-structured for ideal conditions, it lacks explicit tests for less than ideal or unexpected conditions, which could improve its robustness by ensuring that all edge cases and error paths are covered.
Analysis chain
The test TestHandleCircleRequestSent is well-structured and covers the process of handling a circle request sent event. Ensure that all edge cases and error paths are covered in the test to improve its robustness.
Given the limited information provided by the initial script, a more detailed examination of the TestHandleCircleRequestSent function and any related code is necessary to accurately assess the coverage of edge cases. To proceed, I will generate a new script to extract the content of this function for review.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify coverage of edge cases in TestHandleCircleRequestSent
rg "TestHandleCircleRequestSent" --type go
Length of output: 147
Script:
#!/bin/bash
# Extract the content of TestHandleCircleRequestSent for detailed review
rg "TestHandleCircleRequestSent" -A 50 --type go services/cctp-relayer/relayer/synapse_test.go
Length of output: 2218
| func (s *CCTPRelayerSuite) TestBridgeSynapseCCTP() { | ||
| // create a new relayer | ||
| mockAPI := attestation.NewMockCircleAPI() | ||
| omniRPCClient := omniClient.NewOmnirpcClient(s.testOmnirpc, s.metricsHandler, omniClient.WithCaptureReqRes()) | ||
| relay, err := relayer.NewCCTPRelayer(s.GetTestContext(), s.GetTestConfig(), s.testStore, s.GetTestScribe(), omniRPCClient, s.metricsHandler, mockAPI) | ||
| s.Nil(err) | ||
|
|
||
| // start relayer | ||
| ctx, cancel := context.WithCancel(s.GetTestContext()) | ||
| defer cancel() | ||
| //nolint:errcheck | ||
| go relay.Run(ctx) | ||
|
|
||
| // mint some USDC on send chain | ||
| originChain := s.testBackends[0] | ||
| destChain := s.testBackends[1] | ||
| _, originMockUsdc := s.deployManager.GetMockMintBurnTokenType(s.GetTestContext(), originChain) | ||
| originChainID, err := originChain.ChainID(s.GetTestContext()) | ||
| s.Nil(err) | ||
| bridgeAmount := big.NewInt(1000000000) // 1000 USDC | ||
| opts := originChain.GetTxContext(s.GetTestContext(), nil) | ||
| tx, err := originMockUsdc.MintPublic(opts.TransactOpts, opts.From, bridgeAmount) | ||
| s.Nil(err) | ||
| originChain.WaitForConfirmation(s.GetTestContext(), tx) | ||
|
|
||
| // approve USDC for spending | ||
| _, originSynapseCCTP := s.deployManager.GetSynapseCCTP(s.GetTestContext(), originChain) | ||
| tx, err = originMockUsdc.Approve(opts.TransactOpts, originSynapseCCTP.Address(), bridgeAmount) | ||
| s.Nil(err) | ||
| originChain.WaitForConfirmation(s.GetTestContext(), tx) | ||
|
|
||
| // send USDC from originChain | ||
| destChainID, err := destChain.ChainID(s.GetTestContext()) | ||
| s.Nil(err) | ||
| tx, err = originSynapseCCTP.SendCircleToken(opts.TransactOpts, opts.From, destChainID, originMockUsdc.Address(), bridgeAmount, 0, []byte{}) | ||
| s.Nil(err) | ||
| originChain.WaitForConfirmation(s.GetTestContext(), tx) | ||
|
|
||
| // TODO: figure out why log is not streamed properly by relayer. | ||
| // for now, inject the log manually | ||
| receipt, err := originChain.TransactionReceipt(s.GetTestContext(), tx.Hash()) | ||
| s.Nil(err) | ||
| var sentLog *types.Log | ||
| for _, log := range receipt.Logs { | ||
| if log.Topics[0] == cctp.CircleRequestSentTopic { | ||
| sentLog = log | ||
| break | ||
| } | ||
| } | ||
| s.Require().NotNil(sentLog) | ||
| _, err = relay.HandleLog(s.GetTestContext(), sentLog, uint32(originChainID.Int64())) | ||
| s.Require().Nil(err) | ||
|
|
||
| // verify that the confirmed request is stored in the backend | ||
| s.Eventually(func() bool { | ||
| var storedMsg relayTypes.Message | ||
| // TODO: shuld make this check for completion | ||
| err = s.testStore.DB().Where("state = ?", relayTypes.Submitted).Last(&storedMsg).Error | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return storedMsg.OriginTxHash == tx.Hash().String() | ||
| }) | ||
|
|
||
| // TODO: verify USDC is credited on recv chain | ||
| // _, recvMockUsdc := c.deployManager.GetMockMintBurnTokenType(c.GetTestContext(), destChain) | ||
| // c.Nil(err) | ||
| // expectedBalance := bridgeAmount | ||
| // c.Eventually(func() bool { | ||
| // balance, err := recvMockUsdc.BalanceOf(nil, opts.From) | ||
| // c.Nil(err) | ||
| // return c.Equal(expectedBalance, balance) | ||
| // }) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The review comment regarding the need to address TODO comments and consider testing failure scenarios and edge cases in the TestBridgeSynapseCCTP function remains valid. The TODO comments regarding log streaming and manual log injection, as well as verifying USDC is credited on the receiving chain, are still present in the code. Additionally, there is no explicit addition of failure scenario tests in the provided function. Therefore, these aspects should be addressed to ensure the test suite is comprehensive and accurately reflects production behavior.
Analysis chain
The TestBridgeSynapseCCTP test simulates the bridging process in the Synapse CCTP context. The TODO comment regarding log streaming and manual log injection should be addressed to ensure the test accurately reflects the production behavior. Additionally, consider testing failure scenarios and edge cases for a more comprehensive test suite.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for resolution of TODOs and addition of failure scenario tests in TestBridgeSynapseCCTP
rg "TestBridgeSynapseCCTP" --type go
Length of output: 135
Script:
#!/bin/bash
# Extract the content of TestBridgeSynapseCCTP function for detailed examination
rg "func \(s \*CCTPRelayerSuite\) TestBridgeSynapseCCTP\(\) \{" -A 100 --type go services/cctp-relayer/relayer/synapse_test.go
Length of output: 2977
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- packages/synapse-interface/utils/hooks/useStipEligibility.ts (1 hunks)
Additional comments: 1
packages/synapse-interface/utils/hooks/useStipEligibility.ts (1)
- 239-242: The logic update to include
token.routeSymbol === 'ETH'as a condition for RFQ eligibility is clear and aligns with the described objectives. However, it would be beneficial for maintainability to add a comment explaining why'ETH'tokens are considered eligible alongside'USD'swapable types. This can help future developers understand the context and reasoning behind this decision.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- packages/synapse-interface/CHANGELOG.md (1 hunks)
- packages/synapse-interface/package.json (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/synapse-interface/CHANGELOG.md
- packages/synapse-interface/package.json
… on send (#2354) * Fixes decimal input with no leading zeros, wallet cancel on send
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yaml
Files selected for processing (4)
- packages/widget/CHANGELOG.md (1 hunks)
- packages/widget/package.json (2 hunks)
- packages/widget/src/state/slices/bridgeTransaction/hooks.ts (1 hunks)
- packages/widget/src/utils/isOnlyZeroes.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/widget/package.json
Additional comments: 3
packages/widget/src/utils/isOnlyZeroes.ts (1)
- 7-11: The updated
isOnlyZeroesfunction with the simplified regex pattern correctly matches the specified string patterns. The implementation is concise and adheres to best practices.packages/widget/src/state/slices/bridgeTransaction/hooks.ts (1)
- 45-84: The restructuring of the
executeBridgeTxnfunction improves clarity and maintains the core functionality effectively. The use of async/await and conditional logic is well implemented.packages/widget/CHANGELOG.md (1)
- 3-27: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [6-371]
The documented version updates in
CHANGELOG.mdare clear and adhere to conventional commits guidelines. The entries provide a traceable history of changes, beneficial for maintainers and users alike.
| All notable changes to this project will be documented in this file. | ||
| See [Conventional Commits](https://conventionalcommits.org) for commit guidelines. | ||
|
|
||
| ## [0.1.10](https://github.com/synapsecns/sanguine/compare/@synapsecns/widget@0.1.9...@synapsecns/widget@0.1.10) (2024-03-22) | ||
|
|
||
|
|
||
| ### Bug Fixes | ||
|
|
||
| * **widget:** Fixes decimal input with no leading zeros, wallet cancel on send ([#2354](https://github.com/synapsecns/sanguine/issues/2354)) ([8c9f382](https://github.com/synapsecns/sanguine/commit/8c9f38222efcb4a911b0dd0e3c7db9914bf2a544)) | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| ## [0.1.9](https://github.com/synapsecns/sanguine/compare/@synapsecns/widget@0.1.8...@synapsecns/widget@0.1.9) (2024-03-21) | ||
|
|
||
| **Note:** Version bump only for package @synapsecns/widget | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| ## [0.1.8](https://github.com/synapsecns/sanguine/compare/@synapsecns/widget@0.1.7...@synapsecns/widget@0.1.8) (2024-03-18) | ||
|
|
||
| **Note:** Version bump only for package @synapsecns/widget |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [33-33]
In the bug fix entry for version 0.1.7, consider capitalizing "urls" to "URLs" for consistency with common technical terminology.
- updated image urls
+ updated image URLsCo-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
* Add destinationAddress prop to pendingBridgeTransaction * Add destinationAddress prop to _TransactionDetails * Use destinationAddress to determine dest explorer link * Rm log * Fix destination address in historical tx * Improve address comparison * Destination address hover
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (11)
- .github/workflows/go.yml (3 hunks)
- .github/workflows/goreleaser-actions.yml (1 hunks)
- .github/workflows/scan.yml (1 hunks)
- packages/synapse-interface/components/Activity/Activity.tsx (1 hunks)
- packages/synapse-interface/components/Portfolio/Transaction/components/Completed.tsx (2 hunks)
- packages/synapse-interface/components/_Transaction/_Transaction.tsx (3 hunks)
- packages/synapse-interface/components/_Transaction/_Transactions.tsx (1 hunks)
- packages/synapse-interface/pages/state-managed-bridge/index.tsx (5 hunks)
- packages/synapse-interface/slices/_transactions/reducer.ts (1 hunks)
- packages/synapse-interface/slices/transactions/actions.ts (2 hunks)
- packages/synapse-interface/utils/hooks/use_TransactionsListener.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/go.yml
- packages/synapse-interface/pages/state-managed-bridge/index.tsx
Additional comments: 11
.github/workflows/scan.yml (1)
- 15-15: The conditional check
if: github.repository_owner == 'synapsecns'ensures that the SonarCloud job runs only for the repository owned by 'synapsecns'. This is a good practice for workflows that should be restricted to specific repositories or when actions incur costs or have security implications.packages/synapse-interface/slices/transactions/actions.ts (1)
- 19-19: Adding the
destinationAddressproperty of typeAddress | nullto thePendingBridgeTransactioninterface is a good practice. It allows for the explicit handling of cases where a destination address might not be available. This addition enhances the type safety and clarity of the code.packages/synapse-interface/components/Portfolio/Transaction/components/Completed.tsx (2)
- 5-5: Importing
getValidAddressfrom theisValidAddressmodule and using it for address comparison is a good practice. It abstracts the logic for address validation and normalization, making the code cleaner and more maintainable.- 33-40: Adjusting CSS classes based on the
isTodaycondition to change text color and cursor style is a clear and effective way to provide visual feedback to the user. This conditional styling enhances the user experience by making it easier to distinguish between transactions completed today and those completed on other days.packages/synapse-interface/utils/hooks/use_TransactionsListener.ts (1)
- 45-45: Adding the
destinationAddressfield to theaddTransactiondispatch within theuse_TransactionsListenerhook is a necessary update to ensure that the newdestinationAddressproperty is correctly handled throughout the application. This change ensures that the application's state remains consistent with the updated data model.packages/synapse-interface/components/_Transaction/_Transactions.tsx (1)
- 49-49: Passing the
destinationAddressprop to the_Transactioncomponent is a necessary update following the addition of thedestinationAddressproperty in the transaction data model. This ensures that the component receives all the necessary data to accurately represent a transaction.packages/synapse-interface/slices/_transactions/reducer.ts (1)
- 9-9: The addition of the
destinationAddressproperty of typeAddress | nullto the_TransactionDetailsinterface aligns with the updates made across the application to handle destination addresses explicitly. This change enhances the data model's expressiveness and supports the new functionality related to transaction handling.packages/synapse-interface/components/_Transaction/_Transaction.tsx (2)
- 23-23: Adding the
destinationAddressproperty to the_TransactionPropsinterface is consistent with the updates made throughout the application to handle destination addresses. This ensures that the_Transactioncomponent can accurately represent transactions with all necessary details.- 67-67: Using the
destinationAddress(if available) or falling back to theconnectedAddressfor generating the destination explorer link is a thoughtful approach. It ensures that the link is always relevant and accurate, enhancing the user experience by providing a direct link to the relevant transaction or address information.packages/synapse-interface/components/Activity/Activity.tsx (1)
- 105-105: Updating the
destinationAddressproperty in therenderTransactionfunction to usetransaction?.toInfo?.addressinstead oftransaction?.fromInfo?.addressis a crucial fix. This change ensures that the destination address accurately reflects the transaction's recipient, aligning with the intended data model and improving the accuracy of transaction representation in the UI..github/workflows/goreleaser-actions.yml (1)
- 103-103: The update to use
secrets.WORKFLOW_PAT || secrets.GITHUB_TOKENfor thegithub_tokenvalue is a good practice, as it provides flexibility in token usage. This change allows the workflow to use a more privileged token (WORKFLOW_PAT) if available, falling back to the defaultGITHUB_TOKENotherwise. This can be particularly useful for workflows that require permissions not granted to theGITHUB_TOKENby default.However, ensure that the
WORKFLOW_PATsecret is properly managed and has only the necessary permissions to minimize security risks.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- packages/synapse-interface/CHANGELOG.md (1 hunks)
- packages/synapse-interface/package.json (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/synapse-interface/CHANGELOG.md
- packages/synapse-interface/package.json
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (4)
- packages/synapse-interface/components/Maintenance/AnnouncementBanner.tsx (1 hunks)
- packages/synapse-interface/components/StateManagedSwap/SwapExchangeRateInfo.tsx (1 hunks)
- packages/synapse-interface/pages/index.tsx (2 hunks)
- packages/synapse-interface/pages/swap/index.tsx (2 hunks)
Additional comments: 6
packages/synapse-interface/pages/index.tsx (2)
- 6-7: The addition of
MaintenanceBannerandAnnouncementBannercomponents is noted. Ensure that these components are properly tested, especially in edge cases where the data might be missing or invalid.- 20-31: The
AnnouncementBannercomponent is correctly implemented with all required props. However, consider validating thebannerId,bannerContents,startDate, andendDateprops to ensure they meet expected formats and ranges. This can prevent runtime errors and improve the robustness of the component.packages/synapse-interface/components/StateManagedSwap/SwapExchangeRateInfo.tsx (1)
- 55-55: The use of optional chaining (
?.) for accessingtoToken?.symbolis a good practice for handling cases wheretoTokenmight benullorundefined. This improves the robustness of the component by preventing potential runtime errors.packages/synapse-interface/components/Maintenance/AnnouncementBanner.tsx (1)
- 57-60: The change from
justify-betweentojustify-centerin thebanner-defaultelement's CSS classes likely affects the alignment of the banner's content. Ensure this change aligns with the desired user interface design and that it has been visually tested across different screen sizes for consistency.packages/synapse-interface/pages/swap/index.tsx (2)
- 50-50: The import of
AnnouncementBanneris correctly added. Ensure that the component is used consistently across the application and that its implementation here aligns with its intended use cases.- 363-368: The addition of the
AnnouncementBannercomponent within theStateManagedSwapcomponent is correctly implemented with specific banner details and dates. Similar to the previous file, consider validating the props to ensure they meet expected formats and ranges. This can prevent runtime errors and improve the robustness of the component.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- packages/synapse-interface/CHANGELOG.md (1 hunks)
- packages/synapse-interface/package.json (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/synapse-interface/CHANGELOG.md
- packages/synapse-interface/package.json
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fe-release #2341 +/- ##
====================================================
+ Coverage 47.90622% 49.20624% +1.30002%
====================================================
Files 366 396 +30
Lines 27128 28661 +1533
Branches 132 311 +179
====================================================
+ Hits 12996 14103 +1107
- Misses 12792 13146 +354
- Partials 1340 1412 +72
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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
New Features
Updates
@synapsecns/contracts-core,@synapsecns/rest-api,@synapsecns/sdk-router, and@synapsecns/synapse-interface.generate:docsscript inpackage.json.Bug Fixes
Chores
yarn.lockto remove and update several dependencies.d4f79512d59732d69609dadb77316868531aaad9: synapse-interface preview link