deployed stargate to gnosis #993
Conversation
WalkthroughThis pull request updates several JSON configuration and deployment files. In the configuration file, the Changes
Possibly related PRs
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 1
🧹 Nitpick comments (1)
deployments/_deployments_log_file.json (1)
24472-24473: Consider using deterministic deployment with SALT.The
SALTfield is empty for both deployments. Consider using a deterministic deployment approach with CREATE2 by providing a SALT value. This helps in verifying that the same contract is deployed across different environments.Also applies to: 24766-24767
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/stargate.json(1 hunks)deployments/_deployments_log_file.json(2 hunks)deployments/gnosis.json(1 hunks)script/deploy/_targetState.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
script/deploy/_targetState.json (2)
Learnt from: mirooon
PR: lifinance/contracts#985
File: script/deploy/_targetState.json:0-0
Timestamp: 2025-02-12T09:44:12.961Z
Learning: The bsca network intentionally maintains different facet versions between staging and production environments, specifically:
1. CalldataVerificationFacet: v1.1.1 in staging vs v1.1.2 in production
2. EmergencyPauseFacet: present only in production
3. Permit2Proxy: present only in production
Learnt from: ezynda3
PR: lifinance/contracts#807
File: script/deploy/_targetState.json:164-164
Timestamp: 2024-12-03T11:01:57.084Z
Learning: Version differences in `CalldataVerificationFacet` between staging and production are acceptable and not an issue.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: generate-tag
🔇 Additional comments (5)
deployments/gnosis.json (1)
38-40: LGTM! Stargate facets deployed to gnosis.The addresses for
StargateFacetV2andReceiverStargateV2match the configuration inscript/deploy/_targetState.jsonfor the gnosis production environment.config/stargate.json (1)
57-57: Note: Fantom is not supported by Stargate V2.The configuration indicates that Fantom is not supported by Stargate V2. Please ensure that any existing integrations with Fantom are properly handled or disabled.
script/deploy/_targetState.json (1)
265-267: Verify version differences between environments.The gnosis environment uses different versions compared to other environments:
- StargateFacetV2: v2.2.0 (others use v1.0.1)
- ReceiverStargateV2: v1.1.0 (others use v1.0.0 or v1.1.0)
Please confirm if these version differences are intentional.
Run the following script to check version differences:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check version differences for StargateFacetV2 and ReceiverStargateV2 across environments. echo "StargateFacetV2 versions:" rg -A 1 '"StargateFacetV2":' echo -e "\nReceiverStargateV2 versions:" rg -A 1 '"ReceiverStargateV2":'Length of output: 22645
Confirmed: Version discrepancies exist between environments.
The grep output shows that while most environment configurations in
script/deploy/_targetState.jsonuse:
- StargateFacetV2: "1.0.1"
- ReceiverStargateV2: "1.0.0"
there is a distinct block—presumably for the gnosis environment—that sets:
- StargateFacetV2: "2.2.0"
- ReceiverStargateV2: "1.1.0"
Please verify with the team that these version differences are intentional.
deployments/_deployments_log_file.json (2)
24470-24470:⚠️ Potential issueFix incorrect deployment timestamps.
Both deployment entries have timestamps set in the future (2025). This appears to be incorrect as deployments should have current or past timestamps.
Please update the timestamps to reflect the actual deployment dates.
Also applies to: 24764-24764
⛔ Skipped due to learnings
Learnt from: ezynda3 PR: lifinance/contracts#823 File: deployments/_deployments_log_file.json:10542-10543 Timestamp: 2024-11-21T08:17:27.878Z Learning: When reviewing deployment timestamps in `deployments/_deployments_log_file.json`, ensure that the timestamps are accurately compared with the current date to avoid incorrectly flagging valid timestamps as future deployment dates.Learnt from: ezynda3 PR: lifinance/contracts#861 File: deployments/_deployments_log_file.json:22488-22488 Timestamp: 2024-11-22T07:19:51.143Z Learning: In `deployments/_deployments_log_file.json`, deployment timestamps may appear to be set in the future and should not be flagged as invalid.
24471-24471: Verify constructor arguments and contract addresses.The constructor arguments contain multiple addresses that should be verified for correctness. Let's validate these addresses and their roles:
Also applies to: 24765-24765
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the addresses in constructor args and deployment addresses # Extract and decode constructor args echo "Version 1.0.1 constructor args:" echo "0x000000000000000000000000af368c91793cb22739386dfcbbb2f1a9e4bcbebf" | \ python3 -c "import sys; print('Address:', '0x' + sys.stdin.read().strip()[26:])" echo -e "\nVersion 1.1.0 constructor args (multiple addresses):" ARGS="0x000000000000000000000000156cebba59deb2cb23742f70dcb0a11cc775591f0000000000000000000000002dfadab8266483bed9fd9a292ce56596a2d1378d000000000000000000000000af368c91793cb22739386dfcbbb2f1a9e4bcbebf0000000000000000000000001a44076050125825900e736c501f859c50fe728c00000000000000000000000000000000000000000000000000000000000186a0" for i in {0..3}; do start=$((26 + i*64)) echo "Address $((i+1)): 0x${ARGS:$start:40}" done echo "Value: 0x${ARGS:282:64}" # Check if these addresses have code deployed echo -e "\nChecking deployment addresses on Gnosis chain:" echo "Version 1.0.1: 0xCB68AAcEb848E5007B56b1ff09D240CE8946B4EC" echo "Version 1.1.0: 0x0dC56076590CdF0efc37114273f02f45A96B5500"Length of output: 1798
Verification Complete: Constructor Arguments and Deployment Addresses Are Correct
- Version 1.0.1 constructor arg correctly decodes to:
0xaf368c91793cb22739386dfcbbb2f1a9e4bcbebf- Version 1.1.0 constructor arguments correctly extract the following addresses:
• Address 1: 0x156cebba59deb2cb23742f70dcb0a11cc775591f
• Address 2: 0x2dfadab8266483bed9fd9a292ce56596a2d1378d
• Address 3: 0xaf368c91793cb22739386dfcbbb2f1a9e4bcbebf
• Address 4: 0x1a44076050125825900e736c501f859c50fe728c- The parsed value is as expected (0x00000000000000000000000000000000000186a0)
- Deployment addresses on the Gnosis chain are correctly listed:
• Version 1.0.1: 0xCB68AAcEb848E5007B56b1ff09D240CE8946B4EC
• Version 1.1.0: 0x0dC56076590CdF0efc37114273f02f45A96B5500All extracted addresses and deployment addresses appear to have been processed correctly. No issues were identified during the verification.
Test Coverage ReportLine Coverage: 79.05% (2249 / 2845 lines) |
…rgateV2-to-gnosis
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 (5)
config/stargate.json(1 hunks)deployments/_deployments_log_file.json(2 hunks)deployments/gnosis.diamond.json(2 hunks)deployments/gnosis.json(1 hunks)script/deploy/_targetState.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- deployments/gnosis.json
- deployments/_deployments_log_file.json
- config/stargate.json
🧰 Additional context used
🧠 Learnings (1)
script/deploy/_targetState.json (2)
Learnt from: mirooon
PR: lifinance/contracts#985
File: script/deploy/_targetState.json:0-0
Timestamp: 2025-02-12T09:44:12.961Z
Learning: The bsca network intentionally maintains different facet versions between staging and production environments, specifically:
1. CalldataVerificationFacet: v1.1.1 in staging vs v1.1.2 in production
2. EmergencyPauseFacet: present only in production
3. Permit2Proxy: present only in production
Learnt from: ezynda3
PR: lifinance/contracts#807
File: script/deploy/_targetState.json:164-164
Timestamp: 2024-12-03T11:01:57.084Z
Learning: Version differences in `CalldataVerificationFacet` between staging and production are acceptable and not an issue.
🪛 Biome (1.9.4)
deployments/gnosis.diamond.json
[error] 100-100: The key 0x5656E307E4dEd573695Dd3eAC64EC4778F910DcA 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 (3)
deployments/gnosis.diamond.json (2)
112-115: New facet addition: Validate new StargateFacetV2 entry.The addition at lines 112–115 correctly introduces the
StargateFacetV2with version"1.0.1". Please confirm that this deployment address is correct and that it has been coordinated with the changes in other configuration files (such as indeployments/gnosis.jsonand related deployment scripts).
117-127: Update to ReceiverStargateV2 configuration.The change in the
"Periphery"section for"ReceiverStargateV2"(line 127) from an empty string to"0x0dC56076590CdF0efc37114273f02f45A96B5500"is clear. Ensure that this new address is aligned with the deployed receiver contract and that it is consistent with all environment configurations.script/deploy/_targetState.json (1)
265-267: Gnosis environment update: Confirm version bumps for Stargate components.Within the
"gnosis"production section, the following updates are noted:
"StargateFacetV2"has been bumped from its previous version (likely"1.0.1") to"2.2.0"."ReceiverStargateV2"has been updated from"1.0.0"to"1.1.0".These changes appear to reflect the intended deployment of the new Stargate facet to gnosis. Please verify that these new versions match the deployed contracts and their associated metadata.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
test/solidity/Facets/DexManagerFacet.t.sol (1)
32-45:⚠️ Potential issueFix array size mismatch in functionSelectors.
The array is declared with size 9 but only 8 selectors are assigned (indexes 0-7).
Apply this diff to fix the array size:
- bytes4[] memory functionSelectors = new bytes4[](9); + bytes4[] memory functionSelectors = new bytes4[](8);
🧹 Nitpick comments (3)
test/solidity/Helpers/TransferrableOwnership.t.sol (1)
32-42: Test naming and error handling improvements look great.The renaming from
testFail*totestRevert_*and the addition of specific error expectations improve test clarity and maintainability.However, consider adding these additional test cases to improve coverage:
- Test canceling a pending ownership transfer
- Test multiple ownership transfer attempts
Also applies to: 44-50, 52-62, 64-70
test/solidity/Facets/OwnershipFacet.t.sol (1)
32-55: Good event emission verification in ownership transfer test.The test properly verifies both OwnershipTransferRequested and OwnershipTransferred events.
Consider adding these test cases:
- Test event emission when canceling ownership transfer
- Test event ordering in ownership transfer flow
test/solidity/Facets/AcrossFacet.t.sol (1)
111-128: Improved test clarity and realism.The changes enhance the test by:
- Using a more descriptive name
- Adding missing token approval
- Using a significantly outdated quote (100 days) to clearly demonstrate the timeout
Consider adding these test cases:
- Test with quote timestamp just slightly past the timeout
- Test with future quote timestamp
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
deployments/gnosis.diamond.json(2 hunks)script/deploy/facets/utils/DeployScriptBase.sol(0 hunks)test/solidity/Facets/AcrossFacet.t.sol(1 hunks)test/solidity/Facets/AcrossFacetV3.t.sol(4 hunks)test/solidity/Facets/CBridgeRefund.t.sol(6 hunks)test/solidity/Facets/DexManagerFacet.t.sol(5 hunks)test/solidity/Facets/OwnershipFacet.t.sol(1 hunks)test/solidity/Facets/RelayFacet.t.sol(3 hunks)test/solidity/Helpers/TransferrableOwnership.t.sol(2 hunks)test/solidity/Periphery/Executor.t.sol(3 hunks)test/solidity/Periphery/FeeCollector.t.sol(5 hunks)
💤 Files with no reviewable changes (1)
- script/deploy/facets/utils/DeployScriptBase.sol
🧰 Additional context used
🧠 Learnings (1)
test/solidity/Facets/RelayFacet.t.sol (2)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:212-213
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In the `GasZipFacetTest`, for the test case `testBase_Revert_SwapAndBridgeWithInvalidSwapData()`, a generic revert is expected, so `vm.expectRevert();` without specifying the expected error is appropriate.
Learnt from: 0xDEnYO
PR: lifinance/contracts#843
File: test/solidity/Facets/RelayFacet.t.sol:657-712
Timestamp: 2024-12-13T03:02:57.533Z
Learning: According to the team's style guide (https://www.notion.so/lifi/Style-Guide-a078a59a6e7144268d5758ba7455bbcb?pvs=4#dbca29533b7e4beaa1c9001fa9bedeca), `testFail_` is a naming convention used for test functions in this repository and does not refer to Foundry's `testFail_` functionality.
⏰ 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 (22)
test/solidity/Facets/DexManagerFacet.t.sol (2)
9-10: LGTM! Good addition of access management and error handling.The introduction of AccessManagerFacet and error types enhances the contract's authorization capabilities and error handling.
Also applies to: 20-20
126-238: LGTM! Comprehensive test coverage for error cases and authorization.The new test cases effectively verify:
- Error handling for invalid inputs
- Authorization checks
- Access management functionality
- Batch operations with proper validation
The clear naming convention with "testRevert_" prefix makes the test intentions explicit.
test/solidity/Helpers/TransferrableOwnership.t.sol (1)
13-16: LGTM! Well-defined error types for ownership scenarios.The error types are clear, specific, and cover essential ownership transfer edge cases.
test/solidity/Facets/OwnershipFacet.t.sol (2)
11-14: LGTM! Consistent error types with TransferrableOwnership.The error types match those in TransferrableOwnership.t.sol, maintaining consistency across the codebase.
16-24: Well-structured events for ownership transfer tracking.The events provide clear tracking of ownership transfer requests and completions.
test/solidity/Facets/CBridgeRefund.t.sol (2)
31-31: LGTM! Clear error type for withdrawal failures.The WithdrawFailed error provides clear indication of refund processing issues.
132-144: Excellent improvements to refund test cases.The changes:
- Follow consistent naming convention with testRevert_*
- Add specific error expectations
- Cover key scenarios: unauthorized access, invalid addresses, and duplicate refunds
The test coverage for refund scenarios is comprehensive.
Also applies to: 148-160, 165-192
test/solidity/Periphery/FeeCollector.t.sol (4)
9-9: LGTM! Added import for UnAuthorized error.The import aligns with the enhanced error handling approach.
188-209: LGTM! Improved test naming and error handling.The test function has been renamed to follow the
testRevert_pattern and now usesvm.expectRevert(UnAuthorized.selector)for better error handling. The changes align with the team's style guide.
211-242: LGTM! Enhanced error handling for batch withdrawal test.The test function has been renamed to follow the
testRevert_pattern and now usesvm.expectRevert(UnAuthorized.selector)for better error handling.
254-264: LGTM! Improved ownership transfer test.The test function has been renamed to follow the
testRevert_pattern and now usesvm.expectRevert(UnAuthorized.selector)for better error handling.test/solidity/Facets/AcrossFacetV3.t.sol (4)
4-4: LGTM! Updated imports.Removed ERC20 import from TestBaseFacet and added InformationMismatch error import.
Also applies to: 9-9
40-40: LGTM! Added InvalidQuoteTimestamp error.The error enhances the contract's error handling capabilities.
255-268: LGTM! Improved quote timestamp validation test.The test function has been renamed to follow the
testRevert_pattern and now usesvm.expectRevert(InvalidQuoteTimestamp.selector)for better error handling.
280-297: LGTM! Added test for receiver mismatch.New test ensures that the bridge data receiver matches with the Across data receiver, improving test coverage.
test/solidity/Periphery/Executor.t.sol (2)
13-13: LGTM! Added import for UnAuthorized error.The import aligns with the enhanced error handling approach.
551-585: LGTM! Improved ERC20Proxy direct call test.The test function has been renamed to follow the
testRevert_pattern, now uses ERC20Proxy instead of AMM, and includesvm.expectRevert(UnAuthorized.selector)for better error handling.test/solidity/Facets/RelayFacet.t.sol (2)
657-684: LGTM! Improved ERC20 token bridging failure test.The test function has been renamed to follow the
testRevert_pattern and now uses a genericvm.expectRevert()without specifying the error message, which aligns with the team's approach for handling generic reverts.
686-706: LGTM! Improved native token bridging failure test.The test function has been renamed to follow the
testRevert_pattern and now uses a genericvm.expectRevert()without specifying the error message.deployments/gnosis.diamond.json (3)
108-111: New Facet Addition:StargateFacetV2
The new facetStargateFacetV2with version"1.0.1"at address"0xCB68AAcEb848E5007B56b1ff09D240CE8946B4EC"is correctly added. Please verify that this address and version match the intended deployment and that all related contract metadata/documentation reflect this update.
123-123: ReceiverStargateV2 Address Update
TheReceiverStargateV2field under thePeripherysection is now populated with"0x0dC56076590CdF0efc37114273f02f45A96B5500", replacing the previous empty string. Confirm that this is the correct and active receiver address used in external contract calls and deployment scripts.
1-129: Overall JSON Structure and Consistency
The JSON configuration appears well-formed with unique keys and clear separation of theFacetsandPeripheryresponsibilities. It is advisable, as part of your deployment checklist, to cross-check these configuration entries against related files (e.g.,deployments/gnosis.jsonand your deployment logs) to ensure consistency across environments.
…rgateV2-to-gnosis
Which Jira task belongs to this PR?
https://lifi.atlassian.net/browse/LF-12362
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)