Add ensure-ops family methods #12967

lemunozm · 2022-12-19T10:35:30Z

This PR adds the following ensure-ops methods for safe arithmetic operations:

ensure_add()
ensure_sub()
ensure_mul()
ensure_div()
ensure_add_assign()
ensure_sub_assign()
ensure_mul_assign()
ensure_div-assign()
ensure_from_rational() for FixedPointNumber
ensure_mul_int() for FixedPointNumber
ensure_div_int() for FixedPointNumber
ensure_from()
ensure_into()

Checks before accepting this PR:

Is sp_runtime::traits the correct place for this?
In order to use any of these traits, the user should import, for example, sp_runtime::traits::ensure::EnsureAdd. If this is too long, it can be shortcutted to sp_runtime::traits::EnsureAdd

This PR does't not break any current API.

Fixes #12754

cla-bot-2021 · 2022-12-19T10:35:36Z

User @lemunozm, please sign the CLA here.

ggwpez · 2022-12-19T11:07:25Z

Is sp_runtime::traits the correct place for this?

IMO rather sp_primitives::arithmetic::traits. Second opinion?

In order to use any of these traits, the user should import, for example, sp_runtime::traits::ensure::EnsureAdd. If this is too long, it can be shortcutted to sp_runtime::traits::EnsureAdd

You can also create super-traits like Ensure: EnsureAdd + EnsureSub … and the same for EnsureAssign to make it a bit less verbose, in the case that multiple are needed.

lemunozm · 2022-12-19T11:15:57Z

IMO rather sp_primitives::arithmetic::traits. Second opinion?

That should be theoretically the best place, but the methods return ArithmeticError that is defined in the runtime. I suppose we don't want to have an arithmetic -> runtime dependency for this.

You can also create super-traits like Ensure: EnsureAdd + EnsureSub … and the same for EnsureAssign to make it a bit less verbose, in the case that multiple are needed.

Sure! I like the idea

bkchr · 2022-12-19T19:13:15Z

Can we not move all of this into sp-arithmetic? Maybe we could move the ArithmeticError to sp-arithmetic as well?

lemunozm · 2022-12-19T19:23:57Z

It could be a possibility 👍🏻 . Makes sense to have this error there. If the runtime reexports the error, it should not be a breaking change.

If you agree, I can do this refactor.

bkchr · 2022-12-19T22:51:56Z

Sounds good 👍

lemunozm · 2022-12-20T10:28:04Z

I've finally reexported the ensure module to make it more accessible. Now you can import this as sp_arithmetics::traits::Ensure. Also, I've reexported the traits to sp-runtime following the same pattern as the CheckedOps traits. Please, give me double-check if this is what you expect.

primitives/arithmetic/src/traits.rs

melekes

👍 (code-wise; don't know enough to reason about business logic)

Co-authored-by: Bastian Köcher <[email protected]>

primitives/arithmetic/src/traits.rs

bkchr · 2022-12-28T10:16:11Z

bot merge

paritytech-processbot · 2022-12-28T10:16:19Z

Error: Statuses failed for 9ba9307

bkchr · 2022-12-28T10:20:33Z

/cmd queue -c fmt $ 1

command-bot · 2022-12-28T10:20:39Z

@bkchr https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2204938 was started for your command "$PIPELINE_SCRIPTS_DIR/fmt.sh" 1. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment /cmd cancel 149-39fefd7e-7a0f-4853-8780-ffbf40fbd59b to cancel this command or /cmd cancel to cancel all commands in this pull request.

command-bot · 2022-12-28T10:21:53Z

@bkchr Command "$PIPELINE_SCRIPTS_DIR/fmt.sh" 1 has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2204938 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2204938/artifacts/download.

bkchr · 2022-12-28T12:52:07Z

bot merge

Polkadot-Forum · 2023-01-20T07:22:45Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-37/1736/1

* add ensure-ops family methods * fix cargo doc * add EnsureOp and EnsureOpAssign meta traits * move ensure module and ArithmeticError to sp-arithmetic * fix doc examples * reexport ensure module content * ensure mod private * reexport to sp-runtime * fix doc example * remove into(). in doc examples, minor doc changes * remove return value from assign methods * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * Update primitives/arithmetic/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> * cargo fmt * Apply suggestions from code review * ".git/.scripts/fmt.sh" 1 Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: command-bot <>

ggwpez added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Dec 19, 2022

lemunozm force-pushed the lm-ensure-methods branch from 7de1c79 to c1cc41a Compare December 19, 2022 17:40

lemunozm force-pushed the lm-ensure-methods branch 2 times, most recently from 47df1eb to 2bc3f02 Compare December 20, 2022 08:36

ggwpez reviewed Dec 20, 2022

View reviewed changes

lemunozm requested a review from ggwpez December 21, 2022 17:12

ggwpez approved these changes Dec 21, 2022

View reviewed changes

ggwpez requested a review from bkchr December 21, 2022 17:57

melekes approved these changes Dec 22, 2022

View reviewed changes

lemunozm added 11 commits December 23, 2022 09:15

add ensure-ops family methods

5684541

fix cargo doc

4d52919

add EnsureOp and EnsureOpAssign meta traits

e6ac815

move ensure module and ArithmeticError to sp-arithmetic

9197649

fix doc examples

61f2fc7

reexport ensure module content

a3e9649

ensure mod private

36feea6

reexport to sp-runtime

43b8814

fix doc example

e4c1514

remove into(). in doc examples, minor doc changes

0a2ee21

remove return value from assign methods

f7b305c

lemunozm and others added 8 commits December 27, 2022 12:02

Update primitives/arithmetic/src/traits.rs

3640a6f

Co-authored-by: Bastian Köcher <[email protected]>

Update primitives/arithmetic/src/traits.rs

5f59e68

Co-authored-by: Bastian Köcher <[email protected]>

Update primitives/arithmetic/src/traits.rs

0e6a2a6

Co-authored-by: Bastian Köcher <[email protected]>

Update primitives/arithmetic/src/traits.rs

d44e76c

Co-authored-by: Bastian Köcher <[email protected]>

Update primitives/arithmetic/src/traits.rs

784a803

Co-authored-by: Bastian Köcher <[email protected]>

Update primitives/arithmetic/src/traits.rs

48e68c5

Co-authored-by: Bastian Köcher <[email protected]>

Update primitives/arithmetic/src/traits.rs

2a2590b

Co-authored-by: Bastian Köcher <[email protected]>

Update primitives/arithmetic/src/traits.rs

b23f70f

Co-authored-by: Bastian Köcher <[email protected]>

lemunozm requested a review from bkchr December 27, 2022 11:05

cargo fmt

b0219f5

bkchr reviewed Dec 28, 2022

View reviewed changes

primitives/arithmetic/src/traits.rs Outdated Show resolved Hide resolved

primitives/arithmetic/src/traits.rs Outdated Show resolved Hide resolved

bkchr reviewed Dec 28, 2022

View reviewed changes

primitives/arithmetic/src/traits.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

9ba9307

".git/.scripts/fmt.sh" 1

048a34b

paritytech-processbot bot merged commit 043a60b into paritytech:master Dec 28, 2022

lemunozm deleted the lm-ensure-methods branch December 28, 2022 12:52

lemunozm mentioned this pull request Jan 2, 2023

Add ensure_pow method #13042

Merged

ggwpez mentioned this pull request Jan 2, 2023

Remove Copy from Ensure* traits #13043

Merged

librelois mentioned this pull request Jan 23, 2023

Update substrate/polkadot/cumulus from v0.9.32 to v0.9.37 moonbeam-foundation/moonbeam#2059

Closed

aurexav mentioned this pull request Feb 8, 2023

To polkadot-v0.9.37 darwinia-network/darwinia-2.0#267

Closed

kacperzuk-neti mentioned this pull request Feb 14, 2023

Polkadot v0.9.37 (pallet-nfts) liberland/liberland_substrate#244

Merged

Add ensure-ops family methods #12967

Add ensure-ops family methods #12967

Uh oh!

Conversation

lemunozm commented Dec 19, 2022

Uh oh!

cla-bot-2021 bot commented Dec 19, 2022

Uh oh!

ggwpez commented Dec 19, 2022

Uh oh!

lemunozm commented Dec 19, 2022

Uh oh!

bkchr commented Dec 19, 2022

Uh oh!

lemunozm commented Dec 19, 2022

Uh oh!

bkchr commented Dec 19, 2022

Uh oh!

lemunozm commented Dec 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkchr commented Dec 28, 2022

Uh oh!

paritytech-processbot bot commented Dec 28, 2022

Uh oh!

bkchr commented Dec 28, 2022

Uh oh!

command-bot bot commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

command-bot bot commented Dec 28, 2022

Uh oh!

bkchr commented Dec 28, 2022

Uh oh!

Polkadot-Forum commented Jan 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

command-bot bot commented Dec 28, 2022 •

edited

Loading