Skip to content

WIP: Permission Claims - Voting Mechanics [skip-ci] #1390

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

Draft
wants to merge 14 commits into
base: release-candidate
Choose a base branch
from

Conversation

rackstar
Copy link
Contributor

@rackstar rackstar commented May 19, 2025

Description

Closes: #1387

Describe the changes made in this PR. A link to the issue is enough if it contains all the relevant information.

Testing

Explain how you tested your changes to ensure they work as expected.

Checklist

  • Performed a self-review of my own code
  • Made corresponding changes to the documentation

@rackstar rackstar self-assigned this May 19, 2025
@rackstar rackstar changed the title WIP: Permission Claims - Voting Mechanics WIP: Permission Claims - Voting Mechanics [skip-ci] May 19, 2025
@rackstar rackstar force-pushed the feat/permissioned-assessment-voting-mechanics branch from 092a03f to 5f132e8 Compare May 19, 2025 08:43
@rackstar rackstar marked this pull request as draft May 19, 2025 08:43
@rackstar rackstar force-pushed the feat/permissioned-assessment-voting-mechanics branch 7 times, most recently from 23adb13 to 0351136 Compare May 19, 2025 10:18
@rackstar rackstar force-pushed the feat/permissioned-assessment-voting-mechanics branch from 0351136 to 9fbba73 Compare May 19, 2025 13:27
Comment on lines 167 to 168
// Get the tally again after the vote
(acceptCount, denyCount) = _getVoteTally(claimId, assessorGroup);
Copy link
Contributor Author

@rackstar rackstar May 19, 2025

Choose a reason for hiding this comment

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

we can avoid calling _getVoteTally a second time by manually adjusting the vote counts:

  • if its an update
    1. remove the previous vote from the tally
    2. increment the tally with the current vote
  • if its a new vote
    1. increment the tally with the current vote

but when I implemented it, it looked less readable. so opted calling _getVotedTally a second time for better readability (but small additional cost in gas)

Copy link
Contributor

Choose a reason for hiding this comment

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

But _getVotedTally seems heavy in gas as it goes through the whole enumerable set, then check maps for votes. I thing we should try to not call it at all when casting regular votes (left comment below about that, one option is to split castVote in few functions as we need total vote count only in special cases).

@rackstar rackstar force-pushed the feat/permissioned-assessment-voting-mechanics branch 2 times, most recently from 5b084ac to 787a114 Compare May 19, 2025 14:16
@rackstar rackstar force-pushed the feat/permissioned-assessment-voting-mechanics branch from 787a114 to 4d86c71 Compare May 19, 2025 14:51
@rackstar rackstar force-pushed the feat/permissioned-assessment-voting-mechanics branch from f0d4421 to 1c692e9 Compare May 19, 2025 15:46

/* ========== STATE VARIABLES ========== */

mapping(uint32 assessorGroupId => EnumerableSet.UintSet assessorGroup) private _assessorGroups;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep the shorter name, just to be groups as before, and there can be a short comment to explain? This seems like too much of word assessor everywhere so it's less readable

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think we can omit naming value in the map if it is just a singular of the variable name

} else {
// Otherwise, check if we need to extend the voting period
uint32 nextDay = uint32(block.timestamp + SILENT_ENDING_PERIOD);
// If the poll ends in less than 24h from the latest vote, extend it to 24h
Copy link
Contributor

Choose a reason for hiding this comment

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

there shouldn't be any voting extensions.

else if (vote == Vote.DENY) denyCount++;

// Unchecked increment to save gas - cannot overflow as assessor group size is a relatively small number
unchecked { ++i; }
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like a micro optimization, not needed here.

/// @param denyCount Number of deny votes
/// @param assessmentEnd Timestamp when the assessment voting period ends
/// @return true if the assessment is decided, false otherwise
function _isAssessmentDecided(uint256 acceptCount, uint256 denyCount, uint32 assessmentEnd) internal view returns (bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like this can be a part of return of _getVoteTally function, not needed as a separate one?

denyCount = 0;

for (uint i = 0; i < assessorGroupLength;) {
uint256 assessorMemberId = assessorGroup.at(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it may be faster to get first the whole array to memory by using values(), and then iterate through the memory array. But we can test this later also (as I'm writing this, I'm also thinking on optimizing the whole EnumberableSet because now our memberIds will be all uint32 and we don't need to use the whole uint256 slot for each of it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I did consider the memory approach as well but since we are accessing each item only once it should be the same gas cost. so went with storage method as it seemed more readable.

and yes, might be worth doing our own custom EnumberableSet that supports uint32. we should go with the memory approach if that is the case

/// @param productTypeId Type of product the claim is for
/// @dev Only callable by internal contracts
/// @dev Reverts if an assessment already exists for the given claimId
function startAssessment(bytes32 claimId, uint16 productTypeId) external onlyInternal {
Copy link
Contributor

Choose a reason for hiding this comment

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

This interface changes how it works from Claims, as we had before assessmentId which was returned and saved in Claims. Now it's more coupled, maybe change is for better, but requires a lot of changes in Claims also then.

struct Ballot {
bytes32 ipfsHash;
Vote vote;
uint32 timestamp;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this timestamp onchain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we could probably get the timestamp from the event but since it also packs into slot 1, I've added it in for convenience.

emit VoteCast(claimId, msg.sender, assessorMemberId, vote, ipfsHash);
}

function closeAssessment(uint256 claimId) external {
Copy link
Contributor

Choose a reason for hiding this comment

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

should it revert in all cases when not closeable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Claim Assessment - voting mechanics
2 participants