DA0-DA0 / dao-contracts

CosmWasm smart contracts for Interchain DAOs.
https://docs.daodao.zone
BSD 3-Clause "New" or "Revised" License
202 stars 132 forks source link

Single Choice and Multiple Choice Veto #752

Closed JakeHartnell closed 7 months ago

JakeHartnell commented 9 months ago

Closes #736

codecov[bot] commented 8 months ago

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (db42d45) 96.25% compared to head (54c5484) 95.71%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## development #752 +/- ## =============================================== - Coverage 96.25% 95.71% -0.54% =============================================== Files 203 204 +1 Lines 50082 50581 +499 =============================================== + Hits 48207 48415 +208 - Misses 1875 2166 +291 ``` | [Files](https://app.codecov.io/gh/DA0-DA0/dao-contracts/pull/752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DA0-DA0) | Coverage Δ | | |---|---|---| | [contracts/external/dao-migrator/src/contract.rs](https://app.codecov.io/gh/DA0-DA0/dao-contracts/pull/752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DA0-DA0#diff-Y29udHJhY3RzL2V4dGVybmFsL2Rhby1taWdyYXRvci9zcmMvY29udHJhY3QucnM=) | `95.14% <100.00%> (+0.03%)` | :arrow_up: | | [contracts/external/dao-migrator/src/msg.rs](https://app.codecov.io/gh/DA0-DA0/dao-contracts/pull/752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DA0-DA0#diff-Y29udHJhY3RzL2V4dGVybmFsL2Rhby1taWdyYXRvci9zcmMvbXNnLnJz) | `25.00% <ø> (-8.34%)` | :arrow_down: | | [...racts/external/dao-migrator/src/testing/helpers.rs](https://app.codecov.io/gh/DA0-DA0/dao-contracts/pull/752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DA0-DA0#diff-Y29udHJhY3RzL2V4dGVybmFsL2Rhby1taWdyYXRvci9zcmMvdGVzdGluZy9oZWxwZXJzLnJz) | `96.23% <100.00%> (ø)` | | | [...ntracts/external/dao-migrator/src/testing/setup.rs](https://app.codecov.io/gh/DA0-DA0/dao-contracts/pull/752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DA0-DA0#diff-Y29udHJhY3RzL2V4dGVybmFsL2Rhby1taWdyYXRvci9zcmMvdGVzdGluZy9zZXR1cC5ycw==) | `97.08% <100.00%> (+<0.01%)` | :arrow_up: | | [...external/dao-migrator/src/testing/state\_helpers.rs](https://app.codecov.io/gh/DA0-DA0/dao-contracts/pull/752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DA0-DA0#diff-Y29udHJhY3RzL2V4dGVybmFsL2Rhby1taWdyYXRvci9zcmMvdGVzdGluZy9zdGF0ZV9oZWxwZXJzLnJz) | `100.00% <100.00%> (ø)` | | | [...xternal/dao-migrator/src/testing/test\_migration.rs](https://app.codecov.io/gh/DA0-DA0/dao-contracts/pull/752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DA0-DA0#diff-Y29udHJhY3RzL2V4dGVybmFsL2Rhby1taWdyYXRvci9zcmMvdGVzdGluZy90ZXN0X21pZ3JhdGlvbi5ycw==) | `100.00% <100.00%> (ø)` | | | [contracts/external/dao-migrator/src/types.rs](https://app.codecov.io/gh/DA0-DA0/dao-contracts/pull/752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DA0-DA0#diff-Y29udHJhY3RzL2V4dGVybmFsL2Rhby1taWdyYXRvci9zcmMvdHlwZXMucnM=) | `83.33% <100.00%> (-11.91%)` | :arrow_down: | | [...s/external/dao-migrator/src/utils/query\_helpers.rs](https://app.codecov.io/gh/DA0-DA0/dao-contracts/pull/752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DA0-DA0#diff-Y29udHJhY3RzL2V4dGVybmFsL2Rhby1taWdyYXRvci9zcmMvdXRpbHMvcXVlcnlfaGVscGVycy5ycw==) | `71.79% <ø> (ø)` | | | [...s/external/dao-migrator/src/utils/state\_queries.rs](https://app.codecov.io/gh/DA0-DA0/dao-contracts/pull/752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DA0-DA0#diff-Y29udHJhY3RzL2V4dGVybmFsL2Rhby1taWdyYXRvci9zcmMvdXRpbHMvc3RhdGVfcXVlcmllcy5ycw==) | `95.37% <100.00%> (ø)` | | | [...opose/dao-pre-propose-approval-single/src/tests.rs](https://app.codecov.io/gh/DA0-DA0/dao-contracts/pull/752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DA0-DA0#diff-Y29udHJhY3RzL3ByZS1wcm9wb3NlL2Rhby1wcmUtcHJvcG9zZS1hcHByb3ZhbC1zaW5nbGUvc3JjL3Rlc3RzLnJz) | `99.92% <100.00%> (+<0.01%)` | :arrow_up: | | ... and [20 more](https://app.codecov.io/gh/DA0-DA0/dao-contracts/pull/752?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DA0-DA0) | | ... and [71 files with indirect coverage changes](https://app.codecov.io/gh/DA0-DA0/dao-contracts/pull/752/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DA0-DA0)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

:exclamation: No coverage uploaded for pull request base (development@db42d45). Click here to learn what that means.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## development #752 +/- ## ============================================== Coverage ? 96.19% ============================================== Files ? 204 Lines ? 51088 Branches ? 0 ============================================== Hits ? 49144 Misses ? 1944 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bekauz commented 7 months ago

@NoahSaso @JakeHartnell I think it's mostly ready. I'll give it another pass in a few days for the final cleanup.

One question though: do we want to accept votes when prop is Timelocked or Vetoed? atm voting is allowed until the prop expiration (not the timelock expiration but the prop itself) is active. Somewhat similar logic to how we allow voting when prop is passed early before expiration so that any members late for voting can still cast their votes to express their opinion.

JakeHartnell commented 7 months ago

One question though: do we want to accept votes when prop is Timelocked or Vetoed? atm voting is allowed until the prop expiration (not the timelock expiration but the prop itself) is active. Somewhat similar logic to how we allow voting when prop is passed early before expiration so that any members late for voting can still cast their votes to express their opinion.

Hmmmm, on the fence about this... guess we should make it consistent?

NoahSaso commented 7 months ago

One question though: do we want to accept votes when prop is Timelocked or Vetoed? atm voting is allowed until the prop expiration (not the timelock expiration but the prop itself) is active. Somewhat similar logic to how we allow voting when prop is passed early before expiration so that any members late for voting can still cast their votes to express their opinion.

Hmmmm, on the fence about this... guess we should make it consistent?

yeah i'm on the fence too.

on one hand, it feels weird that some may get to vote (and have their vote recorded/displayed) while others who did not vote before it was vetoed (pre-expiration) do not get to express an opinion.. but on the other hand, it feels meaningless to vote after a veto, so i foresee some people not voting while others vote anyway, leading to an outcome that is not representative of the members' opinions.

maybe it shouldn't be veto-able until after the prop is decided (i.e. it is early-passed because revoting is disabled, or it expires). or alternatively, votes could be deleted if it's vetoed early (that feels pretty weird too though). wdyt about that?

codecov[bot] commented 7 months ago

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (6ca751c) 96.31% compared to head (37ebd9d) 96.43%.

Files Patch % Lines
...cts/proposal/dao-proposal-multiple/src/contract.rs 94.44% 5 Missing :warning:
...racts/proposal/dao-proposal-single/src/contract.rs 98.09% 2 Missing :warning:
...e-propose/dao-pre-propose-approver/src/contract.rs 0.00% 1 Missing :warning:
...cts/proposal/dao-proposal-multiple/src/proposal.rs 94.11% 1 Missing :warning:
...l/dao-proposal-multiple/src/testing/instantiate.rs 50.00% 1 Missing :warning:
...racts/proposal/dao-proposal-single/src/proposal.rs 95.65% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## development #752 +/- ## =============================================== + Coverage 96.31% 96.43% +0.12% =============================================== Files 203 204 +1 Lines 50092 52194 +2102 =============================================== + Hits 48245 50334 +2089 - Misses 1847 1860 +13 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JakeHartnell commented 7 months ago

this is sooooo cool, super excited for this!!!!! and great job.

i have a handful of questions and a few bugs i believe need correcting. also, two discussion points (that are both covered by conversations i opened in this review, just surfacing them here for visibility):

Thanks for the awesome and helpful bug discovery and comments!

  1. i think we should rename timelock entirely to veto. timelock is ambiguous and somewhat distracting. i think it makes sense for the status to be VetoTimelock as that describes the state of a proposal, but the config option being called veto makes way more sense to me than timelock. thoughts?

Yes. 100%.

  1. should the status of proposals once the veto timelock expires revert back to passed? or should it stay timelocked with an expired expiration field? i'm not really sure. i mentioned some pros and cons in the conversation where i thought of it.

Hmmmmmm, this is a really good question... going to think about this one while addressing low hanging fruit.