Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow for u8 to be used as hold/freeze reason #5348

Merged
merged 7 commits into from
Aug 15, 2024
Merged

Conversation

kianenigma
Copy link
Contributor

..without needing to provide your own newtype around it.

This will allow type Reason = u8 to be used as FreezeReason and HoldReason, which I think is a nice simplification if one doens't want to deal with the complications.

At the same time, it is a bit of an anti-pattern.

Putting it out there to check people's vibes.

@kianenigma kianenigma requested a review from a team as a code owner August 13, 2024 17:46
@bkchr
Copy link
Member

bkchr commented Aug 13, 2024

We have RuntimeHoldReason and RuntimeFreezeReason, why do people need to put there u8?

@kianenigma
Copy link
Contributor Author

kianenigma commented Aug 14, 2024

We have RuntimeHoldReason and RuntimeFreezeReason, why do people need to put there u8?

Simpler code for starter templates and so on. Imagine how many fewer mental barriers you have to "get your first hold working", if you provide a template to students that works with a constrained Reason = u8, rather than needing to pull into RuntimeHoldReason, know about the relation between the two, and how to convert from one to other.

I am working on one such template here https://github.com/kianenigma/flite. WIP.

Basically, this tricks prevents one from needing to understand this whole page.

https://paritytech.github.io/polkadot-sdk/master/polkadot_sdk_docs/reference_docs/frame_runtime_types/index.html

None of this is an issue for us and those who are already familiar ofc.

@bkchr
Copy link
Member

bkchr commented Aug 14, 2024

Imagine how many fewer mental barriers you have to "get your first hold working", if you provide a template to students that works with a constrained Reason = u8, rather than needing to pull into RuntimeHoldReason, know about the relation between the two, and how to convert from one to other.

Not sure how providing a random u8 is easier to understand. You are injecting the other runtime types as well using the derive_impl macro, not sure why you can not do the same for these types?

T::Currency::balance_frozen(&FreezeReason::PoolMinBalance.into(), reward_acc);
having an enum for hold/freeze is IMO much cleaner. It is like you try to campaign for using C style here, which would be an u8. Here you just have a 0 as reason. I would be much more confused on what this 0 means and what I should use there in a real application. Or should I use 0 everywhere? You will in the end also write more docs instead of giving the thing a fixed name WHATEVER_HOLD and it explains the user right away what it is about.

@bkchr
Copy link
Member

bkchr commented Aug 14, 2024

But if you really think that a plain 0 is better there, I will approve this.

@kianenigma kianenigma added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Aug 15, 2024
@kianenigma kianenigma added this pull request to the merge queue Aug 15, 2024
Merged via the queue into master with commit 90c91b1 Aug 15, 2024
169 of 174 checks passed
@kianenigma kianenigma deleted the kiz-variant-count-2 branch August 15, 2024 17:58
ordian added a commit that referenced this pull request Aug 16, 2024
* master:
  Remove redundant minimal template workspace (#5330)
  approval-distribution: Fix handling of conclude (#5375)
  More logs in `is_potential_spam` from `dispute-coordinator` (#5252)
  Fix zombienet bridges test (#5373)
  Update Readme of the `polkadot` crate (#5326)
  allow for `u8` to be used as hold/freeze reason (#5348)
  Moving cargo check for runtimes to GHA (#5340)
  Update links in the documentation (#5175)
  fix visibility for `pallet_nfts` types used as call arguments (#3634)
  Correct some typos in crates' descriptions (#5262)
  Aura: Ensure we are building on each relay chain fork (#5352)
  Update Identity pallet README.md (#5183)
  Bump trie-db from 0.29.0 to 0.29.1 (#5231)
  [Coretime] Always include UnpaidExecution, not just when revenue is > 0 (#5369)
  [Pools] fix derivation of pool account (#4999)
ordian added a commit that referenced this pull request Aug 16, 2024
* master:
  Remove redundant minimal template workspace (#5330)
  approval-distribution: Fix handling of conclude (#5375)
  More logs in `is_potential_spam` from `dispute-coordinator` (#5252)
  Fix zombienet bridges test (#5373)
  Update Readme of the `polkadot` crate (#5326)
  allow for `u8` to be used as hold/freeze reason (#5348)
  Moving cargo check for runtimes to GHA (#5340)
  Update links in the documentation (#5175)
  fix visibility for `pallet_nfts` types used as call arguments (#3634)
  Correct some typos in crates' descriptions (#5262)
  Aura: Ensure we are building on each relay chain fork (#5352)
  Update Identity pallet README.md (#5183)
  Bump trie-db from 0.29.0 to 0.29.1 (#5231)
  [Coretime] Always include UnpaidExecution, not just when revenue is > 0 (#5369)
  [Pools] fix derivation of pool account (#4999)
  Upgrade accidentally downgraded deps (#5365)
  [Pools] Fix issues with member migration to `DelegateStake` (#4822)
  Unify `no_genesis` check (#5360)
  [CI] Fix prdoc command (#5358)
  Beefy: add benchmarks for `report_fork_voting()` (#5188)
ordian added a commit that referenced this pull request Aug 16, 2024
…ct-candidate-weight

* ao-fix-parainclusion-weight-overestimation:
  Remove redundant minimal template workspace (#5330)
  approval-distribution: Fix handling of conclude (#5375)
  More logs in `is_potential_spam` from `dispute-coordinator` (#5252)
  Fix zombienet bridges test (#5373)
  Update Readme of the `polkadot` crate (#5326)
  allow for `u8` to be used as hold/freeze reason (#5348)
  Moving cargo check for runtimes to GHA (#5340)
  Update links in the documentation (#5175)
  fix visibility for `pallet_nfts` types used as call arguments (#3634)
  Correct some typos in crates' descriptions (#5262)
  Aura: Ensure we are building on each relay chain fork (#5352)
  Update Identity pallet README.md (#5183)
  Bump trie-db from 0.29.0 to 0.29.1 (#5231)
  [Coretime] Always include UnpaidExecution, not just when revenue is > 0 (#5369)
  [Pools] fix derivation of pool account (#4999)
  Upgrade accidentally downgraded deps (#5365)
  [Pools] Fix issues with member migration to `DelegateStake` (#4822)
  Unify `no_genesis` check (#5360)
  [CI] Fix prdoc command (#5358)
  Beefy: add benchmarks for `report_fork_voting()` (#5188)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants