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

Add ExistenceRequirement to Currency trait #4000

Merged
merged 12 commits into from
Nov 7, 2019
106 changes: 70 additions & 36 deletions srml/balances/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,23 @@ decl_module! {
let dest = T::Lookup::lookup(dest)?;
<Self as Currency<_>>::transfer(&source, &dest, value)?;
}

/// Transfer some liquid free balance to another account, while checking that the transfer
/// will not kill the account.
///
/// 99% of the time you want `transfer` instead.
gui1117 marked this conversation as resolved.
Show resolved Hide resolved
#[weight = SimpleDispatchInfo::FixedNormal(1_000_000)]
pub fn transfer_some(
gui1117 marked this conversation as resolved.
Show resolved Hide resolved
origin,
dest: <T::Lookup as StaticLookup>::Source,
#[compact] value: T::Balance
) {
let transactor = ensure_signed(origin)?;
let dest = T::Lookup::lookup(dest)?;

Self::transfer_inner(&transactor, &dest, value, ExistenceRequirement::KeepAlive)?;
}

}
}

Expand Down Expand Up @@ -852,42 +869,7 @@ where
}

fn transfer(transactor: &T::AccountId, dest: &T::AccountId, value: Self::Balance) -> Result {
let from_balance = Self::free_balance(transactor);
let to_balance = Self::free_balance(dest);
let would_create = to_balance.is_zero();
let fee = if would_create { T::CreationFee::get() } else { T::TransferFee::get() };
let liability = match value.checked_add(&fee) {
Some(l) => l,
None => return Err("got overflow after adding a fee to value"),
};

let new_from_balance = match from_balance.checked_sub(&liability) {
None => return Err("balance too low to send value"),
Some(b) => b,
};
if would_create && value < T::ExistentialDeposit::get() {
return Err("value too low to create account");
}
Self::ensure_can_withdraw(transactor, value, WithdrawReason::Transfer.into(), new_from_balance)?;

// NOTE: total stake being stored in the same type means that this could never overflow
// but better to be safe than sorry.
let new_to_balance = match to_balance.checked_add(&value) {
Some(b) => b,
None => return Err("destination balance too high to receive value"),
};

if transactor != dest {
Self::set_free_balance(transactor, new_from_balance);
if !<FreeBalance<T, I>>::exists(dest) {
Self::new_account(dest, new_to_balance);
}
Self::set_free_balance(dest, new_to_balance);
T::TransferPayment::on_unbalanced(NegativeImbalance::new(fee));
Self::deposit_event(RawEvent::Transfer(transactor.clone(), dest.clone(), value, fee));
}

Ok(())
Self::transfer_inner(transactor, dest, value, ExistenceRequirement::AllowDeath)
}

fn withdraw(
Expand Down Expand Up @@ -1151,3 +1133,55 @@ where
Self::total_balance(who).is_zero()
}
}

impl<T: Trait<I>, I: Instance> Module<T, I> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not in impl Currency?

impl<T: Trait<I>, I: Instance> Currency<T::AccountId> for Module<T, I>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to hassle Currency implementors with writing an implementation of transfer_inner/transfer_some.

Copy link
Contributor

Choose a reason for hiding this comment

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

we could change transfer signature of Currency trait to:

	fn transfer(transactor: &T::AccountId, dest: &T::AccountId, value: Self::Balance, liveness: ExistenceRequirement) -> Result {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that we should try to avoid breaking anything if we can. See @xlc's first comment in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Xlc comment is about not changing the Call. We can still have transfer and transfer_keep_alive in Call and have transfer(...., liveness: ExistenceRequirement) in Currency trait.

I feel generalizing the trait here doesn't harm

Copy link
Member

Choose a reason for hiding this comment

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

Depends if anyone else has implemented anything on it - third parties might have done. That said, if it is to be changed, now is the time to do it.

fn transfer_inner(
transactor: &T::AccountId,
dest: &T::AccountId,
value: T::Balance,
existential_requirement: ExistenceRequirement,
expenses marked this conversation as resolved.
Show resolved Hide resolved
) -> Result {
let from_balance = Self::free_balance(transactor);
let to_balance = Self::free_balance(dest);
let would_create = to_balance.is_zero();
let fee = if would_create { T::CreationFee::get() } else { T::TransferFee::get() };
let liability = match value.checked_add(&fee) {
Some(l) => l,
None => return Err("got overflow after adding a fee to value"),
};

let new_from_balance = match from_balance.checked_sub(&liability) {
None => return Err("balance too low to send value"),
Some(b) => b,
};
if would_create && value < T::ExistentialDeposit::get() {
return Err("value too low to create account");
}
Self::ensure_can_withdraw(transactor, value, WithdrawReason::Transfer.into(), new_from_balance)?;

// NOTE: total stake being stored in the same type means that this could never overflow
// but better to be safe than sorry.
let new_to_balance = match to_balance.checked_add(&value) {
Some(b) => b,
None => return Err("destination balance too high to receive value"),
};

if transactor != dest {
if existential_requirement == ExistenceRequirement::KeepAlive {
if new_from_balance < Self::minimum_balance() {
return Err("payment would kill account");
}
}

Self::set_free_balance(transactor, new_from_balance);
if !<FreeBalance<T, I>>::exists(dest) {
Self::new_account(dest, new_to_balance);
}
Self::set_free_balance(dest, new_to_balance);
T::TransferPayment::on_unbalanced(NegativeImbalance::new(fee));
Self::deposit_event(RawEvent::Transfer(transactor.clone(), dest.clone(), value, fee));
}

Ok(())
}
}