Backport multisig fixes to voting contracts #178
Conversation
0xFable
requested review from
ueco-jb and
maurolacy
and removed request for
ueco-jb
ueco-jb
changed the title
packages/voting-contract/src/lib.rs
Outdated
| // Update Status | ||
| proposal.update_status(&env.block); | ||
| // We allow execution even after the proposal "expiration" as long as all votes come in before | ||
| // that point. If it was approved on time, it can be executed any time. | ||
| if proposal.current_status(&env.block) != Status::Passed { | ||
| if proposal.status != Status::Passed { |
There was a problem hiding this comment.
This is redundant, since previous call current_status is called by update_status anyway.
There was a problem hiding this comment.
I reverted back 169 to the way it was but curious would it not be more redundant to to keep it that way as proposal.status uses the result of the first current_status call rather than what I went back to which is calling it again?
| // proposal that is open and expired is rejected | ||
| assert_eq!(ContractError::Expired {}, err.downcast().unwrap()); |
There was a problem hiding this comment.
That comment is incorrect.
If the proposal was Rejected, it couldn't be closed: https://github.com/confio/poe-contracts/blob/main/packages/voting-contract/src/lib.rs#L199-L201
Can you revisit logic and adjust comments?
There was a problem hiding this comment.
I updated the commentcan make any further changes needed
| // Update Status | ||
| proposal.update_status(&env.block); |
There was a problem hiding this comment.
This is not needed. Status will be updated below, and the check is already using current_status.
There was a problem hiding this comment.
Funny how we have two, three or even more different implementations / variations of the same logic.
It'll be good to move this voting-contract package to cw-plus at some point, and change all multisigs (cw-plus, poe-contracts, and tgrade-contracts) to use it.
There was a problem hiding this comment.
Created See #194 to track that effort.
Fixes #176
Backported the logic to the package level instead of implementing it in each of the two contracts. If this was not the right approach let me know.
Instead of porting the unit test I made it into a multitest.