chore(protocol): manage deps with node_modules instead of submodules #15759
Conversation
dionysuzx
changed the title
|
This looks good to me. So |
yes, that is all. and the dependencies are frozen by tag, just as before. |
say no more. ✅ |
|
Important Auto Review SkippedReview was skipped due to path filters Files ignored due to path filters (2)
WalkthroughThe recent updates across the protocol's contracts primarily focus on standardizing the import paths for OpenZeppelin contracts and other external libraries. By shifting to the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
packages/protocol/package.jsonis excluded by:!**/*.jsonpnpm-lock.yamlis excluded by:!**/*.yaml
Files selected for processing (60)
- packages/protocol/contracts/L1/TaikoToken.sol (1 hunks)
- packages/protocol/contracts/L1/gov/TaikoGovernor.sol (1 hunks)
- packages/protocol/contracts/L1/gov/TaikoTimelockController.sol (1 hunks)
- packages/protocol/contracts/L1/hooks/AssignmentHook.sol (1 hunks)
- packages/protocol/contracts/L1/libs/LibProposing.sol (1 hunks)
- packages/protocol/contracts/L1/libs/LibProving.sol (1 hunks)
- packages/protocol/contracts/L1/libs/LibVerifying.sol (1 hunks)
- packages/protocol/contracts/L2/CrossChainOwned.sol (1 hunks)
- packages/protocol/contracts/L2/TaikoL2.sol (1 hunks)
- packages/protocol/contracts/automata-attestation/AutomataDcapV3Attestation.sol (1 hunks)
- packages/protocol/contracts/automata-attestation/lib/PEMCertChainLib.sol (1 hunks)
- packages/protocol/contracts/common/OwnerUUPSUpgradable.sol (1 hunks)
- packages/protocol/contracts/libs/LibAddress.sol (1 hunks)
- packages/protocol/contracts/libs/LibDeploy.sol (1 hunks)
- packages/protocol/contracts/signal/SignalService.sol (1 hunks)
- packages/protocol/contracts/team/TimelockTokenPool.sol (1 hunks)
- packages/protocol/contracts/team/airdrop/ERC20Airdrop.sol (1 hunks)
- packages/protocol/contracts/team/airdrop/ERC20Airdrop2.sol (1 hunks)
- packages/protocol/contracts/team/airdrop/ERC721Airdrop.sol (1 hunks)
- packages/protocol/contracts/team/airdrop/MerkleClaimable.sol (1 hunks)
- packages/protocol/contracts/tokenvault/BaseVault.sol (1 hunks)
- packages/protocol/contracts/tokenvault/BridgedERC1155.sol (1 hunks)
- packages/protocol/contracts/tokenvault/BridgedERC20.sol (1 hunks)
- packages/protocol/contracts/tokenvault/BridgedERC20Base.sol (1 hunks)
- packages/protocol/contracts/tokenvault/BridgedERC721.sol (1 hunks)
- packages/protocol/contracts/tokenvault/ERC1155Vault.sol (1 hunks)
- packages/protocol/contracts/tokenvault/ERC20Vault.sol (1 hunks)
- packages/protocol/contracts/tokenvault/ERC721Vault.sol (1 hunks)
- packages/protocol/contracts/tokenvault/LibBridgedToken.sol (1 hunks)
- packages/protocol/contracts/verifiers/SgxVerifier.sol (1 hunks)
- packages/protocol/genesis/GenerateGenesis.g.sol (1 hunks)
- packages/protocol/remappings.txt (1 hunks)
- packages/protocol/script/DeployOnL1.s.sol (2 hunks)
- packages/protocol/script/SetAddress.s.sol (1 hunks)
- packages/protocol/script/SetDcapParams.s.sol (1 hunks)
- packages/protocol/script/upgrade/UpgradeAddressManager.s.sol (1 hunks)
- packages/protocol/script/upgrade/UpgradeAssignmentHook.s.sol (1 hunks)
- packages/protocol/script/upgrade/UpgradeBridge.s.sol (1 hunks)
- packages/protocol/script/upgrade/UpgradeERC1155Vault.s.sol (1 hunks)
- packages/protocol/script/upgrade/UpgradeERC20Vault.s.sol (1 hunks)
- packages/protocol/script/upgrade/UpgradeERC721Vault.s.sol (1 hunks)
- packages/protocol/script/upgrade/UpgradeGuardianProver.s.sol (1 hunks)
- packages/protocol/script/upgrade/UpgradeScript.s.sol (1 hunks)
- packages/protocol/script/upgrade/UpgradeSignalService.s.sol (1 hunks)
- packages/protocol/script/upgrade/UpgradeTaikoGovernor.s.sol (1 hunks)
- packages/protocol/script/upgrade/UpgradeTaikoL1.s.sol (1 hunks)
- packages/protocol/script/upgrade/UpgradeTimelockController.s.sol (1 hunks)
- packages/protocol/test/DeployCapability.sol (1 hunks)
- packages/protocol/test/L1/gov/TaikoGovernor.t.sol (1 hunks)
- packages/protocol/test/TaikoTest.sol (1 hunks)
- packages/protocol/test/automata-attestation/AutomataDcapV3AttestationTest.t.sol (1 hunks)
- packages/protocol/test/automata-attestation/common/AttestationBase.t.sol (1 hunks)
- packages/protocol/test/automata-attestation/utils/DcapTestUtils.t.sol (1 hunks)
- packages/protocol/test/automata-attestation/utils/V3QuoteParseUtils.t.sol (1 hunks)
- packages/protocol/test/common/erc20/FreeMintERC20.sol (1 hunks)
- packages/protocol/test/common/erc20/MayFailFreeMintERC20.sol (1 hunks)
- packages/protocol/test/common/erc20/RegularERC20.sol (1 hunks)
- packages/protocol/test/tokenvault/ERC1155Vault.t.sol (1 hunks)
- packages/protocol/test/tokenvault/ERC721Vault.t.sol (1 hunks)
- packages/protocol/test/verifiers/PseZkVerifier.t.sol (1 hunks)
Files skipped from review due to trivial changes (5)
- packages/protocol/contracts/team/TimelockTokenPool.sol
- packages/protocol/remappings.txt
- packages/protocol/script/upgrade/UpgradeERC20Vault.s.sol
- packages/protocol/script/upgrade/UpgradeERC721Vault.s.sol
- packages/protocol/test/automata-attestation/AutomataDcapV3AttestationTest.t.sol
Additional comments: 55
packages/protocol/test/common/erc20/RegularERC20.sol (1)
- 5-5: The import statement for the ERC20 contract from OpenZeppelin has been updated correctly to use the
node_modulespath.packages/protocol/script/upgrade/UpgradeBridge.s.sol (1)
- 17-18: The import paths for
Script.solandconsole2.solhave been correctly updated to reflect their new locations within theforge-std/srcdirectory.packages/protocol/script/upgrade/UpgradeTaikoL1.s.sol (1)
- 17-18: The import paths for
Script.solandconsole2.solhave been correctly updated to reflect their new locations within theforge-std/srcdirectory.packages/protocol/script/upgrade/UpgradeERC1155Vault.s.sol (1)
- 17-18: The import paths for
Script.solandconsole2.solhave been correctly updated to reflect their new locations within theforge-std/srcdirectory.packages/protocol/script/upgrade/UpgradeSignalService.s.sol (1)
- 17-18: The import paths for
Script.solandconsole2.solhave been correctly updated to reflect their new locations within theforge-std/srcdirectory.packages/protocol/contracts/L1/gov/TaikoTimelockController.sol (1)
- 17-17: The import statement for
TimelockControllerUpgradeable.solfrom OpenZeppelin has been updated correctly to use thenode_modulespath.packages/protocol/contracts/libs/LibDeploy.sol (1)
- 17-18: The import statements for
ERC1967Proxy.solandOwnableUpgradeable.solfrom OpenZeppelin have been correctly updated to use thenode_modulespath.packages/protocol/test/common/erc20/MayFailFreeMintERC20.sol (1)
- 4-4: The import statement for the ERC20 contract from OpenZeppelin has been updated correctly to use the
node_modulespath.packages/protocol/script/SetAddress.s.sol (1)
- 17-18: The import paths for
Script.solandconsole2.solhave been correctly updated to reflect their new locations within theforge-std/srcdirectory.packages/protocol/script/upgrade/UpgradeScript.s.sol (2)
- 18-18: The import statement for
UUPSUpgradeable.solfrom OpenZeppelin has been correctly updated to use thenode_modulespath.- 20-21: The import paths for
console2.solandScript.solhave been correctly updated to reflect their new locations within theforge-std/srcdirectory.packages/protocol/contracts/tokenvault/LibBridgedToken.sol (1)
- 17-17: The import statement for the
Strings.sollibrary from OpenZeppelin Contracts has been updated correctly to use thenode_modulespath.packages/protocol/script/SetDcapParams.s.sol (1)
- 17-18: The import paths for
Script.solandconsole2.solhave been correctly updated to reflect their new locations within theforge-std/srcdirectory.packages/protocol/contracts/team/airdrop/ERC20Airdrop.sol (1)
- 17-18: The import statements for
IERC20.solandIVotes.solfrom OpenZeppelin have been correctly updated to use thenode_modulespath.packages/protocol/contracts/tokenvault/BaseVault.sol (1)
- 17-17: The import statement for
IERC165Upgradeable.solfrom OpenZeppelin Contracts Upgradeable library has been updated correctly to use thenode_modulespath.packages/protocol/contracts/L2/CrossChainOwned.sol (1)
- 17-17: The import statement for
IERC20.solfrom OpenZeppelin has been updated correctly to use thenode_modulespath.packages/protocol/test/DeployCapability.sol (2)
- 5-6: The import paths for OpenZeppelin contracts have been correctly updated to use the
@openzeppelin/namespace, aligning with the PR's objective of managing dependencies throughnode_modules.- 8-9: The use of
forge-stdfor testing and scripting purposes is noted and appropriate for the context of deployment scripts.packages/protocol/contracts/team/airdrop/MerkleClaimable.sol (1)
- 17-17: The import path for
MerkleProof.solhas been correctly updated to use the@openzeppelin/contractsnamespace, aligning with the PR's objective of managing dependencies throughnode_modules.packages/protocol/contracts/libs/LibAddress.sol (1)
- 17-20: The import paths for OpenZeppelin contracts have been correctly updated to use the
@openzeppelin/contractsnamespace, aligning with the PR's objective of managing dependencies throughnode_modules.packages/protocol/test/TaikoTest.sol (1)
- 4-7: The import paths for
forge-stdand OpenZeppelin contracts have been correctly updated, aligning with the PR's objective of managing dependencies throughnode_modules.packages/protocol/contracts/common/OwnerUUPSUpgradable.sol (1)
- 17-18: The import paths for OpenZeppelin contracts have been correctly updated to use the
@openzeppelin/contracts-upgradeablenamespace, aligning with the PR's objective of managing dependencies throughnode_modules.packages/protocol/contracts/tokenvault/BridgedERC20Base.sol (1)
- 17-17: The import path for
ERC20Upgradeable.solhas been correctly updated to use the@openzeppelin/contracts-upgradeablenamespace, aligning with the PR's objective of managing dependencies throughnode_modules.packages/protocol/contracts/team/airdrop/ERC20Airdrop2.sol (1)
- 17-17: The import path for
IERC20.solhas been correctly updated to use the@openzeppelin/contractsnamespace, aligning with the PR's objective of managing dependencies throughnode_modules.packages/protocol/contracts/tokenvault/BridgedERC721.sol (1)
- 17-18: The import paths for OpenZeppelin contracts have been correctly updated to use the
@openzeppelin/contracts-upgradeablenamespace for ERC721 functionality and the@openzeppelin/contractsnamespace for utility functions, aligning with the PR's objective of managing dependencies throughnode_modules.packages/protocol/contracts/L1/TaikoToken.sol (1)
- 17-19: The import paths for OpenZeppelin contracts have been correctly updated to use the
@openzeppelin/contracts-upgradeablenamespace, aligning with the PR's objective of managing dependencies throughnode_modules.packages/protocol/contracts/tokenvault/BridgedERC1155.sol (1)
- 17-21: The import paths for OpenZeppelin contracts and interfaces have been correctly updated to use the
@openzeppelin/contracts-upgradeablenamespace for ERC1155 functionality and the@openzeppelin/contractsnamespace for utility functions, aligning with the PR's objective of managing dependencies throughnode_modules.packages/protocol/contracts/L1/gov/TaikoGovernor.sol (1)
- 17-24: The import paths for OpenZeppelin governance contracts have been correctly updated to use the
@openzeppelin/contracts-upgradeablenamespace, and the addition ofGovernorVotesQuorumFractionUpgradeableis appropriate for extending governance functionalities. This aligns with the PR's objective of managing dependencies throughnode_modules.packages/protocol/contracts/tokenvault/BridgedERC20.sol (1)
- 17-20: Changes in import paths from local to
@openzeppelinnamespace are correct and align with best practices for dependency management.packages/protocol/contracts/L1/hooks/AssignmentHook.sol (1)
- 17-18: Changes in import paths to use the
@openzeppelinnamespace are correct and align with best practices for dependency management.packages/protocol/test/L1/gov/TaikoGovernor.t.sol (1)
- 7-8: Changes in import paths to use the
@openzeppelinnamespace are correct and align with best practices for dependency management.packages/protocol/contracts/verifiers/SgxVerifier.sol (1)
- 17-17: Change in the import path for
ECDSA.solto use the@openzeppelinnamespace is correct and aligns with best practices for dependency management.packages/protocol/contracts/tokenvault/ERC721Vault.sol (1)
- 17-18: Changes in import paths for ERC721 interfaces to use the
@openzeppelinnamespace are correct and align with best practices for dependency management.packages/protocol/test/automata-attestation/utils/DcapTestUtils.t.sol (1)
- 7-8: Changes to import paths for
JSONParserLibandLibStringusing absolute paths are correct and align with best practices for dependency management.packages/protocol/contracts/L1/libs/LibProposing.sol (1)
- 17-17: Updating the import statement for
IERC20.solto use the@openzeppelinpackage is correct and enhances dependency management and security.packages/protocol/genesis/GenerateGenesis.g.sol (1)
- 4-6: Updating the import paths to point to the
srcdirectory within theforge-stdpackage is correct and improves clarity and maintainability of the code.packages/protocol/contracts/tokenvault/ERC20Vault.sol (1)
- 17-18: Updating the import paths for ERC20 and SafeERC20 libraries to use the
@openzeppelinpackage is correct and aligns with best practices for secure and reliable code.packages/protocol/contracts/automata-attestation/lib/PEMCertChainLib.sol (1)
- 4-4: The import path for
LibStringhas been updated correctly to reflect the new location within thesoladylibrary. This change aligns with the PR's objective to streamline dependency management.packages/protocol/test/automata-attestation/common/AttestationBase.t.sol (3)
- 4-6: The import paths for
forge-stdmodules have been updated correctly to point to thesrcdirectory. This change aligns with the PR's objective to streamline dependency management.- 10-10: The import path for
P256Verifierhas been updated correctly to reflect its location within thep256-verifiermodule. This change is consistent with the PR's objective.- 15-15: The import path for
Base64from thesoladylibrary has been updated correctly to point to thesrc/utilsdirectory. This change aligns with the PR's objective to streamline dependency management.packages/protocol/test/verifiers/PseZkVerifier.t.sol (1)
- 5-5: Change in import path for
Strings.solaligns with the PR's objective and follows best practices for dependency management.packages/protocol/contracts/L1/libs/LibProving.sol (1)
- 17-17: Change in import path for
IERC20.solaligns with the PR's objective and follows best practices for dependency management.packages/protocol/script/DeployOnL1.s.sol (1)
- 17-17: The import path for
Strings.solfrom OpenZeppelin has been updated to use@openzeppelin/contractsnamespace, aligning with the PR's objective to standardize dependency management.packages/protocol/contracts/automata-attestation/AutomataDcapV3Attestation.sol (2)
- 13-14: The import paths for
Base64andLibStringhave been updated to reflect their new locations within thesoladylibrary. This change aligns with the PR's objective to optimize and standardize import paths.- 21-21: The import statement for
console.solhas been commented out. While this is a non-functional change, it suggests a cleanup of debugging code, which is a good practice for maintaining clean and production-ready code.packages/protocol/test/tokenvault/ERC721Vault.t.sol (1)
- 4-4: The import statement for the ERC721 contract has been updated to use the
@openzeppelinpackage. Ensure that@openzeppelin/contractsis included in the project'spackage.jsondependencies to avoid runtime errors.packages/protocol/test/tokenvault/ERC1155Vault.t.sol (8)
- 4-4: The import statement for the ERC1155 contract has been updated to use the OpenZeppelin Contracts library. This change aligns with the PR's objective to manage dependencies using
node_modulesinstead of submodules, improving maintainability and ease of dependency management.- 6-7: Ensure that the
TaikoTest.solimport and its usage throughout the test suite are consistent and correctly implemented. This base test setup is crucial for the integrity of the test environment.- 1-7: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [9-11]
The constructor of
TestTokenERC1155correctly initializes the ERC1155 token with a base URI. This setup is essential for testing token minting and interactions within the ERC1155Vault tests.
- 1-7: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [13-15]
The
PrankDestBridgecontract is a mock to simulate cross-chain interactions without deploying real bridge code. This approach is effective for isolating the ERC1155Vault functionality in tests but ensure that the mock behavior closely mirrors the expected real-world bridge interactions.
- 1-7: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [17-19]
The
UpdatedBridgedERC1155contract introduces a new methodhelloWorld. Verify that this addition is solely for testing purposes and assess its relevance to the overall test objectives.
- 1-7: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [21-23]
The
ERC1155VaultTestsetup method is comprehensive, covering various aspects necessary for the tests. Ensure that the setup accurately reflects the test environment requirements, including mock contracts and initial token minting.
- 1-7: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [25-27]
The test cases, such as
test_1155Vault_sendToken_1155, are well-structured and cover significant functionalities of the ERC1155Vault. Ensure that each test case has clear objectives and adequately tests the contract's behavior under various conditions.
- 1-7: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [29-31]
The final test case,
test_1155Vault_upgrade_bridged_tokens_1155, demonstrates upgrading the implementation of a bridged contract. This test is crucial for ensuring that the upgrade mechanism works as expected. Verify that the upgrade process and the assertion checks are correctly implemented.
|
I like the idea, but it seems there are test errors. |
|
I like this idea, but please make sure all tests pass - there is at least a failure |
@d1onys1us i think the failure is coming from an import (and wrong remappings). That one is removed on branch #15781, and once incorporated into main, the base of this PR can be updated. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/protocol/script/DeployERC20Airdrop.s.sol (1 hunks)
- packages/protocol/test/team/airdrop/ERC20Airdrop.t.sol (1 hunks)
Additional comments: 4
packages/protocol/script/DeployERC20Airdrop.s.sol (2)
- 18-18: The import path for
console2.solhas been updated correctly to reflect the new file structure. This change aligns with the PR's objective of improving project structure and dependency management.- 15-21: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [27-45]
The use of
vm.startBroadcastandvm.stopBroadcastfrom Foundry's scripting environment is correctly implemented for transaction broadcasting. This approach is appropriate for deployment scripts in a development or testing context.packages/protocol/test/team/airdrop/ERC20Airdrop.t.sol (2)
- 6-6: The import statement for
OwnableUpgradeablehas been updated to use the@openzeppelinpackage. This change aligns with the PR's objective of standardizing and optimizing import paths for dependencies, improving maintainability and developer experience.- 3-9: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [7-122]
The mock contracts, such as
MockAddressManagerandSimpleERC20Vault, are implemented to facilitate testing without deploying external dependencies. This approach is effective for isolating the contract under test and focusing on its behavior. However, ensure that the behavior of these mock contracts closely mimics that of their real counterparts to ensure test accuracy.
|
@davidtaikocha I think you may need to review this PR. If I remembered correctly, you also need the codebase to work with Hardfork. |
I think the codebase is still in |
what do you think of this change @adaki2004 @dantaik ? i am inspired from:
i always have issues with git submodules and dont really like them.
how to update
after merging run these commands locally (make sure your index is clean):
Summary by CodeRabbit
@openzeppelin/contracts-upgradeablenamespace or correct project structure paths.