Remove votesbook #41
Remove votesbook #41
Conversation
cabrador
force-pushed
the
c/remove-common
branch
6 times, most recently
from
8fb57ad to
3e8cac4
Compare
cabrador
force-pushed
the
c/remove-votesbook
branch
3 times, most recently
from
9e2c8f5 to
37bc330
Compare
cabrador
force-pushed
the
c/remove-votesbook
branch
from
37bc330 to
01bc9ac
Compare
contracts/governance/Governance.sol
Outdated
|
|
||
| /// @notice Governance contract for voting on proposals | ||
| contract Governance is Initializable, ReentrancyGuardTransient, GovernanceSettings, Version { | ||
| contract Governance is ReentrancyGuardTransient, GovernanceSettings, Version, UUPSUpgradeable, OwnableUpgradeable { |
There was a problem hiding this comment.
When using UUPSUpgradeable, you should always include constructor and disable initializers to prevent initialization of the implementation contract.
/** @custom:oz-upgrades-unsafe-allow constructor */
constructor() {
_disableInitializers();
}
There was a problem hiding this comment.
arent we supposed to call this in initialize instead? I cannot call initialize when this is in construct and I cannot call the init functions from ReentrancyGuardTransient, Ownable and UUPSUpgradeable inside constructor
There was a problem hiding this comment.
resolved in private conversation.
cabrador
force-pushed
the
c/remove-votesbook
branch
from
1c14972 to
ce53d55
Compare
| VotesBookKeeper votebook; | ||
| // list of all proposal to which voter has voted for | ||
| // voter => delegatedTo => []proposal IDs | ||
| mapping(address => mapping(address => uint256[])) public votesList; |
There was a problem hiding this comment.
please use names of keys/values in the mappings:
mapping(address voter => mapping(address delegatedTo => uint256[] proposalId)) public votesList;
list of all proposal to which voter has voted for -> list of proposals the voter has voted on
| /// @notice Get the proposal IDs for a voter | ||
| /// @param voter The address of the voter | ||
| /// @param delegatedTo The address of the delegator which the voter has delegated their stake to | ||
| /// @return An array of proposal IDs |
There was a problem hiding this comment.
The comment is not much helpful:
Get the proposal IDs for a voter -> Get proposals for with the voter voted ?
| // voter => delegatedTo => []proposal IDs | ||
| mapping(address => mapping(address => uint256[])) public votesList; | ||
| // voter => delegatedTo => proposal ID => {index in the list + 1} | ||
| mapping(address => mapping(address => mapping(uint256 => uint256))) public votesIndex; |
There was a problem hiding this comment.
- we can add a comment like
Index of vote positions in proposals index in the list->index in the votesList
| /// @dev Remove a vote from the list of votes | ||
| /// @param voter The address of the voter | ||
| /// @param delegatedTo The address of the delegator which the sender has delegated their stake to. | ||
| /// @param proposalID The ID of the proposal |
There was a problem hiding this comment.
Missing doc for @param idx
Edit: suggested removing it bellow.
| /// @param voter The address of the voter | ||
| /// @param delegatedTo The address of the delegator which the sender has delegated their stake to. | ||
| /// @param proposalID The ID of the proposal | ||
| /// @return The index of the vote plus 1 if the vote exists, otherwise 0 |
There was a problem hiding this comment.
The index of the vote -> The index of the proposal in the list of voter's votes
maybe The index of the proposal in the votesList
| return votesList[voter][delegatedTo]; | ||
| } | ||
|
|
||
| /// @notice Get option for which the voter voted (indexed from 1), zero if not voted. |
There was a problem hiding this comment.
This seems to be wrong comment.
Get option for which the voter voted -> Get position of a proposal in the list of voter's votes
| if (!isCanceled) { | ||
| i++; | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks like a very courageous refactoring - I will have to review it more deeper later...
| votebook.onVoteCanceled(voter, delegatedTo, proposalID); | ||
| uint256 idx = votesIndex[voter][delegatedTo][proposalID]; | ||
| if (idx != 0) { | ||
| eraseVote(voter, delegatedTo, proposalID, idx); |
There was a problem hiding this comment.
It seems you can move the idx reading a the condition inside of the eraseVote() - then you can drop the idx param too.
| // expect(await this.votesBookKeeper.getVoteIndex(this.otherAcc, this.defaultAcc, 1)).to.be.equal(0n); | ||
| // } | ||
| // }); | ||
| // }) |
There was a problem hiding this comment.
These tests need to be reworked for the gov contract before this PR can be merged.
cabrador
force-pushed
the
c/remove-votesbook
branch
from
ce53d55 to
6a06744
Compare
#33 and #34 must be merged before this PR
This PR removes the VotesBook contract and merges its funcionality into Governance contact.