Change pallet referenda TracksInfo::tracks to return an iterator #2072

olanod · 2023-10-28T14:35:10Z

Returning an iterator in TracksInfo::tracks() instead of a static slice allows for more flexible implementations of TracksInfo that can use the chain storage without compromising a lot on the performance/memory penalty if we were to return an owned Vec instead.

substrate/frame/referenda/src/types.rs

olanod · 2023-10-30T10:10:24Z

NOTE: I realized that return_position_impl_trait_in_trait was recently stabilized, I think it's worth using it in this feature as it makes usage of the trait much simpler. Please consider updating the rust toolchain :)

EDIT: As the policy is to keep things working with stable Rust, I reverted the usage of the nightly feature(fn tracks() -> impl Iterator<_>).

cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/tracks.rs

substrate/bin/node/runtime/src/lib.rs

cumulus/parachains/runtimes/collectives/collectives-polkadot/src/ambassador/tracks.rs

substrate/frame/referenda/src/lib.rs

…impl_tracksinfo_get`

gui1117 · 2025-02-16T00:45:18Z

substrate/frame/referenda/src/lib.rs

@@ -117,27 +117,6 @@ pub mod benchmarking;

 pub use frame_support::traits::Get;

-#[macro_export]
-macro_rules! impl_tracksinfo_get {


Thanks for having done it, to avoid breaking change we could also keep the macro but make it deprecated. The deprecation message can say: tracks info no longer requires get, the macro invocation can be removed.

But keep it removed is also ok to me.

After a small evaluation, I concluded that removing it is better since we're eliminating some potentially unused code at the almost negligible cost of deleting 5 calls to this macro in the runtimes repo (plus one or two for every runtime being built outside the scope of Polkadot).

To me keeping it removed is a no brainer.

maybe we can also add a comment about this in the PRdoc in a follow-up PR

Yeah. I can add a follow-up PR that only mentions the changes in the API, and that can be included in the stable2503 to let everyone know about the changes.

On it right now 👀

… fix crate name

…itytech#2072) Returning an iterator in `TracksInfo::tracks()` instead of a static slice allows for more flexible implementations of `TracksInfo` that can use the chain storage without compromising a lot on the performance/memory penalty if we were to return an owned `Vec` instead. --------- Co-authored-by: Pablo Andrés Dorado Suárez <[email protected]>

alindima · 2025-02-19T11:44:22Z

This PR broke polkadot.js apps: polkadot-js/apps#11310
Is this something that needs to be fixed in polkadot.js?

pandres95 · 2025-02-19T12:10:34Z

Yes @alindima . As announced in the PR description, runtime users should be aware of the new Track structure.

While internal encoding is the same, the structure it decodes to changes, so it should be adapted.

kianenigma · 2025-02-19T12:11:18Z

This is a good example of PR where I would ask if it was really necessary to cause a breaking change? and also the PRdoc was not highlighting how this should be treated by the API libraries.

gui1117 · 2025-02-19T12:14:44Z

The internal encoding is changed: #2072 (comment)

the field was a Vec<u8> it is now [u8; 25], maybe we can use a type with the same encoding?

ggwpez · 2025-02-19T13:31:26Z

the field was a Vec<u8> it is now [u8; 25], maybe we can use a type with the same encoding?

Yea we could do a custom encoding that prepends the length again. @pandres95 can you please do this and backport into 2503?

gui1117 · 2025-02-19T13:36:20Z

We can also have a type with 2 variant one bounded vec and one static array, with manual encoding. And the decoding always decode the bounded vec variant.

Then there is no breaking change for UI not even padding.

EDIT: prepending the length is an easier implementation though.

pandres95 · 2025-02-19T14:54:48Z

I think the biggest problem here is getting the pallet constant, not really storing the content.

A solution might be transforming the tracks iter into a getter that resolves to the previously known structure. I can do that.

gui1117 · 2025-02-20T00:29:49Z

I think the biggest problem here is getting the pallet constant, not really storing the content.

A solution might be transforming the tracks iter into a getter that resolves to the previously known structure. I can do that.

Yes it will also work, and it will be the least breaking change possible for users

gui1117 · 2025-02-20T01:27:39Z

substrate/frame/referenda/src/lib.rs

+		fn tracks() -> Vec<Track<TrackIdOf<T, I>, BalanceOf<T, I>, BlockNumberFor<T, I>>> {
+			T::Tracks::tracks().map(|t| t.into_owned()).collect()
+		}
+	}


But having this as a constant is incompatible with the trait usage.
In the trait we want to make use of storage to have dynamic tracks. But this being a constant means the track must not change in between runtime upgrade.

I think this PR is inconsistent.
Maybe we should not extend referenda and let other people use a different pallet if they need dynamic tracks.

Or we should make it a view function and remove the constant, which is a bigger breaking change.

Should we revert it?

olanod · 2025-02-20T08:38:24Z

The initial design of this PR took into account Gav's requirements of not having any kind of performance regression(i.e. keep allowing the const definition of tracks) and not breaking anything, as tracks were not stored it was fine but didn't consider the front-end.
Is the name of the track not encoding properly the only breaking change? We can match the previous &str encoding, SCALE encodes string slices like vectors prefixing data with a compact encoding of the length of the bytes, just like the const fn str_arr I introduced previous to this PR to help with the easy declaration of track names in the const context we can extend that function to prepend the needed compact length of the bytes.

ggwpez · 2025-02-21T12:22:33Z

I dont see a fixup MR open - going to revert it until it is sorted. Maybe there exists a better approach of the likes as Gui pointed out above.

…tor (#2072)" This reverts commit c078d2f.

pandres95 · 2025-02-21T13:47:34Z

Hey @ggwpez . The fixup MR will be ready before 5pm (COT) today. I had to figure out the best way possible to make this to be as performant as possible. After some tests, I'm taking a way that's similar to what Gui actually proposed.

olanod requested review from a team October 28, 2023 14:35

bkchr reviewed Oct 28, 2023

View reviewed changes

substrate/frame/referenda/src/types.rs Outdated Show resolved Hide resolved

substrate/frame/referenda/src/types.rs Outdated Show resolved Hide resolved

paritytech-ci requested a review from a team October 28, 2023 17:01

olanod force-pushed the olanod-referenda-owned-tracks branch from b059f67 to 4bc9edc Compare October 30, 2023 09:49

olanod force-pushed the olanod-referenda-owned-tracks branch from 5c3ab3b to 57585ac Compare November 9, 2023 10:11

paritytech-review-bot bot requested a review from a team November 9, 2023 10:12

olanod force-pushed the olanod-referenda-owned-tracks branch 3 times, most recently from 50a8759 to 355615d Compare November 9, 2023 13:48

olanod requested a review from bkchr November 9, 2023 13:50

olanod force-pushed the olanod-referenda-owned-tracks branch 3 times, most recently from c3abd5b to e1a2699 Compare November 10, 2023 17:23

olanod mentioned this pull request Nov 10, 2023

Add str_array utility function to runtime primitives #2275

Merged

olanod force-pushed the olanod-referenda-owned-tracks branch 4 times, most recently from 4994d7e to 47c9710 Compare November 15, 2023 19:05

olanod force-pushed the olanod-referenda-owned-tracks branch 2 times, most recently from 750f1bb to e0d4e7c Compare November 20, 2023 08:39

olanod mentioned this pull request Nov 22, 2023

Pallet referenda tracks for dynamic governance #2448

Draft

olanod force-pushed the olanod-referenda-owned-tracks branch from e0d4e7c to e37fe07 Compare February 20, 2024 15:58

olanod requested a review from a team as a code owner February 20, 2024 15:58

olanod mentioned this pull request Feb 21, 2024

Make pallet_referenda::TracksInfo::tracks return type more flexible #3420

Open

ggwpez reviewed Feb 21, 2024

View reviewed changes

cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/tracks.rs Outdated Show resolved Hide resolved

ggwpez reviewed Feb 21, 2024

View reviewed changes

substrate/bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved

ggwpez reviewed Feb 21, 2024

View reviewed changes

substrate/bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved

ggwpez reviewed Feb 21, 2024

View reviewed changes

cumulus/parachains/runtimes/collectives/collectives-polkadot/src/ambassador/tracks.rs Outdated Show resolved Hide resolved

gui1117 reviewed Feb 15, 2025

View reviewed changes

substrate/frame/referenda/src/lib.rs Outdated Show resolved Hide resolved

pandres95 added 2 commits February 15, 2025 04:10

chore: make ci happy

28d2508

feat(pallet-referenda): add Tracks as an extra constant to remove `…

b1a44be

…impl_tracksinfo_get`

gui1117 approved these changes Feb 16, 2025

View reviewed changes

pandres95 added 2 commits February 16, 2025 23:46

Merge branch 'master' into olanod-referenda-owned-tracks

9780128

fix(prdoc): correct audience from Runtime Users to Runtime User /…

aee2e08

… fix crate name

pandres95 force-pushed the olanod-referenda-owned-tracks branch from 2c4ab10 to aee2e08 Compare February 17, 2025 04:52

ggwpez added this pull request to the merge queue Feb 17, 2025

Merged via the queue into paritytech:master with commit c078d2f Feb 17, 2025
221 of 232 checks passed

pandres95 deleted the olanod-referenda-owned-tracks branch February 17, 2025 12:59

alindima mentioned this pull request Feb 19, 2025

Explorer not rendering for rococo-local runtime polkadot-js/apps#11310

Closed

gui1117 reviewed Feb 20, 2025

View reviewed changes

ggwpez added a commit that referenced this pull request Feb 21, 2025

Revert "Change pallet referenda TracksInfo::tracks to return an itera…

160cdd8

…tor (#2072)" This reverts commit c078d2f.

ggwpez mentioned this pull request Feb 21, 2025

Revert #2072 Tracks iter #7661

Open

pandres95 mentioned this pull request Feb 21, 2025

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

Closed

TarikGul mentioned this pull request Feb 22, 2025

Fix TracksInfo values being undefined and causing a whitescreen polkadot-js/apps#11320

Merged

pandres95 mentioned this pull request Feb 22, 2025

Fix: [Referenda Tracks] Resolve representation issues that are breaking PJS apps #7671

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change pallet referenda TracksInfo::tracks to return an iterator #2072

Change pallet referenda TracksInfo::tracks to return an iterator #2072

olanod commented Oct 28, 2023

olanod commented Oct 30, 2023 •

edited

Loading

gui1117 Feb 16, 2025

pandres95 Feb 17, 2025 •

edited

Loading

gui1117 Feb 19, 2025

pandres95 Feb 19, 2025 •

edited

Loading

alindima commented Feb 19, 2025

pandres95 commented Feb 19, 2025 •

edited

Loading

kianenigma commented Feb 19, 2025

gui1117 commented Feb 19, 2025

ggwpez commented Feb 19, 2025 •

edited

Loading

gui1117 commented Feb 19, 2025 •

edited

Loading

pandres95 commented Feb 19, 2025 •

edited

Loading

gui1117 commented Feb 20, 2025 •

edited

Loading

gui1117 Feb 20, 2025 •

edited

Loading

olanod commented Feb 20, 2025

ggwpez commented Feb 21, 2025

pandres95 commented Feb 21, 2025

Change pallet referenda TracksInfo::tracks to return an iterator #2072

Change pallet referenda TracksInfo::tracks to return an iterator #2072

Conversation

olanod commented Oct 28, 2023

olanod commented Oct 30, 2023 • edited Loading

gui1117 Feb 16, 2025

Choose a reason for hiding this comment

pandres95 Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

gui1117 Feb 19, 2025

Choose a reason for hiding this comment

pandres95 Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

alindima commented Feb 19, 2025

pandres95 commented Feb 19, 2025 • edited Loading

kianenigma commented Feb 19, 2025

gui1117 commented Feb 19, 2025

ggwpez commented Feb 19, 2025 • edited Loading

gui1117 commented Feb 19, 2025 • edited Loading

pandres95 commented Feb 19, 2025 • edited Loading

gui1117 commented Feb 20, 2025 • edited Loading

gui1117 Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

olanod commented Feb 20, 2025

ggwpez commented Feb 21, 2025

pandres95 commented Feb 21, 2025

olanod commented Oct 30, 2023 •

edited

Loading

pandres95 Feb 17, 2025 •

edited

Loading

pandres95 Feb 19, 2025 •

edited

Loading

pandres95 commented Feb 19, 2025 •

edited

Loading

ggwpez commented Feb 19, 2025 •

edited

Loading

gui1117 commented Feb 19, 2025 •

edited

Loading

pandres95 commented Feb 19, 2025 •

edited

Loading

gui1117 commented Feb 20, 2025 •

edited

Loading

gui1117 Feb 20, 2025 •

edited

Loading