Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Keyless proxied account (a.k.a anonymous proxy v2) #11115

Closed
wants to merge 3 commits into from
Closed

Keyless proxied account (a.k.a anonymous proxy v2) #11115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e7f92b0
Auto fmt
olanod Mar 24, 2022
5bbd4a8
Rename anonymous to keyless
olanod Mar 24, 2022
61577b3
Create keyless account from vanity address
olanod Mar 24, 2022
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
101 changes: 65 additions & 36 deletions frame/proxy/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,10 @@ pub mod pallet {
#[pallet::constant]
type MaxPending: Get<u32>;

/// The maximum amount of characters a keyless vanity address can have.
#[pallet::constant]
type MaxVanity: Get<u8>;

/// The type of hash used for hashing the call.
type CallHasher: Hash;

Expand Down Expand Up @@ -267,7 +271,7 @@ pub mod pallet {
///
/// The dispatch origin for this call must be _Signed_.
///
/// WARNING: This may be called on accounts created by `anonymous`, however if done, then
/// WARNING: This may be called on accounts created by `proxied_keyless`, however if done, then
/// the unreserved fees will be inaccessible. **All access to this account will be lost.**
///
/// # <weight>
Expand Down Expand Up @@ -295,6 +299,8 @@ pub mod pallet {
/// want to use `0`.
/// - `delay`: The announcement period required of the initial proxy. Will generally be
/// zero.
/// - `maybe_keyless`: An account to be used as the keyless proxied account instead of the
/// generated one, it will be evaluated to determine if it's indeed a keyless account.
///
/// Fails with `Duplicate` if this has already been called in this transaction, from the
/// same sender, with the same parameters.
Expand All @@ -305,17 +311,23 @@ pub mod pallet {
/// Weight is a function of the number of proxies the user has (P).
/// # </weight>
/// TODO: Might be over counting 1 read
#[pallet::weight(T::WeightInfo::anonymous(T::MaxProxies::get().into()))]
pub fn anonymous(
#[pallet::weight(T::WeightInfo::proxied_keyless(T::MaxProxies::get().into()))]
pub fn proxied_keyless(
Copy link
Contributor

Choose a reason for hiding this comment

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

please avoid breaking changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, this was more of a quick search and replace, I'll keep the previous "deprecated" method around. Any other things to consider when introducing this kind of API changes?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should rename the functions though... Are you suggesting we should add a new function with a new name?

AFAIK this "breaking change" would only be breaking for JS integrations, but all txs and signers will be okay

Copy link
Contributor

Choose a reason for hiding this comment

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

rename it is breaking for JS and ledger. let's avoid that unless absolutely necessary.

and new parameter is a transaction_version breaking change.

origin: OriginFor<T>,
proxy_type: T::ProxyType,
delay: T::BlockNumber,
index: u16,
maybe_keyless: Option<T::AccountId>,
) -> DispatchResult {
let who = ensure_signed(origin)?;

let anonymous = Self::anonymous_account(&who, &proxy_type, index, None);
ensure!(!Proxies::<T>::contains_key(&anonymous), Error::<T>::Duplicate);
let keyless = if let Some(account) = maybe_keyless {
ensure!(Self::is_vain_enough(&account), Error::<T>::NotKeyless);
account
Copy link
Contributor

Choose a reason for hiding this comment

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

what? you allow people to specify the account? that for sure is a security hole.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But is it though? I don't think it's sure, that's my main question here as this is based on an empirical assumption. There's gonna be a line to not cross when evaluating the "keylessness" of an account that might even get blurry at some point, but we can always be super conservative about it to stay on the safe side. The initial approach here is leaving part of the judgment to the implementer that should specify a safe MaxVanity. Perhaps a crypto-math expert can jump in and say with certainty where the safe line is? is it much different from the safety of a pallet id(e.g. modlpy/trsry)?

Copy link
Contributor

Choose a reason for hiding this comment

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

No it is different thing. You allow people to pick any addresses that fits a requirement without many more complicated checking and that will allow reuse address attack etc. Allow anyone to pick their own address is almost always a bad idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would a more complicated checking look like? perhaps make the process more expensive/permissioned? an auction? should governance be like a registar that sells/leases the right to have those accounts? Not to sound stubborn but I think it's a feature too cool to not make the effort to make it work somehow. wouldn't 1AcaLaNetwork1111111111111111111111111111111L6H be a nice account to have for a project?

} else {
Self::keyless_account(&who, &proxy_type, index, None)
};
ensure!(!Proxies::<T>::contains_key(&keyless), Error::<T>::Duplicate);

let proxy_def =
ProxyDefinition { delegate: who.clone(), proxy_type: proxy_type.clone(), delay };
Expand All @@ -325,9 +337,9 @@ pub mod pallet {
let deposit = T::ProxyDepositBase::get() + T::ProxyDepositFactor::get();
T::Currency::reserve(&who, deposit)?;

Proxies::<T>::insert(&anonymous, (bounded_proxies, deposit));
Self::deposit_event(Event::AnonymousCreated {
anonymous,
Proxies::<T>::insert(&keyless, (bounded_proxies, deposit));
Self::deposit_event(Event::KeylessCreated {
keyless,
who,
proxy_type,
disambiguation_index: index,
Expand All @@ -336,28 +348,28 @@ pub mod pallet {
Ok(())
}

/// Removes a previously spawned anonymous proxy.
/// Removes a previously spawned keyless account.
///
/// WARNING: **All access to this account will be lost.** Any funds held in it will be
/// inaccessible.
///
/// Requires a `Signed` origin, and the sender account must have been created by a call to
/// `anonymous` with corresponding parameters.
/// `proxied_keyless` with corresponding parameters.
///
/// - `spawner`: The account that originally called `anonymous` to create this account.
/// - `index`: The disambiguation index originally passed to `anonymous`. Probably `0`.
/// - `proxy_type`: The proxy type originally passed to `anonymous`.
/// - `height`: The height of the chain when the call to `anonymous` was processed.
/// - `ext_index`: The extrinsic index in which the call to `anonymous` was processed.
/// - `spawner`: The account that originally called `proxied_keyless` to create this account.
/// - `index`: The disambiguation index originally passed to `proxied_keyless`. Probably `0`.
/// - `proxy_type`: The proxy type originally passed to `proxied_keyless`.
/// - `height`: The height of the chain when the call to `proxied_keyless` was processed.
/// - `ext_index`: The extrinsic index in which the call to `proxied_keyless` was processed.
///
/// Fails with `NoPermission` in case the caller is not a previously created anonymous
/// account whose `anonymous` call has corresponding parameters.
/// Fails with `NoPermission` in case the caller is not a previously created keyless
/// account whose `proxied_keyless` call has corresponding parameters.
///
/// # <weight>
/// Weight is a function of the number of proxies the user has (P).
/// # </weight>
#[pallet::weight(T::WeightInfo::kill_anonymous(T::MaxProxies::get().into()))]
pub fn kill_anonymous(
#[pallet::weight(T::WeightInfo::kill_keyless(T::MaxProxies::get().into()))]
pub fn kill_keyless(
origin: OriginFor<T>,
spawner: T::AccountId,
proxy_type: T::ProxyType,
Expand All @@ -368,7 +380,7 @@ pub mod pallet {
let who = ensure_signed(origin)?;

let when = (height, ext_index);
let proxy = Self::anonymous_account(&spawner, &proxy_type, index, Some(when));
let proxy = Self::keyless_account(&spawner, &proxy_type, index, Some(when));
ensure!(proxy == who, Error::<T>::NoPermission);

let (_, deposit) = Proxies::<T>::take(&who);
Expand Down Expand Up @@ -536,9 +548,9 @@ pub mod pallet {
let call_hash = T::CallHasher::hash_of(&call);
let now = system::Pallet::<T>::block_number();
Self::edit_announcements(&delegate, |ann| {
ann.real != real ||
ann.call_hash != call_hash ||
now.saturating_sub(ann.height) < def.delay
ann.real != real
|| ann.call_hash != call_hash
|| now.saturating_sub(ann.height) < def.delay
})
.map_err(|_| Error::<T>::Unannounced)?;

Expand All @@ -555,8 +567,8 @@ pub mod pallet {
ProxyExecuted { result: DispatchResult },
/// Anonymous account has been created by new proxy with given
/// disambiguation index and proxy type.
AnonymousCreated {
anonymous: T::AccountId,
KeylessCreated {
keyless: T::AccountId,
who: T::AccountId,
proxy_type: T::ProxyType,
disambiguation_index: u16,
Expand Down Expand Up @@ -601,6 +613,8 @@ pub mod pallet {
Unannounced,
/// Cannot add self as proxy.
NoSelfProxy,
/// The provided keyless account is not vain enough.
NotKeyless,
}

/// The set of account proxies. Maps the account which has delegated to the accounts
Expand Down Expand Up @@ -634,7 +648,7 @@ pub mod pallet {
}

impl<T: Config> Pallet<T> {
/// Calculate the address of an anonymous account.
/// Calculate the address of a keyless account.
///
/// - `who`: The spawner account.
/// - `proxy_type`: The type of the proxy that the sender will be registered as over the
Expand All @@ -643,9 +657,9 @@ impl<T: Config> Pallet<T> {
/// - `index`: A disambiguation index, in case this is called multiple times in the same
/// transaction (e.g. with `utility::batch`). Unless you're using `batch` you probably just
/// want to use `0`.
/// - `maybe_when`: The block height and extrinsic index of when the anonymous account was
/// - `maybe_when`: The block height and extrinsic index of when the keyless account was
/// created. None to use current block height and extrinsic index.
pub fn anonymous_account(
pub fn keyless_account(
who: &T::AccountId,
proxy_type: &T::ProxyType,
index: u16,
Expand All @@ -663,6 +677,16 @@ impl<T: Config> Pallet<T> {
.expect("infinite length input; no invalid inputs for type; qed")
}

fn is_vain_enough(account: &T::AccountId) -> bool {
let vanity_len = T::MaxVanity::get() as usize;
account
.as_ref()
.get(vanity_len..)
.expect("MaxVanity shorter than account")
.iter()
.all(|b| b == &0)
}

/// Register a proxy account for the delegator that is able to make calls on its behalf.
///
/// Parameters:
Expand Down Expand Up @@ -799,8 +823,8 @@ impl<T: Config> Pallet<T> {
force_proxy_type: Option<T::ProxyType>,
) -> Result<ProxyDefinition<T::AccountId, T::ProxyType, T::BlockNumber>, DispatchError> {
let f = |x: &ProxyDefinition<T::AccountId, T::ProxyType, T::BlockNumber>| -> bool {
&x.delegate == delegate &&
force_proxy_type.as_ref().map_or(true, |y| &x.proxy_type == y)
&x.delegate == delegate
&& force_proxy_type.as_ref().map_or(true, |y| &x.proxy_type == y)
};
Ok(Proxies::<T>::get(real).0.into_iter().find(f).ok_or(Error::<T>::NotProxy)?)
}
Expand All @@ -818,19 +842,24 @@ impl<T: Config> Pallet<T> {
match c.is_sub_type() {
// Proxy call cannot add or remove a proxy with more permissions than it already
// has.
Some(Call::add_proxy { ref proxy_type, .. }) |
Some(Call::remove_proxy { ref proxy_type, .. })
Some(Call::add_proxy { ref proxy_type, .. })
| Some(Call::remove_proxy { ref proxy_type, .. })
if !def.proxy_type.is_superset(&proxy_type) =>
false,
// Proxy call cannot remove all proxies or kill anonymous proxies unless it has full
{
false
},
// Proxy call cannot remove all proxies or kill keyless accounts unless it has full
// permissions.
Some(Call::remove_proxies { .. }) | Some(Call::kill_anonymous { .. })
Some(Call::remove_proxies { .. }) | Some(Call::kill_keyless { .. })
if def.proxy_type != T::ProxyType::default() =>
false,
{
false
},
_ => def.proxy_type.filter(c),
}
});
let e = call.dispatch(origin);
Self::deposit_event(Event::ProxyExecuted { result: e.map(|_| ()).map_err(|e| e.error) });
}
}
use_vanitywith_vanity: use_vanitywith_vanity:
33 changes: 17 additions & 16 deletions frame/proxy/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use frame_support::{
assert_noop, assert_ok,
dispatch::DispatchError,
parameter_types,
traits::{ConstU32, ConstU64, Contains},
traits::{ConstU32, ConstU64, ConstU8, Contains},
RuntimeDebug,
};
use sp_core::H256;
Expand Down Expand Up @@ -160,6 +160,7 @@ impl Config for Test {
type WeightInfo = ();
type CallHasher = BlakeTwo256;
type MaxPending = ConstU32<2>;
type MaxVanity = ConstU8<10>;
type AnnouncementDepositBase = ConstU64<1>;
type AnnouncementDepositFactor = ConstU64<1>;
}
Expand Down Expand Up @@ -537,42 +538,42 @@ fn proxying_works() {
}

#[test]
fn anonymous_works() {
fn keyless_works() {
new_test_ext().execute_with(|| {
assert_ok!(Proxy::anonymous(Origin::signed(1), ProxyType::Any, 0, 0));
let anon = Proxy::anonymous_account(&1, &ProxyType::Any, 0, None);
assert_ok!(Proxy::proxied_keyless(Origin::signed(1), ProxyType::Any, 0, 0));
let anon = Proxy::keyless_account(&1, &ProxyType::Any, 0, None);
System::assert_last_event(
ProxyEvent::AnonymousCreated {
anonymous: anon.clone(),
ProxyEvent::KeylessCreated {
keyless: anon.clone(),
who: 1,
proxy_type: ProxyType::Any,
disambiguation_index: 0,
}
.into(),
);

// other calls to anonymous allowed as long as they're not exactly the same.
assert_ok!(Proxy::anonymous(Origin::signed(1), ProxyType::JustTransfer, 0, 0));
assert_ok!(Proxy::anonymous(Origin::signed(1), ProxyType::Any, 0, 1));
let anon2 = Proxy::anonymous_account(&2, &ProxyType::Any, 0, None);
assert_ok!(Proxy::anonymous(Origin::signed(2), ProxyType::Any, 0, 0));
// other calls to keyless allowed as long as they're not exactly the same.
assert_ok!(Proxy::proxied_keyless(Origin::signed(1), ProxyType::JustTransfer, 0, 0));
assert_ok!(Proxy::proxied_keyless(Origin::signed(1), ProxyType::Any, 0, 1));
let anon2 = Proxy::keyless_account(&2, &ProxyType::Any, 0, None);
assert_ok!(Proxy::proxied_keyless(Origin::signed(2), ProxyType::Any, 0, 0));
assert_noop!(
Proxy::anonymous(Origin::signed(1), ProxyType::Any, 0, 0),
Proxy::proxied_keyless(Origin::signed(1), ProxyType::Any, 0, 0),
Error::<Test>::Duplicate
);
System::set_extrinsic_index(1);
assert_ok!(Proxy::anonymous(Origin::signed(1), ProxyType::Any, 0, 0));
assert_ok!(Proxy::proxied_keyless(Origin::signed(1), ProxyType::Any, 0, 0));
System::set_extrinsic_index(0);
System::set_block_number(2);
assert_ok!(Proxy::anonymous(Origin::signed(1), ProxyType::Any, 0, 0));
assert_ok!(Proxy::proxied_keyless(Origin::signed(1), ProxyType::Any, 0, 0));

let call = Box::new(call_transfer(6, 1));
assert_ok!(Balances::transfer(Origin::signed(3), anon, 5));
assert_ok!(Proxy::proxy(Origin::signed(1), anon, None, call));
System::assert_last_event(ProxyEvent::ProxyExecuted { result: Ok(()) }.into());
assert_eq!(Balances::free_balance(6), 1);

let call = Box::new(Call::Proxy(ProxyCall::new_call_variant_kill_anonymous(
let call = Box::new(Call::Proxy(ProxyCall::new_call_variant_kill_keyless(
1,
ProxyType::Any,
0,
Expand All @@ -583,7 +584,7 @@ fn anonymous_works() {
let de = DispatchError::from(Error::<Test>::NoPermission).stripped();
System::assert_last_event(ProxyEvent::ProxyExecuted { result: Err(de) }.into());
assert_noop!(
Proxy::kill_anonymous(Origin::signed(1), 1, ProxyType::Any, 0, 1, 0),
Proxy::kill_keyless(Origin::signed(1), 1, ProxyType::Any, 0, 1, 0),
Error::<Test>::NoPermission
);
assert_eq!(Balances::free_balance(1), 0);
Expand Down
12 changes: 6 additions & 6 deletions frame/proxy/src/weights.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ pub trait WeightInfo {
fn add_proxy(p: u32, ) -> Weight;
fn remove_proxy(p: u32, ) -> Weight;
fn remove_proxies(p: u32, ) -> Weight;
fn anonymous(p: u32, ) -> Weight;
fn kill_anonymous(p: u32, ) -> Weight;
fn proxied_keyless(p: u32, ) -> Weight;
fn kill_keyless(p: u32, ) -> Weight;
}

/// Weights for pallet_proxy using the Substrate node and recommended hardware.
Expand Down Expand Up @@ -140,15 +140,15 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
}
// Storage: unknown [0x3a65787472696e7369635f696e646578] (r:1 w:0)
// Storage: Proxy Proxies (r:1 w:1)
fn anonymous(p: u32, ) -> Weight {
fn proxied_keyless(p: u32, ) -> Weight {
(25_743_000 as Weight)
// Standard Error: 2_000
.saturating_add((25_000 as Weight).saturating_mul(p as Weight))
.saturating_add(T::DbWeight::get().reads(2 as Weight))
.saturating_add(T::DbWeight::get().writes(1 as Weight))
}
// Storage: Proxy Proxies (r:1 w:1)
fn kill_anonymous(p: u32, ) -> Weight {
fn kill_keyless(p: u32, ) -> Weight {
(20_484_000 as Weight)
// Standard Error: 2_000
.saturating_add((105_000 as Weight).saturating_mul(p as Weight))
Expand Down Expand Up @@ -238,15 +238,15 @@ impl WeightInfo for () {
}
// Storage: unknown [0x3a65787472696e7369635f696e646578] (r:1 w:0)
// Storage: Proxy Proxies (r:1 w:1)
fn anonymous(p: u32, ) -> Weight {
fn proxied_keyless(p: u32, ) -> Weight {
(25_743_000 as Weight)
// Standard Error: 2_000
.saturating_add((25_000 as Weight).saturating_mul(p as Weight))
.saturating_add(RocksDbWeight::get().reads(2 as Weight))
.saturating_add(RocksDbWeight::get().writes(1 as Weight))
}
// Storage: Proxy Proxies (r:1 w:1)
fn kill_anonymous(p: u32, ) -> Weight {
fn kill_keyless(p: u32, ) -> Weight {
(20_484_000 as Weight)
// Standard Error: 2_000
.saturating_add((105_000 as Weight).saturating_mul(p as Weight))
Expand Down
5 changes: 3 additions & 2 deletions frame/system/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ pub mod pallet {
+ Debug
+ MaybeDisplay
+ Ord
+ AsRef<[u8]>
+ MaxEncodedLen;

/// Converting trait to take a source type and convert to `AccountId`.
Expand Down Expand Up @@ -1193,7 +1194,7 @@ impl<T: Config> Pallet<T> {
let block_number = Self::block_number();
// Don't populate events on genesis.
if block_number.is_zero() {
return
return;
}

let phase = ExecutionPhase::<T>::get().unwrap_or_default();
Expand Down Expand Up @@ -1615,7 +1616,7 @@ impl<T: Config> StoredMap<T::AccountId, T::AccountData> for Pallet<T> {
},
}
} else if !was_providing && !is_providing {
return Ok(result)
return Ok(result);
}
Account::<T>::mutate(k, |a| a.data = some_data.unwrap_or_default());
Ok(result)
Expand Down