Hardening overrule proposals with additional checks #NTRN-325 #32

oldremez · 2023-01-17T12:36:05Z

The changes made in this PR:

Added more checks to the pre-propose overrule module. It is mostly done to avoid spam proposals. The checks are:
1. Is the subdao in the DAO's subdao list
2. Do subdao and timelock point to each other
3. Is the proposal we're to create overrule proposal for in the timelocked state
4. Is the overrule proposal is already created (to avoid duplication)
Overrule proposal name/description are improved and contain subdao name and proposal id
Added query to get information about wich overrule proposal corresponds to this particular subdao proposal
Added more error results for overrule pre proposal module
The query interface of all the pre-proposal modules changed in a way to be compatible. It used Base() enum wrapper before, which made it impossible to make base queries to those modules as-is. Now, the model with templates implemented.
Timelock contract now automatically creates the overrule proposal when the subdao proposal is to be timelocked
Timelock contract init message changed. Instead of receiving the timelock duration, it receives the overrule pre proposal module.
Instead of checking if time passed for allowing the proposal execution, the timelock contract now queries if the respective overrule proposal of the main DAO is rejected.

Genesis changes: neutron-org/neutron#201
Integration tests: neutron-org/neutron-integration-tests#131

…istry # Conflicts: # artifacts/checksums.txt # artifacts/checksums_intermediate.txt # artifacts/cwd_core.wasm # artifacts/cwd_pre_propose_overrule.wasm # artifacts/cwd_pre_propose_single.wasm # artifacts/cwd_proposal_single.wasm # artifacts/cwd_subdao_core.wasm # artifacts/cwd_subdao_pre_propose_single.wasm # artifacts/cwd_subdao_proposal_single.wasm # artifacts/cwd_subdao_timelock_single.wasm

…istry

zavgorodnii · 2023-04-13T09:03:29Z

contracts/dao/pre-propose/cwd-pre-propose-single-overrule/src/contract.rs

+
+    let mut start_after: Option<SubDao> = None;
+
+    // unfortunately, there is no way to get the total subdao number so we do infinite loop here


i would consider adding another state value to the main dao with the counter of currently registered proposals. you can then query for this value to avoid the weirdness below

do you mean add a query to check is the subdao exist?

I think @oopcode means the query for a number of listed subdaos. So there would be not an "infinite" loop with checking "does it returns something, if not then we probably reached the end of the list" but just loop through the predicted number of items.

yes, it makes perfect sense, but my point was, if we are modifying main dao query interface, we can create a more helpful query for this especially case

I did what @swelf19 proposed because it makes things much cleaner

swelf19

LGTM. Minor issues found only

swelf19 · 2023-04-12T15:50:13Z

contracts/dao/pre-propose/cwd-pre-propose-single-overrule/src/contract.rs

+    if proposal_modules.is_empty() {
+        return Err(PreProposeOverruleError::SubdaoMisconfigured {});
+    }
+
+    let prop_policy: ProposalCreationPolicy = deps.querier.query_wasm_smart(
+        proposal_modules.first().unwrap().address.clone(),


I believe it's easy to refactor these lines without unwrap()

like

proposal_modules .first() .ok_or(PreProposeOverruleError::Std( cosmwasm_std::StdError::GenericErr { msg: "No modules or smth".to_string(), }, ))? .address .clone()

swelf19 · 2023-04-12T18:01:32Z

contracts/dao/pre-propose/cwd-pre-propose-single-overrule/src/contract.rs

+        if subdao_list.is_empty() {
+            return Ok(false);
+        }
+
+        start_after = Some(subdao_list.last().unwrap().clone());


The same about unwrap

This code doesn't exists anymore fortunately

swelf19 · 2023-04-12T18:29:16Z

contracts/dao/pre-propose/cwd-pre-propose-single-overrule/src/contract.rs

+
+            if !is_proposal_timelocked(
+                &deps,
+                &Addr::unchecked(timelock_contract_addr.clone()),


timelock_contract_addr is an Addr type, you no need to wrap it

swelf19 · 2023-04-12T18:32:15Z

contracts/dao/pre-propose/cwd-pre-propose-single-overrule/src/contract.rs

+
+        if subdao_list
+            .into_iter()
+            .any(|subdao| subdao.addr == subdao_core.clone().into_string())


Addr implements PartialEq for String, you can just dereference &Addr subdao.addr == *subdao_core

ratik · 2023-04-13T09:29:42Z

contracts/dao/pre-propose/cwd-pre-propose-single-overrule/src/contract.rs

+            if !is_proposal_timelocked(
+                &deps,
+                &Addr::unchecked(timelock_contract_addr.clone()),
+                proposal_id,
+            )? {
+                return Err(PreProposeOverruleError::ProposalWrongState {});
+            }


Suggested change

if !is_proposal_timelocked(

&deps,

&Addr::unchecked(timelock_contract_addr.clone()),

proposal_id,

)? {

return Err(PreProposeOverruleError::ProposalWrongState {});

}

if !is_proposal_timelocked(&deps, &timelock_contract_addr, proposal_id)? {

return Err(PreProposeOverruleError::ProposalWrongState {});

}

ratik · 2023-04-13T09:32:08Z

contracts/dao/pre-propose/cwd-pre-propose-single-overrule/src/contract.rs

+    timelock_contract: &String,
+) -> Result<Addr, PreProposeOverruleError> {
+    let timelock_config: TimelockTypes::Config = deps.querier.query_wasm_smart(
+        timelock_contract.to_string(),


already &String

Changed the argument type to Addr for consistency reasons

zavgorodnii · 2023-04-13T10:03:55Z

contracts/dao/pre-propose/cwd-pre-propose-single-overrule/src/error.rs

@@ -12,4 +12,16 @@ pub enum PreProposeOverruleError {

    #[error("Base pre propose messages aren't supported.")]
    MessageUnsupported {},
+
+    #[error("Subdao is wrongly configured.")]


misconfigured?

zavgorodnii · 2023-04-13T10:05:02Z

contracts/dao/pre-propose/cwd-pre-propose-single-overrule/src/error.rs

+    #[error("Subdao is wrongly configured.")]
+    SubdaoMisconfigured {},
+
+    #[error("Subdao isn't in the list.")]


remove the full stop

oldremez · 2023-04-13T21:25:16Z

Important notice: after making fixes according to review comments, the mysterious error with "Invalid relocation" on my m1 mac disappeared 🤷‍♂️ will see if it will appear again

oldremez · 2023-04-14T07:04:14Z

https://github.com/neutron-org/neutron-tests/actions/runs/4694337259

oldremez · 2023-04-14T12:26:33Z

https://github.com/neutron-org/neutron-tests/actions/runs/4699297637

oldremez added 8 commits January 11, 2023 17:48

wip

92ba326

wip

6413f49

wip

7d40f0c

messages refactoring

ee32d0a

temp commit

b73035f

query and config fixes

99603e3

wip

959bd60

oldremez changed the base branch from main to neutron_audit_informal_17_01_2023 March 13, 2023 23:03

oldremez added 21 commits March 14, 2023 11:21

Merge branch 'neutron_audit_informal_17_01_2023' into feat/subdao-reg…

0570481

…istry

most of the code is written

fb4ba65

positive test scenario pass

09f4917

refactoring

779bb0b

wip

794ac50

huge refactoring, overrule creation added, more tests

7b66d5f

massive unit tests refactoring

d3bb5c1

overrule pre propose tests pass

8433023

cleaning up some stuff

06e1082

queries tests

eb2388f

QueryBase -> QueryExt

8857a2c

overrule proposal id query test

2c810b4

fixing timelock tests wip

aab8ff0

timelock fixes and tests

78159ae

schema fixes

a881fcb

clippy fixes

ff6776f

schema regen

598b609

refactoring

51d7ef6

fix imports to make release build pass

c0def32

clippy fixes

2c69309

init msg fix

b21d9d4

oldremez added 3 commits April 9, 2023 12:12

error descriptions

1a56936

more error descriptions

8dd178e

fmt

430ffe0

oldremez mentioned this pull request Apr 10, 2023

Update overrules #NTRN-325 neutron-org/neutron#201

Merged

oldremez added 3 commits April 11, 2023 00:14

refactoring+comments

3ee3cab

format fix

9841bb0

tests fix

d2a1fd6

oldremez mentioned this pull request Apr 10, 2023

Testing hardened overrules #NTRN-330 #NTRN-325 neutron-org/neutron-integration-tests#131

Merged

Merge branch 'main' into feat/subdao-registry

2b825b4

zavgorodnii reviewed Apr 13, 2023

View reviewed changes

swelf19 reviewed Apr 13, 2023

View reviewed changes

ratik reviewed Apr 13, 2023

View reviewed changes

zavgorodnii reviewed Apr 13, 2023

View reviewed changes

oldremez added 7 commits April 13, 2023 13:34

post-review error descriptions fixes

c63fad0

post-review refactoring

9fd4d73

post-review fmt

0675ea3

post-review refactoring

6f8dcfd

post-review get subdao query

2d08e28

post-review fmt

d3b0160

post-review clippy

03421cc

ratik approved these changes Apr 14, 2023

View reviewed changes

swelf19 approved these changes Apr 14, 2023

View reviewed changes

zavgorodnii approved these changes Apr 14, 2023

View reviewed changes

zavgorodnii added the trigger-tests Apply this label to PRs to trigger tests in the neutron-tests repository. label Apr 14, 2023

zavgorodnii merged commit 376cd05 into main Apr 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hardening overrule proposals with additional checks #NTRN-325 #32

Hardening overrule proposals with additional checks #NTRN-325 #32

oldremez commented Jan 17, 2023 •

edited

Loading

zavgorodnii Apr 13, 2023

swelf19 Apr 13, 2023

oldremez Apr 13, 2023

swelf19 Apr 13, 2023

oldremez Apr 13, 2023

swelf19 left a comment •

edited

Loading

swelf19 Apr 12, 2023

ratik Apr 13, 2023

oldremez Apr 13, 2023

swelf19 Apr 12, 2023

oldremez Apr 13, 2023

swelf19 Apr 12, 2023

oldremez Apr 13, 2023

swelf19 Apr 12, 2023

oldremez Apr 13, 2023

ratik Apr 13, 2023

oldremez Apr 13, 2023

ratik Apr 13, 2023

oldremez Apr 13, 2023

zavgorodnii Apr 13, 2023

oldremez Apr 13, 2023

zavgorodnii Apr 13, 2023

oldremez Apr 13, 2023

oldremez commented Apr 13, 2023

oldremez commented Apr 14, 2023

oldremez commented Apr 14, 2023


		let mut start_after: Option<SubDao> = None;

		// unfortunately, there is no way to get the total subdao number so we do infinite loop here

Hardening overrule proposals with additional checks #NTRN-325 #32

Hardening overrule proposals with additional checks #NTRN-325 #32

Conversation

oldremez commented Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swelf19 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oldremez commented Apr 13, 2023

oldremez commented Apr 14, 2023

oldremez commented Apr 14, 2023

oldremez commented Jan 17, 2023 •

edited

Loading

swelf19 left a comment •

edited

Loading