Relay Bridge (v1.0.0) [RelayFacet v1.0.0,LibAsset v1.0.1] #843
Conversation
WalkthroughThe pull request introduces significant updates to the relay configuration and associated smart contracts, specifically adding and modifying the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)audit/auditLog.json (2)
Standardize date format and verify audit commit date
Apply this diff to standardize the format: "audit20241202": {
"auditCompletedOn": "02.12.2024",
"auditedBy": "Sujith Somraaj (individual security researcher)",
"auditorGitHandle": "sujithsomraaj",
- "auditReportPath": "./audit/reports/2024-12-02_RelayFacet(v1.0.0).pdf",
+ "auditReportPath": "./audit/reports/2024.12.02_RelayFacet(v1.0.0).pdf",
"auditCommitHash": "291d0a78bc4174b3ec29bb2ce0b27c6b5d3e8ec8"
},
Restore alphabetical ordering of contract entries The contract entries should maintain alphabetical ordering as per the previous convention. Current ordering needs adjustment. The contracts should be ordered as follows:
Let's verify the audit references: 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
CodeRabbit Configuration File (
|
![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 (4)
templates/facetDeployScript.template.hbs (1)
Line range hint 27-31: Consider adding config validation.
The current implementation assumes the JSON config will always have the required address field. Consider adding validation to handle missing or invalid configurations gracefully.
address example = json.readAddress(
string.concat(".", network, ".example")
);
+ require(
+ example != address(0),
+ string.concat("Invalid configuration: missing or zero address for ", network, ".example")
+ );
return abi.encode(example);script/deploy/facets/DeployRelayFacet.s.sol (1)
2-2: Consider pinning the Solidity version.
Using a floating pragma (^0.8.17) could lead to inconsistent behavior across different environments. Consider pinning to a specific version.
-pragma solidity ^0.8.17;
+pragma solidity 0.8.17;docs/RelayFacet.md (2)
51-72: Add security warnings to the example section.
The transaction example should include important security considerations:
- How to validate the quote response
- Importance of checking slippage parameters
- Steps to verify contract addresses
- Warnings about transaction value and gas parameters
35-35: Fix grammar and punctuation issues.
Please apply the following corrections:
- Line 35: Change "swap specific" to "swap-specific"
- Line 47: Add comma after "In the following"
- Line 80: Add comma before "you can execute"
- Line 88: Add comma before "you can execute"
Also applies to: 47-47, 80-80, 88-88
🧰 Tools
🪛 LanguageTool
[uncategorized] ~35-~35: When ‘swap-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... parameter. Swapping is performed by a swap specific library that expects an array of callda...
(SPECIFIC_HYPHEN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- config/relay.json (1 hunks)
- docs/RelayFacet.md (1 hunks)
- script/deploy/facets/DeployRelayFacet.s.sol (1 hunks)
- script/deploy/facets/UpdateRelayFacet.s.sol (1 hunks)
- src/Facets/RelayFacet.sol (1 hunks)
- templates/facetDeployScript.template.hbs (1 hunks)
- test/solidity/Facets/RelayFacet.t.sol (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- config/relay.json
🧰 Additional context used
🪛 LanguageTool
docs/RelayFacet.md
[uncategorized] ~35-~35: When ‘swap-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... parameter. Swapping is performed by a swap specific library that expects an array of callda...
(SPECIFIC_HYPHEN)
[uncategorized] ~47-~47: A comma might be missing here.
Context: ...alls to interact with the Facet In the following some sample calls are shown that allow ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~80-~80: Possible missing comma found.
Context: ... from 30 USDC.e on Avalanche to USDC on Binance you can execute the following request: ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~88-~88: Possible missing comma found.
Context: ...er from 30 USDT on Avalanche to USDC on Binance you can execute the following request: ...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (8)
script/deploy/facets/UpdateRelayFacet.s.sol (2)
9-24: Add upgrade documentation and access control.
For a v1.0.0 release of a bridge component:
- Add inline documentation describing the upgrade process and requirements
- Verify access control in the base contract
- Consider adding explicit version checks
18-23: Verify upgrade safety and configuration loading.
The run() function directly calls update("RelayFacet") without any configuration validation or safety checks. Consider:
- Loading and validating configuration from relay.json
- Adding pre-upgrade state validation
- Documenting the upgrade process
templates/facetDeployScript.template.hbs (1)
27-29: Verify JSON configuration structure across environments.
The variable rename from acrossSpokePool to example suggests this is a template placeholder. However, ensure that:
- The corresponding JSON config files in all environments have the correct structure with the
examplefield - The deployment documentation is updated to reflect this configuration requirement
✅ Verification successful
Configuration structure is correctly defined and matches the template pattern
Based on the verification results:
- The
relay.jsonconfig file correctly defines the expected structure withexampleandexampleAllowedTokensfields for both mainnet and arbitrum networks. - This matches the template pattern seen in
templates/facetConfig.template.hbsand is being used consistently intemplates/facetDeployScript.template.hbs. - The variable rename from
acrossSpokePooltoexampleis part of the standard template instantiation, whereexampleis a placeholder that will be replaced with actual implementation values.
The configuration structure is properly aligned with the facet deployment template system, and no issues were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify JSON config structure across environments
# Expected: All config files should have the .network.example field
# Search for config files and check their structure
fd -e json -d 2 . config/ --exec sh -c 'echo "Checking {}" && jq -e ".mainnet.example, .arbitrum.example" {} > /dev/null'
Length of output: 1228
Script:
#!/bin/bash
# Let's check the content of relay.json since this PR is about Relay Bridge
cat config/relay.json
Length of output: 467
Script:
#!/bin/bash
# Let's also check if there are any other template files that might give us context
fd -e template.hbs . templates/ -x cat {}
Length of output: 17753
Script:
#!/bin/bash
# Search for any references to 'example' in deployment scripts or templates
rg -g '*.{sol,hbs}' 'example' . -C 2
Length of output: 12707
script/deploy/facets/DeployRelayFacet.s.sol (2)
8-11: LGTM!
The contract declaration and constructor are well-structured and properly inherit from the base deployment script.
24-29: Verify relay.json configuration structure.
Let's verify that the configuration file exists and contains the expected network configurations.
✅ Verification successful
Configuration file structure verified and valid
The relay.json configuration file exists and contains the expected structure with:
- Valid JSON format
- Required network configurations for both mainnet and arbitrum
- Expected
examplefield for each network - Additional
exampleAllowedTokensarrays for both networks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify relay.json structure and content
# Check if config file exists and has valid JSON structure
if [ -f "config/relay.json" ]; then
echo "Checking relay.json structure..."
jq -e . "config/relay.json" > /dev/null
# Verify required network configurations
jq -e '.mainnet.example' "config/relay.json" > /dev/null
jq -e '.arbitrum.example' "config/relay.json" > /dev/null
fi
Length of output: 220
Script:
#!/bin/bash
# Let's check if the file exists and display its content
if [ -f "config/relay.json" ]; then
echo "Content of relay.json:"
cat config/relay.json
else
echo "relay.json not found in config directory"
fi
# Let's also check the directory structure to ensure we're looking in the right place
echo -e "\nDirectory structure:"
ls -la config/ 2>/dev/null || echo "config directory not found"
Length of output: 2839
docs/RelayFacet.md (2)
24-29:
RelayData struct appears to be a placeholder.
The struct documentation shows an example parameter that doesn't reflect the actual implementation. Please document:
- All actual struct fields
- Valid parameter values and constraints
- Security implications of each parameter
Let me verify the actual struct implementation:
#!/bin/bash
# Find the actual RelayData struct definition
ast-grep --pattern 'struct RelayData {
$$$
}'15-18:
Enhance method documentation with security considerations.
The method documentation needs improvement:
- Fix the typo: "swapAndStartBridgeTokensViarelay" should be "swapAndStartBridgeTokensViaRelay"
- Add important details for each method:
- Parameter validation rules
- Security considerations for token approvals
- Return values
- Potential failure scenarios and error handling
Let me verify the actual implementation details:
#!/bin/bash
# Find the method implementations to document their behavior accurately
ast-grep --pattern 'function startBridgeTokensViaRelay($$$) {
$$$
}'test/solidity/Facets/RelayFacet.t.sol (1)
49-56:
Define or import uniswap before use
The variable uniswap is used to access function selectors, but it is not defined or imported in the provided code. This will result in a compilation error.
Please ensure that uniswap is properly defined or imported. You can verify if uniswap is available in the codebase by running the following script:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/Facets/RelayFacet.sol (1)
46-46: Fix the typo in comment: 'attestaion' should be 'attestation'
There's a typo in the comment. The word "attestaion" should be corrected to "attestation".
Apply this diff to correct the typo:
// Verify that the bridging quote has been signed by the Relay solver
- // as attested using the attestaion API
+ // as attested using the attestation API
// API URL: https://api.relay.link/requests/{requestId}/signature/v2
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/Facets/RelayFacet.sol (1)
47-47: Correct the typo in the comment.
There's a spelling error in the comment. The word "attestaion" should be "attestation".
Apply this diff to fix the typo:
- // as attested using the attestaion API
+ // as attested using the attestation API📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/Facets/RelayFacet.sol (1 hunks)
🔇 Additional comments (1)
src/Facets/RelayFacet.sol (1)
135-135: Verify safe extraction of quoteId from callData.
Directly slicing callData starting from byte 68 may lead to errors if callData is shorter than expected. Ensure that callData has sufficient length before slicing to prevent out-of-bounds access.
Consider adding a length check:
+ require(
+ _relayData.callData.length >= 68,
+ "callData too short to extract quoteId"
+ );
bytes memory quoteId = _relayData.callData[68:];
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- config/relay.json (1 hunks)
- src/Facets/RelayFacet.sol (1 hunks)
- test/solidity/Facets/RelayFacet.t.sol (1 hunks)
🔇 Additional comments (7)
config/relay.json (1)
1-46: Verify production addresses and add documentation.
The configuration uses the same production addresses (0xa5f565... and 0xf70da...) across all networks. Please:
- Confirm these are the intended production addresses
- Document the ownership and purpose of these addresses
- Verify if using identical addresses across different networks is intentional from a security perspective
src/Facets/RelayFacet.sol (2)
79-97: LGTM! Well-structured with comprehensive security measures.
The function implements proper security controls including reentrancy protection, quote validation, and bridge data validation. The modifiers effectively prevent unwanted source swaps and destination calls.
103-124: LGTM! Comprehensive implementation with proper swap integration.
The function correctly handles both swapping and bridging operations while maintaining all necessary security checks.
test/solidity/Facets/RelayFacet.t.sol (4)
9-22: Approval: Stub contract and utility functions are correctly implemented
The TestRelayFacet contract correctly extends RelayFacet and provides the utility functions addDex and setFunctionApprovalBySignature. These functions appropriately interact with LibAllowList to manage allowed contracts and function selectors.
36-36: Reminder: Uncomment initRelay to initialize allowed tokens
The call to relayFacet.initRelay(EXAMPLE_ALLOWED_TOKENS); is still commented out. If initializing the allowed tokens is necessary for the proper functioning of relayFacet in your tests, please uncomment this line to ensure correct initialization.
66-71: Ensure all fields in validRelayData are properly initialized
The validRelayData struct is initialized with empty strings for callData and signature. While signature is updated later, ensure that callData is appropriately set before it's used, especially in scenarios where it's required for the relay operation.
75-87: Verify the use of validRelayData in relay function calls
In the initiateBridgeTxWithFacet function, validRelayData is used in calls to relayFacet.startBridgeTokensViaRelay. Ensure that all necessary fields within validRelayData, such as callData and signature, are correctly set for both native and non-native transactions to prevent unexpected behavior during testing.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (2)
config/relay.json (1)
1-46: Add schema validation and documentation.Consider adding:
- A JSON schema file to validate the configuration format
- Documentation about the expected structure and address formats
- A version field for future compatibility
Example structure:
{ "version": "1.0.0", "schema": "./relay.schema.json", "networks": { // current content } }Would you like me to help create the JSON schema file and documentation?
deployments/_deployments_log_file.json (1)
24357-24357: Consider using deterministic deployment with salt.The
SALTfield is empty for both deployments. Using a deterministic salt value can help ensure consistent contract addresses across different networks and make the deployment process more predictable.Also applies to: 24371-24371
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
config/relay.json(1 hunks)deployments/_deployments_log_file.json(1 hunks)deployments/arbitrum.diamond.staging.json(2 hunks)deployments/arbitrum.staging.json(1 hunks)script/demoScripts/demoRelay.ts(1 hunks)src/Facets/RelayFacet.sol(1 hunks)test/solidity/Facets/RelayFacet.t.sol(1 hunks)
🔥 Files not summarized due to errors (1)
- deployments/_deployments_log_file.json: Error: Server error: no LLM provider could handle the message
🚧 Files skipped from review as they are similar to previous changes (3)
- deployments/arbitrum.staging.json
- src/Facets/RelayFacet.sol
- test/solidity/Facets/RelayFacet.t.sol
🔇 Additional comments (7)
config/relay.json (1)
35-35: Verify the linea relayReceiver address.
The address 0x00000000aa467eba42a3d604b3d74d63b2b6c6cb contains many leading zeros, which is unusual and could potentially be a typo. Please verify this address is correct.
deployments/arbitrum.diamond.staging.json (2)
152-152: Verify Receiver address consistency.
Please confirm that this Receiver address matches the one specified in the relay configuration:
#!/bin/bash
# Description: Verify Receiver address consistency across configuration files
# Test: Check if the same receiver address is used in relay config
echo "Checking relay configuration..."
rg -i "0x36E9B2E8A627474683eF3b1E9Df26D2bF04396f3" config/relay.json128-130: Verify RelayFacet deployment and audit status.
Please ensure:
- The deployed address matches the one used in deployment scripts and audit reports
- The RelayFacet contract has undergone preliminary audit as per PR requirements
Let's verify the deployment address consistency:
✅ Verification successful
RelayFacet deployment address and verification confirmed
The deployment address 0x74763722d92832d247cFa92825b06098cf72BAA2 is verified:
- Matches across deployment configurations in
arbitrum.staging.jsonandarbitrum.diamond.staging.json - Deployment logs confirm it was deployed on Oct 31, 2024, with verification status "true"
- Documentation in
docs/RelayFacet.mdconfirms this is a legitimate facet for token bridging functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify RelayFacet deployment address matches across configuration files
# Test 1: Check deployment scripts for the address
echo "Checking deployment scripts..."
rg -i "0x74763722d92832d247cFa92825b06098cf72BAA2" scripts/deployment/
# Test 2: Check if address appears in audit reports
echo "Checking audit reports..."
fd -e pdf . audits/ --exec rg -i "0x74763722d92832d247cFa92825b06098cf72BAA2" {} \;
Length of output: 502
Script:
#!/bin/bash
# Let's try a different approach to verify the RelayFacet deployment
# Check for deployment scripts and configurations mentioning RelayFacet
echo "Checking for RelayFacet deployment configurations..."
rg -i "RelayFacet" --type json
# Check for any deployment addresses in deployment scripts
echo "Checking deployment scripts for facet deployments..."
rg -i "RelayFacet" scripts/ -A 5 -B 5
# Look for deployment task or configuration
echo "Checking for deployment tasks..."
fd -e js -e ts . scripts/ --exec rg -l "deploy.*Relay" {} \;
# Check for any audit or verification files
echo "Checking for audit or verification files..."
fd -e md -e txt . -d 2
Length of output: 2373
Script:
#!/bin/bash
# Let's check the RelayFacet documentation and deployment logs
# Check RelayFacet documentation
echo "Checking RelayFacet documentation..."
cat docs/RelayFacet.md
# Check deployment logs
echo "Checking deployment logs..."
cat deployments/_deployments_log_file.json | grep -A 10 -B 10 "RelayFacet"
# Check target vs deployed files for staging
echo "Checking target vs deployed files..."
cat target_vs_deployed_staging.txt
Length of output: 8343
deployments/_deployments_log_file.json (3)
24354-24354: High optimizer runs confirmed.
The optimizer runs are set to 1,000,000, which is appropriate for contracts that will be frequently used and benefit from gas optimization.
Also applies to: 24368-24368
24358-24358: Verification status confirmed.
Both contract deployments are marked as verified, which is essential for transparency and security.
Also applies to: 24372-24372
24353-24358: Verify the constructor argument addresses and document their purpose.
The constructor is initialized with two addresses that are consistent across both deployments:
0xa5f565650890fba1824ee0f21ebbbf660a1799340xf70da97812cb96acdf810712aa562db8dfa3dbef
Please ensure these addresses are correct and consider documenting their purpose in the deployment log.
Also applies to: 24367-24372
✅ Verification successful
Constructor addresses are correctly configured and documented
The addresses in the constructor arguments match the configuration in config/relay.json:
- First address (
0xa5f565...934) is therelayReceiver - Second address (
0xf70da9...bEF) is therelaySolver
Both addresses are consistently used across network configurations and test files, confirming their intended roles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for address documentation and verify their roles
# Expected: Find references to these addresses in configuration or documentation
echo "Searching for the first constructor address..."
rg -i "0xa5f565650890fba1824ee0f21ebbbf660a179934"
echo "Searching for the second constructor address..."
rg -i "0xf70da97812cb96acdf810712aa562db8dfa3dbef"
Length of output: 2066
script/demoScripts/demoRelay.ts (1)
28-28:
Verify the user parameter in request parameters
The user parameter is set to deployments.LiFiDiamond, which is the contract address. Typically, user should be the address of the end-user initiating the request.
Please confirm if deployments.LiFiDiamond is the intended value for user. If not, consider changing it to address:
-user: deployments.LiFiDiamond,
+user: address,Also applies to: 93-93
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
docs/RelayFacet.md (3)
10-14: Enhance the flowchart with more implementation details.The current mermaid diagram could be more informative by including:
- The token flow (Native vs non-Native)
- The RelayReceiver contract
- The quote validation process
- The solver EOA interaction
Here's a suggested enhanced diagram:
graph LR; W[Wallet] --> D{LiFiDiamond} D -- DELEGATECALL --> RF[RelayFacet] RF -- Native Tokens --> RR[RelayReceiver] RF -- Other Tokens --> S[Solver EOA] RF -- Validate Quote --> V[Quote Validation]
28-30: Complete the RelayData struct documentation.The parameter documentation is incomplete. Add descriptions for:
receivingAssetId: The address of the token to be received on the destination chainsignature: The solver's signature validating the quoteUpdate the documentation as follows:
/// @param requestId the id of the quote request. -/// @param receivingAssetId +/// @param receivingAssetId the address of the token to be received on the destination chain /// @param callData the bridging calldata generated by the relay api +/// @param signature the solver's ECDSA signature validating the quote
43-43: Fix hyphenation in compound modifier.Add a hyphen to "swap-specific" as it's used as a compound modifier.
-Swapping is performed by a swap specific library that expects an array of calldata +Swapping is performed by a swap-specific library that expects an array of calldata🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: When ‘swap-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... parameter. Swapping is performed by a swap specific library that expects an array of callda...(SPECIFIC_HYPHEN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
docs/README.md(1 hunks)docs/RelayFacet.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/README.md
🧰 Additional context used
🪛 LanguageTool
docs/RelayFacet.md
[uncategorized] ~43-~43: When ‘swap-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... parameter. Swapping is performed by a swap specific library that expects an array of callda...
(SPECIFIC_HYPHEN)
Test Coverage ReportLine Coverage: 78.26% (2179 / 2784 lines) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
templates/facetDoc.template.hbs (1)
Line range hint
1-100: Documentation template needs completion for relay-specific details.The template contains placeholder text "The {{titleCase name}} Facet works by ..." that needs to be completed with relay-specific implementation details. This is particularly important as this is a new facet implementation (v1.0.0).
Consider adding:
- Detailed explanation of the relay mechanism
- Security considerations
- Gas optimization tips
- Common integration patterns
Would you like me to help draft the relay-specific documentation content?
docs/RelayFacet.md (3)
1-15: Add security considerations section.The introduction should include critical security aspects:
- Quote validation mechanism using signatures
- Trust model and role of the relay solver
- Security implications of using DELEGATECALL
- Asset validation process
Would you like me to help draft a security considerations section?
55-55: Fix grammatical issue.Add a comma for better readability:
-In the following some sample calls are shown that allow you to retrieve +In the following, some sample calls are shown that allow you to retrieve🧰 Tools
🪛 LanguageTool
[uncategorized] ~55-~55: Possible missing comma found.
Context: ...alls to interact with the Facet In the following some sample calls are shown that allow ...(AI_HYDRA_LEO_MISSING_COMMA)
90-92: Add API response examples.The curl examples show how to make requests, but it would be helpful to include example responses for both the "Cross Only" and "Swap & Cross" scenarios. This would help developers understand what to expect from the API.
Would you like me to help draft example API responses for both scenarios?
Also applies to: 98-100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
deployments/_deployments_log_file.json(1 hunks)deployments/arbitrum.diamond.staging.json(2 hunks)deployments/optimism.diamond.staging.json(2 hunks)deployments/optimism.staging.json(1 hunks)deployments/polygon.diamond.staging.json(2 hunks)deployments/polygon.staging.json(1 hunks)docs/RelayFacet.md(1 hunks)script/deploy/facets/DeployRelayFacet.s.sol(1 hunks)templates/facetDoc.template.hbs(1 hunks)test/solidity/Facets/RelayFacet.t.sol(1 hunks)
🔥 Files not summarized due to errors (1)
- deployments/_deployments_log_file.json: Error: Server error: no LLM provider could handle the message
🚧 Files skipped from review as they are similar to previous changes (5)
- deployments/arbitrum.diamond.staging.json
- deployments/polygon.diamond.staging.json
- deployments/polygon.staging.json
- script/deploy/facets/DeployRelayFacet.s.sol
- test/solidity/Facets/RelayFacet.t.sol
🧰 Additional context used
🪛 LanguageTool
docs/RelayFacet.md
[uncategorized] ~43-~43: When ‘swap-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... parameter. Swapping is performed by a swap specific library that expects an array of callda...
(SPECIFIC_HYPHEN)
[uncategorized] ~55-~55: Possible missing comma found.
Context: ...alls to interact with the Facet In the following some sample calls are shown that allow ...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (7)
deployments/optimism.staging.json (2)
42-44: LGTM - JSON structure maintains consistency.
The changes maintain proper JSON formatting and follow the established pattern for contract deployments in the staging environment.
43-43: Verify RelayFacet contract deployment and audit status.
As this is a new contract deployment in the staging environment, please ensure:
- The contract has undergone preliminary audit as per PR requirements
- The deployment address matches the output from deployment scripts
templates/facetDoc.template.hbs (2)
17-17: LGTM! Method signature format updated for consistency.
The change from {{camelCase name}} to {{titleCase name}} in the method signature aligns with the naming conventions used throughout the codebase.
Line range hint 1-100: Verify API examples are accurate for the relay implementation.
The curl examples use placeholder values that should be verified to work with the relay implementation:
- Chain identifiers (AVA, BSC)
- Token addresses (USDC, USDT)
- Amount decimals (30000000)
deployments/optimism.diamond.staging.json (2)
140-140: Verify Receiver address restoration.
The Receiver address has been restored in the Periphery section. Please confirm:
- This is the correct address that should be used
- The address is consistent with other network configurations
Let's verify the Receiver address across deployment files:
#!/bin/bash
# Search for Receiver address across all deployment files
echo "Checking Receiver address consistency across networks:"
rg '"Receiver": "0x36E9B2E8A627474683eF3b1E9Df26D2bF04396f3"' deployments/128-130: Verify audit status and address consistency for RelayFacet.
As per PR requirements, please ensure:
- The RelayFacet contract has undergone preliminary audit
- The deployed address is consistent across all network configurations
Let's verify the address consistency across deployment files:
deployments/_deployments_log_file.json (1)
24370-24370: Verify constructor arguments for proper initialization.
The constructor is initialized with two addresses:
- 0xa5f565650890fba1824ee0f21ebbbf660a179934
- 0xf70da97812cb96acdf810712aa562db8dfa3dbef
Please verify these addresses correspond to the correct contract dependencies across all networks.
#!/bin/bash
# Description: Search for these addresses in the codebase to understand their roles
# Search for the first address
echo "Searching for first constructor address..."
rg -i "a5f565650890fba1824ee0f21ebbbf660a179934"
# Search for the second address
echo "Searching for second constructor address..."
rg -i "f70da97812cb96acdf810712aa562db8dfa3dbef"
# Look for potential interface or contract definitions that might use these addresses
echo "Checking for related contracts..."
ast-grep --pattern 'interface $_ {
$$$
}'Also applies to: 24384-24384, 24398-24398
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
script/demoScripts/utils/demoScriptHelpers.ts (2)
246-246: Extract zero address as a constant.The zero address is a commonly used value and should be defined as a constant at the top of the file alongside other address constants.
+ export const ADDRESS_ZERO = '0x0000000000000000000000000000000000000000' // ... later in the code ... - receivingAssetId: '0x0000000000000000000000000000000000000000', + receivingAssetId: ADDRESS_ZERO,
216-217: Remove or guard console.log statements.Production code should not contain debug logging statements that might expose sensitive transaction details. Consider:
- Removing these logs
- Using a proper logging framework with configurable levels
- Guarding logs behind a debug flag
Also applies to: 227-228
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
script/demoScripts/demoRelay.ts(1 hunks)script/demoScripts/utils/demoScriptHelpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- script/demoScripts/demoRelay.ts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
test/solidity/Facets/RelayFacet.t.sol (1)
36-36: Nitpick: Explicitly declare visibility forRELAY_SOLVERThe state variable
RELAY_SOLVERdoes not have an explicit visibility modifier. While Solidity defaults tointernalfor state variables, it's best practice to explicitly declare the visibility for clarity and maintainability.Apply this diff to specify the visibility:
- address RELAY_SOLVER = vm.addr(PRIVATE_KEY); + address internal RELAY_SOLVER = vm.addr(PRIVATE_KEY);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
test/solidity/Facets/RelayFacet.t.sol(1 hunks)
🧰 Additional context used
📓 Learnings (1)
test/solidity/Facets/RelayFacet.t.sol (2)
Learnt from: ezynda3
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:185-187
Timestamp: 2024-11-05T12:39:31.996Z
Learning: In this codebase, changing the order of signature components from (r, s, v) to (v, r, s) is not required.
Learnt from: ezynda3
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:29-30
Timestamp: 2024-10-31T09:10:04.225Z
Learning: In test code, using hardcoded placeholder private keys is acceptable and should not be flagged as an issue.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/solidity/Facets/RelayFacet.t.sol (2)
71-71: Define chain IDs as constants for clarity and maintainabilityMagic numbers such as
1151111081099710and20000000000001are used multiple times to represent destination chain IDs for Solana and Bitcoin, respectively. Defining these chain IDs as constants with descriptive names will improve code readability and maintainability.You can define the chain IDs as constants at the beginning of the contract:
+ uint256 constant SOLANA_CHAIN_ID = 1151111081099710; + uint256 constant BITCOIN_CHAIN_ID = 20000000000001;Then replace the magic numbers in your code:
- bridgeData.destinationChainId = 1151111081099710; + bridgeData.destinationChainId = SOLANA_CHAIN_ID;And in the
_getMappedChainIdfunction:- if (chainId == 20000000000001) { + if (chainId == BITCOIN_CHAIN_ID) { return 8253038; } - if (chainId == 1151111081099710) { + if (chainId == SOLANA_CHAIN_ID) { return 792703809; }Also applies to: 137-137, 275-275, 376-376, 414-414, 676-682
645-671: Enhance readability by simplifying nested expressions insignDataThe
signDatafunction contains deeply nested expressions, which can make the code harder to read and maintain. Breaking down the complex expression into intermediate variables will improve clarity.Here's an example of how you can refactor the code:
function signData( ILiFi.BridgeData memory _bridgeData, RelayFacet.RelayData memory _relayData ) internal view returns (bytes memory) { + bytes32 bridgeAddress = bytes32(uint256(uint160(address(relayFacet)))); + bytes32 sendingAssetId = bytes32(uint256(uint160(_bridgeData.sendingAssetId))); + uint256 mappedChainId = _getMappedChainId(_bridgeData.destinationChainId); + bytes32 receiver = _bridgeData.receiver == LibAsset.NON_EVM_ADDRESS + ? _relayData.nonEVMReceiver + : bytes32(uint256(uint160(_bridgeData.receiver))); + bytes32 messageHash = keccak256( + abi.encodePacked( + _relayData.requestId, + block.chainid, + bridgeAddress, + sendingAssetId, + mappedChainId, + receiver, + _relayData.receivingAssetId + ) + ); + bytes32 prefixedHash = keccak256( + abi.encodePacked( + "\x19Ethereum Signed Message:\n32", + messageHash + ) + ); - bytes32 message = keccak256( - abi.encodePacked( - "\x19Ethereum Signed Message:\n32", - keccak256( - abi.encodePacked( - _relayData.requestId, - block.chainid, - bytes32(uint256(uint160(address(relayFacet)))), - bytes32(uint256(uint160(_bridgeData.sendingAssetId))), - _getMappedChainId(_bridgeData.destinationChainId), - _bridgeData.receiver == LibAsset.NON_EVM_ADDRESS - ? _relayData.nonEVMReceiver - : bytes32(uint256(uint160(_bridgeData.receiver))), - _relayData.receivingAssetId - ) - ) - ) - ); (uint8 v, bytes32 r, bytes32 s) = vm.sign(PRIVATE_KEY, prefixedHash); bytes memory signature = abi.encodePacked(r, s, v); return signature; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
test/solidity/Facets/RelayFacet.t.sol(1 hunks)
🧰 Additional context used
📓 Learnings (1)
test/solidity/Facets/RelayFacet.t.sol (2)
Learnt from: ezynda3
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:185-187
Timestamp: 2024-11-05T12:39:31.996Z
Learning: In this codebase, changing the order of signature components from (r, s, v) to (v, r, s) is not required.
Learnt from: ezynda3
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:29-30
Timestamp: 2024-10-31T09:10:04.225Z
Learning: In test code, using hardcoded placeholder private keys is acceptable and should not be flagged as an issue.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
test/solidity/Facets/RelayFacet.t.sol (5)
76-78: Enhance comment clarity for bridgeData adjustmentsThe comment "adjust bridgeData" is too vague. Consider adding more context about why these specific values are being set.
-// adjust bridgeData +// Configure bridgeData for relay bridge testing with Polygon as destination bridgeData.bridge = "relay"; bridgeData.destinationChainId = 137;
80-87: Add descriptive comments for RelayData initializationThe initialization of validRelayData could benefit from more detailed comments explaining the purpose of each field.
validRelayData = RelayFacet.RelayData({ - requestId: bytes32("1234"), - nonEVMReceiver: "", + requestId: bytes32("1234"), // Unique identifier for the relay request + nonEVMReceiver: "", // Empty for EVM chains, filled for non-EVM destinations receivingAssetId: bytes32( uint256(uint160(0x2791Bca1f2de4661ED88A30C99A7a9449Aa84174)) - ), // Polygon USDC + ), // Polygon USDC contract address as bytes32 signature: "" });
578-581: Clarify comment about bridged amount calculationThe comment about bridged amount calculation could be more precise and structured.
-//@dev the bridged amount will be higher than bridgeData.minAmount since the code will -// deposit all remaining ETH to the bridge. We cannot access that value (minAmount + remaining gas) -// therefore the test is designed to only check if an event was emitted but not match the parameters +/** @dev Bridged amount verification limitation + * The actual bridged amount includes: + * 1. bridgeData.minAmount + * 2. Remaining ETH from gas (cannot be precisely calculated in test) + * Therefore, we only verify event emission without parameter matching + */
275-593: Consider extracting common test setup logicThe native token bridging tests contain significant code duplication, particularly in swap data setup. Consider extracting this common logic into a helper function.
+// Add this helper function +function setupNativeTokenSwapData(uint256 amountOut) internal returns (uint256) { + address[] memory path = new address[](2); + path[0] = ADDRESS_USDC; + path[1] = ADDRESS_WRAPPED_NATIVE; + + uint256[] memory amounts = uniswap.getAmountsIn(amountOut, path); + uint256 amountIn = amounts[0]; + + bridgeData.minAmount = amountOut; + + delete swapData; + swapData.push( + LibSwap.SwapData({ + callTo: address(uniswap), + approveTo: address(uniswap), + sendingAssetId: ADDRESS_USDC, + receivingAssetId: address(0), + fromAmount: amountIn, + callData: abi.encodeWithSelector( + uniswap.swapTokensForExactETH.selector, + amountOut, + amountIn, + path, + _facetTestContractAddress, + block.timestamp + 20 minutes + ), + requiresDeposit: true + }) + ); + + return amountIn; +}Then use it in the test functions:
-// prepare swap data -address[] memory path = new address[](2); -path[0] = ADDRESS_USDC; -path[1] = ADDRESS_WRAPPED_NATIVE; - -uint256 amountOut = defaultNativeAmount; - -// Calculate USDC input amount -uint256[] memory amounts = uniswap.getAmountsIn(amountOut, path); -uint256 amountIn = amounts[0]; - -bridgeData.minAmount = amountOut; - -delete swapData; -swapData.push(/* ... */); +uint256 amountIn = setupNativeTokenSwapData(defaultNativeAmount);
652-667: Consider using parameterized test for chain ID mappingThe
test_mapsCorrectChainIdfunction could be improved by using a more structured approach to test data.-function test_mapsCorrectChainId(uint256 chainId) public { +struct ChainIdMapping { + uint256 input; + uint256 expected; + string description; +} + +function test_mapsCorrectChainId() public { + ChainIdMapping[] memory mappings = new ChainIdMapping[](3); + mappings[0] = ChainIdMapping({ + input: 20000000000001, + expected: 8253038, + description: "Bitcoin" + }); + mappings[1] = ChainIdMapping({ + input: 1151111081099710, + expected: 792703809, + description: "Solana" + }); + mappings[2] = ChainIdMapping({ + input: 1, + expected: 1, + description: "Default case" + }); + + for (uint256 i = 0; i < mappings.length; i++) { + uint256 mapped = relayFacet.getMappedChainId(mappings[i].input); + assertEq( + mapped, + mappings[i].expected, + string.concat("Failed mapping for ", mappings[i].description) + ); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
test/solidity/Facets/RelayFacet.t.sol(1 hunks)
🧰 Additional context used
📓 Learnings (1)
test/solidity/Facets/RelayFacet.t.sol (1)
Learnt from: ezynda3
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:29-30
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In test code, using hardcoded placeholder private keys is acceptable and should not be flagged as an issue.
🔇 Additional comments (2)
test/solidity/Facets/RelayFacet.t.sol (2)
8-12: LGTM: Reverter contract is well-designed for testing
The contract provides a clean way to simulate failing transactions, which is essential for testing error handling scenarios.
14-34: LGTM: TestRelayFacet provides appropriate testing extensions
The contract properly extends RelayFacet with testing-specific functionality while maintaining the core behavior. The additional methods for DEX management and function approvals are well-structured.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/Facets/RelayFacet.sol (4)
33-34: Fix NatSpec param documentation format.The parameter documentation uses incorrect format
@paramsinstead of@param.Apply this diff to fix the documentation:
- /// @params receivingAssetId address of receiving asset - /// @params signature attestation signature provided by the Relay solver + /// @param receivingAssetId address of receiving asset + /// @param signature attestation signature provided by the Relay solver
67-69: Enhance signature verification documentation.While the API URL is provided, it would be helpful to document the expected signature format and any requirements for the attestation process.
Add additional documentation about:
- Expected signature format (e.g., r, s, v components)
- Any specific requirements for the attestation process
- Example of a valid signature for testing purposes
103-151: Consider documenting the refund process.While the methods correctly handle native token refunds via the
refundExcessNativemodifier, it would be helpful to document:
- When refunds occur
- How excess amounts are calculated
- Any minimum/maximum thresholds
211-225: Consider using a mapping for chain IDs.While the current implementation works, using a mapping would be more maintainable and easier to update when adding new chains.
Consider refactoring to use a mapping:
+ mapping(uint256 => uint256) private chainIdMapping; + + constructor(address _relayReceiver, address _relaySolver) { + relayReceiver = _relayReceiver; + relaySolver = _relaySolver; + + // Bitcoin + chainIdMapping[20000000000001] = 8253038; + // Solana + chainIdMapping[1151111081099710] = 792703809; + } + function _getMappedChainId( uint256 chainId ) internal view returns (uint256) { - // Bitcoin - if (chainId == 20000000000001) { - return 8253038; - } - - // Solana - if (chainId == 1151111081099710) { - return 792703809; - } - - return chainId; + return chainIdMapping[chainId] == 0 ? chainId : chainIdMapping[chainId]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Facets/RelayFacet.sol(1 hunks)test/solidity/Facets/RelayFacet.t.sol(1 hunks)
🧰 Additional context used
📓 Learnings (2)
src/Facets/RelayFacet.sol (1)
Learnt from: ezynda3
PR: lifinance/contracts#843
File: src/Facets/RelayFacet.sol:165-170
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In the `RelayFacet` contract (`src/Facets/RelayFacet.sol`), within the `_startBridge` function, low-level `call` is intentionally used to transfer tokens so that extra bytes can be added to the calldata, as required for integrating with Relay Protocol.
test/solidity/Facets/RelayFacet.t.sol (2)
Learnt from: ezynda3
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:185-187
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In this codebase, changing the order of signature components from (r, s, v) to (v, r, s) is not required.
Learnt from: ezynda3
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:29-30
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In test code, using hardcoded placeholder private keys is acceptable and should not be flagged as an issue.
🔇 Additional comments (17)
src/Facets/RelayFacet.sol (3)
1-23: LGTM! Contract structure and imports are well-organized.
The contract inherits appropriate base contracts for security (ReentrancyGuard), functionality (SwapperV2), and validation (Validatable). All imports are utilized effectively.
44-52: LGTM! Events and errors are well-defined.
The events use appropriate indexed parameters for efficient filtering, and the error is descriptive.
96-99: LGTM! Constructor implementation is straightforward.
The constructor correctly sets the immutable addresses for the relay receiver and solver.
test/solidity/Facets/RelayFacet.t.sol (14)
8-12: Reverter Contract Implemented Correctly
The Reverter contract is appropriately designed to always revert transactions, which is useful for testing error propagation in the RelayFacetTest.
14-38: TestRelayFacet Extends RelayFacet Properly
The TestRelayFacet contract effectively extends RelayFacet and includes necessary test functions such as addDex, setFunctionApprovalBySignature, getMappedChainId, and setConsumedId.
45-46: Usage of Hardcoded Private Key in Tests
The hardcoded PRIVATE_KEY is acceptable in the context of this test code, as per the project's conventions.
51-93: Test Environment Initialized Successfully in setUp Function
The setUp function correctly initializes the test environment, including setting up custom block numbers, deploying the TestRelayFacet, adding function selectors, and initializing validRelayData.
123-134: Test for Invalid Signature Works as Expected
The testRevert_BridgeWithInvalidSignature function properly tests the contract's behavior when provided with an invalid signature.
148-176: Bridging Tokens to Solana Is Tested Correctly
The test_CanBridgeTokensToSolana function accurately sets up the bridge data for a Solana transfer and checks for the correct event emissions.
178-205: Replaying Transaction IDs Results in Proper Revert
The testRevert_WhenReplayingTransactionIds function ensures that replay attacks using the same requestId are correctly prevented by the contract.
207-248: Bridging Native Tokens to Solana Is Handled Properly
The test_CanBridgeNativeTokensToSolana function tests the bridging of native tokens to Solana, correctly adjusting the bridgeData and asserting balance changes.
250-308: Swapping and Bridging Tokens to Solana Functions Correctly
The test_CanSwapAndBridgeTokensToSolana function validates the swap from DAI to USDC followed by bridging to Solana, ensuring events are emitted and balances are updated appropriately.
404-440: Bridging Tokens to Bitcoin Is Implemented Correctly
The test_CanBridgeTokensToBitcoin function sets the correct parameters for a Bitcoin transfer and verifies the expected outcomes.
442-479: Bridging Native Tokens to Bitcoin Is Tested Accurately
The test_CanBridgeNativeTokensToBitcoin function ensures that native token bridging to Bitcoin works as intended.
629-659: Revert Behavior Is Properly Bubbled Up When Bridging Tokens Fails
The testFail_RevertIsBubbledWhenBridgingTokensFails function accurately simulates a failure during token bridging and checks that the revert message is correctly propagated.
661-684: Revert Behavior Is Properly Bubbled Up When Bridging Native Tokens Fails
The testFail_RevertIsBubbledWhenBridgingNativeTokensFails function effectively tests the revert propagation when bridging native tokens fails.
703-729: Signature Generation in signData Function Is Consistent with Codebase Standards
The signData function implements the signature generation using (r, s, v) order, which is acceptable for this codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/Facets/RelayFacet.sol (2)
30-34: Fix documentation typos and add missing parameter description.The struct documentation has the following issues:
@paramsshould be@param- Missing documentation for the
signatureparameterApply this diff to fix the documentation:
/// @param requestId Relay API request ID /// @param nonEVMReceiver set only if bridging to non-EVM chain - /// @params receivingAssetId address of receiving asset - /// @params signature attestation signature provided by the Relay solver + /// @param receivingAssetId address of receiving asset + /// @param signature attestation signature provided by the Relay solver
222-229: Use constants for chain IDs.The chain IDs are currently hardcoded as magic numbers. Using named constants would improve code maintainability and readability.
Apply this diff to improve the code:
+ // Chain ID constants + uint256 private constant LIFI_BITCOIN_CHAIN_ID = 20000000000001; + uint256 private constant RELAY_BITCOIN_CHAIN_ID = 8253038; + uint256 private constant LIFI_SOLANA_CHAIN_ID = 1151111081099710; + uint256 private constant RELAY_SOLANA_CHAIN_ID = 792703809; function _getMappedChainId( uint256 chainId ) internal pure returns (uint256) { // Bitcoin - if (chainId == 20000000000001) { - return 8253038; + if (chainId == LIFI_BITCOIN_CHAIN_ID) { + return RELAY_BITCOIN_CHAIN_ID; } // Solana - if (chainId == 1151111081099710) { - return 792703809; + if (chainId == LIFI_SOLANA_CHAIN_ID) { + return RELAY_SOLANA_CHAIN_ID; } return chainId; }test/solidity/Facets/RelayFacet.t.sol (1)
710-725: Enhance Readability by Defining Constants for Chain IDs and Mapped ValuesIn the
test_mapsCorrectChainIdfunction, consider defining constants for the known chain IDs and their corresponding mapped values. This change will improve readability and maintainability, making it easier for developers to understand and update the code.Apply this diff to define constants:
+ uint256 constant BITCOIN_CHAIN_ID = 20000000000001; + uint256 constant BITCOIN_MAPPED_ID = 8253038; + uint256 constant SOLANA_CHAIN_ID = 1151111081099710; + uint256 constant SOLANA_MAPPED_ID = 792703809; function test_mapsCorrectChainId(uint256 chainId) public { uint256 mapped = relayFacet.getMappedChainId(chainId); // Bitcoin - if (chainId == 20000000000001) { - assertEq(mapped, 8253038); + if (chainId == BITCOIN_CHAIN_ID) { + assertEq(mapped, BITCOIN_MAPPED_ID); return; } // Solana - if (chainId == 1151111081099710) { - assertEq(mapped, 792703809); + if (chainId == SOLANA_CHAIN_ID) { + assertEq(mapped, SOLANA_MAPPED_ID); return; } assertEq(mapped, chainId); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Facets/RelayFacet.sol(1 hunks)test/solidity/Facets/RelayFacet.t.sol(1 hunks)
🧰 Additional context used
📓 Learnings (2)
src/Facets/RelayFacet.sol (1)
Learnt from: ezynda3
PR: lifinance/contracts#843
File: src/Facets/RelayFacet.sol:165-170
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In the `RelayFacet` contract (`src/Facets/RelayFacet.sol`), within the `_startBridge` function, low-level `call` is intentionally used to transfer tokens so that extra bytes can be added to the calldata, as required for integrating with Relay Protocol.
test/solidity/Facets/RelayFacet.t.sol (1)
Learnt from: ezynda3
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:29-30
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In test code, using hardcoded placeholder private keys is acceptable and should not be flagged as an issue.
🔇 Additional comments (2)
src/Facets/RelayFacet.sol (1)
103-106: 🛠️ Refactor suggestion
Add zero-address validation in constructor.
The constructor should validate that neither _relayReceiver nor _relaySolver is the zero address to prevent potential deployment issues.
Apply this diff to add validation:
constructor(address _relayReceiver, address _relaySolver) {
+ if (_relayReceiver == address(0) || _relaySolver == address(0)) {
+ revert("RelayFacet: zero address");
+ }
relayReceiver = _relayReceiver;
relaySolver = _relaySolver;
}Likely invalid or redundant comment.
test/solidity/Facets/RelayFacet.t.sol (1)
1-774: Well-structured and Comprehensive Test Suite
The test file is well-organized and provides extensive coverage of the RelayFacet functionalities, including various bridging scenarios, error handling, and edge cases. The use of helper functions and thorough assertions ensures reliability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
audit/auditLog.json (1)
70-73: Consider documenting version bump criteria.The RelayFacet contract mapping is correctly structured and references the appropriate audit. However, for better version management, consider adding a VERSIONING.md file that documents the criteria for version bumps (major, minor, patch) specific to each contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
audit/reports/2024-12-02_RelayFacet(v1.0.0).pdfis excluded by!**/*.pdf
📒 Files selected for processing (1)
audit/auditLog.json(1 hunks)
🔇 Additional comments (1)
audit/auditLog.json (1)
Line range hint 1-86: LGTM! File structure maintains consistency.
The JSON structure is valid, well-formatted, and maintains consistency with existing entries.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
audit/auditLog.json(3 hunks)deployments/_deployments_log_file.json(1 hunks)deployments/arbitrum.staging.json(1 hunks)deployments/optimism.diamond.json(1 hunks)deployments/polygon.diamond.staging.json(2 hunks)deployments/polygon.staging.json(1 hunks)docs/README.md(1 hunks)package.json(1 hunks)script/deploy/resources/deployRequirements.json(1 hunks)
🔥 Files not summarized due to errors (1)
- deployments/_deployments_log_file.json: Error: Server error: no LLM provider could handle the message
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/README.md
- deployments/polygon.staging.json
- deployments/arbitrum.staging.json
- package.json
- script/deploy/resources/deployRequirements.json
- deployments/polygon.diamond.staging.json
- deployments/optimism.diamond.json
🧰 Additional context used
🪛 Biome (1.9.4)
audit/auditLog.json
[error] 68-68: The key EmergencyPauseFacet was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (4)
deployments/_deployments_log_file.json (1)
26130-26172:
Critical: Identical RelayFacet contract address across networks requires verification
The deployment logs show the RelayFacet contract (v1.0.0) deployed with the same address 0x3cf7dE0e31e13C93c8Aada774ADF1C7eD58157f5 across Arbitrum, Polygon, and Optimism networks. This is highly unusual since:
- The empty SALT value indicates CREATE2 wasn't used
- Different deployment timestamps suggest separate deployments
- Non-deterministic deployments should result in different addresses
Please verify the actual deployed addresses using this script:
#!/bin/bash
# Description: Verify actual deployed addresses and deployment mechanism
# Check deployment scripts for address handling
echo "Checking deployment scripts for address tracking..."
rg -A 5 "RelayFacet.*deploy"
# Search for potential proxy or implementation patterns
echo "Checking for proxy patterns..."
ast-grep --pattern 'contract $_ is $$$Proxy$$$'
# Search config files for these addresses
echo "Checking config files for constructor arg addresses..."
rg -i "a5f565650890fba1824ee0f21ebbbf660a179934|f70da97812cb96acdf810712aa562db8dfa3dbef" "config/"Action items:
- Verify the actual deployed addresses on each network using block explorers
- Review the deployment script's address tracking logic
- Update the deployment logs with correct network-specific addresses
- Consider adding network validation checks in deployment scripts
audit/auditLog.json (3)
52-58:
The audit entry states the audit was completed on 02.12.2024, but the referenced commit (291d0a7) is dated 12.11.2024. This ~20-day discrepancy needs clarification:
- Either the audit commit hash should point to a more recent commit that reflects the final audited state
- Or the audit completion date needs to be corrected if it was actually completed earlier
56-56: Verify audit report file exists
Please ensure the referenced audit report file exists at the specified path.
✅ Verification successful
Audit report file verification successful
The referenced audit report file exists at the specified path ./audit/reports/2024-12-02_RelayFacet(v1.0.0).pdf.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the audit report exists
if [ -f "./audit/reports/2024-12-02_RelayFacet(v1.0.0).pdf" ]; then
echo "Audit report found"
else
echo "Audit report missing"
fi
Length of output: 111
110-113: Verify RelayFacet version matches audited code
Given the audit commit date discrepancy, please verify that version 1.0.0 of RelayFacet matches the code that was actually audited at commit 291d0a7.
✅ Verification successful
RelayFacet version matches the audit log entry
The version 1.0.0 specified in the audit log matches the contract's version annotation (@custom:version 1.0.0) in the source code at src/Facets/RelayFacet.sol.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the version in the contract matches the audit log
rg -A 5 'contract\s+RelayFacet' | grep -A 5 'version'
# Check git history for version changes
git log -p --since="2024-11-12" -G "contract\s+RelayFacet" --follow
Length of output: 53
Script:
#!/bin/bash
# Search for RelayFacet contract definition
rg -A 5 'contract\s+RelayFacet'
# Find all files containing RelayFacet contract
fd -e sol -x grep -l "contract RelayFacet" {}
Length of output: 1048
Script:
#!/bin/bash
# Check the content of RelayFacet.sol for version information
cat src/Facets/RelayFacet.sol | head -n 20
# Look for version/versioning related comments or annotations
rg -i "version|v1\.0\.0" src/Facets/RelayFacet.sol
Length of output: 964
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
test/solidity/Facets/RelayFacet.t.sol (2)
140-563: Refactor Test Functions to Reduce Code DuplicationThe test functions for bridging and swapping tokens to Solana and Bitcoin (
test_CanBridgeTokensToSolana,test_CanBridgeNativeTokensToSolana,test_CanSwapAndBridgeTokensToSolana,test_CanSwapAndBridgeNativeTokensToSolana,test_CanBridgeTokensToBitcoin,test_CanBridgeNativeTokensToBitcoin,test_CanSwapAndBridgeTokensToBitcoin,test_CanSwapAndBridgeNativeTokensToBitcoin) contain substantial code duplication in their setup and execution.Consider abstracting the common setup code into reusable helper functions or using parameterized tests. This will improve maintainability and make future updates easier.
714-729: Define Chain ID Mappings as Constants for ClarityThe
test_mapsCorrectChainIdfunction uses hardcoded magic numbers for certain chain IDs and their mappings (e.g.,20000000000001to8253038). This can make the code harder to read and maintain.Define the chain IDs and their mapped values as constants with descriptive names. For example:
uint256 constant BITCOIN_CHAIN_ID = 20000000000001; uint256 constant BITCOIN_MAPPED_ID = 8253038;This enhances readability and makes future updates or additions clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/solidity/Facets/RelayFacet.t.sol(1 hunks)
🧰 Additional context used
📓 Learnings (1)
test/solidity/Facets/RelayFacet.t.sol (3)
Learnt from: ezynda3
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:0-0
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In the `signData` function within `RelayFacetTest` in `test/solidity/Facets/RelayFacet.t.sol`, the use of the EIP-712 standard for hashing and signing is not required.
Learnt from: ezynda3
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:29-30
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In test code, using hardcoded placeholder private keys is acceptable and should not be flagged as an issue.
Learnt from: ezynda3
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:185-187
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In this codebase, changing the order of signature components from (r, s, v) to (v, r, s) is not required.
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)
Example TXs:
Bridge ETH: https://arbiscan.io/tx/0xe8f3840bc57816aabf4a6979f26c37b37e08ae28c6a149789442e26cdc79c425
Bridge USDC: https://arbiscan.io/tx/0x13acf381bed55da6b210275a8ff4ca9532cb692586a5f3c23f63ac79e7859dd5
Swap USDC and bridge ETH https://arbiscan.io/tx/0x35e33b9ec453763ffbe01c6fb5428bbf1052b5add1a63e4d9365e82f10fb6418
Bridge USDC to Solana: https://arbiscan.io/tx/0x0414fd29f753463f14013785b049e86e2e7ce25f7f45d1c1fb93155900f00d22