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.

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

Fix status/execution bugs in flex-multisig #643

Merged
merged 5 commits into from
Feb 2, 2022
Merged

Conversation

uint
Copy link
Contributor

@uint uint commented Jan 31, 2022

Closes #630, ports tests/fixes from https://github.com/confio/poe-contracts.

confio/poe-contracts#12 was not an issue here, but I added a test for it.

confio/poe-contracts#14 is indeed a bug here, fixing in this PR.

confio/poe-contracts#16 was a false alarm in the first place.

@uint uint requested a review from maurolacy January 31, 2022 19:35
@uint
Copy link
Contributor Author

uint commented Jan 31, 2022

@uint uint marked this pull request as ready for review January 31, 2022 19:46
Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Looks good.

Same comments than in confio/poe-contracts#60 apply:

  • Why not use update_status() in execute (mutable) calls everywhere, and leave current_status() just for queries?

Perhaps take the opportunity and fix the other issues with state here as well. Or, in a follow-up. Your call.

@uint
Copy link
Contributor Author

uint commented Feb 2, 2022

Looks good.

Same comments than in confio/poe-contracts#60 apply:

* Why not use `update_status()` in execute (mutable) calls everywhere, and leave `current_status()` just for queries?

As in here? https://github.com/CosmWasm/cw-plus/pull/643/files#diff-f395b2761fa612ba44398ada5f0a4d9a9fc7333ce4565281fafd99329c4dcb23R200-R206

I don't think there's a point using update_status() if it's changed to Executed in the next line anyway. But we could add something like mark_executed() to the Proposal type if you think that's clearer.

Perhaps take the opportunity and fix the other issues with state here as well. Or, in a follow-up. Your call.

Nah, I'd rather see another PR for that.

@maurolacy
Copy link
Contributor

maurolacy commented Feb 2, 2022

I don't think there's a point using update_status() if it's changed to Executed in the next line anyway.

I agree. In fact, update_status makes sense only in a limited number of cases, as the status changes will be discarded in case of error.

But we could add something like mark_executed() to the Proposal type if you think that's clearer.

I think that will be good, but that it will better to do it in (even) another iteration.

Perhaps take the opportunity and fix the other issues with state here as well. Or, in a follow-up. Your call.

Nah, I'd rather see another PR for that.

OK.

@uint uint merged commit 169cf11 into main Feb 2, 2022
@uint uint deleted the 630-flex-multisig-status-bugs branch February 2, 2022 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cw3-flex/fixed-multisig] Status changes after voting and proposal expiration
2 participants