Skip to content

Address feedback #1

@mistadikay

Description

@mistadikay

First thing I noticed is mutable owner in instantiate - it is technically correct, but I really like to compose values instead of build them with mutation - here it is possible with the transpose function:

let owner = msg
  .owner
  .map(|owner| deps.api.addr_validate(&owner))
  .transpose()? // How to get rid of the internal error
  .unwrap_or(info.sender);

Also there are couple of redundant clones (eg line 51 of contract.rs) - you can easily trace those guys just running cargo clippy on your codebase.

A bit more important issue I see is creating a generic_err (line 57) - we sometimes do it if we are working on some POC or contract draft, but in general as you have the custom error type created, it is preferable to create another error variant for that.

In line 65 I see you nest ifs. Obviously you cannot just && condition with if let, but here is a perfect opportunity to use the matches! macro, as you never use deconstructed value: matches!(&state.max_bid, Some(max_bid) if total_bid < max_bid.1).

Also this whole if is just an ensure! macro from cosmwasm_std - I really like to reduce this kind of calls into:

ensure!(
  !matches!(&state.max_bid, Some(max_bid) if total_bid < max_bid.1),
  ContractError::BidTooSmall,
);

I see you are using Uint128 to store the commission rate (as percentage). It is correct, but it might be useful to check out the cosmwasm_std::Decimal type which is designed for such cases.

Another nitpick - in tests you use assert_eq!(something, true) - this is equivalent to just assert!(something) - clippy also complains on that.

Besides that - good solution, what was mostly expected. Really like the test, attention to details is visible there.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions