Fix RFQ events, pragmas, refund
#1778
Conversation
mikeyrf
requested review from
ChiTimesChi,
aureliusbtc and
trajan0x
as code owners
|
Warning Rate Limit Exceeded@trajan0x has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 16 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe updates revolve around enhancing the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/contracts-rfq/contracts/interfaces/IFastBridge.sol (1 hunks)
- packages/contracts-rfq/test/FastBridge.t.sol (6 hunks)
Files skipped from review due to trivial changes (1)
- packages/contracts-rfq/contracts/interfaces/IFastBridge.sol
Additional comments: 6
packages/contracts-rfq/test/FastBridge.t.sol (6)
363-363: The addition of the
indexedkeyword to theBridgeRequestedevent parameterstransactionIdandsenderis correct and aligns with the PR objectives to enhance event filtering capabilities.839-839: The addition of the
indexedkeyword to theBridgeRelayedevent parameterstransactionId,relayer, andtois correct and aligns with the PR objectives to enhance event filtering capabilities.1091-1091: The addition of the
indexedkeyword to theBridgeProofProvidedevent parametertransactionIdis correct and aligns with the PR objectives to enhance event filtering capabilities.1222-1222: The addition of the
indexedkeyword to theBridgeDepositClaimedevent parameterstransactionIdandrelayeris correct and aligns with the PR objectives to enhance event filtering capabilities.1457-1457: The addition of the
indexedkeyword to theBridgeProofDisputedevent parametertransactionIdis correct and aligns with the PR objectives to enhance event filtering capabilities.1554-1554: The addition of the
indexedkeyword to theBridgeDepositRefundedevent parameterstransactionIdandtois correct and aligns with the PR objectives to enhance event filtering capabilities.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1778 +/- ##
===================================================
+ Coverage 50.86557% 50.98229% +0.11672%
===================================================
Files 395 395
Lines 26861 26825 -36
Branches 295 295
===================================================
+ Hits 13663 13676 +13
+ Misses 11866 11820 -46
+ Partials 1332 1329 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- packages/contracts-rfq/contracts/Admin.sol (1 hunks)
- packages/contracts-rfq/contracts/FastBridge.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IAdmin.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridge.sol (2 hunks)
Files skipped from review due to trivial changes (2)
- packages/contracts-rfq/contracts/Admin.sol
- packages/contracts-rfq/contracts/interfaces/IAdmin.sol
Files skipped from review as they are similar to previous changes (1)
- packages/contracts-rfq/contracts/interfaces/IFastBridge.sol
Additional comments: 2
packages/contracts-rfq/contracts/FastBridge.sol (2)
- 2-2: The Solidity version has been pinned to
0.8.20from a previous caret version^0.8.20. This change prevents the contract from compiling with any Solidity compiler version other than0.8.20. Ensure that this is intentional and that there are no features or fixes in newer compiler versions that the contract relies on.Verification successful
The verification process has confirmed that the main contracts within the
contracts-rfqdirectory are using the pinned Solidity version0.8.20, while a test file and a script file are using the caret version^0.8.20. This setup suggests a deliberate choice to compile the main contracts with a specific version for stability, while allowing tests and scripts to be compatible with newer versions of the Solidity compiler.* 2-2: The PR title suggests changes related to adding the `indexed` keyword to event parameters, but no such changes are visible in the provided code. Please verify if the correct file has been modified or if the intended changes are missing from this PR.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that there are no newer Solidity versions being used in other contracts. rg 'pragma solidity' --files-with-matches | xargs rg '^pragma solidity \^?0\.8\.20;'Length of output: 523
Verification successful
The search in the
IFastBridge.solfile has successfully found event definitions that include theindexedkeyword. This confirms that the changes mentioned in the PR, which aimed to enhance the functionality of Request for Quote (RFQ) events by adding theindexedkeyword to specific event parameters, have been made in the correct file.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for event definitions in the FastBridge contract to verify if 'indexed' keyword is present. ast-grep --lang solidity --pattern $'contract FastBridge { $$$ event $_($_) { $$$ } $$$ }'Length of output: 208
Script:
#!/bin/bash # Search for event definitions with the 'indexed' keyword in the FastBridge.sol file. rg 'event .+\(.*indexed.*\)' packages/contracts-rfq/contracts/FastBridge.solLength of output: 78
Script:
#!/bin/bash # Search for any event definitions in the FastBridge.sol file to confirm their presence or absence. rg 'event ' packages/contracts-rfq/contracts/FastBridge.solLength of output: 93
Script:
#!/bin/bash # Search for event definitions with the 'indexed' keyword in the IFastBridge.sol interface file. rg 'event .+\(.*indexed.*\)' packages/contracts-rfq/contracts/IFastBridge.solLength of output: 168
Script:
#!/bin/bash # Locate the IFastBridge.sol file within the repository. fd 'IFastBridge.sol'Length of output: 80
Script:
#!/bin/bash # Search for event definitions with the 'indexed' keyword in the correct IFastBridge.sol file. rg 'event .+\(.*indexed.*\)' packages/contracts-rfq/contracts/interfaces/IFastBridge.solLength of output: 640
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -1,5 +1,5 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.20; | |||
| pragma solidity 0.8.20; | |||
Check warning
Code scanning / Slither
Different pragma directives are used Warning
| @@ -1,5 +1,5 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.20; | |||
| pragma solidity 0.8.20; | |||
Check warning
Code scanning / Slither
Incorrect versions of Solidity Warning
| @@ -1,5 +1,5 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.20; | |||
| pragma solidity ^0.8.0; | |||
Check warning
Code scanning / Slither
Incorrect versions of Solidity Warning
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- packages/contracts-rfq/contracts/FastBridge.sol (2 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridge.sol (2 hunks)
- packages/contracts-rfq/test/FastBridge.t.sol (11 hunks)
- packages/contracts-rfq/test/FastBridgeMock.sol (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/contracts-rfq/contracts/FastBridge.sol
- packages/contracts-rfq/contracts/interfaces/IFastBridge.sol
- packages/contracts-rfq/test/FastBridge.t.sol
Additional comments: 3
packages/contracts-rfq/test/FastBridgeMock.sol (3)
46-46: The change from
msg.sendertoparams.senderin theBridgeTransactionstruct within themockBridgeRequestfunction alters the source of the sender's address. This should be verified to ensure that it aligns with the intended logic and that all calls to this function have been updated accordingly.59-69: The
emit BridgeRequestedevent now includes additional parameters. Ensure that all subscribers to this event are updated to handle the new event signature. This is crucial for maintaining compatibility with off-chain services that listen to these events.73-84: The
mockBridgeRequestRawfunction now emits theBridgeRequestedevent with additional parameters extracted from theBridgeTransactionstruct. This change should be verified to ensure that the new parameters are correctly populated and that all subscribers to this event are updated to handle the new event signature.
| bytes32 indexed transactionId, | ||
| address indexed sender, | ||
| bytes request, | ||
| uint32 destChainId, | ||
| address originToken, | ||
| address destToken, | ||
| uint256 originAmount, | ||
| uint256 destAmount, | ||
| bool sendChainGas |
There was a problem hiding this comment.
This feels like everything we might want to use in the future.
| bytes32 indexed transactionId, | ||
| address indexed relayer, | ||
| address indexed to, | ||
| address token, | ||
| uint256 amount, | ||
| uint256 chainGasAmount |
There was a problem hiding this comment.
Let's add originChainId here. We would also want to have the same origin/destToken and origin/destAmount pairs being emitted here as in BridgeRequested
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- packages/contracts-rfq/contracts/FastBridge.sol (3 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridge.sol (2 hunks)
- packages/contracts-rfq/test/FastBridge.t.sol (15 hunks)
- packages/contracts-rfq/test/FastBridgeMock.sol (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- packages/contracts-rfq/contracts/FastBridge.sol
- packages/contracts-rfq/contracts/interfaces/IFastBridge.sol
- packages/contracts-rfq/test/FastBridge.t.sol
- packages/contracts-rfq/test/FastBridgeMock.sol
Co-authored-by: Trajan0x <[email protected]>
mikeyrf
changed the title
refund()refund
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- packages/contracts-rfq/contracts/FastBridge.sol (4 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridge.sol (3 hunks)
- packages/contracts-rfq/test/FastBridge.t.sol (22 hunks)
- packages/contracts-rfq/test/FastBridgeMock.sol (3 hunks)
Additional comments: 29
packages/contracts-rfq/contracts/interfaces/IFastBridge.sol (5)
2-2: The pragma solidity version has been broadened from
^0.8.20to^0.8.0. This change increases compatibility with a wider range of compiler versions, which could be beneficial for future updates or interoperability with other contracts. However, it's important to ensure that all contract code is compatible with the full range of compilers from0.8.0onwards.27-36: The
BridgeRequestedevent has been expanded with additional parameters, includingdestChainId,originToken,destToken,originAmount,destAmount, andsendChainGas. These changes will provide more detailed information about bridge requests, which can be useful for off-chain services tracking these events. Ensure that all off-chain services that consume these events are updated to handle the new event structure.39-47: Similarly, the
BridgeRelayedevent now includes new parameters such asoriginChainId,originToken,destToken,originAmount,destAmount, andchainGasAmount. This enhancement will provide more transparency and traceability for relayed bridge transactions. As with theBridgeRequestedevent, any off-chain services listening to this event must be updated accordingly.49-54: New events
BridgeProofProvided,BridgeProofDisputed, andBridgeDepositRefundedhave been added with indexed parameters. These events will improve the observability of the bridge's state changes and dispute mechanisms. It's crucial to ensure that the logic emitting these events is correctly implemented in the contract functions.94-94: The
refundfunction signature has been modified to remove thetoparameter. This change simplifies the refund process by eliminating the need to specify a recipient, which will always be the original sender of the bridge request. Ensure that this change is reflected in all parts of the system that interact with therefundfunction.packages/contracts-rfq/test/FastBridgeMock.sol (5)
46-46: The
originSenderis now derived from theparamsobject instead ofmsg.senderin themockBridgeRequestfunction. This change allows for more flexibility in testing by specifying different senders. Ensure that all tests are updated to pass the correctsenderwithin theparamsobject.59-69: The
mockBridgeRequestfunction's event emission has been updated to include the new parameters. This change ensures that the mock contract's behavior aligns with the updatedFastBridgecontract. Verify that the tests are correctly handling the new event parameters.73-84: The
mockBridgeRequestRawfunction has been updated to emit theBridgeRequestedevent with the new parameters. This consistency with the actual contract's behavior is crucial for accurate testing. Confirm that the tests account for these changes.98-100: The
mockBridgeRelayerfunction's event emission has been updated to include the new parameters for theBridgeRelayedevent. This update is necessary to maintain parity with the changes in theFastBridgecontract. Ensure that the tests reflect these changes.127-127: The
refundfunction signature in theFastBridgeMockcontract has been updated to match theIFastBridgeinterface. This change will help maintain consistency across the codebase. Confirm that the mock contract'srefundfunction is not called with the old signature anywhere in the tests.packages/contracts-rfq/contracts/FastBridge.sol (3)
110-119: The
BridgeRequestedevent in theFastBridgecontract now includes additional parameters, which aligns with the changes in theIFastBridgeinterface. This update will provide more detailed information for off-chain services and should be communicated to any stakeholders who rely on these events.152-161: The
BridgeRelayedevent has been updated with new parameters, consistent with the changes to theIFastBridgeinterface. This enhancement will improve the transparency of relayed transactions. Ensure that any systems or services that listen to this event are updated to handle the new parameters.239-249: The
refundfunction's signature has been modified to remove thetoparameter, and the logic has been updated to send the collateral back to the original sender. This simplification is consistent with the changes in theIFastBridgeinterface and should be reflected in all contract interactions.packages/contracts-rfq/test/FastBridge.t.sol (16)
363-372: The
BridgeRequestedevent has been updated with additional parameters. Ensure that all contracts and off-chain systems that listen to this event are updated to handle the new event signature.899-907: The
BridgeRelayedevent has been updated with additional parameters. Ensure that all contracts and off-chain systems that listen to this event are updated to handle the new event signature.Verification successful
I will now generate a new shell script using
rgto search for instances where theBridgeRelayedevent is used in the codebase.
I will generate a new shell script to search for the
BridgeRelayedevent without specifying the file type, focusing on.solfiles.
The search results indicate that the
BridgeRelayedevent is declared in two files:FastBridge.t.solandIFastBridge.sol. It is also emitted in multiple places withinFastBridgeMock.sol,FastBridge.t.sol, andFastBridge.sol.Given this information, it is confirmed that the
BridgeRelayedevent has been updated and is used in several places within the codebase. Therefore, the review comment is correct in stating that all contracts and off-chain systems that listen to this event need to be updated to handle the new event signature.* 387-397: The test `test_successfulBridge` emits the `BridgeRequested` event with the updated parameters. Ensure that the test reflects the correct usage of the new event parameters and that the expected values match the actual values used in the contract's logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for instances where the BridgeRelayed event is used. ast-grep --lang solidity --pattern $'event BridgeRelayed($_, $_, $_, $_, $_, $_, $_, $_, $_)'Length of output: 211
Script:
#!/bin/bash # Search for the declaration and usage of the BridgeRelayed event in Solidity files. rg 'event BridgeRelayed\(' -t solidity rg 'emit BridgeRelayed\(' -t solidityLength of output: 146
Script:
#!/bin/bash # Search for the declaration and usage of the BridgeRelayed event in .sol files. rg 'event BridgeRelayed\(' --glob '*.sol' rg 'emit BridgeRelayed\(' --glob '*.sol'Length of output: 684
487-497: The test
test_successfulBridgeWithProtocolFeeOnemits theBridgeRequestedevent with the updated parameters. Ensure that the test reflects the correct usage of the new event parameters and that the expected values match the actual values used in the contract's logic, especially with the protocol fee rate applied.549-559: The test
test_successfulBridgeWithETHAndProtocolFeeOnemits theBridgeRequestedevent with the updated parameters. Ensure that the test reflects the correct usage of the new event parameters and that the expected values match the actual values used in the contract's logic, especially with the protocol fee rate applied and when using ETH as the token.610-620: The test
test_successfulBridgeWithChainGasemits theBridgeRequestedevent with the updated parameters. Ensure that the test reflects the correct usage of the new event parameters and that the expected values match the actual values used in the contract's logic, especially with thesendChainGasparameter.710-720: The test
test_successfulBridgeWithSenderNotUseremits theBridgeRequestedevent with the updated parameters. Ensure that the test reflects the correct usage of the new event parameters and that the expected values match the actual values used in the contract's logic, especially with thesenderparameter being different from theuser.937-939: The test
test_successfulRelayDestinationemits theBridgeRelayedevent with the updated parameters. Ensure that the test reflects the correct usage of the new event parameters and that the expected values match the actual values used in the contract's logic.982-992: The test
test_successfulRelayDestinationWithETHemits theBridgeRelayedevent with the updated parameters. Ensure that the test reflects the correct usage of the new event parameters and that the expected values match the actual values used in the contract's logic, especially when using ETH as the token.1038-1048: The test
test_successfulRelayDestinationWithETHAndChainGasemits theBridgeRelayedevent with the updated parameters. Ensure that the test reflects the correct usage of the new event parameters and that the expected values match the actual values used in the contract's logic, especially when using ETH as the token and including chain gas amount.1095-1097: The test
test_successfulRelayDestinationWithChainGasemits theBridgeRelayedevent with the updated parameters. Ensure that the test reflects the correct usage of the new event parameters and that the expected values match the actual values used in the contract's logic, especially with the inclusion of the chain gas amount.1183-1183: The
BridgeProofProvidedevent is declared but not used within the provided code. Ensure that this event is emitted correctly where the proof of relay is provided, and that all listeners are updated to handle this event.1314-1316: The
BridgeDepositClaimedevent is declared but not used within the provided code. Ensure that this event is emitted correctly where the deposit claim is processed, and that all listeners are updated to handle this event.1551-1551: The
BridgeProofDisputedevent is declared but not used within the provided code. Ensure that this event is emitted correctly where the proof dispute is processed, and that all listeners are updated to handle this event.1648-1648: The
BridgeDepositRefundedevent is declared but not used within the provided code. Ensure that this event is emitted correctly where the deposit refund is processed, and that all listeners are updated to handle this event.1802-1848: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-1833]
Ensure that all test functions are updated to reflect the changes in the contract's event parameters and logic. This includes verifying that the expected values in the tests match the actual values used in the contract's logic, especially with the inclusion of new parameters such as chain IDs, token addresses, amounts, and gas parameters.
Verification successful
Based on the provided script output, the test functions
test_successfulBridge,test_successfulBridgeWithETH,test_successfulRelayDestination, andtest_successfulRelayProofin theFastBridge.t.solfile appear to be correctly updated to reflect the changes in theFastBridgecontract's event parameters and logic. The tests include the new parameters for theBridgeRequestedandBridgeRelayedevents and check the correct behavior of the contract functions.For instance, in
test_successfulBridge, theBridgeRequestedevent is expected to emit with the new parameters, and the test checks that thefastBridgecontract's state is updated accordingly after a bridge transaction. Similarly,test_successfulRelayProofchecks that theBridgeProofProvidedevent is emitted and that the contract's state reflects the proof being provided.The tests also include logic to simulate transactions, set up roles, and check the contract's state after operations, which is consistent with typical testing practices for smart contracts.
Given the information from the script output, it seems that the tests are aligned with the PR objectives and the AI-generated summary of changes from the diff for
packages/contracts-rfq/test/FastBridge.t.sol.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests that might be affected by the changes in the contract's event parameters and logic. rg --type solidity "function test_"Length of output: 70
Script:
#!/bin/bash # Search for test functions in Solidity files by looking for files with the '.sol' extension and then searching for the pattern 'function test_' within those files. rg --glob '*.sol' 'function test_'Length of output: 63110
Script:
#!/bin/bash # Verify that the test functions within FastBridge.t.sol are correctly updated to reflect the changes in the contract's event parameters and logic. rg --glob '*.sol' 'function test_' 'packages/contracts-rfq/test/FastBridge.t.sol'Length of output: 4022
Script:
#!/bin/bash # Analyze specific test functions to verify that they align with the changes in the contract's event parameters and logic. # Focus on tests related to the `BridgeRequested` and `BridgeRelayed` events. rg --glob '*.sol' 'test_successfulBridge|test_successfulBridgeWithETH|test_successfulRelayDestination|test_successfulRelayProof' 'packages/contracts-rfq/test/FastBridge.t.sol'Length of output: 1901
Script:
#!/bin/bash # Request the content of specific test functions related to the `BridgeRequested` and `BridgeRelayed` events within the `FastBridge.t.sol` file. rg --glob '*.sol' -A 10 'function test_successfulBridge\(\)' 'packages/contracts-rfq/test/FastBridge.t.sol' rg --glob '*.sol' -A 10 'function test_successfulBridgeWithETH\(\)' 'packages/contracts-rfq/test/FastBridge.t.sol' rg --glob '*.sol' -A 10 'function test_successfulRelayDestination\(\)' 'packages/contracts-rfq/test/FastBridge.t.sol' rg --glob '*.sol' -A 10 'function test_successfulRelayProof\(\)' 'packages/contracts-rfq/test/FastBridge.t.sol'Length of output: 2092
Script:
#!/bin/bash # Request the full content of the test functions related to the `BridgeRequested` and `BridgeRelayed` events within the `FastBridge.t.sol` file. rg --glob '*.sol' -A 30 'function test_successfulBridge\(\)' 'packages/contracts-rfq/test/FastBridge.t.sol' rg --glob '*.sol' -A 30 'function test_successfulBridgeWithETH\(\)' 'packages/contracts-rfq/test/FastBridge.t.sol' rg --glob '*.sol' -A 30 'function test_successfulRelayDestination\(\)' 'packages/contracts-rfq/test/FastBridge.t.sol' rg --glob '*.sol' -A 30 'function test_successfulRelayProof\(\)' 'packages/contracts-rfq/test/FastBridge.t.sol'Length of output: 5557
Co-authored-by: Trajan0x <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (14)
- packages/contracts-rfq/broadcast/FastBridge.s.sol/1/run-1704831049.json
- packages/contracts-rfq/broadcast/FastBridge.s.sol/1/run-1704831763.json
- packages/contracts-rfq/broadcast/FastBridge.s.sol/1/run-1704831771.json
- packages/contracts-rfq/broadcast/FastBridge.s.sol/1/run-1704831793.json
- packages/contracts-rfq/broadcast/FastBridge.s.sol/1/run-latest.json
- packages/contracts-rfq/broadcast/FastBridge.s.sol/10/run-1704829920.json
- packages/contracts-rfq/broadcast/FastBridge.s.sol/10/run-1704829927.json
- packages/contracts-rfq/broadcast/FastBridge.s.sol/10/run-1704829947.json
- packages/contracts-rfq/broadcast/FastBridge.s.sol/10/run-latest.json
- packages/contracts-rfq/broadcast/FastBridge.s.sol/42161/run-1704829842.json
- packages/contracts-rfq/broadcast/FastBridge.s.sol/42161/run-1704829847.json
- packages/contracts-rfq/broadcast/FastBridge.s.sol/42161/run-latest.json
- services/rfq/contracts/fastbridge/fastbridge.contractinfo.json
- services/rfq/contracts/testcontracts/fastbridgemock/fastbridgemock.contractinfo.json
Files selected for processing (1)
- services/rfq/README.md (1 hunks)
Files skipped from review due to trivial changes (1)
- services/rfq/README.md
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- services/rfq/relayer/quoter/quoter.go (3 hunks)
Additional comments: 5
services/rfq/relayer/quoter/quoter.go (5)
7-9: The addition of new import statements for OpenTelemetry and hexutil is noted. Ensure that these packages are approved for use and are compatible with the project's dependencies.
91-99: The
ShouldProcessfunction now accepts aparentCtxparameter for tracing. Verify that all invocations of this function have been updated to include the new parameter.* 92-98: The implementation of tracing within `ShouldProcess` using OpenTelemetry is noted. Ensure that the tracing attributes and events are consistent with the project's observability standards.Verification successful
--- The search results from ripgrep show several invocations of the `ShouldProcess` function. The relevant lines from the output are: 1. `services/rfq/relayer/service/handlers.go: if !q.Quoter.ShouldProcess(ctx, request) {` 2. `services/rfq/relayer/quoter/quoter_test.go: s.True(s.manager.ShouldProcess(s.GetTestContext(), quote))` 3. `services/rfq/relayer/quoter/quoter_test.go: s.True(s.manager.ShouldProcess(s.GetTestContext(), quote))` 4. `services/rfq/relayer/quoter/quoter_test.go: s.False(s.manager.ShouldProcess(s.GetTestContext(), quote))` 5. `services/rfq/relayer/quoter/quoter_test.go: s.False(s.manager.ShouldProcess(s.GetTestContext(), quote))` From these lines, we can see that the `ShouldProcess` function is being called with a context parameter (`ctx` or `s.GetTestContext()`), which aligns with the updated function signature that now requires a `parentCtx` parameter. This suggests that the invocations of `ShouldProcess` have been updated to include the new parameter. Based on this information, it appears that the review comment has been addressed, and all invocations of `ShouldProcess` have been updated to pass the new `parentCtx` parameter. </details> <details> <summary>Scripts Executed</summary> The following scripts were executed for the analysis: --- Script: ```shell #!/bin/bash # Search for invocations of ShouldProcess without the parentCtx parameter. ast-grep --lang go --pattern $'($_).ShouldProcess($$$)'Length of output: 56
Script:
#!/bin/bash # Search for invocations of ShouldProcess to ensure they include the new parentCtx parameter. rg 'ShouldProcess' --goLength of output: 452
Script:
#!/bin/bash # Correct the ripgrep command to search for invocations of ShouldProcess in Go files. rg 'ShouldProcess' --type goLength of output: 1052
- 124-130: The
isProfitableQuotefunction now accepts aparentCtxparameter and returns an error along with the boolean result. Verify that all invocations of this function have been updated to handle the new return type.Verification successful
The review comment concerning the
isProfitableQuotefunction has been addressed in the codebase. The function is called with the correct parameters, and both the boolean result and the error are handled appropriately.* 88-134: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [125-145]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for invocations of isProfitableQuote without handling the new error return value. ast-grep --lang go --pattern $'($_).isProfitableQuote($$$)'Length of output: 60
Script:
#!/bin/bash # Search for any occurrences of the function isProfitableQuote to confirm it is being used. rg 'isProfitableQuote'Length of output: 534
The implementation of tracing within
isProfitableQuoteusing OpenTelemetry is noted. Ensure that the tracing attributes and events are consistent with the project's observability standards.
| @@ -85,31 +88,47 @@ func NewQuoterManager(config relconfig.Config, metricsHandler metrics.Handler, i | |||
| } | |||
|
|
|||
| // ShouldProcess determines if a quote should be processed. | |||
| func (m *Manager) ShouldProcess(ctx context.Context, quote reldb.QuoteRequest) bool { | |||
| func (m *Manager) ShouldProcess(parentCtx context.Context, quote reldb.QuoteRequest) (res bool) { | |||
There was a problem hiding this comment.
There is a TODO comment indicating that the loop in the generateQuotes function is broken. Do you require assistance with this?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- packages/sdk-router/src/constants/addresses.ts (1 hunks)
- packages/sdk-router/src/rfq/fastBridge.test.ts (1 hunks)
- packages/sdk-router/src/sdk.test.ts (2 hunks)
- packages/sdk-router/src/typechain/FastBridge.ts (3 hunks)
Files not summarized due to errors (1)
- packages/sdk-router/src/typechain/FastBridge.ts: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- packages/sdk-router/src/constants/addresses.ts
Additional comments: 19
packages/sdk-router/src/rfq/fastBridge.test.ts (1)
- 113-113: The update to the
bridgeRequestedTopicconstant seems to be a change in the event signature. Ensure that this new topic hash corresponds to the updatedBridgeRequestedevent in theFastBridgecontract. If the event parameters have changed, this hash should reflect the new keccak256 hash of the updated event signature.packages/sdk-router/src/sdk.test.ts (3)
854-860: The transaction hash and SynapseTxId for
rfqOpToArbTxhave been updated. Ensure that these new values are correct and correspond to valid transactions in the test environment.863-869: Similarly, the transaction hash and SynapseTxId for
rfqArbToOpTxhave been updated. Verify that these new values are accurate and reflect the intended test cases.997-997: The constant
arbSynRFQhas been updated. Confirm that this new address is correct and points to the intended contract in the test environment.packages/sdk-router/src/typechain/FastBridge.ts (15)
16-28: The import statements and namespace declarations appear to be standard and correct. No issues found here.
13-44: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [32-62]
The
BridgeParamsStructand its corresponding output type have been defined with several fields. Ensure that these fields match the smart contract's event or function parameters that they are intended to represent.
- 51-81: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [66-106]
Similarly, the
BridgeTransactionStructand its output type are defined. Verification is needed to ensure that these fields are consistent with the smart contract's definitions.
111-150: The
functionssection of the interface lists all the callable functions. It is important to verify that all functions are correctly typed and match the smart contract's functions.154-323: The
getFunctionandencodeFunctionDatamethods are auto-generated and should correctly map to the contract's functions. Ensure that the function names and parameters are consistent with the actual contract functions.326-435: The
decodeFunctionResultmethods are used to decode the data returned by the contract's functions. Verify that the return types match the expected types from the contract's functions.438-455: The
eventssection lists all the events that the contract can emit. It is crucial to verify that these events and their parameters match the events defined in the smart contract.Verification successful
The output from the script provides the Solidity code for the
FastBridgecontract and theIFastBridgeinterface, which includes the event definitions. We can now compare these event definitions with the TypeScript event fragments listed in theFastBridge.tsfile to verify that they match.Based on the provided Solidity code, the events defined in the
FastBridgecontract are:
BridgeRequestedBridgeRelayedBridgeProofProvidedBridgeProofDisputedBridgeDepositClaimedBridgeDepositRefundedThe parameters for each event in the Solidity code are as follows:
BridgeRequested:transactionId,sender,request,destChainId,originToken,destToken,originAmount,destAmount,sendChainGasBridgeRelayed:transactionId,relayer,to,originChainId,originToken,destToken,originAmount,destAmount,chainGasAmountBridgeProofProvided:transactionId,relayer,transactionHashBridgeProofDisputed:transactionId,relayerBridgeDepositClaimed:transactionId,relayer,to,token,amountBridgeDepositRefunded:transactionId,to,token,amountComparing these to the TypeScript event fragments in
FastBridge.ts, we can conclude that the events and their parameters match the events defined in the Solidity smart contract.* 458-475: The `getEvent` method should correctly reference the events in the contract. Ensure that the event names and parameters are consistent with the actual contract events.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the events and their parameters match the events defined in the smart contract. ast-grep --lang solidity --pattern $'event $_($_)'Length of output: 168
Script:
#!/bin/bash # Find Solidity files that may contain the event definitions. fd --extension sol --exec rg 'event\s+\w+\('Length of output: 17109
Script:
#!/bin/bash # Find the specific Solidity file that defines the FastBridge contract and its events. fd 'FastBridge\.sol' --exec cat {}Length of output: 15559
479-706: The event interfaces and filters are defined for each event. Verify that these match the events emitted by the contract and that the types of the parameters are correct.
709-894: The
FastBridgecontract interface includes methods for interacting with the contract. Ensure that these methods are correctly implemented and match the contract's ABI.897-1050: The methods without the
functionskeyword are shorthand for calling the corresponding methods within thefunctionsobject. Verify that these shorthand methods are correctly mapped and that their signatures match the full method definitions.1053-1179: The
callStaticmethods allow for simulating transactions. Verify that these methods are correctly implemented and match the contract's ABI for static calls.1183-1356: The
filterssection defines filters for the contract's events. Verify that these filters are correctly implemented and match the events in the contract's ABI.Verification successful
The output from the script provides the event definitions from the
FastBridge.solandIFastBridge.solcontracts. We can see that the eventsBridgeRequested,BridgeRelayed,BridgeProofProvided,BridgeProofDisputed,BridgeDepositClaimed, andBridgeDepositRefundedare defined in theIFastBridge.solcontract.Comparing this to the provided TypeScript code snippet from
FastBridge.ts, we can verify that the event filters in the TypeScript code match the events defined in the Solidity contract. Each event in the TypeScript code has a corresponding event in the Solidity contract with matching parameters.Based on this information, we can conclude that the event filters in the TypeScript code are correctly implemented and match the events in the contract's ABI.
* 1359-1522: The `estimateGas` methods are used to estimate the gas cost of transactions. Verify that these methods are correctly implemented and match the contract's ABI.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the filters match the events in the contract's ABI. ast-grep --lang solidity --pattern $'event $_($_)'Length of output: 168
Script:
#!/bin/bash # Search for event definitions in Solidity contract files rg --glob '*.sol' 'event 'Length of output: 58397
Script:
#!/bin/bash # Narrow down the search to events in FastBridge.sol and IFastBridge.sol rg --glob '*FastBridge.sol' 'event ' || rg --glob '*IFastBridge.sol' 'event 'Length of output: 876
Verification successful
The shell script has successfully found occurrences of
estimateGasmethods in Solidity files. The methods found are:function estimateGasFee(uint256 _dstChainId, bytes calldata _options) external returns (uint256);This method appears to be part of the contract's ABI, as it is defined with the
externalvisibility modifier, which is commonly used for functions that are intended to be part of the contract's interface.Given this information, it seems that the
estimateGasmethods are indeed present in the Solidity contract files and are likely to match the contract's ABI, as they are defined with the appropriate visibility for external calls.* 1525-1695: The `populateTransaction` methods are used to create transaction objects. Verify that these methods are correctly implemented and match the contract's ABI.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the estimateGas methods match the contract's ABI. ast-grep --lang solidity --pattern $'function $_($_) $_'Length of output: 174
Script:
#!/bin/bash # Find Solidity files and then search for the estimateGas methods within those files. fd '\.sol$' --exec rg 'function estimateGas'Length of output: 250
Verification successful
Upon reviewing the ABI and the function fragments from the
FastBridge.tsfile, it appears that the function names and their parameters match correctly. The ABI lists functions such asDEFAULT_ADMIN_ROLE,DISPUTE_PERIOD,addGovernor,bridge,claim,dispute, and others, which are also present in theFastBridge.tsfile with the correct function fragments.Based on the provided outputs, there are no discrepancies between the ABI and the TypeScript file regarding the function names and their parameters. Therefore, it can be concluded that the
populateTransactionmethods are correctly implemented and match the contract's ABI.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the populateTransaction methods match the contract's ABI. ast-grep --lang solidity --pattern $'function $_($_) $_'Length of output: 174
Script:
#!/bin/bash # Find the ABI file for the FastBridge contract fd 'FastBridge.json' --exec cat {} \;Length of output: 25242
Script:
#!/bin/bash # Extract the method names from the FastBridge.ts to compare with the ABI rg 'function' packages/sdk-router/src/typechain/FastBridge.tsLength of output: 4099
trajan0x
force-pushed
the
fix/rfq-events
branch
from
642600d to
8885e77
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- services/rfq/relayer/pricer/fee_pricer.go (5 hunks)
- services/rfq/relayer/pricer/fee_pricer_test.go (8 hunks)
- services/rfq/relayer/quoter/quoter.go (3 hunks)
Additional comments: 11
services/rfq/relayer/pricer/fee_pricer.go (2)
23-27: The addition of the
useMultiplierparameter to theGetOriginFee,GetDestinationFee, andGetTotalFeemethods in theFeePricerinterface andfeePricerstruct is consistent with the PR objectives to refine fee calculation logic.157-170: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [118-167]
The logic within the
getFeemethod correctly uses theuseMultiplierparameter to determine whether to apply the fixed fee multiplier to the fee calculation. This change aligns with the PR objectives to offer a more nuanced fee calculation.services/rfq/relayer/quoter/quoter.go (3)
91-118: The modifications to the
ShouldProcessfunction, including the addition of theparentCtxparameter and the use of OpenTelemetry for tracing, enhance observability and are consistent with the PR objectives to improve error handling and tracing.125-149: The
isProfitableQuotefunction has been updated to accept aparentCtxparameter and includes tracing logic. This change improves error handling and observability, aligning with the PR objectives.91-91: The previous comment regarding the TODO in the
generateQuotesfunction is still valid and does not need to be repeated.services/rfq/relayer/pricer/fee_pricer_test.go (6)
27-27: The test case
TestGetOriginFeehas been updated to include theuseMultiplierparameter when callingGetOriginFee. This change is necessary to test the updated method signature and logic.62-62: The test case
TestGetDestinationFeenow correctly includes theuseMultiplierparameter when callingGetDestinationFee, aligning with the changes in the method signature.101-101: The test case
TestGetTotalFeehas been updated to pass theuseMultiplierparameter toGetTotalFee, ensuring the test reflects the updated method signature.162-162: The test case
TestGetTotalFeeWithMultipliercorrectly includes theuseMultiplierparameter in theGetTotalFeemethod call, which is essential for testing the new fee calculation logic.181-181: The test case
TestGetTotalFeeWithMultiplierhas been updated to test the scenario where theuseMultiplieris set totrue, but theFixedFeeMultiplieris negative. This is a good test of the system's robustness.200-200: The test case
TestGetTotalFeeWithMultiplieralso includes a scenario where theFixedFeeMultiplieris set to zero, which should default to 1. This test ensures that the fee calculation behaves as expected in edge cases.
Description
Pre-merge checklist:
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
New Features
BridgeRequestedandBridgeRelayedin smart contracts.BridgeProofProvided,BridgeProofDisputed, andBridgeDepositRefunded.Enhancements
Bug Fixes
FAST_BRIDGE_EXCEPTION_MAPto use uppercase notation.Documentation
listener_test.gofor better readability.Refactor
Style
Chores
depguardlinter to the project configuration for enhanced dependency checks.