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 3 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
7 changes: 7 additions & 0 deletions state-chain/pallets/cf-funding/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(ExecutorAddressBinding::<T>::contains_key(&caller));
}

impl_benchmark_test_suite!(Pallet, crate::mock::new_test_ext(), crate::mock::Test,);
}
52 changes: 51 additions & 1 deletion 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 ExecutorAddressBinding<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.

We now have BoundAddress and ExecutorAddressBinding.

We should make the names of these more consistent, to make it clearer that they are related concepts.

How about BoundRedeemAddress and BoundExecutorAddress? This also matches the event names.

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

/// List of restricted addresses
#[pallet::storage]
pub type RestrictedAddresses<T: Config> =
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
InvalidExecutorAddress,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can be more precise than just 'wrong'. We could use the same terminology as above, similar to AccountBindingRestrictionViolated -> ExecutorBindingRestrictionViolated for example.


/// 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) = ExecutorAddressBinding::<T>::get(&account_id) {
ensure!(
executor_addr == executor.unwrap_or_default(),
Error::<T>::InvalidExecutorAddress
);
}

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

// Not allowed to redeem if we are an active bidder in the auction phase
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 @@ -682,6 +703,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!(
!ExecutorAddressBinding::<T>::contains_key(&account_id),
Error::<T>::ExecutorAddressAlreadyBound,
);
ExecutorAddressBinding::<T>::insert(account_id.clone(), executor_address);
Self::deposit_event(Event::BoundExecutorAddress {
account_id,
address: executor_address,
});
Ok(().into())
}
}

#[pallet::genesis_config]
Expand Down
43 changes: 41 additions & 2 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, Error, EthereumAddress, ExecutorAddressBinding,
PendingRedemptions, RedemptionAmount, RedemptionTax, RestrictedAddresses, RestrictedBalances,
};
use cf_primitives::FlipBalance;
use cf_test_utilities::assert_event_sequence;
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!(ExecutorAddressBinding::<Test>::contains_key(ALICE));
assert_eq!(ExecutorAddressBinding::<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>::InvalidExecutorAddress
);
});
}
Loading