Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error messages in Coins::sub() #10

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sushiwushi
Copy link

This PR replaces the StdError::overflow() error with more informative error in the sub() function of the Coins struct.

The new errors display the amount of tokens wanted and the amount of tokens available, which will be helpful for developers who forget to mint tokens before sending native tokens in cw_multi_test (such as, me).

#[test]
fn overflow_error_not_enough_bal() {
    let mut app = App::default();
    let contract_box = ContractWrapper::new(
        crate::contract::execute,
        crate::contract::instantiate,
        crate::contract::query,
    );
    let code_id = app.store_code(Box::new(contract_box));
    let init_msg = crate::msg::InstantiateMsg {};
    let owner = "owner";
    let owner_addr = Addr::unchecked(owner);

    let funds = Coin {
        denom: DENOM.to_string(),
        amount: Uint128::new(1000),
    };

    // app.sudo(cw_multi_test::SudoMsg::Bank(
    //     cw_multi_test::BankSudo::Mint {
    //         to_address: owner.to_string(),
    //         amount: vec![funds.clone()],
    //     },
    // ))
    // .unwrap();

    app.instantiate_contract(code_id, owner_addr, &init_msg, &[funds], "dojo", None)
        .unwrap();
}

Error:

Caused by:
    0: Overflow: Cannot Sub with 0 and 1000
    1: Cannot Sub with 0 and 1000'

Copy link
Contributor

@chipshort chipshort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long wait. Are you still interested in this?

@@ -147,6 +147,16 @@ impl ops::Sub<Coin> for NativeBalance {
fn sub(mut self, other: Coin) -> StdResult<Self> {
match self.find(&other.denom) {
Some((i, c)) => {
if c.amount < other.amount {
return Err(StdError::GenericErr {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more advisable to use the StdError::generic_err constructor than constructing the variant directly. This code would fail to compile with the cosmwasm-std backtraces feature enabled.

Comment on lines +169 to +171
return Err(StdError::GenericErr {
msg: format!("Insufficient balance. Wanted: {}, Available: 0", other.amount)
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small problem I see with this is that this makes it a bit more cumbersome to handle the error and makes an unnecessary allocation in that case.
With this change, you have to do something like:

if let Err(StdError::GenericErr { msg, .. }) = result {
    if msg.starts_with("Insufficient balance") {
        // handle
    }
}

as opposed to the current:

if let Err(StdError::Overflow { .. }) = result {
    // handle
}

But I think that's a bit of a niche case, so I am fine with this tradeoff for now.
Ideally, this would have it's own error type that converts into StdError (but that's a breaking change).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants