status/proof mapping packing [SLT-186] #3173
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Outside diff range and nitpick comments (2)
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (2)
312-314: LGTM: bridgeProofs access updated correctly.The
bridgeProofsaccess has been properly updated to use a struct-like syntax, which is consistent with the PR objectives of consolidating mappings. This change improves the code structure and readability.Consider using destructuring assignment for better readability:
-(uint96 timestamp, address relayer) = (fastBridge.bridgeProofs(txId).timestamp, fastBridge.bridgeProofs(txId).relayer); +var (timestamp, relayer) = (fastBridge.bridgeProofs(txId).timestamp, fastBridge.bridgeProofs(txId).relayer);
326-328: LGTM: bridgeProofs access updated correctly for eth test.The
bridgeProofsaccess has been properly updated to use a struct-like syntax in the eth test, which is consistent with the previous update for token tests and the PR objectives of consolidating mappings.Consider using destructuring assignment for better readability:
-(uint96 timestamp, address relayer) = (fastBridge.bridgeProofs(txId).timestamp, fastBridge.bridgeProofs(txId).relayer); +var (timestamp, relayer) = (fastBridge.bridgeProofs(txId).timestamp, fastBridge.bridgeProofs(txId).relayer);
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (9 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (30 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (1 hunks)
Additional comments not posted (24)
packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
5-5: Approve new import, but verify its usageThe addition of the
IFastBridgeV2import is appropriate, likely reflecting updates to the FastBridge contract. However, I noticed that the test file doesn't explicitly use this new interface yet.To ensure proper test coverage for the new interface:
- Verify if any test cases need updating to use
IFastBridgeV2.- Check if new test cases are required for any new functionality in
IFastBridgeV2.Run the following script to check for usage of
IFastBridgeV2in this test file:packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (18)
17-17: LGTM: Import statement updated correctly.The addition of
IFastBridgeV2to the import statement is consistent with the changes in the contract, allowing the use ofIFastBridgeV2.BridgeStatusinstead ofFastBridgeV2.BridgeStatus.
170-172: LGTM: assertEq function signature updated correctly.The
assertEqfunction signature has been properly updated to useIFastBridgeV2.BridgeStatusinstead ofFastBridgeV2.BridgeStatus. This change is consistent with the updated import statement and ensures that the test cases use the correctBridgeStatusenum from the interface.
186-186: LGTM: assertEq call updated correctly.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.REQUESTED. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum.
194-194: LGTM: assertEq call updated correctly.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.REQUESTED. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum.
211-211: LGTM: assertEq call updated correctly for eth test.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.REQUESTEDin the eth test. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum for eth transactions.
221-221: LGTM: assertEq call updated correctly for eth test.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.REQUESTEDin the eth test. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum for eth transactions.
236-236: LGTM: assertEq call updated correctly for user-specific nonce test.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.REQUESTEDin the user-specific nonce test. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum for user-specific nonce transactions.
385-385: LGTM: assertEq call updated correctly for claim token test.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.RELAYER_CLAIMED. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum for claimed transactions.
397-397: LGTM: assertEq call updated correctly for permissionless claim token test.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.RELAYER_CLAIMED. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum for permissionless claimed transactions.
409-409: LGTM: assertEq call updated correctly for permissionless claim token test with zero address.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.RELAYER_CLAIMED. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum for permissionless claimed transactions with zero address.
420-420: LGTM: assertEq call updated correctly for claim token test with different address.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.RELAYER_CLAIMED. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum for claimed transactions with a different address.
432-432: LGTM: assertEq call updated correctly for claim token test with long delay.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.RELAYER_CLAIMED. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum for claimed transactions with a long delay.
451-451: LGTM: assertEq call updated correctly for claim eth test.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.RELAYER_CLAIMED. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum for claimed eth transactions.
464-464: LGTM: assertEq call updated correctly for permissionless claim eth test.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.RELAYER_CLAIMED. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum for permissionless claimed eth transactions.
477-477: LGTM: assertEq call updated correctly for permissionless claim eth test with zero address.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.RELAYER_CLAIMED. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum for permissionless claimed eth transactions with zero address.
489-489: LGTM: assertEq call updated correctly for claim eth test with different address.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.RELAYER_CLAIMED. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum for claimed eth transactions with a different address.
501-501: LGTM: assertEq call updated correctly for claim eth test with long delay.The
assertEqcall has been properly updated to useIFastBridgeV2.BridgeStatus.RELAYER_CLAIMED. This change is consistent with the previous updates and ensures that the test case checks for the correct bridge status using the interface enum for claimed eth transactions with a long delay.
581-581: LGTM: assertEq calls updated correctly for dispute tests.The
assertEqcalls have been properly updated to useIFastBridgeV2.BridgeStatus.REQUESTEDin the dispute tests. These changes are consistent with the previous updates and ensure that the test cases check for the correct bridge status using the interface enum for disputed transactions.Also applies to: 593-593, 605-605, 618-618
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2)
43-46:bridgeStatusesfunction correctly provides bridge transaction statusThe
bridgeStatusesfunction is appropriately defined to return theBridgeStatusfor a giventransactionId. This matches the intended functionality and complies with interface design.
48-51:bridgeProofsfunction correctly returns proof detailsThe
bridgeProofsfunction is correctly defined to return the timestamp and relayer address associated with a bridge transaction. This aligns with the intended interface functionality.packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
225-225: Consistent Status Verification in Claim FunctionIn the
claimfunction, you check if the status isRELAYER_PROVED. Ensure that no other statuses (e.g.,REFUNDED,DISPUTED) could lead to unintended access to this function.
253-254: Ensure Safe Time Calculations in Dispute FunctionIn the
disputefunction, the_timeSincecalculation might be susceptible to unexpected behavior if block timestamps are manipulated. Ensure that the function is secure against such scenarios.
257-258: Confirm Complete Reset of Proof DataWhen calling
delete bridgeTxDetails[transactionId].proof;, verify that all fields within theBridgeProofstruct are reset to their default values, and that no residual data remains.
| struct BridgeTxDetails { | ||
| BridgeStatus status; | ||
| BridgeProof proof; | ||
| } |
There was a problem hiding this comment.
Ensure BridgeProof type is properly defined or imported
The BridgeProof type used in the BridgeTxDetails struct is not defined within this interface. Please verify that BridgeProof is properly defined or imported from another contract or interface to prevent compilation errors.
| function bridgeStatuses(bytes32 transactionId) public view returns (BridgeStatus status) | ||
| { | ||
| return bridgeTxDetails[transactionId].status; | ||
| } |
There was a problem hiding this comment.
Optimize Getter Function Return Type
In the bridgeStatuses function, you're returning a BridgeStatus enum. While enums are convenient, returning them can be less gas-efficient. Consider returning a uint8 instead, which represents the underlying value of the enum and reduces gas consumption.
Consider this change:
-function bridgeStatuses(bytes32 transactionId) public view returns (BridgeStatus status)
+function bridgeStatuses(bytes32 transactionId) public view returns (uint8 status)
{
- return bridgeTxDetails[transactionId].status;
+ return uint8(bridgeTxDetails[transactionId].status);
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function bridgeStatuses(bytes32 transactionId) public view returns (BridgeStatus status) | |
| { | |
| return bridgeTxDetails[transactionId].status; | |
| } | |
| function bridgeStatuses(bytes32 transactionId) public view returns (uint8 status) | |
| { | |
| return uint8(bridgeTxDetails[transactionId].status); | |
| } |
| if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); | ||
| BridgeProof memory proof = bridgeTxDetails[transactionId].proof; |
There was a problem hiding this comment.
Check for Uninitialized Proof Before Access
When accessing bridgeTxDetails[transactionId].proof, ensure that the proof exists to prevent accessing uninitialized memory, which could lead to incorrect data retrieval.
Add a check for proof initialization:
BridgeProof memory proof = bridgeTxDetails[transactionId].proof;
+if (proof.relayer == address(0)) revert ProofNotInitialized();Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); | |
| BridgeProof memory proof = bridgeTxDetails[transactionId].proof; | |
| if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); | |
| BridgeProof memory proof = bridgeTxDetails[transactionId].proof; | |
| if (proof.relayer == address(0)) revert ProofNotInitialized(); |
| function bridgeProofs(bytes32 transactionId) public view returns (BridgeProof memory proof) | ||
| { | ||
| return bridgeTxDetails[transactionId].proof; | ||
| } |
There was a problem hiding this comment.
Avoid Returning Structs Directly from Public Functions
The bridgeProofs function returns a BridgeProof struct. Returning structs can be expensive in terms of gas and may expose internal implementation details. Instead, consider returning individual fields of the struct.
Refactor the function to return individual fields:
-function bridgeProofs(bytes32 transactionId) public view returns (BridgeProof memory proof)
+function bridgeProofs(bytes32 transactionId) public view returns (uint96 timestamp, address relayer)
{
- return bridgeTxDetails[transactionId].proof;
+ BridgeProof memory proof = bridgeTxDetails[transactionId].proof;
+ return (proof.timestamp, proof.relayer);
}Committable suggestion was skipped due to low confidence.
| if (bridgeTxDetails[transactionId].status != BridgeStatus.REQUESTED) revert StatusIncorrect(); | ||
| bridgeTxDetails[transactionId].status = BridgeStatus.RELAYER_PROVED; | ||
| bridgeTxDetails[transactionId].proof = BridgeProof({timestamp: uint96(block.timestamp), relayer: relayer}); // overflow ok |
There was a problem hiding this comment.
Tip
Codebase Verification
❗ Unexpected Status Transition Detected
The status transition from REQUESTED to REFUNDED is not defined in the expected state machine (REQUESTED → RELAYER_PROVED → RELAYER_CLAIMED). Please review the state transitions to ensure they adhere to the intended workflow.
Analysis chain
Confirm Proper Status Transition and Proof Assignment
Before updating the status to RELAYER_PROVED and assigning the proof, ensure that the transition from REQUESTED to RELAYER_PROVED is valid according to your state machine. This prevents invalid status transitions.
Run the following script to verify status transitions:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all status transitions in the code follow the defined state machine.
# Expected statuses in order: REQUESTED -> RELAYER_PROVED -> RELAYER_CLAIMED
# Search for all status updates
rg --type solidity 'bridgeTxDetails\[.*\].status = BridgeStatus\.\w+' -A 2
Length of output: 109
Script:
#!/bin/bash
# Description: Verify that all status transitions in the Solidity code follow the defined state machine.
# Expected statuses in order: REQUESTED -> RELAYER_PROVED -> RELAYER_CLAIMED
# Search for all status updates in .sol files
rg --glob '*.sol' 'bridgeTxDetails\[.*\].status\s*=\s*BridgeStatus\.\w+' -A 2
Length of output: 1552
Deploying sanguine-fe with
|
| Latest commit: |
78c84a8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://389f6a87.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-fbv2-storagepacking.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3173 +/- ##
===================================================
- Coverage 40.97804% 38.45965% -2.51839%
===================================================
Files 459 424 -35
Lines 25643 24462 -1181
Branches 343 148 -195
===================================================
- Hits 10508 9408 -1100
+ Misses 14383 14314 -69
+ Partials 752 740 -12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| struct BridgeTxDetails { | ||
| BridgeStatus status; | ||
| BridgeProof proof; | ||
| } |
There was a problem hiding this comment.
This will not have a desired effect of saving the gas on reads and writes, as the internal composition of the struct takes more than a single storage slot (256 bits). We don't export a gas report as of now, but in this case it's possible to generate using forge t --mc FastBridgeV2SrcTest --nmt revert --gas-report.
- Using
FastBridgeV2SrcTesttest contract gives us the overview for the source chain-related functions --nmt revertexcludes test functions having a word "revert" in the name, which makes the report more useful (revert cases in general consume less gas, and thus need to be filtered)
There was a problem hiding this comment.
This are the results when run on master, prior to this PR
There was a problem hiding this comment.
And this are the results after this PR changes:
There was a problem hiding this comment.
To gain the performance in gas consumption, the BridgeTxDetails needs to fit within 256 bits:
BridgeStatusenum is equivalent touint8address relayertakes 160 bits- which leaves up to 88 bits for the proof's timestamp
This is an opportunity to follow the suit of #3170 and also store the block number, when the prove happened. This could be useful for some of the Relayer implementations. This will require using up to 88 bits for both block number and timestamp, which is more than enough for practical purposes.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (9 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (30 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol
🔇 Additional comments not posted (5)
packages/contracts-rfq/contracts/FastBridgeV2.sol (5)
187-190: LGTM: Proper status check and proof updateThe changes in the
provefunction correctly update the status and proof details in thebridgeTxDetailsmapping. The status check ensures that only transactions in theREQUESTEDstate can be proved, maintaining the expected state machine flow.
209-210: LGTM: Correct status check and proof retrievalThe
canClaimfunction has been properly updated to use the newbridgeTxDetailsmapping. It correctly checks the status and retrieves the proof, maintaining the existing logic while using the new data structure.
226-226: LGTM: Proper status handling and proof retrieval in claim functionThe
claimfunction has been correctly updated to use the newbridgeTxDetailsmapping. It properly checks and updates the transaction status, and correctly retrieves the proof details. The changes maintain the existing logic while utilizing the new data structure effectively.Also applies to: 228-228, 239-239
27-27: 🛠️ Refactor suggestionConsider changing
bridgeTxDetailsvisibility to privateThe
bridgeTxDetailsmapping is declared as public, which allows direct access to the internal data structure. To improve encapsulation and control access to this data, consider changing the visibility to private and using the newly added getter functionsbridgeStatusesandbridgeProofsfor external access.Apply this change:
- mapping(bytes32 => BridgeTxDetails) public bridgeTxDetails; + mapping(bytes32 => BridgeTxDetails) private bridgeTxDetails;Likely invalid or redundant comment.
113-113:⚠️ Potential issueEnsure transaction ID uniqueness before setting status
When setting the status to
REQUESTED, it's important to verify that thetransactionIdis unique and hasn't been used in a previous transaction. This prevents overwriting existing transaction details.Consider adding a check:
+if (bridgeTxDetails[transactionId].status != BridgeStatus.NONE) revert TransactionAlreadyExists(); bridgeTxDetails[transactionId].status = BridgeStatus.REQUESTED;Don't forget to define the
TransactionAlreadyExistserror in yourErrors.solfile.Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol (3)
15-28: LGTM! Consider adding a comment for clarity.The
assertCorrectProoffunction is well-implemented and consistent with the stated purpose. It correctly retrieves and verifies the proof details.Consider adding a brief comment explaining the purpose of this function, e.g.:
/// @dev Verifies that the proof for a given transaction matches the expected timestamp and relayer
Line range hint
30-38: Skipped tests are well-documented. Consider future test updates.The decision to skip
test_failedRelayNotRelayerandtest_failedClaimNotRelayeris well-justified and clearly explained in the comments.While skipping these tests is appropriate for now, consider replacing them with new tests that verify the updated permission model in FastBridgeV2. This will ensure that the new behavior is properly tested.
Line range hint
40-55: Modified test correctly reflects updated permission model.The
test_failedClaimNotOldRelayerfunction has been appropriately updated to align with the new permission model in FastBridgeV2.To improve clarity, consider renaming the test function to better reflect its current purpose, e.g.,
test_failedClaimNotProvenRelayer. This would more accurately describe what the test is now verifying.packages/contracts-rfq/test/FastBridge.t.sol (2)
1725-1725: Good use ofassertCorrectProofin dispute test!The replacement of direct assertions with
assertCorrectProofin this dispute test case maintains consistency with other changes and improves readability.Consider adding a comment or updating the test name to clarify that this assertion is checking for reset proof details after a successful dispute. For example:
- assertCorrectProof(transactionId, 0, address(0)); + // Assert that the proof details are reset after a successful dispute + assertCorrectProof(transactionId, 0, address(0));
1749-1749: Consistent use ofassertCorrectProofin overflow dispute test!The use of
assertCorrectProofin this overflow dispute test case maintains consistency with other changes and improves the overall structure of the test suite.Similar to the previous suggestion, consider adding a comment to clarify that this assertion is checking for reset proof details after a successful dispute in an overflow scenario. For example:
- assertCorrectProof(transactionId, 0, address(0)); + // Assert that the proof details are reset after a successful dispute, even in an overflow scenario + assertCorrectProof(transactionId, 0, address(0));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/contracts-rfq/test/FastBridge.t.sol (7 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol (1 hunks)
🔇 Additional comments not posted (5)
packages/contracts-rfq/test/FastBridge.t.sol (5)
55-66: Great addition of theassertCorrectProofhelper function!This new function encapsulates the logic for asserting the correctness of bridge transaction proofs, which will help reduce code duplication and improve the maintainability of the test suite. It's a good practice to create such helper functions for commonly used assertions.
1366-1366: Good use of the newassertCorrectProoffunction!Replacing the direct assertions with a call to
assertCorrectProofimproves the readability and consistency of the test case. This change will make it easier to maintain and update these assertions in the future if needed.
1398-1398: Consistent use ofassertCorrectProofin overflow test!The replacement of direct assertions with
assertCorrectProofin this overflow test case maintains consistency with other similar changes. This consistency improves the overall structure and readability of the test suite.
1427-1427: Consistent application ofassertCorrectProofin delay test!The use of
assertCorrectProofin this test case with a huge delay maintains the consistent structure across different scenarios. This consistency in test structure will make it easier for developers to understand and maintain the test suite in the future.
Line range hint
1-1749: Overall improvement in test structure and readability!The introduction of the
assertCorrectProofhelper function and its consistent application across various test cases significantly enhances the structure and readability of the test suite. This refactoring reduces code duplication and improves maintainability, making it easier for developers to understand and update the tests in the future.The consistent use of this new function across different scenarios (normal operations, overflow conditions, and disputes) ensures a uniform approach to asserting proof correctness throughout the test suite.
These changes represent a positive step in improving the quality and maintainability of the test code for the FastBridge contract.
|
gas report after BridgeTxDetails refactor |
Bundle ReportChanges will decrease total bundle size by 157.85kB (-0.44%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
Line range hint
225-249: Emit event for status change toRELAYER_CLAIMEDAfter updating the status to
RELAYER_CLAIMED, consider emitting an event to log this state change. This improves transparency and facilitates off-chain tracking of transaction statuses.Add an event emission:
_thisBridgeTxDetails.status = BridgeStatus.RELAYER_CLAIMED; +emit BridgeStatusUpdated(transactionId, BridgeStatus.RELAYER_CLAIMED);Ensure you've defined the
BridgeStatusUpdatedevent as suggested in a previous comment. This addition would enhance the contract's observability and make it easier to track the complete lifecycle of bridge transactions.
Line range hint
273-285: Emit event for status change toREFUNDEDAfter updating the status to
REFUNDED, it would be beneficial to emit an event logging this state change. This improves transparency and aids in off-chain tracking of transaction statuses.Add an event emission:
bridgeTxDetails[transactionId].status = BridgeStatus.REFUNDED; +emit BridgeStatusUpdated(transactionId, BridgeStatus.REFUNDED);Ensure you've defined the
BridgeStatusUpdatedevent as suggested in previous comments. This addition would further enhance the contract's observability and provide a complete picture of the bridge transaction lifecycle.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (8 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol
🔇 Additional comments not posted (5)
packages/contracts-rfq/contracts/FastBridgeV2.sol (5)
196-203: LGTM: Efficient timestamp calculationThe
_timeSincefunction has been optimized to useuint40for timestamp calculations, which aligns well with the storage optimization inBridgeTxDetails. The unchecked block correctly handles potential overflow scenarios. This implementation is both gas-efficient and functionally correct.
209-212: LGTM: Correct usage of consolidated mappingThe
canClaimfunction has been successfully updated to use the newbridgeTxDetailsmapping. It correctly accesses the relevant fields from theBridgeTxDetailsstruct while maintaining the original logic. This change aligns well with the optimization goals of the PR.
36-38: 🛠️ Refactor suggestionOptimize
bridgeStatusesreturn type for gas efficiencyThe
bridgeStatusesfunction returns aBridgeStatusenum. While enums are convenient for readability, returning them can be less gas-efficient. Consider returning auint8instead, which represents the underlying value of the enum.-function bridgeStatuses(bytes32 transactionId) public view returns (BridgeStatus status) { +function bridgeStatuses(bytes32 transactionId) public view returns (uint8 status) { - return bridgeTxDetails[transactionId].status; + return uint8(bridgeTxDetails[transactionId].status); }This change would maintain backwards compatibility while potentially reducing gas costs for callers.
Likely invalid or redundant comment.
27-27: 🛠️ Refactor suggestionConsider making
bridgeTxDetailsmapping privateWhile the new
bridgeTxDetailsmapping effectively consolidatesbridgeStatusesandbridgeProofs, making it public allows direct access to the entireBridgeTxDetailsstruct. Consider making this mapping private and relying on specific getter functions for better encapsulation and potential gas savings.- mapping(bytes32 => BridgeTxDetails) public bridgeTxDetails; + mapping(bytes32 => BridgeTxDetails) private bridgeTxDetails;This change would align with the principle of minimal exposure and could potentially save gas by preventing unnecessary SLOAD operations when only specific fields are needed.
Likely invalid or redundant comment.
111-111:⚠️ Potential issueAdd check for existing transactions before setting status
When setting the status to
REQUESTED, it's important to ensure that thetransactionIddoesn't already exist in thebridgeTxDetailsmapping. This prevents accidentally overwriting an existing transaction.Consider adding a check:
+ if (bridgeTxDetails[transactionId].status != BridgeStatus.NONE) revert TransactionAlreadyExists(); bridgeTxDetails[transactionId].status = BridgeStatus.REQUESTED;This addition would prevent potential issues with duplicate transaction IDs and improve the contract's robustness.
Likely invalid or redundant comment.
| if (bridgeTxDetails[transactionId].status != BridgeStatus.REQUESTED) revert StatusIncorrect(); | ||
| BridgeTxDetails storage _thisBridgeTxDetails = bridgeTxDetails[transactionId]; | ||
| _thisBridgeTxDetails.status = BridgeStatus.RELAYER_PROVED; | ||
| _thisBridgeTxDetails.proofBlockTimestamp = uint40(block.timestamp); | ||
| _thisBridgeTxDetails.proofBlockNumber = uint48(block.number); | ||
| _thisBridgeTxDetails.proofRelayer = relayer; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Emit event for status change to RELAYER_PROVED
After updating the status to RELAYER_PROVED, consider emitting an event to log this state change. This aids in transparency and off-chain tracking of transaction statuses.
Add an event emission:
_thisBridgeTxDetails.status = BridgeStatus.RELAYER_PROVED;
_thisBridgeTxDetails.proofBlockTimestamp = uint40(block.timestamp);
_thisBridgeTxDetails.proofBlockNumber = uint48(block.number);
_thisBridgeTxDetails.proofRelayer = relayer;
+emit BridgeStatusUpdated(transactionId, BridgeStatus.RELAYER_PROVED);Don't forget to define the event:
event BridgeStatusUpdated(bytes32 indexed transactionId, BridgeStatus status);This addition would improve the contract's observability and make it easier to track the lifecycle of bridge transactions.
| function bridgeProofs(bytes32 transactionId) public view returns (BridgeProof memory) { | ||
| BridgeTxDetails storage _thisBridgeTxDetails = bridgeTxDetails[transactionId]; | ||
| return BridgeProof(_thisBridgeTxDetails.proofBlockTimestamp, _thisBridgeTxDetails.proofRelayer); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning individual values instead of BridgeProof struct
The bridgeProofs function returns a BridgeProof struct. Returning structs can be gas-inefficient and may expose internal implementation details. Consider returning individual fields of the struct instead.
-function bridgeProofs(bytes32 transactionId) public view returns (BridgeProof memory) {
+function bridgeProofs(bytes32 transactionId) public view returns (uint40 timestamp, address relayer) {
BridgeTxDetails storage _thisBridgeTxDetails = bridgeTxDetails[transactionId];
- return BridgeProof(_thisBridgeTxDetails.proofBlockTimestamp, _thisBridgeTxDetails.proofRelayer);
+ return (_thisBridgeTxDetails.proofBlockTimestamp, _thisBridgeTxDetails.proofRelayer);
}This change would maintain backwards compatibility while potentially reducing gas costs and limiting exposure of internal structures.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function bridgeProofs(bytes32 transactionId) public view returns (BridgeProof memory) { | |
| BridgeTxDetails storage _thisBridgeTxDetails = bridgeTxDetails[transactionId]; | |
| return BridgeProof(_thisBridgeTxDetails.proofBlockTimestamp, _thisBridgeTxDetails.proofRelayer); | |
| } | |
| function bridgeProofs(bytes32 transactionId) public view returns (uint40 timestamp, address relayer) { | |
| BridgeTxDetails storage _thisBridgeTxDetails = bridgeTxDetails[transactionId]; | |
| return (_thisBridgeTxDetails.proofBlockTimestamp, _thisBridgeTxDetails.proofRelayer); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
7-13: Consider renaming theNULLstate for improved clarityThe
BridgeStatusenum provides a clear representation of the possible states of a bridge transaction. However, theNULLstate could be renamed to improve clarity and avoid potential confusion with null values in other programming contexts.Consider renaming
NULLto a more descriptive term such asNOT_INITIATEDorUNINITIALIZED. For example:enum BridgeStatus { - NULL, // doesn't exist yet + NOT_INITIATED, // doesn't exist yet REQUESTED, RELAYER_PROVED, RELAYER_CLAIMED, REFUNDED }The rest of the enum looks good and follows a logical order of states for a bridge transaction.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (8 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (28 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/contracts-rfq/contracts/FastBridgeV2.sol
- packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol
🔇 Additional comments (1)
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
15-20: Efficient struct packing for gas optimizationThe
BridgeTxDetailsstruct is well-designed for gas efficiency:
- It uses smaller uint types (uint40 and uint48) for timestamp and block number, which are sufficient for their purposes.
- The struct is packed into a single 256-bit storage slot:
- BridgeStatus enum (8 bits)
- uint40 proofBlockTimestamp (40 bits)
- uint48 proofBlockNumber (48 bits)
- address proofRelayer (160 bits)
Total: 256 bitsThis efficient packing addresses the gas optimization concerns raised in previous comments and will lead to reduced gas costs for storage operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (1)
67-72: LGTM: Status checks updated correctly.The status checks have been consistently updated to use
IFastBridgeV2.BridgeStatusinstead ofFastBridgeV2.BridgeStatus. This change aligns the tests with the new interface definition while maintaining the original logic.Consider extracting the
IFastBridgeV2.BridgeStatusenum into a local variable to improve code readability:+ IFastBridgeV2.BridgeStatus status; - assertEq(fastBridge.bridgeStatuses(getTxId(bridgedTokenTx)), IFastBridgeV2.BridgeStatus.REQUESTED); + status = fastBridge.bridgeStatuses(getTxId(bridgedTokenTx)); + assertEq(status, IFastBridgeV2.BridgeStatus.REQUESTED); // Apply similar changes to the remaining assertionsThis change would make the code more concise and easier to read, especially if you need to perform additional operations with the status in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (10 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (27 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol
- packages/contracts-rfq/test/FastBridgeV2.t.sol
🔇 Additional comments (17)
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (17)
4-5: LGTM: Import statements updated correctly.The import statements have been appropriately updated to include
IFastBridgeV2. This change is necessary to use theBridgeStatusenum from the interface, which aligns with the PR objective of optimizing gas usage by consolidating mappings.
90-90: LGTM: Status check intest_bridge_tokenupdated correctly.The status check has been properly updated to use
IFastBridgeV2.BridgeStatus.REQUESTED. This change maintains consistency with the interface update while preserving the original test logic.
98-98: LGTM: Status check intest_prove_tokenupdated correctly.The status check has been appropriately updated to use
IFastBridgeV2.BridgeStatus.RELAYER_PROVED. This change maintains consistency with the interface update while preserving the original test logic.
108-108: LGTM: Status check intest_proveWithAddress_tokenupdated correctly.The status check has been properly updated to use
IFastBridgeV2.BridgeStatus.RELAYER_PROVED. This change maintains consistency with the interface update while preserving the original test logic.
117-117: LGTM: Status check intest_claim_tokenupdated correctly.The status check has been appropriately updated to use
IFastBridgeV2.BridgeStatus.RELAYER_CLAIMED. This change maintains consistency with the interface update while preserving the original test logic.
125-125: LGTM: Status check intest_claimWithAddress_tokenupdated correctly.The status check has been properly updated to use
IFastBridgeV2.BridgeStatus.RELAYER_CLAIMED. This change maintains consistency with the interface update while preserving the original test logic.
133-133: LGTM: Status check intest_dispute_tokenupdated correctly.The status check has been appropriately updated to use
IFastBridgeV2.BridgeStatus.REQUESTED. This change maintains consistency with the interface update while preserving the original test logic.
141-141: LGTM: Status check intest_refundPermissioned_tokenupdated correctly.The status check has been properly updated to use
IFastBridgeV2.BridgeStatus.REFUNDED. This change maintains consistency with the interface update while preserving the original test logic.
150-150: LGTM: Status check intest_refundPermissionless_tokenupdated correctly.The status check has been appropriately updated to use
IFastBridgeV2.BridgeStatus.REFUNDED. This change maintains consistency with the interface update while preserving the original test logic.
159-159: LGTM: Status check intest_bridge_ethupdated correctly.The status check has been properly updated to use
IFastBridgeV2.BridgeStatus.REQUESTED. This change maintains consistency with the interface update while preserving the original test logic.
167-167: LGTM: Status check intest_prove_ethupdated correctly.The status check has been appropriately updated to use
IFastBridgeV2.BridgeStatus.RELAYER_PROVED. This change maintains consistency with the interface update while preserving the original test logic.
177-177: LGTM: Status check intest_proveWithAddress_ethupdated correctly.The status check has been properly updated to use
IFastBridgeV2.BridgeStatus.RELAYER_PROVED. This change maintains consistency with the interface update while preserving the original test logic.
187-187: LGTM: Status check intest_claim_ethupdated correctly.The status check has been appropriately updated to use
IFastBridgeV2.BridgeStatus.RELAYER_CLAIMED. This change maintains consistency with the interface update while preserving the original test logic.
195-195: LGTM: Status check intest_claimWithAddress_ethupdated correctly.The status check has been properly updated to use
IFastBridgeV2.BridgeStatus.RELAYER_CLAIMED. This change maintains consistency with the interface update while preserving the original test logic.
203-203: LGTM: Status check intest_dispute_ethupdated correctly.The status check has been appropriately updated to use
IFastBridgeV2.BridgeStatus.REQUESTED. This change maintains consistency with the interface update while preserving the original test logic.
211-211: LGTM: Status checks in refund functions updated correctly.The status checks in both
test_refundPermissioned_ethandtest_refundPermissionless_ethhave been properly updated to useIFastBridgeV2.BridgeStatus.REFUNDED. These changes maintain consistency with the interface update while preserving the original test logic for both permissioned and permissionless refund scenarios.Also applies to: 220-220
Line range hint
1-223: Summary: Test file successfully updated to use the new interface.This review has covered all changes made to the
FastBridgeV2.GasBench.Src.t.soltest file. The primary modification involves updating all references fromFastBridgeV2.BridgeStatustoIFastBridgeV2.BridgeStatus. These changes are consistent across all test functions and align with the PR objective of optimizing gas usage by consolidating mappings.Key points:
- All status checks now use the
IFastBridgeV2.BridgeStatusenum.- The logic and structure of the tests remain unchanged.
- The updates ensure that the tests are using the new interface definition.
The consistency of these changes across the file reduces the risk of introducing errors and maintains the integrity of the test suite. This update successfully adapts the gas benchmark tests to work with the refactored
FastBridgeV2contract.
Changes to gas cost
🧾 Summary (50% most significant diffs)
Full diff report 👇
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
Line range hint
283-294: Emit event for status change toREFUNDEDAfter updating the status to
REFUNDED, consider emitting an event to log this state change. This improves transparency and aids in off-chain tracking of transaction statuses.Add an event emission after setting the status:
bridgeTxDetails[transactionId].status = BridgeStatus.REFUNDED; +emit BridgeStatusUpdated(transactionId, BridgeStatus.REFUNDED);Ensure that the
BridgeStatusUpdatedevent is defined in your contract:event BridgeStatusUpdated(bytes32 indexed transactionId, BridgeStatus status);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (8 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (27 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol
🔇 Additional comments (6)
packages/contracts-rfq/contracts/FastBridgeV2.sol (6)
41-44: Efficient implementation ofbridgeProofsThe
bridgeProofsfunction efficiently returns individual values (timestamp and relayer) instead of a struct. This approach is gas-efficient and provides good backwards compatibility.The implementation looks good and follows best practices for gas optimization.
206-213: Efficient implementation of_timeSinceThe
_timeSincefunction usesuint40for timestamp calculations, which is a good gas optimization technique. The unchecked block is correctly used to handle potential overflow, ensuring the function works correctly even whenblock.timestamp > type(uint40).max.The implementation is efficient and handles edge cases well.
219-221: Correct implementation ofcanClaimThe
canClaimfunction has been updated to use the newbridgeTxDetailsmapping for checking the transaction status and proof details. The logic remains consistent with its previous implementation, ensuring that claims can only be made after the dispute period has passed.The changes maintain the function's intended behavior while adapting to the new data structure.
37-39: 🛠️ Refactor suggestionOptimize
bridgeStatusesreturn type for gas efficiencyThe
bridgeStatusesfunction returns aBridgeStatusenum. While enums are convenient for readability, returning them can be less gas-efficient. Consider returning auint8instead, which represents the underlying value of the enum.Consider this change:
-function bridgeStatuses(bytes32 transactionId) public view returns (BridgeStatus status) { +function bridgeStatuses(bytes32 transactionId) public view returns (uint8 status) { - return bridgeTxDetails[transactionId].status; + return uint8(bridgeTxDetails[transactionId].status); }This optimization would reduce gas costs for callers while maintaining the function's functionality.
Likely invalid or redundant comment.
28-28: 🛠️ Refactor suggestionConsider changing
bridgeTxDetailsvisibility to privateThe
bridgeTxDetailsmapping is declared as public, which allows direct access to the internal data structure. For better encapsulation and control over data access, consider changing its visibility to private and using getter functions to expose necessary data.Apply this change:
- mapping(bytes32 => BridgeTxDetails) public bridgeTxDetails; + mapping(bytes32 => BridgeTxDetails) private bridgeTxDetails;This change would improve encapsulation and allow for more controlled access to the bridge transaction details.
Likely invalid or redundant comment.
Line range hint
1-304: Overall improvements with consolidated data structureThe changes to the
FastBridgeV2contract, particularly the introduction of thebridgeTxDetailsmapping and corresponding updates to various functions, improve the contract's efficiency and maintain backwards compatibility. The core logic of the bridge functionality remains consistent with the previous implementation.The refactoring enhances the contract's structure while preserving its intended behavior.
To ensure the changes haven't introduced any regressions, consider running the following verification steps:
- Comprehensive test suite execution covering all modified functions.
- Gas usage comparison between the old and new implementations.
- Formal verification of critical functions, if applicable.
This script will help verify that all status transitions in the code follow the expected state machine.
Description
A lot of functions are reading/writing from/to both
bridgeStatusesandbridgeProofs, wasting more gas than necessary.bridgeStatuses(bytes32)andbridgeProofs(bytes32)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests