FE Release only for #3238 #3250
Conversation
* add fe-release label * fix label --------- Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
* 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>
…[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
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several changes across multiple files. Key modifications include the addition of a new configuration option Changes
Possibly related PRs
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
|
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🧹 Outside diff range and nitpick comments (49)
packages/contracts-rfq/test/mocks/NoOpContract.sol (2)
4-5: LGTM: Appropriate linter directive and contract naming.The linter directive to disable the "no-empty-blocks" rule is suitable for this mock contract. The contract name
NoOpContractis descriptive and follows the PascalCase naming convention.Consider adding a brief comment explaining the purpose of this mock contract, e.g.:
/// @dev A mock contract that accepts ETH without performing any operations contract NoOpContract {This would provide more context for developers who might use this contract in the future.
6-7: LGTM: Properly implemented receive function.The
receivefunction is correctly implemented to accept ETH, which aligns with the contract's purpose as a mock that needs to accept ETH. The empty function body is appropriate for a no-op contract.Consider using a more standardized format for the function comment:
/// @notice Allows the contract to receive ETH /// @dev This function is empty as per the no-op nature of the contract receive() external payable {}This format provides more detailed information and follows common Solidity documentation practices.
docs/bridge/docs/01-About/03-Routes.md (1)
3-3: Consider adding more specific information to the header.The current header "Chains & Tokens" is concise, but it could be more descriptive to better indicate the content of this section.
Consider updating the header to something like:
-# Chains & Tokens +# Supported Chains & Tokenspackages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol (2)
1-2: LGTM! Consider specifying a more precise Solidity version range.The SPDX license identifier and pragma solidity version are correctly set. However, for better consistency across the project, consider specifying a narrower range for the Solidity version, e.g.,
^0.8.9or even an exact version like0.8.19. This can help prevent potential issues with breaking changes in minor Solidity releases.
5-12: LGTM! Consider adding NatSpec documentation for improved clarity.The function signature for
fastBridgeTransferReceivedis well-structured and includes appropriate parameters for a bridge transfer. Thepayablemodifier allows for flexibility in handling different types of transfers.To enhance clarity and maintainability, consider adding NatSpec documentation to explain:
- The purpose of each parameter
- The expected behavior of the function
- The meaning of the
bytes4return value (e.g., if it's meant to be a specific function selector for acknowledgment)Example:
/// @notice Handles incoming fast bridge transfers /// @param token The address of the token being transferred /// @param amount The amount of tokens transferred /// @param callParams Additional parameters for the transfer /// @return bytes4 A function selector acknowledging the transfer function fastBridgeTransferReceived( address token, uint256 amount, bytes memory callParams ) external payable returns (bytes4);packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol (2)
4-4: Consider specifying rules for solhint-disableThe
solhint-disabledirective is currently disabling all rules. It's generally better to disable only specific rules that are necessary to ignore. This helps maintain code quality by ensuring other important rules are still enforced.Consider updating the directive to disable only specific rules, for example:
// solhint-disable-next-line no-empty-blocks, payable-fallback
11-12: LGTM: Correctly implemented mock function with clear documentationThe
fastBridgeTransferReceivedfunction is correctly implemented as a mock that doesn't return any value. The comment clearly states that this is an incorrect implementation, which is perfect for testing purposes.Consider enhancing the comment to explicitly mention that this mock is designed to test error handling for non-compliant implementations:
/// @notice Incorrectly implemented - method does not return anything. Used to test error handling for non-compliant implementations.packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol (1)
Line range hint
1-18: Enhance documentation for improved clarity and maintainability.While the contract is well-structured, consider the following improvements:
- Add a natspec comment for the contract explaining its purpose and how it differs from
FastBridgeV2DstGasBenchmarkTest.- Include a comment for the
EXCLUSIVITY_PERIODconstant explaining its significance in the context of this test.- Add a comment in the
setUpfunction explaining why you're skipping half of theEXCLUSIVITY_PERIOD.These additions will enhance the contract's readability and make it easier for other developers to understand and maintain the code.
Here's an example of how you might implement these suggestions:
/// @title FastBridgeV2DstExclusivityTest /// @notice This contract extends FastBridgeV2DstGasBenchmarkTest to specifically test exclusivity-related functionality in FastBridgeV2. contract FastBridgeV2DstExclusivityTest is FastBridgeV2DstGasBenchmarkTest { /// @notice The duration of the exclusivity period used for testing, set to 60 seconds. uint256 public constant EXCLUSIVITY_PERIOD = 60 seconds; 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}); } // ... rest of the contract }packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (3)
6-6: LGTM! Consider adding parameters for improved debugging.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 violation.
14-14: LGTM! Consider adding parameters for better error handling.The new
RecipientIncorrectReturnValueerror is a good addition for handling cases where a recipient returns an incorrect value.To enhance error handling and debugging, consider adding parameters to provide more context:
error RecipientIncorrectReturnValue(bytes expected, bytes actual);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 and the extra blank line disrupt the interface's structure.
Consider the following improvements:
- Group related errors together or order them alphabetically for easier navigation.
- Remove the extra blank line after
RecipientNoReturnValueto maintain consistent spacing.This will enhance the overall readability and maintainability of the interface.
packages/contracts-rfq/test/mocks/RecipientMock.sol (2)
6-7: Enhance the warning comment for clarity.The comment indicating that this is a mock for testing purposes is helpful. However, it could be more explicit to prevent any potential misuse.
Consider updating the comment to:
/// @notice Mock contract for testing purposes only. DO NOT USE IN PRODUCTION. /// @dev This contract implements IFastBridgeRecipient for testing the FastBridge functionality.This change provides a stronger warning and additional context about the contract's purpose.
11-14: LGTM: Correct implementation of fastBridgeTransferReceived.The
fastBridgeTransferReceivedfunction is correctly implemented according to the interface requirements. Returning the function selector is an appropriate way to confirm interface compliance in a mock contract.Consider enhancing the comment to provide more context:
/// @notice Minimal viable implementation of the fastBridgeTransferReceived hook. /// @dev This function returns its own selector to confirm interface compliance. /// It does not perform any actual logic, as it's intended for testing purposes only.This additional information could help other developers understand the purpose and limitations of this mock implementation.
packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol (2)
1-7: LGTM! Consider adding more detailed documentation.The contract declaration, license, and import statements are correct. The explicit warning about not using this contract in production is a good practice for test mocks.
Consider adding more detailed documentation about the purpose of this mock contract and how it's intended to be used in tests. This will help other developers understand its role in the testing suite more quickly.
11-14: LGTM! Consider making the return value configurable.The
fastBridgeTransferReceivedfunction is correctly implemented for its intended purpose as a mock with excessive return value. The comment clearly indicates that the implementation is incorrect, which is important for test contracts.Consider making the hardcoded return value (1337) configurable, either through a constructor parameter or a setter function. This would allow for more flexible testing scenarios without changing the contract code. Here's an example implementation:
contract ExcessiveReturnValueRecipient { uint256 private returnValue; constructor(uint256 _returnValue) { returnValue = _returnValue; } function setReturnValue(uint256 _newValue) external { returnValue = _newValue; } function fastBridgeTransferReceived(address, uint256, bytes memory) external payable returns (bytes4, uint256) { return (IFastBridgeRecipient.fastBridgeTransferReceived.selector, returnValue); } // ... rest of the contract }This change would make the mock contract more versatile for different test cases.
packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol (2)
11-15: LGTM: fastBridgeTransferReceived function serves its purpose for testing.The function is correctly implemented to return an incorrect selector value, which is essential for testing error handling in the main contract. The comment clearly indicates that the implementation is incorrect, which is good for preventing misunderstandings.
Consider adding a comment explaining why the last bit is flipped, to make the intention even clearer. For example:
// Flip the last bit to simulate an incorrect return value return IFastBridgeRecipient.fastBridgeTransferReceived.selector ^ 0x00000001;
1-16: Overall, the IncorrectReturnValueRecipient contract is well-implemented for its testing purpose.The contract successfully creates a mock implementation of
IFastBridgeRecipientthat returns an incorrect value. This is valuable for testing error handling and edge cases in the main contract. The code is clean, well-commented, and clearly indicates its purpose as a testing tool.Consider adding this mock contract to a dedicated 'mocks' or 'test-utils' directory if not already done, to keep test-related contracts organized and separate from production code.
packages/rfq-indexer/api/src/controllers/disputesController.ts (2)
7-27: Function structure looks good, consider adding input validation.The async function structure is well-suited for handling database operations, and the use of Express.js types enhances type safety. The logical flow (query, process, respond) is clear and appropriate.
Consider adding input validation for any query parameters or request body that might be used in future iterations of this controller. This proactive approach can improve the robustness of the API.
9-16: Query construction is solid, consider adding pagination.The query construction using a CTE is clean and potentially performant. Ordering by timestamp ensures the most recent disputes are returned first, which is logical for this use case.
Consider implementing pagination for the disputes query. As the number of disputes grows, returning all results in a single request could become inefficient. Pagination would improve performance and reduce the payload size for large datasets.
Example implementation:
const limit = parseInt(req.query.limit as string) || 10; const offset = parseInt(req.query.offset as string) || 0; const query = db .with('disputes', () => qDisputes({activeOnly: true})) .selectFrom('disputes') .selectAll() .orderBy('blockTimestamp_dispute', 'desc') .limit(limit) .offset(offset);.github/workflows/labeler.yml (1)
34-40: LGTM! Consider a minor improvement for consistency.The new step to add the 'fe-release' label is well-implemented and correctly integrated into the existing workflow. It's properly conditioned to only run for pull requests targeting the 'fe-release' branch, which is appropriate.
For consistency with the rest of the file, consider using single quotes for the 'github_token' key in the 'with' section.
Here's a suggested minor change:
with: - github_token: ${{ secrets.GITHUB_TOKEN }} + github_token: '${{ secrets.GITHUB_TOKEN }}' labels: 'fe-release'packages/rfq-indexer/api/src/queries/disputesQueries.ts (2)
4-11: LGTM: Well-structured query initialization and joinThe query initialization and left join are well-implemented. The join condition effectively identifies stale or invalid disputes by comparing timestamps, which is a good approach. The code comment explaining the purpose of the join is helpful for maintainability.
Consider using a more descriptive name for the join callback parameter:
- .leftJoin('BridgeProofProvidedEvents', (join) => + .leftJoin('BridgeProofProvidedEvents', (joinClause) =>This small change can make the code slightly more self-documenting.
21-23: LGTM: Effective conditional filteringThe conditional filtering for
activeOnlyis well-implemented. Usingis nullto check for non-existence in the joined table is the correct approach.For improved readability, consider extracting the condition into a variable with a descriptive name:
if (activeOnly) { - query = query.where('BridgeProofProvidedEvents.transactionId', 'is', null); + const noMatchingProof = 'BridgeProofProvidedEvents.transactionId'; + query = query.where(noMatchingProof, 'is', null); }This change makes the code more self-documenting and easier to understand at a glance.
packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol (1)
11-12: Approve changes with a minor suggestionThe refactoring of
createFixturesV2improves code organization and maintainability. The use of separate functions for setting token and ETH exclusivity parameters enhances flexibility in testing.Consider adding a brief comment explaining the purpose of these function calls to improve code documentation.
You could add a comment like this:
+ // Set up exclusivity parameters for token and ETH tests setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD); setEthTestExclusivityParams(relayerB, EXCLUSIVITY_PERIOD);packages/rfq-indexer/api/src/controllers/conflictingProofsController.ts (1)
47-47: Approve updated error message, but consider adding more context.The error message now correctly specifies that it's related to fetching active conflicting proofs, which is consistent with the other changes. This improves error tracking and debugging.
Consider adding more context to the error message, such as including relevant query parameters or the specific stage where the error occurred. This could further enhance debugging capabilities. For example:
- console.error('Error fetching active conflicting proofs:', error) + console.error('Error fetching active conflicting proofs:', error, { query: query.toSQL() })packages/rfq-indexer/api/README.md (2)
14-35: Approve API call updates and suggest code block improvementsThe updates to the API calls section, particularly the curl examples with correct URL paths, are good improvements.
To further enhance readability and enable syntax highlighting, consider adding language specifications to the code blocks. For example:
- ``` + ```bash curl http://localhost:3001/api/pending-transactions/missing-relay ```Apply this change to all code blocks in this section.
🧰 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)
38-41: Approve new "Env Vars" section with minor suggestionThe addition of the "Env Vars" section is valuable for developers setting up the project.
To improve readability, consider using a markdown list for the environment variables:
-## Env Vars +## Env Vars + +- `NODE_ENV`: Set to `"development"` for localhost testing. +- `DATABASE_URL`: PostgreSQL connection URL for the ponder index. - -- **NODE_ENV**: Set to `"development"` for localhost testing. -- **DATABASE_URL**: PostgreSQL connection URL for the ponder index.packages/rfq-indexer/api/src/graphql/types/events.graphql (1)
83-91: LGTM! Consider adding adisputedByfield.The new
BridgeProofDisputedEventtype is well-structured and consistent with other event types in the file. It includes essential fields likeid,transactionId,blockNumber, etc.Consider adding a
disputedByfield to capture the address of the account that initiated the dispute. This could be useful for auditing and analytics purposes. For example:type BridgeProofDisputedEvent { id: String! transactionId: String! blockNumber: BigInt! blockTimestamp: Int! transactionHash: String! originChainId: Int! originChain: String! + disputedBy: String! }packages/rfq-indexer/api/src/routes/disputesRoute.ts (1)
7-62: Excellent OpenAPI documentation with a minor formatting suggestion.The OpenAPI documentation for the '/disputes' endpoint is comprehensive and well-structured. It provides clear information about the endpoint's purpose and possible responses, which is crucial for API consumers.
Consider adjusting the indentation for consistency. For example:
* properties: -* Bridge: -* type: object -* description: General transaction fields +* Bridge: +* type: object +* description: General transaction fieldsThis minor adjustment will improve readability and maintain consistent formatting throughout the documentation.
packages/rfq-indexer/api/src/controllers/transactionIdController.ts (2)
16-16: Approved: Enhanced proof retrieval logicThe modification to fetch all proofs, including invalidated ones, provides a more comprehensive transaction history. This change aligns well with the goal of showing a complete picture of the transaction's lifecycle.
Consider slightly rewording the comment for clarity:
- // display proofs even if they have been invalidated/replaced by a dispute + // Retrieve all proofs, including those invalidated or replaced by disputes
17-17: Approved: Addition of dispute handlingThe introduction of dispute handling with
qDisputes({activeOnly: true})enhances the transaction information retrieval. Filtering for active disputes ensures that only relevant dispute information is displayed.Consider slightly rewording the comment for clarity:
- // do not show disputes that have been invalidated/replaced by a proof + // Retrieve only active disputes, excluding those invalidated or replaced by proofspackages/rfq-indexer/api/src/graphql/queries/queries.graphql (2)
122-122: New query looks good, consider adding a comment for clarityThe new
pendingTransactionsMissingRelayExceedDeadlinequery is well-placed and consistent with existing queries. It provides a useful way to fetch transactions that have exceeded a deadline.Consider adding a comment to briefly explain the deadline criteria, as it's not evident from the schema alone. This would improve clarity for other developers working with this GraphQL API.
115-129: Summary: Valuable additions to the GraphQL schemaThe changes introduced in this file enhance the GraphQL schema by adding support for disputed relays and transactions exceeding deadlines. The new
DisputedRelaytype and the queriespendingTransactionsMissingRelayExceedDeadlineanddisputedRelaysare well-structured and consistent with the existing schema. These additions provide valuable functionality for handling edge cases in the relay process, which should improve the overall robustness of the system.As the schema grows, consider organizing related types and queries into separate files or modules to maintain code clarity and manageability. This could involve creating separate files for transaction-related, relay-related, and dispute-related schema definitions.
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. However, for consistency, consider renamingchainIdtooriginChainIdandchaintooriginChainto match the naming convention used in other interfaces.Here's a suggested modification:
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/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
36-41: Enhancement: AddedcallParamstoBridgeParamsV2structThe addition of
bytes callParamsto theBridgeParamsV2struct enhances the flexibility of the bridge operation by allowing arbitrary call parameters to be passed. This is a good improvement that enables more complex interactions during bridging.However, consider the following:
- Ensure that the
callParamsare properly validated and sanitized in the implementation to prevent potential security vulnerabilities.- Update the documentation for the
bridgefunction to reflect this new parameter and its usage.- Consider adding a size limit to
callParamsto prevent excessive gas costs.packages/rfq-indexer/api/src/routes/pendingTransactionsRoute.ts (2)
6-7: LGTM! Consider improving import readability.The addition of
pendingTransactionsMissingRelayExceedDeadlineControllerto the import statement is correct and consistent with the new route added later in the file.For improved readability, consider breaking the import statement into multiple lines:
import { pendingTransactionsMissingClaimController, pendingTransactionsMissingProofController, pendingTransactionsMissingRelayController, pendingTransactionsMissingRelayExceedDeadlineController } from '../controllers/pendingTransactionsController'
150-190: LGTM! Minor improvements suggested.The new route '/exceed-deadline' is well-implemented with comprehensive OpenAPI documentation. Great job on maintaining consistency with the existing code structure.
There's a minor typo in the 404 response description. Change "No pending transactionst that exceed the deadline found" to "No pending transactions that exceed the deadline found".
For consistency with other routes in this file, consider moving the router.get() call to a single line:
router.get('/exceed-deadline', pendingTransactionsMissingRelayExceedDeadlineController)packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (3)
14-15: LGTM. Consider adding documentation for the newcallParamsfield.The addition of the
callParamsfield to theBridgeParamsV2structure is consistent with the reported changes. While the empty string initialization is acceptable for these tests, it would be beneficial to add a comment explaining the purpose and potential usage of this new field for future reference.
20-21: LGTM. Ensure consistency in documentation if implemented.The addition of the
callParamsfield to theethParamsV2structure is consistent with the previous change totokenParamsV2. If documentation is added for thecallParamsfield intokenParamsV2, ensure that similar documentation is provided here for consistency.
Line range hint
1-180: Summary: Minor structural changes with no impact on existing tests.The changes in this file are limited to adding a
callParamsfield to bothtokenParamsV2andethParamsV2structures. These additions:
- Maintain consistency between token and ETH parameter structures.
- Don't affect the existing test logic or scenarios.
- Prepare the
BridgeParamsV2structure for potential future enhancements.Consider adding documentation for the new
callParamsfield to clarify its purpose and expected usage in future implementations.packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (3)
Line range hint
107-111: LGTM! Consider adding a comment for clarity.The refactoring improves code readability by using the
setTokenTestExclusivityParamshelper method. The test logic remains intact while accommodating the new exclusivity feature.Consider adding a brief comment explaining the purpose of the exclusivity period and its impact on the test:
+ // Test bridging with exclusivity period to ensure correct handling of relayer assignment setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD); bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2});
Line range hint
188-192: LGTM! Consider adding a comment for consistency.The refactoring improves code readability by using the
setEthTestExclusivityParamshelper method. The test logic remains intact while accommodating the new exclusivity feature for ETH transactions.For consistency with the token test, consider adding a brief comment explaining the purpose of the exclusivity period:
+ // Test ETH bridging with exclusivity period to ensure correct handling of relayer assignment setEthTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD); bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2});
Line range hint
1-265: Consider adding edge case tests for exclusivity periods.The changes improve test coverage by including exclusivity scenarios for both token and ETH bridging. To further enhance the test suite, consider adding the following edge case tests:
- Test bridging with an expired exclusivity period.
- Test bridging with a very short exclusivity period (e.g., 1 second).
- Test bridging with a very long exclusivity period.
- Test multiple bridge transactions with overlapping exclusivity periods.
These additional tests would help ensure robust handling of various exclusivity scenarios in the FastBridgeV2 contract.
Would you like assistance in implementing these additional test cases?
packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (1)
50-54: Rephrase informal language in comments for clarityThe comment includes informal language: "This is weird, but it is what it is." Consider rephrasing for professionalism and clarity.
Apply this diff to improve the comment:
- // 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 right after the offset). Although unexpected, this behavior aligns with Solidity's encoding specifications.packages/rfq-indexer/api/src/controllers/pendingTransactionsController.ts (1)
122-138: Add unit tests for the new controllers to ensure reliabilityTo ensure that the new endpoints function correctly and to catch potential issues early, please add unit tests covering the new controllers. This will help maintain code quality and prevent future regressions.
Also applies to: 140-167
packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol (3)
238-241: Use the correct data format when mocking calls without calldataWhen mocking a call without any calldata, it's better to use
bytes("")orhex""to explicitly indicate an empty byte array, ensuring clarity in the test.Apply this diff to improve clarity:
vm.mockCallRevert({callee: userB, data: "", revertData: bytes(REVERT_MSG)}); +vm.mockCall(userB, bytes(""), revertData);Also applies to: 244-247
14-19: Initialize fixtures after callingsuper.createFixtures()It's generally safer to call
super.createFixtures()before initializing variables likeuserBto ensure that any dependencies or initializations in the base class are completed first.Apply this diff to adjust the order:
function createFixtures() public virtual override { + super.createFixtures(); // In the inherited tests userB is always used as the recipient of the tokens. userB = address(new RecipientMock()); vm.label(userB, "ContractRecipient"); - super.createFixtures(); }
11-12: Consider visibility and storage location for constantsSince
CALL_PARAMSandREVERT_MSGare constants, you might consider specifying them asinternalinstead ofpublicif they are only used within this contract, reducing the contract's public interface.Apply this diff if appropriate:
-bytes public constant CALL_PARAMS = abi.encode("Hello, world!"); -bytes public constant REVERT_MSG = "GM, this is a revert"; +bytes internal constant CALL_PARAMS = abi.encode("Hello, world!"); +bytes internal constant REVERT_MSG = "GM, this is a revert";packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (2)
159-306: Consider simplifying excessively long function names for readabilitySome of the test function names are quite long and may impact readability, such as
test_relay_token_withRelayerAddress_noReturnValueRecipient_revertWhenCallParamsPresent. Consider simplifying the function names while maintaining clarity, possibly by using descriptive comments to convey additional details.
227-247: Ensure consistent use of thevirtualkeyword in test functionsIn the "NON PAYABLE RECIPIENT" section, test functions are declared without the
virtualkeyword:function test_relay_token_nonPayableRecipient() public { // ... }However, similar test functions in other sections are declared as
public virtual. For consistency and to maintain extensibility in derived contracts, consider adding thevirtualkeyword to these functions unless there is a specific reason not to.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (50)
- .github/renovate.json (1 hunks)
- .github/workflows/labeler.yml (1 hunks)
- 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)
- 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)
- 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 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 (7)
- docs/bridge/CHANGELOG.md
- docs/bridge/docs/02-Bridge/04-Code-Examples.md
- docs/bridge/package.json
- packages/contracts-rfq/package.json
- packages/contracts-rfq/test/mocks/NonPayableRecipient.sol
- packages/rfq-indexer/api/CHANGELOG.md
- packages/rfq-indexer/api/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)
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 (71)
packages/contracts-rfq/test/mocks/NoOpContract.sol (2)
1-2: LGTM: Proper license and Solidity version specified.The SPDX License Identifier is correctly included, and an appropriate Solidity version (^0.8.0) is specified. This ensures compatibility with recent Solidity features and security improvements.
1-8: Overall assessment: Well-implemented mock contract.This
NoOpContractis a well-structured and purposeful mock implementation. It correctly serves its function of accepting ETH without performing any operations, which is useful for testing scenarios. The contract follows Solidity best practices and uses appropriate syntax.The suggested minor improvements to documentation would enhance readability and maintainability, but the contract is functional and correct as is.
packages/rfq-indexer/api/src/queries/index.ts (1)
3-3: LGTM! New query export added correctly.The addition of the
qDisputesexport from thedisputesQueriesmodule is consistent with the existing pattern in this file. This change enhances the functionality of the RFQ Indexer API by making dispute-related queries available.Let's verify the existence of the
disputesQueriesmodule and theqDisputesexport:✅ Verification successful
Verification Successful:
disputesQueriesmodule andqDisputesexport confirmed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of the disputesQueries.ts file and the qDisputes export # Check if the file exists if fd -t f "disputesQueries.ts" packages/rfq-indexer/api/src/queries; then echo "disputesQueries.ts file found." else echo "Error: disputesQueries.ts file not found." exit 1 fi # Check for the qDisputes export if rg -q "export const qDisputes" packages/rfq-indexer/api/src/queries/disputesQueries.ts; then echo "qDisputes export found in disputesQueries.ts" else echo "Error: qDisputes export not found in disputesQueries.ts" exit 1 fiLength of output: 380
docs/bridge/docs/01-About/03-Routes.md (3)
5-6: LGTM! Clear and informative explanation.The explanation provides a good overview of the purpose of this page and directs users to the Synapse Bridge for route verification.
7-7: Ensure proper rendering of the Routes component.The
Routescomponent is correctly placed in the document. However, it's important to verify that it renders as expected and provides the intended information about supported routes.As this is a documentation file, we can't directly verify the rendering. However, we can check if the component is being used consistently across the documentation:
#!/bin/bash # Description: Check for consistent usage of the Routes component in documentation # Test: Search for other instances of the Routes component in documentation rg --type md '<Routes />' docs
1-1: Verify the import path for the Routes component.The import statement looks correct, but it's important to ensure that the
Routescomponent exists at the specified path@site/src/components/Routes.Run the following script to verify the existence of the Routes component:
✅ Verification successful
Verified the import path for the Routes component.
The
Routescomponent exists atdocs/bridge/src/components/Routes.tsx, which matches the import path specified.🏁 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 "Routes.(js|jsx|ts|tsx)" src/componentsLength of output: 82
Script:
#!/bin/bash # Description: Search for the definition of the Routes component # Search for the Routes component definition across the entire codebase rg --type tsx -e 'export\s+default\s+Routes' src/Length of output: 79
Script:
#!/bin/bash # Description: Search for the definition of the Routes component across all .tsx files # Search for the Routes component definition in .tsx files rg -g '*.tsx' -e 'export\s+default\s+Routes' src/Length of output: 96
Script:
#!/bin/bash # Description: Search for the Routes component across the entire repository # Search for Routes component files with common extensions fd -e js -e jsx -e ts -e tsx "Routes\.(js|jsx|ts|tsx)"Length of output: 332
packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol (1)
4-4: LGTM! Interface declaration follows best practices.The interface name
IFastBridgeRecipientis descriptive and follows the common convention of prefixing interfaces with 'I'. It clearly indicates its purpose for implementing recipients of fast bridge transfers.packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol (3)
6-7: LGTM: Clear contract naming and purposeThe contract name
NoReturnValueRecipientclearly indicates its purpose, and the comment explicitly states that it's for testing purposes and should not be used in production. This is good practice for test mocks.
8-9: LGTM: Correctly implementedreceivefunctionThe
receivefunction is correctly implemented to allow the contract to accept ETH payments. The comment provides clear context for its purpose in the mock contract.
1-13: Overall: Well-implemented test mock with clear documentationThis contract is well-structured and clearly documented as a test mock for a non-compliant recipient. It correctly implements the necessary functions for its testing purpose, with appropriate comments explaining its intentional incorrect behavior. The explicit warning against production use is a good practice for test mocks.
packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol (1)
16-17: Improved code organization for setting exclusivity parameters.The refactoring of
createFixturesV2()to usesetTokenTestExclusivityParamsandsetEthTestExclusivityParamsenhances code organization and potentially improves reusability. This change consolidates the logic for setting exclusivity parameters into dedicated functions, which is a good practice.However, to ensure completeness:
- Verify that these new methods are properly defined and implemented in the parent contract or a related contract.
- Confirm that all necessary parameters (previously set directly) are now covered by these new methods.
- Consider adding comments explaining the purpose of these methods and the
EXCLUSIVITY_PERIODconstant for better documentation.To verify the implementation of the new methods, run the following script:
✅ Verification successful
Verified implementation of exclusivity parameter methods.
The functions
setTokenTestExclusivityParamsandsetEthTestExclusivityParamsare properly defined inpackages/contracts-rfq/test/FastBridgeV2.t.sol. Their implementations correctly set the exclusivity parameters as intended, ensuring the refactoredcreateFixturesV2()function operates as expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of setTokenTestExclusivityParams and setEthTestExclusivityParams # Test: Search for the method implementations echo "Searching for setTokenTestExclusivityParams implementation:" rg --type solidity -A 5 'function setTokenTestExclusivityParams' echo "\nSearching for setEthTestExclusivityParams implementation:" rg --type solidity -A 5 'function setEthTestExclusivityParams'Length of output: 455
Script:
#!/bin/bash # Description: Verify the implementation of setTokenTestExclusivityParams and setEthTestExclusivityParams # Test: Search for the method implementations within .sol files echo "Searching for setTokenTestExclusivityParams implementation in .sol files:" rg 'function setTokenTestExclusivityParams' --glob '*.sol' -A 5 echo "\nSearching for setEthTestExclusivityParams implementation in .sol files:" rg 'function setEthTestExclusivityParams' --glob '*.sol' -A 5Length of output: 1689
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 (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
IFastBridgeRecipientinterface is correct and uses an appropriate relative path.
8-9: LGTM: Correctly implemented receive function.The
receivefunction is properly implemented, allowing the contract to accept ETH. The comment clearly explains its purpose, which is crucial for understanding the mock's functionality in testing scenarios.packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol (1)
8-9: LGTM! Thereceivefunction is correctly implemented.The
receivefunction is properly implemented to allow the contract to accept ETH payments, which is necessary for testing purposes. The comment clearly explains its purpose.packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol (2)
1-7: LGTM: Contract declaration and purpose are well-defined.The contract is correctly structured with proper license, pragma, and import statements. The comment clearly indicates that this is a mock contract for testing purposes, which is a good practice for preventing misuse in production environments.
8-9: LGTM: Receive function is correctly implemented.The
receivefunction is properly declared asexternal payable, allowing the contract to accept ETH. This is essential for testing scenarios involving ETH transfers.packages/rfq-indexer/api/src/routes/index.ts (2)
17-17: LGTM! Verify the implementation of the disputes route.The route registration for
/disputesis correctly implemented and consistent with the existing code structure.Let's verify the implementation of the disputes route:
#!/bin/bash # Description: Check the implementation of the disputesRoute # Test: Search for the disputesRoute implementation ast-grep --lang typescript --pattern $'const disputesRoute = express.Router() $$$ export default disputesRoute'
8-8: LGTM! Verify the existence of the imported file.The import statement for
disputesRouteis correctly formatted and consistent with the existing code style.Let's verify the existence of the imported file:
packages/rfq-indexer/api/src/db/index.ts (2)
25-25: LGTM! Verify the database schema update.The addition of
BridgeProofDisputedEventsto theDatabaseinterface is consistent with the import change and follows the existing pattern. This change enables the database to handle the new event type.To ensure the database schema has been updated accordingly, please run the following script:
#!/bin/bash # Description: Verify the database schema update for BridgeProofDisputedEvents # Test: Search for database migration files or schema definitions fd -e sql -e ts 'migration|schema' --exec rg --type sql --type typescript 'BridgeProofDisputedEvents'
10-10: LGTM! Verify the type definition in the source file.The addition of
BridgeProofDisputedEventsto the import statement is consistent with the existing pattern for event types. This change enhances the system's capability to handle a new type of event.To ensure the type is correctly defined, please run the following script:
packages/rfq-indexer/api/src/controllers/disputesController.ts (1)
1-5: LGTM: Imports are well-organized and relevant.The imports are logically structured, with external libraries first followed by internal modules. All imported items appear to be necessary for the controller's functionality.
docs/bridge/src/components/Routes.tsx (2)
5-7: Improved chain data accessThe changes simplify the code by directly accessing chain data from
CHAINS.CHAINS_BY_ID. This approach is more maintainable and potentially more performant as it eliminates the need for an intermediate object.
32-33: Simplified token image renderingThe changes improve readability by directly using
token.iconas the image source. However, it appears thattoken.iconhas changed from a function to a string URL.Could you confirm if
token.iconis now always expected to be a string URL? If so, has this change been consistently applied across the codebase? Let's verify:#!/bin/bash # Search for usages of token.icon to ensure consistency rg --type typescript --type javascript 'token\.icon'.github/renovate.json (1)
20-20: Approved change, but clarification needed on rationaleThe addition of the "ignorePaths" option is syntactically correct and will effectively prevent Renovate from updating dependencies in the "packages/contracts-core" directory and its subdirectories.
However, could you please clarify the rationale behind ignoring this specific path? Understanding the reasoning would help ensure this aligns with the project's overall dependency management strategy.
To better understand the impact of this change, let's verify the structure of the ignored directory:
This will help us understand what kind of dependencies might be present in the ignored directory.
Consider adding a comment in the configuration file explaining why this path is being ignored. This will help future maintainers understand the decision. For example:
"ignorePaths": [ // Ignoring contracts-core due to [reason] "packages/contracts-core/**" ],✅ Verification successful
Verified: 'ignorePaths' correctly excludes 'packages/contracts-core' directory
The
ignorePathsconfiguration in.github/renovate.jsonsuccessfully excludes thepackages/contracts-coredirectory, which contains its ownpackage.json. This ensures that Renovate will not automatically update dependencies within this directory, allowing for manual or alternative dependency management as intended.Consider adding a comment in the configuration file to document the rationale behind this exclusion for future reference.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the structure of the ignored directory # Test: List contents of the ignored directory echo "Contents of packages/contracts-core:" ls -R packages/contracts-core # Test: Check for package.json files in the ignored directory echo -e "\npackage.json files in the ignored directory:" find packages/contracts-core -name "package.json"Length of output: 7366
packages/rfq-indexer/api/src/queries/disputesQueries.ts (4)
1-3: LGTM: Well-structured function signature and importThe import statement and function signature are well-defined. The use of TypeScript for type safety is commendable, and providing a default value for the
activeOnlyparameter is a good practice.
12-19: LGTM: Comprehensive and well-aliased field selectionThe field selection is well-structured and comprehensive. The use of aliases with the '_dispute' suffix is a good practice, as it clearly distinguishes these fields in the result set. This approach will be particularly useful if the query is extended to include fields from the joined table in the future.
25-26: LGTM: Appropriate query returnReturning the constructed query object is the correct approach. This allows for flexibility in how the query is used, enabling further chaining or immediate execution by the caller as needed.
1-26: Overall assessment: Well-implemented and maintainable query functionThe
qDisputesfunction is well-structured, efficient, and follows good coding practices. It effectively handles dispute queries with optional filtering for active disputes. The use of TypeScript, clear variable naming, and helpful comments contribute to the code's maintainability. The left join approach for identifying stale disputes is a smart solution.While the implementation is already of high quality, the minor suggestions provided in the review (such as improving variable naming in the join clause and extracting the condition in the WHERE clause) can further enhance readability and maintainability.
Great job on this implementation!
packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol (1)
Line range hint
1-34: Overall approval of changes and integrationThe modifications to
createFixturesV2are well-integrated with the rest of the contract. The test functions andbridgefunction remain unchanged and continue to use the parameters set by the new setup functions. This maintains the integrity of the existing tests while improving the code structure.The changes enhance the contract's maintainability without altering its core functionality or test coverage.
packages/rfq-indexer/api/src/controllers/conflictingProofsController.ts (2)
44-44: Approve updated response message.The response message now accurately reflects that only active conflicting proofs are being queried. This change is consistent with the modification to the
qProofsfunction call and provides clearer information to the client.
Line range hint
1-50: Overall improvements to conflicting proofs query, but additional testing recommended.The changes made to this file consistently improve the specificity of the conflicting proofs query by focusing on active proofs. This is reflected in the database query, response message, and error logging. These modifications enhance the clarity and potentially the performance of the
conflictingProofsController.To ensure these changes don't have unintended consequences, please consider the following:
- Update unit tests to reflect the new focus on active proofs.
- Verify that any other parts of the system that might expect all proofs (both active and inactive) are not negatively impacted.
- Consider adding integration tests to validate the end-to-end behavior of this controller with the new changes.
Would you like assistance in generating these additional tests?
packages/rfq-indexer/api/src/graphql/types/events.graphql (1)
Line range hint
22-81: LGTM! Formatting improvements enhance readability.The formatting changes to the existing types improve the overall consistency and readability of the file. The adjustments to whitespace and line breaks make the structure of each type more clear and uniform.
packages/rfq-indexer/api/src/routes/disputesRoute.ts (1)
1-5: LGTM: Imports and router setup are well-structured.The imports and router initialization follow best practices for Express.js applications. The separation of the controller logic into a separate file (
disputesController) promotes modularity and maintainability.packages/rfq-indexer/api/src/controllers/transactionIdController.ts (2)
4-4: LGTM: Import addition for dispute handlingThe addition of
qDisputesto the imports is consistent with the new dispute handling functionality introduced in this update.
25-25: Approved: Integration of dispute data in the combined queryThe addition of the left join for disputes and the selection of all dispute fields in the combined query ensures that dispute information is properly integrated into the transaction data retrieval process.
To ensure the dispute integration is complete, please verify that all necessary dispute fields are being used in the
nest_resultsfunction or any subsequent processing of the query results.Also applies to: 31-31
✅ Verification successful
Verified: Dispute fields are properly utilized in result processing
The shell script results confirm that dispute-related fields are actively used within the
nestResultsfunction and thetransactionIdController.tsfile. This ensures that dispute information is correctly processed and integrated into the transaction data retrieval.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of dispute fields in result processing # Search for dispute-related field usage in nest_results function and other relevant files rg --type typescript -g '!*test*' 'dispute|disputes' packages/rfq-indexer/api/src/utils/nestResults.ts packages/rfq-indexer/api/src/controllers/transactionIdController.tsLength of output: 207
Script:
#!/bin/bash # Description: Verify the usage of dispute fields in result processing # Search for dispute-related field usage in nest_results function and other relevant files rg 'dispute|disputes' packages/rfq-indexer/api/src/utils/nestResults.ts packages/rfq-indexer/api/src/controllers/transactionIdController.tsLength of output: 997
packages/rfq-indexer/api/src/graphql/queries/queries.graphql (2)
115-118: LGTM: NewDisputedRelaytype looks goodThe new
DisputedRelaytype is well-defined and includes the necessary fields (BridgeandBridgeProof) to represent a disputed relay. The use of non-nullable fields is appropriate for this use case.
129-129: LGTM: NewdisputedRelaysquery is well-definedThe
disputedRelaysquery is a good addition to the schema. It provides a clear way to fetch disputed relays, and its return type correctly matches the newly addedDisputedRelaytype.packages/rfq-indexer/api/src/types/index.ts (1)
100-100: LGTM! The EventType update is consistent with the new interface.The addition of 'DISPUTE' to the
EventTypetype is appropriate and aligns with the introduction of theBridgeProofDisputedEventsinterface. This update ensures that the system can properly handle and categorize dispute events.packages/contracts-rfq/CHANGELOG.md (1)
6-11: LGTM: New version entry is well-formatted and informative.The new version entry (0.7.0) is clear, concise, and follows the conventional commit format. It provides useful information about the new feature and includes links to the related issue and pull request, which is helpful for tracking the change.
🧰 Tools
🪛 Markdownlint
9-9: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
packages/rfq-indexer/api/src/queries/proofsQueries.ts (2)
4-4: Function signature updated appropriatelyThe addition of the optional
activeOnlyparameter with a default value offalseis correctly implemented and adds flexibility to the function.
20-22: Conditional filtering based on 'activeOnly' parameter is correctThe
if (activeOnly)block correctly filters out proofs that have been disputed by checking fornullinBridgeProofDisputedEvents.transactionId. This ensures only active proofs are returned when required.packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol (1)
8-8: Initialization ofCALL_PARAMSis appropriateThe
CALL_PARAMSconstant is correctly initialized usingabi.encode("Hello, World!"), ensuring proper encoding of the string.packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol (5)
11-18: Tests are correctly setting up negative exclusivity parametersThe negative values assigned to
quoteExclusivitySecondsand adjustments toexclusivityEndTimeeffectively simulate invalid exclusivity conditions. This setup is appropriate for testing how the contract handles incorrect exclusivity parameters.
Line range hint
27-29: Test correctly expects a revert when exclusivity end time is zeroIn
test_bridge_revert_exclusivityEndTimeZero, settingtokenParamsV2.quoteExclusivitySecondsto-int256(block.timestamp)is an accurate approach to produce an exclusivity end time of zero. The expectation of a revert withExclusivityParamsIncorrectis appropriate.
Line range hint
33-35: Test accurately checks for underflow in exclusivity periodThe function
test_bridge_revert_exclusivityPeriodUnderflowcorrectly setsquoteExclusivitySecondsto-int256(block.timestamp + 1)to induce an underflow condition. Expecting a revert due toExclusivityParamsIncorrectaligns with the intended test scenario.
Line range hint
39-41: ETH exclusivity end time zero test is correctly implementedThe test
test_bridge_eth_revert_exclusivityEndTimeZeromirrors the token test for ETH parameters. SettingethParamsV2.quoteExclusivitySecondsto-int256(block.timestamp)appropriately creates an invalid exclusivity end time, and the expected revert is correctly handled.
Line range hint
45-47: ETH exclusivity period underflow test is accurateIn
test_bridge_eth_revert_exclusivityPeriodUnderflow, the setup induces an underflow by assigning-int256(block.timestamp + 1)toquoteExclusivitySeconds. The test correctly anticipates a revert withExclusivityParamsIncorrect.packages/rfq-indexer/api/src/controllers/pendingTransactionsController.ts (1)
14-14:⚠️ Potential issueVerify that
blockTimestamp_depositandsevenDaysAgouse the same time unitsEnsure that
blockTimestamp_depositandsevenDaysAgoare both in the same time units (either seconds or milliseconds) to avoid incorrect date comparisons. IfblockTimestamp_depositis in milliseconds whilesevenDaysAgois in seconds, the comparison may not work as intended.Run the following script to verify the units of
blockTimestamp_deposit:Also applies to: 122-123
packages/contracts-rfq/test/FastBridgeV2.t.sol (4)
127-138: Initialization oftokenParamsV2andethParamsV2with default values looks correctThe
tokenParamsV2andethParamsV2structures are properly initialized with default values, setting up the base parameters for the tests.
166-169:setTokenTestCallParamsfunction implementation is correctThe function
setTokenTestCallParamscorrectly assigns thecallParamsto bothtokenParamsV2andtokenTx, ensuring consistency in test parameters.
180-183:setEthTestCallParamsfunction implementation is correctThe function
setEthTestCallParamsappropriately sets thecallParamsfor bothethParamsV2andethTx, maintaining consistency in the test setup.
213-221:getMockQuoteIdfunction implementation is appropriateThe
getMockQuoteIdfunction correctly returns mock quote IDs based on the provided relayer address. The implementation is suitable for testing purposes.packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol (3)
155-155: Confirm the existence ofAddress.FailedInnerCallin theAddresslibrarySimilarly, verify that
Address.FailedInnerCall.selectorreferences a valid custom error in theAddresslibrary to prevent unexpected test failures.Run the following script to check for
FailedInnerCall:#!/bin/bash # Description: Verify that `FailedInnerCall` is defined in OpenZeppelin's `Address` library. # Test: Search for the `FailedInnerCall` custom error in the `Address.sol` file. fd 'Address.sol' | xargs grep -E 'error\s+FailedInnerCall\(' # Expected: The custom error `FailedInnerCall` should be declared in the library.Also applies to: 161-161, 167-167, 173-173
61-61: Verify the availability of custom errors in OpenZeppelin'sAddresslibraryEnsure that
Address.AddressEmptyCode.selectorcorrectly references a custom error defined in theAddresslibrary. If using an older version of OpenZeppelin contracts, these custom errors may not be available.Run the following script to confirm the presence of
AddressEmptyCode:Also applies to: 67-67, 73-73, 79-79
51-54: Ensureabi.encodeCallmatches the function signature offastBridgeTransferReceivedVerify that the parameters used in
abi.encodeCallcorrectly match the function signature inRecipientMock. Any mismatch can lead to failed expectations in the test.Run the following script to confirm the function signature:
packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (3)
Line range hint
6-44: Imports and initialization are correctly implementedThe import statements and the
setUpfunction correctly initialize the mock recipient contracts for testing. The contracts are instantiated and labeled appropriately.
46-57: Recipient setter functions are correctly definedThe
setTokenTestRecipientandsetEthTestRecipientfunctions accurately set the recipient addresses for token and ETH tests, ensuring flexibility in assigning different mock recipients during testing.
58-60: assertEmptyCallParams function is correctly implementedThe
assertEmptyCallParamsfunction properly verifies that thecallParamsare empty, which is crucial for tests where no call parameters are expected.packages/contracts-rfq/contracts/FastBridgeV2.sol (9)
5-5: ImportingAddressLibrary is AppropriateThe addition of
import {Address} from "@openzeppelin/contracts/utils/Address.sol";is necessary and appropriate for usingAddress.functionCallWithValuelater in the contract.
13-13: ImportingIFastBridgeRecipientInterfaceIncluding
import {IFastBridgeRecipient} from "./interfaces/IFastBridgeRecipient.sol";is essential for interacting with recipient contracts via the_checkedCallRecipientfunction.
29-31: Enforcing Maximum Length forcallParamsDefining
MAX_CALL_PARAMS_LENGTHand using it to limit the length ofcallParamsenhances security by preventing excessively large inputs that could lead to DoS attacks.
53-58: Ensure Default Parameters inBridgeParamsV2Are AppropriateThe default values provided for
quoteRelayer,quoteExclusivitySeconds,quoteId, andcallParamsin thebridgefunction are set to neutral values. Please verify that these defaults align with the intended behavior and do not introduce unintended side effects, especially concerning exclusivity periods.
148-148: ValidatingcallParamsLength Enhances SecurityThe check
if (paramsV2.callParams.length > MAX_CALL_PARAMS_LENGTH) revert CallParamsLengthAboveMax();ensures thatcallParamsdo not exceed the maximum allowed length, preventing potential DoS attacks due to excessively large inputs.
180-180: Safe Casting ofexclusivityEndTimeCasting
exclusivityEndTimetouint256is safe here because prior validation ensures it falls within the valid range(0 .. params.deadline].
232-256: Ensure Correct Handling of ETH Transfers andmsg.valueIn the
relayfunction, whencallParamsis empty and the token is ETH, the relayer must includemsg.valueequal toamountto successfully transfer ETH to the recipient. Please ensure that this requirement is clearly documented and that relayers are aware to provide the correctmsg.valueto prevent transaction failures.
269-270: Deprecation of Gas Rebate FunctionSetting
chainGasAmountto0and noting that the gas rebate function is deprecated is appropriate if gas rebates are no longer supported. Ensure that any related documentation and interfaces are updated accordingly.
360-379: Ensure Recipient Contracts Correctly ImplementfastBridgeTransferReceivedTo guarantee successful execution of
_checkedCallRecipient, ensure that all recipient contracts correctly implement thefastBridgeTransferReceivedfunction as specified in theIFastBridgeRecipientinterface, including returning the correct function selector.Run the following script to verify the implementations:
✅ Verification successful
All Recipient Contracts Correctly Implement
fastBridgeTransferReceivedVerified that all contracts implementing
fastBridgeTransferReceivedreturn the correct function selector as specified in theIFastBridgeRecipientinterface.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all contracts implementing 'fastBridgeTransferReceived' return the correct selector. # Find all Solidity files that implement 'fastBridgeTransferReceived' with the correct signature ast-grep --lang solidity --pattern 'function fastBridgeTransferReceived(address $_, uint256 $_, bytes calldata $_) returns (bytes4) { $$$ }' # Optionally, inspect the function body to ensure the correct selector is returned # Expected return statement: return this.fastBridgeTransferReceived.selector;Length of output: 259
Script:
#!/bin/bash # Description: Verify that all contracts implementing 'fastBridgeTransferReceived' return the correct selector. # Find all Solidity files that implement 'fastBridgeTransferReceived' with the correct signature rg -l 'function\s+fastBridgeTransferReceived\s*\(\s*address\s+\w+,\s*uint256\s+\w+,\s*bytes\s+calldata\s+\w+\s*\)\s+returns\s+\(bytes4\)' -- '*.sol' | while read -r file; do # Check if the return statement is correct if ! rg -q 'return\s+this\.fastBridgeTransferReceived\.selector\s*;' "$file"; then echo "Incorrect return statement in $file" fi doneLength of output: 211
Script:
#!/bin/bash # Description: Comprehensive verification of 'fastBridgeTransferReceived' implementations in Solidity contracts. # Find all Solidity files in the repository sol_files=$(fd -e sol) if [ -z "$sol_files" ]; then echo "No Solidity files found in the repository." exit 0 fi # Iterate through each Solidity file for file in $sol_files; do # Check if the file contains the 'fastBridgeTransferReceived' function with the correct signature if rg -q 'function\s+fastBridgeTransferReceived\s*\(\s*address\s+\w+,\s*uint256\s+\w+,\s*bytes\s+calldata\s+\w+\s*\)\s+returns\s+\(bytes4\)' "$file"; then # Check if the function returns the correct selector if ! rg -A3 'function\s+fastBridgeTransferReceived\s*\(\s*address\s+\w+,\s*uint256\s+\w+,\s*bytes\s+calldata\s+\w+\s*\)\s+returns\s+\(bytes4\)' "$file" | rg -q 'return\s+this\.fastBridgeTransferReceived\.selector\s*;'; then echo "Incorrect return statement in $file" fi fi doneLength of output: 91820
packages/rfq-indexer/api/src/graphql/resolvers.ts (2)
91-103: Consistent addition ofqDisputesfunctionThe
qDisputesfunction is correctly implemented and follows the established pattern used in other query functions. It accurately selects the necessary fields from theBridgeProofDisputedEventstable.
237-249: IncludeBridgeProofDisputedEventsin theeventsqueryThe inclusion of
BridgeProofDisputedEventsin theeventsresolver ensures that dispute events are now part of the overall event retrieval process. The selected fields are consistent with other event types.
| interface IFastBridgeRecipient { | ||
| function fastBridgeTransferReceived( | ||
| address token, | ||
| uint256 amount, | ||
| bytes memory callParams | ||
| ) | ||
| external | ||
| payable | ||
| returns (bytes4); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Incorrect implementations of IFastBridgeRecipient found in test mocks:
packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol: Missingbytes4return value.packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol: Returns additionaluint256value.packages/contracts-rfq/test/mocks/NonPayableRecipient.sol: Function ispureinstead ofpayable.
Please ensure all contracts implementing this interface adhere strictly to its specifications to maintain consistency and prevent potential issues.
🔗 Analysis chain
Consider security implications and error handling in implementations.
The IFastBridgeRecipient interface provides a flexible structure for handling incoming fast bridge transfers. When implementing this interface, consider the following:
-
Security: Ensure that only authorized bridge contracts can call this function to prevent unauthorized transfers.
-
Error handling: Implement robust error handling in the contract that implements this interface. Consider how failed transfers should be managed.
-
Gas optimization: The
callParamsallows for flexible data passing, but be mindful of gas costs when processing this data. -
Reentrancy: Since the function is
payableand may handle token transfers, implement reentrancy guards in the implementing contract. -
Return value: Clarify the expected
bytes4return value. If it's meant to be a function selector for acknowledgment, consider defining it as a constant in the interface:
bytes4 constant TRANSFER_RECEIVED = bytes4(keccak256("fastBridgeTransferReceived(address,uint256,bytes)"));This will allow easy verification in the calling contract.
To verify the usage of this interface across the project, run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for contracts implementing IFastBridgeRecipient
echo "Contracts implementing IFastBridgeRecipient:"
rg --type solidity "contract.*is.*IFastBridgeRecipient"
# Search for calls to fastBridgeTransferReceived
echo "\nCalls to fastBridgeTransferReceived:"
rg --type solidity "fastBridgeTransferReceived\("
Length of output: 361
Script:
#!/bin/bash
# Search for contracts implementing IFastBridgeRecipient
echo "Contracts implementing IFastBridgeRecipient:"
rg --glob '*.sol' 'contract\s+\w+\s+is\s+.*IFastBridgeRecipient'
# Search for calls to fastBridgeTransferReceived
echo "\nCalls to fastBridgeTransferReceived:"
rg --glob '*.sol' 'fastBridgeTransferReceived\('
Length of output: 1614
| } catch (error) { | ||
| console.error('Error fetching active disputes:', error) | ||
| res.status(500).json({ message: 'Internal server error' }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and logging.
While the current error handling structure is appropriate for an Express.js controller, there's room for improvement:
- Consider using a more robust logging solution instead of
console.error. This will be crucial for debugging in a production environment. - It might be beneficial to provide more detailed error information in development environments while keeping it generic in production.
- Implement error classification to provide more specific HTTP status codes when appropriate.
Here's an example of how you could enhance the error handling:
import logger from '../utils/logger'; // Assume we have a logger utility
try {
// ... existing code ...
} catch (error) {
logger.error('Error fetching active disputes:', error);
if (process.env.NODE_ENV === 'development') {
res.status(500).json({ message: 'Internal server error', error: error.message });
} else {
res.status(500).json({ message: 'Internal server error' });
}
}| <img width="28" height="28" src={chainImg} alt={chain.name} /> | ||
| {chain.name} <code>{id}</code> |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent chainImg Usage Across Codebase
The change to use chainImg as a string in docs/bridge/src/components/Routes.tsx is not consistently applied elsewhere. In several files, chainImg is still accessed as an object with a src property, which may lead to potential runtime errors.
- For example:
packages/synapse-interface/components/ui/SelectSpecificTokenButton.tsxpackages/synapse-interface/components/ui/SelectorWrapper.tsxpackages/synapse-interface/constants/chains/master.tsx
Please ensure that the chainImg type is uniformly updated across the codebase to prevent inconsistencies.
🔗 Analysis chain
Simplified chain image rendering
The changes improve readability by directly using chainImg as the image source. However, it appears that chainImg has changed from a function to a string URL.
Could you confirm if chainImg is now always expected to be a string URL? If so, has this change been consistently applied across the codebase? Let's verify:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of chainImg to ensure consistency
rg --type typescript --type javascript 'chainImg'
Length of output: 89
Script:
#!/bin/bash
# Search for usages of chainImg to ensure consistency using file extensions
rg 'chainImg' --type-add 'ts=*.ts,*.tsx' --type-add 'js=*.js,*.jsx' -t ts -t js
Length of output: 139
Script:
#!/bin/bash
# Search for usages of chainImg to ensure consistency using correct type definitions
rg 'chainImg' --type-add 'ts:*.ts,*.tsx' --type-add 'js:*.js,*.jsx' -t ts -t js
Length of output: 5908
Script:
#!/bin/bash
# Search for usages of chainImg using glob patterns to avoid type definition errors
rg 'chainImg' --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx'
Length of output: 5900
| @@ -13,7 +13,7 @@ export const conflictingProofsController = async ( | |||
| const query = db | |||
| .with('deposits', () => qDeposits()) | |||
| .with('relays', () => qRelays()) | |||
| .with('proofs', () => qProofs()) | |||
| .with('proofs', () => qProofs({activeOnly: true})) | |||
There was a problem hiding this comment.
💡 Codebase verification
Multiple qProofs() calls lack the activeOnly parameter.
The recent update adds {activeOnly: true} to qProofs() in conflictingProofsController.ts, but several other parts of the codebase still call qProofs() without this filter. This may lead to inconsistent data handling and potential performance issues.
Affected Files:
packages/rfq-indexer/api/src/graphql/resolvers.ts(5 instances)packages/rfq-indexer/api/src/controllers/transactionIdController.ts(1 instance)packages/rfq-indexer/api/src/controllers/pendingTransactionsController.ts(2 instances)
Please review and update these usages to ensure consistent behavior across the application.
🔗 Analysis chain
Approve filtering for active proofs, but verify impact.
The addition of {activeOnly: true} to qProofs() effectively filters for active proofs only. This change aligns with the updated response message and potentially improves query performance.
To ensure this change doesn't negatively impact other parts of the system, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other usages of qProofs that might be affected by this change
# Test: Search for other usages of qProofs. Expect: No usages without activeOnly parameter.
rg --type typescript 'qProofs\s*\(\s*\)' -g '!conflictingProofsController.ts'
Length of output: 114
Script:
#!/bin/bash
# Description: Check for other usages of qProofs that might be affected by this change
# Search for qProofs() calls in .ts and .tsx files, excluding conflictingProofsController.ts
rg 'qProofs\s*\(' --glob '!conflictingProofsController.ts' --glob '*.ts' --glob '*.tsx'
Length of output: 1136
| All API calls can be viewed in Swagger: | ||
|
|
||
| [Swagger Documentation](http://localhost:3001/api-docs) | ||
|
|
There was a problem hiding this comment.
Update Swagger documentation link
The addition of Swagger documentation is great for API users. However, the current link points to localhost, which won't be accessible to most users.
Please update the Swagger link to point to the production URL. For example:
-[Swagger Documentation](http://localhost:3001/api-docs)
+[Swagger Documentation](https://triumphant-magic-production.up.railway.app/api-docs)Committable suggestion was skipped due to low confidence.
| mockRecipientRevert(tokenTx); | ||
| vm.expectRevert(REVERT_MSG); | ||
| relay({caller: relayerA, msgValue: 0, bridgeTx: tokenTx}); | ||
| } |
There was a problem hiding this comment.
Handle revert messages correctly in recipient revert tests
In the tests where you expect the recipient to revert with a custom message, ensure that the revert data is correctly encoded and that vm.expectRevert is matching the revert reason accurately.
Consider updating the expected revert to match Solidity's standard revert format:
vm.expectRevert(REVERT_MSG);
+vm.expectRevert(abi.encodeWithSignature("Error(string)", REVERT_MSG));Alternatively, if you're using Solidity 0.8.4 or later, you can define and use custom errors for clarity.
Also applies to: 220-223, 226-229, 232-235
| } | ||
|
|
||
| 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.
Replace vm.mockCallRevert with vm.mockCall to mock revert behavior
The cheat code vm.mockCallRevert does not exist in Foundry's cheat codes. To mock a reverting call, you should use vm.mockCall and provide the revert data that represents a revert with the specific message.
Apply this diff to fix the issue:
function mockRecipientRevert(IFastBridgeV2.BridgeTransactionV2 memory bridgeTx) public {
- vm.mockCallRevert({callee: userB, data: getExpectedCalldata(bridgeTx), revertData: bytes(REVERT_MSG)});
+ bytes memory revertData = abi.encodeWithSelector(
+ bytes4(keccak256("Error(string)")),
+ REVERT_MSG
+ );
+ vm.mockCall(userB, getExpectedCalldata(bridgeTx), revertData);
}📝 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.
| vm.mockCallRevert({callee: userB, data: getExpectedCalldata(bridgeTx), revertData: bytes(REVERT_MSG)}); | |
| bytes memory revertData = abi.encodeWithSelector( | |
| bytes4(keccak256("Error(string)")), | |
| REVERT_MSG | |
| ); | |
| vm.mockCall(userB, getExpectedCalldata(bridgeTx), revertData); |
|
|
||
| function test_relay_eth_noCallParams_revert_recipientReverts() public { | ||
| setEthTestCallParams(""); | ||
| vm.mockCallRevert({callee: userB, data: "", revertData: bytes(REVERT_MSG)}); |
There was a problem hiding this comment.
Correct the use of vm.mockCallRevert when mocking ETH transfer reverts
Similar to the previous issue, vm.mockCallRevert is not a valid cheat code. Use vm.mockCall with the appropriate revert data to simulate the recipient reverting on an ETH transfer.
Apply this diff to fix the issue:
vm.mockCallRevert({callee: userB, data: "", revertData: bytes(REVERT_MSG)});
+bytes memory revertData = abi.encodeWithSelector(
+ bytes4(keccak256("Error(string)")),
+ REVERT_MSG
+);
+vm.mockCall(userB, "", revertData);Committable suggestion was skipped due to low confidence.
| // 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 comment indicates that the note is outdated due to the addition of callParams in the BridgeTransactionV2 struct. Please update or remove the comment to reflect the current state of the code.
Would you like assistance in updating this comment or addressing the TODO?
| 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
Remove unused CTEs deposits and relays
In the disputedRelays resolver, the common table expressions (CTEs) deposits and relays are defined but not utilized in the query. Removing these unused CTEs will optimize the query.
Apply this diff to remove the unused CTEs:
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())
},📝 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('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()) | |
| }, |
Description
FE Release only for #3238 (supersedes: #3240)
f7cda31: docs preview link
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores