Feature request: a cosmwasm_std::Coins class similar to sdk.Coins

[larry0x]

protocols often need to handle multiple coins at the same time. for SDK modules, this is easy to do thanks to the sdk.Coins class, which provides a number of useful helper methods. however i am not aware of something similar in cosmwasm_std.

i propose to create a cosmwasm_std::Coins class with methods that mirror those of sdk.Coins:

struct Coins(Vec<Coin>);

impl Coins {
    fn from_str(&str) -> Result<Self, ParseError>;
    fn to_string(&self) -> String;
    fn denoms(&self) -> HashSet<String>;
    fn add(&mut self, coin: &Coin);
    fn add_many(&mut self, coins: &[Coin]);
    fn sub(&mut self, coin: &Coin) -> Result<(), OverflowError>;
    fn sub_many(&mut self, coins: &[Coin]) -> Result<(), OverflowError>;
    fn mul_int(&mut self, x u128);
    fn quo_int(&mut self, x u128);
    fn max(&self, coins &[Coin]) -> Coins;
    fn min(&self, coins &[Coin]) -> Coins;
    fn is_all_gt(&self, coins &[Coin]) -> bool;
    fn is_all_lt(&self, coins &[Coin]) -> bool;
    fn is_equal(&self, coins &[Coin]) -> bool;
    fn amount_of(&self, denom &str) -> Uint128;
    fn find(&self, denom &str) -> Option<&Coin>;
    // and more...
}

i have worked on the cw_asset library, which provides a similar: AssetList class: https://github.com/mars-protocol/cw-asset/blob/main/src/asset_list.rs

this new class can be added without breaking the existing API.

it would be great if the funds field in MessageInfo is also changed to Coins instead Coin, but then it would be API-breaking.

[larry0x]

on a separate note, it may also be interesting to have a DecCoin class as well:

struct DecCoin {
    pub denom: String,
    pub amount: Decimal,
}

[webmaster128]

This also came up recently in #1359.

The main issue I have with this is that Coins is a list, which makes very little sense is very questionable. It creates those situations where there are good and bad instances, clean and dirty instances. See func sanitizeCoins(coins []Coin) Coins { and func (coins Coins) Validate() error { in Cosmos SDK.

It would be good to implement Coins in a type that makes dirty instances impossible, like a map from denom to uint. Sorting can happen whenever you roll out the values.

[larry0x]

This also came up recently in #1359.

The main issue I have with this is that Coins is a list, which makes very little sense is very questionable. It creates those situations where there are good and bad instances, clean and dirty instances. See func sanitizeCoins(coins []Coin) Coins { and func (coins Coins) Validate() error { in Cosmos SDK.

It would be good to implement Coins in a type that makes dirty instances impossible, like a map from denom to uint. Sorting can happen whenever you roll out the values.

in our cw_asset::AssetList, we purge zero amounts every time deduct (equivalent to sub) is called: https://github.com/mars-protocol/cw-asset/blob/v2.2.0/src/asset_list.rs#L392

similarly, we can also do sorting every time add is called.

but the map idea makes sense as well, and is perhaps cheaper in gas. i'm wondering if Rust has something similar to Python's OrderedDict data structure

[webmaster128]

Looks good, but it can be implemented easier with struct Coins(HashMap<String, Uint128>);. Then add suddenly becomes something like

fn add(&mut self, rhs: Coin) {
    *self.0.entry(rhs.denom).or_default() += rhs.amount;
}

The purging only needs to be done when constructing the instance (where you loop through inputs anyways) and in deduct you only need to worry about the denom you are deducting.

[larry0x]

Looks good, but it can be implemented easier with struct Coins(HashMap<String, Uint128>);. Then add suddenly becomes something like

fn add(&mut self, rhs: Coin) {

    *self.0.entry(rhs.denom).or_default() += rhs.amount;

}

The purging only needs to be done when constructing the instance (where you loop through inputs anyways) and in deduct you only need to worry about the denom you are deducting.

Yeah, i'm also thinking about using a BTreeMap which is naturally sorted by denom. It's more expensive compared to HashMap when doing searches or insertions, but the difference should be acceptable since contracts usually only deal with small numbers of coins.

I'm going to create a separate repo to play with the idea before contributing here.

[webmaster128]

Yeah I think BTreeMap makes sense.

Happy to see and review a draft PR!

[webmaster128]

it would be great if the funds field in MessageInfo is also changed to Coins instead Coin, but then it would be API-breaking.

Having an From<Vec<Coin>> for Coins implementation mitigates this. Having Vec<Coin> for JSON interfaces and Coins for advanced operations with simple converters between the two is probably nice.

[larry0x]

hi @webmaster128, i have been playing with the BTreeMap implementation of Coins, and certainly it has many good properties. however, there are two issues related serialization, which i want to learn about your thoughts.

my Coins type is defined as follows, mapping each denom to amount:

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
pub struct Coins(pub BTreeMap<String, Uint128>);

1. serialization into JSON

while Rust vector types serializes to JSON arrays, e.g.

// typescript definition of Vec<cosmwasm_std::Coin>
type VecCoin = {
  denom: string;
  amount: string;
}[];

map types such as BTreeMap serialize to JSON objects:

// typescript for BTreeMap<String, Uint128>
type BtreemapStringUint128 = {
  [key: string]: string;
};

i'm not sure if this is desired. on the one hand, it is a more compact representation (shorter JSON strings, so cheaper gas when writing to contract store?) on the other hand, it 1) losses the ordering of keys, 2) may cause confusions for developers.

i can certainly manually implement the Serialize/DeserializeOwned traits, instead of deriving them, such that Coins is serialized into JSON arrays like Vec<Coin>. thoughts?

2. if the string has a duplicate key, deserialization will NOT throw an error

if i attempt to deserialize the following JSON string:

let s = r#"{
    "uatom": "12345",
    "factory/osmo1234abcd/subdenom": "88888",
    "uatom": "67890",
    "ibc/1234ABCD": "69420"
}"#;
let coins = serde_json::from_str::<Coins>(s).unwrap(); // success, no error

the JSON string contains a duplicate denom uatom. ideally we want this to fail, with an error indicating there is a duplicate. however, the actual behavior is that there's no error, the Coins object being produced has an uatom amount of 67890 (the value seen later overwrites the value seen earlier).

turns out JSON's syntax does allow duplicate keys... i did not know this.

the same when casting from Vec<Coin>:

let vec = vec![
    coin(12345, "uatom"),
    coin(88888, "factory/osmo1234abcd/subdenom"),
    coin(67890, "uatom"),
    coin(69420, "ibc/1234ABCD"),
];
let coins = vec
    .into_iter()
    .map(|coin| (coin.denom, coin.amount))
    .collect::<Coins>();

the result is the same as in the JSON example: no error, the uatom amount is 67890.

i think this can be solved by:

• for JSON, manually implement the serde traits, as discussed above
• for Vec<Coin>, implement TryFrom instead of From, throw error if there're dups

thoughts?

[larry0x]

UPDATE: i implemented the custom Deserialize trait here: mars-protocol/cw-coins@e0d6924

it still uses the { [key: string]: string } JSON format, but will throw an error if the JSON string contains duplicate keys.

[ethanfrey]

I think serialising this format doesn't make sense. It should only be used as an in-memory helper.

I would have simple TryFrom<Vec<Coin>> (which errors if this input is invalid) and Into<Vec<Coin>> implementations (which returns a valid, sorted list)

And just convert in memory when needed, and convert back to a Vec for serialization.

If you want to avoid that (coding) overhead, I would implement the serialize and deserialise as simple wrappers: Basically, deserialise into Vec<Coin> then convert to Coins. Which could then later be optimised if needed as it is hidden in the implementation.

[uint]

Hey, @larry0x! Are you still willing to go for this, or are you dropping it?

[larry0x]

Hey, @larry0x! Are you still willing to go for this, or are you dropping it?

@uint I created a proof-of-concept with serialization/deserialization (but without the arithmetic methods, yet) here: https://github.com/steak-enjoyers/cw-plus-plus/tree/main/packages/coins

I think this can remain as a 3rd party helper library instead of part of cosmwasm-std.

[uint]

Thanks, cool!

@ethanfrey Is this something we want to happen in cosmwasm-std, or are we happy?

[ethanfrey]

I am very happy with this as a 3rd party module.
If it is maintained module (and on crates.io), we should add it to cw-awesome.

[ValarDragon]

I'm very supportive of a Coins object that wraps Vec<Coin> being in cosmwasm_std, its pretty unclear how to interact with coins of multiple denoms. Thisfeels like a fairly standard need for contracts, or at least the contracts I'm working with all require this. Total_funds in deps.info itself is awkward because of this, you can't add it to anything!

At least getting the version thats recommended for doing arithmetic on Vec<Coins> into docs would be really helpful.

[webmaster128]

I agree with @ValarDragon (despite my general tendency to minimalism). This is also a low hanging fruit for us to develop and maintain.

I'd love to start the work based on @larry0x's code. Unfortunately I cannot do that due to licensing. Would you submit that file under Apache 2.0, Larry? Feel free to just add it as part of a draft PR and I can pick it up.

[larry0x]

I agree with @ValarDragon (despite my general tendency to minimalism). This is also a low hanging fruit for us to develop and maintain.

I'd love to start the work based on @larry0x's code. Unfortunately I cannot do that due to licensing. Would you submit that file under Apache 2.0, Larry? Feel free to just add it as part of a draft PR and I can pick it up.

I can make a draft PR, but a problem is that Coins is a wrapper of BTreeMap

pub struct Coins(BTreeMap<String, Uint128>);

but serde-json-wasm does not support deserializing into maps (https://github.com/CosmWasm/serde-json-wasm/blob/v0.4.1/src/de/mod.rs#L579-L587). Thoughts on this?

[webmaster128]

Thoughts on this?

Yes, we don't use serde for Coins. It can have a from Vec<Coin> constructor and a to Vec<Coin> exporter. This is the format we use to communicate with Cosmos SDK anyways.

[larry0x]

@webmaster128 draft PR ---> #1487