-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Conversation
#[pallet::constant] | ||
type Prefix: Get<&'static [u8]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here introduced a metadata difference compared to FRAME v1 version. The constant type should be &[u8]
, however there is no way to change the type for metadata(if I didn't missing any v2 new syntax). Need to wait for paritytech/substrate#8827 being solved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this metadata change really matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change will make Prefix
type as & 'static[u8]
. Not sure if polkadotjs handles this type, and if not, I think it would result a createTypes
error(which would probably break apps). @jacogr could you please help to confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked CI. Yes there's a createType
error, and polkadotjs API can't be initialized because of it https://gitlab.parity.io/parity/polkadot/-/jobs/928510 (Line 198)
2021-05-16 15:57:58 REGISTRY: Unable to resolve type &'static[u8], it will fail on construction
2021-05-16 15:57:58 API/INIT: Error: FATAL: Unable to initialize the API: createType(&'static[u8]):: Cannot construct unknown type &'static[u8]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, and really not getting in the way here (which probably means I am), but I have mixed feelings about this PR -
- It needs to be done, since we want all converted
- It is absolutely a nightmare to test this end-to-end to ensure nothing has been broken (so if something does break, this becomes a support horror)
- It is not very active (most have claimed), but when used it just need to work flawlessly
So all-in-all, needs to be done, but not quite seeing the overall benefit since there is no heavy traffic to it. I'm almost in the "if it ain't broke, don't fix it" camp here :)
EDIT: On 2 above, generally actually have to get the W3F guys to test this one end-to-end
Co-authored-by: Keith Yeung <[email protected]>
bot merge |
Trying merge. |
Part of #2882
Converts the
Claims
pallet to the new pallet attribute macro introduced in #6877.Following the upgrade guidelines here: https://crates.parity.io/frame_support/attr.pallet.html#upgrade-guidelines.
From https://crates.parity.io/frame_support/attr.pallet.html#checking-upgrade-guidelines
So users of the
Claims
pallet must be careful about the name they used inconstruct_runtime!
. Hence theruntime-migration
label, which might not be needed depending on the configuration of theClaims
pallet.