-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(contracts-rfq): relayer exclusivity [SLT-187] #3202
Conversation
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
Changes to gas cost
🧾 Summary (50% most significant diffs)
Full diff report 👇
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3202 +/- ##
====================================================
+ Coverage 40.97804% 90.35088% +49.37284%
====================================================
Files 459 60 -399
Lines 25643 1254 -24389
Branches 343 150 -193
====================================================
- Hits 10508 1133 -9375
+ Misses 14383 116 -14267
+ Partials 752 5 -747
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (9)
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (3)
7-7
: LGTM! Consider adding parameters for more detailed error reporting.The
ExclusivityParamsIncorrect
error is a good addition for handling issues related to exclusivity parameters. It aligns well with the PR objective of introducing relayer exclusivity.To provide more context when this error is thrown, consider adding parameters to specify which exclusivity parameter(s) are incorrect. For example:
error ExclusivityParamsIncorrect(string paramName, uint256 providedValue, uint256 expectedValue);This would allow for more detailed error reporting and easier debugging.
18-18
: LGTM! Consider adding a timestamp parameter for consistency.The
ExclusivityPeriodNotPassed
error is a valuable addition for handling timing issues related to the exclusivity feature. Its placement near other time-related errors (DeadlineExceeded
,DisputePeriodNotPassed
, etc.) is appropriate.For consistency with other time-related errors and to provide more context, consider adding a timestamp parameter:
error ExclusivityPeriodNotPassed(uint256 currentTime, uint256 exclusivityEndTime);This would align with the pattern used in errors like
DeadlineExceeded
and provide useful information for debugging.
Line range hint
7-18
: Summary: Good additions for relayer exclusivity error handling.The two new errors,
ExclusivityParamsIncorrect
andExclusivityPeriodNotPassed
, are well-aligned with the PR objective of introducing relayer exclusivity. They enhance the contract's ability to handle specific error cases related to this new feature.The additions maintain the existing style and structure of the interface, making them a seamless integration. These focused changes improve error reporting capabilities without introducing risks to existing functionality.
As the relayer exclusivity feature is implemented, ensure that these errors are used consistently throughout the relevant contracts. Consider creating unit tests that specifically trigger these error conditions to verify their correct usage.
packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol (3)
9-11
: Approved: Good explanation for making the contract abstract.The change to make
FastBridgeV2ParityTest
an abstract contract is well-justified by the comment explaining thatFastBridgeV2
is no longer fully backwards compatible withFastBridge
. This prevents these tests from running, which is appropriate given the compatibility issues.Consider adding more detailed documentation about the specific incompatibilities between
FastBridgeV2
andFastBridge
. This could help developers understand the exact nature of the breaking changes and guide them in updating any dependent code.
Line range hint
22-41
: Changes to test functions reflect updated contract behavior.The modifications to the test functions provide valuable insights into the changes made in the
FastBridgeV2
contract:
test_failedRelayNotRelayer
is skipped, indicating that the relay function is no longer permissioned.test_failedClaimNotRelayer
is skipped, suggesting that the claim function is no longer permissioned by role.test_failedClaimNotOldRelayer
is modified to remove role assignment, but still tests for sender correctness.These changes are consistent with the comment about backwards incompatibility and provide a clear picture of how the permission model has evolved in
FastBridgeV2
.Consider adding new test cases that specifically target the new behavior of the
FastBridgeV2
contract, especially around the updated permission model. This will ensure that the new functionality is properly tested and documented.
Line range hint
1-41
: Summary: Appropriate updates to reflect FastBridgeV2 changesThe modifications to
FastBridgeV2ParityTest
effectively address the backwards incompatibility issues betweenFastBridgeV2
andFastBridge
:
- Making the contract abstract prevents unintended test execution.
- Skipping and modifying specific tests reflect changes in the permission model.
- Comments provide clear explanations for the changes.
These updates ensure that the test suite accurately represents the current state of the
FastBridgeV2
contract.To further improve the codebase:
- Consider creating a new test suite specifically for
FastBridgeV2
that covers its unique features and behavior.- Update the main
FastBridgeV2
contract documentation to clearly outline the breaking changes and new features compared toFastBridge
.- If not already done, ensure that migration guides are available for users of the original
FastBridge
contract.packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
29-29
: Address the TODO to optimize the encoding schemeThere's a TODO comment suggesting a change to the encoding scheme to prevent extra gas consumption during decoding. Optimizing the encoding could lead to significant gas savings and improve contract efficiency.
Would you like me to propose an optimized encoding scheme or open a GitHub issue to track this task?
packages/contracts-rfq/test/FastBridgeV2.t.sol (2)
36-37
: Restrict visibility of state variables for better encapsulationThe state variables
tokenTx
andethTx
are declared aspublic
. If external access to these variables is not required, consider changing their visibility tointernal
orprivate
to enhance encapsulation and reduce potential security risks.
41-42
: Align visibility of new parameters with best practicesSimilarly,
tokenParamsV2
andethParamsV2
are declared aspublic
. If these parameters are only used within the contract or by inheriting contracts, consider setting their visibility tointernal
orprivate
for consistency and improved security.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (5 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (4 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.ProtocolFees.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (17 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (6 hunks)
🔇 Additional comments (69)
packages/contracts-rfq/test/FastBridgeV2.Src.ProtocolFees.t.sol (3)
Line range hint
1-24
: Summary: Transaction structure refactoring implemented, awaiting further contextThe changes in this file introduce a new
txV1
structure for organizing transaction data in the test contract. While the modifications are consistent and don't alter the test logic, they suggest a broader refactoring effort in the main contracts.Key points:
- The changes are well-implemented and consistent across both token and ETH transactions.
- The overall behavior of the test contract remains unchanged.
- More context is needed to understand the full scope of these changes, particularly their relation to the "relayer exclusivity" feature mentioned in the PR title.
Next steps:
- Provide additional context on the
txV1
structure and its purpose.- Clarify how these changes contribute to the relayer exclusivity feature.
- Ensure consistency across the entire codebase and update documentation as necessary.
Once these points are addressed, the changes can be approved from a technical standpoint.
16-22
: Refactoring of transaction structure noted. Verify consistency across the codebase.The changes introduce a new
txV1
nested structure for bothtokenTx
andethTx
objects. This refactoring suggests an update to the transaction data structure, possibly to accommodate new features or improve organization.While the test logic remains unchanged, this modification reflects changes in the main contract being tested. To ensure system-wide consistency:
- Verify that all other test files and main contract files have been updated to use this new structure.
- Check if documentation needs to be updated to reflect this change in transaction structure.
- Consider adding a comment explaining the purpose of the
txV1
structure for better code readability.To ensure consistency across the codebase, run the following script:
16-22
: Request for additional context on txV1 structure and relayer exclusivityThe introduction of the
txV1
structure appears to be part of a larger refactoring effort. While these changes improve the organization of transaction data, a few points need clarification:
- Could you provide more context on the purpose of the
txV1
structure? Is this related to a new version of the transaction format?- Are there any implications for backwards compatibility with existing systems or contracts?
- The PR title mentions "relayer exclusivity", but these changes don't seem directly related to that feature. Could you explain how these modifications contribute to the relayer exclusivity feature?
Adding comments in the code or updating the PR description with this information would greatly help in understanding the full scope and impact of these changes.
To gather more context, let's check for any related changes or documentation:
packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol (5)
8-8
: Definition ofEXCLUSIVITY_PERIOD
Enhances ReadabilityThe use of
60 seconds
forEXCLUSIVITY_PERIOD
improves code readability by explicitly specifying the time unit.
10-22
:createFixturesV2
Correctly Initializes Exclusivity ParametersThe
createFixturesV2
function properly overrides the base function and initializes the exclusivity parameters for both token and ETH transactions. AssigningquoteRelayer
,quoteExclusivitySeconds
, andquoteId
ensures that the tests accurately reflect the intended behavior.
24-27
: Overloadedbridge
Function Appropriately Selects ParametersThe overloaded
bridge
function effectively determines the correct set of parameters (ethParamsV2
ortokenParamsV2
) based on theoriginToken
. This ensures that the bridging logic uses the appropriate configuration for each token type.
29-39
: Maintains Caller Context Usingvm.prank
Utilizing
vm.prank(caller)
preserves the caller's context during thefastBridge.bridge
invocation. This is essential for accurately simulating transaction conditions and testing access control within the contract.
41-63
: Comprehensive Revert Tests for Exclusivity ParametersThe test functions:
test_bridge_revert_quoteRelayerSet_exclusivityPeriodNotSet
test_bridge_revert_quoteRelayerNotSet_exclusivityPeriodSet
test_bridge_eth_revert_quoteRelayerSet_exclusivityPeriodNotSet
test_bridge_eth_revert_quoteRelayerNotSet_exclusivityPeriodSet
correctly verify that the contract reverts when exclusivity parameters are incorrectly configured. This ensures the contract enforces the requirement that both
quoteRelayer
andquoteExclusivitySeconds
must be set appropriately.packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2)
42-42
: Verify backward compatibility of the updatedbridge
functionThe
bridge
function signature now includes an additional parameterBridgeParamsV2 memory paramsV2
. Please ensure that this change maintains backward compatibility with existing contracts and systems that implement this interface.To confirm, you might review dependent contracts for compatibility.
64-66
: LGTM: New functiongetBridgeTransactionV2
addedThe addition of the
getBridgeTransactionV2
function enhances the interface by allowing decoding of bridge requests intoBridgeTransactionV2
structs effectively.packages/contracts-rfq/test/FastBridgeV2.t.sol (4)
5-5
: Addition ofIFastBridgeV2
import is appropriateThe new import statement correctly includes the
IFastBridgeV2
interface, which is necessary for integrating the updated bridge functionality into the test contract.
48-48
: EnsurecreateFixturesV2()
initializes all necessary fieldsAfter invoking
createFixturesV2()
, verify that all the required fields intokenTx
andethTx
are properly initialized. This is crucial to prevent uninitialized data from affecting the outcome of the tests.
Line range hint
84-99
: Confirm correct assignment totxV1
intokenTx
andethTx
Assigning
IFastBridge.BridgeTransaction
totokenTx.txV1
andethTx.txV1
should be compatible with theBridgeTransactionV2
struct. Ensure that thetxV1
field exists withinBridgeTransactionV2
and that this assignment aligns with the updated contract interfaces to prevent type mismatches.
128-130
: Verification of transaction ID hashing methodThe
getTxId
function correctly computes the transaction ID usingkeccak256(abi.encode(bridgeTx))
. Ensure that this hashing method is consistent with the one used in production contracts to maintain integrity between the tests and the actual implementation.packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (21)
1-7
: LGTM!License header, pragma statement, imports, and contract declaration are correctly implemented.
8-26
: Correct initialization in 'createFixturesV2'.The
createFixturesV2
function properly initializes the bridge parameters and transaction data with the appropriate relayers and exclusivity periods.
32-35
: Test 'test_relay_token_exclusivityLastSecond' is correctly implemented.Function correctly advances the time to the end of the exclusivity period and calls
test_relay_token
to verify expected behavior.
37-40
: Test 'test_relay_token_exclusivityOver' is correctly implemented.Function correctly advances time beyond the exclusivity period and calls
test_relay_token
to ensure proper functionality.
42-45
: Revert test 'test_relay_token_notQuotedRelayer_revert' is correctly implemented.Function correctly expects a revert when a non-quoted relayer attempts to relay during the exclusivity period.
47-51
: Revert test 'test_relay_token_notQuotedRelayer_exclusivityLastSecond_revert' is correctly implemented.Function skips to the end of the exclusivity period and correctly expects a revert when a non-quoted relayer attempts to relay.
53-62
: Test 'test_relay_token_notQuotedRelayer_exclusivityOver' is correctly implemented.Function correctly simulates a relay after the exclusivity period has passed, ensuring that a non-quoted relayer can relay successfully and asserts the correct balances.
64-67
: Test 'test_relay_token_withRelayerAddress_exclusivityLastSecond' is correctly implemented.Function advances time to the end of the exclusivity period and tests relay with a specified relayer address.
69-72
: Test 'test_relay_token_withRelayerAddress_exclusivityOver' is correctly implemented.Function advances time beyond the exclusivity period and tests relay with a specified relayer address, ensuring correct behavior.
74-77
: Revert test 'test_relay_token_withRelayerAddress_notQuotedRelayer_revert' is correctly implemented.Function correctly expects a revert when a non-quoted relayer attempts to relay with a specified relayer address during the exclusivity period.
79-83
: Revert test 'test_relay_token_withRelayerAddress_notQuotedRelayer_exclusivityLastSecond_revert' is correctly implemented.Function skips to the end of the exclusivity period and correctly expects a revert when a non-quoted relayer attempts to relay with a specified relayer address.
96-99
: Comment section for ETH relay tests is appropriate.Comments clearly indicate the upcoming tests for ETH relays and the exclusivity rights of Relayer B.
100-103
: Test 'test_relay_eth_exclusivityLastSecond' is correctly implemented.Function advances time to the end of the exclusivity period and calls
test_relay_eth
to verify expected behavior.
105-108
: Test 'test_relay_eth_exclusivityOver' is correctly implemented.Function advances time beyond the exclusivity period and calls
test_relay_eth
to ensure correct functionality.
110-113
: Revert test 'test_relay_eth_notQuotedRelayer_revert' is correctly implemented.Function correctly expects a revert when a non-quoted relayer attempts to relay ETH during the exclusivity period.
115-119
: Revert test 'test_relay_eth_notQuotedRelayer_exclusivityLastSecond_revert' is correctly implemented.Function skips to the end of the exclusivity period and correctly expects a revert when a non-quoted relayer attempts to relay ETH.
121-130
: Test 'test_relay_eth_notQuotedRelayer_exclusivityOver' is correctly implemented.Function correctly simulates a relay of ETH after the exclusivity period has passed, ensuring that a non-quoted relayer can relay successfully and asserts the correct balances.
132-135
: Test 'test_relay_eth_withRelayerAddress_exclusivityLastSecond' is correctly implemented.Function advances time to the end of the exclusivity period and tests ETH relay with a specified relayer address.
137-140
: Test 'test_relay_eth_withRelayerAddress_exclusivityOver' is correctly implemented.Function advances time beyond the exclusivity period and tests ETH relay with a specified relayer address, ensuring correct behavior.
142-145
: Revert test 'test_relay_eth_withRelayerAddress_notQuotedRelayer_revert' is correctly implemented.Function correctly expects a revert when a non-quoted relayer attempts to relay ETH with a specified relayer address during the exclusivity period.
147-151
: Revert test 'test_relay_eth_withRelayerAddress_notQuotedRelayer_exclusivityLastSecond_revert' is correctly implemented.Function skips to the end of the exclusivity period and correctly expects a revert when a non-quoted relayer attempts to relay ETH with a specified relayer address.
packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (12)
4-4
: Update import statement to useIFastBridgeV2
The import statement now includes
IFastBridgeV2
, reflecting the transition to the updated interface. This ensures compatibility with the new contract changes.
32-33
: Mint tokens for bothrelayerA
andrelayerB
inmintTokens
By minting tokens for both relayers, you enhance the test coverage to include scenarios involving multiple relayers. This is a good practice for comprehensive testing.
38-39
: Approve tokens forrelayerB
Adding an approval for
relayerB
ensures thatfastBridge
can spend tokens on behalf of both relayers, aligning with the tokens minted earlier.
42-48
: UpdateexpectBridgeRelayed
function to acceptBridgeTransactionV2
The function signature now accepts
IFastBridgeV2.BridgeTransactionV2
, matching the updated interface. This change is consistent and necessary for the new transaction structure.
63-63
: Updaterelay
function to useBridgeTransactionV2
The
relay
function now acceptsIFastBridgeV2.BridgeTransactionV2
, aligning it with the new interface. This update ensures that the function handles the new transaction data structure.
73-73
: UpdaterelayWithAddress
function to useBridgeTransactionV2
Similarly,
relayWithAddress
now acceptsBridgeTransactionV2
. This change is consistent with the interface updates and maintains compatibility.
82-88
: IntroducecheckRelayedViews
for enhanced test assertionsThe new
checkRelayedViews
function encapsulates checks for relayed transactions, improving modularity and readability of your tests.
93-97
: Enhancetest_relay_token
with additional checksIncluding
checkRelayedViews
and balance assertions strengthens the test by verifying that the transaction was relayed correctly and that balances are updated as expected.
134-149
: Test block data recording intest_relay_eth_withRelayerAddress_checkBlockData
The test accurately sets custom block numbers and timestamps, then verifies that
fastBridge
records them correctly. This adds valuable coverage for block data tracking in your tests.
123-130
:⚠️ Potential issueVerify
caller
andrelayer
handling in ETH relay testsIn
test_relay_eth_withRelayerAddress
,relayerA
callsrelayWithAddress
withrelayerB
as the relayer. Ensure that this behavior is intended and that the contract properly handles scenarios where the caller and relayer are different.Investigate if there are checks in place to prevent misuse when the caller specifies a different relayer. Use the script provided in the previous comment to assist in this verification.
104-108
:⚠️ Potential issuePotential issue with mismatched
caller
andrelayer
inrelayWithAddress
In
test_relay_token_withRelayerAddress
,relayerB
callsrelayWithAddress
usingrelayerA
as the relayer. This allows one relayer to relay a transaction and attribute it to another relayer. Verify if the protocol permits this behavior and assess potential security implications, such as misattribution or unauthorized relaying.Run the following script to examine how
relayWithAddress
handles therelayer
parameter and check for verification mechanisms:#!/bin/bash # Description: Check for validation in relayWithAddress regarding caller and relayer # Search for the relayWithAddress function and look for any authorization checks ast-grep --lang solidity --pattern $'function relay(bytes memory request, address relayer) public {\n $$$\n}' \ packages/contracts-rfq/contracts/* | rg --after-context=10 'require|msg.sender|validation'
53-58
: Access transaction details usingbridgeTx.txV1
You're accessing properties through
bridgeTx.txV1
. Verify thattxV1
is correctly defined withinBridgeTransactionV2
and that all the accessed fields are properly mapped.Run the following script to confirm that
BridgeTransactionV2
containstxV1
with the expected fields:✅ Verification successful
BridgeTransactionV2 correctly includes
txV1
as a member.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure of BridgeTransactionV2 and ensure it contains txV1 # Search for the definition of BridgeTransactionV2 and check for txV1 ast-grep --lang solidity --pattern $'struct BridgeTransactionV2 {\n $$$\n BridgeTransaction txV1;\n $$$\n}' \ packages/contracts-rfq/contracts/* # Expectation: The output should show that txV1 is a member of BridgeTransactionV2Length of output: 651
Script:
#!/bin/bash # Description: Verify that BridgeTransactionV2 contains txV1 using ripgrep # Search for the BridgeTransactionV2 struct definition rg 'struct\s+BridgeTransactionV2\s*\{([^}]*)\}' packages/contracts-rfq/contracts/*.sol # Within the struct definition, search for txV1 rg 'struct\s+BridgeTransactionV2\s*\{([^}]*)\}' packages/contracts-rfq/contracts/*.sol | rg 'BridgeTransaction\s+txV1;'Length of output: 414
Script:
#!/bin/bash # Description: Locate the definition of BridgeTransactionV2 in Solidity files # Search for the definition of the struct BridgeTransactionV2 rg 'struct\s+BridgeTransactionV2\s*\{' packages/contracts-rfq/contracts/**/*.sol # If BridgeTransactionV2 is found, display its contents rg -A 10 'struct\s+BridgeTransactionV2\s*\{' packages/contracts-rfq/contracts/**/*.solLength of output: 2270
packages/contracts-rfq/contracts/FastBridgeV2.sol (7)
78-81
: Addition ofgetBridgeTransactionV2
function is appropriateThe new
getBridgeTransactionV2
function correctly decodes aBridgeTransactionV2
from the providedbytes
request. This addition aligns with the introduction of the new transaction structure and maintains code clarity.
85-88
: Ensures backward compatibility by overloadingbridge
functionThe
bridge
function is appropriately overloaded to accept the newBridgeParamsV2
. By initializing default values forBridgeParamsV2
when they are not provided, it preserves backward compatibility with existing implementations.
100-103
: Validation of exclusivity parameters is correctly implementedThe condition ensures that
quoteRelayer
andquoteExclusivitySeconds
are either both set or both unset, preventing inconsistent exclusivity configurations. This validation is crucial for the correct operation of exclusivity logic.
114-116
: Calculation ofexclusivityEndTime
is appropriateThe calculation correctly handles cases where exclusivity parameters are not set by defaulting
exclusivityEndTime
to zero. This ensures that the exclusivity period is only applied when specified.
119-135
: Proper encoding ofBridgeTransactionV2
with nestedtxV1
The encapsulation of
BridgeTransaction
astxV1
withinBridgeTransactionV2
effectively extends the existing transaction structure to include exclusivity parameters. This approach maintains compatibility while adding new functionality.
169-176
: Exclusivity checks inrelay
function are correctly implementedThe added logic in the
relay
function enforces the exclusivity period effectively. It ensures that during the exclusivity window, only the specifiedexclusivityRelayer
can relay the transaction, thereby honoring exclusivity agreements.
152-152
: EnsureBridgeRequestedV2
event is defined and documentedThe
emit BridgeRequestedV2
statement emits a new event withtransactionId
andparamsV2.quoteId
. Verify that theBridgeRequestedV2
event is properly defined and documented in the contract to avoid any issues during event handling.Run the following script to confirm that the
BridgeRequestedV2
event is defined:packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (15)
4-4
: Import statement updated correctlyThe addition of
IFastBridgeV2
ensures access to the new interface required for updated function signatures.
30-30
: Addition ofBridgeRequestedV2
event is appropriateThe new event
BridgeRequestedV2
correctly capturestransactionId
andquoteId
. SincequoteId
is of typebytes
, it cannot be indexed, which is acceptable for variable-length data.
75-77
: Explicit visibility and mutability specifiers addedThe
bridge
function now explicitly declares its visibility aspublic
and is markedvirtual
to allow overriding in derived contracts, enhancing extensibility.
85-87
: Updatedprove
function to useBridgeTransactionV2
The function signature has been updated to accept
IFastBridgeV2.BridgeTransactionV2
, aligning with the new interface and ensuring compatibility with the updated bridge transaction structure.
90-92
: Updatedclaim
function to useBridgeTransactionV2
The function now accepts
IFastBridgeV2.BridgeTransactionV2
, maintaining consistency with the updated interface.
95-97
: Updated overloadedclaim
function to useBridgeTransactionV2
The overloaded
claim
function is appropriately updated to acceptIFastBridgeV2.BridgeTransactionV2
, ensuring consistency across function implementations.
105-107
: Updatedrefund
function to useBridgeTransactionV2
The
refund
function signature now acceptsIFastBridgeV2.BridgeTransactionV2
, aligning with the updated transaction structure for refunds.
110-121
: UpdatedexpectBridgeRequested
to use nestedtxV1
fields inBridgeTransactionV2
The function correctly adapts to the new
BridgeTransactionV2
structure by accessing the required fields from the nestedtxV1
member to emit theBridgeRequested
event.
148-149
: UpdatedexpectBridgeDepositClaimed
to usebridgeTx.txV1
fieldsThe function correctly retrieves
originToken
andoriginAmount
frombridgeTx.txV1
, aligning with the updatedBridgeTransactionV2
structure.
499-502
: Updated balance assertions to usetokenTx.txV1
fieldsThe assertions now reference
tokenTx.txV1.originFeeAmount
andtokenTx.txV1.originAmount
, reflecting the updated transaction structure.
565-568
: Updated ETH balance assertions to useethTx.txV1
fieldsThe assertions correctly reference
ethTx.txV1.originFeeAmount
andethTx.txV1.originAmount
, ensuring consistency with the new transaction structure.
864-864
: Verify usage ofethParams
instead ofethParamsV2
The assertion uses
ethParams.originAmount
. Given the update toBridgeTransactionV2
, please verify thatethParams
contains values consistent with the new transaction structure. If necessary, update toethParamsV2
to ensure accuracy.
878-879
: Potential inconsistency in balance calculationsThe balance assertions use
ethParams.originAmount
. Verify that these values are accurate in light of the updated transaction parameters, and ensure consistent usage ofethParamsV2
if appropriate.
892-892
: Verify usage ofethParams
instead ofethParamsV2
The assertion references
ethParams.originAmount
. Confirm thatethParams
is aligned with the updated transaction structure, and update toethParamsV2
if necessary.
906-906
: Verify usage ofethParams
instead ofethParamsV2
In this assertion,
ethParams.originAmount
is used. Please ensure thatethParams
reflects the values from the updatedBridgeTransactionV2
structure. Consider usingethParamsV2
if needed.
Bundle ReportChanges will increase total bundle size by 407.1kB (1.14%) ⬆️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit |
Gas diffs will be much closer to the true values once #3204 is merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
116-125
: LGTM: New function added for backward compatibility.The
getBridgeTransaction
function provides a way to decode V1 transactions, ensuring backward compatibility. This is a good practice for maintaining existing integrations.Consider addressing the TODO comment in a future iteration.
The TODO comment about reducing cyclomatic complexity should be addressed in a future update to improve code maintainability.
126-182
: LGTM: Enhanced bridge functionality with new features and validations.The updated
bridge
function now supports exclusivity parameters and uses the newBridgeTransactionV2
structure. The additional validations improve the robustness of the function.Consider refactoring to reduce complexity.
As noted in the TODO comment, the function's cyclomatic complexity has increased. Consider breaking down this function into smaller, more manageable pieces in a future update to improve readability and maintainability.
Line range hint
1-348
: Overall assessment: Well-implemented changes with room for future improvements.The changes to
FastBridgeV2.sol
successfully introduce new functionality for exclusivity in bridging while maintaining backward compatibility. The code is consistent in its approach to handling the newBridgeTransactionV2
structure across different functions.Recommendations for future iterations:
- Address the TODO comments about reducing cyclomatic complexity, particularly in the
bridge
andrelay
functions.- Consider breaking down larger functions into smaller, more manageable pieces to improve readability and maintainability.
- Ensure comprehensive testing of the new exclusivity features and their interaction with existing functionality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (5 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.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 (4 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)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol
- packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol
- packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol
- packages/contracts-rfq/test/FastBridgeV2.t.sol
🔇 Additional comments (11)
packages/contracts-rfq/contracts/FastBridgeV2.sol (6)
44-47
: LGTM: Backward-compatible implementation of new bridge functionality.The overloaded
bridge
function maintains compatibility with existing calls while introducing new functionality throughBridgeParamsV2
. The default values ensure that the contract's behavior remains consistent for existing integrations.
185-205
: LGTM: Relay function updated with new exclusivity checks.The
relay
function has been successfully updated to work withBridgeTransactionV2
and implement the new exclusivity feature. The use ofbridgeRelays
instead of directly accessingbridgeRelayDetails
is a good practice for code organization.
191-191
: LGTM: Improved encapsulation in relay check.The use of
bridgeRelays(transactionId)
instead of directly accessingbridgeRelayDetails
improves code organization and encapsulation. This change makes the code more maintainable and flexible for future updates.
193-193
: LGTM: Updated to use new transaction structure.The use of
getBridgeTransactionV2
ensures that the relay function correctly decodes and works with the newBridgeTransactionV2
structure. This change is consistent with the overall updates to the contract.
257-257
: LGTM: Claim function updated to use new transaction structure.The
claim
function has been correctly updated to usegetBridgeTransactionV2
, ensuring consistency with the new transaction structure used throughout the contract.
301-303
: LGTM: New function added for decoding V2 transactions.The
getBridgeTransactionV2
function is a necessary addition for working with the newBridgeTransactionV2
structure. It's consistent with the existinggetBridgeTransaction
function and supports the contract's evolution.packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol (5)
12-16
: Ensure negative values are handled correctly inquoteExclusivitySeconds
.Assigning negative values to
quoteExclusivitySeconds
(lines 12 and 15) may lead to unintended behavior if the contract does not properly handle negative exclusivity periods. While this may be intentional for testing negative scenarios, please confirm that the contract logic is designed to handle negative values without causing integer underflows or other issues.To verify that negative
quoteExclusivitySeconds
values are appropriately handled, please double-check the contract's input validation and error handling for negative exclusivity periods.
19-21
: Confirm intentional setting of pastexclusivityEndTime
.Setting
exclusivityEndTime
to a past timestamp usingblock.timestamp - EXCLUSIVITY_PERIOD_ABS
(lines 19 and 21) results in an exclusivity period that has already expired. Ensure this is intentional for testing purposes to verify the contract's behavior when the exclusivity period is over.
30-32
: Check for integer underflow with negative exclusivity seconds.In
test_bridge_revert_exclusivityEndTimeZero
(line 30), assigningtokenParamsV2.quoteExclusivitySeconds = -int256(block.timestamp)
may cause integer underflow, resulting in a large positive value. Confirm that this value triggers the expectedExclusivityParamsIncorrect
revert and that the test accurately represents the intended negative scenario.Ensure that the contract handles negative exclusivity periods correctly and that the test case effectively checks for the expected revert behavior.
48-50
: Address integer underflow risks in ETH exclusivity period underflow test.Assigning
ethParamsV2.quoteExclusivitySeconds = -int256(block.timestamp + 1)
(line 48) can lead to integer underflow. Ensure that the contract's logic includes safeguards against such cases and that the test accurately reflects the scenario.Review the contract's input validation for exclusivity periods to prevent negative values and ensure consistent behavior across different transaction types.
24-26
: Verify correct override of thebridge
function.The overridden
bridge
function (line 24) should match the base class's function signature exactly. Ensure that the parameters and their order are consistent with the base contract to prevent any unexpected behavior.Run the following script to compare the
bridge
function signatures:This will help confirm that the function signatures are correctly matched.
tokenParamsV2.quoteExclusivitySeconds = -int256(block.timestamp + 1); | ||
vm.expectRevert(ExclusivityParamsIncorrect.selector); | ||
bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid potential integer underflow in exclusivity period calculation.
Setting tokenParamsV2.quoteExclusivitySeconds = -int256(block.timestamp + 1)
(line 36) could result in integer underflow, leading to unexpected behavior. It's important to verify that the contract logic properly handles such edge cases.
Consider adding input validation in the contract to prevent negative exclusivity periods or underflows. This will enhance the contract's robustness against invalid inputs.
ethParamsV2.quoteExclusivitySeconds = -int256(block.timestamp); | ||
vm.expectRevert(ExclusivityParamsIncorrect.selector); | ||
bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper handling of negative exclusivity periods in ETH tests.
In test_bridge_eth_revert_exclusivityEndTimeZero
(line 42), setting ethParamsV2.quoteExclusivitySeconds = -int256(block.timestamp)
may cause integer underflow similar to the token test. Verify that this test correctly triggers the expected revert and that the contract handles negative values appropriately.
It's essential to confirm that negative exclusivity periods are adequately managed in ETH transactions to prevent unforeseen issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Could not think of any addtl tests not already covered
Description
User should be able to provide new fields for the
bridge()
function (field names are tentative):Old
bridge()
function should be preserved, by using the default empty/zero values for these fields.Objective:
quoteRelayer
to have exclusive fill rights until exclusivity deadline.Summary by CodeRabbit
New Features
FastBridgeV2
contract with enhanced transaction handling capabilities.BridgeQuoteDetails
to log bridge quote details with unique identifiers.Bug Fixes
Tests