Contract changes for storing addresses #1622
Conversation
WalkthroughThe changes introduced in the ManagementContract.sol file are primarily focused on managing important contract addresses. This includes the addition of a mapping to store these addresses, an array to store their keys, and functions to set an address and retrieve all keys. An event has also been added to log changes to these addresses. Changes
TipsChat with CodeRabbit Bot (
|
![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/generated/ManagementContract/ManagementContract.go
Files selected for processing (1)
- contracts/src/management/ManagementContract.sol (2 hunks)
Additional comments: 3
contracts/src/management/ManagementContract.sol (3)
13-14: The addition of the
ImportantContractAddressUpdatedevent is a good practice for logging changes to important contract addresses. This will help in tracking changes and debugging if necessary.16-26: The addition of
importantContractKeysandimportantContractAddressesis a practical solution for the testnet phase. However, as noted in the comments, this might not be a long-term solution. It's important to revisit this decision in the future and consider a more scalable and secure way of managing important contract addresses. Also, consider adding a comment to explain the purpose ofimportantContractKeysandimportantContractAddressesfor better code readability.+ // importantContractKeys stores the keys for important contract addresses + string[] public importantContractKeys; + // importantContractAddresses is a mapping from keys to important contract addresses + mapping (string => address) public importantContractAddresses;Committable suggestion (Beta)
// importantContractKeys stores the keys for important contract addresses string[] public importantContractKeys; // importantContractAddresses is a mapping from keys to important contract addresses mapping (string => address) public importantContractAddresses;
- 154-156: The
getImportantContractKeysfunction is a good addition for retrieving all keys of important contract addresses. It returnsimportantContractKeys, which can be used to fetch the corresponding addresses fromimportantContractAddresses.
| // Function to set an important contract's address, only callable by owner | ||
| function setImportantContractAddress(string memory key, address newAddress) public onlyOwner { | ||
| if (importantContractAddresses[key] == address(0)) { | ||
| importantContractKeys.push(key); | ||
| } | ||
| importantContractAddresses[key] = newAddress; | ||
| emit ImportantContractAddressUpdated(key, newAddress); | ||
| } |
There was a problem hiding this comment.
The setImportantContractAddress function is a good addition for managing important contract addresses. It checks if the key is new and adds it to importantContractKeys if it is. Then it updates the address in importantContractAddresses and emits an event. This function is only callable by the owner, which is a good security measure. However, consider adding input validation to ensure that the newAddress is not a zero address.
+ function setImportantContractAddress(string memory key, address newAddress) public onlyOwner {
+ require(newAddress != address(0), "New address cannot be a zero address");
+ if (importantContractAddresses[key] == address(0)) {
+ importantContractKeys.push(key);
+ }
+ importantContractAddresses[key] = newAddress;
+ emit ImportantContractAddressUpdated(key, newAddress);
+ }Committable suggestion (Beta)
| // Function to set an important contract's address, only callable by owner | |
| function setImportantContractAddress(string memory key, address newAddress) public onlyOwner { | |
| if (importantContractAddresses[key] == address(0)) { | |
| importantContractKeys.push(key); | |
| } | |
| importantContractAddresses[key] = newAddress; | |
| emit ImportantContractAddressUpdated(key, newAddress); | |
| } | |
| function setImportantContractAddress(string memory key, address newAddress) public onlyOwner { | |
| require(newAddress != address(0), "New address cannot be a zero address"); | |
| if (importantContractAddresses[key] == address(0)) { | |
| importantContractKeys.push(key); | |
| } | |
| importantContractAddresses[key] = newAddress; | |
| emit ImportantContractAddressUpdated(key, newAddress); | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- contracts/src/management/ManagementContract.sol (2 hunks)
Additional comments: 3
contracts/src/management/ManagementContract.sol (3)
13-14: The new event
ImportantContractAddressUpdatedis introduced to log changes to important contract addresses. This is a good practice for tracking changes and can be useful for debugging and auditing purposes.21-26: The new mapping
importantContractAddressesand the arrayimportantContractKeysare introduced to store important contract addresses and their corresponding keys. This is a temporary solution for testnets and may not be suitable for long-term use. Consider using a more permanent solution for production environments.154-156: The new function
GetImportantContractKeysis introduced to retrieve all keys for the important contract addresses. This function is a good practice for providing access to the keys.
| function SetImportantContractAddress(string memory key, address newAddress) public onlyOwner { | ||
| if (importantContractAddresses[key] == address(0)) { | ||
| importantContractKeys.push(key); | ||
| } | ||
| importantContractAddresses[key] = newAddress; | ||
| emit ImportantContractAddressUpdated(key, newAddress); | ||
| } |
There was a problem hiding this comment.
The new function SetImportantContractAddress is introduced to update an important contract's address. This function is only callable by the owner, which is a good security practice. However, there is no check for whether the newAddress is a valid address. Consider adding a check to ensure that the newAddress is not the zero address.
- if (importantContractAddresses[key] == address(0)) {
+ require(newAddress != address(0), "New address cannot be the zero address");
+ if (importantContractAddresses[key] == address(0)) {Committable suggestion (Beta)
| function SetImportantContractAddress(string memory key, address newAddress) public onlyOwner { | |
| if (importantContractAddresses[key] == address(0)) { | |
| importantContractKeys.push(key); | |
| } | |
| importantContractAddresses[key] = newAddress; | |
| emit ImportantContractAddressUpdated(key, newAddress); | |
| } | |
| function SetImportantContractAddress(string memory key, address newAddress) public onlyOwner { | |
| require(newAddress != address(0), "New address cannot be the zero address"); | |
| if (importantContractAddresses[key] == address(0)) { | |
| importantContractKeys.push(key); | |
| } | |
| importantContractAddresses[key] = newAddress; | |
| emit ImportantContractAddressUpdated(key, newAddress); | |
| } |
Why this change is needed
While we're in testnet it is convenient to store the addresses of important network contracts in the management contract as an accessible source of truth.
What changes were made as part of this PR
Add a list of keys and a map of key -> address. The list is necessary so that callers can 'get all' without having to know them up front, means we can serve them all from the node without having to make changes if new ones are added.
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks