-
Notifications
You must be signed in to change notification settings - Fork 3
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
use the unsigned fixed type for balances: U64F64
#353
Conversation
if let Some(i) = nominal_income { | ||
validate_nominal_income(&i).map_err(|_| <Error<T>>::InvalidNominalIncome)?; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate only validated if the value is negative; type safety gives us this guarantee now already.
@@ -215,26 +218,26 @@ mod tests { | |||
|
|||
#[test] | |||
fn demurrage_works() { | |||
let bal = BalanceEntry::<u32>::new(1.into(), 0); | |||
let bal = BalanceEntry::<u32>::new(1u32.into(), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implicit type is i32, which can obviously not be into
-ed.
primitives/src/balances.rs
Outdated
return None | ||
} | ||
|
||
Some(U64F64::from_le_bytes(source.to_le_bytes())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this because byte order is not what makes this work and it confused me. what you do could be done with scale encode and decode and it would be at least obvious that it is a hacky typecast
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the LE is confusing, I have made it more trivial and just use to_bits
and from_bits
(much better) and also made a remark about the conversion/
I don't think it is good to to use the scale codec because it is much more inefficient than using the #[inline]
bit conversions that operate on a stack instead of on the heap.
With the from_bits
and to_bits
conversions, I don't even consider this hacky anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice cleanup. only one hack I don't like ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.