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

Refactor transaction storage pallet to use fungible traits #1800

Merged
merged 12 commits into from
Oct 30, 2023
Merged

Refactor transaction storage pallet to use fungible traits #1800

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
362e261
frame/transaction-storage: replace Currency with fungible traits
acatangiu Oct 7, 2023
0bf9b61
Merge branch 'master' of github.com:paritytech/polkadot-sdk into fram…
acatangiu Oct 10, 2023
00e6fc7
address review comments
acatangiu Oct 10, 2023
c10caac
fix benchmarks
acatangiu Oct 10, 2023
d716906
Merge branch 'master' of github.com:paritytech/polkadot-sdk into fram…
acatangiu Oct 30, 2023
820a279
fix merge damage
acatangiu Oct 30, 2023
6c0c9c4
address review comments
acatangiu Oct 30, 2023
e8bf46a
kitchensink-runtime: bump MaxHolds to account for newly introduced ho…
acatangiu Oct 30, 2023
17addaa
fix incomplete rename
acatangiu Oct 30, 2023
11fa7ca
Update substrate/frame/transaction-storage/src/lib.rs
acatangiu Oct 30, 2023
5248376
fix warning
acatangiu Oct 30, 2023
27f6999
Merge branch 'master' of github.com:paritytech/polkadot-sdk into fram…
acatangiu Oct 30, 2023
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
3 changes: 2 additions & 1 deletion substrate/bin/node/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ impl pallet_balances::Config for Runtime {
type WeightInfo = pallet_balances::weights::SubstrateWeight<Runtime>;
type FreezeIdentifier = RuntimeFreezeReason;
type MaxFreezes = ConstU32<1>;
type MaxHolds = ConstU32<5>;
type MaxHolds = ConstU32<6>;
}

parameter_types! {
Expand Down Expand Up @@ -1833,6 +1833,7 @@ impl pallet_nfts::Config for Runtime {
impl pallet_transaction_storage::Config for Runtime {
type RuntimeEvent = RuntimeEvent;
type Currency = Balances;
type RuntimeHoldReason = RuntimeHoldReason;
type RuntimeCall = RuntimeCall;
type FeeDestination = ();
type WeightInfo = pallet_transaction_storage::weights::SubstrateWeight<Runtime>;
Expand Down
16 changes: 8 additions & 8 deletions substrate/frame/transaction-storage/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@

use super::*;
use frame_benchmarking::v1::{benchmarks, whitelisted_caller};
use frame_support::traits::{Currency, Get, OnFinalize, OnInitialize};
use frame_support::traits::{Get, OnFinalize, OnInitialize};
use frame_system::{pallet_prelude::BlockNumberFor, EventRecord, Pallet as System, RawOrigin};
use sp_runtime::traits::{Bounded, One, Zero};
use sp_runtime::traits::{Bounded, CheckedDiv, One, Zero};
use sp_std::*;
use sp_transaction_storage_proof::TransactionStorageProof;

Expand Down Expand Up @@ -103,9 +103,6 @@ fn proof() -> Vec<u8> {
array_bytes::hex2bytes_unchecked(PROOF)
}

type BalanceOf<T> =
<<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;

fn assert_last_event<T: Config>(generic_event: <T as Config>::RuntimeEvent) {
let events = System::<T>::events();
let system_event: <T as frame_system::Config>::RuntimeEvent = generic_event.into();
Expand All @@ -129,7 +126,8 @@ benchmarks! {
store {
let l in 1 .. T::MaxTransactionSize::get();
let caller: T::AccountId = whitelisted_caller();
T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
let initial_balance = BalanceOf::<T>::max_value().checked_div(&2u32.into()).unwrap();
acatangiu marked this conversation as resolved.
Show resolved Hide resolved
T::Currency::set_balance(&caller, initial_balance);
}: _(RawOrigin::Signed(caller.clone()), vec![0u8; l as usize])
verify {
assert!(!BlockTransactions::<T>::get().is_empty());
Expand All @@ -138,7 +136,8 @@ benchmarks! {

renew {
let caller: T::AccountId = whitelisted_caller();
T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
let initial_balance = BalanceOf::<T>::max_value().checked_div(&2u32.into()).unwrap();
acatangiu marked this conversation as resolved.
Show resolved Hide resolved
T::Currency::set_balance(&caller, initial_balance);
TransactionStorage::<T>::store(
RawOrigin::Signed(caller.clone()).into(),
vec![0u8; T::MaxTransactionSize::get() as usize],
Expand All @@ -152,7 +151,8 @@ benchmarks! {
check_proof_max {
run_to_block::<T>(1u32.into());
let caller: T::AccountId = whitelisted_caller();
T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
let initial_balance = BalanceOf::<T>::max_value().checked_div(&2u32.into()).unwrap();
acatangiu marked this conversation as resolved.
Show resolved Hide resolved
T::Currency::set_balance(&caller, initial_balance);
for _ in 0 .. T::MaxBlockTransactions::get() {
TransactionStorage::<T>::store(
RawOrigin::Signed(caller.clone()).into(),
Expand Down
38 changes: 26 additions & 12 deletions substrate/frame/transaction-storage/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,14 @@ mod tests;
use codec::{Decode, Encode, MaxEncodedLen};
use frame_support::{
dispatch::GetDispatchInfo,
traits::{Currency, OnUnbalanced, ReservableCurrency},
traits::{
fungible::{
hold::Balanced as FnBalanced, Inspect as FnInspect, Mutate as FnMutate,
MutateHold as FnMutateHold,
},
tokens::fungible::Credit,
OnUnbalanced,
},
};
use sp_runtime::traits::{BlakeTwo256, Dispatchable, Hash, One, Saturating, Zero};
use sp_std::{prelude::*, result};
Expand All @@ -42,10 +49,8 @@ use sp_transaction_storage_proof::{

/// A type alias for the balance type from this pallet's point of view.
type BalanceOf<T> =
<<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;
type NegativeImbalanceOf<T> = <<T as Config>::Currency as Currency<
<T as frame_system::Config>::AccountId,
>>::NegativeImbalance;
<<T as Config>::Currency as FnInspect<<T as frame_system::Config>::AccountId>>::Balance;
pub type CreditOf<T> = Credit<<T as frame_system::Config>::AccountId, <T as Config>::Currency>;

// Re-export pallet items so that they can be accessed from the crate namespace.
pub use pallet::*;
Expand Down Expand Up @@ -89,6 +94,13 @@ pub mod pallet {
use frame_support::pallet_prelude::*;
use frame_system::pallet_prelude::*;

/// A reason for this pallet placing a hold on funds.
#[pallet::composite_enum]
pub enum HoldReason {
/// The funds are held as deposit for the used storage.
StorageFeeHold,
}

#[pallet::config]
pub trait Config: frame_system::Config {
/// The overarching event type.
Expand All @@ -98,10 +110,14 @@ pub mod pallet {
+ Dispatchable<RuntimeOrigin = Self::RuntimeOrigin>
+ GetDispatchInfo
+ From<frame_system::Call<Self>>;
/// The currency trait.
type Currency: ReservableCurrency<Self::AccountId>;
/// The fungible type for this pallet.
type Currency: FnMutate<Self::AccountId>
+ FnMutateHold<Self::AccountId, Reason = Self::RuntimeHoldReason>
+ FnBalanced<Self::AccountId>;
/// The overarching runtime hold reason.
acatangiu marked this conversation as resolved.
Show resolved Hide resolved
type RuntimeHoldReason: From<HoldReason>;
/// Handler for the unbalanced decrease when fees are burned.
type FeeDestination: OnUnbalanced<NegativeImbalanceOf<Self>>;
type FeeDestination: OnUnbalanced<CreditOf<Self>>;
/// Weight information for extrinsics in this pallet.
type WeightInfo: WeightInfo;
/// Maximum number of indexed transactions in the block.
Expand All @@ -112,8 +128,6 @@ pub mod pallet {

#[pallet::error]
pub enum Error<T> {
/// Insufficient account balance.
InsufficientFunds,
/// Invalid configuration.
NotConfigured,
/// Renewed extrinsic is not found.
Expand Down Expand Up @@ -432,8 +446,8 @@ pub mod pallet {
let byte_fee = ByteFee::<T>::get().ok_or(Error::<T>::NotConfigured)?;
let entry_fee = EntryFee::<T>::get().ok_or(Error::<T>::NotConfigured)?;
let fee = byte_fee.saturating_mul(size.into()).saturating_add(entry_fee);
ensure!(T::Currency::can_slash(&sender, fee), Error::<T>::InsufficientFunds);
let (credit, _) = T::Currency::slash(&sender, fee);
T::Currency::hold(&HoldReason::StorageFeeHold.into(), &sender, fee)?;
let (credit, _) = T::Currency::slash(&HoldReason::StorageFeeHold.into(), &sender, fee);
Copy link
Contributor

@liamaharon liamaharon Oct 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Seems logically equivalent to just reducing the account balance by fee if the balance is gt fee (which seems way simpler)? Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if equivalent, the imbalance/credit does need to go to T::FeeDestination, but maybe this can be changed too.

@paritytech/frame-coders may have a stronger opinion which way to go here.

Copy link
Contributor

@juangirini juangirini Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You could very well use decrease_balance() which returns the reduced balance. I don't see why that wouldn't work.
Though, I don't know why it was not used in the original code

Disregard my previous comment, I think @acatangiu you are right, we do want slash to manage the imbalance and T::FeeDestination to do whatever it needs to be done for us.

decrease_balance() should not be used directly for that reason.

polkadot-sdk/substrate/frame/support/src/traits/tokens/fungible/regular.rs

Lines 122 to 127 in 21d36b7

/// **WARNING**
/// Do not use this directly unless you want trouble, since it allows you to alter account balances
/// without keeping the issuance up to date. It has no safeguards against accidentally creating
/// token imbalances in your system leading to accidental inflation or deflation. It's really just
/// for the underlying datatype to implement so the user gets the much safer `Balanced` trait to
/// use.

acatangiu marked this conversation as resolved.
Show resolved Hide resolved
T::FeeDestination::on_unbalanced(credit);
Ok(())
}
Expand Down
52 changes: 14 additions & 38 deletions substrate/frame/transaction-storage/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,11 @@ use crate::{
self as pallet_transaction_storage, TransactionStorageProof, DEFAULT_MAX_BLOCK_TRANSACTIONS,
DEFAULT_MAX_TRANSACTION_SIZE,
};
use frame_support::traits::{ConstU16, ConstU32, ConstU64, OnFinalize, OnInitialize};
use sp_core::H256;
use sp_runtime::{
traits::{BlakeTwo256, IdentityLookup},
BuildStorage,
use frame_support::{
derive_impl,
traits::{ConstU32, ConstU64, OnFinalize, OnInitialize},
};
use sp_runtime::{traits::IdentityLookup, BuildStorage};

pub type Block = frame_system::mocking::MockBlock<Test>;

Expand All @@ -37,58 +36,35 @@ frame_support::construct_runtime!(
System: frame_system::{Pallet, Call, Config<T>, Storage, Event<T>},
Balances: pallet_balances::{Pallet, Call, Config<T>, Storage, Event<T>},
TransactionStorage: pallet_transaction_storage::{
Pallet, Call, Storage, Config<T>, Inherent, Event<T>
Pallet, Call, Storage, Config<T>, Inherent, Event<T>, HoldReason
},
}
);

#[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)]
impl frame_system::Config for Test {
type BaseCallFilter = frame_support::traits::Everything;
type BlockWeights = ();
type BlockLength = ();
type RuntimeOrigin = RuntimeOrigin;
type RuntimeCall = RuntimeCall;
type Nonce = u64;
type Hash = H256;
type Hashing = BlakeTwo256;
type AccountId = u64;
type Lookup = IdentityLookup<Self::AccountId>;
type Block = Block;
type RuntimeEvent = RuntimeEvent;
type BlockHashCount = ConstU64<250>;
type DbWeight = ();
type Version = ();
type PalletInfo = PalletInfo;
type AccountData = pallet_balances::AccountData<u64>;
type OnNewAccount = ();
type OnKilledAccount = ();
type SystemWeightInfo = ();
type SS58Prefix = ConstU16<42>;
type OnSetCode = ();
type MaxConsumers = ConstU32<16>;
type AccountId = u64;
type BlockHashCount = ConstU64<250>;
type Lookup = IdentityLookup<Self::AccountId>;
}

#[derive_impl(pallet_balances::config_preludes::TestDefaultConfig as pallet_balances::DefaultConfig)]
acatangiu marked this conversation as resolved.
Show resolved Hide resolved
impl pallet_balances::Config for Test {
type Balance = u64;
type DustRemoval = ();
type RuntimeEvent = RuntimeEvent;
type ExistentialDeposit = ConstU64<1>;
type AccountStore = System;
type WeightInfo = ();
type MaxLocks = ();
type MaxReserves = ();
type ReserveIdentifier = ();
type FreezeIdentifier = ();
type MaxFreezes = ();
type RuntimeHoldReason = ();
type RuntimeFreezeReason = ();
type MaxHolds = ();
type RuntimeHoldReason = RuntimeHoldReason;
type RuntimeFreezeReason = RuntimeFreezeReason;
type MaxHolds = ConstU32<128>;
}

impl pallet_transaction_storage::Config for Test {
type RuntimeEvent = RuntimeEvent;
type RuntimeCall = RuntimeCall;
type Currency = Balances;
type RuntimeHoldReason = RuntimeHoldReason;
type FeeDestination = ();
type WeightInfo = ();
type MaxBlockTransactions = ConstU32<{ DEFAULT_MAX_BLOCK_TRANSACTIONS }>;
Expand Down
3 changes: 2 additions & 1 deletion substrate/frame/transaction-storage/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use super::{Pallet as TransactionStorage, *};
use crate::mock::*;
use frame_support::{assert_noop, assert_ok};
use frame_system::RawOrigin;
use sp_runtime::{DispatchError, TokenError::FundsUnavailable};
use sp_transaction_storage_proof::registration::build_proof;

const MAX_DATA_SIZE: u32 = DEFAULT_MAX_TRANSACTION_SIZE;
Expand Down Expand Up @@ -71,7 +72,7 @@ fn burns_fee() {
RawOrigin::Signed(5).into(),
vec![0u8; 2000 as usize]
),
Error::<Test>::InsufficientFunds,
DispatchError::Token(FundsUnavailable),
);
assert_ok!(TransactionStorage::<Test>::store(
RawOrigin::Signed(caller).into(),
Expand Down