Upgrade OZ libraries #1707
Upgrade OZ libraries #1707
Conversation
WalkthroughThe recent updates to the smart contract codebase include a Solidity compiler version bump to 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 filter (1)
- contracts/package.json
Files selected for processing (3)
- contracts/hardhat.config.ts (2 hunks)
- contracts/src/management/ManagementContract.sol (2 hunks)
- contracts/src/messaging/MessageBus.sol (1 hunks)
Additional comments: 5
contracts/hardhat.config.ts (1)
- 23-23: The Solidity compiler version has been updated to
0.8.20. Ensure that all smart contracts in the project are compatible with this version and that no deprecated features are being used that could cause compilation errors or warnings.contracts/src/management/ManagementContract.sol (3)
7-7: The import of
MessageHashUtilsis new. Verify that the usage of this utility aligns with the intended cryptographic practices and that it's being used securely throughout the contract.18-18: The constructor now explicitly sets the owner to
msg.senderusing theOwnableconstructor. Confirm that this change is consistent with the intended ownership initialization logic for all contracts inheriting fromOwnable.Verification successful
The initial script intended to search for constructors inheriting from
Ownableand using the new explicit ownership assignment failed becausesolidityis not a supported language forast-grep. To continue the verification process, we can userg(ripgrep) to search for constructors in Solidity files that inherit fromOwnable. We will look for the patternconstructor() Ownable(msg.sender)to confirm that the change is consistent across the codebase.
The script has successfully identified that the explicit ownership assignment using
Ownable(msg.sender)in the constructor is present in bothMessageBus.solandManagementContract.sol. This confirms that the change to the constructor inManagementContract.solis consistent with at least one other contract in the codebase that inherits fromOwnable.Based on the information provided, it appears that the intended ownership initialization logic for contracts inheriting from
Ownablehas been correctly implemented in the instances found.* 124-127: The signature verification logic has been updated. Ensure that the new logic correctly verifies the signer's address and that there are no security implications due to the changes in how `calculatedHashSigned` is computed.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for constructors that inherit Ownable to ensure they are using the new explicit ownership assignment ast-grep --lang solidity --pattern $'contract $$$ is Ownable { constructor() Ownable($_) { $$$ } }'Length of output: 217
Script:
#!/bin/bash # Search for constructors in Solidity files that inherit from Ownable and use the new explicit ownership assignment rg 'constructor\(\) Ownable\(msg.sender\)' --glob '*.sol'Length of output: 227
contracts/src/messaging/MessageBus.sol (1)
- 11-11: The
MessageBuscontract now includes a constructor that sets the owner tomsg.sender. This is a significant change in terms of contract ownership. Ensure that this change is intentional and that it does not introduce any security risks, especially if theMessageBuscontract is meant to be owned by a specific entity or contract.
| }, | ||
| 'src/testing/**/*': { | ||
| default: 'off' |
There was a problem hiding this comment.
The warning settings for files in the 'src/testing' directory have been turned off. This could potentially hide important warnings during the testing phase. It's recommended to only disable specific warnings that are known to be non-critical after thorough evaluation.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (10)
- contracts/generated/ConstantSupplyERC20/ConstantSupplyERC20.go
- contracts/generated/CrossChainMessenger/CrossChainMessenger.go
- contracts/generated/EthereumBridge/EthereumBridge.go
- contracts/generated/GasConsumerBalance/GasConsumerBalance.go
- contracts/generated/ManagementContract/ManagementContract.go
- contracts/generated/MessageBus/MessageBus.go
- contracts/generated/ObsERC20/ObsERC20.go
- contracts/generated/ObscuroBridge/ObscuroBridge.go
- contracts/generated/WrappedERC20/WrappedERC20.go
- contracts/package.json
Files selected for processing (2)
- contracts/src/common/ConstantSupplyERC20.sol (1 hunks)
- contracts/test/bridge-test.ts (1 hunks)
Additional comments: 4
contracts/src/common/ConstantSupplyERC20.sol (2)
4-4: The import statement has been changed to use destructuring. This is a stylistic choice and does not impact functionality.
8-12: The constructor reformatting improves readability and maintains the same logic.
contracts/test/bridge-test.ts (2)
35-45: The addition of the
ownerparameter when deploying theConstantSupplyERC20contract is consistent with the new constructor signature. Ensure that theownervariable is used correctly in the context of the contract deployment.40-45: The use of a try-catch block for the ERC20 contract deployment is a good practice for handling potential deployment errors.
| @@ -1,11 +1,13 @@ | |||
| // SPDX-License-Identifier: Apache 2 | |||
| pragma solidity >=0.7.0 <0.9.0; | |||
There was a problem hiding this comment.
Consider narrowing the pragma solidity version to ^0.8.20 to match the updated Solidity version in hardhat.config.ts and ensure compatibility with the new OpenZeppelin library version.
|
|
||
| const ERC20 = await hre.ethers.getContractFactory("ConstantSupplyERC20", owner); | ||
|
|
||
| console.log(`Deploying erc20`); |
There was a problem hiding this comment.
Consider removing or commenting out the console.log statement before deployment for cleaner production code.
|
|
||
| const erc20 = await ERC20.deploy("XXX", "XXX"); | ||
| erc20address = erc20.address; | ||
| console.log(`Deployed erc20`); |
There was a problem hiding this comment.
Consider removing or commenting out the console.log statement after deployment for cleaner production code.
There was a problem hiding this comment.
LGTM (I think).
@BedrockSquirrel , pls have a look as well when you get a chance
| // the data must be signed with by the correct private key | ||
| // signature = f(PubKey, PrivateKey, message) | ||
| // address = f(signature, message) | ||
| // valid if attesterID = address | ||
| bytes32 calculatedHashSigned = ECDSA.toEthSignedMessageHash(abi.encodePacked(attesterID, requesterID, hostAddress, responseSecret)); | ||
| bytes32 calculatedHashSigned = abi.encodePacked(attesterID, requesterID, hostAddress, responseSecret).toEthSignedMessageHash(); |
There was a problem hiding this comment.
nope, its in the go code where the signature is generated; We need to append to correct recovery id.
This reverts commit cc3b104.
Why this change is needed
Migrate OZ libraries to version 5
What changes were made as part of this PR
Upgraded the packages, made the contracts compile. Fix for shadowing is separate.
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks