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

Proposal: Defensive trait for infallible frame operations #10626

Merged
paritytech-processbot[bot] merged 17 commits into
masterfrom
kiz-defensive-trait
Jan 24, 2022
Merged

Proposal: Defensive trait for infallible frame operations #10626
paritytech-processbot[bot] merged 17 commits into
masterfrom
kiz-defensive-trait

Conversation

@kianenigma
Copy link
Copy Markdown
Contributor

@kianenigma kianenigma commented Jan 10, 2022

I've had this idea for a while and we've also talked about it internally.

Most ideas pointed to a rather more complicated implementation where you would also flip some storage item that could easily be monitored if any of these operations happened.

This is my quicker and simpler version, to begin with.

I am marking this as draft, as I am not sure if the direction makes all stake holders happy or not: @paritytech/frame-coders. If we want this, then I will scan the runtime and find use cases. I have already counted a few dozens of instances where the defensive-arithmetic could be used.

@github-actions github-actions Bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 10, 2022
@apopiak
Copy link
Copy Markdown
Contributor

apopiak commented Jan 10, 2022

I've thought about something similar a few times, really like this idea! 👍
I've also thought that it could be introduced as a macro and then we could capture the surrounding context (line number etc.).

@emostov
Copy link
Copy Markdown
Contributor

emostov commented Jan 10, 2022

This makes sense to me and I think it would improve ergonomics, at least with the style of code I have gotten used to writing the last few months.

@kianenigma kianenigma marked this pull request as ready for review January 13, 2022 07:08
@kianenigma kianenigma added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 13, 2022
emostov
emostov reviewed Jan 17, 2022
View reviewed changes
Comment thread frame/support/src/traits/misc.rs Outdated
emostov
emostov reviewed Jan 17, 2022
View reviewed changes
Comment thread frame/support/src/traits/misc.rs Outdated
emostov
emostov reviewed Jan 17, 2022
View reviewed changes
Comment thread frame/support/src/traits/misc.rs
shawntabrizi
shawntabrizi approved these changes Jan 19, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me, and I would have a hard time seeing how this is something we would not want.

KiChjang
KiChjang approved these changes Jan 19, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@KiChjang KiChjang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit unsure about the naming (what does defensive really mean in such a context? conservative?) but the idea makes sense.

ggwpez
ggwpez reviewed Jan 19, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool @kianenigma !
Looking forward to using these functions.

Comment thread frame/support/src/traits/misc.rs Outdated
Comment thread frame/support/src/traits/misc.rs Outdated
Comment thread frame/support/src/traits/misc.rs Outdated
Comment thread frame/staking/src/pallet/impls.rs
Comment thread frame/support/src/traits/misc.rs
@ggwpez
Copy link
Copy Markdown
Member

ggwpez commented Jan 19, 2022

Bit unsure about the naming (what does defensive really mean in such a context? conservative?) but the idea makes sense.

I wondered the same. assert is often used in other languages.

emostov
emostov reviewed Jan 19, 2022
View reviewed changes
Comment thread frame/support/src/traits/misc.rs Outdated
kianenigma
kianenigma commented Jan 20, 2022
View reviewed changes
Comment thread frame/identity/src/benchmarking.rs
@kianenigma kianenigma added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Jan 20, 2022
ggwpez
ggwpez reviewed Jan 20, 2022
View reviewed changes
Comment thread frame/support/src/traits/misc.rs
@jakoblell jakoblell added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jan 24, 2022
@kianenigma
Copy link
Copy Markdown
Contributor Author

kianenigma commented Jan 24, 2022

bot merge

@paritytech-processbot paritytech-processbot Bot merged commit b0c1475 into master Jan 24, 2022
@paritytech-processbot paritytech-processbot Bot deleted the kiz-defensive-trait branch January 24, 2022 16:18
@kianenigma
Copy link
Copy Markdown
Contributor Author

kianenigma commented Jan 24, 2022

bot merge

@paritytech-processbot
Copy link
Copy Markdown

paritytech-processbot Bot commented Jan 24, 2022

Error: Github API says #10626 is not mergeable

@kianenigma
Copy link
Copy Markdown
Contributor Author

kianenigma commented Jan 24, 2022

bot merge

@paritytech-processbot
Copy link
Copy Markdown

paritytech-processbot Bot commented Jan 24, 2022

Error: Github API says #10626 is not mergeable

@KiChjang
Copy link
Copy Markdown
Contributor

KiChjang commented Jan 24, 2022

Leave it be; @paritytech-processbot is stubborn.

@kianenigma
Copy link
Copy Markdown
Contributor Author

kianenigma commented Jan 24, 2022

main reason is that I am on a weak Wifi and a consequence of that seems to be github send my comments multiple times :/

@xlc xlc mentioned this pull request Jan 25, 2022
Open
eskimor pushed a commit that referenced this pull request Jan 27, 2022
@kianenigma @ggwpez @eskimor
Proposal: Defensive trait for infallible frame operations (#10626)
156440c 
* add a blueprint of how defensive traits could look like

* add something for arithmetic as well

* add some use cases in different pallets

* some build fixing

* Some new stuff and examples

* Fix deadly bug

* add more doc.

* undo faulty change to assets pallet

* Update frame/support/src/traits/misc.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* some review comments

* remove draft comment

* Fix ident test

* Fix proxy tests as well

* a few new ideas

* Fix build

* Fix doc

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Wizdave97 pushed a commit to ComposableFi/substrate that referenced this pull request Feb 4, 2022
@kianenigma @ggwpez @Wizdave97
Proposal: Defensive trait for infallible frame operations (paritytech…
ca23f55 
…#10626)

* add a blueprint of how defensive traits could look like

* add something for arithmetic as well

* add some use cases in different pallets

* some build fixing

* Some new stuff and examples

* Fix deadly bug

* add more doc.

* undo faulty change to assets pallet

* Update frame/support/src/traits/misc.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* some review comments

* remove draft comment

* Fix ident test

* Fix proxy tests as well

* a few new ideas

* Fix build

* Fix doc

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@sorpaas sorpaas mentioned this pull request Feb 15, 2022
Merged
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
@kianenigma @ggwpez @grishasobol
Proposal: Defensive trait for infallible frame operations (paritytech…
4e622ec 
…#10626)

* add a blueprint of how defensive traits could look like

* add something for arithmetic as well

* add some use cases in different pallets

* some build fixing

* Some new stuff and examples

* Fix deadly bug

* add more doc.

* undo faulty change to assets pallet

* Update frame/support/src/traits/misc.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* some review comments

* remove draft comment

* Fix ident test

* Fix proxy tests as well

* a few new ideas

* Fix build

* Fix doc

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@kianenigma @ggwpez @ark0f
Proposal: Defensive trait for infallible frame operations (paritytech…
2eb5936 
…#10626)

* add a blueprint of how defensive traits could look like

* add something for arithmetic as well

* add some use cases in different pallets

* some build fixing

* Some new stuff and examples

* Fix deadly bug

* add more doc.

* undo faulty change to assets pallet

* Update frame/support/src/traits/misc.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* some review comments

* remove draft comment

* Fix ident test

* Fix proxy tests as well

* a few new ideas

* Fix build

* Fix doc

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@gavofyork gavofyork Awaiting requested review from gavofyork

@bkchr bkchr Awaiting requested review from bkchr

4 more reviewers

@ggwpez ggwpez ggwpez approved these changes

@emostov emostov emostov left review comments

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@KiChjang KiChjang KiChjang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@kianenigma @apopiak @emostov @ggwpez @KiChjang @shawntabrizi @jakoblell