Make perthings operate in type operator#2501
Conversation
|
Can we add a test? |
|
I don't have strong opinion but I want to break the test saturating_mul for those reasons:
But could be confinced as:
|
|
Categorically agreeing on panicking on normal As for the test case, I would say it should go away over the normal |
|
well CheckedMul trait cannot be implemented because it is too restrictive (we don't want to multiply Perbills but Perbill with another type) The question is: to we want Mul implementation to do saturation or panic on overlfow. I don't know. |
|
implementation updated to be valid even for number near the maximum value allowed |
|
given this is exclusively for the runtime, and thus panicking can cause economic weaknesses (or worse), i think we want to saturate. |
|
ok so current implementation operates without loss and without overflow by seperating operation on quotient and remainder. This come with the cost of some more operation but nothing huge (just remainder operation in arithmetic of N, some division and some multiplication in arithmetic of N and u64). I think it is ready to review, we still want to implement operation for PerU128 but it is not as straightforward as we cannot do easily the remainder operation of b with u128::MAX because b type might not overset u128. |
|
I think the current new implementations are much better and should be kept. You can, for now, ignore PerU128; as you mentioned you cannot do the same with it atm. I will create an issue for my self and be done in the foreseeable future to re-think how PerU128 is used in staking and if it can be capped back to maybe Perquintill? At the time I made PerU128 be able to handle this: substrate/srml/staking/src/phragmen.rs Line 167 in 7e7192c where |
kianenigma
left a comment
There was a problem hiding this comment.
I tried adding some more test cases locally for the edge cases but the impl seems to okay.
Given that this eliminates the chance of overflow then there's also no more dilemma regarding saturating or panicking.
|
@kianenigma you can add new test to this PR if you want |
|
will wait for the additional test from @kianenigma |
|
|
|
||
| assert_eq!(Perbill::one() * 255_u64, 255); | ||
| // panics | ||
| assert_ne!(Perbill::one() * 255_u8, 255); |
There was a problem hiding this comment.
hmm yes I expect As<u64> to be a type that contains u64 which is false. maybe we should bound From<u32> as well so it can't panic.
There was a problem hiding this comment.
maybe we shouldn't bound As but only From<u32> and TryInto<u32> actually it would work and never panic: we convert to u32 the remainder which is less that a billion.
There was a problem hiding this comment.
the thing is also that From<u32> is not implemented for simple arithmetic nor balance generic. And bounding a new trait to Currency::Balance is not super handy. Though we will want this at some point when we get rid of As
There was a problem hiding this comment.
or we could also bound some stuff like Rem<u32, Output=N>
There was a problem hiding this comment.
All of the above additional bounds though would have to be enforced to Balance, right? if so then I would say we should leave it as it is (test can also stay as a demonstration), as I don't see this as really our concern.
These are utilities that we use and provide to substrate users as well. In our case, our balance types are actually big enough to work fine with this impl; And as for the users, this seems to be the most generally safe approach. There will always be anomalies, but in this case, it will be a balance type of u16 and u8 which is not really realistic.
What we could ultimately do to favor these group (or they can just do it themselves) is to create an actual struct percent(u8). In this case, all u16 and u8 balances will also have sane arithmetic (I haven't done this but it looks to work).
Unless if there is an obvious simple way that we don't see, I see the current code as okay.
There was a problem hiding this comment.
Noting that if, for example, if your balance is u8, the granularity of a per-thing being more than 1/255 is actually pointless as there is no way to reflect it on the actual data type.
There was a problem hiding this comment.
not really it can be bound on module side by having an associated type Balance: From<u32> and write Curreny associated type like Currency<Balance=Self::Balance> but it creates a new associated type which is not beautiful.
* perthings operate in type operator * implementation with rem * fmt * doc * better fmt * bump version * Tests for pet-things * demonstrate output as type of operation * Remove redundant assertions. * rename test * update lock * bump impl version
#2441
Done:
(b / bill) * part + ((b.rem(bill)) * part) / bill
first term operate in N arithmetic thus it is ensure to be OK, second term operate in u64 arithmetic but as remainder is less that a billion and billion*billion operation does fit in u64 it is OK.
TODO: