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

Add pre-propose-approval support for multiple choice proposals, and freeze approver #883

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

NoahSaso
Copy link
Member

@NoahSaso NoahSaso commented Sep 17, 2024

this does two things:

  1. it creates a dao-pre-propose-approval-multiple contract so that multiple choice proposal modules can have approvers, just like single choice proposal modules. it also ensures the approver contract works for both approval-single and approval-multiple

  2. it freezes the approver in the proposal so that a previously created pending proposal does not change approvers if the approver gets updated while it's still pending. this maintains consistency with other DAO logic to keep things intuitive, where the members who can vote on a proposal are frozen upon proposal creation.

Copy link

codecov bot commented Sep 17, 2024

Codecov Report

Attention: Patch coverage is 97.86788% with 61 lines in your changes missing coverage. Please review.

Project coverage is 96.65%. Comparing base (340b1e1) to head (91d5e8b).
Report is 1 commits behind head on development.

Files with missing lines Patch % Lines
...se/dao-pre-propose-approval-single/src/contract.rs 52.38% 50 Missing ⚠️
.../dao-pre-propose-approval-multiple/src/contract.rs 96.97% 9 Missing ⚠️
...opose/dao-pre-propose-approval-multiple/src/msg.rs 75.00% 1 Missing ⚠️
...ose/dao-pre-propose-approval-multiple/src/tests.rs 99.95% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           development     #883      +/-   ##
===============================================
- Coverage        96.67%   96.65%   -0.03%     
===============================================
  Files              241      245       +4     
  Lines            65993    67224    +1231     
===============================================
+ Hits             63802    64974    +1172     
- Misses            2191     2250      +59     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NoahSaso NoahSaso changed the title dao-pre-propose-approval-multiple Add pre-propose-approval support for multiple choice proposals Sep 18, 2024
@NoahSaso NoahSaso changed the title Add pre-propose-approval support for multiple choice proposals Add pre-propose-approval support for multiple choice proposals, and freeze approver Sep 18, 2024
Copy link
Collaborator

@bekauz bekauz left a comment

Choose a reason for hiding this comment

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

this looks great, I don't have much to add.

maybe except that the dao_pre_propose_base design combined with message extensions is super clean and holding up really well!


let completed_proposals =
dao_pre_propose_approval_single_v241::state::COMPLETED_PROPOSALS
.range(deps.storage, None, None, Order::Ascending)
Copy link
Collaborator

Choose a reason for hiding this comment

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

wonder from how many props this could fail. maybe worth to do some test runs?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah.. what's the easiest way to test this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

both test-tube and interchaintest provide this. probably interchaintest would be easiest to test across multiple chains, but require a bit more upfront setup than test-tube

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.

2 participants