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

HoldReason: Improve usage #13869

Merged
merged 5 commits into from
May 24, 2023

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Apr 10, 2023

HoldReason was switched recently to use the composite_enum attribute that will merge the enums from all pallets in the runtime to RuntimeHoldReason. pallet-nis was still requiring that the variant was passed as constant to call hold. The proper implementation is to use the HoldReason from inside the pallet directly when calling hold. This is done by adding a RuntimeHoldReason as type to the Config trait and requiring that Currency is using the same reason. Besides that the pr changes the name HoldIdentifier in pallet_balances::Config to RuntimeHoldReason.

Downstream code changes

For the pallet_balances::Config trait you need to change HoldIdentifier to RuntimeHoldReason and pass the RuntimeHoldReason that is being generated by construct_runtime!.

For the pallet_nis::Config trait you need to remove HoldReason as this is not being handled internally of the pallet, but you need to add RuntimeHoldReason which you also need to set to RuntimeHoldReason that is being generated by construct_runtime!.

polkadot companion: paritytech/polkadot#7119

cumulus companion: paritytech/cumulus#2631

`HoldReason` was switched recently to use the `composite_enum` attribute that will merge the enums
from all pallets in the runtime to `RuntimeHoldReason`. `pallet-nis` was still requiring that the
variant was passed as constant to call `hold`. The proper implementation is to use the `HoldReason`
from inside the pallet directly when calling `hold`. This is done by adding a `RuntimeHoldReason` as
type to the `Config` trait and requiring that `Currency` is using the same reason. Besides that the
pr changes the name `HoldIdentifier` in `pallet_balances::Config` to `RuntimeHoldReason`.
@bkchr bkchr added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. labels Apr 10, 2023
@bkchr bkchr requested review from KiChjang and a team April 10, 2023 22:39
@bkchr bkchr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Apr 10, 2023
frame/nis/src/lib.rs Outdated Show resolved Hide resolved
frame/nis/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Copy link
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.

Not sure if Gav would approve the name change from HoldIdentifier to RuntimeHoldReason, but LGTM otherwise.

@ggwpez ggwpez requested a review from gavofyork April 12, 2023 18:37
@bkchr
Copy link
Member Author

bkchr commented Apr 12, 2023

Not sure if Gav would approve the name change from HoldIdentifier to RuntimeHoldReason, but LGTM otherwise.

Naming here just follows RuntimeCall and RuntimeEvent to convey the message better on what this is.

bkchr added a commit to paritytech/polkadot that referenced this pull request Apr 23, 2023
@stale
Copy link

stale bot commented May 23, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label May 23, 2023
@bkchr bkchr requested a review from jsidorenko as a code owner May 24, 2023 09:27
@bkchr bkchr requested a review from a team May 24, 2023 09:27
@bkchr bkchr requested review from cheme and koute as code owners May 24, 2023 09:27
@jsidorenko
Copy link
Contributor

Do we need to open a cumulus companion PR?

@stale stale bot removed A3-stale labels May 24, 2023
@bkchr
Copy link
Member Author

bkchr commented May 24, 2023

Do we need to open a cumulus companion PR?

Yes, I will do this shortly. Was the fractionalization already released on Westmint?

@jsidorenko
Copy link
Contributor

the companion PR was merged yesterday

bkchr added a commit to paritytech/cumulus that referenced this pull request May 24, 2023
@bkchr
Copy link
Member Author

bkchr commented May 24, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 79d37ef into master May 24, 2023
@paritytech-processbot paritytech-processbot bot deleted the bkchr-fix-up-runtime-hold-reason branch May 24, 2023 21:59
paritytech-processbot bot pushed a commit to paritytech/polkadot that referenced this pull request May 24, 2023
* Companion: Substrate#13869

paritytech/substrate#13869

* update lockfile for {"substrate"}

* ".git/.scripts/commands/fmt/fmt.sh"

---------

Co-authored-by: parity-processbot <>
paritytech-processbot bot pushed a commit to paritytech/cumulus that referenced this pull request May 24, 2023
* Companion for: Substrate#13869

paritytech/substrate#13869

* Fix

* Warning

* update lockfile for {"polkadot", "substrate"}

---------

Co-authored-by: parity-processbot <>
ggwpez pushed a commit to ggwpez/runtimes that referenced this pull request Jul 6, 2023
* Companion for: Substrate#13869

paritytech/substrate#13869

* Fix

* Warning

* update lockfile for {"polkadot", "substrate"}

---------

Co-authored-by: parity-processbot <>
ggwpez pushed a commit to ggwpez/runtimes that referenced this pull request Jul 6, 2023
* Companion for: Substrate#13869

paritytech/substrate#13869

* Fix

* Warning

* update lockfile for {"polkadot", "substrate"}

---------

Co-authored-by: parity-processbot <>
Ank4n pushed a commit that referenced this pull request Jul 8, 2023
* HoldReason: Improve usage

`HoldReason` was switched recently to use the `composite_enum` attribute that will merge the enums
from all pallets in the runtime to `RuntimeHoldReason`. `pallet-nis` was still requiring that the
variant was passed as constant to call `hold`. The proper implementation is to use the `HoldReason`
from inside the pallet directly when calling `hold`. This is done by adding a `RuntimeHoldReason` as
type to the `Config` trait and requiring that `Currency` is using the same reason. Besides that the
pr changes the name `HoldIdentifier` in `pallet_balances::Config` to `RuntimeHoldReason`.

* Update frame/nis/src/lib.rs

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

* Review comment

* Fixes

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ggwpez pushed a commit to ggwpez/runtimes that referenced this pull request Jul 13, 2023
* Companion: Substrate#13869

paritytech/substrate#13869

* update lockfile for {"substrate"}

* ".git/.scripts/commands/fmt/fmt.sh"

---------

Co-authored-by: parity-processbot <>
ggwpez pushed a commit to ggwpez/runtimes that referenced this pull request Jul 13, 2023
* Companion for: Substrate#13869

paritytech/substrate#13869

* Fix

* Warning

* update lockfile for {"polkadot", "substrate"}

---------

Co-authored-by: parity-processbot <>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* HoldReason: Improve usage

`HoldReason` was switched recently to use the `composite_enum` attribute that will merge the enums
from all pallets in the runtime to `RuntimeHoldReason`. `pallet-nis` was still requiring that the
variant was passed as constant to call `hold`. The proper implementation is to use the `HoldReason`
from inside the pallet directly when calling `hold`. This is done by adding a `RuntimeHoldReason` as
type to the `Config` trait and requiring that `Currency` is using the same reason. Besides that the
pr changes the name `HoldIdentifier` in `pallet_balances::Config` to `RuntimeHoldReason`.

* Update frame/nis/src/lib.rs

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

* Review comment

* Fixes

---------

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.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants