Add new function selectors to MayanFacet [MayanFacet v1.1.0] #1032
Conversation
lifi-action-bot
changed the title
WalkthroughThe changes include the removal of the import statements for Changes
Possibly related PRs
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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: 0
🧹 Nitpick comments (3)
src/Facets/MayanFacet.sol (3)
250-253: Consider adding more detailed comments with parameter layout.For easier maintenance and understanding, consider adding more detailed comments showing the full parameter layout for each function.
- case 0x1c59b7fc { - // 0x1c59b7fc MayanCircle::createOrder((address,uint256,uint64,bytes32,uint16,bytes32,uint64,uint64,uint64,bytes32,uint8)) - receiver := mload(add(protocolData, 0x84)) - } + case 0x1c59b7fc { + // 0x1c59b7fc MayanCircle::createOrder((address,uint256,uint64,[*bytes32*],uint16,bytes32,uint64,uint64,uint64,bytes32,uint8)) + // Location 0x84 (132 bytes): After function selector (4) + address (32) + uint256 (32) + uint64 (32) = 100 bytes + alignment = 132 + receiver := mload(add(protocolData, 0x84)) + }
254-257: Consider adding more detailed comments with parameter layout.For easier maintenance and understanding, consider adding more detailed comments showing the full parameter layout for this function.
- case 0x9be95bb4 { - // 0x9be95bb4 MayanCircle::bridgeWithLockedFee(address,uint256,uint64,uint256,uint32,bytes32) - receiver := mload(add(protocolData, 0xc4)) - } + case 0x9be95bb4 { + // 0x9be95bb4 MayanCircle::bridgeWithLockedFee(address,uint256,uint64,uint256,uint32,[*bytes32*]) + // Location 0xc4 (196 bytes): After function selector (4) + address (32) + uint256 (32) + uint64 (32) + uint256 (32) + uint32 (32) = 164 bytes + alignment = 196 + receiver := mload(add(protocolData, 0xc4)) + }
258-261: Consider adding more detailed comments with parameter layout.For easier maintenance and understanding, consider adding more detailed comments showing the full parameter layout for this function.
- case 0x2072197f { - // 0x2072197f MayanCircle::bridgeWithFee(address,uint256,uint64,uint64,bytes32,uint32,uint8,bytes) - receiver := mload(add(protocolData, 0xa4)) - } + case 0x2072197f { + // 0x2072197f MayanCircle::bridgeWithFee(address,uint256,uint64,uint64,[*bytes32*],uint32,uint8,bytes) + // Location 0xa4 (164 bytes): After function selector (4) + address (32) + uint256 (32) + uint64 (32) + uint64 (32) = 132 bytes + alignment = 164 + receiver := mload(add(protocolData, 0xa4)) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Facets/MayanFacet.sol(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (4)
src/Facets/MayanFacet.sol (4)
9-9: LGTM: Addition of LibBytes import.The LibBytes library import is appropriate for handling bytes manipulation operations in this contract.
19-19: Version bump from 1.0.0 to 1.1.0 is appropriate.The version increment follows semantic versioning principles as the PR adds new functionality (additional function selectors) without breaking existing behavior.
250-261: LGTM: New function selectors correctly implemented.The implementation of three new case statements for MayanCircle function selectors follows the established pattern in the contract for extracting receiver addresses from protocol data.
Each selector correctly identifies the memory offset where the receiver address should be located in the protocol data:
0x1c59b7fc(createOrder): Offset at 0x840x9be95bb4(bridgeWithLockedFee): Offset at 0xc40x2072197f(bridgeWithFee): Offset at 0xa4
21-21:❓ Verification inconclusive
Good addition of LibBytes library usage.
The
using LibBytes for bytes;directive enables the contract to use LibBytes functions as methods on bytes objects, which is a good practice for code readability and maintainability.However, I don't see explicit usage of LibBytes functions in the code. Is this being used elsewhere or prepared for future implementation?
🏁 Script executed:
#!/bin/bash # Check if LibBytes functions are actually used in the MayanFacet contract # Search for common LibBytes functions like concat, slice, toAddress, etc. echo "Searching for LibBytes function usage in MayanFacet.sol:" rg -n "\.(concat|slice|toAddress|fromAddress|toUint)" src/Facets/MayanFacet.sol || echo "No direct usage found" # Check if the library is being used in _replaceInputAmount or other functions echo -e "\nChecking if LibBytes functions could be used to improve _replaceInputAmount implementation:" cat src/Libraries/LibBytes.sol | grep -B 1 -A 3 "function" | head -20Length of output: 1002
LibBytes Addition – Clarification Needed
Great inclusion of the LibBytes directive (using LibBytes for bytes;), which sets a strong foundation for improved readability. However, our verification confirms that none of the LibBytes functions (e.g.,concat,slice,toAddress) are currently being invoked withinsrc/Facets/MayanFacet.sol. Please clarify whether these functions are intended for future use or if they may have been accidentally omitted. Consider adding a brief comment explaining the intended usage or, if unnecessary, removing the directive for clarity.
Test Coverage ReportLine Coverage: 78.31% (2026 / 2587 lines) |
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
deployments/arbitrum.diamond.staging.json (2)
148-151: New Deployment Entry with Empty Metadata.A new entry for address
0xF82830B952Bc60b93206FA22f1cD4770cedb2840is added with empty"Name"and"Version". Confirm if these fields are intended as placeholders or if they must be populated before deploying to production.
152-155: New Deployment Entry Missing Facet Information.The entry for address
0xDd661337B48BEA5194F6d26F2C59fF0855E15289has also been added with empty values for"Name"and"Version". Please verify whether this entry is upcoming functionality or if the metadata should be defined now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deployments/_deployments_log_file.json(1 hunks)deployments/arbitrum.diamond.staging.json(2 hunks)deployments/arbitrum.staging.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: generate-tag
🔇 Additional comments (6)
deployments/arbitrum.staging.json (1)
36-36: Contract Address Update: Validate MayanFacet Address Consistency
The updated address"0x6C96d5C36d9aDBD3F4e0337D2d1E133A59288D1A"for MayanFacet reflects the intended change as per the PR objectives and aligns with the new 1.1.0 version of the contract. Please ensure that this address is consistently updated across all dependent deployment files (e.g.,arbitrum.diamond.staging.jsonand_deployments_log_file.json) and has undergone thorough verification (on-chain validation and preliminary audit) to confirm its correctness.deployments/_deployments_log_file.json (1)
24267-24277: New Deployment Entry for Version "1.1.0"The new JSON entry for version "1.1.0" has been added correctly with all the required fields (ADDRESS, OPTIMIZER_RUNS, TIMESTAMP, CONSTRUCTOR_ARGS, SALT, and VERIFIED). Please ensure that the "VERIFIED" flag set to "true" accurately reflects the deployment's status, and double-check that the timestamp and constructor arguments align with the actual deployment configuration. Additionally, confirm that this change is consistently applied across other related deployment files (such as
deployments/arbitrum.diamond.staging.jsonanddeployments/arbitrum.staging.json) as referenced in the PR objectives.deployments/arbitrum.diamond.staging.json (4)
141-142: Facet Name Update for DeBridgeDlnFacet.The entry for address
0xE15C7585636e62b88bA47A40621287086E0c2E33has been updated with"Name": "DeBridgeDlnFacet"and"Version": "1.0.0". Please ensure that all contract references and documentation are in sync with this updated facet identifier.
156-158: MayanFacet Version Bump.The entry for address
0x6C96d5C36d9aDBD3F4e0337D2d1E133A59288D1Anow reflects"Name": "MayanFacet"and an updated"Version": "1.1.0", matching the intended changes in the MayanFacet contract. Ensure that any dependent systems or integration tests are updated accordingly.
168-168: Updated Permit2Proxy Address.The
"Permit2Proxy"field is updated to"0xb33Fe241BEd9bf5F694101D7498F63a0d060F999". Please double-check that all external integrations referencing this address are adjusted to the new value.
169-173: Removal of Deprecated 'Receiver' Entry.The PR summary indicates that the
"Receiver"entry has been removed. Please confirm that any logic or integrations previously relying on the"Receiver"field are updated accordingly to prevent any runtime discrepancies.🧰 Tools
🪛 Gitleaks (8.21.2)
172-172: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/solidity/Facets/MayanFacet.t.sol(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Enforce Min Test Coverage
test/solidity/Facets/MayanFacet.t.sol
[error] 497-497: Compiler run failed: Identifier not found or not unique.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: generate-tag
🔇 Additional comments (8)
test/solidity/Facets/MayanFacet.t.sol (8)
22-32: Good addition of a test wrapper for exposing internal functions.The
TestMayanFacetExposedcontract effectively exposes the internal_parseReceiverfunction for testing purposes, which follows good testing practices for internal functions.
43-44: Good use of a constant for non-EVM addresses.Using a constant
NON_EVM_ADDRESSinstead of hardcoded addresses across tests improves maintainability.
50-57: Well-structured custom errors and events.The custom errors and event definition follow good practices for error handling and event emission in testing.
312-314: Improved test function naming.The function name
testRevert_FailsIfNonEVMReceiverIsIncorrectfollows better naming conventions for test functions that expect reversions and uses the constant variable instead of hardcoded values.
410-441: Good test for non-EVM token bridging.The test properly validates the bridging functionality for native tokens to non-EVM chains including appropriate event emission checks.
443-463: Well-structured test for empty non-EVM receiver reversion.This test correctly validates the failure case when a non-EVM chain is targeted but the receiver is empty.
465-493: Good validation of receiver matching.This test ensures that the receiver in bridge data matches the receiver extracted from Mayan protocol data, which is an important security check.
497-509: Incomplete test implementation with commented assertion.The test function
test_ParseReceivercreates a test instance but has a commented-out assertion line, making this test ineffective at validating behavior.Uncomment and fix the assertion to properly test the
_parseReceiverfunction. Consider adding assertions for the other test cases mentioned in the comments.🧰 Tools
🪛 GitHub Actions: Enforce Min Test Coverage
[error] 497-497: Compiler run failed: Identifier not found or not unique.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/solidity/Facets/MayanFacet.t.sol (1)
495-545: Excellent comprehensive testing of _parseReceiver function.The test_ParseReceiver function provides thorough coverage of different protocol data formats and function selectors. The use of real transaction data from Tenderly and detailed assertion messages makes this test robust and maintainable.
Consider extracting the large hex strings into constants or helper functions to improve readability for future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/solidity/Facets/MayanFacet.t.sol(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: generate-tag
🔇 Additional comments (8)
test/solidity/Facets/MayanFacet.t.sol (8)
22-32: Good addition of test-only contract to expose internal functionality.The TestMayanFacetExposed contract provides a clean way to test the internal _parseReceiver function without modifying the main contract. The code is well-documented with clear comments explaining its purpose.
43-44: Good practice using a constant over hardcoded values.Using NON_EVM_ADDRESS as a constant improves maintainability by centralizing the test value in one place.
50-57: Well-defined error types and events.The new error types and event definition follow a consistent style with the rest of the codebase and provide clear parameter names that make test assertions easier to validate.
312-313: Consistent test naming and use of constants.The rename to testRevert_FailsIfNonEVMReceiverIsIncorrect follows a more consistent naming pattern with other test functions, and using NON_EVM_ADDRESS constant improves readability.
410-441: Comprehensive test for non-EVM chain bridging.This test effectively validates the ability to bridge native tokens to non-EVM chains, properly expecting the BridgeToNonEVMChain event and verifying the final balance.
443-463: Good error case testing.This test properly validates the failure case when attempting to bridge to a non-EVM chain but the non-EVM receiver is empty.
465-493: Thorough validation of receiver matching.This test ensures that transactions fail when the bridge data receiver doesn't match the receiver in the Mayan protocol data, which is an important security check.
496-498: Verify the use of zero address for the IMayan interface.The test initializes TestMayanFacetExposed with IMayan(address(0)). While this may be acceptable for a test-only contract that only calls a pure function, verify that this doesn't lead to unexpected behaviors in other contexts.
Which Jira task belongs to this PR?
https://lifi.atlassian.net/browse/LF-12602
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)