Revamp minting/burning handling #2028

gavofyork · 2019-03-18T07:17:39Z

This introduces hooks to allow runtimes to handle the various points that the SRML modules need to mint or burn currency differently.

Also:

Fix AnySignature;
Use AnySignature for substrate-node (supersedes Fix MultiSigner, simplify tests #2033);
Rename *_total_stake_by to *_total_issuance_by in Balances Module (supersedes Rename *_total_stake_by to *_total_issuance_by in Balances Module #2025)

gui1117 · 2019-03-18T10:20:44Z

srml/balances/src/lib.rs

 	fn on_free_too_low(who: &T::AccountId) {
-		Self::decrease_total_stake_by(Self::free_balance(who));
+		// underflow should never happen, but if it does, there's not much we can do about it.
+		let _ = Self::decrease_total_stake_by(Self::free_balance(who));


Maybe we could introduce a set of Events for this particular cases, like standard event in system, Underflow(String) and Overflow(String) with String being the name of the value ?

First, you most likely don't want to use String, since we strive to avoid it in the runtime logic.
Second, what use-cases do you see for this?

The idea is that it is better to be aware that a saturated overflow or underflow happens than it being silent

(I'm not sure about how much it is relevant)

if overflow/underflow happens, then it's a fundamental logic error/state corruption and governance or a hard fork is needed to fix it. it'll be obvious if it happens.

gui1117 · 2019-03-18T10:56:39Z

srml/contract/src/gas.rs

+
+	// this should be infallible since we just charged for it this block. nonetheless, we play it
+	// safe and only issue the refund if issuance doesn't overflow.
+	if let Ok(_) = <balances::Module<T>>::increase_total_stake_by(refund) {


Because contracts executed can't increase issuance isn't it ?

Practically, yes, this shouldn't be able to overflow because:

we decrease the total issuance when we buy gas

because gas is only spent, the total issuance is increased by lower amount than it was decreased.

there are no* code executed between pt.1 and pt.2 beyond the contract module and it doesn't do any transfers.

(* with subtle caveat that the contract module can call callbacks such as T::DetermineContractAddress which theoretically could violate this, but we don't care about this cases and most likely will put a warning somewhere in the docs that this is not recommended since it can violate some assumptions here)

contracts module will need moving to the new model.

gavofyork · 2019-03-18T20:18:45Z

@pepyakin please check my changes to contract module

ala #2025

srml/contract/src/gas.rs

srml/support/src/traits.rs

srml/balances/src/lib.rs

xlc · 2019-03-19T00:30:17Z

srml/balances/src/lib.rs

+	///
+	/// This just removes the nonce and leaves an event.
 	fn reap_account(who: &T::AccountId) {
 		<system::AccountNonce<T>>::remove(who);


There is T::OnNewAccount, this here also be T::OnReapAccount as well?

You mostly care about the Free part of an account being destroyed (T::OnFreeBalanceZero). I don't mind adding a hook if you can come up with a use case.

pepyakin

Contract changes look good to me!

gui1117 · 2019-03-19T11:13:10Z

srml/balances/src/lib.rs

+		value: Self::Balance
+	) -> UpdateBalanceOutcome {
+		// Should be infallible if our state isn't corruption. Not much we can do if state it corrupted.
+		let _ = S::on_unbalanced_increase(value);


OK I was wrong for this method:
the doc said: NOTE: This assumes that the total stake remains unchanged after this operation
because when I decoupled treasury it was not increase stake

substrate/srml/treasury/src/lib.rs

Lines 226 to 232 in e01b450

// return their deposit.

let _ = <balances::Module<T>>::unreserve(&p.proposer, p.bond);

// provide the allocation.

<balances::Module<T>>::increase_free_balance_creating(&p.beneficiary, p.value);

Self::deposit_event(RawEvent::Awarded(index, p.value, p.beneficiary));

But finally it seems what I thought as intentionnal was a bug.
So this method shouldn't exist anymore and be renamed to reward_creating. (or reward_create_account as suggested by @joepetrowski )

Also this confirm the point of @shawntabrizi as TotalIssuance, ReservedBalance and FreeBalance are dependent storages, we could make ourself safer even by providing method that will operate them on them coherently. We can also provides bypass method for some optimisation (but most of usages can use safe abstraction actually as we almost always call modification of total issuance and then modification of free balance) and we can even name those bypass method with a unguaranted_ or other very warnant prefix.

This would make reviewing easier

hmm safe operator could be implemented in another PR obviously but renaming of increase_free_balance_creating to reward_create_account seems relevant to me

They are not dependent storages. Total issuance includes balances outside of the basic AccountId mapping such as that in parachains.

In any case, this is outside the scope of this PR.

gui1117 · 2019-03-19T11:17:29Z

srml/balances/src/lib.rs

-		Self::make_transfer(from, to, amount)
+impl<T: Trait> OnUnbalancedDecrease<T::Balance> for BurnAndMint<T> {
+	fn on_unbalanced_decrease(amount: T::Balance) -> Result {
+		<Module<T>>::decrease_total_issuance_by(amount)


I don't understand how it could be useful, could you instance another implementation ?

Yes. You might want to put it in the treasury, for example.

tomusdrw

lgtm after a brief look

tomusdrw · 2019-03-19T11:17:43Z

core/sr-primitives/Cargo.toml

 rstd = { package = "sr-std", path = "../sr-std", default-features = false }
 runtime_io = { package = "sr-io", path = "../sr-io", default-features = false }
 log = { version = "0.4", optional = true }
+keyring = { package = "substrate-keyring", path = "../keyring", optional = true }


What is keyring needed for?

This grumble is unaddressed

I think this doesn't matter since this one is being superseded by #2048

tomusdrw · 2019-03-19T11:19:18Z

core/sr-primitives/src/lib.rs

-	fn verify<L: Lazy<[u8]>>(&self, mut msg: L, signer: &AnySigner) -> bool {
-		runtime_io::sr25519_verify(self.0.as_fixed_bytes(), msg.get(), &signer.0.as_bytes()) ||
-			runtime_io::ed25519_verify(self.0.as_fixed_bytes(), msg.get(), &signer.0.as_bytes())
+	type Signer = sr25519::Public;


I've used AnySigner in #2033, I think it's a bit more explicit, can later work on merging this.

tomusdrw · 2019-03-19T11:27:17Z

srml/balances/src/lib.rs

-		Self::decrease_total_stake_by(free_slash);
-		if free_slash < value {
-			Self::slash_reserved(who, value - free_slash)
+		let res = if free_slash < value {


Could be:

if let Some(remaining_slash) = value.checked_sub(free_slash) { ... }

this is about to be rewritten in an upcoming PR anyway so will leave it until then

tomusdrw · 2019-03-19T11:27:26Z

srml/balances/src/lib.rs

+		let res = if free_slash < value {
+			let remaining_slash = value - free_slash;
+			let reserved_balance = Self::reserved_balance(who);
+			let reserved_slash = cmp::min(reserved_balance, remaining_slash);


Don't we have saturating_sub?

yeah, i would prefer to address these minor things in the next PR though since merging will be a time drain.

tomusdrw · 2019-03-19T11:27:55Z

srml/balances/src/lib.rs

+			let reserved_balance = Self::reserved_balance(who);
+			let reserved_slash = cmp::min(reserved_balance, remaining_slash);
+			Self::set_reserved_balance(who, reserved_balance - reserved_slash);
+			if reserved_slash < remaining_slash {


The entire if-else expression could be just remaining_slash.checked_sub(reserved_slash)

it's about to be rewitten :)

tomusdrw · 2019-03-19T11:30:35Z

srml/balances/src/lib.rs

+		let b = Self::free_balance(transactor);
+		let transaction_fee = Self::transaction_base_fee() + Self::transaction_byte_fee() * <T::Balance as As<u64>>::sa(encoded_len as u64);
+		if b < transaction_fee + Self::existential_deposit() {
+			return Err("not enough funds for transaction fee");


Why is it not possible to go below existential_deposit? We don't want the account to be killed by paying fees?

We assume that the account exists at the point of dispatch.

tomusdrw · 2019-03-19T11:33:04Z

srml/contract/src/gas.rs

+	let fee = <T::Gas as As<T::Balance>>::as_(gas_spent) * gas_meter.gas_price;
+
+	// this should be infallible since we just charged for it this block.
+	let _ = T::GasPayment::on_unbalanced_decrease(fee);


Shouldn't the comment be just expect proof?

nothing should ever panic in the runtime. better to fail and allow a chain to be rescued through governance in subsequent blocks than brick it completely.

rphmeier · 2019-03-20T10:49:52Z

srml/support/src/traits.rs

+}
+
+#[derive(Copy, Clone, Eq, PartialEq)]
+pub enum ExistenceRequirement {


rphmeier · 2019-03-20T10:58:53Z

Generally approve. There are some other grumbles and needs a merge. I'm not an expert at this section of the code though.

Be a little safer with total issuance.

debe665

gavofyork added the A0-please_review Pull request needs code review. label Mar 18, 2019

devops-parity added the A4-gotissues label Mar 18, 2019

gavofyork mentioned this pull request Mar 18, 2019

Is TotalIssuance updated correctly? #1979

Closed

gavofyork added 2 commits March 18, 2019 09:15

PairT instead of _Pair

b50142f

Remove rev causing upset

771bf0e

gui1117 reviewed Mar 18, 2019

View reviewed changes

gavofyork added 2 commits March 18, 2019 12:32

Remove fees stuff.

f7d9f5e

Fix build (including tests)

eb096ec

gavofyork changed the title ~~Be a little safer with total issuance~~ Revamp minting/reward handling Mar 18, 2019

gavofyork changed the title ~~Revamp minting/reward handling~~ Revamp minting/burning handling Mar 18, 2019

Update runtime, bump version

ae9c4b8

jacogr mentioned this pull request Mar 18, 2019

Fix MultiSigner, simplify tests #2033

Merged

gavofyork added 2 commits March 18, 2019 18:52

Fix

3f7acd5

Merge remote-tracking branch 'origin/master' into gav-fixes

da0d3b1

devops-parity added the B2-breaksapi label Mar 18, 2019

Handle gas refunds properly.

10069bc

gavofyork removed the A4-gotissues label Mar 18, 2019

This was referenced Mar 18, 2019

Fees module questions #1993

Closed

Fees module refactor #2012

Closed

Rename identifier

e263901

ala #2025

shawntabrizi mentioned this pull request Mar 18, 2019

Rename *_total_stake_by to *_total_issuance_by in Balances Module #2025

Closed

xlc reviewed Mar 19, 2019

View reviewed changes

jacogr mentioned this pull request Mar 19, 2019

InvalidTransaction on latest master (after first tx success, latest master) #2022

Closed

pepyakin approved these changes Mar 19, 2019

View reviewed changes

Address grumbles

f835aa6

gui1117 reviewed Mar 19, 2019

View reviewed changes

tomusdrw reviewed Mar 19, 2019

View reviewed changes

jacogr mentioned this pull request Mar 19, 2019

Bump UI, HDKD keyring (new substrate crypto) polkadot-js/apps#813

Merged

xlc mentioned this pull request Mar 19, 2019

Contract buy gas did not check can withdraw #1980

Closed

gavofyork mentioned this pull request Mar 19, 2019

Introduce safe types for handling imbalances #2048

Merged

rphmeier added A5-grumble and removed A0-please_review Pull request needs code review. labels Mar 20, 2019

rphmeier reviewed Mar 20, 2019

View reviewed changes

srml/support/src/traits.rs

}

#[derive(Copy, Clone, Eq, PartialEq)]

pub enum ExistenceRequirement {

Copy link

Contributor

rphmeier Mar 20, 2019

Choose a reason for hiding this comment

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

docs?

gavofyork closed this Mar 20, 2019

arkpar deleted the gav-fixes branch April 23, 2020 18:10

	// return their deposit.
	let _ = <balances::Module<T>>::unreserve(&p.proposer, p.bond);

	// provide the allocation.
	<balances::Module<T>>::increase_free_balance_creating(&p.beneficiary, p.value);

	Self::deposit_event(RawEvent::Awarded(index, p.value, p.beneficiary));

Revamp minting/burning handling #2028

Revamp minting/burning handling #2028

Uh oh!

Conversation

gavofyork commented Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gui1117 Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavofyork commented Mar 18, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pepyakin left a comment

Choose a reason for hiding this comment

Uh oh!

gui1117 Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavofyork commented Mar 18, 2019 •

edited

Loading

gui1117 Mar 18, 2019 •

edited

Loading

gui1117 Mar 19, 2019 •

edited

Loading