Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix: Proposal status and filters for multisig #250

Closed
wants to merge 6 commits into from
Closed

Fix: Proposal status and filters for multisig #250

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
db408b3
add fixes for statuses
josemarinas Jul 10, 2023
dbc9905
fix tests and update changelog
josemarinas Jul 12, 2023
340534c
fix tests
josemarinas Jul 12, 2023
82705e0
fix filters
josemarinas Jul 17, 2023
049a362
fix tests
josemarinas Jul 19, 2023
f02486b
update status fucntion
josemarinas Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions modules/client/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ TEMPLATE:
-->

## [UPCOMING]
### Fixed
- Bug while filtering multisig propsals by status
- Bug computing proposal status
### Changed
- Updates `@aragon/osx-ethers` to v1.3.0-rc0.

Expand Down
13 changes: 8 additions & 5 deletions modules/client/src/client-common/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ export function computeProposalStatus(
if (startDate >= now) {
return ProposalStatus.PENDING;
}
Copy link
Contributor

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 ?

Copy link
Contributor Author

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

Copy link
Contributor

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?

if (proposal.potentiallyExecutable || proposal.earlyExecutable) {
if (
(now > endDate || proposal.earlyExecutable) &&
proposal.potentiallyExecutable
) {
return ProposalStatus.SUCCEEDED;
}
if (endDate >= now) {
Expand All @@ -64,22 +67,22 @@ 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 };
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, good catch

break;
case ProposalStatus.ACTIVE:
where = { startDate_lt: now, endDate_gte: now, executed: false };
where = { startDate_lt: now, endDate_gte: now, executed: false, potentiallyExecutable: false };
break;
case ProposalStatus.EXECUTED:
where = { executed: true };
break;
case ProposalStatus.SUCCEEDED:
where = { potentiallyExecutable: true, endDate_lt: now };
where = { potentiallyExecutable: true };
Copy link
Contributor

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 ?

Copy link
Contributor Author

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 :)

break;
case ProposalStatus.DEFEATED:
Copy link
Contributor

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.

Copy link
Contributor Author

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

where = {
potentiallyExecutable: false,
brickpop marked this conversation as resolved.
Show resolved Hide resolved
endDate_lt: now,
executed: false,
potentiallyExecutable: false,
};
break;
default:
Expand Down
1 change: 0 additions & 1 deletion modules/client/test/integration/addresslistVoting-client/methods.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,7 +869,6 @@ describe("Client Address List", () => {
QueryAddresslistVotingProposals,
{
where: {
potentiallyExecutable: false,
endDate_lt: nowFilter,
executed: false,
},
Expand Down
1 change: 0 additions & 1 deletion modules/client/test/integration/multisig-client/methods.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,6 @@ describe("Client Multisig", () => {
QueryMultisigProposals,
{
where: {
potentiallyExecutable: false,
endDate_lt: nowFilter,
executed: false,
},
Expand Down
1 change: 0 additions & 1 deletion modules/client/test/integration/tokenVoting-client/methods.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1165,7 +1165,6 @@ describe("Token Voting Client", () => {
QueryTokenVotingProposals,
{
where: {
potentiallyExecutable: false,
endDate_lt: nowFilter,
executed: false,
},
Expand Down
Loading