Cwd proposal delegate #562
Draft
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 ReportBase: 95.07% // Head: 94.29% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #562 +/- ##
==========================================
- Coverage 95.07% 94.29% -0.78%
==========================================
Files 41 43 +2
Lines 3775 3873 +98
==========================================
+ Hits 3589 3652 +63
- Misses 186 221 +35
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. |
0xekez
reviewed
Dec 2, 2022
| cosmwasm/rust-optimizer:0.12.6 | ||
| """ | ||
|
|
||
| [dependencies] |
There was a problem hiding this comment.
all of these can be cosmwasm-x = { workspace = true }
| REPLY_ID_EXECUTE_PROPOSAL_HOOK => { | ||
| let id = EXECUTE_CTX.load(deps.storage)?; | ||
| let Delegation { | ||
| policy_preserve_on_failure, |
Comment on lines
90
to
92
| if let SubMsgResult::Err(_) = msg.result { | ||
| // && with `let` expression above gives unstable Rust error | ||
| if policy_preserve_on_failure { |
There was a problem hiding this comment.
Suggested change
| if let SubMsgResult::Err(_) = msg.result { | |
| // && with `let` expression above gives unstable Rust error | |
| if policy_preserve_on_failure { | |
| if msg.result.is_err() && policy_preserve_on_failure { |
Comment on lines
+124
to
+127
| fn query_delegation(deps: Deps, delegation_id: u64) -> StdResult<DelegationResponse> { | ||
| let delegation = DELEGATIONS.load(deps.storage, delegation_id)?; | ||
| Ok(delegation) | ||
| } |
There was a problem hiding this comment.
probably want to make this a paginated function so that FE can list all of the delegations pending. packages/cw-paginate should make that simple
Comment on lines
119
to
122
| fn query_delegation_count(deps: Deps) -> StdResult<DelegationCountResponse> { | ||
| let count = DELEGATION_COUNT.load(deps.storage)?; | ||
| Ok(DelegationCountResponse { count }) | ||
| } |
There was a problem hiding this comment.
idea: why expose this at all? all we use it for is a nonce for delegations. let's make it a private state variable and only expose a function for incrementing it. gets us pretty close to correct-by-constructuon!
ex:
pub(crate) fn advance_current_id(store: &mut dyn Storage) -> StdResult<u64> {
let CURRENT_ID: Item<u64> = Item::new("current_id");
let id: u64 = CURRENT_ID.may_load(store)?.unwrap_or_default() + 1;
CURRENT_ID.save(store, &id)?;
Ok(id)
}| if info.sender != admin { | ||
| return Err(ContractError::Unauthorized {}); | ||
| } | ||
| assert_not_expired(&delegation.expiration, &env.block)?; |
| } | ||
| // If delegation is irrevocable, return Error | ||
| let Delegation { | ||
| policy_irrevocable, .. |
There was a problem hiding this comment.
what's the reason for having this option? i feel things like this can give fake guarantees if the contract is instantiated with a DAO as it's admin. should we be instantiating this without an admin perhap?
There was a problem hiding this comment.
good point. The DAO could always remove the proposal module and all delegations would be revocable. However, that would eliminate all their existing delegations as well. There's 2 options to go with this:
- Rename the policy to
policy_module_irrevocableand add a warning - Remove the policy altogether
Comment on lines
11
to
12
| // Add any other custom errors you like here. | ||
| // Look at https://docs.rs/thiserror/1.0.21/thiserror/ for details. |
There was a problem hiding this comment.
Suggested change
| // Add any other custom errors you like here. | |
| // Look at https://docs.rs/thiserror/1.0.21/thiserror/ for details. |
|
Will integrate changes and start on the tests! A bit slow on this, been grokking some Cosmos stuff. Need to learn Go, how dreadful |
Co-authored-by: ekez <[email protected]>
Co-authored-by: ekez <[email protected]>
Cosm-Orc Gas UsageNo gas diff larger than 0.5% Raw Report for b8e34e3
|
|
Ok, last commits were:
Lmk. |
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.
Need design review before I start writing tests! : )
Low prio, no hurries. Thought would be fun first contract to write for DaoDao.