paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

Replace `EnsureAccountLiquid` with a centralised store of illiquidity #1896

Closed xlc closed 5 years ago

xlc commented 5 years ago

It is not observe to me how to query user free liquid balance easily. Balances module have free balance and reserved balance. Staking module and Democracy module have ability to mark all user's balance illiquid. With upcoming #1782 it seems able to mark partially of the balance illiquid.

So the frontend code needs to query 3 different modules to know if a user is able to spend his money. This will be worse if I need a custom module also have ability to stake user's fund in order to unlock certain operations.

It will be good if there is a centralized logic to query user's liquid balance, and better, user's balance status. e.g. 500 spendable DOT, 500 DOT spendable after 200 block, 500 DOT non-spendable until unstaked. This means instead of balances module ask other modules if it can transfer user's fund, it should be other modules freeze user's fund via Currency trait. Isn't this is the purpose of reserved_balance?

gavofyork commented 5 years ago

Isn't this is the purpose of reserved_balance?

No. Reserved balance is essentially an alternative to temporarily seizing the balance from the user. The only difference is that it ensures it's still available to the slashing system to be slashed, to any token-voting systems, and guarantees that no more can be returned than what was seized. Crucially, balance reservations are mutually exclusive. If your account's total balance is reserved, then it cannot be reserved again until the first reservation is unreserved. Additionally, reserved balances can never be used to pay for transactions.

The notion of liquidity, as defined by EnsureAccountLiquid trait is quite different. It can be used to prevent the balance from transfer, or to prevent the balance being used to pay for transactions, or being used for a reserve (or any combination thereof). Crucially, the same balance can be made illiquid by different parts of the runtime for different reasons.

Indeed the UI will need to consolidate multiple modules' worth of information to give a full picture of the status of a balance. Balances is the obvious candidate for such a centralised module and such functionality might be reasonable to introduce next to the balance reservation functionality. The API would need some consideration in order to ensure it's general enough to replace EnsureAccountLiquid in all use cases. Looking at thee code though, I think it should be possible.

gavofyork commented 5 years ago

Would probably be a storage item like:

pub struct BalanceLock {
  amount: Balance,
  until: BlockNumber,
  reasons: Vec<WithdrawReason>,
}
Locks get(locks): map AccountId => Vec<BalanceLock>;

and a utility function to query:

fn can_withdraw(who: &AccountId, amount: Balance, reason: WithdrawReason) -> bool {
  let now = <system::Module<T>>::block_number();
  let total = Self::free_balance(who);
  Self::locks(who).into_iter().all(|l| now >= l.until || total >= l.amount + amount || !reasons.contains(reason))
}

and another to introduce an entry and clean up stale entries:

fn lockup(who: &AccountId, amount: Balance, reasons: Vec<WithdrawReason>) {
  let now = <system::Module<T>>::block_number();
  <Locks<T>>::mutate(who, |locks| { locks = locks.into_iter().filter(|l| l.until > now).collect(); locks.push(BalanceLock { who: who.clone(), amount, reasons }); });
}

I don't think there's any need to remove locks, but if there are then it may be necessary to introduce a module identifier so they can be easily and unambiguously identified.

xlc commented 5 years ago

It will be good if there is module identifier support. I am currently use hash of the module name and convert it into AccoundId and use it as the module identifier. But it requires redefine AccountId with additional trait bonds and a bit messy.