Skip to content
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

Improve comments in the contract's code. #872

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions contracts/0.4.24/nos/NodeOperatorsRegistry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ contract NodeOperatorsRegistry is AragonApp, Versioned {
uint8 internal constant TOTAL_DEPOSITED_KEYS_COUNT_OFFSET = 3;

// TargetValidatorsStats
/// @dev Target limit mode, allows limiting target active validators count for operator (0 = disabled, 1 = soft mode, 2 = forced mode)
/// @dev Target limit mode, allows limiting target active validators count for operator (0 = disabled, 1 = soft mode, 2 = boosted mode)
uint8 internal constant TARGET_LIMIT_MODE_OFFSET = 0;
/// @dev relative target active validators limit for operator, set by DAO
/// @notice used to check how many keys should go to exit, 0 - means all deposited keys would be exited
Expand Down Expand Up @@ -689,7 +689,7 @@ contract NodeOperatorsRegistry is AragonApp, Versioned {

/// @notice Updates the limit of the validators that can be used for deposit by DAO
/// @param _nodeOperatorId Id of the node operator
/// @param _targetLimitMode target limit mode (0 = disabled, 1 = soft mode, 2 = forced mode)
/// @param _targetLimitMode target limit mode (0 = disabled, 1 = soft mode, 2 = boosted mode)
/// @param _targetLimit Target limit of the node operator
function updateTargetValidatorsLimits(uint256 _nodeOperatorId, uint256 _targetLimitMode, uint256 _targetLimit) public {
_onlyExistedNodeOperator(_nodeOperatorId);
Expand Down
39 changes: 25 additions & 14 deletions contracts/0.8.9/DepositSecurityModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ interface IStakingRouter {
/**
* @title DepositSecurityModule
* @dev The contract represents a security module for handling deposits.
*
* The contract allows pausing deposits in response to potential security incidents and
* requires a quorum of guardians to authorize deposit operations. It also provides a mechanism
* to unvet signing keys (a vetted key is a validator key approved for receiving ether deposits)
* in case of any issues.
*/
contract DepositSecurityModule {
/**
Expand Down Expand Up @@ -579,20 +584,7 @@ contract DepositSecurityModule {
bytes calldata vettedSigningKeysCounts,
Signature calldata sig
) external {
/// @dev The most likely reason for the signature to go stale
uint256 onchainNonce = STAKING_ROUTER.getStakingModuleNonce(stakingModuleId);
if (nonce != onchainNonce) revert ModuleNonceChanged();

uint256 nodeOperatorsCount = nodeOperatorIds.length / 8;

if (
nodeOperatorIds.length % 8 != 0 ||
vettedSigningKeysCounts.length % 16 != 0 ||
vettedSigningKeysCounts.length / 16 != nodeOperatorsCount ||
nodeOperatorsCount > maxOperatorsPerUnvetting
) {
revert UnvetPayloadInvalid();
}
_checkIfUnvetPayloadValid(nodeOperatorIds, vettedSigningKeysCounts);

address guardianAddr = msg.sender;
int256 guardianIndex = _getGuardianIndex(msg.sender);
Expand Down Expand Up @@ -625,4 +617,23 @@ contract DepositSecurityModule {
vettedSigningKeysCounts
);
}

function _checkIfUnvetPayloadValid(
uint256 stakingModuleId,
uint256 nonce,
bytes calldata nodeOperatorIds,
bytes calldata vettedSigningKeysCounts
) internal view {
/// @dev The most likely reason for the signature to go stale
uint256 onchainNonce = STAKING_ROUTER.getStakingModuleNonce(stakingModuleId);
if (nonce != onchainNonce) revert ModuleNonceChanged();

uint256 nodeOperatorsCount = nodeOperatorIds.length / 8;

if (
nodeOperatorIds.length % 8 != 0 ||
vettedSigningKeysCounts.length != nodeOperatorsCount * 16 ||
nodeOperatorsCount > maxOperatorsPerUnvetting
) revert UnvetPayloadInvalid();
}
}
2 changes: 1 addition & 1 deletion contracts/0.8.9/interfaces/IStakingModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ interface IStakingModule {

/// @notice Returns all-validators summary belonging to the node operator with the given id
/// @param _nodeOperatorId id of the operator to return report for
/// @return targetLimitMode shows whether the current target limit applied to the node operator (0 = disabled, 1 = soft mode, 2 = forced mode)
/// @return targetLimitMode shows whether the current target limit applied to the node operator (0 = disabled, 1 = soft mode, 2 = boosted mode)
/// @return targetValidatorsCount relative target active validators limit for operator
/// @return stuckValidatorsCount number of validators with an expired request to exit time
/// @return refundedValidatorsCount number of validators that can't be withdrawn, but deposit
Expand Down
53 changes: 27 additions & 26 deletions contracts/0.8.9/oracle/AccountingOracle.sol
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ contract AccountingOracle is BaseOracle {
bytes32 internal constant EXTRA_DATA_PROCESSING_STATE_POSITION =
keccak256("lido.AccountingOracle.extraDataProcessingState");

bytes32 internal constant ZERO_HASH = bytes32(0);
bytes32 internal constant ZERO_BYTES32 = bytes32(0);

address public immutable LIDO;
ILidoLocator public immutable LOCATOR;
Expand Down Expand Up @@ -275,15 +275,7 @@ contract AccountingOracle is BaseOracle {
/// data for a report is possible after its processing deadline passes or a new data report
/// arrives.
///
/// Depending on the size of the extra data, the processing might need to be split into
/// multiple transactions. Each transaction contains a chunk of report data (an array of items)
/// and the hash of the next transaction. The last transaction will contain ZERO_HASH
/// as the next transaction hash.
///
/// | 32 bytes | array of items
/// | nextHash | ...
///
/// Each item being encoded as follows:
/// Extra data is an array of items, each item being encoded as follows:
///
/// 3 bytes 2 bytes X bytes
/// | itemIndex | itemType | itemPayload |
Expand Down Expand Up @@ -356,7 +348,7 @@ contract AccountingOracle is BaseOracle {
/// @dev Hash of the extra data. See the constant defining a specific extra data
/// format for the info on how to calculate the hash.
///
/// Must be set to a zero hash if the oracle report contains no extra data.
/// Must be set to a `ZERO_BYTES32` if the oracle report contains no extra data.
///
bytes32 extraDataHash;

Expand All @@ -374,16 +366,25 @@ contract AccountingOracle is BaseOracle {
///
uint256 public constant EXTRA_DATA_FORMAT_EMPTY = 0;

/// @notice The list format for the extra data array. Used when all extra data processing
/// fits into a single or multiple transactions.
/// @notice The list format for the extra data array. Used when the oracle reports contains extra data.
///
/// Depending on the extra data size, it's passed within a single or multiple transactions.
/// Each transaction contains data consisting of 1) the keccak256 hash of the next
/// transaction's data or `ZERO_BYTES32` if there are no more data chunks, and 2) a chunk
/// of report data (an array of items).
///
/// | 32 bytes | X bytes |
/// | Next transaction's data hash or `ZERO_BYTES32` | array of items |
///
/// Depend on the extra data size it passed within a single or multiple transactions.
/// Each transaction contains next transaction hash and a bytearray containing data items
/// packed tightly.
/// The `extraDataHash` field of the `ReportData` struct is calculated as a keccak256 hash
/// over the first transaction's data, i.e. over the first data chunk with the second
/// transaction's data hash (or `ZERO_BYTES32`) prepended.
///
/// Hash is a keccak256 hash calculated over the transaction data (next transaction hash and bytearray items).
/// The Solidity equivalent of the hash calculation code would be `keccak256(data)`,
/// where `data` has the `bytes` type.
/// ReportData.extraDataHash := hash0
/// hash0 := keccak256(| hash1 | extraData[0], ... extraData[n] |)
/// hash1 := keccak256(| hash2 | extraData[n + 1], ... extraData[m] |)
/// ...
/// hashK := keccak256(| ZERO_BYTES32 | extraData[x + 1], ... extraData[extraDataItemsCount] |)
///
uint256 public constant EXTRA_DATA_FORMAT_LIST = 1;

Expand Down Expand Up @@ -420,7 +421,7 @@ contract AccountingOracle is BaseOracle {

/// @notice Submits report extra data in the EXTRA_DATA_FORMAT_LIST format for processing.
///
/// @param data The extra data chunk with items list. See docs for the `EXTRA_DATA_FORMAT_LIST`
/// @param data The extra data chunk. See docs for the `EXTRA_DATA_FORMAT_LIST`
/// constant for details.
///
function submitReportExtraDataList(bytes calldata data) external {
Expand Down Expand Up @@ -461,7 +462,7 @@ contract AccountingOracle is BaseOracle {
ConsensusReport memory report = _storageConsensusReport().value;
result.currentFrameRefSlot = _getCurrentRefSlot();

if (report.hash == ZERO_HASH || result.currentFrameRefSlot != report.refSlot) {
if (report.hash == ZERO_BYTES32 || result.currentFrameRefSlot != report.refSlot) {
return result;
}

Expand Down Expand Up @@ -585,8 +586,8 @@ contract AccountingOracle is BaseOracle {

function _handleConsensusReportData(ReportData calldata data, uint256 prevRefSlot) internal {
if (data.extraDataFormat == EXTRA_DATA_FORMAT_EMPTY) {
if (data.extraDataHash != ZERO_HASH) {
revert UnexpectedExtraDataHash(ZERO_HASH, data.extraDataHash);
if (data.extraDataHash != ZERO_BYTES32) {
revert UnexpectedExtraDataHash(ZERO_BYTES32, data.extraDataHash);
}
if (data.extraDataItemsCount != 0) {
revert UnexpectedExtraDataItemsCount(0, data.extraDataItemsCount);
Expand All @@ -598,7 +599,7 @@ contract AccountingOracle is BaseOracle {
if (data.extraDataItemsCount == 0) {
revert ExtraDataItemsCountCannotBeZeroForNonEmptyData();
}
if (data.extraDataHash == ZERO_HASH) {
if (data.extraDataHash == ZERO_BYTES32) {
revert ExtraDataHashCannotBeZeroForNonEmptyData();
}
}
Expand Down Expand Up @@ -708,7 +709,7 @@ contract AccountingOracle is BaseOracle {

ConsensusReport memory report = _storageConsensusReport().value;

if (report.hash == ZERO_HASH || procState.refSlot != report.refSlot) {
if (report.hash == ZERO_BYTES32 || procState.refSlot != report.refSlot) {
revert CannotSubmitExtraDataBeforeMainData();
}

Expand Down Expand Up @@ -757,7 +758,7 @@ contract AccountingOracle is BaseOracle {
_processExtraDataItems(data, iter);
uint256 itemsProcessed = iter.index + 1;

if (dataHash == ZERO_HASH) {
if (dataHash == ZERO_BYTES32) {
if (itemsProcessed != procState.itemsCount) {
revert UnexpectedExtraDataItemsCount(procState.itemsCount, itemsProcessed);
}
Expand Down
2 changes: 1 addition & 1 deletion test/0.8.9/stakingRouter/stakingRouter.module-sync.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ describe("StakingRouter:module-sync", () => {

context("updateTargetValidatorsLimits", () => {
const NODE_OPERATOR_ID = 0n;
const TARGET_LIMIT_MODE = 1; // 1 - soft, i.e. on WQ request; 2 - forced
const TARGET_LIMIT_MODE = 1; // 1 - soft, i.e. on WQ request; 2 - boosted
const TARGET_LIMIT = 100n;

it("Reverts if the caller does not have the role", async () => {
Expand Down
Loading