twinstake / permissioned-delegation-matic

0 stars 0 forks source link

`ValidatorShare` is no longer `OwnableLockable` #3

Closed eliotstock closed 1 year ago

eliotstock commented 1 year ago

Here's the diff between our version of ValidatorShare.sol and the original. I'd expect to see only the addition of the whitelist in this diff.

Why did you remove OwnableLockable? Could be a valid choice, I'm just wondering.

diff contracts/contracts/staking/validatorShare/ValidatorShare.sol Staking-Contract-matic/contracts/staking/validatorShare/ValidatorShare.sol 
8c8
< import {OwnableLockable} from "../../common/mixin/OwnableLockable.sol";
---
> 
11a12
> import {Whitelist} from "../../Whitelist.sol";
13c14
< contract ValidatorShare is IValidatorShare, ERC20NonTradable, OwnableLockable, Initializable {
---
> contract ValidatorShare is IValidatorShare, ERC20NonTradable, Initializable , Whitelist {
55c56,57
<         address _stakeManager
---
>         address _stakeManager,
>         address _owner
62d63
< 
64a66
>         Whitelist__initialize(_owner);
112c114
<     function buyVoucher(uint256 _amount, uint256 _minSharesToMint) public returns(uint256 amountToDeposit) {
---
>     function buyVoucher(uint256 _amount, uint256 _minSharesToMint) onlyWhiteListed public returns(uint256 amountToDeposit) {
121c123
<     function restake() public returns(uint256, uint256) {
---
>     function restake() onlyWhiteListed public returns(uint256, uint256) {
rac-sri commented 1 year ago

It's not removed. Just the inheritance order is modified. Whitelist inherits OwnableLockable first, then Whitelist is inherited by ValidatorShare. Previously, whitelist used owner() from Ownable.sol so modifying the inheritance hierarchy was required. I can move it back too if that makes code cleaner, else logic remains the same.

eliotstock commented 1 year ago

OK no worries, sounds fair enough.