-
Notifications
You must be signed in to change notification settings - Fork 14
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
Fix: Proposal status and filters for multisig #250
Conversation
470f658
to
dbc9905
Compare
if ( | ||
(proposal.potentiallyExecutable || proposal.earlyExecutable) && | ||
endDate >= now | ||
) { | ||
return ProposalStatus.SUCCEEDED; | ||
} | ||
if (endDate >= now) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But now, this means that a passed proposal that was not executed will be considered as DEFEATED, which is not correct. Why would an executable proposal be displayed as defeated?
If the plugin won't allow it to be executed after expired, then we would need another state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think so, if is potentially executable and the date expired it will appear as succeeded. Maybe I should give it a go with the tests ant test more edge cases but i think tthe change is ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the execution of an executable proposal that already ended would not enter any of the "if" clauses. It would keep trying until the end of the function, when it would simply return DEFEATED
break; | ||
case ProposalStatus.EXECUTED: | ||
where = { executed: true }; | ||
break; | ||
case ProposalStatus.SUCCEEDED: | ||
where = { potentiallyExecutable: true, endDate_lt: now }; | ||
where = { potentiallyExecutable: true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the proposal is potentially executable but the date expired? Isn't this condition tied to any other variable? executed
, executable
, endDate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the end date only shows you when you can and cant vote, there is not such thing as expired. If now is greater then endDate and potentiallyExecutable is true, congrats, your proposal passed :)
@@ -48,7 +48,10 @@ export function computeProposalStatus( | |||
if (startDate >= now) { | |||
return ProposalStatus.PENDING; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be any check for the special case of minApprovals = 1 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, i don't think so, the thing was that minApprovals = 1 triggered the potentiallyExecutable = true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But without a check doesn't this mean that we will get succeeded
by default for those proposals where minApprovals
= 1 without anyone actually having voted? Perhaps we could add a check to see if there exists at least 1 vote when potentiallyExecutable
= true?
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Jose. One thing I fear is that it might be quite difficult to set these statuses and generate the filters without knowing the type of proposal since the current two types are similar yet also behave slightly differently
@@ -48,7 +48,10 @@ export function computeProposalStatus( | |||
if (startDate >= now) { | |||
return ProposalStatus.PENDING; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But without a check doesn't this mean that we will get succeeded
by default for those proposals where minApprovals
= 1 without anyone actually having voted? Perhaps we could add a check to see if there exists at least 1 vote when potentiallyExecutable
= true?
@@ -77,7 +80,6 @@ export function computeProposalStatusFilter( | |||
break; | |||
case ProposalStatus.DEFEATED: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't quite work for the TokenVoting proposals. While the Multisig proposal is expired regardless of whether the approval threshold is met when the endDate
has passed, this is not the case with the TokenVoting Proposal; we need to know whether it is potentiallyExecutable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep then we are not going to be able to fix it.
Either we wait for the split where each plugin has its own compute status function or we create a compute status filter function for the multisig
@@ -64,7 +67,7 @@ export function computeProposalStatusFilter( | |||
const now = Math.round(new Date().getTime() / 1000).toString(); | |||
switch (status) { | |||
case ProposalStatus.PENDING: | |||
where = { startDate_gte: now }; | |||
where = { startDate_gte: now, potentiallyExecutable: false, executed: false }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this means we won't get any pending Multisig proposals with minApproval
= 1, since by default potentiallyExecutable
is set to true for those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, good catch
Closing this one and starting a new one to split the computeProposalStatus and computeProposalStatusFilter for each plugin |
Description
The subgraph sets potentiallyExecutable = true for any multisig proposal where minApproval is 1. So the filters turn up wrong on such cases on this line https://github.com/aragon/sdk/blob/develop/modules/client/src/client-common/utils.ts#L80 (for multisig with minApproval = 1, the defeated condition will be wrong) Also for "Succeeded" in multisig, if the proposal goes past the proposal end date, it should be considered as "Defeated".
An example DAO to check this https://staging-app.aragon.org/#/daos/polygon/0x517b7b401bb0b2c9af9663d0bf7b3688b8cb4fa2/governance
Task ID: OS-561
Type of change
Checklist:
CHANGELOG.md
file in the root folder of the package after the[UPCOMING]
title and before the latest version.