Skip to content

Commit

Permalink
feat: resolve review points
Browse files Browse the repository at this point in the history
  • Loading branch information
@mortimr
mortimr committed Mar 29, 2022
1 parent 29c258e commit bbfd108
Show file tree
Hide file tree
Showing 3 changed files with 346 additions and 328 deletions.
6 changes: 6 additions & 0 deletions .prettierignore
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
node_modules
artifacts
cache
coverage*
gasReporterOutput.json
deploy/*.ts
225 changes: 134 additions & 91 deletions src/StakingContract.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,35 +11,46 @@ import "./test/console.sol";
contract StakingContract {
using StateLib for bytes32;

// review(nmvalera): we can hard code all *_SLOT values
bytes32 internal constant FEE_SLOT = keccak256("StakingContract.fee");
bytes32 internal constant ADMIN_SLOT = keccak256("StakingContract.admin");
bytes32 internal constant VERSION_SLOT =
/* keccak256("StakingContract.version") */
hex"d5c553085b8382c47128ae7612257fd5dc3b4fc4d3a108925604d3c8700c025b";
bytes32 internal constant OPERATOR_SLOT =
/* keccak256("StakingContract.operator") */
hex"dfe7334ae89a4aa54c085540947bfa7e13e6b6933be4c49f359d18e88c0dbde5";
bytes32 internal constant SIGNATURES_SLOT =
/* keccak256("StakingContract.signatures") */
hex"2805e4a7c8c139ac2ebe63141d90c488245fd479906b2c60bd42603b8a2ca08b";
bytes32 internal constant PUBLIC_KEYS_SLOT =
/* keccak256("StakingContract.publicKeys") */
hex"cc0b8384259c4a4e6418cdc72955757e9214822019f44d8b5283077c1b46d43c";
bytes32 internal constant WITHDRAWERS_SLOT =
/* keccak256("StakingContract.withdrawers") */
hex"86647fdbbdb534026d3e0f93a551cecf651c2b40fcdfef4b9fd9ed826133e265";
bytes32 internal constant VALIDATORS_COUNT_SLOT =
/* keccak256("StakingContract.validatorsCount") */
hex"e9622dd0bba60226e1dbc661ca8aae56cc90dc7e9b3f33ece002f6764b3801b8";
bytes32 internal constant DEPOSIT_CONTRACT_SLOT =
/* keccak256("StakingContract.depositContract") */
hex"bc8b9852d17d50256bb221fdf6ee12d78dd493d807e907f7d223c40d65abd6b9";
bytes32 internal constant WITHDRAWAL_CREDENTIALS_SLOT =
/* keccak256("StakingContract.withdrawalCredentials") */
hex"2783da738595cd6ebaec6fd0f06d62f2266a9e475e2d1feb1d26aa2d1e051255";

// review(nmvalera): can we add an entry for the node operator like OPERATOR_SLOT
bytes32 internal constant VERSION_SLOT = keccak256("StakingContract.version");
bytes32 internal constant SIGNATURES_SLOT = keccak256("StakingContract.signatures");
bytes32 internal constant PUBLIC_KEYS_SLOT = keccak256("StakingContract.publicKeys");

// review(nmvalera): rename VALIDATOR_COUNT_SLOT -> VALIDATORS_COUNT_SLOT
bytes32 internal constant VALIDATOR_COUNT_SLOT = keccak256("StakingContract.validatorCount");
bytes32 internal constant DEPOSIT_CONTRACT_SLOT = keccak256("StakingContract.depositContract");

// review(nmvalera): rename PUBLIC_KEY_OWNERSHIP_SLOT -> WITHDRAWERS_SLOT
bytes32 internal constant PUBLIC_KEY_OWNERSHIP_SLOT = keccak256("StakingContract.publicKeyOwnership");
bytes32 internal constant WITHDRAWAL_CREDENTIALS_SLOT = keccak256("StakingContract.withdrawalCredentials");

// review(nmvalera): can we add an entry for the operator

uint256 public constant DEPOSIT_SIZE = 32 ether;
uint256 public constant PUBLIC_KEY_LENGTH = 48;
uint256 public constant SIGNATURE_LENGTH = 96;
uint256 public constant PUBLIC_KEY_LENGTH = 48;
uint256 public constant DEPOSIT_SIZE = 32 ether;

error Unauthorized();
error AlreadyInitialized();
error InvalidCall();
error InvalidArgument();
error InvalidValue();
error Unauthorized();
error NotEnoughKeys();
error DepositFailure();
error InvalidArgument();
error UnsortedIndexes();
error InvalidPublicKeys();
error InvalidSignatures();
error AlreadyInitialized();
error InvalidMessageValue();
error FundedValidatorDeletionAttempt();

event Deposit(address indexed caller, address indexed withdrawer, bytes publicKey, bytes32 publicKeyRoot);

Expand All @@ -53,47 +64,39 @@ contract StakingContract {
_;
}

modifier onlyAdmin() {
if (msg.sender != ADMIN_SLOT.getAddress()) {
modifier onlyOperator() {
if (msg.sender != OPERATOR_SLOT.getAddress()) {
revert Unauthorized();
}

_;
}

function initialize_1(
address _admin,
uint256 _fee,
address _operator,
address _depositContract,
bytes32 _withdrawalCredentials
) external init(1) {
ADMIN_SLOT.setAddress(_admin);
FEE_SLOT.setUint256(_fee);
OPERATOR_SLOT.setAddress(_operator);
DEPOSIT_CONTRACT_SLOT.setAddress(_depositContract);
WITHDRAWAL_CREDENTIALS_SLOT.setBytes32(_withdrawalCredentials);
}

// review(nmvalera): if I understand correctly this one should be named fundedValidatorsCount()
function getValidatorCount() external view returns (uint256) {
return VALIDATOR_COUNT_SLOT.getUint256();
function fundedValidatorsCount() external view returns (uint256) {
return VALIDATORS_COUNT_SLOT.getUint256();
}

// question(nmvalera): rename getKeyCount -> totalValidatorCount
function getKeyCount() external view returns (uint256) {
function totalValidatorCount() external view returns (uint256) {
return PUBLIC_KEYS_SLOT.getStorageBytesArray().value.length;
}

function getWithdrawer(bytes memory _publicKey) external view returns (address) {
bytes32 pubkeyRoot = sha256(BytesLib.pad64(_publicKey));
StateLib.Bytes32ToAddressMappingSlot storage publicKeyOwnership = PUBLIC_KEY_OWNERSHIP_SLOT
StateLib.Bytes32ToAddressMappingSlot storage publicKeyOwnership = WITHDRAWERS_SLOT
.getStorageBytes32ToAddressMapping();
return publicKeyOwnership.value[pubkeyRoot];
}

function getFee() external view returns (uint256) {
return FEE_SLOT.getUint256();
}

function deposit(address _withdrawer) external payable {
_deposit(_withdrawer);
}
Expand All @@ -106,61 +109,110 @@ contract StakingContract {
revert InvalidCall();
}

// review(nmvalera): rename to registerValidators
// review(nmvalera): modifier should be onlyOperator (modifier to be created)
// review(nmvalera): why not doing the same as River function addValidators(uint256 _keyCount,bytes calldata _publicKeys, bytes calldata _signatures)? It will be easier for skillz to operate all contracts the same way
function registerValidatorKeys(bytes[] memory publicKeys, bytes[] memory signatures) external onlyAdmin {
if (publicKeys.length != signatures.length || publicKeys.length == 0) {
function registerValidators(
uint256 keyCount,
bytes calldata publicKeys,
bytes calldata signatures
) external onlyOperator {
if (keyCount == 0) {
revert InvalidArgument();
}

if (publicKeys.length % PUBLIC_KEY_LENGTH != 0 || publicKeys.length / PUBLIC_KEY_LENGTH != keyCount) {
revert InvalidPublicKeys();
}

if (signatures.length % SIGNATURE_LENGTH != 0 || signatures.length / SIGNATURE_LENGTH != keyCount) {
revert InvalidSignatures();
}

StateLib.BytesArraySlot storage publicKeysStore = PUBLIC_KEYS_SLOT.getStorageBytesArray();
StateLib.BytesArraySlot storage signaturesStore = SIGNATURES_SLOT.getStorageBytesArray();

for (uint256 i; i < publicKeys.length; ) {
if (publicKeys[i].length != 48 || signatures[i].length != 96) {
// review(nmvalera): is it possible to be more explicit in the message, like "invalid pubkey at position {i} has length {publicKeys[i].length} expects 48"
revert InvalidArgument();
for (uint256 i; i < keyCount; ) {
bytes memory publicKey = BytesLib.slice(publicKeys, i * PUBLIC_KEY_LENGTH, PUBLIC_KEY_LENGTH);
bytes memory signature = BytesLib.slice(signatures, i * SIGNATURE_LENGTH, SIGNATURE_LENGTH);

publicKeysStore.value.push(publicKey);
signaturesStore.value.push(signature);

unchecked {
++i;
}
publicKeysStore.value.push(publicKeys[i]);
signaturesStore.value.push(signatures[i]);
// question(nmvalera): what does unchecked mean?
}
}

function removeValidators(uint256[] calldata _indexes) external onlyOperator {
if (_indexes.length == 0) {
revert InvalidArgument();
}

uint256 validatorsCount = VALIDATORS_COUNT_SLOT.getUint256();
StateLib.BytesArraySlot storage publicKeysStore = PUBLIC_KEYS_SLOT.getStorageBytesArray();
StateLib.BytesArraySlot storage signaturesStore = SIGNATURES_SLOT.getStorageBytesArray();

for (uint256 i; i < _indexes.length; ) {
if (i > 0 && _indexes[i] >= _indexes[i - 1]) {
revert UnsortedIndexes();
}

if (_indexes[i] < validatorsCount) {
revert FundedValidatorDeletionAttempt();
}

if (_indexes[i] == publicKeysStore.value.length - 1) {
publicKeysStore.value.pop();
signaturesStore.value.pop();
} else {
publicKeysStore.value[_indexes[i]] = publicKeysStore.value[publicKeysStore.value.length - 1];
publicKeysStore.value.pop();
signaturesStore.value[_indexes[i]] = signaturesStore.value[signaturesStore.value.length - 1];
signaturesStore.value.pop();
}

unchecked {
++i;
}
}
}

// review(nmvalera): add a method getValidator(uint256 _idx) that returns a validator (pubkey, signature, withdrawer, and funded)
function getValidator(uint256 _idx)
external
view
returns (
bytes memory publicKey,
bytes memory signature,
address withdrawer,
bool funded
)
{
StateLib.BytesArraySlot storage publicKeysStore = PUBLIC_KEYS_SLOT.getStorageBytesArray();
StateLib.BytesArraySlot storage signaturesStore = SIGNATURES_SLOT.getStorageBytesArray();
StateLib.Bytes32ToAddressMappingSlot storage withdrawers = WITHDRAWERS_SLOT.getStorageBytes32ToAddressMapping();
uint256 validatorCount = VALIDATORS_COUNT_SLOT.getUint256();

// review(nmvalera): add a method removeValidators(uint256[] calldata _indexes)
publicKey = publicKeysStore.value[_idx];
signature = signaturesStore.value[_idx];
withdrawer = withdrawers.value[sha256(BytesLib.pad64(publicKey))];
funded = _idx < validatorCount;
}

function setWithdrawer(bytes memory _publicKey, address _newWithdrawer) external {
bytes32 pubkeyRoot = sha256(BytesLib.pad64(_publicKey));
StateLib.Bytes32ToAddressMappingSlot storage publicKeyOwnership = PUBLIC_KEY_OWNERSHIP_SLOT
StateLib.Bytes32ToAddressMappingSlot storage publicKeyOwnership = WITHDRAWERS_SLOT
.getStorageBytes32ToAddressMapping();

if (msg.sender != publicKeyOwnership.value[pubkeyRoot]) {
// review(nmvalera): is it possible to be more explicit in the message, like "{msg.sender} is not the withdrawer for {_publicKey}"
revert Unauthorized();
}

publicKeyOwnership.value[pubkeyRoot] = _newWithdrawer;
}

// review(nmvalera): following up on discussion with ledger we should not need the fee upfront
function setFee(uint256 _newFee) external onlyAdmin {
FEE_SLOT.setUint256(_newFee);
}

// review(nmvalera): rename _useKeys -> _depositValidator
function _useKeys(
// review(nmvalera): rename _publicKey -> _pubkey (let's try to be consistent every where using pubkey and not publicKey)
function _depositValidator(
bytes memory _publicKey,
bytes memory _signature,
bytes32 _withdrawalCredentials,
address _withdrawer
bytes32 _withdrawalCredentials
) internal {
bytes32 pubkeyRoot = sha256(BytesLib.pad64(_publicKey));
bytes32 signatureRoot = sha256(
Expand Down Expand Up @@ -189,29 +241,18 @@ contract StakingContract {
depositDataRoot
);

require(address(this).balance == targetBalance, "EXPECTING_DEPOSIT_TO_HAPPEN");

// review(nmvalera): seems that setting withdrawer and emitting Deposit event should be part of _deposit() function. 2 reasons:
// 1. we scope the responsibility of _useKeys() to only depositing a key to the official deposit contract
// 2. we avoid to load storage publicKeyOwnership at every step of the loop
StateLib.Bytes32ToAddressMappingSlot storage publicKeyOwnership = PUBLIC_KEY_OWNERSHIP_SLOT
.getStorageBytes32ToAddressMapping();

publicKeyOwnership.value[pubkeyRoot] = _withdrawer;

emit Deposit(msg.sender, _withdrawer, _publicKey, pubkeyRoot);
if (address(this).balance != targetBalance) {
revert DepositFailure();
}
}

function _deposit(address _withdrawer) internal {
uint256 fee = FEE_SLOT.getUint256();

if (msg.value == 0 || msg.value % (DEPOSIT_SIZE + fee) != 0) {
// review(nmvalera): can we make the message more explicit like "expects value to be a multiple of {DEPOSIT_SIZE + fee} but got {msg.value}
revert InvalidValue();
if (msg.value == 0 || msg.value % DEPOSIT_SIZE != 0) {
revert InvalidMessageValue();
}

uint256 depositCount = msg.value / (DEPOSIT_SIZE + fee);
uint256 validatorCount = VALIDATOR_COUNT_SLOT.getUint256();
uint256 depositCount = msg.value / DEPOSIT_SIZE;
uint256 validatorCount = VALIDATORS_COUNT_SLOT.getUint256();
StateLib.BytesArraySlot storage publicKeysStore = PUBLIC_KEYS_SLOT.getStorageBytesArray();
StateLib.BytesArraySlot storage signaturesStore = SIGNATURES_SLOT.getStorageBytesArray();
bytes32 withdrawalCredentials = WITHDRAWAL_CREDENTIALS_SLOT.getBytes32();
Expand All @@ -220,18 +261,20 @@ contract StakingContract {
revert NotEnoughKeys();
}

StateLib.Bytes32ToAddressMappingSlot storage publicKeyOwnership = WITHDRAWERS_SLOT
.getStorageBytes32ToAddressMapping();

for (uint256 i; i < depositCount; ) {
_useKeys(
publicKeysStore.value[validatorCount + i],
signaturesStore.value[validatorCount + i],
withdrawalCredentials,
_withdrawer
);
bytes memory publicKey = publicKeysStore.value[validatorCount + i];
bytes32 publicKeyRoot = sha256(BytesLib.pad64(publicKey));
_depositValidator(publicKey, signaturesStore.value[validatorCount + i], withdrawalCredentials);
publicKeyOwnership.value[publicKeyRoot] = _withdrawer;
emit Deposit(msg.sender, _withdrawer, publicKey, publicKeyRoot);
unchecked {
++i;
}
}

VALIDATOR_COUNT_SLOT.setUint256(validatorCount + depositCount);
VALIDATORS_COUNT_SLOT.setUint256(validatorCount + depositCount);
}
}
Loading

0 comments on commit bbfd108

Please sign in to comment.