Dao voting cw4 cleanup #691
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #691 +/- ##
==========================================
+ Coverage 93.82% 93.94% +0.12%
==========================================
Files 60 60
Lines 5149 5189 +40
==========================================
+ Hits 4831 4875 +44
+ Misses 318 314 -4
☔ View full report in Codecov by Sentry. |
Cosm-Orc Gas Usage
Raw Report for ae9600a
|
bekauz
reviewed
Apr 5, 2023
| } | ||
| GroupContract::Existing { address } => { | ||
| let group_contract = deps.api.addr_validate(&address)?; |
There was a problem hiding this comment.
could something go wrong here if the address is valid but does not point to a valid GroupContract?
There was a problem hiding this comment.
Yes. @0xekez, any ideas for validation here?
There was a problem hiding this comment.
Without providing any additional data, I guess you can query ListMembers with limit: 1, and verify the response is valid format (MemberListResponse { members }) and not empty?
This will:
- Verify the contract have
ListMembersquery with the right response format - Verify the members list is not empty
There was a problem hiding this comment.
Perhaps it would be nice to use a similar pattern as with UncheckedDepositInfo and CheckedDepositInfo in dao-voting module to validate the group contract
There was a problem hiding this comment.
Whats the conclusion on this? I think some minimal verification is best to at least try and catch some obvious user errors
Art3miX
reviewed
Apr 8, 2023
| } | ||
| GroupContract::Existing { address } => { | ||
| let group_contract = deps.api.addr_validate(&address)?; |
There was a problem hiding this comment.
Without providing any additional data, I guess you can query ListMembers with limit: 1, and verify the response is valid format (MemberListResponse { members }) and not empty?
This will:
- Verify the contract have
ListMembersquery with the right response format - Verify the members list is not empty
Good question! IMO this should not be enforced. What about things like SubDAOs where a certain groups contract may be used across multiple SubDAOs? That said, folks should exercise caution when using existing contracts if they don't understand who controls them. Feel this should be more of a UX thing. |
Art3miX
reviewed
Apr 26, 2023
Art3miX
reviewed
Apr 26, 2023
Art3miX
reviewed
Apr 26, 2023
JakeHartnell
force-pushed
the
dao-voting-cw4-cleanup
branch
from
732fb0a to
4f94ef8
Compare
nightly broke a bunch of linting stuff, we should probably be using stable
bekauz
approved these changes
May 18, 2023
Art3miX
approved these changes
May 24, 2023
Comment on lines
+94
to
+100
| pub fn execute( | ||
| _deps: DepsMut, | ||
| _env: Env, | ||
| _info: MessageInfo, | ||
| _msg: ExecuteMsg, | ||
| ) -> Result<Response, ContractError> { | ||
| Err(ContractError::NoExecute {}) |
There was a problem hiding this comment.
Complains no tests, I guess you can just add a simple test that trying to execute something errors?
should be a good reminder in the future to add test if execute msgs are added as well
| } | ||
| GroupContract::Existing { address } => { | ||
| let group_contract = deps.api.addr_validate(&address)?; |
There was a problem hiding this comment.
Whats the conclusion on this? I think some minimal verification is best to at least try and catch some obvious user errors
onewhiskeypls
approved these changes
May 24, 2023
There was a problem hiding this comment.
untested ACK: lgtm.
just 2 things came up:
- note: I attempted to compile and test; however, I ran in to this error. It may be an environment issue on my machine, but I wanted to bring it up anyways:
error[E0658]: `let...else` statements are unstable
--> /Users/whiskey/.cargo/registry/src/github.com-1ecc6299db9ec823/cw-ownable-derive-0.5.1/src/lib.rs:22:5
- thought: there's a test case that checks total vp at height (
test_power_at_height) and it does modification of VP (see line 284 of tests.rs. If these are possible scenarios, I'd actually like to see another test that instantiates a dao with disproportionate vp -- asetup_test_casewith VP's of 1, 1, 2, 0
This was referenced Jun 14, 2023
JakeHartnell
added a commit
that referenced
this pull request
Jul 9, 2023
* Refactor cw4 voting * Fix up tests and bugs * Add test for using an existing group contract * Update schema * Fix clippy * nightly -> stable for CI nightly broke a bunch of linting stuff, we should probably be using stable
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #675