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

pallet: Referenda Tracks #2

Merged
merged 2 commits into from
Dec 28, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions substrate/frame/referenda-tracks/README.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
# Remark Storage Pallet
# Referenda Tracks Pallet

Allows storing arbitrary data off chain.
- [`Config`][Config]
- [`Call`][Call]

## Overview

Manage referenda voting tracks.

## Interface

### Dispatchable Functions

- `insert` - Insert a new referenda Track.
- `update` - Update the configuration of an existing referenda Track.
- `remove` - Remove an existing track

License: Apache-2.0
31 changes: 31 additions & 0 deletions substrate/frame/referenda-tracks/src/impl_tracks_info.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
use super::*;

impl<T: Config<I>, I> pallet_referenda::TracksInfo<BalanceOf<T, I>, BlockNumberFor<T>>
for Pallet<T, I>
{
type Id = T::TrackId;
type RuntimeOrigin = <T::RuntimeOrigin as OriginTrait>::PalletsOrigin;
type TracksIter = TracksIter<T, I>;

fn tracks() -> Self::TracksIter {
Tracks::<T, I>::iter().map(|(id, info)| Cow::Owned(Track { id, info }))
}
fn track_for(origin: &Self::RuntimeOrigin) -> Result<Self::Id, ()> {
OriginToTrackId::<T, I>::get(origin).ok_or(())
}
fn tracks_ids() -> Vec<Self::Id> {
TracksIds::<T, I>::get().into_inner()
}
fn info(id: Self::Id) -> Option<Cow<'static, TrackInfoOf<T, I>>> {
Tracks::<T, I>::get(id).map(Cow::Owned)
}
}

impl<T: Config<I>, I: 'static> Get<Vec<TrackOf<T, I>>> for crate::Pallet<T, I> {
fn get() -> Vec<TrackOf<T, I>> {
// expensive but it doesn't seem to be used anywhere
<Pallet<T, I> as pallet_referenda::TracksInfo<BalanceOf<T, I>, BlockNumberFor<T>>>::tracks()
.map(|t| t.into_owned())
.collect()
}
}
190 changes: 140 additions & 50 deletions substrate/frame/referenda-tracks/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,27 @@
// See the License for the specific language governing permissions and
// limitations under the License.

//! Edit and manage referenda voring tracks.
//! # Referenda Tracks Pallet
//!
//! - [`Config`][Config]
//! - [`Call`][Call]
//!
//! ## Overview
//!
//! Manage referenda voting tracks.
//!
//! ## Interface
//!
//! ### Dispatchable Functions
//!
//! - [`insert`][`crate::Pallet::insert`] - Insert a new referenda Track.
//! - [`update`][`crate::Pallet::update`] - Update the configuration of an existing referenda Track.
//! - [`remove`][`crate::Pallet::remove`] - Remove an existing track

#![cfg_attr(not(feature = "std"), no_std)]

mod benchmarking;
mod impl_tracks_info;
pub mod weights;

#[cfg(test)]
Expand All @@ -29,99 +46,172 @@ mod tests;
use core::iter::Map;
use frame_support::{storage::PrefixIterator, traits::OriginTrait};
use frame_system::pallet_prelude::BlockNumberFor;
use pallet_referenda::{BalanceOf, PalletsOriginOf, Track, TrackIdOf, TrackInfoOf};
use pallet_referenda::{BalanceOf, PalletsOriginOf, Track, TrackInfoOf};
use sp_core::Get;
use sp_std::{borrow::Cow, vec::Vec};

pub use pallet::*;
pub use weights::WeightInfo;

type TrackIdOf<T, I> = <T as Config<I>>::TrackId;
type TrackOf<T, I> = Track<<T as Config<I>>::TrackId, BalanceOf<T, I>, BlockNumberFor<T>>;

type TracksIter<T, I> = Map<
PrefixIterator<(<T as Config<I>>::TrackId, TrackInfoOf<T, I>)>,
fn((<T as Config<I>>::TrackId, TrackInfoOf<T, I>)) -> Cow<'static, TrackOf<T, I>>,
>;

impl<T: Config<I>, I> pallet_referenda::TracksInfo<BalanceOf<T, I>, BlockNumberFor<T>>
for Pallet<T, I>
{
type Id = T::TrackId;
type RuntimeOrigin = <T::RuntimeOrigin as OriginTrait>::PalletsOrigin;
type TracksIter = TracksIter<T, I>;

fn tracks() -> Self::TracksIter {
Tracks::<T, I>::iter().map(|(id, info)| Cow::Owned(Track { id, info }))
}
fn track_for(origin: &Self::RuntimeOrigin) -> Result<Self::Id, ()> {
OriginToTrackId::<T, I>::get(origin).ok_or(())
}
fn tracks_ids() -> Vec<Self::Id> {
TracksIds::<T, I>::get().into_inner()
}
fn info(id: Self::Id) -> Option<Cow<'static, TrackInfoOf<T, I>>> {
Tracks::<T, I>::get(id).map(Cow::Owned)
}
}

impl<T: Config<I>, I: 'static> Get<Vec<TrackOf<T, I>>> for crate::Pallet<T, I> {
fn get() -> Vec<Track<T::TrackId, BalanceOf<T, I>, BlockNumberFor<T>>> {
// expensive but it doesn't seem to be used anywhere
<Pallet<T, I> as pallet_referenda::TracksInfo<BalanceOf<T, I>, BlockNumberFor<T>>>::tracks()
.map(|t| t.into_owned())
.collect()
}
}

#[frame_support::pallet]
pub mod pallet {
use super::*;
use frame_support::pallet_prelude::*;
use frame_support::{pallet_prelude::*, traits::EnsureOrigin};
use frame_system::pallet_prelude::*;

#[pallet::config]
pub trait Config<I: 'static = ()>: frame_system::Config + pallet_referenda::Config<I> {
type UpdateOrigin: EnsureOrigin<Self::RuntimeOrigin>;

type RuntimeEvent: From<Event<Self, I>>
+ IsType<<Self as frame_system::Config>::RuntimeEvent>;

type TrackId: Parameter + Member + Copy + MaxEncodedLen + Ord;

type MaxTracks: Get<u32>;
// type WeightInfo: WeightInfo;
///
type WeightInfo: WeightInfo;
}

#[pallet::pallet]
pub struct Pallet<T, I = ()>(_);

#[pallet::call]
impl<T: Config<I>, I: 'static> Pallet<T, I> {
#[pallet::call_index(0)]
#[pallet::weight(Weight::from_parts(10_000, 0) + T::DbWeight::get().writes(1))]
pub fn udpate(origin: OriginFor<T>, _id: TrackIdOf<T, I>) -> DispatchResultWithPostInfo {
let _sender = ensure_signed(origin)?;
// Self::deposit_event(Event::Foo { sender });
Ok(().into())
}
}

#[pallet::storage]
pub type TracksIds<T: Config<I>, I: 'static = ()> =
StorageValue<_, BoundedVec<T::TrackId, <T as Config<I>>::MaxTracks>, ValueQuery>;
StorageValue<_, BoundedVec<TrackIdOf<T, I>, <T as Config<I>>::MaxTracks>, ValueQuery>;

#[pallet::storage]
pub type OriginToTrackId<T: Config<I>, I: 'static = ()> =
StorageMap<_, Blake2_128Concat, PalletsOriginOf<T>, T::TrackId>;
StorageMap<_, Blake2_128Concat, PalletsOriginOf<T>, TrackIdOf<T, I>>;

#[pallet::storage]
pub type Tracks<T: Config<I>, I: 'static = ()> =
StorageMap<_, Blake2_128Concat, T::TrackId, TrackInfoOf<T, I>>;
StorageMap<_, Blake2_128Concat, TrackIdOf<T, I>, TrackInfoOf<T, I>>;

#[pallet::event]
#[pallet::generate_deposit(pub(super) fn deposit_event)]
pub enum Event<T: Config<I>, I: 'static = ()> {
// Foo(T::AccountId),
/// A new track has been inserted
Created { id: TrackIdOf<T, I> },
/// The information for a track has been updated
Updated { id: TrackIdOf<T, I>, info: TrackInfoOf<T, I> },
Copy link
Member

Choose a reason for hiding this comment

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

As events end up in the storage it's usually better to keep them small, TrackInfo can be bulky so instead we could include the origin in the events to give clients what they need to construct the storage key and easily fetch the info from the storage when the event happens.

/// A track has been removed
Removed { id: TrackIdOf<T, I> },
}

#[pallet::error]
pub enum Error<T, I = ()> {}
pub enum Error<T, I = ()> {
/// The maxmimum amount of track IDs was exceeded
MaxTracksExceeded,
/// The specified ID for this track was not found
TrackIdNotFound,
/// The specified ID for this track was already existing
TrackIdAlreadyExisting,
/// The track cannot be removed
CannotRemove,
}

#[pallet::call(weight(<T as Config<I>>::WeightInfo))]
impl<T: Config<I>, I: 'static> Pallet<T, I> {
/// Insert a new referenda Track.
///
/// Parameters:
/// - `id`: The Id of the track to be inserted.
/// - `info`: The configuration of the track.
/// - `pallet_origin`: A generic origin that will be matched to the track.
///
/// Emits `Created` event when successful.
///
/// Weight: `O(1)`
#[pallet::call_index(0)]
pub fn insert(
origin: OriginFor<T>,
id: TrackIdOf<T, I>,
info: TrackInfoOf<T, I>,
pallet_origin: PalletsOriginOf<T>,
) -> DispatchResultWithPostInfo {
T::UpdateOrigin::ensure_origin(origin)?;
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in one of our talks, for insert and remove we will need a different origin(AdminOrigin?) since the entity adding and removing tracks is of higher privilege(e.g. root) than the origin updating the track info(e.g. a community updating info for its own origins).


ensure!(Tracks::<T, I>::get(id) == None, Error::<T, I>::TrackIdAlreadyExisting);

TracksIds::<T, I>::try_append(id).map_err(|_| Error::<T, I>::MaxTracksExceeded)?;
Tracks::<T, I>::set(id, Some(info));
OriginToTrackId::<T, I>::set(pallet_origin.clone(), Some(id));

Self::deposit_event(Event::Created { id });
Ok(().into())
}

/// Update the configuration of an existing referenda Track.
///
/// Parameters:
/// - `id`: The Id of the track to be updated.
/// - `info`: The new configuration of the track.
///
/// Emits `Updated` event when successful.
///
/// Weight: `O(1)`
#[pallet::call_index(1)]
pub fn update(
origin: OriginFor<T>,
id: TrackIdOf<T, I>,
info: TrackInfoOf<T, I>,
) -> DispatchResultWithPostInfo {
T::UpdateOrigin::ensure_origin(origin)?;
Copy link
Member

@olanod olanod Dec 22, 2023

Choose a reason for hiding this comment

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

Thinking that it wouldn't hurt if UpdateOrigin(different from AdminOrigin) is an EnsureOriginWithArg<Self::RuntimeOrigin, TrackIdOf<T, I>>, I could see us using some special track ids that encode information about the community to be able to easily determine later if the community(or subgroup) is allowed to edit the given origin.


Tracks::<T, I>::try_mutate(id, |track| {
if track.is_none() {
return Err(Error::<T, I>::TrackIdNotFound);
};

*track = Some(info.clone());

Ok(())
})?;

Self::deposit_event(Event::Updated { id, info });
Ok(().into())
}

/// Remove an existing track
///
/// Parameters:
/// - `id`: The Id of the track to be deleted.
///
/// Emits `Removed` event when successful.
///
/// Weight: `O(n)`
#[pallet::call_index(2)]
pub fn remove(origin: OriginFor<T>, id: TrackIdOf<T, I>) -> DispatchResultWithPostInfo {
T::UpdateOrigin::ensure_origin(origin)?;

ensure!(Tracks::<T, I>::contains_key(id), Error::<T, I>::TrackIdNotFound);

OriginToTrackId::<T, I>::iter_keys().for_each(|origin| {
Copy link
Member

Choose a reason for hiding this comment

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

Iterating all the origins looks unnecessarily expensive, I think it's reasonable to expect the extrinsic to be called with the corresponding pallet_origin.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking the same.

However, I noticed this structure allowed for a multiple origins to single track setup, like the one that exists on OpenGov currently.

My guess is that, unless we figure out how to make that setup work, this solution has a low chance of being accepted, as it would turn out to be a worse solution than what's already out there (?)

Maybe I'm wrong on the last part, but it's worth exploring those scenarios, and in that case, this expense is necessary.

let track_id = OriginToTrackId::<T, I>::get(origin.clone())
.expect("The given key is provided by StorageMap::iter_keys; qed");
if track_id == id {
OriginToTrackId::<T, I>::remove(origin);
}
});
Tracks::<T, I>::remove(id);
TracksIds::<T, I>::try_mutate(|tracks_ids| {
let new_tracks_ids = tracks_ids.clone().into_iter().filter(|i| i != &id).collect();
*tracks_ids = BoundedVec::<_, _>::truncate_from(new_tracks_ids);

Ok::<(), DispatchError>(())
})?;

Self::deposit_event(Event::Removed { id });
Ok(().into())
}
}
}
28 changes: 25 additions & 3 deletions substrate/frame/referenda-tracks/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ use frame_support::{
weights::Weight,
};
use frame_system::{EnsureRoot, EnsureSignedBy};
use pallet_referenda::{PalletsOriginOf, TrackIdOf, TrackInfoOf};
use scale_info::TypeInfo;
use sp_core::H256;
use sp_io::TestExternalities;
use sp_runtime::{
traits::{BlakeTwo256, IdentityLookup},
BuildStorage, Perbill,
Expand Down Expand Up @@ -118,7 +120,8 @@ impl pallet_referenda_tracks::Config for Test {
type TrackId = u8;
type RuntimeEvent = RuntimeEvent;
type MaxTracks = MaxTracks;
// type WeightInfo = ();
type UpdateOrigin = EnsureRoot<u64>;
type WeightInfo = ();
}

parameter_types! {
Expand Down Expand Up @@ -177,13 +180,32 @@ impl<Class> VoteTally<u32, Class> for Tally {
}
}

pub fn new_test_ext() -> sp_io::TestExternalities {
pub fn new_test_ext(
maybe_tracks: Option<Vec<(TrackIdOf<Test, ()>, TrackInfoOf<Test, ()>, PalletsOriginOf<Test>)>>,
) -> sp_io::TestExternalities {
let balances = vec![(1, 100), (2, 100), (3, 100), (4, 100), (5, 100), (6, 100)];

let t = RuntimeGenesisConfig {
system: Default::default(),
balances: pallet_balances::GenesisConfig::<Test> { balances },
}
.build_storage()
.unwrap();
t.into()

let mut ext = TestExternalities::new(t);
ext.execute_with(|| {
System::set_block_number(1);

if let Some(tracks) = maybe_tracks {
for (id, info, pallet_origin) in tracks {
crate::Pallet::<Test, ()>::insert(RuntimeOrigin::root(), id, info, pallet_origin)
.expect("can insert track");
}

System::reset_events();
} else {
}
});

ext
}
Loading