openethereum / sol-rs

Solaris - Solidity testing framework in Rust.
GNU General Public License v3.0
54 stars 14 forks source link

improving sol::unit #17

Closed snd closed 6 years ago

snd commented 6 years ago

there are some unit conversion functions in sol::unit: https://github.com/paritytech/sol-rs/blob/aed4bb377db6bf7459e5ee3792e1d7cddfb45cef/solaris/src/unit.rs

i have some thoughts on improving them:

alternatively we could model it similar to this:

enum Unit {
    Wei,
    Ether
        // ...
}

impl Unit {
    pub fn to_wei(&self, value_in_unit: U256) -> U256 {
        match *self {
            Unit::Wei => value_in_unit,
            Unit::Ether => value_in_unit * 1000000000000000000u64.into(),
                        // ...
        }
    }
}

let value = Unit::Ether.to_wei(200.into());

would love some feedback

tomusdrw commented 6 years ago

Existence of this functions is related to #6 - ethabi doesn't use proper numeric types, so we need to do conversions ourselves.

  1. *_to_wei is pretty verbose
  2. I would avoid trait since it has to be imported.
  3. What do you think about:
    sol::Wei::from_ether(5)

    instead?

snd commented 6 years ago

I would avoid trait since it has to be imported.

which trait are you referring to?

sol::Wei::from_ether(5)

i like it. it's understandable without additional context which ether(5) isn't imho. sol::unit::ether_to_wei(5) seems similarly verbose though. maybe sol::from_ether(5)? makes clear that it's converting from ether and one can guess that it's converting to wei.

tomusdrw commented 6 years ago

which trait are you referring to?

Sorry for some reason I interpreted your enum proposal as trait.

I'd prefer to keep the wei and not pollute sol interface:

sol::wei::from_ether(5);
// or 
use std::wei;
wei::from_ether(5);
snd commented 6 years ago

i really like that. can i go ahead and implement this? are you ok with the from_* functions taking a T: Into<U256>. then one can pass in byte arrays, u64, etc directly.

tomusdrw commented 6 years ago

Sure, go ahead.

snd commented 6 years ago

resolved in https://github.com/paritytech/sol-rs/pull/18