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

Refactor errors naming to follow convention #230

Merged
merged 5 commits into from
Dec 6, 2024
Merged
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
14 changes: 7 additions & 7 deletions contracts/Escrow.sol
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ contract Escrow is ISignallingEscrow, IRageQuitEscrow {

error EmptyUnstETHIds();
error UnclaimedBatches();
error UnexpectedUnstETHId();
error UnfinalizedUnstETHIds();
error NonProxyCallsForbidden();
error BatchesQueueIsNotClosed();
error InvalidBatchSize(uint256 size);
error InvalidFromUnstETHId(uint256 unstETHId);
error CallerIsNotDualGovernance(address caller);
error InvalidHintsLength(uint256 actual, uint256 expected);
error InvalidETHSender(address actual, address expected);
Expand Down Expand Up @@ -489,15 +489,15 @@ contract Escrow is ISignallingEscrow, IRageQuitEscrow {
revert BatchesQueueIsNotClosed();
}

/// @dev This check is primarily required when only unstETH NFTs are locked in the Escrow
/// and there are no WithdrawalsBatches. In this scenario, the RageQuitExtensionPeriod can only begin
/// when the last locked unstETH id is finalized in the WithdrawalQueue.
/// When the WithdrawalsBatchesQueue is not empty, this invariant is maintained by the following:
/// @dev This check is required when only unstETH NFTs are locked in the Escrow and there are no WithdrawalsBatches.
/// In this scenario, the RageQuitExtensionPeriod can only begin when the last locked unstETH id is finalized
/// in the WithdrawalQueue. When the WithdrawalsBatchesQueue is not empty, this invariant is maintained by
/// the following:
/// - Any locked unstETH during the VetoSignalling phase has an id less than any unstETH NFT created
/// during the request for withdrawal batches.
/// - Claiming the withdrawal batches requires the finalization of the unstETH with the given id.
/// - The finalization of unstETH NFTs occurs in FIFO order.
if (_batchesQueue.getLastClaimedOrBoundaryUnstETHId() > WITHDRAWAL_QUEUE.getLastFinalizedRequestId()) {
if (_batchesQueue.getBoundaryUnstETHId() > WITHDRAWAL_QUEUE.getLastFinalizedRequestId()) {
revert UnfinalizedUnstETHIds();
}

Expand Down Expand Up @@ -629,7 +629,7 @@ contract Escrow is ISignallingEscrow, IRageQuitEscrow {
uint256[] memory hints
) internal {
if (fromUnstETHId != unstETHIds[0]) {
revert UnexpectedUnstETHId();
revert InvalidFromUnstETHId(fromUnstETHId);
}

if (hints.length != unstETHIds.length) {
Expand Down
1 change: 0 additions & 1 deletion contracts/interfaces/ISignallingEscrow.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {SharesValue} from "../types/SharesValue.sol";
import {UnstETHRecordStatus} from "../libraries/AssetsAccounting.sol";

import {IEscrowBase} from "./IEscrowBase.sol";
import {IRageQuitEscrow} from "./IRageQuitEscrow.sol";

interface ISignallingEscrow is IEscrowBase {
struct VetoerDetails {
Expand Down
8 changes: 4 additions & 4 deletions contracts/libraries/DualGovernanceStateMachine.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {DualGovernanceConfig} from "./DualGovernanceConfig.sol";
import {DualGovernanceStateTransitions} from "./DualGovernanceStateTransitions.sol";

/// @notice Enum describing the state of the Dual Governance State Machine
/// @param Unset The initial (uninitialized) state of the Dual Governance State Machine. The state machine cannot
/// @param NotInitialized The initial (uninitialized) state of the Dual Governance State Machine. The state machine cannot
/// operate in this state and must be initialized before use.
/// @param Normal The default state where the system is expected to remain most of the time. In this state, proposals
/// can be both submitted and scheduled for execution.
Expand All @@ -32,7 +32,7 @@ import {DualGovernanceStateTransitions} from "./DualGovernanceStateTransitions.s
/// is triggered when the Second Seal Threshold is reached. During this state, the scheduling of proposals for
/// execution is forbidden, but new proposals can still be submitted.
enum State {
Unset,
NotInitialized,
Normal,
VetoSignalling,
VetoSignallingDeactivation,
Expand Down Expand Up @@ -118,7 +118,7 @@ library DualGovernanceStateMachine {
IDualGovernanceConfigProvider configProvider,
IEscrowBase escrowMasterCopy
) internal {
if (self.state != State.Unset) {
if (self.state != State.NotInitialized) {
revert AlreadyInitialized();
}

Expand All @@ -130,7 +130,7 @@ library DualGovernanceStateMachine {
DualGovernanceConfig.Context memory config = configProvider.getDualGovernanceConfig();
_deployNewSignallingEscrow(self, escrowMasterCopy, config.minAssetsLockDuration);

emit DualGovernanceStateChanged(State.Unset, State.Normal, self);
emit DualGovernanceStateChanged(State.NotInitialized, State.Normal, self);
}

/// @notice Executes a state transition for the Dual Governance State Machine, if applicable.
Expand Down
4 changes: 2 additions & 2 deletions contracts/libraries/EmergencyProtection.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ library EmergencyProtection {
error InvalidEmergencyExecutionCommittee(address committee);
error InvalidEmergencyModeDuration(Duration value);
error InvalidEmergencyProtectionEndDate(Timestamp value);
error UnexpectedEmergencyModeState(bool value);
error UnexpectedEmergencyModeState(bool state);

// ---
// Events
Expand Down Expand Up @@ -197,7 +197,7 @@ library EmergencyProtection {
/// @param isActive The expected value of the emergency mode.
function checkEmergencyMode(Context storage self, bool isActive) internal view {
if (isEmergencyModeActive(self) != isActive) {
revert UnexpectedEmergencyModeState(isActive);
revert UnexpectedEmergencyModeState(!isActive);
}
}

Expand Down
4 changes: 2 additions & 2 deletions contracts/libraries/EscrowState.sol
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ library EscrowState {
// ---

error ClaimingIsFinished();
error UnexpectedState(State value);
error UnexpectedEscrowState(State state);
error EthWithdrawalsDelayNotPassed();
error RageQuitExtensionPeriodNotStarted();
error InvalidMinAssetsLockDuration(Duration newMinAssetsLockDuration);
Expand Down Expand Up @@ -184,7 +184,7 @@ library EscrowState {
/// @param state The expected state.
function _checkState(Context storage self, State state) private view {
if (self.state != state) {
revert UnexpectedState(state);
revert UnexpectedEscrowState(self.state);
}
}

Expand Down
80 changes: 38 additions & 42 deletions contracts/libraries/ExecutableProposals.sol
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,7 @@ library ExecutableProposals {
// ---

error EmptyCalls();
error ProposalNotFound(uint256 proposalId);
error ProposalNotScheduled(uint256 proposalId);
error ProposalNotSubmitted(uint256 proposalId);
error UnexpectedProposalStatus(uint256 proposalId, Status status);
error AfterSubmitDelayNotPassed(uint256 proposalId);
error AfterScheduleDelayNotPassed(uint256 proposalId);

Expand Down Expand Up @@ -141,19 +139,21 @@ library ExecutableProposals {
/// @param afterSubmitDelay The required delay duration after submission before the proposal can be scheduled.
///
function schedule(Context storage self, uint256 proposalId, Duration afterSubmitDelay) internal {
ProposalData memory proposalState = self.proposals[proposalId].data;
ProposalData memory proposalData = self.proposals[proposalId].data;

_checkProposalNotCancelled(self, proposalId, proposalData);

if (!_isProposalSubmitted(self, proposalId, proposalState)) {
revert ProposalNotSubmitted(proposalId);
if (proposalData.status != Status.Submitted) {
revert UnexpectedProposalStatus(proposalId, proposalData.status);
}

if (afterSubmitDelay.addTo(proposalState.submittedAt) > Timestamps.now()) {
if (afterSubmitDelay.addTo(proposalData.submittedAt) > Timestamps.now()) {
revert AfterSubmitDelayNotPassed(proposalId);
}

proposalState.status = Status.Scheduled;
proposalState.scheduledAt = Timestamps.now();
self.proposals[proposalId].data = proposalState;
proposalData.status = Status.Scheduled;
proposalData.scheduledAt = Timestamps.now();
self.proposals[proposalId].data = proposalData;

emit ProposalScheduled(proposalId);
}
Expand All @@ -166,8 +166,10 @@ library ExecutableProposals {
function execute(Context storage self, uint256 proposalId, Duration afterScheduleDelay) internal {
Proposal memory proposal = self.proposals[proposalId];

if (!_isProposalScheduled(self, proposalId, proposal.data)) {
revert ProposalNotScheduled(proposalId);
_checkProposalNotCancelled(self, proposalId, proposal.data);

if (proposal.data.status != Status.Scheduled) {
revert UnexpectedProposalStatus(proposalId, proposal.data.status);
}

if (afterScheduleDelay.addTo(proposal.data.scheduledAt) > Timestamps.now()) {
Expand All @@ -192,21 +194,6 @@ library ExecutableProposals {
// Getters
// ---

/// @notice Determines whether a proposal is eligible for execution based on its status and delay requirements.
/// @param self The context of the Executable Proposal library.
/// @param proposalId The id of the proposal to check for execution eligibility.
/// @param afterScheduleDelay The required delay duration after scheduling before the proposal can be executed.
/// @return bool `true` if the proposal is eligible for execution, otherwise `false`.
function canExecute(
Context storage self,
uint256 proposalId,
Duration afterScheduleDelay
) internal view returns (bool) {
ProposalData memory proposalState = self.proposals[proposalId].data;
return _isProposalScheduled(self, proposalId, proposalState)
&& Timestamps.now() >= afterScheduleDelay.addTo(proposalState.scheduledAt);
}

/// @notice Determines whether a proposal is eligible to be scheduled based on its status and required delay.
/// @param self The context of the Executable Proposal library.
/// @param proposalId The id of the proposal to check for scheduling eligibility.
Expand All @@ -217,9 +204,24 @@ library ExecutableProposals {
uint256 proposalId,
Duration afterSubmitDelay
) internal view returns (bool) {
ProposalData memory proposalState = self.proposals[proposalId].data;
return _isProposalSubmitted(self, proposalId, proposalState)
&& Timestamps.now() >= afterSubmitDelay.addTo(proposalState.submittedAt);
ProposalData memory proposalData = self.proposals[proposalId].data;
return proposalId > self.lastCancelledProposalId && proposalData.status == Status.Submitted
&& Timestamps.now() >= afterSubmitDelay.addTo(proposalData.submittedAt);
}

/// @notice Determines whether a proposal is eligible for execution based on its status and delay requirements.
/// @param self The context of the Executable Proposal library.
/// @param proposalId The id of the proposal to check for execution eligibility.
/// @param afterScheduleDelay The required delay duration after scheduling before the proposal can be executed.
/// @return bool `true` if the proposal is eligible for execution, otherwise `false`.
function canExecute(
Context storage self,
uint256 proposalId,
Duration afterScheduleDelay
) internal view returns (bool) {
ProposalData memory proposalData = self.proposals[proposalId].data;
return proposalId > self.lastCancelledProposalId && proposalData.status == Status.Scheduled
&& Timestamps.now() >= afterScheduleDelay.addTo(proposalData.scheduledAt);
}

/// @notice Returns the total count of submitted proposals.
Expand Down Expand Up @@ -268,24 +270,18 @@ library ExecutableProposals {

function _checkProposalExists(uint256 proposalId, ProposalData memory proposalData) private pure {
if (proposalData.status == Status.NotExist) {
revert ProposalNotFound(proposalId);
revert UnexpectedProposalStatus(proposalId, Status.NotExist);
}
}

function _isProposalSubmitted(
function _checkProposalNotCancelled(
Context storage self,
uint256 proposalId,
ProposalData memory proposalData
) private view returns (bool) {
return proposalId > self.lastCancelledProposalId && proposalData.status == Status.Submitted;
}

function _isProposalScheduled(
Context storage self,
uint256 proposalId,
ProposalData memory proposalData
) private view returns (bool) {
return proposalId > self.lastCancelledProposalId && proposalData.status == Status.Scheduled;
) private view {
if (_isProposalCancelled(self, proposalId, proposalData)) {
revert UnexpectedProposalStatus(proposalId, Status.Cancelled);
}
}

function _isProposalCancelled(
Expand Down
40 changes: 15 additions & 25 deletions contracts/libraries/WithdrawalsBatchesQueue.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@ import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";

/// @notice The state of the WithdrawalBatchesQueue.
/// @param Absent The initial (uninitialized) state of the WithdrawalBatchesQueue.
/// @param NotInitialized The initial (uninitialized) state of the WithdrawalBatchesQueue.
/// @param Opened In this state, the WithdrawalBatchesQueue allows the addition of new batches of unstETH ids.
/// @param Closed The terminal state of the queue where adding new batches is no longer permitted.
enum State {
Absent,
NotInitialized,
Opened,
Closed
}
Expand All @@ -23,9 +23,7 @@ library WithdrawalsBatchesQueue {

error EmptyBatch();
error InvalidUnstETHIdsSequence();
error WithdrawalsBatchesQueueIsInAbsentState();
error WithdrawalsBatchesQueueIsNotInOpenedState();
error WithdrawalsBatchesQueueIsNotInAbsentState();
error UnexpectedWithdrawalsBatchesQueueState(State state);

// ---
// Events
Expand Down Expand Up @@ -91,9 +89,7 @@ library WithdrawalsBatchesQueue {
/// @param boundaryUnstETHId The id of the unstETH NFT which is used as the boundary value for the withdrawal queue.
/// `boundaryUnstETHId` value is used as a lower bound for the adding unstETH ids.
function open(Context storage self, uint256 boundaryUnstETHId) internal {
if (self.info.state != State.Absent) {
revert WithdrawalsBatchesQueueIsNotInAbsentState();
}
_checkState(self, State.NotInitialized);

self.info.state = State.Opened;

Expand All @@ -108,7 +104,7 @@ library WithdrawalsBatchesQueue {
/// @param self The context of the Withdrawals Batches Queue library.
/// @param unstETHIds An array of sequential unstETH ids to be added to the queue.
function addUnstETHIds(Context storage self, uint256[] memory unstETHIds) internal {
_checkInOpenedState(self);
_checkState(self, State.Opened);

uint256 unstETHIdsCount = unstETHIds.length;

Expand Down Expand Up @@ -163,7 +159,7 @@ library WithdrawalsBatchesQueue {
/// @notice Closes the WithdrawalsBatchesQueue, preventing further batch additions.
/// @param self The context of the Withdrawals Batches Queue library.
function close(Context storage self) internal {
_checkInOpenedState(self);
_checkState(self, State.Opened);
self.info.state = State.Closed;
emit WithdrawalsBatchesQueueClosed();
}
Expand Down Expand Up @@ -220,13 +216,13 @@ library WithdrawalsBatchesQueue {
return self.info.totalUnstETHIdsCount - self.info.totalUnstETHIdsClaimed;
}

/// @notice Returns the id of the last claimed unstETH. When the queue is empty, returns 0.
/// @notice Returns the id of the boundary unstETH.
/// @dev Reverts with an index OOB error if called when the `WithdrawalsBatchesQueue` is in the
/// `NotInitialized` state.
/// @param self The context of the Withdrawals Batches Queue library.
/// @return lastClaimedUnstETHId The id of the lastClaimedUnstETHId or 0 when the queue is empty
function getLastClaimedOrBoundaryUnstETHId(Context storage self) internal view returns (uint256) {
_checkNotInAbsentState(self);
QueueInfo memory info = self.info;
return self.batches[info.lastClaimedBatchIndex].firstUnstETHId + info.lastClaimedUnstETHIdIndex;
/// @return boundaryUnstETHId The id of the boundary unstETH.
function getBoundaryUnstETHId(Context storage self) internal view returns (uint256) {
return self.batches[0].firstUnstETHId;
}

/// @notice Returns if all unstETH ids in the queue have been claimed.
Expand Down Expand Up @@ -277,15 +273,9 @@ library WithdrawalsBatchesQueue {
info.totalUnstETHIdsClaimed += SafeCast.toUint64(unstETHIdsCount);
}

function _checkNotInAbsentState(Context storage self) private view {
if (self.info.state == State.Absent) {
revert WithdrawalsBatchesQueueIsInAbsentState();
}
}

function _checkInOpenedState(Context storage self) private view {
if (self.info.state != State.Opened) {
revert WithdrawalsBatchesQueueIsNotInOpenedState();
function _checkState(Context storage self, State expectedState) private view {
if (self.info.state != expectedState) {
revert UnexpectedWithdrawalsBatchesQueueState(self.info.state);
}
}
}
2 changes: 1 addition & 1 deletion docs/specification.md
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ This contract is a singleton, meaning that any DG deployment includes exactly on

```solidity
enum State {
Unset, // Indicates an uninitialized state during the contract creation
NotInitialized, // Indicates an uninitialized state during the contract creation
Normal,
VetoSignalling,
VetoSignallingDeactivation,
Expand Down
4 changes: 3 additions & 1 deletion test/scenario/dg-update-tokens-rotation.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,9 @@ contract DualGovernanceUpdateTokensRotation is ScenarioTestBlueprint {
_step("7. After the update malicious proposal is cancelled and can't be executed via new DualGovernance");
{
vm.expectRevert(
abi.encodeWithSelector(ExecutableProposals.ProposalNotSubmitted.selector, maliciousProposalId)
abi.encodeWithSelector(
ExecutableProposals.UnexpectedProposalStatus.selector, maliciousProposalId, ProposalStatus.Cancelled
)
);
newDualGovernanceInstance.scheduleProposal(maliciousProposalId);

Expand Down
Loading
Loading