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

feat: [PRO-823] bind-nodes-executor-to-address #3987

Merged
merged 6 commits into from
Sep 18, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
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
2 changes: 1 addition & 1 deletion api/lib/src/queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ impl QueryApi {

Ok(self
.state_chain_client
.storage_map_entry::<pallet_cf_funding::BoundAddress<state_chain_runtime::Runtime>>(
.storage_map_entry::<pallet_cf_funding::BoundRedeemAddress<state_chain_runtime::Runtime>>(
block_hash,
&account_id,
)
Expand Down
9 changes: 8 additions & 1 deletion state-chain/pallets/cf-funding/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ benchmarks! {
let caller: T::AccountId = whitelisted_caller();
}:_(RawOrigin::Signed(caller.clone()), Default::default())
verify {
assert!(BoundAddress::<T>::contains_key(&caller));
assert!(BoundRedeemAddress::<T>::contains_key(&caller));
}

update_restricted_addresses {
Expand All @@ -204,5 +204,12 @@ benchmarks! {
let _ = call.dispatch_bypass_filter(T::EnsureGovernance::try_successful_origin().unwrap());
}

bind_executor_address {
let caller: T::AccountId = whitelisted_caller();
}:_(RawOrigin::Signed(caller.clone()), Default::default())
verify {
assert!(BoundExecutorAddress::<T>::contains_key(&caller));
}

impl_benchmark_test_suite!(Pallet, crate::mock::new_test_ext(), crate::mock::Test,);
}
63 changes: 58 additions & 5 deletions state-chain/pallets/cf-funding/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,11 @@ pub mod pallet {
#[pallet::storage]
pub type RedemptionTTLSeconds<T: Config> = StorageValue<_, u64, ValueQuery>;

/// Registered addresses for an executor.
#[pallet::storage]
pub type BoundExecutorAddress<T: Config> =
StorageMap<_, Blake2_128Concat, AccountId<T>, EthereumAddress, OptionQuery>;

/// List of restricted addresses
#[pallet::storage]
pub type RestrictedAddresses<T: Config> =
Expand All @@ -151,7 +156,7 @@ pub mod pallet {

/// Map of bound addresses for accounts.
#[pallet::storage]
pub type BoundAddress<T: Config> =
pub type BoundRedeemAddress<T: Config> =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately this will require a storage migration (if it's not empty) 🙈

I don't mind leaving it out until we release - right now the storage is empty on perseverance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mhhh.... Well that's bad :D. But I think we will release from scratch anyway and if it's empty on perseverance I don't think it should be an issue or?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes that's what I mean - no need to migrate unless we use this in perseverance (and I don't think we will).

StorageMap<_, Blake2_128Concat, AccountId<T>, EthereumAddress>;

/// The fee levied for every redemption request. Can be updated by Governance.
Expand Down Expand Up @@ -233,6 +238,9 @@ pub mod pallet {

/// An account has been bound to an address.
BoundRedeemAddress { account_id: AccountId<T>, address: EthereumAddress },

/// An account has been bound to an executor address.
BoundExecutorAddress { account_id: AccountId<T>, address: EthereumAddress },
}

#[pallet::error]
Expand Down Expand Up @@ -288,6 +296,12 @@ pub mod pallet {

/// Stop Bidding is disabled due to Safe Mode.
StopBiddingDisabled,

/// Wrong executor address
ExecutorBindingRestrictionViolated,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: please update the comment too - it's included in the type metadata and displayed to users, it should be useful. Something like The executor for this account is bound to another address.


/// The account is already bound to an executor address.
ExecutorAddressAlreadyBound,
}

#[pallet::call]
Expand Down Expand Up @@ -363,6 +377,13 @@ pub mod pallet {
) -> DispatchResultWithPostInfo {
let account_id = ensure_signed(origin)?;

if let Some(executor_addr) = BoundExecutorAddress::<T>::get(&account_id) {
ensure!(
executor_addr == executor.unwrap_or_default(),
Error::<T>::ExecutorBindingRestrictionViolated
);
}

ensure!(T::SafeMode::get().redeem_enabled, Error::<T>::RedeemDisabled);

// Not allowed to redeem if we are an active bidder in the auction phase
Expand All @@ -379,7 +400,7 @@ pub mod pallet {
let mut restricted_balances = RestrictedBalances::<T>::get(&account_id);
let redemption_fee = RedemptionTax::<T>::get();

if let Some(bound_address) = BoundAddress::<T>::get(&account_id) {
if let Some(bound_address) = BoundRedeemAddress::<T>::get(&account_id) {
ensure!(
bound_address == address ||
restricted_balances.keys().any(|res_address| res_address == &address),
Expand Down Expand Up @@ -621,7 +642,7 @@ pub mod pallet {
///
/// - [BadOrigin](frame_support::error::BadOrigin)
#[pallet::call_index(7)]
#[pallet::weight(T::WeightInfo::update_restricted_addresses(addresses_to_add.len() as u32, addresses_to_remove.len() as u32))]
#[pallet::weight(T::WeightInfo::update_restricted_addresses(addresses_to_add.len() as u32, addresses_to_remove.len() as u32, 10_u32))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems to be a little odd, but I don't think that there is really a better way to do it. We can not access the real amount of RestrictedAddresses without decoding the storage, which would lead to an open door for DDOS attacks (I think). I think the trick is to benchmark it correctly and then pick a number that is higher than the maximum number of RestrictedAddresses for an account but not extraordinarily high to waste unnecessary transaction fees.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I just picked 10 🤷‍♂️

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, sounds reasonable.

pub fn update_restricted_addresses(
origin: OriginFor<T>,
addresses_to_add: Vec<EthereumAddress>,
Expand Down Expand Up @@ -660,8 +681,11 @@ pub mod pallet {
address: EthereumAddress,
) -> DispatchResultWithPostInfo {
let account_id = ensure_signed(origin)?;
ensure!(!BoundAddress::<T>::contains_key(&account_id), Error::<T>::AccountAlreadyBound);
BoundAddress::<T>::insert(&account_id, address);
ensure!(
!BoundRedeemAddress::<T>::contains_key(&account_id),
Error::<T>::AccountAlreadyBound
);
BoundRedeemAddress::<T>::insert(&account_id, address);
Self::deposit_event(Event::BoundRedeemAddress { account_id, address });
Ok(().into())
}
Expand All @@ -682,6 +706,35 @@ pub mod pallet {
Self::deposit_event(Event::<T>::RedemptionTaxAmountUpdated { amount });
Ok(())
}

/// Binds executor address to an account.
///
/// ## Events
///
/// - [BoundExecutorAddress](Event::BoundExecutorAddress)
///
/// ## Errors
///
/// - [ExecutorAddressAlreadyBound](Error::ExecutorAddressAlreadyBound)
/// - [BadOrigin](frame_support::error::BadOrigin)
#[pallet::call_index(10)]
#[pallet::weight(T::WeightInfo::bind_executor_address())]
pub fn bind_executor_address(
origin: OriginFor<T>,
executor_address: EthereumAddress,
) -> DispatchResultWithPostInfo {
let account_id = ensure_signed(origin)?;
ensure!(
!BoundExecutorAddress::<T>::contains_key(&account_id),
Error::<T>::ExecutorAddressAlreadyBound,
);
BoundExecutorAddress::<T>::insert(account_id.clone(), executor_address);
Self::deposit_event(Event::BoundExecutorAddress {
account_id,
address: executor_address,
});
Ok(().into())
}
}

#[pallet::genesis_config]
Expand Down
53 changes: 46 additions & 7 deletions state-chain/pallets/cf-funding/src/tests.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{
mock::*, pallet, ActiveBidder, Error, EthereumAddress, PendingRedemptions, RedemptionAmount,
RedemptionTax, RestrictedAddresses, RestrictedBalances,
mock::*, pallet, ActiveBidder, BoundExecutorAddress, Error, EthereumAddress,
PendingRedemptions, RedemptionAmount, RedemptionTax, RestrictedAddresses, RestrictedBalances,
};
use cf_primitives::FlipBalance;
use cf_test_utilities::assert_event_sequence;
Expand All @@ -10,7 +10,7 @@ use cf_traits::{
};
use sp_core::H160;

use crate::BoundAddress;
use crate::BoundRedeemAddress;
use frame_support::{assert_noop, assert_ok};
use pallet_cf_flip::Bonder;
use sp_runtime::{traits::BadOrigin, DispatchError};
Expand Down Expand Up @@ -862,7 +862,7 @@ fn can_only_redeem_funds_to_bound_address() {
const UNRESTRICTED_ADDRESS: EthereumAddress = H160([0x03; 20]);
const AMOUNT: u128 = 100;
RestrictedAddresses::<Test>::insert(RESTRICTED_ADDRESS_1, ());
BoundAddress::<Test>::insert(ALICE, BOUND_ADDRESS);
BoundRedeemAddress::<Test>::insert(ALICE, BOUND_ADDRESS);
assert_ok!(Funding::funded(
RuntimeOrigin::root(),
ALICE,
Expand Down Expand Up @@ -903,7 +903,7 @@ fn redeem_funds_until_restricted_balance_is_zero_and_then_redeem_to_redeem_addre
const UNRESTRICTED_ADDRESS: EthereumAddress = H160([0x03; 20]);
const AMOUNT: u128 = 100;
RestrictedAddresses::<Test>::insert(RESTRICTED_ADDRESS, ());
BoundAddress::<Test>::insert(ALICE, REDEEM_ADDRESS);
BoundRedeemAddress::<Test>::insert(ALICE, REDEEM_ADDRESS);
assert_ok!(Funding::funded(
RuntimeOrigin::root(),
ALICE,
Expand Down Expand Up @@ -1277,8 +1277,8 @@ fn can_bind_redeem_address() {
address,
}) if address == REDEEM_ADDRESS,
);
assert!(BoundAddress::<Test>::contains_key(ALICE));
assert_eq!(BoundAddress::<Test>::get(ALICE).unwrap(), REDEEM_ADDRESS);
assert!(BoundRedeemAddress::<Test>::contains_key(ALICE));
assert_eq!(BoundRedeemAddress::<Test>::get(ALICE).unwrap(), REDEEM_ADDRESS);
});
}

Expand Down Expand Up @@ -1467,3 +1467,42 @@ fn check_restricted_balances_are_getting_removed() {
assert!(RestrictedBalances::<Test>::get(ALICE).get(&RESTRICTED_ADDRESS).is_none());
});
}

#[test]
fn bind_executor_address() {
new_test_ext().execute_with(|| {
const EXECUTOR_ADDRESS: EthereumAddress = H160([0x01; 20]);
assert_ok!(Funding::bind_executor_address(RuntimeOrigin::signed(ALICE), EXECUTOR_ADDRESS));
assert_event_sequence!(
Test,
RuntimeEvent::Funding(crate::Event::BoundExecutorAddress {
account_id: ALICE,
address,
}) if address == EXECUTOR_ADDRESS,
);
assert!(BoundExecutorAddress::<Test>::contains_key(ALICE));
assert_eq!(BoundExecutorAddress::<Test>::get(ALICE).unwrap(), EXECUTOR_ADDRESS);
assert_noop!(
Funding::bind_executor_address(RuntimeOrigin::signed(ALICE), EXECUTOR_ADDRESS),
Error::<Test>::ExecutorAddressAlreadyBound
);
});
}

#[test]
fn detect_wrong_executor_address() {
new_test_ext().execute_with(|| {
const EXECUTOR_ADDRESS: EthereumAddress = H160([0x01; 20]);
const WRONG_EXECUTOR_ADDRESS: EthereumAddress = H160([0x02; 20]);
assert_ok!(Funding::bind_executor_address(RuntimeOrigin::signed(ALICE), EXECUTOR_ADDRESS));
assert_noop!(
Funding::redeem(
RuntimeOrigin::signed(ALICE),
100.into(),
ETH_DUMMY_ADDR,
Some(WRONG_EXECUTOR_ADDRESS)
),
Error::<Test>::ExecutorBindingRestrictionViolated
);
});
}
Loading