Introduce CreateBare, deprecated CreateInherent#7597
Conversation
| /// | ||
| /// It can also be used to create an unsigned transaction as they are both the same extrinsic | ||
| /// variant: `Bare`. Unsigned transaction are deprecated in favor of general transaction. | ||
| pub trait CreateInherent<LocalCall>: CreateTransactionBase<LocalCall> { |
There was a problem hiding this comment.
The naming is still just wrong and trying to fix this via docs is not making it better. As I already said in the original pr, the name of the trait is wrong.
I would propose we introduce a new CreateBare with proper docs and tag this trait deprecated.
There was a problem hiding this comment.
I updated the PR: deprecate CreateInherent, introduce CreateBare and replace all usage in polkadot-sdk.
It is a breaking change so I am not sure I can backport anything to 2412, but I will backport it to 2503.
I acknowledge my mistake in transaction extension review, also overall I underestimated the stability and the pace of new feature in polkadot-sdk in 2024.
As a side note I think offchain traits could be improved as well:
- we could remove
LocalCalleverywhere, people can boundRuntimeCall: From<Call>in their config CreateTransactionBasecould simply boundframe_system::Configas supertrait instead of having a duplicated associated typeRuntimeCallthat bring name conflict. (We do boundConfigas supertrait forSigningTypes).
But better not to break them.
There was a problem hiding this comment.
I agree with the offchain trait refactor, but it's probably best to do it after phase 2 of Extrinsic Horizon is done.
|
|
||
| /// Submit an unsigned transaction onchain with a signed payload | ||
| pub trait SendUnsignedTransaction<T: SigningTypes + CreateInherent<LocalCall>, LocalCall> { | ||
| pub trait SendUnsignedTransaction<T: SigningTypes + CreateBare<LocalCall>, LocalCall> { |
There was a problem hiding this comment.
If you change this, it basically means that people may have CreateInherent implemented, but it will not be used by anything.
There was a problem hiding this comment.
Basically also resulting in compile errors.
There was a problem hiding this comment.
If we do this, runtime will have to implement CreateInherent with the intention of creating a bare extrinsic.
It is ok to me, and the least breaking change, but it keeps the faulty trait usage: CreateInherent, I will also add in the doc of CreateInherent the intention.
There was a problem hiding this comment.
I updated the code:
CreateInherenthas a new methodcreate_barewith default implementation. User can override it to specify the behavior. (not ideal but not a breaking change.)CreateBareis introduced, automatically implemented forCreateInherentusingCreateInherent::create_bare.CreateBareis used instead ofCreateInherentforSendUnsignedTransactionand it implementation forSigner.- pallets such as beefy are not using
CreateBarein their trait bound to avoid any breaking change. - User are expected to implement
CreateInherentfor their runtime in order to keep pallet's usage. - User are expected to use
CreateBarein their pallet, the idiomatic trait to create a bare extrinsic.
Possible alternative design: slightly more breaking, but provides a path forward:
- I considered deprecated
CreateInherentand useCreateBarein polkadot-sdk pallets. That would be a breaking change: e.g. if a pallets bounds beefy config and useT::create_inherentmethod then their code then it won't compile. They have to usecreate_bareand importCreateBasetrait in scope. (note: we could introduceCreateBare::create_inherentso that rustc give some hint to import the trait in scope.)
There was a problem hiding this comment.
I think it's ok as it is right now, fixes the name and helps the dev a bit with the default impl.
|
/cmd fmt |
|
Considering approvals here for the PR when it was in a different state, I am not sure I should consider this PR approved. But IMO it looks good. |
|
Having Having In this PR, I choose a rename of the trait, I know renames are not very good but the documentation Another solution would simply to mark the trait as deprecated and create a new trait, and update all our usage for the new trait If you have some opinion @bkchr |
|
@bkchr because you approve a different version that I had to change because it didn't work at the end. This final version is: User using I think it is good. |
* master: (99 commits) Snowbridge: Remove asset location check for compatibility (#8473) add poke_deposit extrinsic to pallet-bounties (#8382) litep2p/peerset: Reject non-reserved peers in the reserved-only mode (#8650) Charge deposit based on key length (#8648) [pallet-revive] make subscription task panic on error (#8587) tx/metrics: Add metrics for the RPC v2 `transactionWatch_v1_submitAndWatch` (#8345) Bridges: Fix - Improve try-state for pallet-xcm-bridge-hub (#8615) Introduce CreateBare, deprecated CreateInherent (#7597) Use hashbrown hashmap/hashset in validation context (#8606) ci: rm gitlab config (#8622) 🔪 flaky and Zombienet tests (#8600) cumulus: adjust unincluded segment size metric buckets (#8617) Benchmark storage access on block validation (#8069) Revert 7934 es/remove tj changes (#8611) collator-protocol: add more collation observability (#8230) `fatxpool`: add fallback for ready at light (#8533) txpool: fix tx removal from unlocks set (#8500) XCMP weight metering: account for the MQ page position (#8344) fix epmb solution duplicate issue + add remote mining apparatus to epm (#8585) Fix generated address returned by Substrate RPC runtime call (#8504) ...
Rename `CreateInherent` to `CreateBare`, add method `create_bare` and deprecate `create_inherent`. Both unsigned transaction and inherent use the extrinsic type `Bare`. Before this PR `CreateInherent` trait was use to generate unsigned transaction, now unsigned transaction can be generated using a proper trait `CreateBare`. How to upgrade: * Change usage of `CreateInherent` to `CreateBare` and `create_inherent` to `create_bare`. * Implement `CreateBare` for the runtime, the method `create_bare` is usually implemented using `Extrinsic::new_bare`. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This brings in `stable2506` Polkadot SDK, and integrates many new features. Integrated breaking changes to be verified by the original authors: - [x] ~paritytech/polkadot-sdk#8127 @kianenigma @Ank4n~ This will come in with AHM, and not before. - [x] paritytech/polkadot-sdk#7597 @gui1117 - [x] paritytech/polkadot-sdk#8254 @bkchr - [x] paritytech/polkadot-sdk#7592 @bkontur - [x] paritytech/polkadot-sdk#8382 @UtkarshBhardwaj007 - [x] paritytech/polkadot-sdk#8021 @serban300 - [x] paritytech/polkadot-sdk#8344 @serban300 - [x] paritytech/polkadot-sdk#8262 @athei - [x] paritytech/polkadot-sdk#8584 @athei - [x] paritytech/polkadot-sdk#8299 @skunert - [x] paritytech/polkadot-sdk#8652 @pgherveou - [x] paritytech/polkadot-sdk#8554 @pgherveou - [x] paritytech/polkadot-sdk#8281 @mrshiposha - [x] paritytech/polkadot-sdk#7730 @franciscoaguirre - [x] paritytech/polkadot-sdk#8599 @yrong @claravanstaden - [x] paritytech/polkadot-sdk#8531 @bkontur - [x] paritytech/polkadot-sdk#8409 @kianenigma - [x] paritytech/polkadot-sdk#9137 @franciscoaguirre - [x] paritytech/polkadot-sdk#7944 @bkontur - [x] paritytech/polkadot-sdk#8179 @bkontur - [x] paritytech/polkadot-sdk#8037 @yrong --------- Co-authored-by: GitHub Action <[email protected]> Co-authored-by: claravanstaden <[email protected]> Co-authored-by: Branislav Kontur <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Alain Brenzikofer <[email protected]> Co-authored-by: kianenigma <[email protected]> Co-authored-by: Francisco Aguirre <[email protected]> Co-authored-by: ron <[email protected]> Co-authored-by: joe petrowski <[email protected]> Co-authored-by: Overkillus <[email protected]>
Rename `CreateInherent` to `CreateBare`, add method `create_bare` and deprecate `create_inherent`. Both unsigned transaction and inherent use the extrinsic type `Bare`. Before this PR `CreateInherent` trait was use to generate unsigned transaction, now unsigned transaction can be generated using a proper trait `CreateBare`. How to upgrade: * Change usage of `CreateInherent` to `CreateBare` and `create_inherent` to `create_bare`. * Implement `CreateBare` for the runtime, the method `create_bare` is usually implemented using `Extrinsic::new_bare`. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Rename
CreateInherenttoCreateBare, add methodcreate_bareand deprecatecreate_inherent.Both unsigned transaction and inherent use the extrinsic type
Bare.Before this PR
CreateInherenttrait was use to generate unsigned transaction, now unsigned transaction can be generated using a proper traitCreateBare.How to upgrade:
CreateInherenttoCreateBareandcreate_inherenttocreate_bare.CreateBarefor the runtime, the methodcreate_bareis usually implemented usingExtrinsic::new_bare.