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

Conversation

@xlc
Copy link
Contributor

@xlc xlc commented Feb 1, 2019

This introduces ChargeFee and TransferAsset trait which reduces the coupling between balances module and other modules.

A new Fees module is added to handle all transaction fee related work. Later it can be used to handle transaction fee distribution. It also provide a unified Charged even to simplify client side transaction fee listening.

The new module and traits should be able to handle various kinds of token implementations (current balances module, multi token module, UTXO module).

Close #1515
Relates to #1514
May conflicts to #1641

@gavofyork gavofyork added the A0-please_review Pull request needs code review. label Feb 1, 2019
@gavofyork
Copy link
Member

Looks like an overall good direction!

@gavofyork gavofyork added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Feb 1, 2019
Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

This looks good! It is difficult for me to see though how the implementation for UTXO chains would look like.

@xlc
Copy link
Contributor Author

xlc commented Feb 4, 2019

@pepyakin It is hard to be confident this will actually work with UTXO implementation without actually integrate with it, but here are my thoughts:

The UTXO module will implement TransferAsset trait and it won't do the actual transfer, but instead making some checks that this transfer is specified in the transaction. e.g. call transfer from A to B with amount of 100, will check the there is an output for B with remaining amount > 100 and reduce the remaining amount for this output by 100. The transfer_from method or transfer from use account to system account method, will check the amount is less than remaining amount of tx fee in this tx (i.e. total input - total output).
Then the rewards part (which should be a new trait which I don't want to include in this PR to limit its scope) will create a new coinbase output for each validators for them to get their rewards. The UTXO module could potentially merge multiple coinbase outputs (one from tx fee rewards, one from block validation rewards) into one to reduce storage usage.

@xlc xlc changed the title Transaction Fee Module (WIP) Transaction Fee Module Feb 12, 2019
@xlc
Copy link
Contributor Author

xlc commented Feb 12, 2019

@shawntabrizi @pepyakin This is ready for review

Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Just some nitpicks.

@xlc
Copy link
Contributor Author

xlc commented Feb 14, 2019

@bkchr can you have a look again and maybe merge it? This is ready and we would like to have this included to unblock us from next task, which is refactor balances and contract module to use this to charge fee. This then will make it possible to block explorer to easily fetch the total transaction fee of a transfer and then produce an accurate balance statement easily.

@bkchr
Copy link
Member

bkchr commented Feb 14, 2019

Will take a last look tomorrow morning and merge then probably.

@bkchr bkchr merged commit 14ee64c into paritytech:master Feb 15, 2019
@xlc xlc deleted the fee branch February 15, 2019 10:27
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Apr 12, 2019
* wip

* Split bytes fee charging and charging by amount into different traits.

* Move to edition 2018.

* Implemented charge fee traits for fees module.

* Implemented 'on_finalise' for fee module.

* Updated fees finalize impl.

* Renaming and documentation update.

* Added overflow & underflow check for fee calculation.

* Added mock and unit tests for fee module.

* More unit tests for fees module.

* Fixed srml-executive unit tests.

* Remove transaction base/bytes fee from balances module, fix unit tests.

* fix compile error

* Fixed unit test.

* Minor fixes.

* Bump spec version.

* Bump spec version.

* Updated fees module and runtime wasm.

* Fees module code style improvement; updated runtime wasm.

* Bump spec and impl version.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A3-in_progress Pull request is in progress. No review needed at this stage.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants