Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add ensure_ops methods (as checked_ops but returning ArithmeticError instead) #12754

Closed
lemunozm opened this issue Nov 21, 2022 · 6 comments · Fixed by #12967
Closed

Add ensure_ops methods (as checked_ops but returning ArithmeticError instead) #12754

lemunozm opened this issue Nov 21, 2022 · 6 comments · Fixed by #12967
Labels
J0-enhancement An additional feature request. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.

Comments

@lemunozm
Copy link
Contributor

Hi! Because of the high safety standards needed in these contexts, mathematical operations tend to be made using the checked version. The checked version follows the same pattern in most cases:

a.checked_<operation>(b).ok_or(ArithmeticError::<Variant>)?

This has two issues:

  • Your algorithm becomes more difficult to read. When the code tends to be mathematical, readability is even more important. If it's difficult to read, it's very easy to have human errors and it makes auditories more difficult to do.
  • The concrete ArithmeticError returned is not easy to know. Depending on the value, it can differ. For example, a - b, could give Underflow, but also Overflow if b is a huge negative value.

In our parachain we've resolved both issues by creating an ensure_<operation> method family that reduces the code boilerplate and returns the correct ArithmeticError instead of an Option. The previous line could be rewritten in a more readable way:

a.ensure_<operation>(b)?

I think this can be useful for many other projects, so my intention is to move our ensure module (docs here) to sp_runtime::traits::ensure or any better place you recommend if you agree on this addition.

Thanks!

@github-actions github-actions bot added the J2-unconfirmed Issue might be valid, but it’s not yet known. label Nov 21, 2022
@bkchr
Copy link
Member

bkchr commented Nov 21, 2022

Sounds reasonable to me.

WDYT @kianenigma?

@lemunozm
Copy link
Contributor Author

Hi @bkchr, @kianenigma, any updates about it?

@ggwpez ggwpez added J0-enhancement An additional feature request. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. and removed J2-unconfirmed Issue might be valid, but it’s not yet known. labels Dec 15, 2022
@ggwpez
Copy link
Member

ggwpez commented Dec 15, 2022

Idea looks good, but no update until someone wants to work on it 😛

@bkchr
Copy link
Member

bkchr commented Dec 15, 2022

Yes! @lemunozm can you provide some pr?

@lemunozm
Copy link
Contributor Author

Sure! I'll do it soon

@Polkadot-Forum
Copy link

This issue 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

trusch added a commit to KILTprotocol/kilt-node that referenced this issue Jan 23, 2023
## fixes KILTProtocol/ticket#2392

## Breaking Changes for us

~~None! 🥳~~

Edit: Forgot to also check with try-runtime feature enabled. There is a
small tweak necessary because of [This PR about
on-runtime-upgrade](paritytech/substrate#13045)

No database migrations, no runtime migrations and no new host functions.

## Polkadot Release Link

https://github.com/paritytech/polkadot/releases/tag/v0.9.37

## Release Analysis Forum Post

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

## Cool new stuff that might be useful (or not)

* [frame_support::storage: Add
StorageStreamIter](paritytech/substrate#12721)
* If we have a StorageValue that contains something iterable, we can
directly iterate over it, without copying the memory first by a regular
get() call.
* [Add ensure_* mathematical
methods](paritytech/substrate#12754)
* The checked_* family of calls returns an Option which is in 99% of the
cases mapped to an error
* ensure_* calls directly return an error which can be propagated using
questionmark operator more easily
* [Kusama shows how to express complex local origins in XCM
messages](paritytech/polkadot#6273)
* Perhaps the most interesting one in this release, would be a good idea
for @weichweich and @ntn-x2 to have a look into this
* [pallet_uniques successor NFTv2 is out! 🥳 😄
](paritytech/substrate#12765)
* Finally we can have NFTs with owner controlled metadata on our chain.
* They even literally mention that this way users can write DIDs
directly on their NFT!
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
J0-enhancement An additional feature request. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants