Skip to content
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

Merged
merged 36 commits into from
Oct 1, 2024

Conversation

ChiTimesChi
Copy link
Collaborator

@ChiTimesChi ChiTimesChi commented Sep 26, 2024

Description
User should be able to provide new fields for the bridge() function (field names are tentative):

  1. quoteId - Optional - sent on Deposit, emitted on BridgeRequested. Not included in request bytes. Used for off-chain functionality and tracking only.
  2. quoteRelayer - Optional - sent on Deposit, emitted, included in request bytes, enforced by relay function
  3. quoteRelayerSeconds - Optional - sent on Deposit, generates a timestamp deadline (block ts + X) which is emitted and included in request bytes. Enforced by relay function.

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

    • Introduced a new FastBridgeV2 contract with enhanced transaction handling capabilities.
    • Added support for exclusivity parameters in bridge transactions, improving transaction management.
    • New event BridgeQuoteDetails to log bridge quote details with unique identifiers.
  • Bug Fixes

    • Improved validation logic for exclusivity parameters and periods, ensuring better user feedback.
  • Tests

    • Comprehensive testing for the new transaction structure and exclusivity logic in both token and ETH relaying scenarios.
    • Updated tests to verify the emission of new events and the correct handling of transaction parameters.

Copy link
Contributor

coderabbitai bot commented Sep 26, 2024

Walkthrough

The pull request introduces updates to the FastBridgeV2 contract and its associated interfaces, enhancing transaction handling with new structures for exclusivity parameters. Key changes include the addition of a new interface IFastBridgeV2, modifications to existing function signatures, and updates to the testing framework to accommodate the new transaction structures. These changes refine the bridge mechanism, ensuring improved management of transaction exclusivity and validation processes.

Changes

Files Change Summary
packages/contracts-rfq/contracts/FastBridgeV2.sol Added getBridgeTransactionV2 function, overloaded bridge function with new parameters, modified transaction request structure to include exclusivity fields, and updated relay function logic.
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol Introduced BridgeParamsV2 and BridgeTransactionV2 structs, added BridgeQuoteDetails event, and updated bridge function signature.
packages/contracts-rfq/test/FastBridgeV2.t.sol Updated tests to transition from IFastBridge to IFastBridgeV2, modified function signatures to use BridgeTransactionV2, and added new tests for exclusivity handling.
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol Updated tests to reflect changes in transaction handling and added new tests for exclusivity scenarios in bridging tokens and ETH.
packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol Implemented exclusivity parameters in tests for both token and ETH transactions, ensuring correct handling of exclusivity conditions and reverts.
packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol Added tests to validate negative scenarios for exclusivity parameters, checking for expected reverts under incorrect conditions.

Possibly related PRs

Suggested labels

javascript, Sol, Typescript

Suggested reviewers

  • trajan0x
  • abtestingalpha

🐰 In the meadow, where bunnies play,
A bridge was built, brightening the day.
With exclusivity and new paths to roam,
Tokens and ETH now find their home.
So hop along, let the relayers cheer,
For FastBridgeV2 is finally here! 🌟


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

github-actions bot commented Sep 26, 2024

Changes to gas cost

Generated at commit: 7acdf381c46fc911287304b67092a609387d62a6, compared to commit: eead134ff4d19554413efdab732c38e9d90fe763

🧾 Summary (50% most significant diffs)

Contract Method Avg (+/-) %
FastBridgeV2 bridgeProofs
claim(bytes)
claim(bytes,address)
prove(bytes,bytes32)
relay(bytes)
+22 ❌
+524 ❌
+634 ❌
+337 ❌
+664 ❌
+3.65%
+1.05%
+1.23%
+0.97%
+0.94%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
FastBridgeV2 2,673,824 (+294,021) RELAYER_ROLE
bridgeProofs
claim(bytes)
claim(bytes,address)
dispute
prove(bytes,bytes32)
prove(bytes32,bytes32,address)
refund
relay(bytes)
relay(bytes,address)
307 (+22)
624 (+22)
42,732 (+524)
45,695 (+634)
31,284 (+34)
35,067 (+338)
32,140 (+12)
46,492 (+435)
65,594 (+664)
66,011 (+664)
+7.72%
+3.65%
+1.24%
+1.41%
+0.11%
+0.97%
+0.04%
+0.94%
+1.02%
+1.02%
307 (+22)
624 (+22)
50,266 (+524)
51,979 (+634)
31,284 (+25)
35,094 (+337)
32,140 (+1)
50,198 (+435)
71,489 (+664)
71,906 (+664)
+7.72%
+3.65%
+1.05%
+1.23%
+0.08%
+0.97%
+0.00%
+0.87%
+0.94%
+0.93%
307 (+22)
624 (+22)
50,275 (+524)
51,988 (+634)
31,284 (+22)
35,097 (+338)
32,140 (0)
50,207 (+435)
71,489 (+664)
71,906 (+664)
+7.72%
+3.65%
+1.05%
+1.23%
+0.07%
+0.97%
0.00%
+0.87%
+0.94%
+0.93%
307 (+22)
624 (+22)
57,782 (+524)
58,245 (+634)
31,284 (+22)
35,211 (+338)
32,140 (0)
53,886 (+435)
77,385 (+664)
77,802 (+664)
+7.72%
+3.65%
+0.92%
+1.10%
+0.07%
+0.97%
0.00%
+0.81%
+0.87%
+0.86%
72 (+8)
8 (0)
4 (0)
4 (0)
4 (0)
40 (+4)
40 (+4)
8 (0)
2 (0)
2 (0)

Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.35088%. Comparing base (f0b13bc) to head (ad5bd73).
Report is 21 commits behind head on master.

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     
Flag Coverage Δ
cctp-relayer ?
core ?
ethergo ?
git-changes-action ?
omnirpc ?
opbot ?
packages 90.56974% <ø> (ø)
screener-api ?
scribe ?
solidity 89.40678% <100.00000%> (-5.29995%) ⬇️
tools ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

cloudflare-workers-and-pages bot commented Sep 26, 2024

Deploying sanguine-fe with  Cloudflare Pages  Cloudflare Pages

Latest commit: ad5bd73
Status:🚫  Build failed.

View logs

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 and ExclusivityPeriodNotPassed, 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 that FastBridgeV2 is no longer fully backwards compatible with FastBridge. This prevents these tests from running, which is appropriate given the compatibility issues.

Consider adding more detailed documentation about the specific incompatibilities between FastBridgeV2 and FastBridge. 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:

  1. test_failedRelayNotRelayer is skipped, indicating that the relay function is no longer permissioned.
  2. test_failedClaimNotRelayer is skipped, suggesting that the claim function is no longer permissioned by role.
  3. 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 changes

The modifications to FastBridgeV2ParityTest effectively address the backwards incompatibility issues between FastBridgeV2 and FastBridge:

  1. Making the contract abstract prevents unintended test execution.
  2. Skipping and modifying specific tests reflect changes in the permission model.
  3. 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:

  1. Consider creating a new test suite specifically for FastBridgeV2 that covers its unique features and behavior.
  2. Update the main FastBridgeV2 contract documentation to clearly outline the breaking changes and new features compared to FastBridge.
  3. 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 scheme

There'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 encapsulation

The state variables tokenTx and ethTx are declared as public. If external access to these variables is not required, consider changing their visibility to internal or private to enhance encapsulation and reduce potential security risks.


41-42: Align visibility of new parameters with best practices

Similarly, tokenParamsV2 and ethParamsV2 are declared as public. If these parameters are only used within the contract or by inheriting contracts, consider setting their visibility to internal or private for consistency and improved security.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between edf99d3 and 98d4539.

📒 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 context

The 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:

  1. The changes are well-implemented and consistent across both token and ETH transactions.
  2. The overall behavior of the test contract remains unchanged.
  3. 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:

  1. Provide additional context on the txV1 structure and its purpose.
  2. Clarify how these changes contribute to the relayer exclusivity feature.
  3. 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 both tokenTx and ethTx 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:

  1. Verify that all other test files and main contract files have been updated to use this new structure.
  2. Check if documentation needs to be updated to reflect this change in transaction structure.
  3. 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 exclusivity

The 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:

  1. Could you provide more context on the purpose of the txV1 structure? Is this related to a new version of the transaction format?
  2. Are there any implications for backwards compatibility with existing systems or contracts?
  3. 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 of EXCLUSIVITY_PERIOD Enhances Readability

The use of 60 seconds for EXCLUSIVITY_PERIOD improves code readability by explicitly specifying the time unit.


10-22: createFixturesV2 Correctly Initializes Exclusivity Parameters

The createFixturesV2 function properly overrides the base function and initializes the exclusivity parameters for both token and ETH transactions. Assigning quoteRelayer, quoteExclusivitySeconds, and quoteId ensures that the tests accurately reflect the intended behavior.


24-27: Overloaded bridge Function Appropriately Selects Parameters

The overloaded bridge function effectively determines the correct set of parameters (ethParamsV2 or tokenParamsV2) based on the originToken. This ensures that the bridging logic uses the appropriate configuration for each token type.


29-39: Maintains Caller Context Using vm.prank

Utilizing vm.prank(caller) preserves the caller's context during the fastBridge.bridge invocation. This is essential for accurately simulating transaction conditions and testing access control within the contract.


41-63: Comprehensive Revert Tests for Exclusivity Parameters

The 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 and quoteExclusivitySeconds must be set appropriately.

packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2)

42-42: Verify backward compatibility of the updated bridge function

The bridge function signature now includes an additional parameter BridgeParamsV2 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 function getBridgeTransactionV2 added

The addition of the getBridgeTransactionV2 function enhances the interface by allowing decoding of bridge requests into BridgeTransactionV2 structs effectively.

packages/contracts-rfq/test/FastBridgeV2.t.sol (4)

5-5: Addition of IFastBridgeV2 import is appropriate

The new import statement correctly includes the IFastBridgeV2 interface, which is necessary for integrating the updated bridge functionality into the test contract.


48-48: Ensure createFixturesV2() initializes all necessary fields

After invoking createFixturesV2(), verify that all the required fields in tokenTx and ethTx 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 to txV1 in tokenTx and ethTx

Assigning IFastBridge.BridgeTransaction to tokenTx.txV1 and ethTx.txV1 should be compatible with the BridgeTransactionV2 struct. Ensure that the txV1 field exists within BridgeTransactionV2 and that this assignment aligns with the updated contract interfaces to prevent type mismatches.


128-130: Verification of transaction ID hashing method

The getTxId function correctly computes the transaction ID using keccak256(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 use IFastBridgeV2

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 both relayerA and relayerB in mintTokens

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 for relayerB

Adding an approval for relayerB ensures that fastBridge can spend tokens on behalf of both relayers, aligning with the tokens minted earlier.


42-48: Update expectBridgeRelayed function to accept BridgeTransactionV2

The function signature now accepts IFastBridgeV2.BridgeTransactionV2, matching the updated interface. This change is consistent and necessary for the new transaction structure.


63-63: Update relay function to use BridgeTransactionV2

The relay function now accepts IFastBridgeV2.BridgeTransactionV2, aligning it with the new interface. This update ensures that the function handles the new transaction data structure.


73-73: Update relayWithAddress function to use BridgeTransactionV2

Similarly, relayWithAddress now accepts BridgeTransactionV2. This change is consistent with the interface updates and maintains compatibility.


82-88: Introduce checkRelayedViews for enhanced test assertions

The new checkRelayedViews function encapsulates checks for relayed transactions, improving modularity and readability of your tests.


93-97: Enhance test_relay_token with additional checks

Including 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 in test_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 issue

Verify caller and relayer handling in ETH relay tests

In test_relay_eth_withRelayerAddress, relayerA calls relayWithAddress with relayerB 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 issue

Potential issue with mismatched caller and relayer in relayWithAddress

In test_relay_token_withRelayerAddress, relayerB calls relayWithAddress using relayerA 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 the relayer 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 using bridgeTx.txV1

You're accessing properties through bridgeTx.txV1. Verify that txV1 is correctly defined within BridgeTransactionV2 and that all the accessed fields are properly mapped.

Run the following script to confirm that BridgeTransactionV2 contains txV1 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 BridgeTransactionV2

Length 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/**/*.sol

Length of output: 2270

packages/contracts-rfq/contracts/FastBridgeV2.sol (7)

78-81: Addition of getBridgeTransactionV2 function is appropriate

The new getBridgeTransactionV2 function correctly decodes a BridgeTransactionV2 from the provided bytes request. This addition aligns with the introduction of the new transaction structure and maintains code clarity.


85-88: Ensures backward compatibility by overloading bridge function

The bridge function is appropriately overloaded to accept the new BridgeParamsV2. By initializing default values for BridgeParamsV2 when they are not provided, it preserves backward compatibility with existing implementations.


100-103: Validation of exclusivity parameters is correctly implemented

The condition ensures that quoteRelayer and quoteExclusivitySeconds 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 of exclusivityEndTime is appropriate

The 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 of BridgeTransactionV2 with nested txV1

The encapsulation of BridgeTransaction as txV1 within BridgeTransactionV2 effectively extends the existing transaction structure to include exclusivity parameters. This approach maintains compatibility while adding new functionality.


169-176: Exclusivity checks in relay function are correctly implemented

The added logic in the relay function enforces the exclusivity period effectively. It ensures that during the exclusivity window, only the specified exclusivityRelayer can relay the transaction, thereby honoring exclusivity agreements.


152-152: Ensure BridgeRequestedV2 event is defined and documented

The emit BridgeRequestedV2 statement emits a new event with transactionId and paramsV2.quoteId. Verify that the BridgeRequestedV2 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 correctly

The addition of IFastBridgeV2 ensures access to the new interface required for updated function signatures.


30-30: Addition of BridgeRequestedV2 event is appropriate

The new event BridgeRequestedV2 correctly captures transactionId and quoteId. Since quoteId is of type bytes, it cannot be indexed, which is acceptable for variable-length data.


75-77: Explicit visibility and mutability specifiers added

The bridge function now explicitly declares its visibility as public and is marked virtual to allow overriding in derived contracts, enhancing extensibility.


85-87: Updated prove function to use BridgeTransactionV2

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: Updated claim function to use BridgeTransactionV2

The function now accepts IFastBridgeV2.BridgeTransactionV2, maintaining consistency with the updated interface.


95-97: Updated overloaded claim function to use BridgeTransactionV2

The overloaded claim function is appropriately updated to accept IFastBridgeV2.BridgeTransactionV2, ensuring consistency across function implementations.


105-107: Updated refund function to use BridgeTransactionV2

The refund function signature now accepts IFastBridgeV2.BridgeTransactionV2, aligning with the updated transaction structure for refunds.


110-121: Updated expectBridgeRequested to use nested txV1 fields in BridgeTransactionV2

The function correctly adapts to the new BridgeTransactionV2 structure by accessing the required fields from the nested txV1 member to emit the BridgeRequested event.


148-149: Updated expectBridgeDepositClaimed to use bridgeTx.txV1 fields

The function correctly retrieves originToken and originAmount from bridgeTx.txV1, aligning with the updated BridgeTransactionV2 structure.


499-502: Updated balance assertions to use tokenTx.txV1 fields

The assertions now reference tokenTx.txV1.originFeeAmount and tokenTx.txV1.originAmount, reflecting the updated transaction structure.


565-568: Updated ETH balance assertions to use ethTx.txV1 fields

The assertions correctly reference ethTx.txV1.originFeeAmount and ethTx.txV1.originAmount, ensuring consistency with the new transaction structure.


864-864: Verify usage of ethParams instead of ethParamsV2

The assertion uses ethParams.originAmount. Given the update to BridgeTransactionV2, please verify that ethParams contains values consistent with the new transaction structure. If necessary, update to ethParamsV2 to ensure accuracy.


878-879: Potential inconsistency in balance calculations

The balance assertions use ethParams.originAmount. Verify that these values are accurate in light of the updated transaction parameters, and ensure consistent usage of ethParamsV2 if appropriate.


892-892: Verify usage of ethParams instead of ethParamsV2

The assertion references ethParams.originAmount. Confirm that ethParams is aligned with the updated transaction structure, and update to ethParamsV2 if necessary.


906-906: Verify usage of ethParams instead of ethParamsV2

In this assertion, ethParams.originAmount is used. Please ensure that ethParams reflects the values from the updated BridgeTransactionV2 structure. Consider using ethParamsV2 if needed.

Copy link

codecov bot commented Sep 26, 2024

Bundle Report

Changes will increase total bundle size by 407.1kB (1.14%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
sdk-router-@synapsecns/sdk-router-cjs 526.74kB 409.5kB (349.29%) ⬆️
explorer-ui-server-cjs* 865.43kB 639 bytes (-0.07%) ⬇️
synapse-interface-client-array-push* 7.28MB 147 bytes (0.0%) ⬆️
synapse-interface-server-cjs 1.49MB 127 bytes (0.01%) ⬆️
widget-cjs-esm* 271.27kB 2.03kB (-0.74%) ⬇️

ℹ️ *Bundle size includes cached data from a previous commit

@ChiTimesChi
Copy link
Collaborator Author

Gas diffs will be much closer to the true values once #3204 is merged

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 new BridgeTransactionV2 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 new BridgeTransactionV2 structure across different functions.

Recommendations for future iterations:

  1. Address the TODO comments about reducing cyclomatic complexity, particularly in the bridge and relay functions.
  2. Consider breaking down larger functions into smaller, more manageable pieces to improve readability and maintainability.
  3. Ensure comprehensive testing of the new exclusivity features and their interaction with existing functionality.
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between d6e76ca and ad5bd73.

📒 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 through BridgeParamsV2. 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 with BridgeTransactionV2 and implement the new exclusivity feature. The use of bridgeRelays instead of directly accessing bridgeRelayDetails is a good practice for code organization.


191-191: LGTM: Improved encapsulation in relay check.

The use of bridgeRelays(transactionId) instead of directly accessing bridgeRelayDetails 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 new BridgeTransactionV2 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 use getBridgeTransactionV2, 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 new BridgeTransactionV2 structure. It's consistent with the existing getBridgeTransaction 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 in quoteExclusivitySeconds.

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 past exclusivityEndTime.

Setting exclusivityEndTime to a past timestamp using block.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), assigning tokenParamsV2.quoteExclusivitySeconds = -int256(block.timestamp) may cause integer underflow, resulting in a large positive value. Confirm that this value triggers the expected ExclusivityParamsIncorrect 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 the bridge 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.

Comment on lines +36 to +38
tokenParamsV2.quoteExclusivitySeconds = -int256(block.timestamp + 1);
vm.expectRevert(ExclusivityParamsIncorrect.selector);
bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Comment on lines +42 to +44
ethParamsV2.quoteExclusivitySeconds = -int256(block.timestamp);
vm.expectRevert(ExclusivityParamsIncorrect.selector);
bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Copy link
Collaborator

@parodime parodime left a 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

@ChiTimesChi ChiTimesChi merged commit eb3db7c into master Oct 1, 2024
51 of 52 checks passed
@ChiTimesChi ChiTimesChi deleted the feat/FbV2-relayer-exclusivity branch October 1, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants