Use `ChargeFee` in balances and contract modules. #1815

shaunxw · 2019-02-18T02:30:03Z

This is a follow-up change related to fees module. (issue #1515 and PR #1648 )

ChargeFee is introduced into balances and contract modules, to handle fee charge logic:

balances: transfer/creation fee.
contract: gas fee buy/refund.

parity-cla-bot · 2019-02-18T02:30:07Z

It looks like @shaopengw hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

parity-cla-bot · 2019-02-18T02:30:08Z

It looks like @shaopengw hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

shaunxw · 2019-02-18T02:41:41Z

[clabot:check]

parity-cla-bot · 2019-02-18T02:41:44Z

It looks like @shaopengw signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

parity-cla-bot · 2019-02-18T02:41:44Z

It looks like @shaopengw signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

gavofyork · 2019-02-18T18:27:46Z

Where's the code for the fees module?

pepyakin · 2019-02-18T18:47:02Z

srml/contract/src/gas.rs

-	if new_balance < Some(<balances::Module<T>>::existential_deposit()) {
-		return Err("not enough funds for transaction fee");
-	}
+	<T as Trait>::ChargeFee::charge_fee(transactor, cost)?;


The previous version of code ensures that new_balance cannot be below existential deposit. In other words we can't kill an account buy purchasing gas. Looking at the current implementation this property won't hold anymore.

@pepyakin Maybe in general ChargeFee (and TransferAsset in the implementation) trait are not supposed to kill transactor's account. Account got killed is more like an unexpected side effect while paying fees or doing transfer in other srml modules except balances. I could update the implementation of TransferAsset, to make the functions return Errs instead of killing accounts.

pepyakin · 2019-02-18T18:54:41Z

srml/contract/src/gas.rs

 	let refund = <T::Gas as As<T::Balance>>::as_(gas_meter.gas_left) * gas_meter.gas_price;
-	<balances::Module<T>>::set_free_balance(transactor, b + refund);
-	<balances::Module<T>>::increase_total_stake_by(refund);
+	<T as Trait>::ChargeFee::refund_fee(transactor, refund)


Because we ensure that buy_gas doesn't destroy account, the previous version can't fail. This one apparently can and if it fails for some reason, the whole transaction will be deemed as failed.

This be very misleading, given that the changes that were performed in that extrinsic are actually commited.

If refund_fee always called after charge_fee and the amount of refund the less that charged, then it can't fail. refund_fee is designed to haveResult return type to make it more robust, as we can't guarantee how it is called.

Is it better to document this kind of expected way of usage, and get rid of Result return type? Or we ignore the return here as it can't fail in this case?

pepyakin · 2019-02-18T19:00:49Z

srml/contract/src/lib.rs

 			// NOTE: this should go after the commit to the storage, since the storage changes
 			// can alter the balance of the caller.
-			gas::refund_unused_gas::<T>(&origin, gas_meter);
+			gas::refund_unused_gas::<T>(&origin, gas_meter)?;


Note that here (and above) when we can't refund funds for some reason, we abort the execution of the extrinsic with an error, even though all changes are commited.

Moreover, we lose all calls recorded so far.

pepyakin · 2019-02-18T19:05:27Z

srml/contract/src/lib.rs

 	type DetermineContractAddress: ContractAddressFor<CodeHash<Self>, Self::AccountId>;

+	/// Fee charge.
+	type ChargeFee: ChargeFee<Self::AccountId, Amount=Self::Balance>;


If we are ok with changes here regarding contracts, we need to document the requirements of ChargeFee implementation. I.e. refund_fee should fail and etc.

shaunxw · 2019-02-18T20:50:38Z

Where's the code for the fees module?

The fees module code has been merged in PR #1648 .

gavofyork · 2019-02-19T15:49:41Z

I see now; missed that one. We'll let #1821 go in before this one.

shaunxw · 2019-02-21T02:51:43Z

ChargeFee and TransferAsset traits are updated that they won't kill accounts. Killing accounts is more an unexpected side effect for charge_fee and remove_from, as they are typically used together with refund_fee and add_to. Also it's more reasonable to return Err if balance would be below existential deposit after paying fee.

shaunxw · 2019-02-21T03:16:54Z

For enforcing ExistentialDeposit law, another possible solution is that, instead of kill an account instantly when balance below existential_deposit, balances module tracks accounts whose balance got changed, and only do check and kill accounts if needed on finalize.

This could solve these situations:

An account got killed but then created later in the same tx/block execution, with creation fee charged. For instance, in one tx, Alice transferred money to Bob and her account got killed, but in another tx in the same block, Charles transferred her some found.
Alice has some particular amount of money that, she cannot buy gas fee as her account would got killed after payment(because of here), but actually she should be able to, as the refund amount will make her account still alive.
ChargeFee and TransferAsset trait (and other possible traits in the future) implementation doesn't have to take care of accounts got killed or not after functions called.

xlc · 2019-02-26T23:03:13Z

@gavofyork what is the plan for this PR? I think this is going to be conflict with #1782. Do you intent to wait until #1782 is merged?

gavofyork · 2019-02-28T10:29:07Z

Yes, but the good news is that it should be merged today or tomorrow so we can reevaluate how and if these changes make sense in the light of the api changes.

gavofyork · 2019-02-28T10:32:34Z

Batching up deletions may make sense. But will need some thought as it introduces additional runtime costs that I’d rather not have.

xlc · 2019-02-28T11:05:15Z

What additional runtime cost are you concerning about?
It defers (and merge) the needs to check account balance less than existential deposit to end of block, instead of in the transaction processing. I don't think this overhead is significant?
I can see more storage access will be needed in order to implement this, but this should be possible to be optimized with some special in-memory block scope store? This will benefits to other modules as well (fees module been one, possibly system module).

gavofyork · 2019-03-02T13:36:36Z

I can see more storage access will be needed in order to implement this,

Precisely my concern.

tomusdrw · 2019-03-07T13:43:13Z

srml/balances/src/lib.rs

-			Self::decrease_total_stake_by(fee);
+			T::ChargeFee::charge_fee(transactor, fee)?;
+
+			Self::set_free_balance(transactor, new_from_balance - fee);


Why is the fee subtracted again? What protects us from overflows?

@tomusdrw If not subtracted, fee will actually be charged twice, since charge_fee has already removed fee from balance. Because fee was added to new_from_balance (by calculating liability), the subtraction here cannot fail.

If I'm not mistaken it's the other way around:

let liability = match value.checked_add(&fee)...; let new_from_balance = match from_balance.checked_sub(&liability)...; T::ChargeFee::charge_fee(transactor, fee)?; Self::set_free_balance(transactor, new_from_balance - fee);

The fee is subtracted from from_balance and then again subtracted before setting the free_balance. So unless I'm missing something: new_from_balance = from_balance - value - fee - fee

@tomusdrw You're right, my mistake. Will update.

Updated. Also added two unit tests for balance transfer, to cover fee charge.

gavofyork · 2019-03-12T12:04:37Z

srml/balances/src/tests.rs


 		Balances::set_lock(ID_1, &1, 10, u64::max_value(), WithdrawReason::TransactionPayment.into());
-		assert_ok!(<Balances as TransferAsset<_>>::transfer(&1, &2, 1));
+		assert_ok!(Balances::ensure_account_can_withdraw(&1, 1, WithdrawReason::Transfer, 9));


These are not logically equivalent. Any such changes should be well-documented in the PR with why unit tests are being altered.

The reason of the change is old behavior is not correct.
balances transfer method not only withdraw for transfer reason, but also withdraw for tx fee reason. Therefore it should fail, which indeed failed with the updated behavior.
Related: #1962, #1978

@shaopengw maybe should have assert_err!(<Balances as TransferAsset<_>>::transfer(&1, &2, 1));

@xlc The assert_noop! below has covered the TransactionPayment withdraw should fail case, so it should be fine without adding assert err for transfer.

If these tests are mean to be unit tests, maybe other transfer calls in this test should also be changed to ensure_account_can_withdraw like this line, as transfer implies both WithdrawReason::TransactionPayment and WithdrawReason::Transfer.

The reason of the change is old behavior is not correct.
balances transfer method not only withdraw for transfer reason, but also withdraw for tx fee reason

The old behaviour is correct. The only time transaction payment happens is prior to extrinsic execution in executive. Any further withdrawals are not "transaction payment" but high-level actions that are logically unrelated. The two should not be confused.

gavofyork · 2019-03-20T13:47:29Z

superceded in #2048

shaunxw added 3 commits February 18, 2019 12:15

Use ChargeFee trait to charge and refund gas fee.

0366548

Use ChargeFee trait in balances module.

26226da

Build runtime wasm and bump version.

26a4a06

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

pepyakin reviewed Feb 18, 2019

View reviewed changes

gavofyork added the A1-onice label Feb 19, 2019

shaunxw added 6 commits February 20, 2019 10:47

Merge branch 'master' into sw/charge-fee

b149ea8

Removed amount type of fees trait in mocks.

6985eb1

Ensure charge fee and transfer asset won't kill accounts.

f8a848d

Refunding unused gas cannot fail.

7cfae90

Merge branch 'master' into sw/charge-fee

2dd4456

Rebuild runtime wasm and bump spec version.

2bbc795

gavofyork self-assigned this Feb 21, 2019

gui1117 mentioned this pull request Feb 21, 2019

Decouple balances module with other modules #1514

Closed

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

gavofyork removed the A1-onice label Mar 5, 2019

shaunxw added 2 commits March 6, 2019 12:25

Merge branch 'master' into sw/charge-fee

7514bb7

Merge branch 'master' into sw/charge-fee

fb91b07

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

Rebuild runtime and bump version.

6692796

xlc mentioned this pull request Mar 7, 2019

Add OnFeeCharged trait in fee modules. #1924

Closed

tomusdrw reviewed Mar 7, 2019

View reviewed changes

shaunxw added 3 commits March 8, 2019 11:28

Fixed lock reasons unit test.

db83d03

Merge branch 'master' into sw/charge-fee

c132067

Updated runtime wasm.

045e9c3

xlc mentioned this pull request Mar 8, 2019

How to implement an in memory store for extrinsic scope variable? #1949

Closed

shaunxw added 5 commits March 11, 2019 15:28

Updated make_transfer implementation.

84dacde

Added unit tests for balance transfer.

24dc4f0

Merge branch 'master' into sw/charge-fee

e032c84

Rebuild runtime wasm.

703bc20

Minor comments update.

687cf01

gavofyork reviewed Mar 12, 2019

View reviewed changes

xlc mentioned this pull request Mar 12, 2019

Contract buy gas did not check can withdraw #1980

Closed

gavofyork removed the A0-please_review Pull request needs code review. label Mar 13, 2019

xlc mentioned this pull request Mar 14, 2019

Fees module questions #1993

Closed

gavofyork closed this Mar 20, 2019

Use ChargeFee in balances and contract modules. #1815

Use ChargeFee in balances and contract modules. #1815

Uh oh!

Conversation

shaunxw commented Feb 18, 2019

Uh oh!

parity-cla-bot commented Feb 18, 2019

Uh oh!

parity-cla-bot commented Feb 18, 2019

Uh oh!

shaunxw commented Feb 18, 2019

Uh oh!

parity-cla-bot commented Feb 18, 2019

Uh oh!

parity-cla-bot commented Feb 18, 2019

Uh oh!

gavofyork commented Feb 18, 2019

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!

shaunxw Feb 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!

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

Uh oh!

Uh oh!

gavofyork commented Feb 19, 2019

Uh oh!

shaunxw commented Feb 21, 2019

Uh oh!

shaunxw commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xlc commented Feb 26, 2019

Uh oh!

gavofyork commented Feb 28, 2019

Uh oh!

gavofyork commented Feb 28, 2019

Uh oh!

xlc commented Feb 28, 2019

Uh oh!

gavofyork commented Mar 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomusdrw Mar 8, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavofyork commented Mar 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Use `ChargeFee` in balances and contract modules. #1815

Use `ChargeFee` in balances and contract modules. #1815

shaunxw Feb 18, 2019 •

edited

Loading

shaunxw commented Feb 18, 2019 •

edited

Loading

shaunxw commented Feb 21, 2019 •

edited

Loading

tomusdrw Mar 8, 2019 •

edited

Loading