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

Implement new pre-propose interface and support native deposits in cw-proposal-single #481

Merged
merged 44 commits into from
Oct 10, 2022

Conversation

0xekez
Copy link
Contributor

@0xekez 0xekez commented Sep 13, 2022

This PR implements a new pre-propose interface on cw-proposal-single. The
pre-propose interface gives proposal modules the ability to delegate permission
to create proposals to a different contract. If one is present, only the
pre-propose module associated with a proposal module may create proposals.

In order to allow pre-propose modules to take actions like refunding proposal
deposits, pre-propose modules receive proposal hooks from their associated
proposal module. Proposal hooks fire when a proposal is created and when its
status changes. Like other receivers of proposal hooks, if the pre-propose
module fails to handle a hook message and errors it will be removed and the
proposal-module will fail-open by allowing any address to create a proposal
after the failure.

In summary, when a pre-propose module is associated with a proposal module:

  1. Only that pre-propose module may create new proposals.
  2. The pre-propose module will receive proposal hooks.
  3. If the pre-propose module fails to handle a hook, it is removed.

The pre-propose interface.

Pre-propose modules are only required to implement the following execute
message:

pub enum ExecuteMsg {
    /// Handles proposal hooks fired by the associated proposal
    /// module.
    ProposalHook(proposal_hooks::ProposalHookMsg),
}

The semantics of how a proposal is created and are left up to individual
modules.

Deposits

The initial use case for these pre-propose modules is to allow for more complex
proposal deposit systems. To this end: this PR builds on Jake's previous
work
and adds support for
native token proposal deposits, in addition to retaining support for cw20
deposits.

cw-pre-propose-base

Proposal deposit logic is currently identical across the cw-proposal-single
and cw-proposal-multiple proposal modules. The cw-pre-propose-base package
introduced in this PR implements this shared logic in the form of a pre-propose
module that may be extended to support the proposal semantics of a particular
module. The design of this base contract is based on and shares many
similarities with
cw721-base.

The cw-pre-propose-base-proposal-single contract added in this PR extends this
contract to support cw-proposal-single proposal messages, and the
cw-pre-propose-base-proposal-multiple contract put together by Jake in this
PR
does the same thing for
cw-proposal-multiple.

Here is a flow chart of the proposal creation process with cw-pre-propose-base:

In addition to the logic described in that flowchart, the module also supports
updating it's configuration, and the DAO withdrawing deposits via the following
executed messages:

pub enum ExecuteMsg {
    /// Updates the configuration of this module. This will completely
    /// override the existing configuration. This new configuration
    /// will only apply to proposals created after the config is
    /// updated. Only the DAO may execute this message.
    UpdateConfig {
        deposit_info: Option<UncheckedDepositInfo>,
        open_proposal_submission: bool,
    },
    /// Withdraws funds inside of this contract to the message
    /// sender. The contracts entire balance for the specifed DENOM is
    /// withdrawn to the message sender. Only the DAO may call this
    /// method.
    ///
    /// This is intended only as an escape hatch in the event of a
    /// critical bug in this contract or it's proposal
    /// module. Withdrawing funds will cause future attempts to return
    /// proposal deposits to fail their transactions as the contract
    /// will have insufficent balance to return them. In the case of
    /// `cw-proposal-single` this transaction failure will cause the
    /// module to remove the pre-propose module from its proposal hook
    /// receivers.
    ///
    /// More likely than not, this should NEVER BE CALLED unless a bug
    /// in this contract or the proposal module it is associated with
    /// has caused it to stop receiving proposal hook messages, or if
    /// a critical security vulnerability has been found that allows
    /// an attacker to drain proposal deposits.
    Withdraw {
        /// The denom to withdraw funds for. If no denom is specified,
        /// the denomination currently configured for proposal
        /// deposits will be used.
        ///
        /// You may want to specify a denomination here if you are
        /// withdrawing funds that were previously accepted for
        /// proposal deposits but are not longer used due to an
        /// `UpdateConfig` message being executed on the contract.
        denom: Option<UncheckedDenom>,
    },
}

Some important behaviors of this module to note:

  1. If this module receives a new proposal hook for a proposal that it did not
    create, it will do nothing and not fail the transaction. This addresses the
    case where this module has replaced an existing module and there are
    outstanding proposals.
  2. Proposal deposits are snapshotted meaning that updating the config of this
    module will not change deposit information for in-flight proposals.

Proposal Module Changes

Proposal modules are largely not impacted by these changes. The existing
proposal hook system (though slightly modified) is used to feed updates to
pre-propose modules.

There are only three changes to the cw-proposal-single interface.

  1. Deposit information in InstantiateMsg is replaced with pre_propose_info
    which allows instantiating a pre-propose module as part of the proposal
    module's instantiation.
  2. A new proposer: Option<String> field is added to the ExecuteMsg::Propose
    variant. If a pre-propose module is associated with the proposal module, it
    is expected that this is Some(address to attribute proposal creation). If
    no pre-propose module is associated, this field is expected to be None. In
    this case the proposal module will fill in the proposal's proposer with the
    sender of the message.
  3. A new ExecuteMsg::UpdatePreProposeInfo message variant is added which
    allows the DAO to update its pre-propose module.

Notably, if the DAO swaps out a pre-propose module, the old module will stop
receiving proposal hooks. It is expected (and covered in tests) that, in this
case, the DAO will executed the Withdraw method on its pre-propose module and
reimburse proposers of in-flight proposals who would otherwise not receive their
deposits as a result of the pre-propose module holding them no longer receiving
proposal hooks.

If you've made it this far, thanks for reading and taking the time to review my
PR!

@0xekez 0xekez changed the title Zeke/deposit modules Implement new pre-propose interface and support native deposits in cw-proposal-single Sep 13, 2022
Copy link
Contributor

@onewhiskeypls onewhiskeypls left a comment

Choose a reason for hiding this comment

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

utACK: Gave it a quick pass with the smoothest brain possible...will need to revisit a few things tomorrow, especially the test coverage. Feel free to ignore...everything

contracts/cw-proposal-single/Cargo.toml Show resolved Hide resolved
contracts/cw-pre-propose-base-proposal-single/src/tests.rs Outdated Show resolved Hide resolved
packages/cw-pre-propose-base/src/execute.rs Outdated Show resolved Hide resolved
packages/proposal-hooks/src/lib.rs Outdated Show resolved Hide resolved
packages/proposal-hooks/src/lib.rs Show resolved Hide resolved
packages/cw-pre-propose-base/src/execute.rs Show resolved Hide resolved
packages/cw-pre-propose-base/Cargo.toml Show resolved Hide resolved
contracts/cw-proposal-single/src/contract.rs Show resolved Hide resolved
packages/cw-core-interface/src/lib.rs Show resolved Hide resolved
packages/cw-denom/src/lib.rs Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2022

Codecov Report

Base: 93.62% // Head: 94.40% // Increases project coverage by +0.78% 🎉

Coverage data is based on head (e7e3ad6) compared to base (d822491).
Patch coverage: 94.41% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #481      +/-   ##
==========================================
+ Coverage   93.62%   94.40%   +0.78%     
==========================================
  Files          34       36       +2     
  Lines        3480     3361     -119     
==========================================
- Hits         3258     3173      -85     
+ Misses        222      188      -34     
Impacted Files Coverage Δ
contracts/cw-proposal-single/src/proposal.rs 95.58% <ø> (+7.75%) ⬆️
...ntracts/cw20-staked-balance-voting/src/contract.rs 94.35% <ø> (ø)
packages/voting/src/status.rs 87.50% <ø> (ø)
packages/voting/src/threshold.rs 100.00% <ø> (+16.00%) ⬆️
packages/voting/src/voting.rs 96.38% <ø> (ø)
packages/cw-pre-propose-base/src/execute.rs 86.30% <86.30%> (ø)
contracts/cw-proposal-single/src/v1_state.rs 94.73% <94.73%> (ø)
packages/cw-denom/src/lib.rs 97.77% <97.77%> (ø)
contracts/cw-proposal-single/src/contract.rs 99.30% <98.29%> (+5.34%) ⬆️
contracts/cw-core/src/contract.rs 93.57% <100.00%> (+0.01%) ⬆️
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@0xekez
Copy link
Contributor Author

0xekez commented Oct 8, 2022

dang. code coverage isn't telling us much here. this file, for example is described as being 83% covered, despite having 100% coverage by visual inspection.

@github-actions
Copy link

github-actions bot commented Oct 9, 2022

Cosm-Orc Gas Usage

Contract Op Name Gas Used Old Gas Used Gas Diff File
cw_core Instantiate__exc_stake_create_dao 1204295 980056 +22.8802% ci/integration_tests/src/helpers/helper.rs:101
cw_core Instantiate__exc_items_create_dao 1205368 981130 +22.8551% ci/integration_tests/src/helpers/helper.rs:101
cw_core Instantiate__exc_admin_msgs_create_dao 1204319 980080 +22.8797% ci/integration_tests/src/helpers/helper.rs:101
cw_core Instantiate__exc_admin_msgs_create_dao_with_admin 1205368 981130 +22.8551% ci/integration_tests/src/helpers/helper.rs:101
cw_core Instantiate__inst_admin_create_dao 1205368 981130 +22.8551% ci/integration_tests/src/helpers/helper.rs:101
cw_core Instantiate__inst_dao_no_admin 1204319 980080 +22.8797% ci/integration_tests/src/helpers/helper.rs:101
cw20_stake Store__Store 28795340 28643210 +0.5311% ci/integration_tests/src/helpers/chain.rs:98
cw20_staked_balance_voting Store__Store 29283190 29117530 +0.5689% ci/integration_tests/src/helpers/chain.rs:98
cw4_voting Store__Store 21683400 21510590 +0.8034% ci/integration_tests/src/helpers/chain.rs:98
cw721_stake Store__Store 31001830 30723310 +0.9065% ci/integration_tests/src/helpers/chain.rs:98
cw_admin_factory Store__Store 16228500 16046340 +1.1352% ci/integration_tests/src/helpers/chain.rs:98
cw_named_groups Store__Store 19472840 19289580 +0.9500% ci/integration_tests/src/helpers/chain.rs:98
cw_names_registry Store__Store 21131375 20949215 +0.8695% ci/integration_tests/src/helpers/chain.rs:98
cw_native_staked_balance_voting Store__Store 24360140 24081730 +1.1561% ci/integration_tests/src/helpers/chain.rs:98
cw_proposal_single Store__Store 52931430 51120610 +3.5423% ci/integration_tests/src/helpers/chain.rs:98
cw_staking_denom_voting Store__Store 17367550 17199360 +0.9779% ci/integration_tests/src/helpers/chain.rs:98
cw_token_swap Store__Store 19265930 19083660 +0.9551% ci/integration_tests/src/helpers/chain.rs:98
stake_cw20_external_rewards Store__Store 25316150 25148730 +0.6657% ci/integration_tests/src/helpers/chain.rs:98
stake_cw20_reward_distributor Store__Store 19316640 19137230 +0.9375% ci/integration_tests/src/helpers/chain.rs:98
Raw Report for e7e3ad6
Contract Op Name Gas Used Gas Wanted File
cw20_base Execute__exc_stake_stake_tokens 234578 319223 ci/integration_tests/src/tests/cw20_stake_test.rs:76
cw20_base Store__Store 34543940 51791270 ci/integration_tests/src/helpers/chain.rs:98
cw_core Instantiate__exc_stake_create_dao 1204295 1781831 ci/integration_tests/src/helpers/helper.rs:101
cw_core Execute__exc_items_rm 192600 264389 ci/integration_tests/src/tests/cw_core_test.rs:174
cw_core Execute__exc_items_set 194267 266889 ci/integration_tests/src/tests/cw_core_test.rs:139
cw_core Instantiate__exc_items_create_dao 1205368 1783440 ci/integration_tests/src/helpers/helper.rs:101
cw_core Store__Store 45480360 68195900 ci/integration_tests/src/helpers/chain.rs:98
cw_core Execute__exc_admin_msgs_pause_dao 194393 267078 ci/integration_tests/src/tests/cw_core_test.rs:79
cw_core Instantiate__exc_admin_msgs_create_dao 1204319 1781867 ci/integration_tests/src/helpers/helper.rs:101
cw_core Instantiate__exc_admin_msgs_create_dao_with_admin 1205368 1783440 ci/integration_tests/src/helpers/helper.rs:101
cw_core Instantiate__inst_admin_create_dao 1205368 1783440 ci/integration_tests/src/helpers/helper.rs:101
cw_core Instantiate__inst_dao_no_admin 1204319 1781867 ci/integration_tests/src/helpers/helper.rs:101
cw20_stake Store__Store 28795340 43168370 ci/integration_tests/src/helpers/chain.rs:98
cw20_staked_balance_voting Store__Store 29283190 43900145 ci/integration_tests/src/helpers/chain.rs:98
cw4_group Store__Store 22594860 33867650 ci/integration_tests/src/helpers/chain.rs:98
cw4_voting Store__Store 21683400 32500460 ci/integration_tests/src/helpers/chain.rs:98
cw721_stake Store__Store 31001830 46478105 ci/integration_tests/src/helpers/chain.rs:98
cw_admin_factory Store__Store 16228500 24318110 ci/integration_tests/src/helpers/chain.rs:98
cw_named_groups Store__Store 19472840 29184620 ci/integration_tests/src/helpers/chain.rs:98
cw_names_registry Store__Store 21131375 31672751 ci/integration_tests/src/helpers/chain.rs:98
cw_native_staked_balance_voting Store__Store 24360140 36515570 ci/integration_tests/src/helpers/chain.rs:98
cw_pre_propose_single Store__Store 32918800 49353560 ci/integration_tests/src/helpers/chain.rs:98
cw_proposal_single Store__Store 52931430 79372505 ci/integration_tests/src/helpers/chain.rs:98
cw_staking_denom_voting Store__Store 17367550 26026685 ci/integration_tests/src/helpers/chain.rs:98
cw_token_swap Store__Store 19265930 28874255 ci/integration_tests/src/helpers/chain.rs:98
stake_cw20_external_rewards Store__Store 25316150 37949585 ci/integration_tests/src/helpers/chain.rs:98
stake_cw20_reward_distributor Store__Store 19316640 28950320 ci/integration_tests/src/helpers/chain.rs:98

@0xekez
Copy link
Contributor Author

0xekez commented Oct 9, 2022

gas cost increase in instantiation is due to the addition of a new module which adds another WasmMsg::Instantiate to the creation process. i think this is ok.

Copy link
Member

@JakeHartnell JakeHartnell left a comment

Choose a reason for hiding this comment

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

:shipit:

@bmorphism
Copy link
Contributor

My rather colloquial review on the bird app: https://twitter.com/bmorphism/status/1578923792071815168

really excited for V2! ☺️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants