feat: Add minimal approval configuration #28

clauBv23 · 2024-08-26T12:37:35Z

Task ID: OD-1451

TBD:

The new updateMinApproval permission id should be UPDATE_VOTING_SETTINGS_PERMISSION_ID or a new one should be created?
Old initialize function on Token voting should revert or continue allowing initialize the plugin without minApproval value?

… to the majority vote base init function

…tinue supporting old version

…rly set when updating or installing

… min approval is not reached

…oval

…tialize function reverts

packages/contracts/src/MajorityVotingBase.sol

nivida · 2024-08-28T15:04:53Z

packages/contracts/src/MajorityVotingBase.sol

@@ -266,17 +273,25 @@ abstract contract MajorityVotingBase is
        uint256 minProposerVotingPower
    );

+    /// @notice Emitted when the min approval value is updated.
+    /// @param minApproval The minimum amount of yes votes needed for a proposal succeed.
+    event VotingMinApprovalUpdated(uint32 minApproval);


Is it possible to have this event defined in the interface?

I'm not entirely certain about the approach we're taking with this, but I noticed that VotingMinApprovalUpdated](

token-voting-plugin/packages/contracts/src/MajorityVotingBase.sol

Lines 262 to 274 in 65198a0

/// @notice Emitted when the voting settings are updated.

/// @param votingMode A parameter to select the vote mode.

/// @param supportThreshold The support threshold value.

/// @param minParticipation The minimum participation value.

/// @param minDuration The minimum duration of the proposal vote in seconds.

/// @param minProposerVotingPower The minimum voting power required to create a proposal.

event VotingSettingsUpdated(

VotingMode votingMode,

uint32 supportThreshold,

uint32 minParticipation,

uint64 minDuration,

uint256 minProposerVotingPower

);

) was defined in the contract rather than the interface.

Given that events are not part of the interface ID, I believe it’s reasonable to define them in the interface. However, for the sake of consistency, I lean to keep it here unless you have a strong reason to move it to the interface.

nivida · 2024-08-28T15:05:44Z

packages/contracts/src/MajorityVotingBase.sol

+    // todo TBD define if permission should be the same one as update settings
+    /// @notice Updates the minimal approval value.
+    /// @param _minApproval The new minimal approval value.
+    function updateMinApproval(


Are we always doing breaks for function arguments or not? I see a small inconsistency here :-)

It will depend on the line length (100 - 120).
If there are too many arguments will break into lines, if not it should be on the same one.
In this case, there is a single argument but the modifier name on the auth function increases the line length and breaks in lines

packages/contracts/src/MajorityVotingBase.sol

novaknole · 2024-08-29T07:52:27Z

packages/contracts/src/MajorityVotingBase.sol

+            revert RatioOutOfBounds({limit: RATIO_BASE, actual: _minApproval});
+        }
+
+        minApprovals = _minApproval;


small incosistency. name them the same.

done f7e0f97

you still use _minApproval instead of _minApprovals. hahaha

and rename the function as well to _updateMinApprovals.

hopefully all are renamed now 92070c4 9b6cf85

packages/contracts/src/build-metadata.json

novaknole · 2024-08-29T08:05:21Z

packages/contracts/src/MajorityVotingBase.sol

@@ -224,6 +227,10 @@ abstract contract MajorityVotingBase is
    /// @notice The struct storing the voting settings.
    VotingSettings private votingSettings;

+    /// @notice The minimal ratio of yes votes needed for a proposal succeed.
+    /// @dev is not on the VotingSettings for compatibility reasons.
+    uint32 private minApprovals; // added in v1.3


What's the reason for going with uint32 instead of uint256 ? I wonder the reason in your mind was that in the future, if we add more state variable, they would be tightly packed ? If that's the reason, I think it's a wrong reason.

packing only makes good and efficient sense if state variables are stored in the same tx. I highly doubt that whatever new state variable we add, that value and minApprovals value would be updated in the same tx. From the solidity docs:

If you are not reading or writing all the values in a slot at the same time, this can have the opposite effect, though: When one value is written to a multi-value storage slot, the storage slot has to be read first and then combined with the new value such that other data in the same slot is not destroyed.

Up to you as I haven't thought this very deep, but from first impression, that's the impression I got.

Also, can you remind me of why we can not put this state variable inside VotingSettings struct ?

I went with uint32 mainly because minParticipation and supportThreshold the other ratios we already store are defined as uint32.
At some point questioned it because an extra operation is needed for casting the value every single time, however just continued with uint32 for consistency among the ratios.

Didn't store it inside VotingSettings because it is being received as a parameter for several functions:

__MajorityVotingBase_init: is not important because is not on any interface id.

initialize: that is in TOKEN_VOTING_INTERFACE_ID but we are already overriding it so we can manage it.

updateVotingSettings: that is in IMajorityVoting interface, adding a new entry to the struct will change the function selector, and even if we implement erc165 with both IDs, we still need to "support" the old function which has a no longer existent struct.

A new struct could be created to add this parameter and deprecate the old one, but to be honest, I don't have a strong opinion on this, neither option is clean.

I went with uint32 mainly because minParticipation and supportThreshold the other ratios we already store are defined as uint32.

This was because of packing, but for new minApproval, no packing needed. I'd go with uint256.

As for other question, I missed this detail and yes, I think we don't have any better way. Have to think this through

I also agree to go with uint256 done in 4ce4388

novaknole

1st ROUND.

clauBv23 · 2024-08-30T14:56:51Z

packages/contracts/src/MajorityVotingBase.sol

@@ -211,6 +213,7 @@ abstract contract MajorityVotingBase is
            this.totalVotingPower.selector ^
            this.getProposal.selector ^
            this.updateVotingSettings.selector ^
+            this.updateMinApprovals.selector ^
            this.createProposal.selector;

    /// @notice The ID of the permission required to call the `updateVotingSettings` function.


Suggested change

/// @notice The ID of the permission required to call the `updateVotingSettings` function.

/// @notice The ID of the permission required to call the `updateVotingSettings` and `updateMinApprovals` functions.

@clauBv23 - sorry for the long delay in getting to this review. Functionally this looks like it works as specified in the requirements.

The only thing that really makes me uncomfortable is not including minApprovals in the voting setting struct. It is a governance setting just like the others, so having it separate, and with its own permission doesn't make any practical sense.

Will this always be a problem when we want to extend the functionality of existing plugins? Are there any other product inputs that can help inform how to improve this decision?

novaknole · 2024-10-18T06:48:54Z

Closing this as the same changes have already been applied by another PR.

clauBv23 added 30 commits August 26, 2024 13:43

feat: update majority voting to support min approvals

c4605ea

ci: separate the updateMinApproval in a different function and add it…

5d955d8

… to the majority vote base init function

feat: add minApproval to token voting, duplicate init function to con…

6a2456c

…tinue supporting old version

fix: tests and compilation errors

bdba026

feat: update the token voting setup so the min advance value in prope…

88f9d04

…rly set when updating or installing

fix: typo in the setup function selector

635b96c

feat: add new installation and update param in the build metadata file

42ef256

fix: update tests to work with new setup and configuration param

77f3d6f

feat: add natspec comments to the majority votes interface

0403d51

feat: add natspect comment to the token voting contract

902eb13

ci: remove not needed contract

7ec12c9

feat: add natspect and needed event when updating the min approval value

59165e4

feat: add test for checking the proposal can not be executed when the…

5d1c070

… min approval is not reached

feat: define new interfaces to check them on the tests

b6e9ca7

ci: move the min approval variable definition position

9b7394c

fix: min approval values

04ca7fb

feat: add test for edge cases

8955f48

feat: modify the majority votes init function to receive the min appr…

638ea07

…oval

feat: add tets for checking the updateMinApproval function

7ec2e54

ci: remove comment

e39d535

feat: add test for old interfaces on majority votes

cb01be0

fix: import missing constant

f175112

feat: add validation for minimal approval value

a959426

fixL typos

c0bd627

feat: change test to the new initialize function

08409b5

feat: define the initialize function signature as constant to reuse it

9bc036a

feat: revert on old initialize function

bb689ff

feat: use new constant signatures and add test to check deprecated in…

119e15c

…tialize function reverts

ci: improve natspec comment

7bf608b

ci: remove comments

5c04b20

clauBv23 added 2 commits August 28, 2024 15:51

ci: remove comment

6f7d789

ci: comment

65198a0

clauBv23 requested review from nivida and novaknole August 28, 2024 14:07

nivida reviewed Aug 28, 2024

View reviewed changes

clauBv23 added 5 commits August 28, 2024 17:16

fix: typo

8a69416

feat: rename min approval variable

af67a74

fix: remove check no longer needed

939dac7

ci: refact

1219954

ci: remove comment

210afb0

novaknole reviewed Aug 29, 2024

View reviewed changes

packages/contracts/src/MajorityVotingBase.sol Show resolved Hide resolved

novaknole reviewed Aug 29, 2024

View reviewed changes

packages/contracts/src/build-metadata.json Outdated Show resolved Hide resolved

novaknole reviewed Aug 29, 2024

View reviewed changes

clauBv23 added 9 commits August 29, 2024 11:30

ci: add comment

fb95ba5

fic: rename function for consistency

f7e0f97

fix: update metadata description

345d6b7

fix lint

4fe246c

ci: rename all vars name

9b6cf85

ci: rename functions for consistency

92070c4

feat: change the min approval type from uint32 to uint256

4ce4388

fix: test

c720fed

fix test

1073c87

clauBv23 commented Aug 30, 2024

View reviewed changes

novaknole closed this Oct 18, 2024

novaknole deleted the feat/add-minimal-approval-configuration branch October 18, 2024 06:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add minimal approval configuration #28

feat: Add minimal approval configuration #28

clauBv23 commented Aug 26, 2024 •

edited

Loading

nivida Aug 28, 2024

clauBv23 Aug 28, 2024 •

edited

Loading

nivida Aug 28, 2024

clauBv23 Aug 28, 2024 •

edited

Loading

novaknole Aug 29, 2024

clauBv23 Aug 29, 2024

novaknole Aug 29, 2024

novaknole Aug 29, 2024

clauBv23 Aug 29, 2024

novaknole Aug 29, 2024

novaknole Aug 29, 2024

clauBv23 Aug 29, 2024

novaknole Aug 29, 2024

clauBv23 Aug 29, 2024

novaknole left a comment

clauBv23 Aug 30, 2024

evanaronson Sep 20, 2024

novaknole commented Oct 18, 2024

	/// @notice Emitted when the voting settings are updated.
	/// @param votingMode A parameter to select the vote mode.
	/// @param supportThreshold The support threshold value.
	/// @param minParticipation The minimum participation value.
	/// @param minDuration The minimum duration of the proposal vote in seconds.
	/// @param minProposerVotingPower The minimum voting power required to create a proposal.
	event VotingSettingsUpdated(
	VotingMode votingMode,
	uint32 supportThreshold,
	uint32 minParticipation,
	uint64 minDuration,
	uint256 minProposerVotingPower
	);

	/// @notice The ID of the permission required to call the `updateVotingSettings` function.
	/// @notice The ID of the permission required to call the `updateVotingSettings` and `updateMinApprovals` functions.

feat: Add minimal approval configuration #28

feat: Add minimal approval configuration #28

Conversation

clauBv23 commented Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

clauBv23 Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clauBv23 Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novaknole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novaknole commented Oct 18, 2024

clauBv23 commented Aug 26, 2024 •

edited

Loading

clauBv23 Aug 28, 2024 •

edited

Loading

clauBv23 Aug 28, 2024 •

edited

Loading