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

Fix[Referenda]: Use BoundedVec<u8> as field type for TrackInfo::name #7667

Closed

Conversation

pandres95
Copy link
Contributor

This PR fixes an issue that appeared after merging #2072 to stable2503, where encoding breaking changes cause issues on PJS.

@pandres95 pandres95 requested review from a team as code owners February 21, 2025 18:29
@pandres95 pandres95 changed the base branch from master to stable2503 February 21, 2025 18:30
/// Detailed information about the configuration of a referenda track
#[derive(Clone, Encode, Decode, MaxEncodedLen, TypeInfo, Eq, PartialEq, Debug)]
pub struct TrackInfo<Balance, Moment, const N: usize = DEFAULT_MAX_TRACK_NAME_LEN> {
/// Name of this track.
#[codec(encoded_as = "ArrayLikeVec<N>")]
pub name: [u8; N],
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed to an array and not a static Str as before?

Copy link
Contributor

Choose a reason for hiding this comment

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

it is to be able to decode it from storage, the intention is to be able to have tracks defined in storage.

Copy link
Contributor Author

@pandres95 pandres95 Feb 22, 2025

Choose a reason for hiding this comment

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

Yes. That's the reason.

@gui1117 I tried changing the approach to use BoundedVec as it encodes better, but then there's the problem of needing to allocate memory while statically initializing the tracks data variables in the runtimes.

That approach requires reviving the usage of lazy_static (which I guess we don't like it very much @bkchr).

@gui1117
Copy link
Contributor

gui1117 commented Feb 22, 2025

it should be checked that this works with pjs, and if not we should wait for pjs to be fixed.

@gui1117
Copy link
Contributor

gui1117 commented Feb 22, 2025

Also this doesn't fix the fact that Tracks is a constant while at the same time intending to allow non-constant definition of tracks.
Maybe the constant metadata should be replaced with a pallet view.

@pandres95
Copy link
Contributor Author

pandres95 commented Feb 22, 2025

@gui1117 @ggwpez

I initially noticed the above committed solution (e138d56) fixed the crash in PJS apps. However, the TypeInfo wasn't matching the encoded response from the API, so the consts for referenda weren't being rendered on PJS apps.

Screenshot 2025-02-21 at 11 36 40 pm

So, I tried changing the structure to use BoundedVec as an alternative solution.

Side note This alternative implies returning to lazy_static for runtimes, since there are no viable no_std static lazy loaders in the rust library. This, at least until SyncUnsafeCell is no longer a nightly feature. Then we'll be able to use that instead of lazy_static.

The problem is mostly solved by using BoundedVec, but even though it the issue, it doesn't solve the Governance components rendering. That last problem is definitely in the hands of PJS apps.

Screenshot 2025-02-21 at 11 26 48 pm

I can propose a fix in polkadot-js/apps#11310 for the components, but using BoundedVec seems like the definitive fix.

Screenshot 2025-02-21 at 11 26 44 pm

P.S. Also calling @alindima @TarikGul who might be interested in this.

@pandres95 pandres95 changed the title Fix[Referenda]: Use Vec<u8> as associated encoding type for TrackInfo Fix[Referenda]: Use BoundedVec<u8> as field type for TrackInfo::name Feb 22, 2025
@paritytech-review-bot paritytech-review-bot bot requested a review from a team February 22, 2025 05:08
@pandres95 pandres95 force-pushed the fix/encoding-on-tracks branch from 1e353be to ddfc42c Compare February 22, 2025 05:09
@pandres95 pandres95 force-pushed the fix/encoding-on-tracks branch from ddfc42c to 5f58579 Compare February 22, 2025 05:10
@pandres95
Copy link
Contributor Author

@ggwpez @gui1117 @bkchr @TarikGul closed as superseed by #7671

@pandres95 pandres95 closed this Feb 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants