MP-2547 burn contract #434
Conversation
| @@ -0,0 +1,30 @@ | |||
| [package] | |||
| name = "mars-burn-contract" | |||
There was a problem hiding this comment.
Can we remove the suffix? Maybe mars-burner?
| backtraces = ["cosmwasm-std/backtraces"] | ||
| library = [] | ||
|
|
||
| [dependencies] |
There was a problem hiding this comment.
Could you please fix aligment? One space from biggest horizontal name
| thiserror = { workspace = true } | ||
|
|
||
| [dev-dependencies] | ||
| cw-multi-test = { workspace = true } |
| #[cfg_attr(not(feature = "library"), entry_point)] | ||
| pub fn query(deps: Deps, _env: Env, msg: QueryMsg) -> StdResult<Binary> { | ||
| match msg { | ||
| QueryMsg::GetBurntAmount { |
| let limit = limit.unwrap_or(DEFAULT_LIMIT).min(MAX_LIMIT) as usize; | ||
| let start = start_after.map(|denom| Bound::ExclusiveRaw(denom.into_bytes())); | ||
|
|
||
| let burnt_amounts: StdResult<Vec<BurntAmountResponse>> = BURNT_AMOUNTS |
There was a problem hiding this comment.
Let's use better pagination, something like this https://github.com/mars-protocol/contracts/blob/master/contracts/params/src/query.rs#L83
| use mars_types::burn::{BurntAmountResponse, ExecuteMsg, QueryMsg}; | ||
|
|
||
| #[test] | ||
| fn burn_funds() { |
There was a problem hiding this comment.
Would be nice to have multitest way of test to double check burned amount in the contract but this type of test should be fine as well.
|
|
||
| pub fn execute_burn_funds(deps: DepsMut, env: Env, denom: String) -> ContractResult<Response> { | ||
| let contract_address = env.contract.address; | ||
| let balance = deps.querier.query_balance(contract_address, denom.clone())?; |
There was a problem hiding this comment.
I wonder if we should check that no other funds were mistakenly sent with the msg?
For instance if I send [100uosmo, 100uatom] and pass uosmo as a denom, I will lose the uatom right?
Might be an over optimisation given that this is a burn address so you should expect to lose funds you send to it, but could prevent accidental mistakes.
There was a problem hiding this comment.
Another nit: I think in general using must_pay is probably best practice here but I don't actually see any issue with the way it is implemented. It might actually be desirable that we just burn the balance of the contract for that denom
There was a problem hiding this comment.
I don't necessarily want to tie into the same tx a bank send with a wasm burn always, imo keep it flexible so you can do these separately (e.g. if there is an issue with IBC hooks, we can separate out the IBC send with the wasm execute to burn)
We could be more defensive here and set a burn denom and reject any other denom sent to the contract, although imo this contract could be useful for others so I think it could be an over-optimisation in this case, but i'm pretty 50:50 on that so could be swayed
Contract to be deployed on Neutron for burning the token.
I've added some state/queries so for now we can easily keep track of total burn amounts.
Originally I was going to just use this contract from Osmosis to do the native burn, but I think it makes sense to also route the burns from Neutron through this contract so we have 1 central place that does the accounting for the total burn amounts.
Eventually when a mature indexing solution is in place to account for the burns and show historical data we could simplify the contract removing the state/queries