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.

Sign up for GitHub

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

Multiple proxies added #1

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Multiple proxies added #1

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5b4fa92
Multiple proxies added
grbIzl Dec 7, 2024
61e3c2f
Ordering traits removed
grbIzl Dec 9, 2024
add5677
Fix runtimes build
grbIzl Dec 9, 2024
26cc5a2
Use AccountID in execute call
grbIzl Dec 9, 2024
960a59b
Check supersetting
grbIzl Dec 9, 2024
c4944cb
Bad origin test added for add_proxy
grbIzl Dec 9, 2024
82f8b48
Introduce proxy admin type for proxies management
grbIzl Dec 9, 2024
85beb0a
Runtimes updated
grbIzl Dec 9, 2024
56ead68
Storage changed to map
grbIzl Dec 10, 2024
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
131 changes: 126 additions & 5 deletions pallets/collective-proxy/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,36 @@ mod benchmarking;
pub mod weights;
pub use weights::WeightInfo;

/// The parameters under which a particular account has a proxy relationship with some other
/// account.
#[derive(
Encode,
Decode,
Clone,
Copy,
Eq,
PartialEq,
Ord,
PartialOrd,
MaxEncodedLen,
TypeInfo,
)]
pub struct ProxyDefinition<AccountId, CallFilter> {
/// The account which may act on behalf of another.
pub proxy: AccountId,
/// A value defining the subset of calls that it is allowed to make.
pub filter: CallFilter,
}

#[frame_support::pallet]
pub mod pallet {
use super::*;

/// The current storage version.
pub const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);

#[pallet::pallet]
#[pallet::storage_version(STORAGE_VERSION)]
pub struct Pallet<T>(_);

// TODO: The pallet is intentionally very basic. It could be improved to handle more origins, more aliases, etc.
Expand All @@ -68,11 +93,22 @@ pub mod pallet {
/// Origin that can act on behalf of the collective.
type CollectiveProxy: EnsureOrigin<<Self as frame_system::Config>::RuntimeOrigin>;

/// Account representing the collective treasury.
type ProxyAccountId: Get<Self::AccountId>;
/// Origin with permissions to add and remove proxies for the collective.
type ProxyAdmin: EnsureOrigin<<Self as frame_system::Config>::RuntimeOrigin>;

/// Filter to determine whether a call can be executed or not.
type CallFilter: InstanceFilter<<Self as Config>::RuntimeCall> + Default;
type CallFilter: InstanceFilter<<Self as Config>::RuntimeCall>
+ Member
+ Clone
+ Encode
+ Decode
+ MaxEncodedLen
+ TypeInfo
+ Default;

/// The maximum amount of proxies allowed for a single account.
#[pallet::constant]
type MaxProxies: Get<u32>;

/// Weight info
type WeightInfo: WeightInfo;
Expand All @@ -85,6 +121,25 @@ pub mod pallet {
CollectiveProxyExecuted { result: DispatchResult },
}

#[pallet::error]
pub enum Error<T> {
/// There are too many proxies registered
TooManyProxies,
/// Proxy registration not found.
NotFound,
}

/// The set of account proxies
#[pallet::storage]
pub type Proxies<T: Config> = StorageValue<
Copy link

Choose a reason for hiding this comment

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

Would you consider using a StorageMap, as Dino suggested? This can enforce uniqueness and efficient lookups. Also, it could simplify logic ensuring no redundant filters are stored.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, reworked

_,
BoundedVec<
ProxyDefinition<T::AccountId, T::CallFilter>,
T::MaxProxies,
>,
ValueQuery,
>;

#[pallet::call]
impl<T: Config> Pallet<T> {
/// Executes the call on a behalf of an aliased account.
Expand All @@ -98,19 +153,22 @@ pub mod pallet {
})]
pub fn execute_call(
origin: OriginFor<T>,
proxy: T::AccountId,
call: Box<<T as Config>::RuntimeCall>,
) -> DispatchResult {
// Ensure origin is valid.
T::CollectiveProxy::ensure_origin(origin)?;

let def = Self::find_proxy(proxy)?;

// Account authentication is ensured by the `CollectiveProxy` origin check.
let mut origin: T::RuntimeOrigin =
frame_system::RawOrigin::Signed(T::ProxyAccountId::get()).into();
frame_system::RawOrigin::Signed(def.proxy).into();

// Ensure custom filter is applied.
origin.add_filter(move |c: &<T as frame_system::Config>::RuntimeCall| {
let c = <T as Config>::RuntimeCall::from_ref(c);
T::CallFilter::default().filter(c)
def.filter.filter(c)
});

// Dispatch the call.
Expand All @@ -121,5 +179,68 @@ pub mod pallet {

Ok(())
}

/// Register a proxy account for the sender that is able to make calls on its behalf.
///
/// The dispatch origin for this call must be _Signed_.
///
/// Parameters:
/// - `proxy`: The account that the `caller` would like to make a proxy.
/// - `filter`: Call filter used for the proxy
#[pallet::call_index(1)]
#[pallet::weight(T::WeightInfo::add_proxy(T::MaxProxies::get()))]
pub fn add_proxy(
Copy link

Choose a reason for hiding this comment

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

What happens when the ProxyAdmin tries to overwrite an existing proxy with a stricter filter? (e.g., temporary restriction during maintenance/emergency)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Reworked to map

origin: OriginFor<T>,
proxy: T::AccountId,
filter: T::CallFilter,
) -> DispatchResult {
T::ProxyAdmin::ensure_origin(origin)?;
Proxies::<T>::try_mutate(|proxies| -> Result<(), DispatchError> {
if !proxies.iter().any(|p| p.proxy == proxy && p.filter.is_superset(&filter)) {
Copy link

Choose a reason for hiding this comment

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

The default implementation of is_superset returns false if no custom logic is provided - here.

Imagine, if Proxies contains:

Proxies = [
    ProxyDefinition { proxy: Account1, filter: Filter1 }
]

and you add Filter2, which is a superset of Filter1, the add_proxy logic will push it into Proxies:

Proxies = [
    ProxyDefinition { proxy: Account1, filter: Filter1 },
    ProxyDefinition { proxy: Account1, filter: Filter2 }
]

However, the find_proxy will still return the outdated proxy:

ProxyDefinition { proxy: Account1, filter: Filter1 }

Maybe this could lead to incorrect permissions being applied. Adding a test case to verify and address this scenario would be highly beneficial.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hey Igor! I reworked storage to map to address your (and Dino's) comments.

let proxy_def = ProxyDefinition {
proxy: proxy.clone(),
filter: filter.clone(),
};
proxies.try_push(proxy_def).map_err(|_| Error::<T>::TooManyProxies)?;
}
Ok(())
})
}
Copy link

Choose a reason for hiding this comment

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

What happens if there are multiple Proxies for the same account Id?

Copy link
Owner Author

Choose a reason for hiding this comment

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

They differ by calling filter and the first found with the suitable filter will be used. I think, I relied this logic on the base proxy pallet

Copy link

Choose a reason for hiding this comment

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

Yes, I understand it's implemented like that.

But since CollectiveProxy can register any filter they want, for any account they want, what is the use case for having multiple ProxyDefinition objects with e.g. the same account Id but different privileges?

If one privilege is superset of the other, why would proxy ever use the less privileged one?

To follow-up on my previous comment - one thing to consider here is the data struct used to store the values. Like it is now, we can have vector full of essentially the same values. Is this good for the functionality?

Would you suggest a change here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Aa, I got your concern. I think, it can be easily addressed with superset validation that would prevent the case you're describing (the change added).

Copy link

Choose a reason for hiding this comment

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

That would help, yes, but you can also replace the existing vector with a Map-like collection. E.g. if key is the account Id, then it's not possible to have duplicates.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah. I did experiments with such structures (ordering leftovers were caused by them) but decided that storage costs are not worth it and used simple vec eventually. Added code will prevent any redundant duplication.

Copy link

Choose a reason for hiding this comment

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

I see!

Out of curiosity, how much less performant was the BTreeMap (I'm assuming you tried using that 🙂 )?

Back to my suggestion above, performance wise, this should be equivalent or faster than the vec approach:

    #[pallet::storage]
    pub type Proxies<T: Config> = StorageMap<
        _,
        (T::CollectiveProxy, T::AccountId),
        T::CallFilter,
        ValueQuery,
    >;

The T::CollectiveProxy would be extended to be a parameter-like type.
The T::CallFilter would follow your approach.

This way you can support more than 1 collective proxy type, each proxy can delegate to multiple accounts, but only one delegation type is possible.

Anyways, just something of top of my hat, haven't tried or implemented it 🙂

Thank you for all the replies & the effort.
Except for the one at the start of this comment, no more questions from me :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I was considering standard BoundedBTreeMap. I don't think, there is any significant difference in performance for such small sets of data (adding to equation codec's complexity makes things even more complicated). My concern was mostly about the additional storage costs for TreeMap. I don't remember already but there was a general optimization hint to use compact storage structures as much as possible.

Thank you for your comments and questions! It helped me a lot to understand better the use case.


/// Unregister a proxy account for the sender.
///
/// The dispatch origin for this call must be _Signed_.
///
/// Parameters:
/// - `proxy`: The account that the `caller` would like to remove as a proxy.
/// - `filter`: Call filter used for the proxy
#[pallet::call_index(2)]
#[pallet::weight(T::WeightInfo::remove_proxy(T::MaxProxies::get()))]
pub fn remove_proxy(
origin: OriginFor<T>,
proxy: T::AccountId,
filter: T::CallFilter,
) -> DispatchResult {
T::ProxyAdmin::ensure_origin(origin)?;
Proxies::<T>::try_mutate(|proxies| -> Result<(), DispatchError> {
let proxy_def = ProxyDefinition {
proxy: proxy.clone(),
filter: filter.clone(),
};
proxies.retain(|def| def != &proxy_def);
Ok(())
})
}
}

impl<T: Config> Pallet<T> {
pub fn find_proxy(
proxy: T::AccountId,
) -> Result<ProxyDefinition<T::AccountId, T::CallFilter>, DispatchError> {
let f = |x: &ProxyDefinition<T::AccountId, T::CallFilter>| -> bool {
x.proxy == proxy
};
Ok(Proxies::<T>::get().into_iter().find(f).ok_or(Error::<T>::NotFound)?)
}
}
}
39 changes: 31 additions & 8 deletions pallets/collective-proxy/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,24 +108,47 @@ ord_parameter_types! {
pub const CollectiveProxyManager: AccountId = PRIVILEGED_ACCOUNT;
}

#[derive(Default)]
pub struct MockCallFilter;
#[derive(
Copy,
Clone,
Eq,
PartialEq,
std::fmt::Debug,
parity_scale_codec::Encode,
parity_scale_codec::Decode,
parity_scale_codec::MaxEncodedLen,
scale_info::TypeInfo,
)]
pub enum MockCallFilter {
Any,
JustTransfer
}
impl Default for MockCallFilter {
fn default() -> Self {
Self::Any
}
}
impl InstanceFilter<RuntimeCall> for MockCallFilter {
fn filter(&self, c: &RuntimeCall) -> bool {
matches!(
c,
RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death { .. })
| RuntimeCall::System(frame_system::Call::remark { .. })
)
match self {
MockCallFilter::Any => true,
MockCallFilter::JustTransfer => {
matches!(
c,
RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death { .. })
)
},
}
}
}

impl pallet_collective_proxy::Config for Test {
type RuntimeEvent = RuntimeEvent;
type RuntimeCall = RuntimeCall;
type CollectiveProxy = EnsureSignedBy<CollectiveProxyManager, AccountId>;
type ProxyAccountId = ProxyAccountId;
type ProxyAdmin = EnsureSignedBy<CollectiveProxyManager, AccountId>;
type CallFilter = MockCallFilter;
type MaxProxies = ConstU32<2>;
type WeightInfo = ();
}

Expand Down
29 changes: 29 additions & 0 deletions pallets/collective-proxy/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ fn execute_call_fails_for_invalid_origin() {
assert_noop!(
CollectiveProxy::execute_call(
RuntimeOrigin::signed(1),
1,
Box::new(RuntimeCall::Balances(BalancesCall::transfer_allow_death {
dest: 2,
value: 10
Expand All @@ -37,14 +38,35 @@ fn execute_call_fails_for_invalid_origin() {
});
}

#[test]
fn add_proxy_fails_for_invalid_origin() {
ExtBuilder::build().execute_with(|| {
assert_noop!(
CollectiveProxy::add_proxy(
RuntimeOrigin::signed(1),
1,
MockCallFilter::JustTransfer
),
BadOrigin
);
});
}

#[test]
fn execute_call_filters_not_allowed_call() {
ExtBuilder::build().execute_with(|| {
let init_balance = Balances::free_balance(COMMUNITY_ACCOUNT);

assert_ok!(CollectiveProxy::add_proxy(
RuntimeOrigin::signed(PRIVILEGED_ACCOUNT),
COMMUNITY_ACCOUNT,
MockCallFilter::JustTransfer
));

// Call is filtered, but `execute_call` succeeds.
assert_ok!(CollectiveProxy::execute_call(
RuntimeOrigin::signed(PRIVILEGED_ACCOUNT),
COMMUNITY_ACCOUNT,
Box::new(RuntimeCall::Balances(BalancesCall::transfer_keep_alive {
dest: 2,
value: 10
Expand Down Expand Up @@ -74,8 +96,15 @@ fn execute_call_succeeds() {
let init_balance = Balances::free_balance(COMMUNITY_ACCOUNT);
let transfer_value = init_balance / 3;

assert_ok!(CollectiveProxy::add_proxy(
RuntimeOrigin::signed(PRIVILEGED_ACCOUNT),
COMMUNITY_ACCOUNT,
MockCallFilter::JustTransfer
));

assert_ok!(CollectiveProxy::execute_call(
RuntimeOrigin::signed(PRIVILEGED_ACCOUNT),
COMMUNITY_ACCOUNT,
Box::new(RuntimeCall::Balances(BalancesCall::transfer_allow_death {
dest: 2,
value: transfer_value
Expand Down
46 changes: 46 additions & 0 deletions pallets/collective-proxy/src/weights.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ use core::marker::PhantomData;
/// Weight functions needed for pallet_collective_proxy.
pub trait WeightInfo {
fn execute_call() -> Weight;
fn add_proxy(p: u32, ) -> Weight;
fn remove_proxy(p: u32, ) -> Weight;
}

/// Weights for pallet_collective_proxy using the Substrate node and recommended hardware.
Expand All @@ -62,6 +64,28 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
// Minimum execution time: 7_732_000 picoseconds.
Weight::from_parts(7_950_000, 0)
}
fn add_proxy(p: u32, ) -> Weight {
// Proof Size summary in bytes:
// Measured: `161 + p * (37 ±0)`
// Estimated: `4706`
// Minimum execution time: 21_495_000 picoseconds.
Weight::from_parts(22_358_457, 4706)
// Standard Error: 1_606
.saturating_add(Weight::from_parts(64_322, 0).saturating_mul(p.into()))
.saturating_add(T::DbWeight::get().reads(1_u64))
.saturating_add(T::DbWeight::get().writes(1_u64))
}
fn remove_proxy(p: u32, ) -> Weight {
// Proof Size summary in bytes:
// Measured: `161 + p * (37 ±0)`
// Estimated: `4706`
// Minimum execution time: 21_495_000 picoseconds.
Weight::from_parts(22_579_308, 4706)
// Standard Error: 2_571
.saturating_add(Weight::from_parts(62_404, 0).saturating_mul(p.into()))
.saturating_add(T::DbWeight::get().reads(1_u64))
.saturating_add(T::DbWeight::get().writes(1_u64))
}
}

// For backwards compatibility and tests
Expand All @@ -73,4 +97,26 @@ impl WeightInfo for () {
// Minimum execution time: 7_732_000 picoseconds.
Weight::from_parts(7_950_000, 0)
}
fn add_proxy(p: u32, ) -> Weight {
// Proof Size summary in bytes:
// Measured: `161 + p * (37 ±0)`
// Estimated: `4706`
// Minimum execution time: 21_495_000 picoseconds.
Weight::from_parts(22_358_457, 4706)
// Standard Error: 1_606
.saturating_add(Weight::from_parts(64_322, 0).saturating_mul(p.into()))
.saturating_add(RocksDbWeight::get().reads(1_u64))
.saturating_add(RocksDbWeight::get().writes(1_u64))
}
fn remove_proxy(p: u32, ) -> Weight {
// Proof Size summary in bytes:
// Measured: `161 + p * (37 ±0)`
// Estimated: `4706`
// Minimum execution time: 21_495_000 picoseconds.
Weight::from_parts(22_579_308, 4706)
// Standard Error: 2_571
.saturating_add(Weight::from_parts(62_404, 0).saturating_mul(p.into()))
.saturating_add(RocksDbWeight::get().reads(1_u64))
.saturating_add(RocksDbWeight::get().writes(1_u64))
}
}
16 changes: 14 additions & 2 deletions runtime/astar/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1465,7 +1465,18 @@ parameter_types! {
pub CommunityTreasuryAccountId: AccountId = CommunityTreasuryPalletId::get().into_account_truncating();
}

#[derive(Default)]
#[derive(
Copy,
Clone,
Debug,
Default,
Eq,
PartialEq,
parity_scale_codec::Encode,
parity_scale_codec::Decode,
parity_scale_codec::MaxEncodedLen,
scale_info::TypeInfo,
)]
pub struct CommunityCouncilCallFilter;
impl InstanceFilter<RuntimeCall> for CommunityCouncilCallFilter {
fn filter(&self, c: &RuntimeCall) -> bool {
Expand All @@ -1483,8 +1494,9 @@ impl pallet_collective_proxy::Config for Runtime {
type RuntimeEvent = RuntimeEvent;
type RuntimeCall = RuntimeCall;
type CollectiveProxy = EnsureRootOrTwoThirdsCommunityCouncil;
type ProxyAccountId = CommunityTreasuryAccountId;
type ProxyAdmin = EnsureRootOrTwoThirdsCommunityCouncil;
type CallFilter = CommunityCouncilCallFilter;
type MaxProxies = ConstU32<2>;
type WeightInfo = pallet_collective_proxy::weights::SubstrateWeight<Runtime>;
}

Expand Down
Loading