decouple balances from some modules

[gui1117]

related to #1514

This is a first raw implementation of a decoupling of balances from other modules.

What is done

• modification of decl_event! to be able to use some more complex generic types in events. It can now handle all type for named generics ( Name = ty ) and if it fails to handle unnamed ones then it compile with a message advising to name it.
• creation of traits in sr-primitives TransferToken and Staking and some modules now use a generic type that is bond to one of them or both.

so that works

what is to be done

• Traits are very inconsistent TransferToken include lots of fee related methods, Staking have slash related methods and total balance.
• We have to abstract also OnDilution and OnFreeBalanceZero traits that are in balances crate.
• The abstraction should stay in srml not in core isn't it ?
• Some associated type names are not good like Staking::Staking...

[xlc]

Good to see some work has been done on this area as well. Me and my colleagues are working on #1515 which may have some overlapping with this https://github.com/xlc/substrate/tree/fee

[xlc]

I think we should keep this trait as simple as possible. Many methods here are internal/private methods.
This is my take: https://github.com/paritytech/substrate/pull/1648/files#diff-cfb20bdf2f3264df022885d7fe2de6a7R147

We should agree on a name to reduce upcoming conflicts...
TransferToken or TransferAsset?
Balance or Amount?

[gui1117]

Thanks, I have very few knowledge on blockchain names. I would be a bit more in favor of Balance than Amount as it characterize the type. But I won't argue at all, frankly I'm following your choice on that.

Yes the trait is bad in the current impl

Also if it happens you need the decl_event! modification I could make it in another PR

[gui1117]

what kind of internal private methods were you actually thinking of ?

[xlc]

