[DNM] test #3238 #3240
[DNM] test #3238 #3240
Conversation
* add fe-release label * fix label --------- Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
WalkthroughThe pull request modifies the workflow file Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/labeler.yml (1)
35-40: LGTM! Consider adding a comment for clarity.The new step correctly implements the addition of the 'fe-release' label for pull requests targeting the 'fe-release' branch. This aligns well with the PR objective and enhances the automated labeling process.
For improved readability, consider adding a brief comment explaining the purpose of this step. For example:
# Add 'fe-release' label for pull requests targeting the fe-release branch - name: Add 'fe-release' label if: github.event.pull_request.base.ref == 'fe-release' uses: actions-ecosystem/action-add-labels@v1 with: github_token: ${{ secrets.GITHUB_TOKEN }} labels: 'fe-release'
* fixing terminology * rfq-indexer-docs * Update RFQ Indexer API Readme (#3221) * adding dispute events to ponder and linting * numbering typo Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * small fixes * sync with master * API changes for disputes * fixing pending txs missing proofs * proof/dispute active & other tweaks/suggs * dispute col rename originChain prefix --------- Co-authored-by: defi-moses <jakedinero@protonmail.com> Co-authored-by: Moses <103143573+Defi-Moses@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Deploying sanguine-fe with
|
| Latest commit: |
4b49879
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e84a2524.sanguine-fe.pages.dev |
| Branch Preview URL: | https://master.sanguine-fe.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (11)
packages/rfq-indexer/api/src/controllers/disputesController.ts (2)
7-8: Consider enhancing error logging for better debugging.The function declaration and overall error handling structure are good. However, to improve debugging and error tracing, consider logging more details about the error.
Here's a suggested improvement for error logging:
- console.error('Error fetching active disputes:', error) + console.error('Error fetching active disputes:', error.message, '\nStack:', error.stack)This change will provide more context about the error, including the error message and stack trace, which can be crucial for debugging in production environments.
Also applies to: 23-27
15-16: Add type annotation to improve type safety.The query execution and result processing look good. However, adding a type annotation to the
disputesvariable would improve type safety and code readability.Consider modifying the code as follows:
const results = await query.execute() const disputes: Dispute[] = nest_results(results)This assumes there's a
Disputeinterface or type defined elsewhere in your project. If not, you should create one to represent the structure of a dispute object.packages/rfq-indexer/api/src/queries/disputesQueries.ts (2)
4-19: LGTM: Query construction is well-structured.The query construction is logically sound and well-implemented. The left join effectively filters out stale or invalid disputes, and the field selection is clear with consistent aliasing.
Consider adding a brief comment explaining the purpose of the left join and field selection for improved code documentation.
1-26: Overall implementation is well-designed and efficient.The
qDisputesfunction provides a flexible and efficient way to query dispute events. The implementation is logically sound, with clear query construction and appropriate use of joins and filters.Consider adding a brief function-level JSDoc comment to describe the purpose of
qDisputes, its parameters, and return value. This would enhance code documentation and improve maintainability. For example:/** * Constructs a query to fetch dispute events from the database. * @param {Object} options - Query options * @param {boolean} [options.activeOnly=false] - If true, only active disputes are returned * @returns {SelectQueryBuilder} A query builder object for further chaining or execution */ export const qDisputes = ({ activeOnly }: { activeOnly: boolean } = { activeOnly: false }) => { // ... existing implementation ... }packages/rfq-indexer/api/README.md (1)
10-32: Improved structure and clarity of API documentation.The restructuring of the API calls section enhances readability and consistency. The new introductory text directing users to Swagger documentation is helpful.
Consider adding language specifications to the fenced code blocks for improved syntax highlighting. For example:
- ``` + ```bash curl http://localhost:3001/api/pending-transactions/missing-relay ```🧰 Tools
🪛 Markdownlint
17-17: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
24-24: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
31-31: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
packages/rfq-indexer/api/src/routes/disputesRoute.ts (1)
7-62: Great job on the comprehensive OpenAPI documentation!The documentation is thorough and follows OpenAPI standards, providing clear information about the endpoint's functionality and possible responses. The detailed schema for the successful response is particularly helpful for API consumers.
One minor suggestion:
Consider updating the description for the 404 response to match the endpoint's functionality. Currently, it says "No disputes found", but it might be more accurate to say "No active disputes found" to align with the endpoint's summary ("Get all active disputes").
- description: No disputes found + description: No active disputes foundpackages/rfq-indexer/api/src/graphql/queries/queries.graphql (2)
122-122: Approved: New query added, but consider improvementsThe new
pendingTransactionsMissingRelayExceedDeadlinequery is a valuable addition. However, consider the following suggestions to enhance its usability and clarity:
- Add a comment or description to explain the specific use case and criteria for "exceeding deadline".
- Consider adding optional arguments (e.g.,
deadline: Int) to make the query more flexible and reusable.Example with improvements:
""" Retrieves pending transactions missing a relay that have exceeded a specified deadline. If no deadline is provided, a default value is used. """ pendingTransactionsMissingRelayExceedDeadline(deadline: Int): [PendingTransactionMissingRelay!]!
129-129: Approved: NewdisputedRelaysquery added, consider enhancementsThe new
disputedRelaysquery is a good addition that complements the newly introducedDisputedRelaytype. To improve its usability and clarity, consider the following suggestions:
- Add a comment or description to explain what constitutes a disputed relay and how they are determined.
- Consider adding optional arguments (e.g.,
limit: Int,offset: Int) for pagination, orfilterfor more targeted queries.Example with improvements:
""" Retrieves a list of disputed relays. A relay is considered disputed when [explain criteria here]. """ disputedRelays(limit: Int, offset: Int, filter: DisputedRelayFilter): [DisputedRelay!]!Don't forget to define the
DisputedRelayFilterinput type if you choose to add filtering capabilities.packages/rfq-indexer/api/src/types/index.ts (1)
84-92: LGTM! Consider aligning property names with other interfaces.The new
BridgeProofDisputedEventsinterface looks good and follows the structure of other event interfaces in the file. It captures essential information for a disputed event.For consistency with other interfaces, consider renaming
chainIdtooriginChainIdandchaintooriginChain.export interface BridgeProofDisputedEvents { id: ColumnType<string> transactionId: ColumnType<string> blockNumber: ColumnType<bigint> blockTimestamp: ColumnType<number> transactionHash: ColumnType<string> - chainId: ColumnType<number> - chain: ColumnType<string> + originChainId: ColumnType<number> + originChain: ColumnType<string> }packages/rfq-indexer/api/src/routes/pendingTransactionsRoute.ts (1)
150-190: LGTM: New route added correctly, with a minor suggestionThe new route for transactions that exceed the deadline has been added correctly. It follows the existing pattern for route definitions and includes comprehensive OpenAPI annotations. The route handler correctly uses the newly imported controller.
However, there's a minor typo in the 404 response description:
- * description: No pending transactionst that exceed the deadline found + * description: No pending transactions that exceed the deadline foundpackages/rfq-indexer/api/src/queries/proofsQueries.ts (1)
9-10: Ensure database indexes exist on joined and filtered columnsFor optimal query performance, please verify that indexes exist on the
transactionIdandblockTimestampcolumns in bothBridgeProofProvidedEventsandBridgeProofDisputedEventstables. This will improve the efficiency of the join and filter operations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (18)
- packages/rfq-indexer/api/CHANGELOG.md (1 hunks)
- packages/rfq-indexer/api/README.md (1 hunks)
- packages/rfq-indexer/api/package.json (1 hunks)
- packages/rfq-indexer/api/src/controllers/conflictingProofsController.ts (2 hunks)
- packages/rfq-indexer/api/src/controllers/disputesController.ts (1 hunks)
- packages/rfq-indexer/api/src/controllers/pendingTransactionsController.ts (4 hunks)
- packages/rfq-indexer/api/src/controllers/transactionIdController.ts (2 hunks)
- packages/rfq-indexer/api/src/db/index.ts (2 hunks)
- packages/rfq-indexer/api/src/graphql/queries/queries.graphql (1 hunks)
- packages/rfq-indexer/api/src/graphql/resolvers.ts (3 hunks)
- packages/rfq-indexer/api/src/graphql/types/events.graphql (5 hunks)
- packages/rfq-indexer/api/src/queries/disputesQueries.ts (1 hunks)
- packages/rfq-indexer/api/src/queries/index.ts (1 hunks)
- packages/rfq-indexer/api/src/queries/proofsQueries.ts (1 hunks)
- packages/rfq-indexer/api/src/routes/disputesRoute.ts (1 hunks)
- packages/rfq-indexer/api/src/routes/index.ts (2 hunks)
- packages/rfq-indexer/api/src/routes/pendingTransactionsRoute.ts (2 hunks)
- packages/rfq-indexer/api/src/types/index.ts (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/rfq-indexer/api/CHANGELOG.md
- packages/rfq-indexer/api/package.json
🧰 Additional context used
🪛 Markdownlint
packages/rfq-indexer/api/README.md
17-17: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
24-24: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
31-31: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
🔇 Additional comments (31)
packages/rfq-indexer/api/src/queries/index.ts (1)
3-3: LGTM! The new export is consistent with the file's structure.The addition of the
qDisputesexport from./disputesQueriesaligns well with the existing pattern in this index file. This change appears to introduce a new query related to disputes, which could be part of a larger feature addition.Let's verify the existence of the
disputesQueriesfile and theqDisputesexport:packages/rfq-indexer/api/src/routes/index.ts (2)
8-8: LGTM: Import statement for disputesRouteThe import statement for
disputesRouteis correctly added and follows the existing pattern in the file. It maintains consistency with other route imports in terms of naming convention and relative path usage.
17-17: LGTM: Route registration for disputesThe route registration for
/disputesis correctly added and follows the existing pattern in the file. It maintains consistency with other route registrations and adheres to RESTful naming conventions.To ensure the completeness of this change, please verify the implementation of
disputesRoute. Run the following script to check its contents:packages/rfq-indexer/api/src/db/index.ts (2)
25-25: LGTM! Verify the database schema.The addition of
BridgeProofDisputedEventsto theDatabaseinterface is consistent with the existing pattern and corresponds to the new import. This expands the database interface to include the new event type.To ensure the database schema has been updated accordingly, run the following script:
✅ Verification successful
Database schema includes BridgeProofDisputedEvents as expected.
The search results confirm that
BridgeProofDisputedEventsis properly integrated into the database schema across relevant migration and TypeScript files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the database schema includes the new event type # Test: Search for database migration files that might include the new event type fd -e sql -e ts 'migration' | xargs rg 'BridgeProofDisputedEvents'Length of output: 4021
10-10: LGTM! Verify the imported type.The addition of
BridgeProofDisputedEventsto the import statement is consistent with the existing pattern. This suggests a new event type is being introduced to the system.To ensure the imported type is correctly defined, run the following script:
✅ Verification successful
Verify that
BridgeProofDisputedEventsis defined as an interface.The initial script searched for a type definition, but an interface was found instead. To confirm the definition, please run the following script:
Verification Successful:
BridgeProofDisputedEventsis correctly defined as an interface.The
BridgeProofDisputedEventsinterface is properly defined inpackages/rfq-indexer/api/src/types/index.tsand is consistently utilized across the codebase without any issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of BridgeProofDisputedEvents type # Test: Search for the type definition in the types file ast-grep --lang typescript --pattern 'export type BridgeProofDisputedEvents = $_'Length of output: 4021
Script:
#!/bin/bash # Description: Verify the definition of BridgeProofDisputedEvents as an interface # Test: Search for the interface definition in the types file ast-grep --lang typescript --pattern 'export interface BridgeProofDisputedEvents {'Length of output: 783
packages/rfq-indexer/api/src/controllers/disputesController.ts (1)
1-5: Imports look good and follow best practices.The necessary dependencies are imported correctly, including Express types, database connection, queries, and utility functions. The imports are concise and follow the best practice of destructuring specific imports.
packages/rfq-indexer/api/src/queries/disputesQueries.ts (3)
1-3: LGTM: Function signature and import look good.The import statement and function signature are well-defined. The optional parameter
activeOnlywith a default value offalseprovides flexibility in query execution.
21-23: LGTM: Active disputes filter is correctly implemented.The additional where clause for active disputes is logically sound. It effectively filters for disputes that don't have a corresponding entry in the
BridgeProofProvidedEventstable whenactiveOnlyis set to true.
25-26: LGTM: Query return is appropriate.Returning the constructed query object is a good practice. It allows for further modifications or immediate execution by the caller, providing flexibility in query usage.
packages/rfq-indexer/api/src/controllers/conflictingProofsController.ts (3)
44-44: LGTM: Updated response message.The response message has been appropriately updated to reflect that no active conflicting proofs were found. This change is consistent with the new focus on active proofs and provides clear feedback to the client.
47-47: LGTM: Updated error logging message.The error logging message has been appropriately updated to reflect that the error occurs while fetching active conflicting proofs. This change is consistent with the new focus on active proofs and provides clear context for debugging.
16-16: LGTM: Filtering for active proofs.The change to filter only active proofs is consistent with the updated functionality. This should improve the relevance of the data returned by the controller.
Let's verify if the
qProofsfunction supports theactiveOnlyparameter:packages/rfq-indexer/api/README.md (2)
38-42: Valuable addition of environment variables documentation.The new "Env Vars" section provides crucial information for local testing and database connection. This addition enhances the README's usefulness for developers setting up the project.
Line range hint
1-42: Overall improvement in README structure and clarity.The changes to this README file significantly enhance its usability:
- The API Calls section is now more consistently formatted and easier to read.
- The addition of the Env Vars section provides crucial setup information.
- The overall structure is more logical and user-friendly.
These improvements will help developers better understand and use the RFQ Indexer API.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...## Important Scripts -yarn dev:local: Runs the API in development mode using ...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint
17-17: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
24-24: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
31-31: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
packages/rfq-indexer/api/src/graphql/types/events.graphql (3)
Line range hint
1-1: LGTM: Addition of BigInt scalar typeThe introduction of the
BigIntscalar type is appropriate for handling large integer values commonly found in blockchain data. Its consistent usage across various event types (e.g.,blockNumber,originAmount,destAmount) ensures proper representation of numerical data.
Line range hint
1-91: LGTM: Overall structure and consistencyThe addition of
BridgeProofDisputedEventmaintains the file's overall structure and naming conventions. The use of theBigIntscalar is consistent across all relevant fields, and the file structure remains clear and easy to understand. The new event type integrates well with the existing types, preserving the file's coherence.
83-91: Verify completeness of BridgeProofDisputedEvent fieldsThe new
BridgeProofDisputedEventtype has been added successfully and follows the naming convention of other event types. It includes common fields such asid,transactionId,blockNumber, etc.However, please verify if additional fields are needed for this event type. Other event types in this file often include fields like
relayer,to,token, andamount. If these are relevant for a proof dispute event, consider adding them for consistency.To help verify the completeness of the
BridgeProofDisputedEventfields, please run the following script:This script will help us understand how
BridgeProofDisputedEventis used in the codebase and if there are any proof dispute-related operations that might require additional fields in the event type.packages/rfq-indexer/api/src/routes/disputesRoute.ts (2)
1-5: LGTM: Imports and router setup are correct.The imports and router setup follow best practices. The use of a separate controller file (
disputesController) promotes good code organization and separation of concerns.
65-65: LGTM: Router export is correct.The default export of the router is the standard way to make it available for use in other parts of the Express application.
packages/rfq-indexer/api/src/graphql/queries/queries.graphql (1)
115-118: LGTM: New typeDisputedRelayaddedThe new
DisputedRelaytype is well-structured and consistent with the existing schema. It appropriately uses non-nullable fields for required information and references existing types.packages/rfq-indexer/api/src/types/index.ts (1)
100-100: LGTM! EventType updated correctly.The addition of 'DISPUTE' to the
EventTypeunion is consistent with the newBridgeProofDisputedEventsinterface. This update allows the system to recognize and handle dispute events properly.packages/rfq-indexer/api/src/routes/pendingTransactionsRoute.ts (2)
6-7: LGTM: Import statement updated correctlyThe import statement has been updated to include the new controller
pendingTransactionsMissingRelayExceedDeadlineController. This change is consistent with the new route being added and follows the existing naming convention.
Line range hint
1-191: Summary: Successful implementation of new 'exceed-deadline' routeThe changes in this file successfully implement a new route for handling transactions that exceed a specified deadline. The modifications include:
- Adding a new import for the
pendingTransactionsMissingRelayExceedDeadlineController.- Implementing a new GET route at '/exceed-deadline' with appropriate OpenAPI annotations.
These changes enhance the API's functionality by providing a way to retrieve transactions that have exceeded their deadline, which aligns with the PR objectives. The implementation is consistent with the existing code structure and follows best practices.
packages/rfq-indexer/api/src/queries/proofsQueries.ts (1)
20-22: Conditional filtering based onactiveOnlyis correctly implementedThe logic for filtering proofs when
activeOnlyistrueis implemented correctly and functions as expected.packages/rfq-indexer/api/src/controllers/transactionIdController.ts (3)
4-4: ImportingqDisputesis AppropriateThe addition of
qDisputesto the import statement correctly incorporates dispute-related queries needed for the new functionality.
16-17: Verify Inconsistency in Active Status Filters for Proofs and DisputesThe query includes all proofs by setting
activeOnly: false, thus displaying both active and inactive proofs. In contrast, it includes only active disputes by settingactiveOnly: true. Please verify if this is intentional, as displaying inactive proofs but excluding inactive disputes might lead to inconsistent data representation.
26-26: Confirm Join Condition with thedisputesTableThe addition of the left join with the
disputestable using.leftJoin('disputes', 'transactionId_deposit', 'transactionId_dispute')should be verified to ensure that it correctly associates disputes with the corresponding deposits. Please confirm that the join keys are accurate and that this join will function as expected.packages/rfq-indexer/api/src/controllers/pendingTransactionsController.ts (4)
4-11: Imports are updated with necessary query modules.The additional imports (
qDisputes, etc.) are appropriate and necessary for the controllers.
24-24: Ensure the 'activeOnly' parameter is handled correctly inqProofs.Adding
{ activeOnly: true }toqProofsmodifies its behavior to include only active proofs. Verify that theqProofsfunction supports this parameter and that this change aligns with the intended functionality.
65-65: Ensure the 'activeOnly' parameter is handled correctly inqProofs.Similar to the previous controller, confirm that the
qProofsfunction properly handles the{ activeOnly: true }parameter and behaves as expected.
122-123: Confirm that filtering deposits to the last 7 days meets business requirements.The added filter
.where('blockTimestamp_deposit', '>', sevenDaysAgo)limits results to deposits from the last seven days. Ensure this aligns with business requirements and that older pending transactions are addressed appropriately elsewhere.
| const query = db | ||
| .with('disputes', () => qDisputes({activeOnly: true})) | ||
| .selectFrom('disputes') | ||
| .selectAll() | ||
| .orderBy('blockTimestamp_dispute', 'desc') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the query by selecting only necessary columns.
The query construction looks good overall, using a CTE and proper ordering. However, selecting all columns (selectAll()) might not be the most efficient approach, especially if the client doesn't need all the data.
Consider modifying the query to select only the columns that are actually needed by the client. This can improve query performance and reduce data transfer. For example:
.selectFrom('disputes')
.select(['id', 'status', 'blockTimestamp_dispute', /* other necessary columns */])This change would require knowing which specific columns are needed for the disputes data in your application.
| if (disputes && disputes.length > 0) { | ||
| res.json(disputes) | ||
| } else { | ||
| res.status(200).json({ message: 'No active disputes found' }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the response structure consistent for both cases.
The response handling covers both cases (disputes found and not found) appropriately. However, the response structure is inconsistent between the two cases.
To improve API consistency, consider using the same structure for both cases. Here's a suggested modification:
if (disputes && disputes.length > 0) {
res.json({ disputes })
} else {
res.json({ disputes: [], message: 'No active disputes found' })
}This change ensures that the client always receives an object with a disputes property (an array, which may be empty) and optionally a message property. This consistency can make it easier for clients to handle the response.
| * message: | ||
| * type: string | ||
| */ | ||
| router.get('/', disputesController) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM: Route definition is correct and concise.
The route definition matches the OpenAPI documentation and uses the imported disputesController as the handler, which is a good practice for separation of concerns.
Consider adding error handling middleware to catch any errors thrown by the controller:
import { ErrorRequestHandler } from 'express';
const errorHandler: ErrorRequestHandler = (err, req, res, next) => {
console.error(err.stack);
res.status(500).json({ message: 'An unexpected error occurred' });
};
router.get('/', disputesController);
router.use(errorHandler);This would ensure that any unhandled errors in the controller are caught and returned as a 500 response, improving the robustness of the API.
| @@ -1,15 +1,25 @@ | |||
| import { db } from '../db' | |||
|
|
|||
| // typical fields to return for a BridgeProofProvided event when it is joined to a BridgeRequest | |||
| export const qProofs = () => { | |||
| return db | |||
| export const qProofs = ({ activeOnly}: { activeOnly: boolean} = { activeOnly: false}) => { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the function signature for better readability
The current function signature can be simplified by setting the default value directly in the destructured parameter and making the type optional. This enhances readability.
Here is a suggested change:
-export const qProofs = ({ activeOnly}: { activeOnly: boolean} = { activeOnly: false}) => {
+export const qProofs = ({ activeOnly = false }: { activeOnly?: boolean } = {}) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const qProofs = ({ activeOnly}: { activeOnly: boolean} = { activeOnly: false}) => { | |
| export const qProofs = ({ activeOnly = false }: { activeOnly?: boolean } = {}) => { |
| .leftJoin('claims', 'transactionId_deposit', 'transactionId_claim') | ||
| .leftJoin('refunds', 'transactionId_deposit', 'transactionId_refund') | ||
| .selectAll('deposits') | ||
| .selectAll('relays') | ||
| .selectAll('proofs') | ||
| .selectAll('disputes') | ||
| .selectAll('claims') |
There was a problem hiding this comment.
Specify Fields When Selecting from disputes to Prevent Sensitive Data Exposure
Using .selectAll('disputes') selects all columns from the disputes table, which may inadvertently include sensitive or unnecessary data in the API response. Consider specifying only the required fields to ensure that sensitive information is not exposed.
Apply this diff to specify the fields to select from the disputes table:
- .selectAll('disputes')
+ .selectFrom('disputes').select(['field1', 'field2', 'field3'])Replace ['field1', 'field2', 'field3'] with the specific fields that should be included in the response.
Committable suggestion was skipped due to low confidence.
| export const pendingTransactionsMissingRelayExceedDeadlineController = async ( | ||
| req: Request, | ||
| res: Response | ||
| ) => { | ||
| try { | ||
| const query = db | ||
| .with('deposits', () => qDeposits()) | ||
| .with('relays', () => qRelays()) | ||
| .with('refunds', () => qRefunds()) | ||
| .with( | ||
| 'combined', | ||
| (qb) => | ||
| qb | ||
| .selectFrom('deposits') | ||
| .selectAll('deposits') | ||
| .leftJoin('relays', 'transactionId_deposit', 'transactionId_relay') | ||
| .leftJoin( | ||
| 'refunds', | ||
| 'transactionId_deposit', | ||
| 'transactionId_refund' | ||
| ) | ||
| .where('transactionId_relay', 'is', null) // is not relayed | ||
| .where('transactionId_refund', 'is', null) // is not refunded | ||
| ) | ||
| .selectFrom('combined') | ||
| .selectAll() | ||
| .orderBy('blockTimestamp_deposit', 'desc') | ||
| .where('blockTimestamp_deposit', '<=', sevenDaysAgo) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication between controllers.
The pendingTransactionsMissingRelayController and pendingTransactionsMissingRelayExceedDeadlineController have similar structures with differences in the where clauses. Refactoring common logic into shared functions can improve maintainability and reduce redundancy.
| console.error('Error fetching pending transactions missing relay:', error) | ||
| res.status(500).json({ message: 'Internal server error' }) | ||
| } |
There was a problem hiding this comment.
Differentiate error messages for clearer debugging.
The error messages in the catch blocks are identical across controllers. To facilitate easier debugging, update the error messages to reflect the specific controller's context.
Suggested changes:
In pendingTransactionsMissingRelayExceedDeadlineController, update the error logging:
- console.error('Error fetching pending transactions missing relay:', error)
+ console.error('Error fetching pending transactions missing relay exceeding deadline:', error)And update the response message in the else block:
- .json({ message: 'No pending transactions missing relay found' })
+ .json({ message: 'No pending transactions missing relay exceeding deadline found' })Also applies to: 166-167
| // typical fields to return for a BridgeProofDisputed event when it is joined to a BridgeRequest | ||
| const qDisputes = () => { | ||
| return db | ||
| .selectFrom('BridgeProofDisputedEvents') | ||
| .select([ | ||
| 'BridgeProofDisputedEvents.transactionId as transactionId_dispute', | ||
| 'BridgeProofDisputedEvents.blockNumber as blockNumber_dispute', | ||
| 'BridgeProofDisputedEvents.blockTimestamp as blockTimestamp_dispute', | ||
| 'BridgeProofDisputedEvents.transactionHash as transactionHash_dispute', | ||
| 'BridgeProofDisputedEvents.originChainId as originChainId_dispute', | ||
| 'BridgeProofDisputedEvents.originChain as originChain_dispute', | ||
| ]) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include the relayer field in qDisputes for consistency
The qDisputes function is missing the relayer field, which is present in similar query functions like qProofs. Including the relayer field ensures consistency across queries and provides complete data for downstream processes.
Apply this diff to include the relayer field:
const qDisputes = () => {
return db
.selectFrom('BridgeProofDisputedEvents')
.select([
'BridgeProofDisputedEvents.transactionId as transactionId_dispute',
'BridgeProofDisputedEvents.blockNumber as blockNumber_dispute',
'BridgeProofDisputedEvents.blockTimestamp as blockTimestamp_dispute',
'BridgeProofDisputedEvents.transactionHash as transactionHash_dispute',
+ 'BridgeProofDisputedEvents.relayer as relayer_dispute',
'BridgeProofDisputedEvents.originChainId as originChainId_dispute',
'BridgeProofDisputedEvents.originChain as originChain_dispute',
])
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // typical fields to return for a BridgeProofDisputed event when it is joined to a BridgeRequest | |
| const qDisputes = () => { | |
| return db | |
| .selectFrom('BridgeProofDisputedEvents') | |
| .select([ | |
| 'BridgeProofDisputedEvents.transactionId as transactionId_dispute', | |
| 'BridgeProofDisputedEvents.blockNumber as blockNumber_dispute', | |
| 'BridgeProofDisputedEvents.blockTimestamp as blockTimestamp_dispute', | |
| 'BridgeProofDisputedEvents.transactionHash as transactionHash_dispute', | |
| 'BridgeProofDisputedEvents.originChainId as originChainId_dispute', | |
| 'BridgeProofDisputedEvents.originChain as originChain_dispute', | |
| ]) | |
| } | |
| // typical fields to return for a BridgeProofDisputed event when it is joined to a BridgeRequest | |
| const qDisputes = () => { | |
| return db | |
| .selectFrom('BridgeProofDisputedEvents') | |
| .select([ | |
| 'BridgeProofDisputedEvents.transactionId as transactionId_dispute', | |
| 'BridgeProofDisputedEvents.blockNumber as blockNumber_dispute', | |
| 'BridgeProofDisputedEvents.blockTimestamp as blockTimestamp_dispute', | |
| 'BridgeProofDisputedEvents.transactionHash as transactionHash_dispute', | |
| 'BridgeProofDisputedEvents.relayer as relayer_dispute', | |
| 'BridgeProofDisputedEvents.originChainId as originChainId_dispute', | |
| 'BridgeProofDisputedEvents.originChain as originChain_dispute', | |
| ]) | |
| } |
| disputedRelays: async () => { | ||
| const query = db | ||
| .with('deposits', () => qDeposits()) | ||
| .with('relays', () => qRelays()) | ||
| .with('proofs', () => qProofs()) | ||
| .with('disputes', () => qDisputes()) | ||
| .with('combined', (qb) => | ||
| qb | ||
| .selectFrom('proofs') | ||
| .leftJoin( | ||
| 'disputes', | ||
| 'transactionId_proof', | ||
| 'transactionId_dispute' | ||
| ) | ||
| .selectAll('proofs') | ||
| .selectAll('disputes') | ||
| ) | ||
| .selectFrom('combined') | ||
| .selectAll() | ||
| .orderBy('blockTimestamp_proof', 'desc') | ||
|
|
||
| return nest_results(await query.execute()) | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Join deposits and relays in disputedRelays for comprehensive data
The disputedRelays resolver currently joins proofs with disputes but does not include data from deposits and relays. Joining these tables provides a complete view of the transactions and ensures all relevant data is available for the nested results.
Apply this diff to include deposits and relays in the query:
const query = db
.with('deposits', () => qDeposits())
.with('relays', () => qRelays())
.with('proofs', () => qProofs())
.with('disputes', () => qDisputes())
.with('combined', (qb) =>
qb
- .selectFrom('proofs')
+ .selectFrom('deposits')
+ .innerJoin('proofs', 'transactionId_deposit', 'transactionId_proof')
+ .leftJoin('relays', 'transactionId_deposit', 'transactionId_relay')
.leftJoin('disputes', 'transactionId_proof', 'transactionId_dispute')
.selectAll('deposits')
.selectAll('proofs')
+ .selectAll('relays')
.selectAll('disputes')
)This change ensures that data from deposits and relays are included, providing a more complete dataset.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| disputedRelays: async () => { | |
| const query = db | |
| .with('deposits', () => qDeposits()) | |
| .with('relays', () => qRelays()) | |
| .with('proofs', () => qProofs()) | |
| .with('disputes', () => qDisputes()) | |
| .with('combined', (qb) => | |
| qb | |
| .selectFrom('proofs') | |
| .leftJoin( | |
| 'disputes', | |
| 'transactionId_proof', | |
| 'transactionId_dispute' | |
| ) | |
| .selectAll('proofs') | |
| .selectAll('disputes') | |
| ) | |
| .selectFrom('combined') | |
| .selectAll() | |
| .orderBy('blockTimestamp_proof', 'desc') | |
| return nest_results(await query.execute()) | |
| }, | |
| disputedRelays: async () => { | |
| const query = db | |
| .with('deposits', () => qDeposits()) | |
| .with('relays', () => qRelays()) | |
| .with('proofs', () => qProofs()) | |
| .with('disputes', () => qDisputes()) | |
| .with('combined', (qb) => | |
| qb | |
| .selectFrom('deposits') | |
| .innerJoin('proofs', 'transactionId_deposit', 'transactionId_proof') | |
| .leftJoin('relays', 'transactionId_deposit', 'transactionId_relay') | |
| .leftJoin( | |
| 'disputes', | |
| 'transactionId_proof', | |
| 'transactionId_dispute' | |
| ) | |
| .selectAll('deposits') | |
| .selectAll('proofs') | |
| .selectAll('relays') | |
| .selectAll('disputes') | |
| ) | |
| .selectFrom('combined') | |
| .selectAll() | |
| .orderBy('blockTimestamp_proof', 'desc') | |
| return nest_results(await query.execute()) | |
| }, |
| .unionAll( | ||
| db | ||
| .selectFrom('BridgeProofDisputedEvents') | ||
| .select([ | ||
| 'BridgeProofDisputedEvents.id', | ||
| 'BridgeProofDisputedEvents.transactionId', | ||
| 'BridgeProofDisputedEvents.blockNumber', | ||
| 'BridgeProofDisputedEvents.blockTimestamp', | ||
| 'BridgeProofDisputedEvents.transactionHash', | ||
| 'BridgeProofDisputedEvents.originChainId', | ||
| 'BridgeProofDisputedEvents.originChain', | ||
| ]) | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Field Selection Detected in Unified Event Queries
The selected fields in the unioned event queries do not match the expected fields based on BridgeRequestEvents. Specifically:
-
BridgeProofDisputedEvents
- Extra fields:
blockNumberasblockNumber_disputeblockTimestampasblockTimestamp_disputeoriginChainIdasoriginChainId_disputeoriginChainasoriginChain_disputetransactionHashastransactionHash_disputetransactionIdastransactionId_dispute
- Extra fields:
-
BridgeRelayedEvents
- Extra fields:
blockNumberasblockNumber_relayblockTimestampasblockTimestamp_relayrelayerasrelayer_relaytoasto_relaytransactionHashastransactionHash_relaytransactionIdastransactionId_relay
- Extra fields:
-
BridgeProofProvidedEvents
- Extra fields:
blockNumberasblockNumber_proofblockTimestampasblockTimestamp_proofrelayerasrelayer_prooftransactionHashastransactionHash_prooftransactionIdastransactionId_proof
- Extra fields:
-
BridgeDepositClaimedEvents
- Extra fields:
amountFormattedasamountFormatted_claimblockNumberasblockNumber_claimblockTimestampasblockTimestamp_claimrelayerasrelayer_claimtoasto_claimtransactionHashastransactionHash_claimtransactionIdastransactionId_claim
- Extra fields:
-
BridgeDepositRefundedEvents
- Extra fields:
amountFormattedasamountFormatted_refundblockNumberasblockNumber_refundblockTimestampasblockTimestamp_refundtoasto_refundtransactionHashastransactionHash_refundtransactionIdastransactionId_refund
- Extra fields:
Action Required:
- Align the field selections in all unioned event queries to match the expected fields from
BridgeRequestEvents. - Remove any extra or inconsistently aliased fields.
- Ensure that all event queries maintain schema consistency to prevent processing issues.
🔗 Analysis chain
Ensure consistent field selection in unified event queries
When adding BridgeProofDisputedEvents to the events query, verify that the selected fields match those of the other event queries to maintain schema consistency. Inconsistent fields can lead to issues when processing the unified results.
Run the following script to compare the selected fields across all event queries:
This script will highlight any discrepancies in the selected fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all unioned queries in the 'events' query select consistent fields.
# Expected fields based on 'BridgeRequestEvents' selection
expected_fields=$(cat <<EOF
BridgeRequestEvents.id
BridgeRequestEvents.transactionId
BridgeRequestEvents.blockNumber
BridgeRequestEvents.blockTimestamp
BridgeRequestEvents.transactionHash
BridgeRequestEvents.originChainId
BridgeRequestEvents.originChain
EOF
)
# Function to extract selected fields from a query
extract_fields() {
rg -A 10 "$1" packages/rfq-indexer/api/src/graphql/resolvers.ts | \
rg "select(\[|\()" -A 10 | \
sed -n '/select/,$p' | \
tr -d ' \t' | \
grep -v '^$' | \
grep -v 'select' | \
grep -v '\[' | \
grep -v '\]' | \
grep -v '),' | \
sed 's/,$//' | \
sort
}
# Extract fields from each event query
fields_disputed=$(extract_fields "selectFrom.*BridgeProofDisputedEvents")
fields_relays=$(extract_fields "selectFrom.*BridgeRelayedEvents")
fields_proofs=$(extract_fields "selectFrom.*BridgeProofProvidedEvents")
fields_claims=$(extract_fields "selectFrom.*BridgeDepositClaimedEvents")
fields_refunds=$(extract_fields "selectFrom.*BridgeDepositRefundedEvents")
# Compare fields
echo "Comparing selected fields in 'BridgeProofDisputedEvents' to expected fields:"
diff <(echo "$expected_fields" | sort) <(echo "$fields_disputed")
echo "Ensure other event queries also match the expected fields."
Length of output: 7234
Script:
#!/bin/bash
# Description: Verify that all unioned queries in the 'events' query select consistent fields without using process substitution.
# Expected fields based on 'BridgeRequestEvents' selection
cat <<EOF > expected_fields.txt
BridgeRequestEvents.id
BridgeRequestEvents.transactionId
BridgeRequestEvents.blockNumber
BridgeRequestEvents.blockTimestamp
BridgeRequestEvents.transactionHash
BridgeRequestEvents.originChainId
BridgeRequestEvents.originChain
EOF
# Function to extract selected fields from a query
extract_fields() {
local query_pattern="$1"
rg -A 10 "$query_pattern" packages/rfq-indexer/api/src/graphql/resolvers.ts | \
rg "select(\[|\()" -A 10 | \
sed -n '/select/,$p' | \
tr -d ' \t' | \
grep -v '^$' | \
grep -v 'select' | \
grep -v '\[' | \
grep -v '\]' | \
grep -v '),' | \
sed 's/,$//' | \
sort > "$2"
}
# Extract fields from each event query
extract_fields "selectFrom.*BridgeProofDisputedEvents" fields_disputed.txt
extract_fields "selectFrom.*BridgeRelayedEvents" fields_relays.txt
extract_fields "selectFrom.*BridgeProofProvidedEvents" fields_proofs.txt
extract_fields "selectFrom.*BridgeDepositClaimedEvents" fields_claims.txt
extract_fields "selectFrom.*BridgeDepositRefundedEvents" fields_refunds.txt
# Compare fields for BridgeProofDisputedEvents
diff expected_fields.txt fields_disputed.txt > diff_disputed.txt
echo "Comparing selected fields in 'BridgeProofDisputedEvents' to expected fields:"
cat diff_disputed.txt
# Compare fields for BridgeRelayedEvents
diff expected_fields.txt fields_relays.txt > diff_relays.txt
echo "Comparing selected fields in 'BridgeRelayedEvents' to expected fields:"
cat diff_relays.txt
# Compare fields for BridgeProofProvidedEvents
diff expected_fields.txt fields_proofs.txt > diff_proofs.txt
echo "Comparing selected fields in 'BridgeProofProvidedEvents' to expected fields:"
cat diff_proofs.txt
# Compare fields for BridgeDepositClaimedEvents
diff expected_fields.txt fields_claims.txt > diff_claims.txt
echo "Comparing selected fields in 'BridgeDepositClaimedEvents' to expected fields:"
cat diff_claims.txt
# Compare fields for BridgeDepositRefundedEvents
diff expected_fields.txt fields_refunds.txt > diff_refunds.txt
echo "Comparing selected fields in 'BridgeDepositRefundedEvents' to expected fields:"
cat diff_refunds.txt
echo "Ensure other event queries also match the expected fields."
Length of output: 8147
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## fe-release #3240 +/- ##
================================================
Coverage ? 90.43902%
================================================
Files ? 54
Lines ? 1025
Branches ? 82
================================================
Hits ? 927
Misses ? 95
Partials ? 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
abtestingalpha
temporarily deployed
to
github-pages
Bundle ReportChanges will decrease total bundle size by 4.15MB (-11.65%) ⬇️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
docs/bridge/src/components/Routes.tsx (2)
6-6: Improved chain image renderingThe change to use an
<img>tag for rendering the chain image is a good improvement. It simplifies the code and makes it more consistent with standard HTML practices.Consider adding error handling for cases where
chainImgmight be undefined:-<img width="28" height="28" src={chainImg} alt={chain.name} /> +{chainImg && <img width="28" height="28" src={chainImg} alt={chain.name} />}This will prevent rendering an empty image if
chainImgis not available for some reason.Also applies to: 18-18
32-33: Improved token image renderingThe change to use an
<img>tag for rendering the token image is a good improvement. It simplifies the code and makes it more consistent with the chain image rendering approach.Consider adding error handling for cases where
token.iconmight be undefined:-<img width="16" height="16" src={token.icon} alt={token.symbol} />{' '} +{token.icon && <img width="16" height="16" src={token.icon} alt={token.symbol} />}{' '}This will prevent rendering an empty image if
token.iconis not available for some reason.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- docs/bridge/CHANGELOG.md (1 hunks)
- docs/bridge/docs/01-About/03-Routes.md (1 hunks)
- docs/bridge/docs/02-Bridge/04-Code-Examples.md (1 hunks)
- docs/bridge/docs/02-Bridge/05-Supported-Routes.md (0 hunks)
- docs/bridge/docs/02-Bridge/_05-Supported-Routes.md (0 hunks)
- docs/bridge/docs/02-Bridge/index.md (1 hunks)
- docs/bridge/package.json (1 hunks)
- docs/bridge/src/components/Routes.tsx (3 hunks)
💤 Files with no reviewable changes (2)
- docs/bridge/docs/02-Bridge/05-Supported-Routes.md
- docs/bridge/docs/02-Bridge/_05-Supported-Routes.md
✅ Files skipped from review due to trivial changes (3)
- docs/bridge/CHANGELOG.md
- docs/bridge/docs/02-Bridge/04-Code-Examples.md
- docs/bridge/package.json
🧰 Additional context used
🔇 Additional comments (3)
docs/bridge/docs/01-About/03-Routes.md (3)
3-5: LGTM! Clear and informative content.The markdown content is well-structured and provides clear information about the purpose of the page. The link to the Synapse Bridge is correctly formatted.
7-7: LGTM! Verify the Routes component output.The
Routescomponent is correctly rendered using JSX syntax. Ensure that the markdown processor used in this project supports JSX syntax.To verify the component's functionality and output, please manually check the rendered page in the development environment or staging site. Confirm that:
- The component renders without errors.
- It displays the expected list of supported tokens organized by blockchain chain.
- The information presented is accurate and up-to-date.
1-1: LGTM! Verify the imported component.The import statement looks correct. However, it's important to ensure that the
Routescomponent exists at the specified path.Run the following script to verify the existence of the Routes component:
✅ Verification successful
Verified: The
Routescomponent exists atdocs/bridge/src/components/Routes.tsx.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the Routes component # Test: Check if the Routes component file exists fd --type f --full-path '.*src/components/Routes.(js|jsx|ts|tsx)$'Length of output: 106
| export default () => | ||
| Object.entries(BRIDGABLE_TOKENS).map(([id, tokens]) => { | ||
| const chain = CHAINS_BY_ID[id] | ||
| const chainImg = chain.chainImg({ width: 28, height: 28 }) | ||
| const chain = CHAINS.CHAINS_BY_ID[id] |
There was a problem hiding this comment.
💡 Codebase verification
Incorrect usage of CHAINS_BY_ID
The CHAINS object does not contain a CHAINS_BY_ID property. You should reference CHAINS_BY_ID directly instead of accessing it via CHAINS.
-
Suggested change in
docs/bridge/src/components/Routes.tsx:- const chain = CHAINS.CHAINS_BY_ID[id] + const chain = CHAINS_BY_ID[id]
🔗 Analysis chain
Incorrect chain access method
The current implementation attempts to access CHAINS.CHAINS_BY_ID[id], which is likely incorrect. Based on the previous implementation and the typical structure of such constants, it's probable that CHAINS_BY_ID is a separate object, not a property of CHAINS.
Consider reverting to the previous access method or ensure that the CHAINS object has the correct structure. If CHAINS_BY_ID is a separate export from the constants file, you should use:
-const chain = CHAINS.CHAINS_BY_ID[id]
+const chain = CHAINS_BY_ID[id]If CHAINS_BY_ID is no longer available, please provide more context on how chain information should be accessed.
To verify the correct structure, we can check the contents of the constants file:
Constants file not found
The shell script did not locate a constants file containing synapse-constants. To verify the structure of CHAINS, we need to identify where it is defined in the codebase.
Failed to locate CHAINS definitions
The previous shell script failed to recognize the tsx file type. To locate the definitions of CHAINS and CHAINS_BY_ID, we need to adjust the search parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the structure of CHAINS in the constants file
# Find the constants file
constants_file=$(fd -e ts -e js -e tsx -e jsx -t f synapse-constants)
if [ -n "$constants_file" ]; then
echo "Contents of the constants file:"
cat "$constants_file"
else
echo "Constants file not found."
fi
Length of output: 149
Script:
#!/bin/bash
# Description: Find the definition of CHAINS and CHAINS_BY_ID in the codebase
# Search for CHAINS definition
echo "Searching for 'CHAINS' definition:"
rg 'CHAINS\s*=' --type ts --type tsx --type js --type jsx
# Search for 'CHAINS_BY_ID' definition
echo "Searching for 'CHAINS_BY_ID' definition:"
rg 'CHAINS_BY_ID\s*=' --type ts --type tsx --type js --type jsx
Length of output: 363
Script:
#!/bin/bash
# Description: Find the definition of CHAINS and CHAINS_BY_ID in the codebase without specifying file types
# Search for CHAINS definition
echo "Searching for 'CHAINS' definition:"
rg 'CHAINS\s*='
# Search for 'CHAINS_BY_ID' definition:
rg 'CHAINS_BY_ID\s*='
Length of output: 1524
| @@ -7,7 +7,7 @@ import SVGBridge from '@site/src/components/SVGBridge' | |||
|
|
|||
| # Synapse Bridge | |||
|
|
|||
| The [Synapse Bridge](https://synapseprotocol.com) and [Solana Bridge](https://solana.synapseprotocol.com/) seamlessly swap on-chain assets between [20+ EVM and non-EVM blockchains](./Supported-Routes) in a safe and secure manner. | |||
| The [Synapse Bridge](https://synapseprotocol.com) and [Solana Bridge](https://solana.synapseprotocol.com/) seamlessly swap on-chain assets between [20+ EVM and non-EVM blockchains](/docs/About/Routes) in a safe and secure manner. | |||
There was a problem hiding this comment.
💡 Codebase verification
Invalid link detected in documentation.
The link /docs/About/Routes in docs/bridge/docs/02-Bridge/index.md does not point to an existing documentation page. Please update the link to docs/bridge/docs/01-About/03-Routes.md to ensure it directs to the correct location.
🔗 Analysis chain
Approve link update with verification request.
The update to the documentation link appears to be an improvement in the overall structure. However, please ensure that the new link '/docs/About/Routes' is valid and leads to the correct page containing information about the supported blockchains.
To verify the existence of the new documentation page, please run the following script:
If the script doesn't find the file, please create it or update the link to the correct location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new Routes documentation file exists
# Test: Search for the Routes documentation file
fd -t f 'Routes.md' docs/About
Length of output: 128
Script:
#!/bin/bash
# Description: Search for Routes.md in the entire repository to verify its existence and correct path.
fd -t f 'Routes.md'
Length of output: 58
…[SLT-233] (#3215) * feat: add `callParams` * feat: scaffold `IFastBridgeRecipient` * feat: add `callParams` to `BridgeTransactionV2` * test: skip `getBridgeTransaction` V2 test for now * test: add coverage for SRC arbitrary calls * feat: check callParams length when bridging * test: non-payable ETH recipient * test: no-op contract recipient * test: incorrect recipients cases (does not affect base tests) * test: define cases for arbitrary call * feat: update relay logic with arbitrary calls * feat: checked call of the recipient hook function * test: refactor state setup * refactor: better comments in `relay` * docs: add a TODO note wrt encoding changes
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (30)
packages/contracts-rfq/test/mocks/NoOpContract.sol (1)
4-8: LGTM: Well-structured mock contract with a minor suggestion.The
NoOpContractis well-implemented for its purpose as a mock that can receive ETH. Thereceivefunction is correctly defined, and the comment explains its purpose clearly.Consider adding a
@devtag to the comment to follow common Solidity documentation practices:- /// @notice Mock needs to accept ETH + /// @dev Mock contract that can receive ETH + /// @notice Allows the contract to receive ETH receive() external payable {}packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol (1)
4-12: Approve interface and function declaration, suggest adding NatSpec comments.The interface
IFastBridgeRecipientand its functionfastBridgeTransferReceivedare well-defined. The function is correctly marked as external and payable, which allows it to be called from other contracts and receive Ether. The bytes4 return type is appropriate for returning a function selector or acknowledging successful execution.Consider adding NatSpec comments to provide more context about the interface and function's purpose, parameters, and return value. This would improve the contract's documentation and make it more developer-friendly. For example:
/// @title IFastBridgeRecipient /// @notice Interface for contracts that can receive tokens via a fast bridge interface IFastBridgeRecipient { /// @notice Called when tokens are received via a fast bridge /// @param token The address of the token being transferred /// @param amount The amount of tokens being transferred /// @param callParams Additional parameters for the transfer /// @return bytes4 A function selector or acknowledgment of the transfer function fastBridgeTransferReceived( address token, uint256 amount, bytes memory callParams ) external payable returns (bytes4); }packages/contracts-rfq/test/mocks/NonPayableRecipient.sol (2)
4-5: LGTM: Clear documentation and appropriate naming.The contract is well-documented, clearly stating its purpose as a mock for testing and warning against production use. The name
NonPayableRecipientaccurately reflects its nature.Consider adding a brief explanation of why this implementation is incorrect, to further clarify its purpose in testing scenarios.
6-9: LGTM: Function implementation serves its purpose as a mock.The
fastBridgeTransferReceivedfunction is correctly implemented for its purpose as a mock:
- It matches the expected interface.
- The
puremodifier is appropriate.- Returning the function selector is a common pattern for interface implementations.
The comment about incorrect implementation is crucial for understanding the mock's purpose.
Consider adding a
@devcomment explaining why returning the function selector is the expected behavior for this mock. This would provide more context for developers unfamiliar with this pattern.packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol (2)
4-4: Consider using a more specific solhint-disable directive.The current
solhint-disabledirective disables all linter rules for the entire file. It's generally better to disable only specific rules that are necessary to ignore. If you need to disable multiple rules, you can list them explicitly.For example:
// solhint-disable-next-line no-empty-blocks, avoid-low-level-callsOr for multiple lines:
/* solhint-disable no-empty-blocks, avoid-low-level-calls */ // ... code ... /* solhint-enable no-empty-blocks, avoid-low-level-calls */
11-12: ThefastBridgeTransferReceivedfunction is correctly implemented for its intended testing purpose.The function is correctly set up as an external, payable function with the appropriate parameters. The empty implementation is intentional, as stated in the comment, to simulate an incorrect implementation for testing purposes.
A minor suggestion to improve the NatSpec comment:
- /// @notice Incorrectly implemented - method does not return anything. + /// @notice Incorrectly implemented - method does not return anything, simulating a contract that fails to implement the expected interface.This provides more context about why the lack of a return value is considered "incorrect" in this mock.
packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol (1)
Line range hint
11-14: Consider adding an explanatory comment for thesetUpfunctionWhile the
setUpfunction implementation looks good, it might be beneficial to add a comment explaining why you're skipping half of theEXCLUSIVITY_PERIOD. This would enhance the readability and maintainability of the test, making it easier for other developers to understand the test setup and its purpose.Example comment:
function setUp() public virtual override { super.setUp(); // Skip half of the exclusivity period to test behavior in the middle of the exclusivity window skip({time: EXCLUSIVITY_PERIOD / 2}); }packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (2)
6-6: LGTM! Consider adding parameters for improved debugging.The
CallParamsLengthAboveMaxerror is a good addition for handling cases where call parameters exceed a maximum length.Consider adding parameters to provide more context, such as:
error CallParamsLengthAboveMax(uint256 actualLength, uint256 maxLength);This would make debugging easier by providing specific information about the violation.
14-14: LGTM! Consider adding parameters for more detailed error reporting.The
RecipientIncorrectReturnValueerror is a valuable addition for handling cases where a recipient returns an incorrect value.To enhance debugging capabilities, consider adding parameters to provide more context:
error RecipientIncorrectReturnValue(bytes expectedValue, bytes actualValue);This would allow for more precise error reporting and easier troubleshooting.
packages/contracts-rfq/test/mocks/RecipientMock.sol (2)
6-9: LGTM: Well-structured contract declaration with appropriate warning.The contract declaration and receive function are correctly implemented. The warning comment about not using in production is a good practice for test contracts.
Consider adding a more detailed comment explaining the purpose of this mock contract and how it's intended to be used in tests. This can help other developers understand its role in the testing suite more quickly.
11-14: LGTM: Correct implementation of fastBridgeTransferReceived.The fastBridgeTransferReceived function is correctly implemented according to the IFastBridgeRecipient interface. The minimal implementation is appropriate for a mock contract used in testing.
Consider adding a comment explaining why the function parameters are not used in this mock implementation. This can help prevent confusion for developers who might expect these parameters to be utilized.
packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol (3)
6-7: LGTM: Well-documented contract declaration.The contract declaration and documentation are clear and follow best practices. The warning against production use is particularly important for this mock contract.
Consider adding a brief explanation of why this mock is needed in the testing context. For example:
/// @notice Incorrectly implemented recipient mock for testing purposes. DO NOT USE IN PRODUCTION. /// @dev This mock is used to test the behavior of the system when a recipient returns an incorrect value.
11-15: LGTM: Well-implemented mock function with intentional error.The fastBridgeTransferReceived() function is correctly implemented to return an intentionally incorrect value. The use of XOR to flip the last bit of the selector is a clever approach.
Consider adding more detailed documentation to explain the specific test scenarios this mock is designed for. For example:
/// @notice Incorrectly implemented - method returns incorrect value. /// @dev This function intentionally returns an incorrect selector to test error handling in the main contract. /// @return A bytes4 value that is the original selector with its last bit flipped.
1-16: Overall: Well-implemented mock contract for testing purposes.This mock contract is well-structured, clearly documented, and serves its purpose of providing an incorrect implementation for testing. The intentional error in the fastBridgeTransferReceived() function is cleverly implemented and well-commented. The contract follows Solidity best practices and includes appropriate warnings against production use.
Ensure that the test suite using this mock covers various scenarios, including:
- Verifying that the main contract correctly handles the incorrect return value.
- Testing error messages or events emitted when dealing with this incorrect implementation.
- Checking that the system's overall integrity is maintained when interacting with this mock.
packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol (1)
11-12: Approve refactoring with a suggestion for improved clarityThe refactoring of
createFixturesV2()improves code maintainability by encapsulating the logic for setting exclusivity parameters. The use of separate methods for token and ETH params with different relayers allows for more flexible and thorough testing scenarios.Consider adding a brief comment explaining the purpose of using different relayers for token and ETH tests, as it might not be immediately clear to other developers why this distinction is made. For example:
// Use different relayers for token and ETH tests to ensure proper handling of distinct relayers setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD); setEthTestExclusivityParams(relayerB, EXCLUSIVITY_PERIOD);packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol (3)
11-13: Approve changes with a minor suggestion.The modifications to
createFixturesV2improve code maintainability by using theEXCLUSIVITY_PERIOD_ABSconstant. The negative exclusivity periods are correctly set for negative testing scenarios.Consider adding a brief comment explaining the purpose of setting negative exclusivity periods for clarity:
// Populate the fields using the absolute exclusivity period setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD_ABS); setEthTestExclusivityParams(relayB, EXCLUSIVITY_PERIOD_ABS); +// Set negative exclusivity periods for negative testing scenarios // Override with negative exclusivity period tokenParamsV2.quoteExclusivitySeconds = -int256(EXCLUSIVITY_PERIOD_ABS); ethParamsV2.quoteExclusivitySeconds = -int256(EXCLUSIVITY_PERIOD_ABS);
Line range hint
36-40: LGTM: Good test for zero exclusivity end time in ETH transactions.This test function effectively checks that the contract reverts with
ExclusivityParamsIncorrectwhen the exclusivity end time would be zero for ETH transactions. The use ofvm.expectRevertis appropriate.For consistency with the token test, consider renaming the function to
test_bridge_eth_revert_exclusivityEndTimeZero:-function test_bridge_eth_revert_exclusivityEndTimeZero() public { +function test_bridge_eth_revert_exclusivityEndTimeZero() public {
Line range hint
42-46: LGTM: Effective test for exclusivity period underflow in ETH transactions.This test function properly verifies that the contract reverts with
ExclusivityParamsIncorrectwhen the exclusivity period would underflow for ETH transactions. The use ofvm.expectRevertis correct for this scenario.For consistency with the token test, consider renaming the function to
test_bridge_eth_revert_exclusivityPeriodUnderflow:-function test_bridge_eth_revert_exclusivityPeriodUnderflow() public { +function test_bridge_eth_revert_exclusivityPeriodUnderflow() public {packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (3)
36-41: LGTM! Consider enhancing the comment for clarity.The addition of
callParamsto theBridgeParamsV2struct enhances the flexibility of the bridge transaction process by allowing arbitrary call parameters to be passed to the destination recipient. This is a valuable improvement.Consider slightly modifying the comment for
callParamsto provide more context:- /// @param callParams Parameters for the arbitrary call to the destination recipient (if any) + /// @param callParams Optional parameters for an arbitrary call to be executed on the destination chain (if any)This change clarifies that the parameters are for an optional call that will be executed on the destination chain, which might be more informative for developers using this interface.
62-62: LGTM! Consider adding a comment for the new field.The addition of
callParamsto theBridgeTransactionV2struct is consistent with its addition toBridgeParamsV2and enhances the functionality of the bridge transaction process.To improve code readability and maintainability, consider adding a comment for the new
callParamsfield:uint256 exclusivityEndTime; + /// @dev Optional parameters for an arbitrary call to be executed on the destination chain bytes callParams;This comment will provide context for the purpose of
callParamswithin theBridgeTransactionV2struct, making it easier for developers to understand its role in the bridging process.
Line range hint
1-124: Summary: Enhancements to bridge transaction flexibilityThe changes to
IFastBridgeV2interface improve the flexibility of the bridge transaction process by introducing thecallParamsfield to bothBridgeParamsV2andBridgeTransactionV2structs. This addition allows for arbitrary call parameters to be passed to the destination recipient, enhancing the capabilities of the bridge.Key points:
- No breaking changes were introduced to the interface.
- The
bridgefunction signature remains unchanged but now accepts the updatedBridgeParamsV2struct with the new field.- These changes provide a more versatile bridging mechanism that can accommodate a wider range of use cases.
As the project evolves, consider the following:
- Ensure that all implementations of this interface properly handle the new
callParamsfield.- Update any related documentation or integration guides to reflect these new capabilities.
- Consider adding validation mechanisms for
callParamsto prevent potential security risks associated with arbitrary calls.packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (1)
20-21: LGTM. Consider using a constant for empty string initialization.The addition of the
callParamsfield toethParamsV2is consistent with the changes made totokenParamsV2. To improve maintainability, consider defining a constant for the empty string initialization used in bothtokenParamsV2andethParamsV2.Example:
string constant EMPTY_CALL_PARAMS = "";Then use
EMPTY_CALL_PARAMSinstead of""in both initializations.packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol (1)
6-6: Specify the reason for disabling solhint rulesIt's good practice to provide a comment explaining why specific Solhint rules are being disabled. This improves code readability and helps other developers understand the rationale behind these exceptions.
packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (1)
56-56: Address the TODO comment: Reevaluate test necessityThere's a TODO comment indicating the need to reevaluate the necessity of this test if or when the encoding scheme changes. Would you like assistance in addressing this, or should I open a GitHub issue to track this task?
packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (6)
159-163: Inconsistent test naming: Function name suggests different behaviorThe test function
test_relay_token_excessiveReturnValueRecipient_revertWhenCallParamsPresentimplies it tests a scenario wherecallParamsare present and expects a revert. However, it asserts thatcallParamsare empty and proceeds without expecting a revert. This may cause confusion.Consider renaming the function to reflect its current behavior or adding comments to clarify that it is intended to be overridden in derived classes where
callParamsare present.
165-172: Inconsistent test naming: Function name implies different scenarioSimilarly, the function
test_relay_token_withRelayerAddress_excessiveReturnValueRecipient_revertWhenCallParamsPresentsuggests testing a revert whencallParamsare present, but it asserts thatcallParamsare empty.Recommend clarifying the function name or adding comments to explain its purpose in the current context.
174-178: Inconsistent test naming in ETH relay testThe test function
test_relay_eth_excessiveReturnValueRecipient_revertWhenCallParamsPresentimplies a revert whencallParamsare present, but it asserts thatcallParamsare empty.Consider adjusting the function name or adding clarifying comments for better readability.
191-223: Inconsistent test naming in Incorrect Return Value Recipient testsThe test functions for the incorrect return value recipient also exhibit similar inconsistencies between their names and behavior. They assert that
callParamsare empty while the function names suggest testing withcallParamspresent.Suggest renaming the functions or enhancing comments to make the intended usage clearer.
254-276: Inconsistent test naming in No-Op Recipient testsThe test functions like
test_relay_token_noOpRecipient_revertWhenCallParamsPresentimply they are testing withcallParamspresent and expecting a revert, but they assert thatcallParamsare empty.Recommend adjusting the function names or adding explanatory comments to clarify the intended test scenarios.
283-305: Inconsistent test naming in No Return Value Recipient testsThe test functions for the no return value recipient suggest they are testing scenarios with
callParamspresent, yet they assert thatcallParamsare empty.Consider renaming these functions or adding comments to better reflect their current behavior and future overriding intentions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (22)
- packages/contracts-rfq/CHANGELOG.md (1 hunks)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (9 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (1 hunks)
- packages/contracts-rfq/package.json (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (3 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (3 hunks)
- packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/NoOpContract.sol (1 hunks)
- packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/NonPayableRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/RecipientMock.sol (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/contracts-rfq/package.json
🧰 Additional context used
🪛 Markdownlint
packages/contracts-rfq/CHANGELOG.md
9-9: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
🔇 Additional comments (42)
packages/contracts-rfq/test/mocks/NoOpContract.sol (1)
1-2: LGTM: Appropriate license and Solidity version.The MIT license is suitable for open-source projects, and the Solidity version (^0.8.0) is recent and includes important security features.
packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol (1)
1-2: LGTM: License and Solidity version are appropriate.The MIT license is suitable for open-source projects, and the Solidity version (^0.8.0) is recent and allows for minor version updates, which is a good practice.
packages/contracts-rfq/test/mocks/NonPayableRecipient.sol (1)
1-2: LGTM: Appropriate license and Solidity version.The MIT license is suitable for open-source projects, and the Solidity version (^0.8.0) is recent and allows for minor updates.
packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol (2)
6-7: Contract declaration and documentation look good.The contract name
NoReturnValueRecipientis clear and descriptive. The NatSpec comment provides important information about the contract's purpose and usage, explicitly stating that it's for testing purposes and should not be used in production. This is good practice for mock contracts.
8-9: Thereceivefunction is correctly implemented for a mock contract.The
receivefunction allows the contract to accept ETH payments, which is necessary for a mock contract that might need to simulate receiving ETH. The empty function body is appropriate for this purpose. The NatSpec comment clearly explains the function's purpose.packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol (1)
16-17: Excellent refactoring ofcreateFixturesV2!The changes improve code readability and maintainability by consolidating multiple parameter assignments into two function calls:
setTokenTestExclusivityParamsandsetEthTestExclusivityParams. This approach ensures consistency by using theEXCLUSIVITY_PERIODconstant andrelayerAacross both token and ETH test setups.packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (2)
15-15: LGTM! Clear and concise error definition.The
RecipientNoReturnValueerror is a good addition for handling cases where a recipient doesn't return a value when one was expected. The error is self-explanatory and doesn't require additional parameters.
6-15: Overall improvement in error handling granularity.The addition of these three new errors (
CallParamsLengthAboveMax,RecipientIncorrectReturnValue, andRecipientNoReturnValue) enhances the error handling capabilities of the FastBridgeV2 contract. These errors provide more specific reporting related to call parameters and recipient return values, which will likely improve debugging and error handling in the contract implementation.packages/contracts-rfq/test/mocks/RecipientMock.sol (2)
1-2: LGTM: Appropriate license and Solidity version.The file structure starts correctly with the SPDX license identifier and an appropriate Solidity version declaration.
4-4: LGTM: Correct import statement.The import statement for the IFastBridgeRecipient interface is correctly formatted and uses an appropriate relative path.
packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol (4)
1-4: LGTM: Appropriate license and import.The contract uses the MIT license, a recent Solidity version (^0.8.0), and correctly imports the IFastBridgeRecipient interface. These choices align with best practices for Solidity development.
6-7: Excellent use of documentation for this mock contract.The contract name is descriptive and follows naming conventions. The comment explicitly stating that this is a mock for testing purposes and should not be used in production is crucial. This helps prevent potential misuse and clearly communicates the contract's intended purpose.
8-9: LGTM: Correctly implemented receive function with clear documentation.The receive function is properly implemented to accept ETH, which is necessary for this mock contract's testing purposes. The accompanying comment clearly explains the function's purpose, enhancing code readability and maintainability.
11-14: LGTM: Well-implemented mock function for testing edge cases.The
fastBridgeTransferReceivedfunction is correctly implemented as a mock for testing purposes. It intentionally returns an excessive number of bytes, which is useful for testing error handling in the actual implementation. The use of the interface selector and a hardcoded value (1337) is a good practice for verifying that this mock is indeed called in tests.Remember that this implementation is specifically designed for testing and should never be used in a production environment.
packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol (3)
1-2: LGTM: Appropriate license and Solidity version.The SPDX-License-Identifier is correctly specified, and the Solidity version is appropriately set to ^0.8.0, allowing for minor updates while ensuring compatibility.
4-4: LGTM: Correct import statement.The import statement for the IFastBridgeRecipient interface is correctly implemented and follows best practices.
8-9: LGTM: Correctly implemented receive() function.The receive() function is properly implemented as external and payable. The NatSpec comment clearly explains its purpose in the context of this mock contract.
packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol (1)
11-12: Verify impact of changes on contract behaviorThe refactoring of
createFixturesV2()appears to maintain the existing behavior of the contract. Thebridge()function and test functions remain unchanged and should still work as expected with the new parameter setting methods.To ensure that the refactoring hasn't introduced any unintended side effects, please run the following verification:
These checks will help confirm that the refactoring is complete and consistent throughout the contract.
✅ Verification successful
Retry verifying the parameter setting methods with adjusted search criteria
The initial search failed because
rgdoes not recognize thesolidityfile type. Let's attempt to search within.solfiles without specifying the file type.
Parameter Setting Methods Verified Successfully
The new parameter setting methods are correctly defined and utilized. Both
tokenParamsV2andethParamsV2are actively used across the contract without any references to the old parameter setting logic.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new parameter setting methods are called correctly and that they set the expected values. # Test 1: Check if the new methods are defined in the parent class or in this contract rg --type solidity -e "function setTokenTestExclusivityParams" -e "function setEthTestExclusivityParams" # Test 2: Verify that tokenParamsV2 and ethParamsV2 are still being used in the contract rg --type solidity -e "tokenParamsV2" -e "ethParamsV2" packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol # Test 3: Check for any references to the old parameter setting logic that might have been missed rg --type solidity -e "quoteRelayer" -e "quoteExclusivitySeconds" -e "quoteId" packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.solLength of output: 459
Script:
#!/bin/bash # Description: Retry verifying that the new parameter setting methods are called correctly and that they set the expected values. # Find all Solidity files and search for the new parameter setting methods fd --extension sol | xargs rg "function setTokenTestExclusivityParams" fd --extension sol | xargs rg "function setEthTestExclusivityParams" # Verify that tokenParamsV2 and ethParamsV2 are still being used in the contract fd --extension sol | xargs rg "tokenParamsV2" fd --extension sol | xargs rg "ethParamsV2" # Check for any references to the old parameter setting logic that might have been missed fd --extension sol | xargs rg "quoteRelayer" fd --extension sol | xargs rg "quoteExclusivitySeconds" fd --extension sol | xargs rg "quoteId"Length of output: 12107
packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol (3)
Line range hint
24-28: LGTM: Well-structured test for zero exclusivity end time.This test function effectively checks that the contract reverts with
ExclusivityParamsIncorrectwhen the exclusivity end time would be zero. The use ofvm.expectRevertis appropriate for testing this scenario.
Line range hint
30-34: LGTM: Effective test for exclusivity period underflow.This test function properly verifies that the contract reverts with
ExclusivityParamsIncorrectwhen the exclusivity period would underflow. The use ofvm.expectRevertis correct for this scenario.
Line range hint
1-48: Overall assessment: Improved test coverage for exclusivity parameters.The changes in this file significantly enhance the test coverage for negative scenarios related to exclusivity parameters in the FastBridgeV2 contract. The new tests effectively cover important edge cases, such as zero exclusivity end time and exclusivity period underflow, for both token and ETH transactions. These additions contribute to a more robust test suite, helping to prevent potential vulnerabilities in the contract's handling of exclusivity parameters.
packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (2)
14-15: LGTM. Please clarify the purpose ofcallParams.The addition of the
callParamsfield totokenParamsV2is consistent with the changes mentioned in the summary. However, could you provide more context on the intended use of this new field? Understanding its purpose would help ensure that it's being used correctly in the tests.
Line range hint
1-180: Consider adding test cases for the newcallParamsfield.While the addition of the
callParamsfield to bothtokenParamsV2andethParamsV2is implemented correctly, there are no test cases that specifically validate this new field. To ensure comprehensive test coverage, consider adding test cases that:
- Verify that
callParamsis correctly set and retrieved.- Test the behavior of the system with non-empty
callParamsvalues.- Ensure that the
callParamsfield doesn't interfere with existing functionality.This will help maintain the robustness of the test suite and validate the new feature.
Would you like assistance in drafting these additional test cases?
packages/contracts-rfq/CHANGELOG.md (2)
6-16: LGTM: New changelog entry is well-formatted and informative.The new entry for version 0.7.0 follows the established format and provides clear information about the release. It includes:
- The correct version number and release date
- A link to compare with the previous version
- A concise description of the new feature
- References to the related issue and pull request
- The commit hash for traceability
This entry maintains the quality and consistency of the changelog.
🧰 Tools
🪛 Markdownlint
9-9: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
9-9: Note on heading format: Consistent with existing structure.The static analysis tool flagged the use of an h3 heading ("### Features") as a potential issue. However, this format is consistent with the rest of the changelog and appears to be intentional. Changing this would require updating the entire document structure, which is beyond the scope of this single entry. The current format is clear and functional for a changelog.
🧰 Tools
🪛 Markdownlint
9-9: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (3)
107-107: Summary: Improved test structure with new helper functionsThe changes in this file introduce two new helper functions,
setTokenTestExclusivityParamsandsetEthTestExclusivityParams, which are used in thetest_bridge_token_withExclusivityandtest_bridge_eth_withExclusivitytests respectively. These changes improve code readability and maintainability by encapsulating the setup of exclusivity parameters.However, the implementations of these new helper functions are not visible in the provided code. To ensure the correctness and completeness of these changes, please provide the implementations of both
setTokenTestExclusivityParamsandsetEthTestExclusivityParamsfunctions.Please provide the implementations of the new helper functions:
#!/bin/bash echo "Implementation of setTokenTestExclusivityParams:" rg -U "function setTokenTestExclusivityParams\(.*?\)" -A 10 echo "Implementation of setEthTestExclusivityParams:" rg -U "function setEthTestExclusivityParams\(.*?\)" -A 10If these functions are implemented in a different file, please provide the file name and the implementations.
Also applies to: 188-188
188-188: Refactor: New helper function used for ETH exclusivity testThe
setEthTestExclusivityParamsfunction has been introduced to set up exclusivity parameters for the ETH test. This refactoring is consistent with the changes made to the token test, improving code readability and maintainability.To ensure the correctness of this change, please provide the implementation of the
setEthTestExclusivityParamsfunction:#!/bin/bash rg -U "function setEthTestExclusivityParams\(.*?\)" -A 10
107-107: Refactor: New helper function used for token exclusivity testThe
setTokenTestExclusivityParamsfunction has been introduced to set up exclusivity parameters for the token test. This refactoring improves code readability and maintainability by encapsulating the parameter setup logic.To ensure the correctness of this change, please provide the implementation of the
setTokenTestExclusivityParamsfunction:packages/contracts-rfq/test/FastBridgeV2.t.sol (4)
127-138: Initialization oftokenParamsV2andethParamsV2increateFixturesV2The default initialization of
tokenParamsV2andethParamsV2with zero values is appropriate for the test setup and ensures that the parameters are reset before each test.
166-169:setTokenTestCallParamsfunction is correctly implementedThe function correctly sets the
callParamsfor bothtokenParamsV2andtokenTx, ensuring consistency between the parameters and the transaction.
180-183:setEthTestCallParamsfunction is correctly implementedThe function properly sets the
callParamsfor bothethParamsV2andethTx, maintaining consistency between the parameters and the transaction.
213-221:getMockQuoteIdfunction is correctly implementedThe function appropriately returns different mock quote IDs based on the provided relayer address, which is suitable for testing purposes.
packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (7)
6-10: Approved: Importing required mock contractsThe import statements for mock contracts are correct and necessary for the added test cases.
26-30: Approved: Declaration of recipient address variablesThe public address variables for the mock recipients are appropriately declared.
32-44: Setup function initializes mock recipients correctlyThe
setUpfunction properly instantiates the mock recipient contracts and labels them for easier identification during testing.
46-50: Approved:setTokenTestRecipientfunction implementationThe helper function correctly sets the recipient addresses for token tests.
52-56: Approved:setEthTestRecipientfunction implementationThe helper function appropriately updates the recipient addresses for ETH tests.
58-60: Approved:assertEmptyCallParamsfunction validates call parametersThe function accurately asserts that
callParamsare empty, ensuring correct test setup.
224-248: Approved: Non-payable recipient tests are correctly implementedThe tests correctly verify that ERC20 transfers continue unaffected to a non-payable recipient and appropriately expect reverts when attempting to transfer ETH to a non-payable recipient.
packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
29-31: Validate the appropriateness ofMAX_CALL_PARAMS_LENGTHThe constant
MAX_CALL_PARAMS_LENGTHis set to2 ** 16 - 1(65535). Ensure this value aligns with the expected maximum size ofcallParamsto prevent potential issues with gas limits or unexpected input sizes.
232-257: Verify reentrancy safety in external callsThe
relayfunction has been refactored to include external calls to_checkedCallRecipient. While the comments mention adherence to the checks-effects-interactions pattern, please double-check that all state changes occur before any external calls to prevent potential reentrancy attacks.
360-383: Review the implementation of_checkedCallRecipientfor securityThe
_checkedCallRecipientfunction usesAddress.functionCallWithValueto invoke the recipient's hook function. Ensure that:
- The recipient contract complies with the
IFastBridgeRecipientinterface.- The call correctly handles the transfer of
msgValue, especially when dealing with ETH transfers.- There are appropriate checks and error handling to prevent security vulnerabilities.
| function fastBridgeTransferReceived( | ||
| address token, | ||
| uint256 amount, | ||
| bytes memory callParams | ||
| ) | ||
| external | ||
| payable | ||
| returns (bytes4); |
There was a problem hiding this comment.
💡 Codebase verification
Issues Found with IFastBridgeRecipient Implementations
Several implementations of the IFastBridgeRecipient interface do not adhere to the defined function signature of fastBridgeTransferReceived:
-
NoReturnValueRecipient.sol- Issue: Missing return value.
- Details: The function does not return the required
bytes4value.
-
ExcessiveReturnValueRecipient.sol- Issue: Additional return value.
- Details: The function returns both
bytes4anduint256, deviating from the interface specification.
-
Other Implementations (
IncorrectReturnValueRecipient.sol,RecipientMock.sol,NonPayableRecipient.sol)- Review Needed: Ensure these implementations correctly handle return values and access controls as per the interface.
Recommendations:
-
Standardize Return Values:
- Ensure all implementations of
fastBridgeTransferReceivedreturn only thebytes4value as defined in theIFastBridgeRecipientinterface.
- Ensure all implementations of
-
Implement Access Controls:
- Review and enforce appropriate access controls in each implementation to restrict function calls to authorized entities.
-
Conduct Comprehensive Testing:
- Perform thorough testing on all implementations to validate compliance with the interface and to identify any potential security vulnerabilities.
🔗 Analysis chain
Consider security implications and potential improvements.
While the function signature is well-structured, there are some security considerations and potential improvements to consider:
-
Access Control: There's no explicit check for who can call this function. Consider adding a modifier or check to ensure only authorized addresses (e.g., the bridge contract) can call this function.
-
Payable Function: The function is marked as payable, allowing it to receive Ether. Ensure this is intentional and necessary for all cases. If not always needed, consider creating separate functions for Ether and non-Ether transfers.
-
Return Value: The bytes4 return type is common, but consider using a more specific return value or enum to indicate different states of the transfer (e.g., success, failure due to insufficient balance, etc.).
-
Parameter Validation: Add input validation for the
amountparameter to prevent potential overflow issues. -
Reentrancy: If this function performs state changes or transfers, ensure it's protected against reentrancy attacks.
To verify the usage and implementation of this interface across the project, you can run the following script:
This will help ensure consistent implementation and usage across the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations and usages of IFastBridgeRecipient
echo "Searching for implementations of IFastBridgeRecipient:"
rg --type solidity "contract.*is.*IFastBridgeRecipient"
echo "\nSearching for usages of fastBridgeTransferReceived function:"
rg --type solidity "fastBridgeTransferReceived\("
Length of output: 429
Script:
#!/bin/bash
# Searching for implementations of IFastBridgeRecipient:
rg -g '*.sol' "contract\s+.*\s+is\s+.*IFastBridgeRecipient"
# Searching for usages of fastBridgeTransferReceived function:
rg -g '*.sol' "fastBridgeTransferReceived\("
Length of output: 1419
| function test_bridge_token_revert_callParamsLengthAboveMax() public { | ||
| bytes memory callParams = new bytes(2 ** 16); | ||
| setTokenTestCallParams(callParams); | ||
| vm.expectRevert(CallParamsLengthAboveMax.selector); | ||
| bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2}); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate revert tests to improve test suite efficiency
The functions test_bridge_token_revert_callParamsLengthAboveMax and test_bridge_eth_revert_callParamsLengthAboveMax are nearly identical. Combining them into a single function can simplify the test code and make future maintenance easier.
Here's how you might refactor these tests:
-function test_bridge_token_revert_callParamsLengthAboveMax() public {
- bytes memory callParams = new bytes(2 ** 16);
- setTokenTestCallParams(callParams);
- vm.expectRevert(CallParamsLengthAboveMax.selector);
- bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2});
-}
-
-function test_bridge_eth_revert_callParamsLengthAboveMax() public {
- bytes memory callParams = new bytes(2 ** 16);
- setEthTestCallParams(callParams);
- vm.expectRevert(CallParamsLengthAboveMax.selector);
- bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2});
-}
+function test_bridge_revert_callParamsLengthAboveMax(bool isToken) public {
+ bytes memory callParams = new bytes(2 ** 16);
+ if (isToken) {
+ setTokenTestCallParams(callParams);
+ vm.expectRevert(CallParamsLengthAboveMax.selector);
+ bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2});
+ } else {
+ setEthTestCallParams(callParams);
+ vm.expectRevert(CallParamsLengthAboveMax.selector);
+ bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2});
+ }
+}Also applies to: 38-43
| function test_bridge_token_callParamsLengthMax() public { | ||
| bytes memory callParams = new bytes(2 ** 16 - 1); | ||
| setTokenTestCallParams(callParams); | ||
| test_bridge_token(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor similar test functions to reduce code duplication
The functions test_bridge_token_callParamsLengthMax and test_bridge_eth_callParamsLengthMax have similar implementations. Consider refactoring them into a single parameterized function to enhance maintainability and reduce redundancy.
Apply this diff to refactor the tests:
-function test_bridge_token_callParamsLengthMax() public {
- bytes memory callParams = new bytes(2 ** 16 - 1);
- setTokenTestCallParams(callParams);
- test_bridge_token();
-}
-
-function test_bridge_eth_callParamsLengthMax() public {
- bytes memory callParams = new bytes(2 ** 16 - 1);
- setEthTestCallParams(callParams);
- test_bridge_eth();
-}
+function test_bridge_callParamsLengthMax(bool isToken) public {
+ bytes memory callParams = new bytes(2 ** 16 - 1);
+ if (isToken) {
+ setTokenTestCallParams(callParams);
+ test_bridge_token();
+ } else {
+ setEthTestCallParams(callParams);
+ test_bridge_eth();
+ }
+}Committable suggestion was skipped due to low confidence.
| function test_getBridgeTransaction_supportsV2(IFastBridgeV2.BridgeTransactionV2 memory bridgeTxV2) public { | ||
| // TODO: reevaluate the necessity of this test if/when the encoding scheme is changed | ||
| vm.skip(true); |
There was a problem hiding this comment.
Consider addressing the skipped test in test_getBridgeTransaction_supportsV2
The test function test_getBridgeTransaction_supportsV2 is currently being skipped using vm.skip(true);. Skipping tests can lead to reduced test coverage and potential issues might go unnoticed. Please consider implementing this test or, if it's no longer necessary due to the encoding compatibility issues, removing it to maintain a clean codebase.
| function setTokenTestCallParams(bytes memory callParams) public { | ||
| tokenParamsV2.callParams = callParams; | ||
| tokenTx.callParams = callParams; | ||
| } | ||
|
|
||
| function setTokenTestExclusivityParams(address relayer, uint256 exclusivitySeconds) public { | ||
| tokenParamsV2.quoteRelayer = relayer; | ||
| tokenParamsV2.quoteExclusivitySeconds = int256(exclusivitySeconds); | ||
| tokenParamsV2.quoteId = bytes.concat("Token:", getMockQuoteId(relayer)); | ||
|
|
||
| tokenTx.exclusivityRelayer = relayer; | ||
| tokenTx.exclusivityEndTime = block.timestamp + exclusivitySeconds; | ||
| } | ||
|
|
||
| function setEthTestCallParams(bytes memory callParams) public { | ||
| ethParamsV2.callParams = callParams; | ||
| ethTx.callParams = callParams; | ||
| } | ||
|
|
||
| function setEthTestExclusivityParams(address relayer, uint256 exclusivitySeconds) public { | ||
| ethParamsV2.quoteRelayer = relayer; | ||
| ethParamsV2.quoteExclusivitySeconds = int256(exclusivitySeconds); | ||
| ethParamsV2.quoteId = bytes.concat("ETH:", getMockQuoteId(relayer)); | ||
|
|
||
| ethTx.exclusivityRelayer = relayer; | ||
| ethTx.exclusivityEndTime = block.timestamp + exclusivitySeconds; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication
The functions setTokenTestCallParams, setEthTestCallParams, setTokenTestExclusivityParams, and setEthTestExclusivityParams have similar logic. Consider creating generic helper functions to reduce code duplication and improve maintainability.
Here's how you could refactor the code:
- Create internal helper functions:
function _setTestCallParams(
IFastBridgeV2.BridgeParamsV2 storage paramsV2,
IFastBridgeV2.BridgeTransactionV2 storage tx,
bytes memory callParams
) internal {
paramsV2.callParams = callParams;
tx.callParams = callParams;
}
function _setTestExclusivityParams(
IFastBridgeV2.BridgeParamsV2 storage paramsV2,
IFastBridgeV2.BridgeTransactionV2 storage tx,
string memory prefix,
address relayer,
uint256 exclusivitySeconds
) internal {
paramsV2.quoteRelayer = relayer;
paramsV2.quoteExclusivitySeconds = int256(exclusivitySeconds);
paramsV2.quoteId = bytes.concat(bytes(prefix), getMockQuoteId(relayer));
tx.exclusivityRelayer = relayer;
tx.exclusivityEndTime = block.timestamp + exclusivitySeconds;
}- Refactor the existing functions to use the helpers:
function setTokenTestCallParams(bytes memory callParams) public {
- tokenParamsV2.callParams = callParams;
- tokenTx.callParams = callParams;
+ _setTestCallParams(tokenParamsV2, tokenTx, callParams);
}
function setEthTestCallParams(bytes memory callParams) public {
- ethParamsV2.callParams = callParams;
- ethTx.callParams = callParams;
+ _setTestCallParams(ethParamsV2, ethTx, callParams);
}
function setTokenTestExclusivityParams(address relayer, uint256 exclusivitySeconds) public {
- tokenParamsV2.quoteRelayer = relayer;
- tokenParamsV2.quoteExclusivitySeconds = int256(exclusivitySeconds);
- tokenParamsV2.quoteId = bytes.concat("Token:", getMockQuoteId(relayer));
- tokenTx.exclusivityRelayer = relayer;
- tokenTx.exclusivityEndTime = block.timestamp + exclusivitySeconds;
+ _setTestExclusivityParams(tokenParamsV2, tokenTx, "Token:", relayer, exclusivitySeconds);
}
function setEthTestExclusivityParams(address relayer, uint256 exclusivitySeconds) public {
- ethParamsV2.quoteRelayer = relayer;
- ethParamsV2.quoteExclusivitySeconds = int256(exclusivitySeconds);
- ethParamsV2.quoteId = bytes.concat("ETH:", getMockQuoteId(relayer));
- ethTx.exclusivityRelayer = relayer;
- ethTx.exclusivityEndTime = block.timestamp + exclusivitySeconds;
+ _setTestExclusivityParams(ethParamsV2, ethTx, "ETH:", relayer, exclusivitySeconds);
}| // solhint-disable func-name-mixedcase, ordering | ||
| contract FastBridgeV2DstArbitraryCallTest is FastBridgeV2DstExclusivityTest { | ||
| bytes public constant CALL_PARAMS = abi.encode("Hello, world!"); | ||
| bytes public constant REVERT_MSG = "GM, this is a revert"; |
There was a problem hiding this comment.
Incorrect declaration of 'REVERT_MSG'
At line 12, REVERT_MSG is declared as bytes but assigned a string literal. In Solidity, string literals are of type string, and assigning them directly to a bytes variable can cause a type mismatch.
Suggested Fix:
Declare REVERT_MSG as a string:
- bytes public constant REVERT_MSG = "GM, this is a revert";
+ string public constant REVERT_MSG = "GM, this is a revert";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bytes public constant REVERT_MSG = "GM, this is a revert"; | |
| string public constant REVERT_MSG = "GM, this is a revert"; |
| } | ||
|
|
||
| function mockRecipientRevert(IFastBridgeV2.BridgeTransactionV2 memory bridgeTx) public { | ||
| vm.mockCallRevert({callee: userB, data: getExpectedCalldata(bridgeTx), revertData: bytes(REVERT_MSG)}); |
There was a problem hiding this comment.
Unnecessary casting of 'REVERT_MSG' to bytes
In line 43, bytes(REVERT_MSG) is used to cast REVERT_MSG to bytes. Since REVERT_MSG should be declared as a string (as per the previous suggestion), casting it to bytes here is appropriate. However, if REVERT_MSG remains as bytes, the cast is redundant.
Suggested Fix (if REVERT_MSG is a string):
No change needed in line 43 if REVERT_MSG is declared as a string.
Suggested Fix (if REVERT_MSG is kept as bytes):
Remove the redundant cast:
- vm.mockCallRevert({callee: userB, data: getExpectedCalldata(bridgeTx), revertData: bytes(REVERT_MSG)});
+ vm.mockCallRevert({callee: userB, data: getExpectedCalldata(bridgeTx), revertData: REVERT_MSG});Committable suggestion was skipped due to low confidence.
| test_relay_token_withRelayerAddress(); | ||
| } | ||
|
|
||
| function test_relay_eth_incorrectReturnValueRecipient_revertWhenCallParamsPresent() public virtual { | ||
| assertEmptyCallParams(ethTx.callParams); | ||
| setEthTestRecipient(incorrectReturnValueRecipient); | ||
| test_relay_eth(); | ||
| } | ||
|
|
||
| function test_relay_eth_withRelayerAddress_incorrectReturnValueRecipient_revertWhenCallParamsPresent() | ||
| public | ||
| virtual | ||
| { | ||
| assertEmptyCallParams(ethTx.callParams); | ||
| setEthTestRecipient(incorrectReturnValueRecipient); | ||
| test_relay_eth_withRelayerAddress(); | ||
| } | ||
|
|
||
| // ═══════════════════════════════════════════ NON PAYABLE RECIPIENT ═══════════════════════════════════════════════ | ||
|
|
||
| /// @notice Should not affect the ERC20 transfer | ||
| function test_relay_token_nonPayableRecipient() public { | ||
| setTokenTestRecipient(nonPayableRecipient); | ||
| test_relay_token(); | ||
| } | ||
|
|
||
| function test_relay_token_withRelayerAddress_nonPayableRecipient() public { | ||
| setTokenTestRecipient(nonPayableRecipient); | ||
| test_relay_token_withRelayerAddress(); | ||
| } | ||
|
|
||
| function test_relay_eth_revert_nonPayableRecipient() public { | ||
| setEthTestRecipient(nonPayableRecipient); | ||
| vm.expectRevert(); | ||
| relay({caller: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); | ||
| } | ||
|
|
||
| function test_relay_eth_withRelayerAddress_revert_nonPayableRecipient() public { | ||
| setEthTestRecipient(nonPayableRecipient); | ||
| vm.expectRevert(); | ||
| relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); | ||
| } | ||
|
|
||
| // ══════════════════════════════════════════════ NO-OP RECIPIENT ══════════════════════════════════════════════════ | ||
|
|
||
| // Note: in this test, the callParams are not present, and the below test functions succeed. | ||
| // Override them in the derived tests where callParams are present to check for a revert. | ||
|
|
||
| function test_relay_token_noOpRecipient_revertWhenCallParamsPresent() public virtual { | ||
| assertEmptyCallParams(tokenTx.callParams); | ||
| setTokenTestRecipient(noOpRecipient); | ||
| test_relay_token(); | ||
| } | ||
|
|
||
| function test_relay_token_withRelayerAddress_noOpRecipient_revertWhenCallParamsPresent() public virtual { | ||
| assertEmptyCallParams(tokenTx.callParams); | ||
| setTokenTestRecipient(noOpRecipient); | ||
| test_relay_token_withRelayerAddress(); | ||
| } | ||
|
|
||
| function test_relay_eth_noOpRecipient_revertWhenCallParamsPresent() public virtual { | ||
| assertEmptyCallParams(ethTx.callParams); | ||
| setEthTestRecipient(noOpRecipient); | ||
| test_relay_eth(); | ||
| } | ||
|
|
||
| function test_relay_eth_withRelayerAddress_noOpRecipient_revertWhenCallParamsPresent() public virtual { | ||
| assertEmptyCallParams(ethTx.callParams); | ||
| setEthTestRecipient(noOpRecipient); | ||
| test_relay_eth_withRelayerAddress(); | ||
| } | ||
|
|
||
| // ═════════════════════════════════════════ NO RETURN VALUE RECIPIENT ═════════════════════════════════════════════ | ||
|
|
||
| // Note: in this test, the callParams are not present, and the below test functions succeed. | ||
| // Override them in the derived tests where callParams are present to check for a revert. | ||
|
|
||
| function test_relay_token_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual { | ||
| assertEmptyCallParams(tokenTx.callParams); | ||
| setTokenTestRecipient(noReturnValueRecipient); | ||
| test_relay_token(); | ||
| } | ||
|
|
||
| function test_relay_token_withRelayerAddress_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual { | ||
| assertEmptyCallParams(tokenTx.callParams); | ||
| setTokenTestRecipient(noReturnValueRecipient); | ||
| test_relay_token_withRelayerAddress(); | ||
| } | ||
|
|
||
| function test_relay_eth_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual { | ||
| assertEmptyCallParams(ethTx.callParams); | ||
| setEthTestRecipient(noReturnValueRecipient); | ||
| test_relay_eth(); | ||
| } | ||
|
|
||
| function test_relay_eth_withRelayerAddress_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual { | ||
| assertEmptyCallParams(ethTx.callParams); | ||
| setEthTestRecipient(noReturnValueRecipient); | ||
| test_relay_eth_withRelayerAddress(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring repetitive test functions to reduce duplication
Many test functions between lines 159-305 share similar structures, differing mainly in the recipient type and the specific test function called. Refactoring these tests using helper functions or parameterized tests could reduce code duplication and enhance maintainability.
For example, you could implement a generic test function that accepts parameters for the recipient address and the expected behavior, which can be reused for different scenarios.
| // TODO: the note below isn't true anymore with the BridgeTransactionV2 struct | ||
| // since the variable length `callParams` was added. This needs to be fixed/acknowledged. | ||
|
|
There was a problem hiding this comment.
Update the outdated comment regarding BridgeTransactionV2
The TODO comment indicates that the note about decoding V1 fields is no longer accurate due to the addition of variable-length callParams. Please revise or remove this comment to reflect the current implementation.
Would you like assistance in updating this comment to accurately describe the current behavior?
| @@ -132,6 +145,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { | |||
| if (params.sender == address(0) || params.to == address(0)) revert ZeroAddress(); | |||
| if (params.originToken == address(0) || params.destToken == address(0)) revert ZeroAddress(); | |||
| if (params.deadline < block.timestamp + MIN_DEADLINE_PERIOD) revert DeadlineTooShort(); | |||
| if (paramsV2.callParams.length > MAX_CALL_PARAMS_LENGTH) revert CallParamsLengthAboveMax(); | |||
There was a problem hiding this comment.
Ensure CallParamsLengthAboveMax error is defined
The code uses revert CallParamsLengthAboveMax();, but there is no visible definition of the CallParamsLengthAboveMax error. Make sure this custom error is declared appropriately in the contract or imported from the correct interface.
Add the error definition to the contract:
+ error CallParamsLengthAboveMax();Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 26
🧹 Outside diff range and nitpick comments (24)
packages/contracts-rfq/test/mocks/NoOpContract.sol (1)
4-4: Consider adding a brief explanation for disabling the Solhint rule.While disabling the no-empty-blocks rule is appropriate for this mock contract, it's beneficial to provide a brief explanation in the comment. This helps other developers understand the intention behind disabling the rule.
Consider updating the comment as follows:
-// solhint-disable-next-line no-empty-blocks +// solhint-disable-next-line no-empty-blocks -- Intentionally empty mock contractpackages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol (2)
1-2: LGTM! Consider specifying a more precise Solidity version.The SPDX license identifier and pragma solidity version are correctly placed. However, for better version control and to avoid potential future incompatibilities, consider specifying a more precise Solidity version range.
You might want to update the pragma statement to a more specific version range, for example:
pragma solidity >=0.8.0 <0.9.0;This ensures compatibility with Solidity 0.8.x versions while preventing potential issues with future major releases.
4-13: LGTM! Consider adding NatSpec documentation.The interface and function declarations are well-structured and follow Solidity conventions. The function modifiers (external, payable) are appropriate for the intended use.
To improve code readability and provide better documentation, consider adding NatSpec comments to the interface and function. For example:
/// @title IFastBridgeRecipient /// @notice Interface for contracts that can receive tokens via a fast bridge interface IFastBridgeRecipient { /// @notice Handles the receipt of tokens transferred via a fast bridge /// @param token The address of the token being transferred /// @param amount The amount of tokens being transferred /// @param callParams Additional parameters for the transfer /// @return A bytes4 value indicating the result of the operation function fastBridgeTransferReceived( address token, uint256 amount, bytes memory callParams ) external payable returns (bytes4); }packages/contracts-rfq/test/mocks/NonPayableRecipient.sol (1)
6-9: LGTM: Function implementation is correct for testing purposes.The
fastBridgeTransferReceivedfunction is correctly implemented as a mock. A few observations:
- The function is properly marked as
externalandpure.- Returning the function selector is a common pattern for interface implementations.
- The notice comment clearly states that the function is incorrectly implemented (not payable) for testing purposes.
Minor suggestion: Consider naming the function parameters for improved readability, even in test contracts.
packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol (2)
4-4: Consider using more specific linter rule disables.The
solhint-disabledirective is currently disabling all linter rules for the entire file. It's generally better to disable only specific rules that are necessary, and preferably only for the relevant code sections. This helps maintain code quality and catch potential issues early.Consider replacing the global disable with specific rule disables as needed, or add a comment explaining why all rules need to be disabled for this file.
11-12: LGTM: Correctly implemented mock function with a minor suggestion.The
fastBridgeTransferReceived()function is correctly implemented as a mock that doesn't return any value, which aligns with the contract's purpose. The natspec comment clearly indicates that this is an incorrect implementation, which is crucial for testing purposes.Consider adding a comment within the function body to explain why it's intentionally left empty. For example:
function fastBridgeTransferReceived(address, uint256, bytes memory) external payable { // Intentionally left empty for testing purposes }This can help prevent confusion for developers who might come across this code in the future.
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (3)
6-6: LGTM. Consider adding parameters for more informative errors.The new
CallParamsLengthAboveMaxerror is a good addition for handling cases where call parameters exceed a maximum length.Consider adding parameters to provide more context, such as:
error CallParamsLengthAboveMax(uint256 actualLength, uint256 maxLength);This would make debugging easier by providing specific information about the error condition.
14-14: LGTM. Consider adding parameters for more detailed error reporting.The new
RecipientIncorrectReturnValueerror is a good addition for handling cases where a recipient returns an incorrect value.To enhance debugging capabilities, consider adding parameters to provide more context:
error RecipientIncorrectReturnValue(bytes expectedValue, bytes actualValue);This would allow for more precise error reporting and easier troubleshooting.
6-16: Consider reorganizing errors for better readability.While the new errors are good additions, their placement within the interface could be improved for better readability and maintainability.
Consider the following suggestions:
- Group related errors together. For example, place
RecipientIncorrectReturnValueandRecipientNoReturnValuenext to each other.- Consider organizing all errors alphabetically for easier lookup.
- Remove the empty line (line 16) to maintain consistent spacing throughout the interface.
These changes would enhance the overall structure and make it easier to navigate the interface as it grows.
packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol (3)
6-7: LGTM: Clear contract declaration and documentation.The contract name is descriptive and the documentation clearly states its purpose as a mock for testing. The warning against production use is crucial.
Consider adding a brief explanation of why this mock is "incorrectly implemented" in the comment to provide more context for other developers.
11-14: LGTM: Correctly implemented mock function with intentional error.The
fastBridgeTransferReceivedfunction is implemented as intended for testing purposes, returning excessive data. The comment clearly indicates that this is an incorrect implementation.Consider adding a comment explaining the specific way in which this implementation is incorrect (i.e., returning an additional uint256 value that's not part of the interface). This would provide more clarity for developers using this mock.
1-15: Overall: Well-implemented mock contract for testing purposes.This mock contract is well-structured, properly documented, and serves its intended purpose for testing. The intentional errors are clearly marked, and the contract adheres to Solidity best practices. The warnings against production use are appropriately placed.
Ensure that the tests using this mock contract explicitly check for the excessive return value to validate error handling in the main contract. This will help maintain the robustness of your testing suite.
packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol (2)
11-12: Improved test setup with dedicated functionsThe changes in
createFixturesV2()enhance the code structure by introducing dedicated functions for setting up token and ETH exclusivity parameters. This approach improves readability and maintainability of the test setup.Consider adding comments to explain the purpose of
setTokenTestExclusivityParamsandsetEthTestExclusivityParamsfunctions if they are not already documented in the parent class.
Line range hint
15-18: Improved flexibility in bridge parameter handlingThe changes in the
bridge()function enhance its flexibility by allowing different parameters to be used based on the origin token type. This is a good improvement in terms of functionality.Consider adding a brief comment explaining the difference between
ethParamsV2andtokenParamsV2for improved clarity. Also, you might want to split the ternary operator into an if-else statement for better readability, especially if more complex logic might be added in the future.packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (1)
Line range hint
1-180: Consider adding tests for the newcallParamsfunctionalityWhile the existing tests remain valid after the addition of the
callParamsfield, it's important to ensure that this new functionality is properly tested. Consider adding new test cases that:
- Verify the correct handling of non-empty
callParamsin both token and ETH relay operations.- Test edge cases related to
callParams, such as very long strings or special characters.- Ensure that the
callParamsfield is correctly passed through the entire bridge process.These additional tests will help maintain the robustness of the FastBridgeV2 contract and prevent potential issues related to the new field.
Would you like assistance in drafting these additional test cases?
packages/contracts-rfq/CHANGELOG.md (1)
6-11: LGTM! Consider adding more detail to the feature description.The new version entry (0.7.0) is well-formatted and follows the conventional commit guidelines. The link to compare versions and the issue reference are correctly included.
To improve clarity for users and developers, consider expanding on the feature description. For example:
"Added support for arbitrary calls without requiring additional native value, enhancing flexibility in contract interactions."🧰 Tools
🪛 Markdownlint
9-9: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (3)
Line range hint
107-112: LGTM! Consider adding a comment for clarity.The refactoring improves the readability of the test by using the
setTokenTestExclusivityParamshelper function. This change encapsulates the exclusivity parameter setup, making the test more maintainable.Consider adding a brief comment explaining the purpose of
setTokenTestExclusivityParamsfor better documentation:+ // Set up exclusivity parameters for the token bridge test setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD);
Line range hint
188-193: LGTM! Consider adding a comment for consistency.The refactoring improves the readability of the ETH bridging test by using the
setEthTestExclusivityParamshelper function. This change is consistent with the token bridging test and enhances maintainability.For consistency with the token bridging test, consider adding a brief comment explaining the purpose of
setEthTestExclusivityParams:+ // Set up exclusivity parameters for the ETH bridge test setEthTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD);
Line range hint
1-265: Consider further test structure improvements in the future.The recent changes to
test_bridge_token_withExclusivityandtest_bridge_eth_withExclusivityhave improved the code's readability and maintainability. To build on this improvement, consider the following suggestions for future refactoring:
- Implement a
beforeEachor similar setup function to reduce duplication in test setups.- Group related tests using
describeblocks (if supported by your testing framework) to improve organization.- Consider parameterizing tests for different scenarios to reduce code duplication further.
These suggestions aim to enhance the overall structure and maintainability of the test suite in the long term.
packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (2)
50-55: Consider rephrasing comments for clarity and professionalismThe comments contain informal language ("This is weird, but it is what it is.") which can be rephrased to maintain a formal tone and enhance clarity.
Suggested change:
-// which is ALWAYS equal to 32 (data starts right after the offset). This is weird, but it is what it is. +// which is always equal to 32 (data starts immediately after the offset). This behavior is due to Solidity's encoding of dynamic types.
Line range hint
56-62: Reevaluate or remove the skipped testThe test
test_getBridgeTransaction_supportsV2is currently skipped usingvm.skip(true);and contains a TODO note to reevaluate its necessity. Keeping skipped tests can clutter the test suite and may lead to confusion.Consider the following actions:
- If the test is no longer needed, remove it to keep the codebase clean.
- If the test is important for future changes, consider implementing it or creating an issue to track this task.
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
Line range hint
46-62: Address the TODO regarding encoding scheme to optimize gas costsThe TODO comment suggests considering changes to the encoding scheme to prevent extra gas consumption during decoding. With the addition of
callParams, revisiting the encoding strategy forBridgeTransactionV2could improve gas efficiency.Would you like assistance in proposing an optimized encoding scheme that could reduce gas costs?
packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol (2)
14-19: Add NatSpec documentation tocreateFixtures()Consider adding NatSpec comments to the
createFixtures()function to improve code readability and provide clarity on its purpose in the test setup.
21-25: Add NatSpec documentation tocreateFixturesV2()Adding NatSpec comments to the
createFixturesV2()function would enhance maintainability by explaining its role in initializing test parameters.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (22)
- packages/contracts-rfq/CHANGELOG.md (1 hunks)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (9 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (1 hunks)
- packages/contracts-rfq/package.json (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (3 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (3 hunks)
- packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/NoOpContract.sol (1 hunks)
- packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/NonPayableRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/RecipientMock.sol (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/contracts-rfq/package.json
🧰 Additional context used
🪛 Markdownlint
packages/contracts-rfq/CHANGELOG.md
9-9: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
🔇 Additional comments (32)
packages/contracts-rfq/test/mocks/NoOpContract.sol (3)
1-2: LGTM: Appropriate license and Solidity version.The MIT license is suitable for open-source projects, and the Solidity version (^0.8.0) is recent and allows for minor updates.
5-6: LGTM: Clear contract name and purpose.The contract name
NoOpContractclearly indicates its purpose, and the docstring provides a concise explanation of why the contract needs to accept ETH. This is helpful for developers who might use this mock contract in tests.
7-7: LGTM: Correctly implemented receive function.The
receivefunction is properly implemented as an external payable function with an empty body. This allows the mock contract to accept ETH transactions without performing any operations, which is the intended behavior.packages/contracts-rfq/test/mocks/NonPayableRecipient.sol (2)
4-5: LGTM: Contract declaration is clear and well-documented.The contract name
NonPayableRecipientclearly indicates its purpose, and the notice comment explicitly states it's for testing purposes only. This is good practice for mock contracts.
1-10: LGTM: Well-structured mock contract for testing purposes.This
NonPayableRecipientcontract is well-designed for its intended use as a test mock:
- It's concise and focused on a single purpose.
- The contract and its function are clearly documented as test-only and intentionally incorrect.
- The implementation serves its purpose for testing scenarios where a non-payable recipient is needed.
The contract is suitable for its intended use in testing and poses no risk as long as it's not used in production environments.
packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol (2)
6-7: LGTM: Clear contract naming and documentation.The contract name
NoReturnValueRecipientis descriptive, and the natspec comment provides crucial information about its purpose and limitations. The explicit warning against production use is an important safeguard.
8-9: LGTM: Correctly implementedreceive()function.The
receive()function is properly implemented to accept ETH payments, which is necessary for this mock contract. The natspec comment provides clear context for its purpose.packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol (1)
16-17: Excellent refactoring of exclusivity parameter setup!The changes improve code readability and maintainability by consolidating the logic for setting exclusivity parameters into dedicated functions. Using the
EXCLUSIVITY_PERIODconstant ensures consistency across the contract.packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (1)
15-15: LGTM. Clear and concise error definition.The new
RecipientNoReturnValueerror is a good addition for handling cases where a recipient doesn't return a value when one was expected. The error is self-explanatory and doesn't require additional parameters.packages/contracts-rfq/test/mocks/RecipientMock.sol (4)
1-7: LGTM: Contract structure and interface implementation are well-defined.The contract structure follows Solidity best practices, including proper SPDX license identifier, pragma statement, and clear import of the interface. The contract name and purpose are explicitly stated, which is good for maintainability.
8-9: LGTM: Receive function is correctly implemented.The
receive()function is properly implemented asexternal payable, allowing the contract to accept ETH. The accompanying comment clearly explains its purpose in the context of this mock contract.
11-14: LGTM: fastBridgeTransferReceived function is correctly implemented for a mock.The
fastBridgeTransferReceivedfunction is properly implemented asexternal payableand matches the interface signature. The minimal implementation, which returns the function selector, is sufficient for testing purposes and ensures interface compliance.
1-15: Overall, the RecipientMock contract is well-implemented for testing purposes.The contract successfully implements the IFastBridgeRecipient interface with minimal, but sufficient implementations for testing. It's clearly marked as a mock for testing, which is good practice. The code is clean, well-commented, and follows Solidity best practices.
packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol (2)
1-4: LGTM: Appropriate license, Solidity version, and import statement.The contract header is well-structured with the correct license identifier and Solidity version. The import statement for the
IFastBridgeRecipientinterface is correctly implemented.
8-9: LGTM: Correctly implemented receive function with clear documentation.The
receive()function is properly implemented to allow the contract to accept ETH. The comment clearly explains the purpose of this function in the context of the mock contract.packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol (3)
1-7: LGTM: Clear contract purpose and implementation.The contract declaration and purpose are well-defined. The explicit warning about not using this mock in production is a good practice to prevent misuse.
8-9: LGTM: Correctly implemented receive function.The receive function is properly implemented to allow the contract to accept ETH, which is necessary for testing scenarios.
11-15: LGTM: Correctly implemented mock function for testing.The
fastBridgeTransferReceivedfunction is intentionally implemented to return an incorrect value by flipping the last bit of the selector. This aligns with the contract's purpose as a mock for testing incorrect implementations.To ensure this mock is only used in test files, run the following script:
packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (1)
14-15: Update toBridgeParamsV2structure: AddedcallParamsfieldThe
createFixturesV2function has been updated to include a newcallParamsfield in bothtokenParamsV2andethParamsV2structures. Additionally, thequoteIdfield is now explicitly set to an empty string.These changes suggest an update to the
BridgeParamsV2structure in the main contract. While they don't affect the logic of the existing tests, please ensure that:
- The main
FastBridgeV2contract has been updated to include thecallParamsfield in itsBridgeParamsV2structure.- Any functions or logic depending on the
BridgeParamsV2structure in the main contract have been adjusted to handle the newcallParamsfield.- The explicit setting of
quoteIdto an empty string aligns with the expected behavior in the main contract.To verify these changes, please run the following script:
Also applies to: 20-21
✅ Verification successful
Verification Successful:
callParamsField Properly Added and UtilizedThe
BridgeParamsV2structure inIFastBridgeV2.solhas been successfully updated to include thecallParamsfield. Additionally, this field is correctly utilized across multiple test files within thepackages/contracts-rfq/test/directory.
Struct Update:
callParamshas been added to theBridgeParamsV2structure incontracts/interfaces/IFastBridgeV2.sol.Test Coverage:
- The
callParamsfield is appropriately set and tested in various test cases, ensuring alignment with the updated contract structure.No issues were found regarding the absence of removed or replaced code in the specified lines of
FastBridgeV2.Dst.Exclusivity.t.sol.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the changes in the FastBridgeV2 contract and related files # Check for the updated BridgeParamsV2 structure in the main contract echo "Checking for updated BridgeParamsV2 structure:" rg --type solidity -A 10 "struct BridgeParamsV2" packages/contracts-rfq/src/ # Check for any usage of callParams in the main contract echo "\nChecking for usage of callParams:" rg --type solidity "callParams" packages/contracts-rfq/src/ # Check for any other test files that might need similar updates echo "\nChecking for other test files that might need updates:" rg --type solidity "BridgeParamsV2" packages/contracts-rfq/test/Length of output: 609
Script:
#!/bin/bash # Description: Verify the changes in the FastBridgeV2 contract and related files # Check for the updated BridgeParamsV2 structure in the main contract echo "Checking for updated BridgeParamsV2 structure:" rg -g "*.sol" -A 10 "struct BridgeParamsV2" packages/contracts-rfq/src/ # Check for any usage of callParams in the main contract echo "\nChecking for usage of callParams:" rg -g "*.sol" "callParams" packages/contracts-rfq/src/ # Check for any other test files that might need similar updates echo "\nChecking for other test files that might need updates:" rg -g "*.sol" "BridgeParamsV2" packages/contracts-rfq/test/Length of output: 1757
Script:
#!/bin/bash # Description: Verify the changes in the FastBridgeV2 contract and related files by searching the entire repository. # Check for the updated BridgeParamsV2 structure in the main contract echo "Checking for updated BridgeParamsV2 structure across the repository:" rg -g "*.sol" -A 10 "struct BridgeParamsV2" # Check for any usage of callParams in the repository echo "\nChecking for usage of callParams across the repository:" rg -g "*.sol" "callParams" # Check for any other test files that might need similar updates echo "\nChecking for other test files that might need updates:" rg -g "*.sol" "BridgeParamsV2" packages/contracts-rfq/test/Length of output: 10513
packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol (1)
1-44: Tests are well-implementedThe tests correctly verify the bridge's behavior with call parameters at the boundary lengths, ensuring proper handling of maximum allowed sizes and correct reversion when exceeding limits.
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (3)
36-41: Addition ofcallParamstoBridgeParamsV2enhances functionalityThe inclusion of
bytes callParamsinBridgeParamsV2allows passing arbitrary call parameters to the destination recipient, increasing flexibility and potential use cases.
39-39: Verify the need forint256type forquoteExclusivitySecondsThe
quoteExclusivitySecondsparameter is currently defined asint256. Since it represents a period of time (duration), should this be auint256to prevent negative values and align with typical usage of time intervals?
62-62: Inclusion ofcallParamsinBridgeTransactionV2Adding
bytes callParamstoBridgeTransactionV2ensures that arbitrary call parameters are included in the bridge transaction, enhancing the functionality and flexibility of the bridge.packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
213-221:getMockQuoteIdfunction is correctly implementedThe function appropriately returns mock quote IDs based on the provided relayer address.
packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol (8)
59-81: Comprehensive tests for recipient not being a contractThe test functions
test_relay_token_revert_recipientNotContract()andtest_relay_eth_revert_recipientNotContract()correctly verify that the relay function reverts when the recipient is not a contract, ensuring robust error handling.
85-116: Thorough testing of excessive return value recipientsThe tests for handling recipients with excessive return values are well-designed, ensuring that the contract correctly reverts when unexpected return data is received.
119-150: Validating behavior with incorrect return value recipientsThe test cases cover scenarios where recipients return incorrect values, which is crucial for ensuring that the relay function handles such cases appropriately.
153-176: Handling no-operation recipientsThe tests effectively verify that the relay function reverts when interacting with recipients that perform no operation, ensuring consistent behavior.
179-210: Testing recipients with no return valueThe included tests for recipients that do not return any value are important for verifying that the relay function handles these scenarios as expected.
213-235: Simulating recipient revertsThe
test_relay_token_revert_recipientReverts()and related functions correctly mock recipient reverts and ensure that the relay function processes these reverts appropriately.
237-249: Consistent handling of no call parameters with recipient revertsThe tests for relaying ETH without call parameters and handling recipient reverts ensure that the relay function maintains consistent error handling in various scenarios.
38-40: Ensure expected call is properly configured inexpectBridgeRelayed()The overridden
expectBridgeRelayed()function correctly adds an expectation for the arbitrary call to the recipient. Ensure that thegetExpectedCalldata()function accurately reflects the intended calldata structure.Run the following script to verify that
getExpectedCalldata()matches the actual function signature:
| function fastBridgeTransferReceived( | ||
| address token, | ||
| uint256 amount, | ||
| bytes memory callParams | ||
| ) | ||
| external | ||
| payable | ||
| returns (bytes4); |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Implementations of fastBridgeTransferReceived Function
Several implementations of IFastBridgeRecipient exhibit inconsistencies in the fastBridgeTransferReceived function:
IncorrectReturnValueRecipient.sol: Returns a modifiedbytes4selector.NonPayableRecipient.sol: Declares the function aspureinstead ofpayableand returns the selector.ExcessiveReturnValueRecipient.sol: Returns bothbytes4anduint256, deviating from the expected return type.NoReturnValueRecipient.sol: Does not return any value, contrary to the interface specification.
These discrepancies can lead to security vulnerabilities and unexpected behaviors within the contracts.
🔗 Analysis chain
Function parameters and return type look good. Consider access control implications.
The function parameters (token, amount, callParams) and return type (bytes4) are well-chosen for a flexible token transfer receipt mechanism. The use of a bytes parameter for callParams allows for extensibility.
However, there are some security considerations to keep in mind:
- Ensure that contracts implementing this interface include proper access control mechanisms to prevent unauthorized calls to
fastBridgeTransferReceived. - Validate and sanitize the
callParamsinput in the implementing contract to prevent potential security vulnerabilities. - Consider documenting the expected format and usage of the
callParamsparameter for implementers. - Clarify the meaning of the bytes4 return value, possibly by defining specific return codes in the interface.
To verify the usage of this interface across the project, run the following script:
This will help ensure that the interface is being used correctly and securely throughout the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find implementations and usages of IFastBridgeRecipient
echo "Searching for implementations of IFastBridgeRecipient:"
ast-grep --lang solidity --pattern $'contract $_ is IFastBridgeRecipient'
echo "\nSearching for usages of fastBridgeTransferReceived:"
rg --type solidity -A 5 $'fastBridgeTransferReceived'
Length of output: 513
Script:
#!/bin/bash
# Description: Find implementations and usages of IFastBridgeRecipient in Solidity files
echo "Searching for implementations of IFastBridgeRecipient:"
rg --type-add 'solidity:*.sol' --type solidity 'contract\s+\w+\s+is\s+IFastBridgeRecipient' .
echo -e "\nSearching for usages of fastBridgeTransferReceived:"
rg --type-add 'solidity:*.sol' --type solidity -A 5 'fastBridgeTransferReceived' .
Length of output: 5718
Script:
#!/bin/bash
# Description: Extract fastBridgeTransferReceived function details from IFastBridgeRecipient implementations
# List of implementation files
files=(
"./packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol"
"./packages/contracts-rfq/test/mocks/RecipientMock.sol"
"./packages/contracts-rfq/test/mocks/NonPayableRecipient.sol"
"./packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol"
"./packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol"
)
for file in "${files[@]}"; do
echo "Extracting from $file:"
rg 'function fastBridgeTransferReceived' "$file" -A 5
echo -e "\n---\n"
done
Length of output: 2917
| # [0.7.0](https://github.com/synapsecns/sanguine/compare/@synapsecns/contracts-rfq@0.6.2...@synapsecns/contracts-rfq@0.7.0) (2024-10-07) | ||
|
|
||
|
|
||
| ### Features |
There was a problem hiding this comment.
Adjust heading level for consistency
The "Features" section is currently using an H3 heading (###). To maintain consistency with the Keep a Changelog format and address the static analysis hint, consider changing it to an H2 heading (##).
Apply this change:
-### Features
+## FeaturesThis adjustment will improve the overall structure and readability of the changelog.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Features | |
| ## Features |
🧰 Tools
🪛 Markdownlint
9-9: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
| function test_bridge_token_callParamsLengthMax() public { | ||
| bytes memory callParams = new bytes(2 ** 16 - 1); | ||
| setTokenTestCallParams(callParams); | ||
| test_bridge_token(); | ||
| } | ||
|
|
||
| function test_bridge_eth_callParamsLengthMax() public { | ||
| bytes memory callParams = new bytes(2 ** 16 - 1); | ||
| setEthTestCallParams(callParams); | ||
| test_bridge_eth(); | ||
| } | ||
|
|
||
| function test_bridge_token_revert_callParamsLengthAboveMax() public { | ||
| bytes memory callParams = new bytes(2 ** 16); | ||
| setTokenTestCallParams(callParams); | ||
| vm.expectRevert(CallParamsLengthAboveMax.selector); | ||
| bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2}); | ||
| } | ||
|
|
||
| function test_bridge_eth_revert_callParamsLengthAboveMax() public { | ||
| bytes memory callParams = new bytes(2 ** 16); | ||
| setEthTestCallParams(callParams); | ||
| vm.expectRevert(CallParamsLengthAboveMax.selector); | ||
| bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2}); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider defining constants for maximum call parameters lengths
To improve readability and maintainability, consider defining constants for the maximum call parameters length (2 ** 16 - 1) and the length exceeding the maximum (2 ** 16). This will make the purpose of these values clearer and avoid magic numbers.
Apply this diff:
+ uint256 constant MAX_CALL_PARAMS_LENGTH = 2 ** 16 - 1;
+ uint256 constant ABOVE_MAX_CALL_PARAMS_LENGTH = 2 ** 16;
function test_bridge_token_callParamsLengthMax() public {
- bytes memory callParams = new bytes(2 ** 16 -1);
+ bytes memory callParams = new bytes(MAX_CALL_PARAMS_LENGTH);
setTokenTestCallParams(callParams);
test_bridge_token();
}
function test_bridge_eth_callParamsLengthMax() public {
- bytes memory callParams = new bytes(2 ** 16 -1);
+ bytes memory callParams = new bytes(MAX_CALL_PARAMS_LENGTH);
setEthTestCallParams(callParams);
test_bridge_eth();
}
function test_bridge_token_revert_callParamsLengthAboveMax() public {
- bytes memory callParams = new bytes(2 ** 16);
+ bytes memory callParams = new bytes(ABOVE_MAX_CALL_PARAMS_LENGTH);
setTokenTestCallParams(callParams);
vm.expectRevert(CallParamsLengthAboveMax.selector);
bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2});
}
function test_bridge_eth_revert_callParamsLengthAboveMax() public {
- bytes memory callParams = new bytes(2 ** 16);
+ bytes memory callParams = new bytes(ABOVE_MAX_CALL_PARAMS_LENGTH);
setEthTestCallParams(callParams);
vm.expectRevert(CallParamsLengthAboveMax.selector);
bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function test_bridge_token_callParamsLengthMax() public { | |
| bytes memory callParams = new bytes(2 ** 16 - 1); | |
| setTokenTestCallParams(callParams); | |
| test_bridge_token(); | |
| } | |
| function test_bridge_eth_callParamsLengthMax() public { | |
| bytes memory callParams = new bytes(2 ** 16 - 1); | |
| setEthTestCallParams(callParams); | |
| test_bridge_eth(); | |
| } | |
| function test_bridge_token_revert_callParamsLengthAboveMax() public { | |
| bytes memory callParams = new bytes(2 ** 16); | |
| setTokenTestCallParams(callParams); | |
| vm.expectRevert(CallParamsLengthAboveMax.selector); | |
| bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2}); | |
| } | |
| function test_bridge_eth_revert_callParamsLengthAboveMax() public { | |
| bytes memory callParams = new bytes(2 ** 16); | |
| setEthTestCallParams(callParams); | |
| vm.expectRevert(CallParamsLengthAboveMax.selector); | |
| bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2}); | |
| } | |
| uint256 constant MAX_CALL_PARAMS_LENGTH = 2 ** 16 - 1; | |
| uint256 constant ABOVE_MAX_CALL_PARAMS_LENGTH = 2 ** 16; | |
| function test_bridge_token_callParamsLengthMax() public { | |
| bytes memory callParams = new bytes(MAX_CALL_PARAMS_LENGTH); | |
| setTokenTestCallParams(callParams); | |
| test_bridge_token(); | |
| } | |
| function test_bridge_eth_callParamsLengthMax() public { | |
| bytes memory callParams = new bytes(MAX_CALL_PARAMS_LENGTH); | |
| setEthTestCallParams(callParams); | |
| test_bridge_eth(); | |
| } | |
| function test_bridge_token_revert_callParamsLengthAboveMax() public { | |
| bytes memory callParams = new bytes(ABOVE_MAX_CALL_PARAMS_LENGTH); | |
| setTokenTestCallParams(callParams); | |
| vm.expectRevert(CallParamsLengthAboveMax.selector); | |
| bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2}); | |
| } | |
| function test_bridge_eth_revert_callParamsLengthAboveMax() public { | |
| bytes memory callParams = new bytes(ABOVE_MAX_CALL_PARAMS_LENGTH); | |
| setEthTestCallParams(callParams); | |
| vm.expectRevert(CallParamsLengthAboveMax.selector); | |
| bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2}); | |
| } |
| // Populate the fields using the absolute exclusivity period | ||
| setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD_ABS); | ||
| setEthTestExclusivityParams(relayerB, EXCLUSIVITY_PERIOD_ABS); | ||
| // Override with negative exclusivity period |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize parameter initialization by passing negative exclusivity periods directly
Currently, the code initializes the exclusivity parameters with positive values and then immediately overrides them with negative values. To eliminate redundancy and improve code clarity, consider passing the negative exclusivity period directly to the initialization functions.
Apply this diff to pass negative exclusivity periods directly:
function createFixturesV2() public virtual override {
- // Populate the fields using the absolute exclusivity period
- setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD_ABS);
- setEthTestExclusivityParams(relayerB, EXCLUSIVITY_PERIOD_ABS);
- // Override with negative exclusivity period
- tokenParamsV2.quoteExclusivitySeconds = -int256(EXCLUSIVITY_PERIOD_ABS);
- ethParamsV2.quoteExclusivitySeconds = -int256(EXCLUSIVITY_PERIOD_ABS);
+ // Populate the fields using the negative exclusivity period
+ setTokenTestExclusivityParams(relayerA, -int256(EXCLUSIVITY_PERIOD_ABS));
+ setEthTestExclusivityParams(relayerB, -int256(EXCLUSIVITY_PERIOD_ABS));
tokenTx.exclusivityEndTime = block.timestamp - EXCLUSIVITY_PERIOD_ABS;
ethTx.exclusivityEndTime = block.timestamp - EXCLUSIVITY_PERIOD_ABS;
}Committable suggestion was skipped due to low confidence.
| tokenParamsV2 = IFastBridgeV2.BridgeParamsV2({ | ||
| quoteRelayer: address(0), | ||
| quoteExclusivitySeconds: 0, | ||
| quoteId: bytes(""), | ||
| callParams: bytes("") | ||
| }); | ||
| ethParamsV2 = IFastBridgeV2.BridgeParamsV2({ | ||
| quoteRelayer: address(0), | ||
| quoteExclusivitySeconds: 0, | ||
| quoteId: bytes(""), | ||
| callParams: bytes("") | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate code duplication in createFixturesV2
The initialization of tokenParamsV2 and ethParamsV2 is nearly identical. Consider creating a helper function to reduce code duplication and enhance maintainability.
| function test_relay_eth_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual { | ||
| assertEmptyCallParams(ethTx.callParams); | ||
| setEthTestRecipient(noReturnValueRecipient); | ||
| test_relay_eth(); | ||
| } |
There was a problem hiding this comment.
Test function may not be testing the intended scenario
The test function test_relay_eth_noReturnValueRecipient_revertWhenCallParamsPresent asserts that callParams are empty, which may not effectively test the scenario.
Assign a value to ethTx.callParams to accurately test the behavior when callParams are present.
| function test_relay_eth_withRelayerAddress_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual { | ||
| assertEmptyCallParams(ethTx.callParams); | ||
| setEthTestRecipient(noReturnValueRecipient); | ||
| test_relay_eth_withRelayerAddress(); | ||
| } |
There was a problem hiding this comment.
Test function may not be testing the intended scenario
test_relay_eth_withRelayerAddress_noReturnValueRecipient_revertWhenCallParamsPresent checks for empty callParams, which doesn't test the intended scenario suggested by the function name.
Modify ethTx.callParams to include data to properly test the expected revert behavior.
| if (params.sender == address(0) || params.to == address(0)) revert ZeroAddress(); | ||
| if (params.originToken == address(0) || params.destToken == address(0)) revert ZeroAddress(); | ||
| if (params.deadline < block.timestamp + MIN_DEADLINE_PERIOD) revert DeadlineTooShort(); | ||
| if (paramsV2.callParams.length > MAX_CALL_PARAMS_LENGTH) revert CallParamsLengthAboveMax(); | ||
| int256 exclusivityEndTime = int256(block.timestamp) + paramsV2.quoteExclusivitySeconds; | ||
| // exclusivityEndTime must be in range (0 .. params.deadline] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor bridge function to reduce cyclomatic complexity.
The TODO comment suggests that the bridge function has a high cyclomatic complexity, which can make the code harder to understand and maintain. Consider refactoring the function by extracting portions of the logic into smaller, reusable functions to enhance readability and maintainability.
| // TODO: the note below isn't true anymore with the BridgeTransactionV2 struct | ||
| // since the variable length `callParams` was added. This needs to be fixed/acknowledged. | ||
|
|
There was a problem hiding this comment.
Update the outdated comment regarding getBridgeTransaction.
The TODO comment indicates that the note about decoding V1 fields is no longer accurate due to the addition of variable-length callParams in BridgeTransactionV2. Please update or remove the comment to reflect the current implementation to avoid confusion.
| bytes memory hookData = | ||
| abi.encodeCall(IFastBridgeRecipient.fastBridgeTransferReceived, (token, amount, callParams)); | ||
| // This will bubble any revert messages from the hook function | ||
| bytes memory returnData = Address.functionCallWithValue({target: recipient, data: hookData, value: msgValue}); | ||
| // Explicit revert if no return data at all | ||
| if (returnData.length == 0) revert RecipientNoReturnValue(); | ||
| // Check that exactly a single return value was returned | ||
| if (returnData.length != 32) revert RecipientIncorrectReturnValue(); | ||
| // Return value should be abi-encoded hook function selector | ||
| if (bytes32(returnData) != bytes32(IFastBridgeRecipient.fastBridgeTransferReceived.selector)) { | ||
| revert RecipientIncorrectReturnValue(); | ||
| } |
There was a problem hiding this comment.
Correct the validation of the recipient's return value in _checkedCallRecipient.
In the _checkedCallRecipient function, the comparison between bytes32(returnData) and bytes32(IFastBridgeRecipient.fastBridgeTransferReceived.selector) may not work as intended because the function selector is 4 bytes, whereas the return data is 32 bytes. This could lead to incorrect validation results.
Apply this diff to fix the comparison by properly decoding the returned function selector:
// Return value should be abi-encoded hook function selector
- if (bytes32(returnData) != bytes32(IFastBridgeRecipient.fastBridgeTransferReceived.selector)) {
+ // Extract the returned selector from the return data
+ bytes4 returnedSelector = abi.decode(returnData, (bytes4));
+ if (returnedSelector != IFastBridgeRecipient.fastBridgeTransferReceived.selector) {
revert RecipientIncorrectReturnValue();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bytes memory hookData = | |
| abi.encodeCall(IFastBridgeRecipient.fastBridgeTransferReceived, (token, amount, callParams)); | |
| // This will bubble any revert messages from the hook function | |
| bytes memory returnData = Address.functionCallWithValue({target: recipient, data: hookData, value: msgValue}); | |
| // Explicit revert if no return data at all | |
| if (returnData.length == 0) revert RecipientNoReturnValue(); | |
| // Check that exactly a single return value was returned | |
| if (returnData.length != 32) revert RecipientIncorrectReturnValue(); | |
| // Return value should be abi-encoded hook function selector | |
| if (bytes32(returnData) != bytes32(IFastBridgeRecipient.fastBridgeTransferReceived.selector)) { | |
| revert RecipientIncorrectReturnValue(); | |
| } | |
| bytes memory hookData = | |
| abi.encodeCall(IFastBridgeRecipient.fastBridgeTransferReceived, (token, amount, callParams)); | |
| // This will bubble any revert messages from the hook function | |
| bytes memory returnData = Address.functionCallWithValue({target: recipient, data: hookData, value: msgValue}); | |
| // Explicit revert if no return data at all | |
| if (returnData.length == 0) revert RecipientNoReturnValue(); | |
| // Check that exactly a single return value was returned | |
| if (returnData.length != 32) revert RecipientIncorrectReturnValue(); | |
| // Return value should be abi-encoded hook function selector | |
| // Extract the returned selector from the return data | |
| bytes4 returnedSelector = abi.decode(returnData, (bytes4)); | |
| if (returnedSelector != IFastBridgeRecipient.fastBridgeTransferReceived.selector) { | |
| revert RecipientIncorrectReturnValue(); | |
| } |
add fe-release label
fix label
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
@synapsecns/rfq-indexer-apiand@synapsecns/bridge-docspackages.698ba5b: docs preview link
aec7fa4: docs preview link