Voting Contract: vote and close uses current status #60

ueco-jb · 2022-02-01T11:26:04Z

closes #55

Closing and voting methods now use current_status() helper, which underneath can update status to Rejected.
Because of that, first time closing a proposal that should be rejected could fail, because there was check in place to see, if status... is not already rejected.
To mitigate that, I used simple solution with saving status before closing, in order to detect if someone tries to close proposal first of 1+nth time.

~~I added extra .save() under each update status, since in case of early exit status update would be lost.~~ So that doesn't work, and I don't like it. :)
So someone tries to vote on proposal, update_status() changes state to Rejected and vote is rejected, but then validator queries the proposal and sees that status was the same as before - probably just Open.

I didn't change anything in state.rs file, simply moved impl Proposal so that it would be directly under its structure, instead on bottom of file for some reason.

…ing and closing current status is used and saved

…rror" This reverts commit ba9771a.

packages/voting-contract/src/multitest/early_end.rs

packages/voting-contract/src/multitest/closing.rs

packages/voting-contract/src/lib.rs

uint · 2022-02-01T17:40:29Z

Overall: really awesome you caught these problems, but I have some comments!

I added extra .save() under each update status, since in case of early exit status update would be lost. So someone tries to vote on proposal, update_status() changes state to Rejected and vote is rejected, but then validator queries the proposal and sees that status was the same as before.

Got a test for this?

I didn't change anything in state.rs file, simply moved impl Proposal so that it would be directly under its structure, instead on bottom of file for some reason.

👌

uint

Unmarking this PR as request changes so as not to block it. Oops.

ueco-jb · 2022-02-02T08:17:49Z

Got a test for this?

I mistakenly took this "closing twice test" as proof of that, but now I see that it was due to this check-for-rejected-status mechanism I added.

This save doesn't write to the storage directly, but to a cache. All such changes are committed to persistent storage later if the fn succeeds and discarded if the fn returns an error.

This still bothers me though. Because as I wrote in description:

So someone tries to vote on proposal, update_status() changes state to Rejected and vote is rejected, but then validator queries the proposal and sees that status was the same as before - probably just Open.

I think this will be confusing and we should mitigate that, but now I don't have any idea how.
Wrong idea here - unless we start returning some empty response with error message in couple cases...?

Reviewer tried to dismiss it himself.

ueco-jb · 2022-02-02T08:40:38Z

Anyway, PR solves problem from issue and works.

I referenced my problems in new issue, so this one could actually be merged.

uint · 2022-02-02T09:06:03Z

So someone tries to vote on proposal, update_status() changes state to Rejected and vote is rejected, but then validator queries the proposal and sees that status was the same as before - probably just Open.

I think this will be confusing and we should mitigate that, but now I don't have any idea how. Wrong idea here - unless we start returning some empty response with error message in couple cases...?

Does this actually happen though? The status query doesn't check the storage directly, but uses a helper that's supposed to return the current status taking expiration into account:
https://github.com/confio/poe-contracts/blob/main/packages/voting-contract/src/lib.rs#L231

I think the idea with this stuff is that queries use current_status() and executions use update_status() and then everything checks out.

maurolacy

Looks good. Just some questions / comments, and what I think is an error when closing.

packages/voting-contract/src/lib.rs

maurolacy · 2022-02-02T08:54:25Z

packages/voting-contract/src/lib.rs

+        return Err(ContractError::Rejected {});
+    }
+
+    prop.update_status(&env.block);


This is the key thing to do. Wouldn't this have to be done in the other cases where current_status() is being called as well? (i.e. our previous "execute proposal" fix).
That means, using current_status() only for queries, and update_status() for execution everywhere. And, "never" reading prop.status directly. More consistent and robust, I think.

We can use an update_status() impl that returns the updated status, as mentioned above (just an idea).

I think the idea with this stuff is that queries use current_status() and executions use update_status() and then everything checks out.

Yeah, you're right. I just realized, that despite status in memory not being correct, in the end it is correct for end user who uses query. We "simply" must be carefull to make sure we use that helper everywhere.

Just wrote a variant of the same above.

If the code errors below, this status update will be discarded. And if it does not error, the status will be updated and saved later anyway. So, there's really no need to call update_status instead of current_status.

The bug is: not calling any of those, and reading the status directly.

packages/voting-contract/src/lib.rs

packages/voting-contract/src/state.rs

ueco-jb · 2022-02-02T09:15:27Z

I think the idea with this stuff is that queries use current_status() and executions use update_status() and then everything checks out.

Yeah, you're right. I just realized, that despite status in memory not being correct, in the end it is correct for end user who uses query.
We "simply" must be carefull to make sure we use that helper everywhere.

maurolacy · 2022-02-02T09:21:39Z

We "simply" must be carefull to make sure we use that helper everywhere.

I was just thinking that a (relatively cumbersome) way to do this would be to have / use two structs, one for state, with private members, and one for serialisation / deserialization, with pub ones.

uint · 2022-02-02T13:08:09Z

We "simply" must be carefull to make sure we use that helper everywhere.

I was just thinking that a (relatively cumbersome) way to do this would be to have / use two structs, one for state, with private members, and one for serialisation / deserialization, with pub ones.

Yeah, that's one way to do it. I opened an issue for this: #63

uint

LGTM

packages/voting-contract/src/lib.rs

…osals" This reverts commit e326913.

ueco-jb self-assigned this Feb 1, 2022

ueco-jb force-pushed the 55-voting-contract-status-reading-bugs branch 3 times, most recently from 3c8eeda to 12c3af7 Compare February 1, 2022 11:34

Voting Contract: Slightly refactor implementation, so that during vot…

0cdf7df

…ing and closing current status is used and saved

ueco-jb force-pushed the 55-voting-contract-status-reading-bugs branch from 12c3af7 to 0cdf7df Compare February 1, 2022 11:37

--local added 2 commits February 1, 2022 12:44

Voting Contract: Save changed status only when returning an error

ba9771a

Revert "Voting Contract: Save changed status only when returning an e…

4a3995f

…rror" This reverts commit ba9771a.

ueco-jb requested review from uint and maurolacy February 1, 2022 11:51

ueco-jb marked this pull request as ready for review February 1, 2022 11:51

uint previously requested changes Feb 1, 2022

View reviewed changes

uint reviewed Feb 1, 2022

View reviewed changes

uint self-requested a review February 1, 2022 19:23

Bring back quorum removed by mistake

fd2e5e6

ueco-jb mentioned this pull request Feb 2, 2022

Voting's helper update_status() should save changed status #62

Closed

maurolacy reviewed Feb 2, 2022

View reviewed changes

Voting Contract: Simplify close rejected logic

1adbde0

ueco-jb force-pushed the 55-voting-contract-status-reading-bugs branch from 7a9de1f to 1adbde0 Compare February 2, 2022 09:20

Voting contract: Don't allow closing not-expired pending proposals

e326913

ueco-jb requested review from hashedone and maurolacy February 2, 2022 12:05

uint approved these changes Feb 2, 2022

View reviewed changes

uint mentioned this pull request Feb 2, 2022

Fix status/execution bugs in flex-multisig CosmWasm/cw-plus#643

Merged

maurolacy reviewed Feb 2, 2022

View reviewed changes

packages/voting-contract/src/lib.rs Outdated Show resolved Hide resolved

Revert "Voting contract: Don't allow closing not-expired pending prop…

94daa76

…osals" This reverts commit e326913.

ueco-jb force-pushed the 55-voting-contract-status-reading-bugs branch from bbbd8ce to 94daa76 Compare February 3, 2022 08:24

ueco-jb merged commit 56844bb into main Feb 3, 2022

ueco-jb deleted the 55-voting-contract-status-reading-bugs branch February 3, 2022 08:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Voting Contract: vote and close uses current status #60

Voting Contract: vote and close uses current status #60

ueco-jb commented Feb 1, 2022 •

edited

Loading

uint commented Feb 1, 2022

uint left a comment

ueco-jb commented Feb 2, 2022 •

edited

Loading

ueco-jb commented Feb 2, 2022

uint commented Feb 2, 2022

maurolacy left a comment

maurolacy Feb 2, 2022 •

edited

Loading

maurolacy Feb 2, 2022

maurolacy Feb 2, 2022 •

edited

Loading

ueco-jb commented Feb 2, 2022

maurolacy commented Feb 2, 2022 •

edited

Loading

uint commented Feb 2, 2022

uint left a comment

Voting Contract: vote and close uses current status #60

Voting Contract: vote and close uses current status #60

Conversation

ueco-jb commented Feb 1, 2022 • edited Loading

uint commented Feb 1, 2022

uint left a comment

Choose a reason for hiding this comment

ueco-jb commented Feb 2, 2022 • edited Loading

ueco-jb commented Feb 2, 2022

uint commented Feb 2, 2022

maurolacy left a comment

Choose a reason for hiding this comment

maurolacy Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

maurolacy Feb 2, 2022

Choose a reason for hiding this comment

maurolacy Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

ueco-jb commented Feb 2, 2022

maurolacy commented Feb 2, 2022 • edited Loading

uint commented Feb 2, 2022

uint left a comment

Choose a reason for hiding this comment

ueco-jb commented Feb 1, 2022 •

edited

Loading

ueco-jb commented Feb 2, 2022 •

edited

Loading

maurolacy Feb 2, 2022 •

edited

Loading

maurolacy Feb 2, 2022 •

edited

Loading

maurolacy commented Feb 2, 2022 •

edited

Loading