Create cosmwasm-math #1186
Comments
I have another proposal. Use num-traits crate for that, so Also consider having all operations defined for native equivalent Also I have another idea which might be part of this. Something I called the "safemath". Basically create the type, let call it Why such thing? Consider: let reg = a.checked_mul(b)?.checked_add(c.checked_mull(d)?)?;Kind of difficult to read, and this is only the linear regression. Now with "safemath": let reg = (c.checked() * b + c.checked() * d).native()?;Shorter, but what is more important - more natural (basically some Another step further may be having similar wrappers defaulting to |
Yes, definitely! I'd suggest something like: #[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub struct Number {
value: Decimal256, // or Uint256
sign: NumberSign,
}
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum NumberSign {
Positive,
Negative
}I implemented something like this with all the ops, cmp implementations you'd expect, as well as helpers like .invert_sign()... very happy to ditch it in favor of a PR here when the building blocks are in place :) |
I made bad experience with the num-traits crate in the past becasue it pulls in float operations. Using generics, hmm, maybe. We only need Uint64 and Uint128. Then for Uint256 and Uint512 we need a separate implementation anyways. How would documentation look like? Everything should be rolled out completely for the specific types. The users should not see the extra abstraction. |
I had some success with |
|
I just had a talk with friend and it turns out decimals are not so trivial, and common rust crates actually makes it wrong (it has overhead of computational error). There is very strong and heavily documented C library doing it properly - https://github.com/dnotq/decNumber. Probably we can use it as example how to do it properly. |
That's a pretty heavy statement... can you be more specific with an example of an arithmetic error? |
|
@hashedone - if you want to discuss something, please quote and reply. Do not edit the history of other people's comments!!! |
|
To address what you said in regard to #1186 (comment):
I am not sure what you mean by u2 vs. u1... but assuming you mean to pack it in the available 256 bits instead of storing the sign flag separately:
|
This talks about "Decimal Floating Point", which is hard to get right for sure. But we don't need floating point types. We can simply use fixed point decimal types, which makes all the things very easy because those are just integers with a fixed decimal point. Most operations are implemented using integer arithmetic. |
|
@webmaster128 - it would be really nice to also have npm wasm-packed lib that exported a typescript interface. Ideally a full class wrapper around the structs with all the methods, but for starters even pure functions could work. That way frontend/cli code could work with the full Decimal256 and do math on the frontend, without having to worry about floating point errors or compatibility issues from some other third-party JS fixed-point lib Of course even if this idea is accepted, it's low-priority and depends on the main crate existing first |
You can use |
|
I see.. and that is great, thanks! That's even better, suggestion could just be a drop-in replacement to that package, maybe even without breaking anything much downstream, i.e. to use the underlying wasm and just pass strings around directly, instead of going through bn.js. Would also make it easier to implement more methods like abs(), pow(), etc. |
|
I think what you are bringing up there (compiling cosmwasm-math to a client lib) is a valid and valuable aproach. It will just not happy anytime soon due to reasource limits and priorities. Compiling Wasm for JS environments is not as simple as it sounds. Embedding them in a way that works for Node, bundlers and the web requires you to inline the Wasm as text in .js files. When it comes to bundling Wasm as part of JS libraries it gets very tricky. In many places we work towards getting rid of Wasm dependencies because they create big bundles and you cannot optimize for bundle size using tree shaking when you have Wasm. |
|
Fair enough, though this crate, isolate from cosmwasm_std, should make the bundle pretty small! Happy to lend a hand when the time is right (I have done a fair amount of browser-targetted wasm dev, e.g. https://github.com/dakom/awsm-web) |
|
I added wrapping_add/wrapping_sub/wrapping_mul/wrapping_pow to the list which are only implemented for Uint64 and Uint128. |
|
how about an EPSILON const and |
|
Seems like the Machine epsilon is well defined for decimal types, so we could add it. But what would be the use case exactly? Should |
|
yeah, I think 1 difference is the right thing to do here, i.e. |
|
|
|
anecdotal, but I usually see ( |
This might be the case for floats. But Decimals have 18 digits and there is no value between 1.987654321987654321 and 1.987654321987654322. You have epsilon steps between all decimals. |
|
Hmm... okay, so maybe this shouldn't be part of the type, and we'll just use extension traits if we need Fwiw the use case I've run into might be due to losing some precision when going between, say, Uint128 and Uint256, for example. |
|
More wrapping stuff here: #1459 (comment) |
|
Any updates here on progress towards SignedDecimals and SignedInts? I'm currently relying on a custom built type for these things (internally using a bool), but it would be really nice to have official support for this. |
|
Thanks for the nudge, @ewoolsey. Could you share your version to help us designing the API we need? I wonder how to best implement this. There can be an enum with Negative and NonNegative cases or we build it bitwise on top of a Uint* storage. The first one gives us a +0 and -0, which can be annoying. |
Yeah I'd love to share it! I'll create a prototype repo and you can have a look! I found using an enum a little clunky so chose to use a struct with a bool instead. I consider what I currently have to be "quick and dirty" but it's passing all the tests I've written for it. My gut says that the correct way to do this is representing the sign as the most significant bit. That's definitely the most work, but probably produces the best performance. |
|
Thank you! This is the part I worry about: impl std::cmp::PartialEq for SignedInt {
fn eq(&self, other: &Self) -> bool {
self.value == other.value && self.is_positive == other.is_positive
}
}which leads to |
|
So in this prototype I attempt to forbid -0. It should be impossible to ever get that value. There are some tests at the bottom that confirm this as well. (ps please pull latest I added some updates there) |
|
The remaining issues can be answered as follows.
Have been extracted to #1710 and #1711.
Is not going to happen because sometimes we have different error cases. In cosmwasm-std 2.0 we'll make sure to make math errors cheap to create and handle (i.e. no String allocations).
Maybe, one day, let's see.
This is not planned anymore. Having everything here and cleaning up on major version bumps should be more practical. Also some cosmwasm-std types like Thanks everyone |
I think having a (relatively) lightweight independent math library would be a nice thing. By example, for packages / crates that need numerical types, and don't need / use the CosmWasm API.
Just make |
This might work. I was initially thinking the other way around (cosmwasm-math being an opt-in library for contract developers). What would be the use case for such a split? More crates leads to more maintenance cost so we better have a good reason. |
As mentioned above, packages and modules that require math types / utils. I recently needed an Not a big deal, because that package will end up being used in contracts development anyway, but it felt inelegant, and a little overkill to add the entire CosmWasm standard library just to have access to |
|
Let's continue this discussion in #1956 |
This is a new package with the following goals:
TODOs:
Sumfor all types consistently (Use the same Sum implementation for all number types #1187)checked_multiply_ratioreturning anOptionallowing to recover from overflowchecked_*functions returnsOptioninstead ofStdResultfor better stdard library complience and avoiding performance hit (see Mark more Uint128, Uint64 and Decimal operations as const #1157).constify all the thingsUint64/Uin128to the primitivesu64andu128. Are we happy with the current API?cosmwasm_std:{Decimal, Decimal256, Uint128, Uint256, Uint512, Uint64}and point the users to the math libraryconstconstructorThe text was updated successfully, but these errors were encountered: