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: use block numbers to track time #210

Merged
merged 12 commits into from
Jun 19, 2023
2 changes: 1 addition & 1 deletion .forge-snapshots/ProposeSigComp.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
149997
148973
2 changes: 1 addition & 1 deletion .forge-snapshots/VoteSigComp.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
50700
50407
2 changes: 1 addition & 1 deletion .forge-snapshots/VoteSigCompMetadata.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
52243
51957
2 changes: 1 addition & 1 deletion .forge-snapshots/VoteTxComp.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
44270
43954
2 changes: 1 addition & 1 deletion .forge-snapshots/VoteTxCompMetadata.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
45854
45545
3 changes: 1 addition & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ src
│ ├─ SXHash.sol - "Snapshot X Types Hashing Library"
│ ├─ SXUtils.sol - "Snapshot X Types Utilities Library"
│ ├─ SignatureVerifier.sol - "Verifies EIP712 Signatures for Snapshot X actions"
│ ├─ SpaceManager.sol - "Manages a whitelist of Spaces that have permissions to execute transactions"
│ └─ TimestampResolver.sol - "Resolves timestamps to block numbers"
│ └─ SpaceManager.sol - "Manages a whitelist of Spaces that have permissions to execute transactions"
├─ ProxyFactory.sol - "Handles the deployment and tracking of Space contracts"
└─ Space.sol - "The base contract for each Snapshot X space"
└─ types.sol - "Definitions for Snapshot X custom types"
Expand Down
35 changes: 15 additions & 20 deletions src/Space.sol
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,6 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
Strategy calldata executionStrategy,
bytes calldata userProposalValidationParams
) external override onlyAuthenticator {
// Casting to `uint32` is fine because this gives us until year ~2106.
uint32 snapshotTimestamp = uint32(block.timestamp);

if (
!IProposalValidationStrategy(proposalValidationStrategy.addr).validate(
author,
Expand All @@ -215,20 +212,20 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
)
) revert FailedToPassProposalValidation();

uint32 startTimestamp = snapshotTimestamp + votingDelay;
uint32 minEndTimestamp = startTimestamp + minVotingDuration;
uint32 maxEndTimestamp = startTimestamp + maxVotingDuration;
// Max block number of 2^32 - 1 = 4,294,967,295
uint32 startBlockNumber = uint32(block.number) + votingDelay;
uint32 minEndBlockNumber = startBlockNumber + minVotingDuration;
uint32 maxEndBlockNumber = startBlockNumber + maxVotingDuration;

// The execution payload is the params of the supplied execution strategy struct.
bytes32 executionPayloadHash = keccak256(executionStrategy.params);

Proposal memory proposal = Proposal(
author,
snapshotTimestamp,
startTimestamp,
startBlockNumber,
IExecutionStrategy(executionStrategy.addr),
minEndTimestamp,
maxEndTimestamp,
minEndBlockNumber,
maxEndBlockNumber,
FinalizationStatus.Pending,
executionPayloadHash,
activeVotingStrategies
Expand All @@ -250,16 +247,16 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
) external override onlyAuthenticator {
Proposal memory proposal = proposals[proposalId];
_assertProposalExists(proposal);
if (block.timestamp >= proposal.maxEndTimestamp) revert VotingPeriodHasEnded();
if (block.timestamp < proposal.startTimestamp) revert VotingPeriodHasNotStarted();
if (block.number >= proposal.maxEndBlockNumber) revert VotingPeriodHasEnded();
if (block.number < proposal.startBlockNumber) revert VotingPeriodHasNotStarted();
if (proposal.finalizationStatus != FinalizationStatus.Pending) revert ProposalFinalized();
if (voteRegistry[proposalId][voter]) revert UserAlreadyVoted();

voteRegistry[proposalId][voter] = true;

uint256 votingPower = _getCumulativePower(
voter,
proposal.snapshotTimestamp,
proposal.startBlockNumber,
userVotingStrategies,
proposal.activeVotingStrategies
);
Expand Down Expand Up @@ -313,7 +310,7 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
Proposal storage proposal = proposals[proposalId];
_assertProposalExists(proposal);
if (author != proposal.author) revert InvalidCaller();
if (block.timestamp >= proposal.startTimestamp) revert VotingDelayHasPassed();
if (block.number >= proposal.startBlockNumber) revert VotingDelayHasPassed();

proposal.executionPayloadHash = keccak256(executionStrategy.params);
proposal.executionStrategy = IExecutionStrategy(executionStrategy.addr);
Expand Down Expand Up @@ -399,16 +396,14 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra

/// @dev Reverts if a specified proposal does not exist.
function _assertProposalExists(Proposal memory proposal) internal pure {
// startTimestamp cannot be set to 0 when a proposal is created,
// so if proposal.startTimestamp is 0 it means this proposal does not exist
// and hence `proposalId` is invalid.
if (proposal.startTimestamp == 0) revert InvalidProposal();
// If a proposal exists, then its execution payload hash will be non-zero.
if (proposal.executionPayloadHash == 0) revert InvalidProposal();
Comment on lines +399 to +400
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really impossible to create a proposal with an empty execution payload? Imagine we want to "ratify" something on-chian, then the execution payload could be empty right?

I think proposal.startBlockNumber == 0 would be a better check here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then the payload hash would be keccak(0) so non zero. Wheras it would technically be possible to have a start block number of zero (ie the proposal was created in the genesis block and there is no voting delay)

}

/// @dev Returns the cumulative voting power of a user over a set of voting strategies.
function _getCumulativePower(
address userAddress,
uint32 timestamp,
uint32 blockNumber,
IndexedStrategy[] calldata userStrategies,
uint256 allowedStrategies
) internal returns (uint256) {
Expand All @@ -427,7 +422,7 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
Strategy memory strategy = votingStrategies[strategyIndex];

totalVotingPower += IVotingStrategy(strategy.addr).getVotingPower(
timestamp,
blockNumber,
userAddress,
strategy.params,
userStrategies[i].params
Expand Down
14 changes: 7 additions & 7 deletions src/execution-strategies/EmergencyQuorumExecutionStrategy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,12 @@ abstract contract EmergencyQuorumExecutionStrategy is IExecutionStrategy, SpaceM
return ProposalStatus.Cancelled;
} else if (proposal.finalizationStatus == FinalizationStatus.Executed) {
return ProposalStatus.Executed;
} else if (block.timestamp < proposal.startTimestamp) {
} else if (block.number < proposal.startBlockNumber) {
return ProposalStatus.VotingDelay;
} else if (emergencyQuorumReached) {
if (_supported(votesFor, votesAgainst)) {
// Proposal is supported
if (block.timestamp < proposal.maxEndTimestamp) {
if (block.number < proposal.maxEndBlockNumber) {
// New votes can still come in so return `VotingPeriodAccepted`.
return ProposalStatus.VotingPeriodAccepted;
} else {
Expand All @@ -70,19 +70,19 @@ abstract contract EmergencyQuorumExecutionStrategy is IExecutionStrategy, SpaceM
}
} else {
// Proposal is not supported
if (block.timestamp < proposal.maxEndTimestamp) {
if (block.number < proposal.maxEndBlockNumber) {
// New votes might still come in so return `VotingPeriod`.
return ProposalStatus.VotingPeriod;
} else {
// New votes can't come in, so it's definitely rejected.
return ProposalStatus.Rejected;
}
}
} else if (block.timestamp < proposal.minEndTimestamp) {
// Proposal has not reached minEndTimestamp yet.
} else if (block.number < proposal.minEndBlockNumber) {
// Proposal has not reached minEndBlockNumber yet.
return ProposalStatus.VotingPeriod;
} else if (block.timestamp < proposal.maxEndTimestamp) {
// Timestamp is between minEndTimestamp and maxEndTimestamp
} else if (block.number < proposal.maxEndBlockNumber) {
// block number is between minEndBlockNumber and maxEndBlockNumber
if (accepted) {
return ProposalStatus.VotingPeriodAccepted;
} else {
Expand Down
12 changes: 6 additions & 6 deletions src/execution-strategies/OptimisticQuorumExecutionStrategy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -48,19 +48,19 @@ abstract contract OptimisticQuorumExecutionStrategy is IExecutionStrategy, Space
return ProposalStatus.Cancelled;
} else if (proposal.finalizationStatus == FinalizationStatus.Executed) {
return ProposalStatus.Executed;
} else if (block.timestamp < proposal.startTimestamp) {
} else if (block.number < proposal.startBlockNumber) {
return ProposalStatus.VotingDelay;
} else if (rejected) {
// We're past the vote start. If it has been rejected, we can short-circuit and return Rejected.
return ProposalStatus.Rejected;
} else if (block.timestamp < proposal.minEndTimestamp) {
// minEndTimestamp not reached, indicate we're still in the voting period.
} else if (block.number < proposal.minEndBlockNumber) {
// minEndBlockNumber not reached, indicate we're still in the voting period.
return ProposalStatus.VotingPeriod;
} else if (block.timestamp < proposal.maxEndTimestamp) {
// minEndTimestamp < now < maxEndTimestamp ; if not `rejected`, we can indicate it can be `accepted`.
} else if (block.number < proposal.maxEndBlockNumber) {
// minEndBlockNumber < block.number < maxEndBlockNumber ; if not `rejected`, we can indicate it can be `accepted`.
return ProposalStatus.VotingPeriodAccepted;
} else {
// maxEndTimestamp < now ; proposal has not been `rejected` ; we can indicate it's `accepted`.
// maxEndBlockNumber < block.number ; proposal has not been `rejected` ; we can indicate it's `accepted`.
return ProposalStatus.Accepted;
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/execution-strategies/SimpleQuorumExecutionStrategy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,11 @@ abstract contract SimpleQuorumExecutionStrategy is IExecutionStrategy, SpaceMana
return ProposalStatus.Cancelled;
} else if (proposal.finalizationStatus == FinalizationStatus.Executed) {
return ProposalStatus.Executed;
} else if (block.timestamp < proposal.startTimestamp) {
} else if (block.number < proposal.startBlockNumber) {
return ProposalStatus.VotingDelay;
} else if (block.timestamp < proposal.minEndTimestamp) {
} else if (block.number < proposal.minEndBlockNumber) {
return ProposalStatus.VotingPeriod;
} else if (block.timestamp < proposal.maxEndTimestamp) {
} else if (block.number < proposal.maxEndBlockNumber) {
if (accepted) {
return ProposalStatus.VotingPeriodAccepted;
} else {
Expand Down
14 changes: 7 additions & 7 deletions src/interfaces/IVotingStrategy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,19 @@ pragma solidity ^0.8.18;

/// @title Voting Strategy Interface
interface IVotingStrategy {
/// @notice Gets the voting power of an address at a given timestamp.
/// @param timestamp The snapshot timestamp to get the voting power at. If a particular voting strategy
/// requires a block number instead of a timestamp, the strategy should resolve the
/// timestamp to a block number.
/// @notice Gets the voting power of an address at a given block number.
/// @param blockNumber The snapshot block number to get the voting power at. If a particular voting strategy
/// requires a block number instead of a block number, the strategy should resolve the
/// block number to a block number.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can remove the whole "resolve" part

/// @param voter The address to get the voting power of.
/// @param params The global parameters that can configure the voting strategy for a particular Space.
/// @param userParams The user parameters that can be used in the voting strategy computation.
/// @return votingPower The voting power of the address at the given timestamp. If there is no voting power,
/// @return votingPower The voting power of the address at the given block number. If there is no voting power,
/// return 0.
function getVotingPower(
uint32 timestamp,
uint32 blockNumber,
address voter,
bytes calldata params,
bytes calldata userParams
) external returns (uint256 votingPower);
) external view returns (uint256 votingPower);
}
16 changes: 7 additions & 9 deletions src/interfaces/space/ISpaceState.sol
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,11 @@ interface ISpaceState {
/// @dev Returns all zeros if the proposal does not exist.
/// @param proposalId The ID of the proposal.
/// @return author The address of the proposal author.
/// @return snapshotTimestamp The timestamp of the proposal snapshot.
/// All Voting Power will be calculated at this timestamp.
/// @return startTimestamp The timestamp of the start of the voting period.
/// @return startBlockNumber The block number of the start of the voting period.
/// This is also the snapshot block number where voting power is calculated at.
/// @return executionStrategy The address of the execution strategy used in the proposal.
/// @return minEndTimestamp The timestamp of the minimum end of the voting period.
/// @return maxEndTimestamp The timestamp of the maximum end of the voting period.
/// @return minEndBlockNumber The block number of the minimum end of the voting period.
/// @return maxEndBlockNumber The block number of the maximum end of the voting period.
/// @return finalizationStatus The finalization status of the proposal. See `FinalizationStatus`.
/// @return executionPayloadHash The keccak256 hash of the execution payload.
/// @return activeVotingStrategies The bit array of the active voting strategies for the proposal.
Expand All @@ -71,11 +70,10 @@ interface ISpaceState {
view
returns (
address author,
uint32 snapshotTimestamp,
uint32 startTimestamp,
uint32 startBlockNumber,
IExecutionStrategy executionStrategy,
uint32 minEndTimestamp,
uint32 maxEndTimestamp,
uint32 minEndBlockNumber,
uint32 maxEndBlockNumber,
FinalizationStatus finalizationStatus,
bytes32 executionPayloadHash,
uint256 activeVotingStrategies
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ contract PropositionPowerAndActiveProposalsLimiterValidationStrategy is
IProposalValidationStrategy
{
/// @notice Validates an author by checking if the proposition power of the author exceeds a threshold over a set of
/// strategies and if the author has reached the maximum number of active proposals at the current timestamp.
/// strategies and if the author has reached the maximum number of active proposals at the current block.
/// @param author Author of the proposal.
/// @param params ABI encoded array that should contain the following:
/// cooldown: Duration to wait before the proposal counter gets reset.
Expand Down
15 changes: 7 additions & 8 deletions src/types.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,17 @@ struct Proposal {
// SLOT 1:
// The address of the proposal creator.
address author;
// The timestamp at which voting power for the proposal is calculated.
uint32 snapshotTimestamp;
// The timestamp at which the voting period starts.
uint32 startTimestamp;
// The block number at which the voting period starts.
// This is also the snapshot block number where voting power is calculated at.
uint32 startBlockNumber;
//
// SLOT 2:
// The address of execution strategy used for the proposal.
IExecutionStrategy executionStrategy;
// The minimum timestamp at which the proposal can be finalized.
uint32 minEndTimestamp;
// The maximum timestamp at which the proposal can be finalized.
uint32 maxEndTimestamp;
// The minimum block number at which the proposal can be finalized.
uint32 minEndBlockNumber;
// The maximum block number at which the proposal can be finalized.
uint32 maxEndBlockNumber;
// An enum that stores whether a proposal is pending, executed, or cancelled.
FinalizationStatus finalizationStatus;
//
Expand Down
8 changes: 4 additions & 4 deletions src/utils/PropositionPower.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@ abstract contract PropositionPower {
Strategy[] memory allowedStrategies,
IndexedStrategy[] memory userStrategies
) internal returns (bool) {
uint256 votingPower = _getCumulativePower(author, uint32(block.timestamp), userStrategies, allowedStrategies);
uint256 votingPower = _getCumulativePower(author, uint32(block.number), userStrategies, allowedStrategies);
return (votingPower >= proposalThreshold);
}

/// @dev Computes the cumulative proposition power of an address at a given timestamp over a set of strategies.
/// @dev Computes the cumulative proposition power of an address at a given block number over a set of strategies.
function _getCumulativePower(
address userAddress,
uint32 timestamp,
uint32 blockNumber,
IndexedStrategy[] memory userStrategies,
Strategy[] memory allowedStrategies
) internal returns (uint256) {
Expand All @@ -44,7 +44,7 @@ abstract contract PropositionPower {
Strategy memory strategy = allowedStrategies[strategyIndex];

totalVotingPower += IVotingStrategy(strategy.addr).getVotingPower(
timestamp,
blockNumber,
userAddress,
strategy.params,
userStrategies[i].params
Expand Down
38 changes: 0 additions & 38 deletions src/utils/TimestampResolver.sol

This file was deleted.

Loading