WIP New deposit features #465
Conversation
Codecov Report
@@ Coverage Diff @@
## main #465 +/- ##
==========================================
- Coverage 95.81% 95.71% -0.11%
==========================================
Files 34 34
Lines 5448 5460 +12
==========================================
+ Hits 5220 5226 +6
- Misses 228 234 +6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
This is great. :) Cool to see an example of cw-asset being used - simpler code and we get to support both native and cw20 tokens.
Left comments as if we'd like to merge. I'm thinking the play here though is probably to leave this open for a bit and use it as a jumping off point to start on #462.
Appreciate you getting this moving and all the thoughts on deposits!
| // If no refunds, or failed proposals not refund, funds go to DAO | ||
| _ => get_return_deposit_msg(deposit_info, &config.dao)?, |
There was a problem hiding this comment.
These match statements might be nice to actually fully write out. It's probably a desirable feature that adding a new variant will cause a compile error and force us to check this logic again. I do love how clean this is though lol
| // If no refunds, or failed proposals not refund, funds go to DAO | |
| _ => get_return_deposit_msg(deposit_info, &config.dao)?, | |
| // If no refunds, or failed proposals not refund, funds go to DAO | |
| DepositRefundPolicy::Never | DepositRefundPolicy::OnlyPassed => get_return_deposit_msg(deposit_info, &config.dao)?, |
| _ => Err(StdError::GenericErr { | ||
| msg: String::from("Unsupported asset"), | ||
| }), |
There was a problem hiding this comment.
Will want to add a custom error here if we choose to merge this before doing the refactor.
JakeHartnell
force-pushed
the
new-deposit-features
branch
from
768adae
to
55dec40
Compare
|
Raw Report for 55dec40
|
| pub struct CheckedDepositInfo { | ||
| /// The address of the cw20 token to be used for proposal | ||
| /// deposits. | ||
| pub token: Addr, | ||
| pub token: AssetInfo, | ||
| /// The number of tokens that must be deposited to create a | ||
| /// proposal. | ||
| pub deposit: Uint128, | ||
| /// If failed proposals should have their deposits refunded. | ||
| pub refund_failed_proposals: bool, | ||
| pub refund_policy: DepositRefundPolicy, | ||
| } |
There was a problem hiding this comment.
This is a subtle one, but this change actually breaks v1 -> v2 migration logic. That logic expects that a v1 deposit info struct will deserialize into this type. By changing the layout, we break that.
Can probably hold off on updating the code until I've got a pre-propose module PR up and we decide if we'd like to merge this.
| let asset = AssetBase::new(info.token.clone(), info.deposit); | ||
| let transfer_msg = match asset.info { | ||
| AssetInfoBase::Cw20(ref _addr) => asset.transfer_from_msg(sender, contract), | ||
| AssetInfoBase::Native(ref _denom) => asset.transfer_msg(contract), |
There was a problem hiding this comment.
Another tricky one. This won't actually require the deposit to be paid if, at the time of proposal creation, there are are info.deposit or more tokens in the proposal module.
Tests don't catch this because they're only testing the case where a single proposal being created. In that case the contract has zero native tokens so not paying the deposit will cause it to transfer info.deposit tokens to itself which it does not have. If it already had a deposit inside of it that would work regardless of if the proposer paid the deposit.
Great example of why breaking this logic out and having it fail open is so important. Deposits are surprisingly tricky to get right and do elegantly.
There was a problem hiding this comment.
Ah, really good catch.
|
Closing in favor of #473 |
Refactor proposal deposits to support new features.
DON'T MERGE. We may choose a different approach.