Defensive ops API design

[benluelo]

The current API design for the DefensiveSaturating trait isn't great - it's unnecessarily monolithic. It should be split into several smaller traits, and then DefensiveSaturating can be retained as a super trait so this won't be a breaking change.

I propose DefensiveSaturating{Add,Sub,Mul}, and then DefensiveSaturatingInc as a super trait of DefensiveSaturatingAdd + One and DefensiveSaturatingDec as a super trait of DefensiveSaturatingSub + One. This will allow users more granular control over the functionality they need, and removes this issue: https://github.com/paritytech/substrate/blob/master/frame/support/src/traits/misc.rs#L367-L368 (why does a type need to me Mul to use defensive_saturating_sub?)

I'm happy to implement this if it's accepted 🙂

[kianenigma]

Please, go ahead :)

[benluelo]

So after some experimentation, I've hit the following blockers:

1. DefensiveSaturating's blanket impl depends on Saturating, and Saturating has the same API issues as mentioned in this issue.

2. sp_arithmetic::Saturating methods take self (and other, where applicable) by value, and num_traits::Saturating{Add, Sub, Mul} take them by reference. The original num_traits::Saturating trait, (which I'm assuming sp_arithmetic::Saturating was originally based off of), takes it's parameters by value, but it's been pseudo-deprecated (not sure why it's not fully deprecated, see Officially deprecate Saturating rust-num/num-traits#255). To attempt to work around this (since all uses of sp_arithmetic::Saturating take their arguments by value), I created Saturating{Add, Sub, Mul} traits mirroring the current implementation, but that caused more issues than it solved (name collisions and ambiguous method resolution, oh boy!)

3. Super-traits are a bit of a pain. See this playground link for a full example of the issue: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=134e923ebec434a4996a6934e8274a9c

TLDR is that this can't be done in a backwards-incompatible way (afaict on a Friday night 😛). If I'm missing something, please let me know! Otherwise, if this refactor is deemed worthy of a breaking change (which IMO it is, lest these traits get even larger and more unwieldy) then I can open a PR with the changes and relevant fixes. (mostly just replacing sp_arithmetic::traits::Saturating with the required Saturating* traits, and adding a bunch of &)

[burdges]

Arithmetic crates like num_traits should support big nums, so they should pass by reference. You might as well pass by value if you're just trying to be even more precise than regular Rust about how you handle u64s.

[benluelo]

Arithmetic crates like num_traits should support big nums, so they should pass by reference. You might as well pass by value if you're just trying to be even more precise than regular Rust about how you handle u64s.

IMO, the traits should take their parameters by reference, since the traits should be usable by types that aren't Copy. There are currently a few structs within substrate that impl Saturating (that also just so happen to be Copy):

• https://github.com/paritytech/substrate/blob/fdff2b23009f25dd42d3a12fee6c998864399de3/frame/democracy/src/types.rs#L50
• https://github.com/paritytech/substrate/blob/fdff2b23009f25dd42d3a12fee6c998864399de3/frame/conviction-voting/src/types.rs#L230
  but I don't think that take by value should be enforced.