Possible for votes to get stuck in locked state if proposal fails to execute

[radicleart]

Execution failure in a proposal means the conclude method fails and votes cannot be reclaimed.

It's possible for this situation to arise after a change in state of the DAO and it can't necessarily be know from looking at the code in the execute whether or not this will arise.

For a simple example say proposal a burns a members EDG balance and then proposal b tries to transfer some of their balance and a is run shortly before b. In this situation Proposal b will fail to execute in conclude and the proposal can neither be voted down or concluded and all votes in the proposal are stuck.

Test Case

Branch feature/toggle-membership-type
See ede000-governance-token-toggle-transfer_test : Ensure DAO can't transfer EDG if token is locked

Suggestion

make
(try! (contract-call? .executor-dao execute proposal tx-sender))
catch the error and conclude the vote as unsuccessful.

[radicleart]

PR 5 includes a fix for this issue.

see Clarity-Innovation-Lab#5

[radicleart]

@MarvinJanssen this issue about the potential for votes to get trapped if execute fails is totally separate from the transfer-lock issue - apologies for the confusion - i closed that PR as this is more important to resolve.

You pointed out a better fix for this issue would be to change reclaim-votes rather than the conclude method.

e.g.

(asserts! (or (get concluded proposal-data) (>= block-height (get end-block-height proposal-data))) err-proposal-not-concluded)

I much prefer this to the change I made to conclude however maybe there should be a day or so (~144) grace added to allow time for conclude to be called - e.g.

(asserts! (or (get concluded proposal-data) (>= block-height (+ (get end-block-height proposal-data) 144))) err-proposal-not-concluded)

[MarvinJanssen]

I don't think we need a grace period because reclaiming votes does not change the proposals map that contains votes-for and votes-against used in conclude. Reclaiming votes only deletes the appropriate key in the member-total-votes which is exclusively used to track votes per member for the purpose of allowing them to reclaim. I think changing the guard in conclude can be done safely.

There is another potential issue worth considering. I had written it down somewhere but cannot find it. In short, in the current design a proposal may fail to execute, which means that conclude can be called again in the future in a bid to make it execute successfully. It is conceivable that a "passed but not successfully executed proposal" can sit dormant until a much later time, and when triggered causes effects that members at the time did not expect or simply forgot about. My reasoning for having left it as-is, is that such a proposal should be considered malicious and not voted in in the first place. If a malicious proposal is successfully voted in then it is over already.

[radicleart]

See your point about the grace period!

I'm coming back to my initial view that the change should be in conclude as solving it in reclaim still leaves the proposal in an undefined state.

If the proposal errors then the vote should be considered to have not passed, the ballot is marked concluded and everyone can reclaim their votes. This would be equivalent to having a re-run of the same vote with an updated proposal. Ideally we'd have a separate state to mark this something like the spoilt state of a ballot paper but I think this is not essential as a proposal with majority votes for but that actually failed would be enough to identify this outcome - as in this situation it would be clear the proposal failed to execute.

[MarvinJanssen]

I think what is in order here is to come up with a set of expected / general proposals and see in which ways they could error out, and how that could affect the operation of the DAO. I am not against a one-shot conclude, regardless of the outcome of the called proposal, but it does materially change the way things work. We could also save the result of the call in the proposals map so that it is not consumed. (Or alternatively, add it to the return value or a print event so that it is logged somewhere.) I still tend to think that occasional proposal dry-runs would prevent people from voting in favour of a proposal that will fail, but of course we only have theory to rely on.

Should we involve the community in coming up with, say, ten common proposals? Then we can evaluate their success and error states and see in which cases a one-shot conclude is preferable or not.