(chore): Parameterize the BalanceTracker version #123
Conversation
…reinitialized in the future
✅ Heimdall Review Status
|
| */ | ||
| function initialize(address payable[] memory _systemAddresses, uint256[] memory _targetBalances) | ||
| function initialize(address payable[] memory _systemAddresses, uint256[] memory _targetBalances, uint8 _version) |
There was a problem hiding this comment.
I can not see any access control on the initialize() function. Now that _version is a user-supplied parameter, this seems to allow arbitrary reinitialization by any caller who provides a higher version number.
There was a problem hiding this comment.
tbh, perhaps we want a way to just change the addresses without having to call initialize
There was a problem hiding this comment.
even if this was hardcoded (like before this PR), it'd have to be incremented every time we need to update the addresses, but nothing about the core implementation actually changed
There was a problem hiding this comment.
My point is that the previous reinitializer(2) prevented it from being reinitialized. Now that it uses reinitializer(_version), I think anyone could reinitialize the contract.
There was a problem hiding this comment.
tbh, perhaps we want a way to just change the addresses without having to call initialize
I think we would still need to enforce access control on who can update those addresses.
There was a problem hiding this comment.
Wow yeah good catch. So you think we should make it ownable and add a setter function instead?
There was a problem hiding this comment.
Probably not worth the trouble actually - inheriting a new contract could screw up the storage layout making it not feasible to upgrade. We'd have to add custom logic
| @@ -16,6 +16,7 @@ contract BalanceTrackerTest is CommonTest { | |||
| event SentProfit(address indexed _profitWallet, bool indexed _success, uint256 _balanceSent); | |||
| event ReceivedFunds(address indexed _sender, uint256 _amount); | |||
|
|
|||
| uint8 constant VERSION = 1; | |||
There was a problem hiding this comment.
Would it be better to set it to 2 to match what was in the contract previously?
This will allow new
BalanceTrackerversions to be initialized in the future.Also updated an internal function name to include a leading underscore - in line with our style guide.