refactor(contracts-rfq): Zaps [SLT-351] #3300
Conversation
WalkthroughThe changes in this pull request involve significant updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (3)
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
|
ChiTimesChi
changed the title
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 #3300 +/- ##
===================================================
- Coverage 34.85970% 34.85584% -0.00386%
===================================================
Files 529 529
Lines 33784 33782 -2
Branches 82 82
===================================================
- Hits 11777 11775 -2
Misses 20987 20987
Partials 1020 1020
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (19)
packages/contracts-rfq/test/mocks/NonPayableRecipient.sol (1)
11-12: Function updated to align with new terminology.The
fastBridgeTransferReceivedfunction has been correctly replaced withzap, aligning with the PR objectives. The function signature and return type remain the same, ensuring compatibility. The return statement has been appropriately updated to use the new function selector.However, consider adding a comment explaining why this function is intentionally not payable, as it might not be immediately clear to other developers that this is a mock for testing incorrect implementations.
/// @notice Incorrectly implemented - method is not payable. +/// @dev This function is intentionally not payable to test handling of incorrect implementations. function zap(address, uint256, bytes memory) external pure returns (bytes4) { return NonPayableRecipient.zap.selector; }packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (1)
Line range hint
1-28: Overall changes look good, consider adding commentsThe modifications to the
IFastBridgeV2Errorsinterface are consistent with the PR objectives and maintain the interface's structure and purpose. The new error declarations provide clear indications of their use cases related to Zap functionality.Consider adding brief comments above each error declaration to explain when these errors are thrown. This would enhance the interface's self-documentation and make it easier for developers to understand and use these errors correctly.
Example:
/// @dev Thrown when the length of zapData exceeds the maximum allowed error ZapDataLengthAboveMax(); /// @dev Thrown when a zap with native token is attempted but not supported error ZapNativeNotSupported();packages/contracts-rfq/test/mocks/RecipientMock.sol (1)
15-18: LGTM: Zap function implemented correctly with a minor suggestion.The
zapfunction has been correctly implemented to replace thefastBridgeTransferReceivedfunction. It properly returns the selector for interface compliance and includes thepayablemodifier.For improved clarity, consider adding parameter names even if they're unused:
- function zap(address, uint256, bytes memory) external payable returns (bytes4) { + function zap(address token, uint256 amount, bytes memory data) external payable returns (bytes4) { return IZapRecipient.zap.selector; }This change would make the function signature more self-documenting without affecting its behavior.
packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol (1)
16-18: LGTM:zapfunction implemented correctly for testing purposes.The new
zapfunction correctly replaces the oldfastBridgeTransferReceivedfunction and aligns with the new zap functionality. The implementation is suitable for its purpose as a mock contract for testing.Consider updating the comment above the function to reflect the new zap functionality:
- /// @notice Incorrectly implemented - method returns excessive bytes. + /// @notice Incorrectly implemented zap function - method returns excessive bytes for testing purposes.packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol (2)
8-8: LGTM: Contract declaration updated correctly.The contract declaration has been properly updated to implement the new
IZapRecipientinterface, which is consistent with the PR objectives.Consider updating the contract comment to reflect the new Zap terminology:
- /// @notice Incorrectly implemented recipient mock for testing purposes. DO NOT USE IN PRODUCTION. + /// @notice Incorrectly implemented Zap recipient mock for testing purposes. DO NOT USE IN PRODUCTION.
16-18: LGTM: Zap function implemented correctly.The
zapfunction has been properly implemented to replace the previousfastBridgeTransferReceivedfunction. The changes are consistent with the newIZapRecipientinterface and maintain the intentional incorrect behavior for testing purposes.Consider updating the function comment to reflect the new Zap terminology:
- /// @notice Incorrectly implemented - method returns incorrect value. + /// @notice Incorrectly implemented Zap function - method returns incorrect value.packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.Zap.t.sol (1)
11-20: LGTM:createFixturesV2implementation is correct and comprehensive.The
createFixturesV2function correctly overrides the parent implementation and extends it with Zap-specific data. The creation and setting ofmockZapDatafor various transaction types ensure thorough testing of the Zap functionality.Consider extracting the
keccak256("Random ID")into a constant or a function parameter for better maintainability and flexibility in testing different scenarios.packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Zap.t.sol (1)
27-29: LGTM: Variable and method names updated to use "Zap" terminology.The changes in the
createFixturesV2function are consistent with the shift to "Zap" terminology:
- Variable renamed from
mockCallParamstomockZapData.- Method calls updated to
setTokenTestZapDataandsetEthTestZapData.These changes maintain consistency while preserving the existing data structure.
Consider adding a brief comment explaining the structure of
mockZapDatato improve code readability. For example:// mockZapData structure: (address user, bytes32 randomId) bytes memory mockZapData = abi.encode(userA, keccak256("Random ID"));packages/contracts-rfq/test/libs/BridgeTransactionV2.t.sol (2)
32-37: LGTM! Consider adding comments for clarity.The changes correctly update the field names from
callValuetozapNativeandcallParamstozapData, aligning with the new terminology introduced in the PR objectives.Consider adding a brief comment explaining the purpose of
zapNativeandzapDatafor improved readability:+ // Assert the amount of additional native gas token requested by the recipient assertEq(a.zapNative, b.zapNative); assertEq(a.deadline, b.deadline); assertEq(a.nonce, b.nonce); assertEq(a.exclusivityRelayer, b.exclusivityRelayer); assertEq(a.exclusivityEndTime, b.exclusivityEndTime); + // Assert the data to be sent to the recipient when a Zap is activated assertEq(a.zapData, b.zapData);
57-57: LGTM! Consider adding a comment for consistency.The change from
callParamstozapDatacorrectly aligns with the new terminology introduced in the PR objectives. This update accurately reflects the changes in theBridgeTransactionV2structure.For consistency with the earlier suggestion, consider adding a brief comment explaining the purpose of
zapData:assertEq(harness.exclusivityRelayer(encodedTx), bridgeTx.exclusivityRelayer); assertEq(harness.exclusivityEndTime(encodedTx), bridgeTx.exclusivityEndTime); + // Assert the data to be sent to the recipient when a Zap is activated assertEq(harness.zapData(encodedTx), bridgeTx.zapData);packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (1)
59-59: LGTM: Correct implementation of V2 to V1 mappingThe updated assignment correctly maps the
zapNativefield from V2 to thesendChainGasfield in V1, aligning with the explanation in the comment. This change ensures proper compatibility between the two versions.For improved readability, consider using a named constant for the comparison:
- expectedTx.sendChainGas = bridgeTxV2.zapNative > 0; + const uint256 NO_ZAP = 0; + expectedTx.sendChainGas = bridgeTxV2.zapNative > NO_ZAP;This makes the intention clearer and easier to maintain if the definition of "no zap" changes in the future.
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (3)
33-38: LGTM! Consider adding more context to thezapNativecomment.The new comments provide clear explanations for the
zapNativeandzapDatafields. The restriction onzapNativeusage is important to prevent misuse.Consider expanding the comment for
zapNativeto explain why it can't be used with the native token:/// @param zapNative ETH value to send to the recipient (if any). Can't be used with native token to prevent double-spending.
60-60: LGTM! Consider clarifying the comment for better understanding.The renaming of
callValuetozapNativeis consistent with the changes inBridgeParamsV2. The added comment provides useful context about the replacement of V1'ssendChainGasflag.Consider slightly rewording the comment for clarity:
uint256 zapNative; // ETH value to send to the recipient (if any) - replaces V1's sendChainGas functionality
65-65: LGTM! Consider adding a comment for consistency.The renaming of
callParamstozapDatais consistent with the changes inBridgeParamsV2.For consistency with the
BridgeParamsV2struct, consider adding a comment to explain thezapDatafield:bytes zapData; // Parameters for the Zap to the destination recipient (if any)packages/contracts-rfq/test/FastBridgeV2.t.sol (3)
164-165: LGTM: Terminology updated as per PR objectives.The replacement of "callValue" with "zapNative" and "callParams" with "zapData" in both
tokenParamsV2andethParamsV2structures aligns with the PR objectives. This consistent update enhances code clarity.Consider adding a brief comment explaining the purpose of
zapNativeandzapDatafor better documentation, especially for developers new to the concept of "Zaps".Also applies to: 171-172
219-226: LGTM: Ethereum-related functions updated consistently.The Ethereum-related functions have been updated in line with the token-related functions, maintaining consistency across the codebase. The changes from "callParams" to "zapData" and "callValue" to "zapNative" are applied correctly.
For consistency with the token-related functions, consider adding brief comments explaining the purpose of
zapNativeandzapDatain the Ethereum context as well.
Line range hint
1-300: Overall assessment: Changes are consistent and align with PR objectives.The modifications in this file successfully update the terminology from "call" to "zap" across various functions and structures. These changes are consistent with the PR objectives and improve the clarity of the code. The test file has been updated appropriately without unnecessary modifications to test utility contracts, respecting the retrieved learning.
Consider adding a brief comment at the beginning of the file explaining the transition from "call" to "zap" terminology. This would provide context for the changes and help future developers understand the rationale behind the new naming conventions.
packages/contracts-rfq/test/FastBridgeV2.Src.Zap.t.sol (2)
6-6: Consider providing justification for disabling Solhint rules.The directive
// solhint-disable func-name-mixedcase, orderingdisables linting for function naming and ordering conventions. Adding a comment to explain why these rules are disabled can enhance code clarity and maintainability.
8-9: Add comments to clarify the purpose of constants.Including comments for
ZAP_DATAandZAP_NATIVEwould improve readability by explaining their roles in the test scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (24)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (14 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol (0 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IZapRecipient.sol (1 hunks)
- packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol (6 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.Zap.t.sol (11 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (7 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Zap.t.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Encoding.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.ArbitraryCall.t.sol (0 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.Zap.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol (0 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Zap.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (4 hunks)
- packages/contracts-rfq/test/harnesses/BridgeTransactionV2Harness.sol (2 hunks)
- packages/contracts-rfq/test/libs/BridgeTransactionV2.t.sol (2 hunks)
- packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol (2 hunks)
- packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/NonPayableRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/RecipientMock.sol (1 hunks)
💤 Files with no reviewable changes (3)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.ArbitraryCall.t.sol
- packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol
🧰 Additional context used
📓 Learnings (6)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118 Timestamp: 2024-10-14T14:45:57.214Z Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118 Timestamp: 2024-10-14T14:45:57.214Z Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (1)
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol:97-97 Timestamp: 2024-10-14T13:23:04.509Z Learning: In `packages/contracts-rfq/test/FastBridgeV2.t.sol`, when testing expected revert behaviors, `abi.encodeWithSelector` is used with custom errors and should not be replaced with `BridgeTransactionV2Lib.encodeV2`.packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.Zap.t.sol (2)
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56 Timestamp: 2024-10-14T13:24:45.293Z Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56 Timestamp: 2024-10-14T13:24:45.293Z Learning: In Solidity test contracts under the `test` directory, such as `FastBridgeV2.t.sol`, contracts are not deployed, so gas consumption is not a concern.packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (5)
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118 Timestamp: 2024-10-14T14:45:57.214Z Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.Src.t.sol:919-993 Timestamp: 2024-10-14T14:48:01.520Z Learning: In Solidity test files for FastBridgeV2 (e.g., `packages/contracts-rfq/test/FastBridgeV2.Src.t.sol`), code duplication in test functions is acceptable to maintain readability and maintainability, even if it contradicts DRY principles.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56 Timestamp: 2024-10-14T13:24:45.293Z Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol:97-97 Timestamp: 2024-10-14T13:23:04.509Z Learning: In `packages/contracts-rfq/test/FastBridgeV2.t.sol`, when testing expected revert behaviors, `abi.encodeWithSelector` is used with custom errors and should not be replaced with `BridgeTransactionV2Lib.encodeV2`.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56 Timestamp: 2024-10-14T13:24:45.293Z Learning: In Solidity test contracts under the `test` directory, such as `FastBridgeV2.t.sol`, contracts are not deployed, so gas consumption is not a concern.packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56 Timestamp: 2024-10-14T13:24:45.293Z Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
🪛 GitHub Check: Slither
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol
[warning] 227-232: Assembly usage
BridgeTransactionV2Lib.zapNative(bytes) (contracts/libs/BridgeTransactionV2.sol#227-232) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#229-231)
🔇 Additional comments (71)
packages/contracts-rfq/contracts/interfaces/IZapRecipient.sol (3)
1-2: LGTM: Appropriate license and Solidity version.The file header correctly specifies the MIT license and uses a recent Solidity version (^0.8.0), which is suitable for this project.
4-4: LGTM: Well-defined interface name.The
IZapRecipientinterface is appropriately named, following the convention of prefixing interfaces with 'I'. This aligns with the PR objectives of updating terminology and introducing the Zap functionality.
5-5: LGTM: Well-defined zap function, with a minor clarification needed.The
zapfunction signature is well-defined and aligns with the PR objectives:
- It includes parameters for token, amount, and zapData as described in the PR summary.
- The
payablemodifier allows it to receive native tokens, consistent with thezapNativeconcept.However, could you please clarify the purpose of the
bytes4return type? Is this intended for selector compatibility or another specific use case?To ensure consistency across the codebase, let's check for other occurrences of similar function signatures:
✅ Verification successful
To accurately search for
zapfunction signatures in Solidity files, please execute the following script:
Verified:
zapfunction signature is consistent across the codebase.
- The
zapfunction inIZapRecipient.soland its mock implementations consistently use thebytes4return type for selector compatibility.- No additional implementations outside of test mocks were found, ensuring no conflicting signatures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar zap function signatures in the codebase rg --type solidity 'function\s+zap\s*\(' -A 3Length of output: 80
Script:
#!/bin/bash # Search for similar zap function signatures in Solidity files rg 'function\s+zap\s*\(' --glob '*.sol' -A 3Length of output: 2073
packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol (2)
Line range hint
1-15: Reminder: Maintain mock contract integrity.While not directly related to the changes, it's important to note:
- The contract is correctly marked as "DO NOT USE IN PRODUCTION" in the comments.
- The empty
testNoReturnValueRecipientfunction serves to exclude this contract from coverage reports.These features are crucial for maintaining the integrity and purpose of this mock contract. Ensure they are preserved in future modifications.
14-14: Function renamed to align with new terminology.The function has been correctly renamed from
fastBridgeTransferReceivedtozap, which aligns with the PR objectives of updating terminology. The function signature and implementation remain the same, maintaining its purpose as an incorrectly implemented mock for testing.To ensure consistency across the codebase, let's verify the usage of this new function name:
✅ Verification successful
Function renaming verified successfully.
No references to
fastBridgeTransferReceivedwere found. Thezapfunction is consistently used across relevant contracts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to 'fastBridgeTransferReceived' and new uses of 'zap' echo "Checking for any remaining references to 'fastBridgeTransferReceived':" rg --type solidity 'fastBridgeTransferReceived' echo "Checking for new uses of 'zap' function:" rg --type solidity 'function zap\('Length of output: 405
Script:
#!/bin/bash # Description: Check for any remaining references to 'fastBridgeTransferReceived' and new uses of 'zap' function in Solidity files echo "Checking for any remaining references to 'fastBridgeTransferReceived':" rg 'fastBridgeTransferReceived' -g '*.sol' echo "Checking for new uses of 'zap' function:" rg 'function zap\(' -g '*.sol'Length of output: 1218
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (1)
11-12: LGTM: New error declarations align with PR objectivesThe new error declarations
ZapDataLengthAboveMax()andZapNativeNotSupported()are well-aligned with the PR objectives of introducing Zap functionality and updating terminology. These errors effectively replace the removedCallParamsLengthAboveMax()andNativeTokenCallValueNotSupported(), maintaining the interface's error handling capabilities while reflecting the new Zap-related concepts.packages/contracts-rfq/test/mocks/RecipientMock.sol (2)
8-8: LGTM: Contract signature updated correctly.The contract now implements the
IZapRecipientinterface, which is consistent with the PR objectives to update terminology and introduce new zap functionality.
4-4: LGTM: Import statement updated correctly.The import statement has been properly updated to reflect the new
IZapRecipientinterface, which aligns with the PR objectives.To ensure the import path is correct, please run the following command:
✅ Verification successful
✅ Verified: Import statement correctly updated.
The
IZapRecipientinterface exists, confirming that the import path is accurate and functional.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and content of the IZapRecipient interface file fd -e sol -x cat {} | rg -U "interface IZapRecipient"Length of output: 83
packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol (2)
4-4: LGTM: Import statement updated correctly.The import statement has been appropriately updated to use
IZapRecipient, which aligns with the new zap functionality introduced in this PR.
Line range hint
1-20: Overall changes are appropriate and consistent.The modifications to this mock contract align well with the PR objectives:
- The import statement has been updated to use
IZapRecipient.- The
fastBridgeTransferReceivedfunction has been replaced with azapfunction, maintaining the same purpose of returning excessive bytes for testing.- The rest of the contract, including its name, receive function, and test function, remains unchanged.
These changes appropriately reflect the new zap functionality while maintaining the contract's purpose as a mock for testing.
packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol (1)
4-4: LGTM: Import statement updated correctly.The import statement has been properly updated to reflect the new
IZapRecipientinterface, which aligns with the PR objectives of introducing Zap functionality.packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.Zap.t.sol (2)
6-7: LGTM: Contract declaration and inheritance are appropriate.The contract name
FastBridgeV2GasBenchmarkSrcZapTestclearly indicates its purpose as a Zap-specific extension of the existing gas benchmark test. The inheritance fromFastBridgeV2GasBenchmarkSrcTestis correct, allowing for the addition of Zap-related functionality while maintaining the base test structure.
8-9: LGTM: Empty test function serves its purpose.The
testFastBridgeV2GasBenchmarkSrcZapTestfunction, although empty, serves a specific purpose as explained in the comment. It prevents this contract from appearing in the coverage report, which is a valid approach for specialized test contracts like this one.packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Zap.t.sol (3)
8-8: LGTM: Contract name updated to reflect new functionality.The contract name change from
FastBridgeV2GasBenchmarkDstArbitraryCallTesttoFastBridgeV2GasBenchmarkDstZapTestaccurately reflects the shift from "arbitrary call" to "Zap" functionality, aligning with the PR objectives.
9-11: LGTM: Comments updated to reflect new terminology.The comments have been appropriately updated to replace "arbitrary call" with "Zap", maintaining consistency with the new terminology while preserving the clarity of the explanation.
16-16: LGTM: Test function name updated.The function name change from
testFastBridgeV2GasBenchmarkDstArbitraryCallTesttotestFastBridgeV2GasBenchmarkDstZapTestis consistent with the contract name change and maintains its purpose as a placeholder for coverage reports.packages/contracts-rfq/test/libs/BridgeTransactionV2.t.sol (2)
52-52: LGTM! Correct update to new terminology.The change from
callValuetozapNativeis consistent with the new terminology introduced in the PR objectives. This update correctly reflects the changes in theBridgeTransactionV2structure.
Line range hint
1-67: Summary: Successful update of terminology in test contractThe changes in this file consistently update the terminology from
callValuetozapNativeandcallParamstozapDatathroughout the test contract. These updates align well with the PR objectives of introducing new zap functionality and enhancing clarity.Key points:
- The
assertEqfunction has been updated to compare the new fields.- The
test_roundtripfunction now asserts the newzapNativeandzapDatafields.- The overall structure and logic of the tests remain intact, ensuring that existing functionality is still properly tested.
These changes successfully reflect the updates made to the
BridgeTransactionV2structure while maintaining the integrity of the test suite.packages/contracts-rfq/test/harnesses/BridgeTransactionV2Harness.sol (3)
75-77: LGTM! Verify the corresponding library function.The change from
callParamstozapDataaligns with the PR objectives and maintains the same behavior. The implementation looks correct.To ensure consistency, let's verify the corresponding library function:
#!/bin/bash # Description: Verify the existence and signature of zapData function in BridgeTransactionV2Lib # Test: Search for the zapData function in BridgeTransactionV2Lib ast-grep --lang solidity --pattern 'function zapData(bytes calldata encodedTx) internal pure returns (bytes calldata) { $$$$ }'
Line range hint
1-77: Overall changes look good. Consider checking for consistency across the project.The updates in this file accurately reflect the new terminology (zap instead of call) as outlined in the PR objectives. The changes are minimal and focused, which is good for maintainability.
To ensure project-wide consistency, let's check for any remaining instances of the old terminology:
#!/bin/bash # Description: Check for any remaining instances of 'callValue' or 'callParams' in Solidity files # Test: Search for 'callValue' and 'callParams' in all Solidity files rg --type solidity -e 'callValue' -e 'callParams'If this search returns any results, consider updating those instances as well to maintain consistency across the project.
55-57: LGTM! Verify the corresponding library function.The change from
callValuetozapNativealigns with the PR objectives and maintains the same behavior. The implementation looks correct.To ensure consistency, let's verify the corresponding library function:
packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (1)
53-54: LGTM: Clear and informative comment updateThe updated comment provides a clear explanation of the relationship between
sendChainGasin V1 andzapNativein V2 of the bridge transaction structure. This helps developers understand the changes in the data structure and the expected behavior.packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
43-44: LGTM! Field names updated to reflect new terminology.The renaming of
callValuetozapNativeandcallParamstozapDataaligns well with the new terminology introduced in the PR objectives. These changes enhance clarity and consistency within the codebase.packages/contracts-rfq/test/FastBridgeV2.t.sol (3)
80-83: LGTM: Updated comments align with new terminology.The changes in these comments accurately reflect the transition from "callParams" to "zapData", which is consistent with the PR objectives. The comments provide clear explanations for creating invalid and mock requests, enhancing code readability.
200-202: LGTM: Function updated to use new "zapData" terminology.The function
setTokenTestZapDatahas been correctly updated to reflect the new terminology. It properly sets thezapDatafor bothtokenParamsV2andtokenTx, maintaining consistency with the PR objectives.
205-207: LGTM: Function updated to use new "zapNative" terminology.The function
setTokenTestZapNativehas been appropriately updated to align with the new terminology. It correctly sets thezapNativevalue for bothtokenParamsV2andtokenTx, maintaining consistency with the PR objectives.packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (13)
26-26: Constant renamed from CALL_VALUE to ZAP_NATIVEThe constant
CALL_VALUEhas been renamed toZAP_NATIVE. This change improves clarity by using terminology consistent with the new "Zap" functionality.
60-61: Function renamed from assertEmptyCallParams to assertEmptyZapDataThe function
assertEmptyCallParamshas been renamed toassertEmptyZapData. This change aligns with the new terminology and improves code readability.
72-72: Updated chainGasAmount calculationThe calculation of
chainGasAmountnow usesbridgeTx.zapNativeinstead ofbridgeTx.callValue. This change is consistent with the new terminology and functionality.
161-168: New test case for token relay with ZapNativeThis new test case verifies the behavior of relaying a token transaction with a non-zero
ZapNativevalue. It checks if the correct amount is transferred to the recipient.
171-178: New test case for token relay with RelayerAddress and ZapNativeThis test case is similar to the previous one but includes a relayer address. It verifies that the correct amount is transferred to the recipient when using both a relayer address and a non-zero
ZapNativevalue.
183-184: Updated test cases for excessive return value recipientThese test cases have been updated to use
zapDatainstead ofcallParams. They now include checks to ensure that the tests succeed whenzapDatais empty and are set up to be overridden in derived tests wherezapDatais present.Also applies to: 186-187, 192-196, 201-205, 208-215, 218-219, 224-228
235-236: Updated test cases for incorrect return value recipientSimilar to the excessive return value recipient tests, these test cases have been updated to use
zapDatainstead ofcallParams. They are set up to succeed whenzapDatais empty and can be overridden in derived tests.Also applies to: 238-239, 244-248, 253-257, 260-267, 270-271, 276-280
298-302: New test cases for non-payable recipient with ZapNativeThese new test cases verify that attempting to relay a token transaction with a non-zero
ZapNativevalue to a non-payable recipient results in a revert. This is important for ensuring correct behavior when dealing with non-payable contracts.Also applies to: 305-309
326-327: Updated test cases for no-op recipientThese test cases have been updated to use
zapDatainstead ofcallParams. They are set up to succeed whenzapDatais empty and can be overridden in derived tests wherezapDatais present.Also applies to: 329-330, 335-336, 341-345, 348-352, 355-356, 361-362
369-370: Updated test cases for no return value recipientThese test cases have been updated to use
zapDatainstead ofcallParams. They are set up to succeed whenzapDatais empty and can be overridden in derived tests wherezapDatais present.Also applies to: 372-373, 378-379, 384-388, 391-398, 401-402, 407-408
494-503: New test cases for token relay with ZapNative revertsThese new test cases verify that attempting to relay a token transaction with incorrect
ZapNativevalues (zero, lower, or higher than expected) results in a revert. This ensures that the contract correctly handles invalidZapNativeamounts.Also applies to: 506-509, 512-521, 524-527
530-538: New test cases for ETH relay with ZapNative revertsThese new test cases verify that attempting to relay an ETH transaction with a non-zero
ZapNativevalue results in a revert. This is important asZapNativeis not supported for ETH transactions.Also applies to: 541-552
Line range hint
1-638: Overall assessment of changesThe modifications in this file are comprehensive and align well with the introduction of the "Zap" functionality. Key points:
- Consistent renaming from
CALL_VALUEtoZAP_NATIVEandcallParamstozapData.- New test cases added to cover
ZapNativefunctionality for both token and ETH transactions.- Existing test cases updated to accommodate the new terminology and potential
zapDatapresence.- Edge cases and error conditions are well-covered, especially for non-payable recipients and incorrect
ZapNativevalues.These changes enhance the test coverage and ensure that the new "Zap" functionality is thoroughly validated. The code maintains good readability and follows consistent naming conventions.
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1)
47-47: LGTM. Verify consistent usage ofzapNativeacross the codebase.The change from
bridgeTx.callValue > 0tobridgeTx.zapNative > 0for determiningsendChainGasaligns with the new zap functionality introduced in this PR. This modification appears to be correct and consistent with the PR objectives.To ensure this change is applied consistently, please run the following script to check for any remaining instances of
callValuethat might need to be updated:packages/contracts-rfq/test/FastBridgeV2.Src.Zap.t.sol (2)
32-44: Ensure correct error handling for zapData length exceeding maximum.The tests correctly expect a revert with
ZapDataLengthAboveMax.selectorwhen zapData exceeds the maximum allowed length. Verify that this behavior aligns with the contract's intended functionality and that the error message provides clear guidance.
86-96: Verify the handling of zapNative in ETH bridging scenarios.The tests correctly expect a revert with
ZapNativeNotSupported.selectorwhen attempting to bridge ETH withzapNative. Ensure that this restriction is intentional and documented in the contract's specifications.packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol (7)
25-26: Offsets forzapNativeandzapDataare correctly definedThe offsets for the new fields
zapNativeandzapDatain the packedBridgeTransactionV2struct are accurately specified, ensuring proper data alignment.
42-43: Constants for new offsets are appropriately addedThe constants
OFFSET_ZAP_NATIVEandOFFSET_ZAP_DATAare defined with correct values corresponding to the struct layout.
53-53: Validation length check updated to include new fieldsThe
validateV2function's length check now accounts for the newzapDatafield, ensuring that the encoded transaction is of sufficient length.
83-85:encodeV2function correctly includes new Zap fieldsThe
encodeV2function now includesbridgeTx.zapNativeandbridgeTx.zapData, ensuring the new Zap fields are encoded properly.
110-111:decodeV2function properly decodes new Zap fieldsThe
decodeV2function has been updated to decodezapNativeandzapData, correctly populating thebridgeTxstruct with the new fields.
226-230: Efficient extraction ofzapNativeusing assemblyThe
zapNativefunction uses inline assembly to efficiently extract thezapNativevalue from the encoded transaction. The assembly code is correctly implemented.
234-236:zapDataextraction via slicing is accurateThe
zapDatafunction correctly extracts thezapDatafrom the encoded transaction using slicing, handling variable-length data appropriately.packages/contracts-rfq/test/FastBridgeV2.Dst.Zap.t.sol (7)
11-12: Contract and constant renamed to reflect Zap functionality.The renaming of the contract to
FastBridgeV2DstZapTestand the constant toZAP_DATAaligns with the updated terminology and enhances code clarity.
24-25: Fixture setup methods updated with ZapData.The calls to
setTokenTestZapData(ZAP_DATA)andsetEthTestZapData(ZAP_DATA)correctly update the test setup to useZAP_DATA, ensuring the tests are configured for the new Zap functionality.
52-53: Expected calldata updated for Zap function call.The
getExpectedCalldatafunction now correctly encodes the call toRecipientMock.zapwith the updated parameters, matching the new function signature and ensuring accurate test assertions.
Line range hint
84-234: Test function names updated to reflect ZapData usage.The renaming of test functions from "CallParams" to "ZapData" throughout the file enhances consistency and accurately reflects the updated functionality. This change improves code readability and maintains alignment with the new terminology.
Line range hint
242-282: Test cases for no return value recipient updated appropriately.The updates to test functions handling
noReturnValueRecipientcases withZapDataensure that scenarios where the recipient does not return a value are properly tested under the new Zap functionality.
306-317: Added tests for recipient reverts with ZapNative.The inclusion of tests that handle recipient reverts when
ZapNativeis present strengthens the test suite by covering additional edge cases, ensuring robust error handling in the contract.
332-342: Updated tests for relaying ETH without ZapData.The tests
test_relay_eth_noZapData_revert_recipientRevertsandtest_relay_eth_withRelayerAddress_noZapData_revert_recipientRevertshave been appropriately updated to handle cases where noZapDatais provided, verifying the contract's behavior in these scenarios.packages/contracts-rfq/contracts/FastBridgeV2.sol (15)
14-14: ImportingIZapRecipientinterface is appropriate.The import statement correctly includes the
IZapRecipientinterface, which is necessary for implementing the new Zap functionality in the contract.
22-23: DefineNATIVE_GAS_TOKENconstant appropriately.The
NATIVE_GAS_TOKENconstant is set to the commonly used placeholder address0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE, representing the native gas token on various networks. Ensure this address is used consistently across all contracts and aligns with any external libraries or standards used.
34-35:MAX_ZAP_DATA_LENGTHis appropriately defined.The maximum length of
zapDatais set to2 ** 16 - 1(65535 bytes), which should be sufficient for the intended use cases. This limit helps prevent excessively large data payloads that could increase gas costs or processing time.
62-63: InitializezapNativeandzapDatawith default values in legacybridgefunction.The legacy
bridgefunction setszapNativeto0andzapDatato an empty byte array. This ensures backward compatibility and prevents unintended behavior when the new parameters are not provided.
Line range hint
122-126: Refund logic correctly handles native gas token and ERC20 tokens.In the
refundfunction, the contract appropriately checks if the token is theNATIVE_GAS_TOKENand usesAddress.sendValuefor native token refunds andIERC20.safeTransferfor ERC20 tokens. This ensures that refunds are handled safely and correctly based on the token type.
Line range hint
140-157:getBridgeTransactionmaintains backward compatibility.The
getBridgeTransactionfunction attempts to decode the request into a V2 transaction. If successful, it maps the V2 structure to the V1 format, handlingzapNativeas a zero/non-zero flag and ignoringzapData. This approach maintains backward compatibility with existing interfaces while introducing new functionality.
206-207: IncludezapNativeandzapDatain bridge transaction encoding.The
bridgefunction correctly incorporateszapNativeandzapDatafromparamsV2when encoding the bridge transaction. This integration ensures that the new Zap functionality is properly included in the transaction data.
222-222: UpdatesendChainGasflag based onzapNativeinBridgeRequestedevent.In the
emit BridgeRequestedstatement, thesendChainGasparameter is set totruewhenzapNativeis non-zero. This accurately reflects whether additional native gas token is requested by the recipient.
236-240: ExtractzapNativefrom the request for use in relay processing.The
zapNativevalue is correctly retrieved from the request usingrequest.zapNative(). This value is essential for determining if additional native gas token should be forwarded during the relay process.
252-252: SetchainGasAmountinBridgeRelayedevent based onzapNative.The
chainGasAmountparameter in theBridgeRelayedevent is accurately set tozapNative, indicating the amount of additional native gas token involved in the relay.
257-259: Correctly disallowzapNativewhen token isNATIVE_GAS_TOKEN.The contract checks if
tokenis theNATIVE_GAS_TOKENand reverts withZapNativeNotSupported()ifzapNativeis not zero. This ensures that additional native gas token is not incorrectly requested when the destination token is already the native gas token.
284-289: Handle scenarios wherezapDatais missing butmsg.valueis sent.The code appropriately transfers the full
msg.valueto the recipient whenzapDatais absent butmsg.valueis non-zero. This accounts for cases where the recipient expects additional native gas token without invoking a Zap.
Line range hint
333-337: Ensure safe transfer of tokens inclaimfunction.In the
claimfunction, the contract correctly handles the transfer of tokens to the relayer. It usesAddress.sendValuefor theNATIVE_GAS_TOKENandIERC20.safeTransferfor ERC20 tokens, ensuring safe execution based on the token type.
361-365: Correctly handleNATIVE_GAS_TOKENin_takeBridgedUserAsset.When the token is the
NATIVE_GAS_TOKEN, the function verifies thatmsg.valuematches theamountand assignsamountTakenaccordingly. This ensures accurate accounting for the native gas token in the bridging process.
429-431: ValidatezapDatalength and disallowzapNativewithNATIVE_GAS_TOKEN.The contract verifies that the length of
zapDatadoes not exceedMAX_ZAP_DATA_LENGTH, reverting withZapDataLengthAboveMax()if it does. It also ensures thatzapNativeis zero whenparams.destTokenis theNATIVE_GAS_TOKEN, preventing unsupported scenarios.
ChiTimesChi
force-pushed
the
refactor/FbV2-zaps
branch
from
f8665f5 to
08c0bb8
Compare
Deploying sanguine-fe with
|
| Latest commit: |
5e51cfe
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6618cbec.sanguine-fe.pages.dev |
| Branch Preview URL: | https://refactor-fbv2-zaps.sanguine-fe.pages.dev |
|
This branch has been rebased on top of #3284 merge conflict changes to make the commit history here cleaner. |
ChiTimesChi
force-pushed
the
refactor/FbV2-zaps
branch
from
08c0bb8 to
58f8578
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol (1)
234-237: LGTM with suggestion: NewzapDatagetter function.The new
zapDatafunction correctly extracts the Zap's data using a slice operation, which is appropriate for variable-length data. The comment accurately describes its purpose.Suggestion: Consider adding a check to ensure that
OFFSET_ZAP_DATAis not out of bounds before slicing. This would prevent potential issues if the encoded transaction is shorter than expected.Here's a suggested improvement:
function zapData(bytes calldata encodedTx) internal pure returns (bytes calldata zapData_) { + require(encodedTx.length >= OFFSET_ZAP_DATA, "BridgeTransactionV2: Invalid encoded transaction length"); zapData_ = encodedTx[OFFSET_ZAP_DATA:]; }packages/contracts-rfq/test/FastBridgeV2.Dst.Zap.t.sol (2)
333-333: LGTM: ETH transfer test setup updated, with a suggestionThe test setup for ETH transfer scenarios has been correctly updated to use
setEthTestZapData("")instead of the previous implementation. This change is consistent with the new terminology and functionality.Consider extracting the empty string
""to a named constant likeNO_ZAP_DATAto improve readability and maintain consistency if this value is used in multiple places. For example:bytes public constant NO_ZAP_DATA = ""; // Then in the test functions: setEthTestZapData(NO_ZAP_DATA);This change would make the intent clearer and easier to maintain if the concept of "no zap data" is used elsewhere in the tests.
Also applies to: 342-342
Line range hint
1-349: Overall LGTM with suggestions for improvementThe changes in this file successfully implement the transition from "arbitrary call" to "zap" terminology. The updates are consistent throughout the test suite, covering various scenarios including token transfers, ETH transfers, and different recipient behaviors. The introduction of
ZAP_NATIVEfunctionality is well-tested across different cases.To further improve the code:
Consider adding comments or documentation to explain the
ZAP_NATIVEconcept and its implications, especially if it represents a significant change in the contract's behavior.It might be beneficial to add a brief comment at the beginning of the file explaining the overall change from "arbitrary call" to "zap" to provide context for future readers.
If
ZAP_NATIVEis a constant or a significant value, consider defining it as a named constant at the contract level for better readability and maintenance.These suggestions would enhance the code's clarity and make it easier for other developers to understand the changes and new functionality.
packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (4)
26-26: LGTM! Consider adding a comment for clarity.The constant name change from CALL_VALUE to ZAP_NATIVE aligns well with the new terminology. However, the purpose of this specific value (1_337_420) is not immediately clear.
Consider adding a brief comment explaining the significance or purpose of this value, especially if it's used for testing scenarios.
183-184: LGTM! Comments updated to reflect new terminology. Consider adding more context.The comments have been correctly updated to mention
zapDatainstead ofcallParams, aligning with the new terminology. This change maintains consistency throughout the test file.Consider expanding these comments slightly to provide more context about the significance of empty
zapDataand why the tests behave differently whenzapDatais present. This could improve the understanding for future developers working on these tests.Also applies to: 235-236, 326-327, 369-370
494-509: LGTM! Comprehensive new tests for ZAP_NATIVE functionality. Consider adding comments.New test functions have been added to cover important edge cases and error conditions related to the ZAP_NATIVE functionality:
- Tests for reverting behavior with incorrect ZAP_NATIVE values in token transactions.
- Tests for reverting behavior when using ZAP_NATIVE with ETH transactions.
These additions improve the test coverage and help ensure the correct implementation of the new functionality.
Consider adding brief comments before each group of new test functions to explain their purpose and the specific scenarios they are testing. This would enhance the readability and maintainability of the test suite.
Also applies to: 530-553
Line range hint
1-624: Overall LGTM! Comprehensive and consistent updates to implement new zap functionality.This update to the FastBridgeV2.Dst.t.sol test file successfully implements the new "zap" terminology and functionality:
- Consistent renaming of variables, functions, and comments from "call" to "zap" related terms.
- Addition of new test cases to cover the ZAP_NATIVE functionality, including edge cases and error conditions.
- Preservation of existing test logic while updating to the new terminology.
The changes align well with the PR objectives and maintain a high standard of test coverage. The consistency in applying these changes across the entire file is commendable.
As the zap functionality introduces new complexity, consider adding a brief comment at the beginning of the file explaining the concept of "zaps" and how they differ from the previous "call" functionality. This would provide valuable context for developers who may be new to the project or revisiting this code in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (24)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (14 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol (0 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IZapRecipient.sol (1 hunks)
- packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol (6 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.Zap.t.sol (11 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (7 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Zap.t.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Encoding.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.ArbitraryCall.t.sol (0 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.Zap.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol (0 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Zap.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (4 hunks)
- packages/contracts-rfq/test/harnesses/BridgeTransactionV2Harness.sol (2 hunks)
- packages/contracts-rfq/test/libs/BridgeTransactionV2.t.sol (2 hunks)
- packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol (2 hunks)
- packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/NonPayableRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/RecipientMock.sol (1 hunks)
💤 Files with no reviewable changes (3)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.ArbitraryCall.t.sol
- packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol
- packages/contracts-rfq/contracts/interfaces/IZapRecipient.sol
- packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Zap.t.sol
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Encoding.t.sol
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.Zap.t.sol
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol
- packages/contracts-rfq/test/FastBridgeV2.t.sol
- packages/contracts-rfq/test/harnesses/BridgeTransactionV2Harness.sol
- packages/contracts-rfq/test/libs/BridgeTransactionV2.t.sol
- packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol
- packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol
- packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol
- packages/contracts-rfq/test/mocks/NonPayableRecipient.sol
- packages/contracts-rfq/test/mocks/RecipientMock.sol
🧰 Additional context used
🔇 Additional comments (51)
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol (6)
25-26: LGTM: Updated comments for new fields.The comments accurately describe the new
zapNativeandzapDatafields and their positions in the packed struct. The use of***for the end ofzapDatacorrectly implies it's a variable-length field.
42-43: LGTM: New offset constants for Zap fields.The new constants
OFFSET_ZAP_NATIVEandOFFSET_ZAP_DATAare correctly defined and consistent with the comments above. The naming convention aligns with other offset constants in the library.
53-53: LGTM: Updated minimum length check invalidateV2.The minimum length check now correctly uses
OFFSET_ZAP_DATA, ensuring that the encoded transaction includes all static fields, including the new Zap fields. The error message remains appropriate for the unchanged error condition.
83-85: LGTM: UpdatedencodeV2to include Zap fields.The
encodeV2function has been correctly updated to include the newzapNativeandzapDatafields at the end of the packed encoding. The comment accurately describes these as "New V2 fields: Zap".
110-111: LGTM: UpdateddecodeV2to extract Zap fields.The
decodeV2function has been correctly updated to extract the newzapNativeandzapDatafields using their corresponding getter functions. The order of extraction matches the encoding order, ensuring consistency.
226-232: LGTM: NewzapNativegetter function.The new
zapNativefunction correctly extracts the Zap's native value using inline assembly. The implementation is consistent with other getter functions in the library, and the comment accurately describes its purpose.packages/contracts-rfq/test/FastBridgeV2.Dst.Zap.t.sol (12)
11-12: LGTM: Contract and constant renaming is consistentThe contract name change from
FastBridgeV2DstArbitraryCallTesttoFastBridgeV2DstZapTestand the constant renaming fromCALL_PARAMStoZAP_DATAare consistent with the overall shift in terminology from "arbitrary call" to "zap". This improves clarity and aligns well with the new functionality.
24-25: LGTM: Fixture setup methods updated correctlyThe changes in the
createFixturesV2function, replacingsetTokenTestCallParamsandsetEthTestCallParamswithsetTokenTestZapDataandsetEthTestZapDatarespectively, are consistent with the new "zap" terminology. This ensures that the test fixtures are set up correctly for the updated functionality.
28-30: LGTM: Comment updated to reflect new terminologyThe comment for the
expectBridgeRelayedfunction has been correctly updated to mention "Zap" instead of "arbitrary call". This change maintains consistency with the new terminology and accurately describes the function's purpose.
52-53: LGTM: Expected calldata updated for new zap functionalityThe
getExpectedCalldatafunction has been correctly updated to useRecipientMock.zapinstead ofRecipientMock.fastBridgeTransferReceived. This change aligns with the new "zap" functionality and ensures that the test is checking for the correct method call.
84-84: LGTM: Test function names updated consistentlyThe test function names have been systematically updated to include "ZapDataPresent" instead of "CallParamsPresent". This change maintains consistency with the new terminology and clearly communicates the purpose of each test. The updates cover various scenarios including token transfers, relayer addresses, and ETH transfers.
Also applies to: 90-90, 100-100, 111-111, 122-122, 128-128
198-198: LGTM: No-op recipient test function names updated consistentlyThe test function names for no-op recipient scenarios have been systematically updated to include "ZapDataPresent" instead of "CallParamsPresent". This change maintains consistency with the new terminology across all test cases, including those for no-op recipients.
Also applies to: 204-204, 210-210, 217-217, 228-228, 234-234
211-211: LGTM:ZAP_NATIVEparameter added to no-op recipient testsThe
ZAP_NATIVEparameter has been consistently added to the no-op recipient test functions. This change ensures that the new zap functionality is thoroughly tested across different scenarios, including cases where the recipient contract doesn't implement the expected behavior.Also applies to: 214-214, 222-222, 225-225
242-242: LGTM: No return value recipient test function names updated consistentlyThe test function names for no return value recipient scenarios have been systematically updated to include "ZapDataPresent" instead of "CallParamsPresent". This change maintains consistency with the new terminology across all test cases, including those for recipients that don't return a value.
Also applies to: 248-248, 258-258, 265-265, 276-276, 282-282
259-259: LGTM:ZAP_NATIVEparameter added to no return value recipient testsThe
ZAP_NATIVEparameter has been consistently added to the no return value recipient test functions. This change ensures that the new zap functionality is thoroughly tested across different scenarios, including cases where the recipient contract doesn't return a value.Also applies to: 262-262, 270-270, 273-273
306-317: LGTM: New test functions added forZAP_NATIVEin recipient reverts scenariosNew test functions have been added to cover scenarios where the recipient contract reverts when
ZAP_NATIVEis used. These additions enhance the test coverage by including important edge cases:
test_relay_token_withZapNative_revert_recipientRevertstest_relay_token_withRelayerAddressZapNative_revert_recipientRevertsThese tests ensure that the contract behaves correctly when a recipient reverts during a zap operation with native token transfer.
332-332: LGTM: ETH transfer test function names updated consistentlyThe test function names for ETH transfer scenarios have been updated from "noCallParams" to "noZapData":
test_relay_eth_noZapData_revert_recipientRevertstest_relay_eth_withRelayerAddress_noZapData_revert_recipientRevertsThese changes maintain consistency with the new terminology and accurately reflect the scenarios being tested.
Also applies to: 341-341
105-105: LGTM: NewZAP_NATIVEparameter introducedThe test functions have been updated to include a new
ZAP_NATIVEparameter, which replaces the previouscallValue. This change appears to be related to the new zap functionality and is consistently applied across relevant test cases.Could you please provide more information about the
ZAP_NATIVEparameter? Specifically:
- What does it represent in the context of a zap operation?
- How is it different from the previous
callValue?- Are there any specific conditions or constraints for its usage?
To help verify the usage of
ZAP_NATIVE, let's search for its definition and other occurrences:Also applies to: 108-108, 116-116, 119-119
packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (5)
60-62: LGTM! Function updated to reflect new terminology.The function has been correctly renamed from
assertEmptyCallParamstoassertEmptyZapData, and the parameter name has been updated accordingly. The assertion logic remains correct, and the error message has been appropriately updated.
72-72: LGTM! Variable name updated to reflect new terminology.The condition for
chainGasAmounthas been correctly updated to usebridgeTx.zapNativeinstead ofbridgeTx.callValue. This change aligns with the new terminology while maintaining the original logic.
161-168: LGTM! Function and variable names updated to new terminology.The test function has been correctly renamed from
test_relay_token_withCallValuetotest_relay_token_withZapNative. All instances ofCALL_VALUEhave been replaced withZAP_NATIVE. These changes consistently apply the new terminology while maintaining the original test logic.
171-178: LGTM! Function and variable names consistently updated.The test function has been appropriately renamed from
test_relay_token_withRelayerAddressCallValuetotest_relay_token_withRelayerAddressZapNative. All occurrences ofCALL_VALUEhave been replaced withZAP_NATIVE. These changes maintain consistency with the new terminology while preserving the original test logic.
186-187: LGTM! Consistent terminology updates across test functions.The test function names and their contents have been systematically updated to use the new "zap" terminology:
- Function names now include "zapData" instead of "callParams".
- Function bodies now use
assertEmptyZapDatainstead ofassertEmptyCallParams.setTokenTestCallValuehas been renamed tosetTokenTestZapNative.These changes maintain consistency with the new terminology while preserving the original test logic and structure.
Also applies to: 192-196, 201-205, 208-215, 218-219, 224-228, 238-239, 244-248, 253-257, 260-267, 270-271, 276-280, 329-330, 335-336, 341-345, 348-352, 355-356, 361-362, 372-373, 378-379, 384-388, 391-398, 401-402, 407-408
packages/contracts-rfq/test/FastBridgeV2.Src.Zap.t.sol (14)
8-9: Constants are defined clearly and enhance readabilityThe constants
ZAP_DATAandZAP_NATIVEare well-defined and their usage promotes clarity and maintainability in the test code.
11-15: Test fixtures are correctly initialized with zap dataThe
createFixturesV2function properly overrides the parent implementation and initializes the test fixtures with the specified zap data for both token and ETH tests.
20-24: Token bridging supports maximum zapData lengthThe test
test_bridge_token_zapDataLengthMax()effectively verifies that the token bridging function can handle the maximum allowed zapData length of2 ** 16 - 1bytes without errors.
26-30: ETH bridging supports maximum zapData lengthThe test
test_bridge_eth_zapDataLengthMax()accurately confirms that the ETH bridging function accepts zapData up to the maximum length, ensuring consistency with token bridging.
32-37: Properly handling zapData length exceeding maximum for tokensThe test
test_bridge_token_revert_zapDataLengthAboveMax()correctly expects a revert when the zapData length exceeds the maximum limit, ensuring that the contract enforces this constraint for token bridging.
39-44: Properly handling zapData length exceeding maximum for ETHThe test
test_bridge_eth_revert_zapDataLengthAboveMax()ensures that ETH bridging operations revert when provided zapData exceeds the maximum allowed length, maintaining contract integrity.
48-52: Token bridging with zapNative and no zapData operates as expectedThe function
test_bridge_token_withZapNative_noZapData()correctly tests token bridging whenzapNativeis specified andzapDatais empty, confirming that the bridging logic handles this scenario appropriately.
54-58: Token bridging from a different sender with zapNative and no zapDataThe test
test_bridge_token_diffSender_withZapNative_noZapData()verifies that token bridging functions correctly when initiated by a different sender, withzapNativespecified and nozapData.
60-65: ETH bridging with zapNative and no zapData correctly revertsThe function
test_bridge_eth_withZapNative_noZapData_revert()appropriately expects a revert due toZapNativeNotSupported, ensuring that the contract disallows unsupported operations for ETH bridging.
67-72: ETH bridging from a different sender with zapNative and no zapData correctly revertsThe test
test_bridge_eth_diffSender_withZapNative_noZapData_revert()confirms that attempting ETH bridging withzapNativefrom a different sender correctly reverts, enforcing the intended contract restrictions.
76-79: Token bridging with zapNative and zapData functions correctlyThe test
test_bridge_token_withZapNative_withZapData()successfully verifies that token bridging works as expected when bothzapNativeandzapDataare provided.
81-84: Token bridging from a different sender with zapNative and zapDataThe function
test_bridge_token_diffSender_withZapNative_withZapData()ensures that token bridging initiated by a different sender operates correctly when bothzapNativeandzapDataare specified.
86-90: ETH bridging with zapNative and zapData correctly revertsThe test
test_bridge_eth_withZapNative_withZapData_revert()accurately expects a revert due toZapNativeNotSupported, confirming that the contract properly restricts unsupported ETH bridging operations.
92-96: ETH bridging from a different sender with zapNative and zapData correctly revertsThe function
test_bridge_eth_diffSender_withZapNative_withZapData_revert()confirms that ETH bridging with bothzapNativeandzapDatafrom a different sender correctly results in a revert, upholding contract constraints.packages/contracts-rfq/contracts/FastBridgeV2.sol (14)
14-14: Import IZapRecipient interface for zap functionalityThe import statement correctly includes
IZapRecipient, which is necessary for the new zap functionality introduced in this update.
34-35: Set MAX_ZAP_DATA_LENGTH constant correctlyThe maximum length for
zapDatais set to2 ** 16 - 1(65535 bytes), which seems appropriate to prevent excessively large data payloads.
62-63: Initialize zapNative and zapData with default values in legacy bridge functionIn the legacy
bridgefunction, settingzapNativeto0andzapDatato an empty byte string ensures backward compatibility with existing implementations.
Line range hint
127-131: Handle native gas token refunds correctlyThe refund logic now checks against
NATIVE_GAS_TOKENto determine if the token is the native gas token and usesAddress.sendValueto refund accordingly. This ensures accurate handling of refunds for both ERC20 tokens and the native gas token.
147-148: Update getBridgeTransaction comments for V2 parametersThe comments accurately reflect how
zapNativeandzapDataare handled ingetBridgeTransaction, noting thatzapNativeis reported as a zero/non-zero flag andzapDatais ignored for backward compatibility.
164-164: Set sendChainGas flag based on zapNative in getBridgeTransactionThe
sendChainGasflag is correctly determined by checking ifzapNativeis non-zero, aligning with the updated logic for zap functionality.
213-214: Include zapNative and zapData in bridge transaction encodingThe parameters
zapNativeandzapDataare properly included in theBridgeTransactionV2Lib.encodeV2call, ensuring they are transmitted and processed correctly in the bridging process.
229-229: Set sendChainGas flag in BridgeRequested eventThe
sendChainGasparameter in theBridgeRequestedevent is correctly set based on whetherzapNativeis non-zero, providing accurate event data for listeners.
247-247: Retrieve zapNative from request in relay functionExtracting
zapNativefrom the request ensures that the relay function has access to the necessary data to process the zap on the destination chain.
264-275: Handle zapNative and msg.value correctly in relay functionThe relay function correctly differentiates between native gas tokens and ERC20 tokens, enforcing that
zapNativeis zero when dealing with the native gas token and checkingmsg.valueappropriately. This prevents incorrect usage and potential errors.
285-296: Process zapData and msg.value appropriatelyThe logic ensures that when
zapDatais present, the_triggerZapWithChecksfunction is called to handle the zap. IfzapDatais absent butmsg.valueis non-zero, the value is transferred directly to the recipient. This correctly accommodates both scenarios.
379-380: Verify msg.value for native gas token in _takeBridgedUserAssetThe function correctly checks that
msg.valuematches theamountspecified when the token is the native gas token, ensuring accurate asset transfer.
396-411: Return data validation in _triggerZapWithChecks remains unaddressedThe issue raised in the previous review about relying on
returnDatalength and contents for validation in_triggerZapWithChecksstill stands. It's recommended to modify theIZapRecipient.zapinterface to return aboolindicating success and adjust the validation accordingly.
447-449: Validate zapData length and disallow zapNative with native gas tokenThe added checks ensure that
zapDatadoes not exceedMAX_ZAP_DATA_LENGTHand thatzapNativeis not used whendestTokenis the native gas token. This prevents unsupported configurations and potential errors.
| /// @notice Address reserved for native gas token (ETH on Ethereum and most L2s, AVAX on Avalanche, etc) | ||
| address public constant NATIVE_GAS_TOKEN = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE; |
There was a problem hiding this comment.
💡 Codebase verification
Remaining references to UniversalTokenLib.ETH_ADDRESS detected. Please ensure all instances are updated to use NATIVE_GAS_TOKEN.
🔗 Analysis chain
Define NATIVE_GAS_TOKEN constant appropriately
The constant NATIVE_GAS_TOKEN is defined to represent the native gas token (e.g., ETH, AVAX) across different networks. This enhances clarity and replaces previous references to UniversalTokenLib.ETH_ADDRESS.
To ensure all references to UniversalTokenLib.ETH_ADDRESS have been updated, you can run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to `UniversalTokenLib.ETH_ADDRESS`
rg 'UniversalTokenLib\.ETH_ADDRESS' --type solidity
Length of output: 86
Script:
#!/bin/bash
# Description: Check for any remaining references to `UniversalTokenLib.ETH_ADDRESS` in Solidity files
rg 'UniversalTokenLib\.ETH_ADDRESS' --glob '*.sol'
Length of output: 5843
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (1)
11-13: LGTM! Consider minor naming consistency improvement.The new error declarations align well with the PR objectives of introducing Zap functionality and updating terminology. The changes are appropriate and consistent with the overall refactoring described in the PR summary.
For improved naming consistency, consider renaming
TokenNotContracttoTokenNotContractAddressto match the verbosity of other error names in the interface.packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1)
258-262: Excellent addition: New test enhances error handling.This new test function effectively validates that the contract correctly handles cases where the
originTokenis not a contract address. It's a valuable addition that improves the robustness of the contract by ensuring proper input validation.Some suggestions to consider:
- You might want to add similar tests for other address parameters (e.g.,
destToken) to ensure comprehensive validation.- Consider adding a positive test case where
originTokenis a valid contract address to ensure the check doesn't incorrectly reject valid inputs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (13 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.FBRV2.t.sol (0 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (3 hunks)
💤 Files with no reviewable changes (1)
- packages/contracts-rfq/test/FastBridgeV2.Src.FBRV2.t.sol
🧰 Additional context used
📓 Learnings (1)
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (4)
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol:97-97 Timestamp: 2024-10-14T13:23:04.509Z Learning: In `packages/contracts-rfq/test/FastBridgeV2.t.sol`, when testing expected revert behaviors, `abi.encodeWithSelector` is used with custom errors and should not be replaced with `BridgeTransactionV2Lib.encodeV2`.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.Src.t.sol:919-993 Timestamp: 2024-10-14T14:48:01.520Z Learning: In Solidity test files for FastBridgeV2 (e.g., `packages/contracts-rfq/test/FastBridgeV2.Src.t.sol`), code duplication in test functions is acceptable to maintain readability and maintainability, even if it contradicts DRY principles.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56 Timestamp: 2024-10-14T13:24:45.293Z Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118 Timestamp: 2024-10-14T14:45:57.214Z Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
🔇 Additional comments (6)
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1)
48-48: LGTM: Updated expectation aligns with new zap functionality.The change from
bridgeTx.callValuetobridgeTx.zapNativein thesendChainGasparameter check is consistent with the PR objectives. This update correctly reflects the new zap functionality introduced in theFastBridgeV2contract.packages/contracts-rfq/contracts/FastBridgeV2.sol (5)
60-61: Confirm Default Parameters in OverloadedbridgeFunction.In the overloaded
bridgefunction,zapNativeandzapDataare set to default values of0andbytes("")respectively. Ensure that these defaults correctly handle scenarios where zap functionality is not required and do not introduce unintended behavior.
Line range hint
246-265: ReviewzapNativeRestrictions inrelayFunction.The
relayfunction reverts ifzapNativeis not zero when dealing with the native gas token:if (zapNative != 0) revert ZapNativeNotSupported();Verify that this restriction aligns with the business logic and that no valid use cases are being inadvertently blocked.
404-415: Note: Previous Issue Regarding Return Data Validation Remains Unresolved.The method
_triggerZapWithChecksstill checks the return data length and value in a way that may not reliably confirm successful execution:if (returnData.length != 32) revert RecipientIncorrectReturnValue(); if (bytes32(returnData) != bytes32(IZapRecipient.zap.selector)) { revert RecipientIncorrectReturnValue(); }As previously suggested, consider updating
IZapRecipient.zapto return a boolean value indicating success and decode the return data accordingly.
128-129: 🛠️ Refactor suggestionVerify Security in
refundFunction Token Transfers.When refunding users, the function handles both native gas tokens and ERC20 tokens. Confirm that:
- The use of
Address.sendValuefor native tokens is safe against reentrancy attacks.- The
safeTransfermethod is appropriate for ERC20 tokens.Consider implementing reentrancy guards if necessary.
19-21: 💡 Codebase verificationInconsistent Replacement of
ETH_ADDRESSwithNATIVE_GAS_TOKENDetectedThe following files still contain references to
ETH_ADDRESSand need to be updated toNATIVE_GAS_TOKEN:
services/explorer/contracts/contracts/SynapseRFQV2_flat.solservices/explorer/contracts/contracts/SynapseRFQV1_flat.solservices/explorer/contracts/contracts/testcontracts/TestSynapseBridgeV1.solservices/explorer/contracts/contracts/testcontracts/TestSynapseBridgeV2.solpackages/contracts-rfq/test/FastBridgeV2.t.solpackages/contracts-rfq/test/FastBridgeV2.Src.t.solpackages/contracts-rfq/test/FastBridgeV2.Dst.t.solpackages/contracts-rfq/test/integration/MulticallTarget.t.solpackages/contracts-rfq/test/UniversalTokenLibHarness.solpackages/contracts-rfq/test/UniversalTokenLib.t.solpackages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.solpackages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.solpackages/contracts-rfq/test/FastBridgeV2.Src.Base.t.solpackages/contracts-rfq/test/FastBridgeV2.Management.t.solpackages/contracts-rfq/test/FastBridge.t.solpackages/contracts-rfq/contracts/libs/UniversalToken.solpackages/contracts-rfq/contracts/legacy/router/DefaultRouter.solpackages/contracts-rfq/contracts/legacy/rfq/FastBridgeRouterV2.solpackages/contracts-rfq/contracts/legacy/router/libs/UniversalToken.solpackages/contracts-rfq/contracts/FastBridge.solpackages/contracts-rfq/contracts/legacy/rfq/interfaces/IFastBridgeRouter.solpackages/contracts-rfq/contracts/legacy/router/interfaces/IRouterAdapter.solpackages/contracts-rfq/contracts/legacy/router/adapters/DefaultAdapter.sol🔗 Analysis chain
Ensure Consistent Replacement of
ETH_ADDRESSwithNATIVE_GAS_TOKEN.The introduction of
NATIVE_GAS_TOKENis meant to replaceETH_ADDRESSto prevent confusion on networks where ETH is not the native gas token. Please verify that all instances ofETH_ADDRESShave been updated throughout the codebase to maintain consistency.You can run the following script to check for any remaining references:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find any remaining references to `ETH_ADDRESS` in Solidity files. rg 'ETH_ADDRESS' --glob '*.sol'Length of output: 18591
⛔ Skipped due to learnings
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118 Timestamp: 2024-10-14T14:45:57.214Z Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Description
This PR updates the terminology in
FastBridgeV2:NATIVE_GAS_TOKENis used instead ofETH_ADDRESSto avoid confusion on chains where ETH is not a native gas token.zapNativeis used to signal the amount of additional native gas token requested by the recipient.zapDatais used to signal the data to be passed to the recipient, when a Zap is triggered on the destination chain.IZapRecipientis used instead ofIFastBridgeRecipient.sanguine/packages/contracts-rfq/contracts/interfaces/IZapRecipient.sol
Line 5 in f8665f5
Additional context
BridgeTransaction, tight packing [SLT-328] [SLT-273] #3284 to be merged first.Summary by CodeRabbit
New Features
zapfunctionality for enhanced bridge transactions.IZapRecipientfor improved contract interactions.Bug Fixes
Documentation
Tests