I don't see why other modules will want to call increase_total_stake_by (that's increase total issuance, nothing to do with staking).
Why anyone other than balances module needs to be aware of the different between set_free_balance_creating and set_free_balance?
I see many methods needs to be exposed only because contract module needs to reimplements the same logic with its overlay account db, if we can improve and avoid this duplication some how (which I don't have a good plan), then all the read config methods should be private (e.g. creation_fee and transfer_fee)

[gui1117]

Yes some methods seems private but it is because of contract specific requirements: This are the reasons:

• it uses an overlay: therefore it needs access to existencial_deposit, ensure_account_liquid, free_balance
  Maybe the overlay should be provided in the trait TransferAsset or in a new one TransferAssetOverlay: TransferAsset {...}

• transfer fees are payed in Gas instead of Balance: therefore it needs access to

  • creation_fee and transfer_fee to actually compute fees
  • set_free_balance_creating to be able to modify free_balance without fee

• buy_gas: it needs to be able to return Error without buying anything when the account would have died. To do so it needs existencial_deposit, free_balance.
  Something like can_slash_without_die(...) -> bool or simulate_transfer_from(...) -> UpdateBalanceOutCome would make it. I'm not very used to naming in substrate yet.

• transaction_byte_fee and transaction_base_fee are used to implement DispatchFeeComputor for DefaultDispatchFeeComputor: It seems this could easily use xlc trait ChargeBytesFee<AccountId>

[gui1117]

Today I was thinking about what to expose in the trait, here some thoughts:

• All modules except contract can work with a quite high level interface on balance, only treasury needs a new method that could be reward_with_no_stake_increase, this interface it: (it would be completed a bit)

  can_slash
  repatriate_reserved
  reserve
  reward
  reward_with_no_stake_increase
  slash
  slash_reserved
  total_balance
  total_issuance

• Contract actually use the low level methods on balance. Abstracting those methods may be a bit hard. But my worry are that the abstraction resulting may not fit all usage at all and some other module would need those low level methods or a new quite specific abstraction.
  Thus I wonder: Does the methods currently exposed by balance have to be abstracted ? I mean: does UTXO fits inside ? (I don't know at all yet), and does multitoken assets ? (next point)
  If the current API works we can make the interface as such.

• About multitoken assets: in the current implementation contract could use one balances and another module another one. But balances module is only one balances. To make it being able to be used in multiple form we can make it generic over a type with a default like this:
  (but for that we need to modify decl_module! a bit)

pub struct DefaultTokenKind;
pub struct AlternativeToken0
pub struct AlternativeToken1
pub struct AlternativeToken2
pub struct AlternativeToken3
...

decl_module! {
	pub struct Module<T: Trait, M=DefaultTokenKind> for enum Call where origin: T::Origin {
  }
}

/////////////////////// then used like ///////////////////////

impl staking::Trait for Runtime {
	type Staking = balances::Module<Self>;
	type OnRewardMinted = Treasury;
	type Event = Event;
}

impl democracy::Trait for Runtime {
	type Staking = balances::Module<Self, AlternativeToken0>;
	type Proposal = Call;
	type Event = Event;
}

[gavofyork]

It would be cool to have parameterisable modules (i.e. to use the same module twice under different configurations).

I think it may make sense to have contracts module require something much more closely resembling balances than other modules (staking, democracy, treasury) need. I'm not sure if this particular contracts module really makes much sense un a UTXO chain.

[xlc]

It seems like it is hard to abstract away the coupling between contract module and balances module without some major refactoring work to been done on contract module to somehow avoid the duplicated balances accounts logic.
It may be worth while to leave that part in this PR to ensure the scope is limited and we don't provide a leaky abstraction.

A potential solution for contract - balances coupling issue is introduce a new OverlayTransferToken trait that implements TransferToken trait and provides some additional method commit, make it suitable for contract module.
One goal I would like to (have to) achieve is make contract module to support multi token in the sense that its transfer method can transfer any kind of the supported tokens. I will want to able to provide an alternative implementation of this OverlayTransferToken which use our generic asset module to implement the semantic of the OverlayAccountDb.

[gui1117]

OK I do agree, contract could be decoupled in another PR, OverlayTransferToken is a good direction but doesn't solve everything as some are related to gas use for fees.

current state of PR:

• modification of decl_event! see first post
• creation of a coherent Staking trait replacing balances module in all modules except Contracts (so Treasury, Council, Democracy and Staking)
• move of OnDilution, OnFreeBalanceZero and EnsureAccountLiquid into sr-primtive/trait

TODO:

• I don't know if Staking, OnDilution, OnFreeBalanceZero and EnsureAccountLiquid fits in sr-primitive/trait, I would have expected them in srml in something like AbstractBalance module but I don't know.
• some methods of balances are now available through the Staking trait only. This may not be wanted

[gui1117]

Also here are some thoughs on abstracting contracts:

in the overlay I only need:

• transfer_fee_to(dest) -> Balance: return fees to transfer to it
• make_transfer_with_no_fee(who, dest, value) -> Result: make the actual transfer but do no fee
• free_balance(who) we must provide this function for the contracts, it is in their api.
• commit: do the actual transfers

to buy gas:

• can_slash_without_die: to be able not to buy gas in case it can't (could be add to staking)
• slash (already in staking)

to refund_gas:

• reward (already in staking)

to implement DefaultComputeDispatchFee

• base_byte_fee(len) -> Balance: still not sure about that part

[gavofyork]

looks good apart from the minor repotting.

[gavofyork]

The naming is rather funky.

[gui1117]

Yes indeed, I replaced Staking by Funding.

[gavofyork]

This appears to be a synonym for increase_free_balance_creating, which has a very important caveat in its documentation missing here:

	/// Adds up to `value` to the free balance of `who`. If `who` doesn't exist, it is created.
	///
	/// This is a sensitive function since it circumvents any fees associated with account
	/// setup. Ensure it is only called by trusted code.
	///
	/// NOTE: This assumes that the total stake remains unchanged after this operation. If
	/// you mean to actually mint value into existence, then use `reward` instead.
	pub fn increase_free_balance_creating(who: &T::AccountId, value: T::Balance) -> UpdateBalanceOutcome {
		Self::set_free_balance_creating(who, Self::free_balance(who) + value)
	}

[gavofyork]

I have used "reward" to mean a safe operation that doesn't need thinking about too deeply: no fees are skipped (the destination account doesn't exist, then the operation fails without introducing a potential crypto-economic flaw) and the total issued amount is increased.

I would prefer to keep it having that meaning, with the lower-level "increase_free_balance" functions having the accordingly less safe implications.

[gavofyork]

More naming issues.

[gavofyork]

@thiolliere please take a look at my changes. if all good, feel free to merge.

[gui1117]

don't you want to make the doc of increase_free_balance_creating more warrant ?
well the new naming and the note seems fine to me. I approve and will merge