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.

Sign up for GitHub

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

Jump to bottom

Add active threshold functionality to CW4 voting module, closes #781 #819

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

Add active threshold functionality to CW4 voting module, closes #781 #819

wants to merge 6 commits into from

Conversation

theghostmac
Copy link

Changelog

  • Added new custom error message to error.rs, InvalidThreshold
  • Added the query_is_active function and corresponding Query::IsActive
  • Added the execute_update_active_threshold function and corresponding ExecuteMsg::UpdateActiveThreshold
  • Added the new optional Config and updated the instantiate function to make use of it.

Author

GhostMac

@JakeHartnell
Copy link
Member

Needs a few things:

  • Fix the build
  • Add tests for active_threshold and being able to update it
  • Make sure CI passes
  • Clean up

JakeHartnell
JakeHartnell reviewed Apr 7, 2024
View reviewed changes
contracts/voting/dao-voting-cw4/src/state.rs Outdated
Comment on lines 13 to 19
#[cw_serde]
pub struct Config {
pub active_threshold_enabled: bool,
pub active_threshold: Option<ActiveThreshold>,
}

pub const CONFIG: Item<Config> = Item::new("config");
Copy link
Member

Choose a reason for hiding this comment

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

We don't really need this as if ACTIVE_THRESHOLD is not set, it's not enabled. There is no other config needed for this contract.

@theghostmac
Copy link
Author

Needs a few things:

  • Fix the build
  • Add tests for active_threshold and being able to update it
  • Make sure CI passes
  • Clean up

I have updated the contract with everything you pointed out. Build passes. Moving on to the tests.

JakeHartnell
JakeHartnell reviewed Apr 7, 2024
View reviewed changes
contracts/voting/dao-voting-cw4/src/state.rs Outdated
Comment on lines 12 to 17
#[cw_serde]
pub struct Config {
pub active_threshold: Option<ActiveThreshold>,
}

pub const CONFIG: Item<Config> = Item::new("config");
Copy link
Member

Choose a reason for hiding this comment

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

This is the same info as ACTIVE_THRESHOLD. Delete Config struct.

@JakeHartnell
Copy link
Member

Needs a few things:

  • Fix the build
  • Add tests for active_threshold and being able to update it
  • Make sure CI passes
  • Clean up

I have updated the contract with everything you pointed out. Build passes. Moving on to the tests.

Close, but see above comments.

  • Get rid of CONFIG, it's duplicate state for no reason
  • Make sure to save ACTIVE_THRESHOLD on instantiation and validate that it's NOT ACTIVE_THRESHOLD::Percent
  • Get rid of ACTIVE_THRESHOLD::Percent in the IsActive query, it does nothing right now and we should not allow it
  • Fix linting errors and tests, CI should pass

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JakeHartnell JakeHartnell JakeHartnell left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@theghostmac @JakeHartnell