Skip to content

Commit

Permalink
refactor: try to fix stack-too-deep with less diff noise
Browse files Browse the repository at this point in the history
  • Loading branch information
@danielattilasimon
danielattilasimon committed Dec 3, 2024
1 parent f632a9c commit 17e7be1
Showing 1 changed file with 134 additions and 137 deletions.
271 changes: 134 additions & 137 deletions src/Governance.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -713,11 +713,14 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
}

// Avoid "stack too deep" by placing these variables in memory
struct AllocateLQTYVars {
struct AllocateLQTYMemory {
VoteSnapshot votesSnapshot_;
GlobalState state;
uint16 currentEpoch;
UserState userState;
InitiativeVoteSnapshot votesForInitiativeSnapshot_;
InitiativeState initiativeState;
InitiativeState prevInitiativeState;
Allocation allocation;
}

/// @dev For each given initiative applies relative changes to the allocation
Expand All @@ -733,162 +736,156 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
"Governance: array-length-mismatch"
);

AllocateLQTYVars memory vars;
AllocateLQTYMemory memory vars;
(vars.votesSnapshot_, vars.state) = _snapshotVotes();
vars.currentEpoch = epoch();
uint16 currentEpoch = epoch();
vars.userState = userStates[msg.sender];

for (uint256 i = 0; i < _initiatives.length; i++) {
_allocateLQTYToInitiative(_initiatives[i], _deltaLQTYVotes[i], _deltaLQTYVetos[i], vars);
}
address initiative = _initiatives[i];
int88 deltaLQTYVotes = _deltaLQTYVotes[i];
int88 deltaLQTYVetos = _deltaLQTYVetos[i];
assert(deltaLQTYVotes != 0 || deltaLQTYVetos != 0);

/// === Check FSM === ///
// Can vote positively in SKIP, CLAIMABLE, CLAIMED and UNREGISTERABLE states
// Force to remove votes if disabled
// Can remove votes and vetos in every stage
(vars.votesForInitiativeSnapshot_, vars.initiativeState) = _snapshotVotesForInitiative(initiative);

(InitiativeStatus status,,) = getInitiativeState(
initiative, vars.votesSnapshot_, vars.votesForInitiativeSnapshot_, vars.initiativeState
);

require(
vars.userState.allocatedLQTY == 0
|| vars.userState.allocatedLQTY <= uint88(stakingV1.stakes(deriveUserProxyAddress(msg.sender))),
"Governance: insufficient-or-allocated-lqty"
);
if (deltaLQTYVotes > 0 || deltaLQTYVetos > 0) {
/// @audit You cannot vote on `unregisterable` but a vote may have been there
require(
status == InitiativeStatus.SKIP || status == InitiativeStatus.CLAIMABLE
|| status == InitiativeStatus.CLAIMED,
"Governance: active-vote-fsm"
);
}

globalState = vars.state;
userStates[msg.sender] = vars.userState;
}
if (status == InitiativeStatus.DISABLED) {
require(deltaLQTYVotes <= 0 && deltaLQTYVetos <= 0, "Must be a withdrawal");
}

function _allocateLQTYToInitiative(
address initiative,
int88 deltaLQTYVotes,
int88 deltaLQTYVetos,
AllocateLQTYVars memory vars
) internal {
assert(deltaLQTYVotes != 0 || deltaLQTYVetos != 0);
/// === UPDATE ACCOUNTING === ///
// == INITIATIVE STATE == //

/// === Check FSM === ///
// Can vote positively in SKIP, CLAIMABLE, CLAIMED and UNREGISTERABLE states
// Force to remove votes if disabled
// Can remove votes and vetos in every stage
(InitiativeVoteSnapshot memory votesForInitiativeSnapshot_, InitiativeState memory initiativeState) =
_snapshotVotesForInitiative(initiative);
// deep copy of the initiative's state before the allocation
vars.prevInitiativeState = InitiativeState(
vars.initiativeState.voteLQTY,
vars.initiativeState.vetoLQTY,
vars.initiativeState.averageStakingTimestampVoteLQTY,
vars.initiativeState.averageStakingTimestampVetoLQTY,
vars.initiativeState.lastEpochClaim
);

(InitiativeStatus status,,) =
getInitiativeState(initiative, vars.votesSnapshot_, votesForInitiativeSnapshot_, initiativeState);

if (deltaLQTYVotes > 0 || deltaLQTYVetos > 0) {
/// @audit You cannot vote on `unregisterable` but a vote may have been there
require(
status == InitiativeStatus.SKIP || status == InitiativeStatus.CLAIMABLE
|| status == InitiativeStatus.CLAIMED,
"Governance: active-vote-fsm"
// update the average staking timestamp for the initiative based on the user's average staking timestamp
vars.initiativeState.averageStakingTimestampVoteLQTY = _calculateAverageTimestamp(
vars.initiativeState.averageStakingTimestampVoteLQTY,
vars.userState.averageStakingTimestamp,
/// @audit This is wrong unless we enforce a reset on deposit and withdrawal
vars.initiativeState.voteLQTY,
add(vars.initiativeState.voteLQTY, deltaLQTYVotes)
);
vars.initiativeState.averageStakingTimestampVetoLQTY = _calculateAverageTimestamp(
vars.initiativeState.averageStakingTimestampVetoLQTY,
vars.userState.averageStakingTimestamp,
/// @audit This is wrong unless we enforce a reset on deposit and withdrawal
vars.initiativeState.vetoLQTY,
add(vars.initiativeState.vetoLQTY, deltaLQTYVetos)
);
}

if (status == InitiativeStatus.DISABLED) {
require(deltaLQTYVotes <= 0 && deltaLQTYVetos <= 0, "Must be a withdrawal");
}
// allocate the voting and vetoing LQTY to the initiative
vars.initiativeState.voteLQTY = add(vars.initiativeState.voteLQTY, deltaLQTYVotes);
vars.initiativeState.vetoLQTY = add(vars.initiativeState.vetoLQTY, deltaLQTYVetos);

// update the initiative's state
initiativeStates[initiative] = vars.initiativeState;

// == GLOBAL STATE == //

// TODO: Veto reducing total votes logic change
// TODO: Accounting invariants
// TODO: Let's say I want to cap the votes vs weights
// Then by definition, I add the effective LQTY
// And the effective TS
// I remove the previous one
// and add the next one
// Veto > Vote
// Reduce down by Vote (cap min)
// If Vote > Veto
// Increase by Veto - Veto (reduced max)

// update the average staking timestamp for all counted voting LQTY
/// Discount previous only if the initiative was not unregistered

/// @audit We update the state only for non-disabled initiaitives
/// Disabled initiaitves have had their totals subtracted already
/// Math is also non associative so we cannot easily compare values
if (status != InitiativeStatus.DISABLED) {
/// @audit Trophy: `test_property_sum_of_lqty_global_user_matches_0`
/// Removing votes from state desynchs the state until all users remove their votes from the initiative
/// The invariant that holds is: the one that removes the initiatives that have been unregistered
vars.state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp(
vars.state.countedVoteLQTYAverageTimestamp,
vars.prevInitiativeState.averageStakingTimestampVoteLQTY,
/// @audit We don't have a test that fails when this line is changed
vars.state.countedVoteLQTY,
vars.state.countedVoteLQTY - vars.prevInitiativeState.voteLQTY
);
assert(vars.state.countedVoteLQTY >= vars.prevInitiativeState.voteLQTY);
/// @audit INVARIANT: Never overflows
vars.state.countedVoteLQTY -= vars.prevInitiativeState.voteLQTY;

vars.state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp(
vars.state.countedVoteLQTYAverageTimestamp,
vars.initiativeState.averageStakingTimestampVoteLQTY,
vars.state.countedVoteLQTY,
vars.state.countedVoteLQTY + vars.initiativeState.voteLQTY
);

vars.state.countedVoteLQTY += vars.initiativeState.voteLQTY;
}

/// === UPDATE ACCOUNTING === ///
// == INITIATIVE STATE == //
// == USER ALLOCATION == //

// deep copy of the initiative's state before the allocation
InitiativeState memory prevInitiativeState = InitiativeState(
initiativeState.voteLQTY,
initiativeState.vetoLQTY,
initiativeState.averageStakingTimestampVoteLQTY,
initiativeState.averageStakingTimestampVetoLQTY,
initiativeState.lastEpochClaim
);
// allocate the voting and vetoing LQTY to the initiative
vars.allocation = lqtyAllocatedByUserToInitiative[msg.sender][initiative];
vars.allocation.voteLQTY = add(vars.allocation.voteLQTY, deltaLQTYVotes);
vars.allocation.vetoLQTY = add(vars.allocation.vetoLQTY, deltaLQTYVetos);
vars.allocation.atEpoch = currentEpoch;
require(!(vars.allocation.voteLQTY != 0 && vars.allocation.vetoLQTY != 0), "Governance: vote-and-veto");
lqtyAllocatedByUserToInitiative[msg.sender][initiative] = vars.allocation;

// update the average staking timestamp for the initiative based on the user's average staking timestamp
initiativeState.averageStakingTimestampVoteLQTY = _calculateAverageTimestamp(
initiativeState.averageStakingTimestampVoteLQTY,
vars.userState.averageStakingTimestamp,
/// @audit This is wrong unless we enforce a reset on deposit and withdrawal
initiativeState.voteLQTY,
add(initiativeState.voteLQTY, deltaLQTYVotes)
);
initiativeState.averageStakingTimestampVetoLQTY = _calculateAverageTimestamp(
initiativeState.averageStakingTimestampVetoLQTY,
vars.userState.averageStakingTimestamp,
/// @audit This is wrong unless we enforce a reset on deposit and withdrawal
initiativeState.vetoLQTY,
add(initiativeState.vetoLQTY, deltaLQTYVetos)
);
// == USER STATE == //

// allocate the voting and vetoing LQTY to the initiative
initiativeState.voteLQTY = add(initiativeState.voteLQTY, deltaLQTYVotes);
initiativeState.vetoLQTY = add(initiativeState.vetoLQTY, deltaLQTYVetos);

// update the initiative's state
initiativeStates[initiative] = initiativeState;

// == GLOBAL STATE == //

// TODO: Veto reducing total votes logic change
// TODO: Accounting invariants
// TODO: Let's say I want to cap the votes vs weights
// Then by definition, I add the effective LQTY
// And the effective TS
// I remove the previous one
// and add the next one
// Veto > Vote
// Reduce down by Vote (cap min)
// If Vote > Veto
// Increase by Veto - Veto (reduced max)

// update the average staking timestamp for all counted voting LQTY
/// Discount previous only if the initiative was not unregistered

/// @audit We update the state only for non-disabled initiaitives
/// Disabled initiaitves have had their totals subtracted already
/// Math is also non associative so we cannot easily compare values
if (status != InitiativeStatus.DISABLED) {
/// @audit Trophy: `test_property_sum_of_lqty_global_user_matches_0`
/// Removing votes from state desynchs the state until all users remove their votes from the initiative
/// The invariant that holds is: the one that removes the initiatives that have been unregistered
vars.state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp(
vars.state.countedVoteLQTYAverageTimestamp,
prevInitiativeState.averageStakingTimestampVoteLQTY,
/// @audit We don't have a test that fails when this line is changed
vars.state.countedVoteLQTY,
vars.state.countedVoteLQTY - prevInitiativeState.voteLQTY
);
assert(vars.state.countedVoteLQTY >= prevInitiativeState.voteLQTY);
/// @audit INVARIANT: Never overflows
vars.state.countedVoteLQTY -= prevInitiativeState.voteLQTY;

vars.state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp(
vars.state.countedVoteLQTYAverageTimestamp,
initiativeState.averageStakingTimestampVoteLQTY,
vars.state.countedVoteLQTY,
vars.state.countedVoteLQTY + initiativeState.voteLQTY
vars.userState.allocatedLQTY = add(vars.userState.allocatedLQTY, deltaLQTYVotes + deltaLQTYVetos);

// Replaces try / catch | Enforces sufficient gas is passed
bool success = safeCallWithMinGas(
initiative,
MIN_GAS_TO_HOOK,
0,
abi.encodeCall(
IInitiative.onAfterAllocateLQTY,
(currentEpoch, msg.sender, vars.userState, vars.allocation, vars.initiativeState)
)
);

vars.state.countedVoteLQTY += initiativeState.voteLQTY;
emit AllocateLQTY(msg.sender, initiative, deltaLQTYVotes, deltaLQTYVetos, currentEpoch, success);
}

// == USER ALLOCATION == //

// allocate the voting and vetoing LQTY to the initiative
Allocation memory allocation = lqtyAllocatedByUserToInitiative[msg.sender][initiative];
allocation.voteLQTY = add(allocation.voteLQTY, deltaLQTYVotes);
allocation.vetoLQTY = add(allocation.vetoLQTY, deltaLQTYVetos);
allocation.atEpoch = vars.currentEpoch;
require(!(allocation.voteLQTY != 0 && allocation.vetoLQTY != 0), "Governance: vote-and-veto");
lqtyAllocatedByUserToInitiative[msg.sender][initiative] = allocation;

// == USER STATE == //

vars.userState.allocatedLQTY = add(vars.userState.allocatedLQTY, deltaLQTYVotes + deltaLQTYVetos);

// Replaces try / catch | Enforces sufficient gas is passed
bool success = safeCallWithMinGas(
initiative,
MIN_GAS_TO_HOOK,
0,
abi.encodeCall(
IInitiative.onAfterAllocateLQTY,
(vars.currentEpoch, msg.sender, vars.userState, allocation, initiativeState)
)
require(
vars.userState.allocatedLQTY == 0
|| vars.userState.allocatedLQTY <= uint88(stakingV1.stakes(deriveUserProxyAddress(msg.sender))),
"Governance: insufficient-or-allocated-lqty"
);

emit AllocateLQTY(msg.sender, initiative, deltaLQTYVotes, deltaLQTYVetos, vars.currentEpoch, success);
globalState = vars.state;
userStates[msg.sender] = vars.userState;
}

/// @inheritdoc IGovernance
Expand Down

0 comments on commit 17e7be1

Please sign in to comment.