-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core balances logic includes reactivation #108
Conversation
contracts/SSVNetwork.sol
Outdated
if(_liquidatable(pod.disabled, _podBalance(pod, _clusterCurrentIndex(clusterId)), pod.validatorCount, operatorIds)) { | ||
revert NotEnoughBalance(); | ||
} | ||
/* TODO: Adam, Lior, Andrew - check following two lines */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamzigdon @andrew-blox @lior-blox check these lines. 538, 539, 540 lines
contracts/ISSVNetwork.sol
Outdated
|
||
function withdraw(bytes32 clusterId, uint256 tokenAmount) external; | ||
|
||
function withdrawAll(bytes32 clusterId) external; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vadiminc How does an operator withdraw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lior-blox right, missed - will add extra interface for operator
contracts/SSVNetwork.sol
Outdated
@@ -53,9 +53,13 @@ contract SSVNetwork is OwnableUpgradeable, ISSVNetwork { | |||
|
|||
struct Pod { | |||
uint64 validatorCount; | |||
uint64 withdrawn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vadiminc why do we need it? cant we deduct it from the snapshot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lior-blox removed
contracts/SSVNetwork.sol
Outdated
revert NotEnoughBalance(); | ||
} | ||
|
||
if(_burnRatePerValidator(operatorIds) > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vadiminc @andrew-blox In which case this can happen?
contracts/SSVNetwork.sol
Outdated
} | ||
|
||
pod.withdrawn += podBalance; | ||
_pods[hashedPod] = pod; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vadiminc Shouldn't it mark the pod as disabled and update the operators?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or to do self-liquidation?
contracts/SSVNetwork.sol
Outdated
revert NotEnoughBalance(); | ||
} | ||
/* TODO: Adam, Lior, Andrew - check following two lines */ | ||
pod = _updatePodData(clusterId, 0, hashedPod, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vadiminc this is a bad hack.
maybe to change instead bool increase to uint? i.e (-1,0,1)?
|
||
it('Deposit', async () => { | ||
minDepositAmount = (helpers.CONFIG.minimalBlocksBeforeLiquidation + 10) * helpers.CONFIG.minimalOperatorFee * 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vadiminc Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lior-blox what do you mean why? instead of to use magic unclear numbers in the test - we know that minDepositAmount covers threshold period before liquidation for per validator
test/helpers/gas-usage.ts
Outdated
[GasGroup.REMOVE_VALIDATOR]: 120000, | ||
[GasGroup.TRANSFER_VALIDATOR_NEW_CLUSTER]: 400000, | ||
[GasGroup.TRANSFER_VALIDATOR]: 260000, | ||
[GasGroup.TRANSFER_VALIDATOR_NON_EXISTING_POD]: 290000, | ||
[GasGroup.BULK_TRANSFER_VALIDATOR]: 362000, | ||
[GasGroup.BULK_TRANSFER_VALIDATOR_NON_EXISTING_POD]: 379000, | ||
[GasGroup.LIQUIDATE_VALIDATOR]: 101000, | ||
[GasGroup.LIQUIDATE_VALIDATOR]: 120000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vadiminc there is no liquidate validator, only POD so please rename, why it increased?
test/helpers/gas-usage.ts
Outdated
[GasGroup.REMOVE_VALIDATOR]: 120000, | ||
[GasGroup.TRANSFER_VALIDATOR_NEW_CLUSTER]: 400000, | ||
[GasGroup.TRANSFER_VALIDATOR]: 260000, | ||
[GasGroup.TRANSFER_VALIDATOR_NON_EXISTING_POD]: 290000, | ||
[GasGroup.BULK_TRANSFER_VALIDATOR]: 362000, | ||
[GasGroup.BULK_TRANSFER_VALIDATOR_NON_EXISTING_POD]: 379000, | ||
[GasGroup.LIQUIDATE_VALIDATOR]: 101000, | ||
[GasGroup.LIQUIDATE_VALIDATOR]: 120000, | ||
[GasGroup.DEPOSIT]: 84000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vadiminc @andrew-blox This new gases are too much
please revisit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-blox @lior-blox i change deposit flow to work with pod and it saved us 5k gas. So now it's 77k gas usage
part of it:
- 20k - token transfer
- 2k - emit event,
the rest is the logic.
test/helpers/gas-usage.ts
Outdated
[GasGroup.DEPOSIT]: 84000, | ||
[GasGroup.WITHDRAW]: 96000, | ||
[GasGroup.REGISTER_POD]: 141000, | ||
[GasGroup.REACTIVATE_POD]: 251000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vadiminc @andrew-blox Not clear why reactivate is so expensive compared to register
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lior-blox @andrew-blox as I've removed "create pod" from the deposit func, we saved 10k for reactivepod as well
contracts/SSVNetwork.sol
Outdated
function deposit(bytes calldata publicKey, uint256 amount) external { | ||
_validatePublicKey(publicKey); | ||
bytes32 hashedValidator = keccak256(publicKey); | ||
|
||
if (_validatorPKs[hashedValidator].owner != msg.sender) { | ||
revert ValidatorNotOwned(); | ||
} | ||
|
||
_deposit(msg.sender, _validatorPKs[hashedValidator].clusterId, amount.shrink()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lior-blox @vadiminc
I understand we have capability of getting cluster or pod id from a validators public key but its a little weird to deposit using a validators public key as the param instead of like a cluster id no?
This also applies for the other deposit and for withdraw.
contracts/SSVNetwork.sol
Outdated
} | ||
|
||
function withdraw(bytes32 clusterId, uint256 amount) external override { | ||
_validateClusterId(clusterId); | ||
function withdraw(bytes calldata publicKey, uint256 amount) external override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this function you call amount.shrink twice (which in itself calls other functions), i suggest to make a var for it.
contracts/SSVNetwork.sol
Outdated
@@ -393,33 +408,51 @@ contract SSVNetwork is OwnableUpgradeable, ISSVNetwork { | |||
|
|||
_token.transfer(msg.sender, amount); | |||
|
|||
emit FundsWithdrawal(amount, clusterId, msg.sender); | |||
emit ValidatorFundsWithdrawal(amount, publicKey, msg.sender); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lior-blox
I think we should go over all emit names for all functions added.
contracts/SSVNetwork.sol
Outdated
} | ||
|
||
function withdrawAll(bytes32 clusterId) external override { | ||
_validateClusterId(clusterId); | ||
function withdraw(uint64 operatorId, uint256 amount) external override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling amount.shrink 4 times
contracts/SSVNetwork.sol
Outdated
emit OperatorFundsWithdrawal(amount.shrink(), operatorId, msg.sender); | ||
} | ||
|
||
function withdrawAll(uint64 operatorId) external override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make var for expand function
No description provided.