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

Bounties Contract #715

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

Bounties Contract #715

wants to merge 13 commits into from

Conversation

JakeHartnell
Copy link
Member

@JakeHartnell JakeHartnell commented Jul 2, 2023

Closes #714

@JakeHartnell JakeHartnell changed the base branch from main to development July 2, 2023 18:31
@codecov
Copy link

codecov bot commented Jul 9, 2023

Codecov Report

❗ No coverage uploaded for pull request base (development@0367c84). Click here to learn what that means.
Patch coverage: 88.79% of modified lines in pull request are covered.
Report is 5 commits behind head on development.

Additional details and impacted files
@@              Coverage Diff               @@
##             development     #715   +/-   ##
==============================================
  Coverage               ?   93.90%           
==============================================
  Files                  ?       63           
  Lines                  ?     5709           
  Branches               ?        0           
==============================================
  Hits                   ?     5361           
  Misses                 ?      348           
  Partials               ?        0           
Files Changed Coverage Δ
contracts/external/cw-bounties/src/contract.rs 88.79% <88.79%> (ø)

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

@github-actions
Copy link

github-actions bot commented Jul 9, 2023

Cosm-Orc Gas Usage

Contract Op Name Gas Used Old Gas Used Gas Diff File
dao_dao_core Store__Store 6297548 6180873 +1.8877% ci/integration-tests/src/helpers/chain.rs:98
cw_vesting Store__Store 4292883 4237984 +1.2954% ci/integration-tests/src/helpers/chain.rs:98
dao_pre_propose_single Store__Store 4279324 4168525 +2.6580% ci/integration-tests/src/helpers/chain.rs:98
dao_proposal_single Store__Store 6789079 6672533 +1.7467% ci/integration-tests/src/helpers/chain.rs:98
dao_voting_cw721_staked Instantiate__instantiate_dao_voting_cw721_staked 180303 178255 +1.1489% ci/integration-tests/src/tests/dao_voting_cw721_staked_test.rs:49
dao_voting_cw721_staked Store__Store 4648095 3348121 +38.8270% ci/integration-tests/src/helpers/chain.rs:98
cw20_stake Store__Store 4039409 3958328 +2.0484% ci/integration-tests/src/helpers/chain.rs:98
cw20_stake_external_rewards Store__Store 3389630 3350513 +1.1675% ci/integration-tests/src/helpers/chain.rs:98
cw20_stake_reward_distributor Store__Store 2897606 2841017 +1.9919% ci/integration-tests/src/helpers/chain.rs:98
cw_admin_factory Store__Store 2071755 2019807 +2.5719% ci/integration-tests/src/helpers/chain.rs:98
cw_fund_distributor Store__Store 3433089 3362993 +2.0843% ci/integration-tests/src/helpers/chain.rs:98
cw_payroll_factory Store__Store 3756022 3698198 +1.5636% ci/integration-tests/src/helpers/chain.rs:98
cw_token_swap Store__Store 2429112 2382897 +1.9394% ci/integration-tests/src/helpers/chain.rs:98
dao_migrator Store__Store 5202909 5126651 +1.4875% ci/integration-tests/src/helpers/chain.rs:98
dao_pre_propose_approval_single Store__Store 5040906 4924660 +2.3605% ci/integration-tests/src/helpers/chain.rs:98
dao_pre_propose_approver Store__Store 3765057 3675123 +2.4471% ci/integration-tests/src/helpers/chain.rs:98
dao_pre_propose_multiple Store__Store 4358416 4246824 +2.6277% ci/integration-tests/src/helpers/chain.rs:98
dao_proposal_condorcet Store__Store 4712211 4625956 +1.8646% ci/integration-tests/src/helpers/chain.rs:98
dao_proposal_multiple Store__Store 6611576 6470617 +2.1784% ci/integration-tests/src/helpers/chain.rs:98
dao_voting_cw20_staked Store__Store 3665035 3622525 +1.1735% ci/integration-tests/src/helpers/chain.rs:98
dao_voting_cw4 Store__Store 2665777 2686291 -0.7637% ci/integration-tests/src/helpers/chain.rs:98
dao_voting_native_staked Store__Store 3032182 2989815 +1.4170% ci/integration-tests/src/helpers/chain.rs:98
Raw Report for 1933279
Contract Op Name Gas Used Gas Wanted File
dao_dao_core Instantiate__inst_dao_no_admin 1275506 1890450 ci/integration-tests/src/helpers/helper.rs:101
dao_dao_core Instantiate__inst_admin_create_dao 1276556 1892027 ci/integration-tests/src/helpers/helper.rs:101
dao_dao_core Execute__exc_items_rm 192822 266513 ci/integration-tests/src/tests/cw_core_test.rs:171
dao_dao_core Execute__exc_items_set 194489 269016 ci/integration-tests/src/tests/cw_core_test.rs:136
dao_dao_core Instantiate__exc_items_create_dao 1276556 1892027 ci/integration-tests/src/helpers/helper.rs:101
dao_dao_core Instantiate__create_dao 1275506 1890450 ci/integration-tests/src/helpers/helper.rs:101
dao_dao_core Instantiate__exc_stake_create_dao 1275482 1890414 ci/integration-tests/src/helpers/helper.rs:101
dao_dao_core Store__Store 6297548 9423533 ci/integration-tests/src/helpers/chain.rs:98
dao_dao_core Execute__exc_admin_msgs_pause_dao 194615 269205 ci/integration-tests/src/tests/cw_core_test.rs:76
dao_dao_core Instantiate__exc_admin_msgs_create_dao 1275506 1890450 ci/integration-tests/src/helpers/helper.rs:101
dao_dao_core Instantiate__exc_admin_msgs_create_dao_with_admin 1276556 1892027 ci/integration-tests/src/helpers/helper.rs:101
cw_vesting Execute__delegate 206783 287457 ci/integration-tests/src/tests/cw_vesting_test.rs:103
cw_vesting Execute__undelegate 251879 332456 ci/integration-tests/src/tests/cw_vesting_test.rs:142
cw_vesting Execute__withdraw_reward 191369 264333 ci/integration-tests/src/tests/cw_vesting_test.rs:124
cw_vesting Instantiate__instantiate 227550 318608 ci/integration-tests/src/tests/cw_vesting_test.rs:59
cw_vesting Store__Store 4292883 6416535 ci/integration-tests/src/helpers/chain.rs:98
cw20_base Execute__cw20_base_increase_allowance 142201 190586 ci/integration-tests/src/helpers/helper.rs:216
cw20_base Execute__send_and_stake_cw20 234268 328683 ci/integration-tests/src/helpers/helper.rs:180
cw20_base Execute__exc_stake_stake_tokens 234536 320954 ci/integration-tests/src/tests/cw20_stake_test.rs:76
cw20_base Store__Store 4191041 6263772 ci/integration-tests/src/helpers/chain.rs:98
cw721_base Instantiate__instantiate_cw721_base 166518 227061 ci/integration-tests/src/tests/dao_voting_cw721_staked_test.rs:22
cw721_base Store__Store 3975657 5940696 ci/integration-tests/src/helpers/chain.rs:98
cw721_base Execute__mint_nft 140715 188355 ci/integration-tests/src/tests/dao_voting_cw721_staked_test.rs:96
cw721_base Execute__stake_nft 234592 329169 ci/integration-tests/src/tests/dao_voting_cw721_staked_test.rs:79
dao_pre_propose_single Execute__pre_propose_propose 1806427 2686829 ci/integration-tests/src/helpers/helper.rs:235
dao_pre_propose_single Store__Store 4279324 6396197 ci/integration-tests/src/helpers/chain.rs:98
dao_proposal_single Execute__dao_proposal_single_vote 5992028 8965158 ci/integration-tests/src/helpers/helper.rs:285
dao_proposal_single Store__Store 6789079 10160769 ci/integration-tests/src/helpers/chain.rs:98
dao_voting_cw721_staked Execute__claim_nfts 5710872 8543412 ci/integration-tests/src/tests/dao_voting_cw721_staked_test.rs:129
dao_voting_cw721_staked Instantiate__instantiate_dao_voting_cw721_staked 180303 247739 ci/integration-tests/src/tests/dao_voting_cw721_staked_test.rs:49
dao_voting_cw721_staked Store__Store 4648095 6949353 ci/integration-tests/src/helpers/chain.rs:98
dao_voting_cw721_staked Execute__unstake_nfts 230320 322761 ci/integration-tests/src/tests/dao_voting_cw721_staked_test.rs:114
multiple_contracts Execute__batch_cw721_stake_max_claims 4198067 6256677 ci/integration-tests/src/tests/dao_voting_cw721_staked_test.rs:258
cw20_stake Store__Store 4039409 6036324 ci/integration-tests/src/helpers/chain.rs:98
cw20_stake_external_rewards Store__Store 3389630 5061656 ci/integration-tests/src/helpers/chain.rs:98
cw20_stake_reward_distributor Store__Store 2897606 4323620 ci/integration-tests/src/helpers/chain.rs:98
cw4_group Store__Store 2793580 4167581 ci/integration-tests/src/helpers/chain.rs:98
cw721_roles Store__Store 5063861 7573002 ci/integration-tests/src/helpers/chain.rs:98
cw_admin_factory Store__Store 2071755 3084843 ci/integration-tests/src/helpers/chain.rs:98
cw_bounties Store__Store 2849220 4251041 ci/integration-tests/src/helpers/chain.rs:98
cw_fund_distributor Store__Store 3433089 5126844 ci/integration-tests/src/helpers/chain.rs:98
cw_payroll_factory Store__Store 3756022 5611244 ci/integration-tests/src/helpers/chain.rs:98
cw_token_swap Store__Store 2429112 3620879 ci/integration-tests/src/helpers/chain.rs:98
dao_migrator Store__Store 5202909 7781574 ci/integration-tests/src/helpers/chain.rs:98
dao_pre_propose_approval_single Store__Store 5040906 7538565 ci/integration-tests/src/helpers/chain.rs:98
dao_pre_propose_approver Store__Store 3765057 5624796 ci/integration-tests/src/helpers/chain.rs:98
dao_pre_propose_multiple Store__Store 4358416 6514835 ci/integration-tests/src/helpers/chain.rs:98
dao_proposal_condorcet Store__Store 4712211 7045527 ci/integration-tests/src/helpers/chain.rs:98
dao_proposal_multiple Store__Store 6611576 9894575 ci/integration-tests/src/helpers/chain.rs:98
dao_voting_cw20_staked Store__Store 3665035 5474763 ci/integration-tests/src/helpers/chain.rs:98
dao_voting_cw4 Store__Store 2665777 3975876 ci/integration-tests/src/helpers/chain.rs:98
dao_voting_cw721_roles Store__Store 2839938 4237118 ci/integration-tests/src/helpers/chain.rs:98
dao_voting_native_staked Store__Store 3032182 4525484 ci/integration-tests/src/helpers/chain.rs:98

@taitruong
Copy link
Contributor

@JakeHartnell, regarding ExecuteMsg::PayOut: https://github.com/DA0-DA0/dao-contracts/blob/bounties/contracts/external/cw-bounties/src/msg.rs#L30

    /// Claims a bounty (only owner)
    PayOut {
        /// Bounty id to claim
        id: u64,
        /// Recipient address where funds from bounty are claimed
        recipient: String,
    },

We could add amount: Option<Uint128> for allowing partial payout - if none is provided then it is a full payout. This way it allows use cases like:

  • partial pay out depending on defined/reached milestones
  • awarding multiple peeps contributing to same bounty

@taitruong
Copy link
Contributor

For Close and PayOut maybe there should be an optional memo. So owner can add reason or other notes.

@taitruong
Copy link
Contributor

@JakeHartnell, regarding ExecuteMsg::PayOut: https://github.com/DA0-DA0/dao-contracts/blob/bounties/contracts/external/cw-bounties/src/msg.rs#L30

    /// Claims a bounty (only owner)
    PayOut {
        /// Bounty id to claim
        id: u64,
        /// Recipient address where funds from bounty are claimed
        recipient: String,
    },

We could add amount: Option<Uint128> for allowing partial payout - if none is provided then it is a full payout. This way it allows use cases like:

  • partial pay out depending on defined/reached milestones
  • awarding multiple peeps contributing to same bounty

Ofc, this is not needed, since owner can create multiple bounties for achieving same goal. But this way, owner can decide which way is appropiate for bounty ((one bounty with partial payout or multiple bounties with each single payout). This change shouldn't be a big deal and keeps contract still simple.

@taitruong
Copy link
Contributor

@JakeHartnell, regarding ExecuteMsg::PayOut: https://github.com/DA0-DA0/dao-contracts/blob/bounties/contracts/external/cw-bounties/src/msg.rs#L30

    /// Claims a bounty (only owner)
    PayOut {
        /// Bounty id to claim
        id: u64,
        /// Recipient address where funds from bounty are claimed
        recipient: String,
    },

We could add amount: Option<Uint128> for allowing partial payout - if none is provided then it is a full payout. This way it allows use cases like:

  • partial pay out depending on defined/reached milestones
  • awarding multiple peeps contributing to same bounty

Nevermind. Figured out partial payment is way too complicated. Like when updating on partial payouts, it will affect BountyStatus when updated amount is equal to partial payout -> Bounty is closed otherwise it keeps open, Bounty need another prop claimed for updated payments made so far. I was almost done implementing this including tests - but trashed it since contract would be too complicated without big value.

@taitruong
Copy link
Contributor

@JakeHartnell, found another bug when updating bounty with lower amount. Like bounty is 100 Juno, update to 50 Juno, then owner gets 50 Juno back. This works. But if owner accidentally sends 50 Juno, 100 Juno should be send back - not just 50.

Fix it in PR 730. Also added more tests extensively and checking Bounty state on each test.

@taitruong
Copy link
Contributor

@JakeHartnell, found another bug when updating bounty with lower amount. Like bounty is 100 Juno, update to 50 Juno, then owner gets 50 Juno back. This works. But if owner accidentally sends 50 Juno, 100 Juno should be send back - not just 50.

Fix it in PR 730. Also added more tests extensively and checking Bounty state on each test.

Check here: #730

@JakeHartnell JakeHartnell changed the title WIP Bounties Contract Bounties Contract Nov 9, 2023
@JakeHartnell JakeHartnell marked this pull request as ready for review November 9, 2023 14:22
Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Attention: 50 lines in your changes are missing coverage. Please review.

Comparison is base (db42d45) 96.25% compared to head (5f90c8b) 96.21%.

Additional details and impacted files
@@               Coverage Diff               @@
##           development     #715      +/-   ##
===============================================
- Coverage        96.25%   96.21%   -0.04%     
===============================================
  Files              203      208       +5     
  Lines            50082    50913     +831     
===============================================
+ Hits             48207    48988     +781     
- Misses            1875     1925      +50     
Files Coverage Δ
contracts/external/cw-bounties/src/state.rs 100.00% <100.00%> (ø)
contracts/external/cw-bounties/src/error.rs 50.00% <50.00%> (ø)
contracts/external/cw-bounties/src/msg.rs 50.00% <50.00%> (ø)
contracts/external/cw-bounties/src/tests.rs 96.52% <96.52%> (ø)
contracts/external/cw-bounties/src/contract.rs 88.42% <88.42%> (ø)

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

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.

cool little contract! LGTM, left some small questions and suggestions

Cargo.toml Show resolved Hide resolved
contracts/external/cw-bounties/src/state.rs Outdated Show resolved Hide resolved
contracts/external/cw-bounties/src/msg.rs Outdated Show resolved Hide resolved
contracts/external/cw-bounties/src/contract.rs Outdated Show resolved Hide resolved
contracts/external/cw-bounties/src/contract.rs Outdated Show resolved Hide resolved
id: u64,
) -> Result<Response, ContractError> {
// Check bounty exists
let mut bounty = BOUNTIES.load(deps.storage, id)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure what the expected outcome here is if bounty id doesn't exist? perhaps may_load?

Copy link
Member Author

Choose a reason for hiding this comment

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

Expected outcome is that it fails.

@JakeHartnell
Copy link
Member Author

Will merge and ship this after we're done with the veto stuff. ❤️

Copy link

codecov bot commented Dec 29, 2023

Codecov Report

Attention: Patch coverage is 94.26523% with 48 lines in your changes are missing coverage. Please review.

Project coverage is 96.27%. Comparing base (d59c282) to head (793085a).

Files Patch % Lines
contracts/external/cw-bounties/src/contract.rs 89.08% 25 Missing ⚠️
contracts/external/cw-bounties/src/tests.rs 96.50% 21 Missing ⚠️
contracts/external/cw-bounties/src/error.rs 0.00% 1 Missing ⚠️
contracts/external/cw-bounties/src/msg.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           development     #715      +/-   ##
===============================================
- Coverage        96.30%   96.27%   -0.04%     
===============================================
  Files              207      212       +5     
  Lines            53490    54327     +837     
===============================================
+ Hits             51513    52302     +789     
- Misses            1977     2025      +48     

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

Copy link
Collaborator

@Art3miX Art3miX 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.

How about a claim by the user? (for V2)

The user will start a claim on the bounty, and than the owner will be able to pay it, if no claims, the owner can do whatever he wish with the bounty, but if there is a claim, the owner will have to deny those claims before canceling the bounty.

The main purpose of this is to put denied claims publicly, so owners will have to provide a reason to why this claim was denied.

@JakeHartnell JakeHartnell force-pushed the bounties branch 2 times, most recently from c60d0e3 to 7b90923 Compare January 9, 2024 01:52
taitruong and others added 12 commits March 26, 2024 14:38
…ate, test all of this (#720)

Co-authored-by: Tai 'Mr. T' Truong <[email protected]>
Co-authored-by: Tai 'Mr. T' Truong <[email protected]>
* check Bounty is correct in all tests

* fix update bounty with lower amount and owner accidentally send funds

* cleanup and test update with sending wrong denom
Co-authored-by: bekauz <[email protected]>
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.

Bounties contract
4 participants