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

dao-voting-native-staked features #724

Closed
wants to merge 12 commits into from
Closed

Conversation

NoahSaso
Copy link
Member

@NoahSaso NoahSaso commented Aug 1, 2023

Resolves #664

This adds support for creating a new token factory denom, managed by the DAO, when instantiating the dao-voting-native-staked contract.

It uses the TokenFactory Core middleware contract so that the contract can be used on chains without token factory support using existing denoms. This contract also allows the manager (DAO) to whitelist additional addresses that can mint tokens, giving access controls to other contracts that may need it.

@NoahSaso NoahSaso changed the base branch from main to development August 1, 2023 00:18
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Patch coverage: 95.08% and project coverage change: -0.01% ⚠️

Comparison is base (ec13a79) 93.98% compared to head (a359ef4) 93.97%.

❗ Current head a359ef4 differs from pull request most recent head d7dd061. Consider uploading reports for the commit d7dd061 to get more accurate results

Additional details and impacted files
@@               Coverage Diff               @@
##           development     #724      +/-   ##
===============================================
- Coverage        93.98%   93.97%   -0.01%     
===============================================
  Files               62       62              
  Lines             5600     5646      +46     
===============================================
+ Hits              5263     5306      +43     
- Misses             337      340       +3     
Files Changed Coverage Δ
...ts/voting/dao-voting-native-staked/src/contract.rs 95.32% <95.08%> (-0.51%) ⬇️

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

@NoahSaso NoahSaso requested review from bekauz and JakeHartnell August 2, 2023 16:56
@NoahSaso NoahSaso requested a review from Art3miX August 8, 2023 20:31
@NoahSaso
Copy link
Member Author

NoahSaso commented Aug 8, 2023

Blocked on cw-multi-test supporting BankQuery::Supply :(
PR here though it seems inactive: CosmWasm/cw-multi-test#17

@NoahSaso NoahSaso changed the title Create token factory denom when instantiating dao-voting-native-staked dao-voting-native-staked features Aug 8, 2023
@NoahSaso NoahSaso linked an issue Aug 8, 2023 that may be closed by this pull request
3 tasks
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.

some feedback :) looks good though

use cosmwasm_schema::cw_serde;
use cosmwasm_std::{to_binary, Addr, StdResult, Storage, SubMsg, Uint128, WasmMsg};

// This is just a helper to properly serialize the above message
Copy link
Collaborator

Choose a reason for hiding this comment

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

which above message?

}

#[voting_module_query]
#[active_query]
#[cw_serde]
#[derive(QueryResponses)]
pub enum QueryMsg {
#[returns(crate::state::Config)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

leftover paths, maybe worth running an fmt for consistency


let initial_supply = initial_balances
.iter()
.fold(Uint128::zero(), |p, n| p + n.amount);
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be nice to add some comments or more descriptive closure parameter names here

});
Ok(Response::new()
.add_message(msg)
.add_submessages(hook_msgs)
.add_attribute("action", "unstake")
.add_attribute("from", info.sender)
.add_attribute("amount", amount)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is not a part of the pr but what happens if in Some(duration) case we exceed the max claims?

Copy link
Member

Choose a reason for hiding this comment

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

Worth an investigation.

addr: String,
) -> Result<Response, ContractError> {
let config: Config = CONFIG.load(deps.storage)?;
if Some(info.sender.clone()) != config.owner && Some(info.sender.clone()) != config.manager {
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps worth adding a check on instantiation that owner and manager are not the same

Copy link
Member

Choose a reason for hiding this comment

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

IMO, going to replace this outdated pattern with cw-ownable like we are using elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

Also, IMO not really a problem if they are the same. Have an existing ticket for the cw-ownable work. May pass on it in my follow on PR because there is already a lot there.

addr: String,
) -> Result<Response, ContractError> {
let config: Config = CONFIG.load(deps.storage)?;
if Some(info.sender.clone()) != config.owner && Some(info.sender.clone()) != config.manager {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

JakeHartnell pushed a commit that referenced this pull request Aug 18, 2023
@JakeHartnell
Copy link
Member

Closing in favor of #728 which has these commits anyway. Thanks for getting a head start on this!

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.

dao-voting-native-staked missing features
3 participants