Code review of [BSIP10] percentage based transfer fee

[vikramrajkumar]

From @abitmore on February 16, 2016 6:44

As @theoreticalbts mentioned in cryptonomex/graphene#570 (comment) I'm now creating this ticket.

• The BSIP10 feature requirement/description is here.
• My main develop branch is here here.
• //Update: the branches below have been merged into the main develop branch (above)
  • In this branch I implemented a feature: with asset_update_operation the new_options can have a null CER, in this case CER of asset_to_update won't change. It addresses the No. 5 known issue in BSIP10. //Update: branch merged to main develop branch
  • In this branch I implemented a new operation committee_member_update_core_asset_operation, so committee can change some options of CORE asset. It addresses the No. 3 known issue in BSIP10. //Update: branch merged to main develop branch

@xeroc suggested me to re-base the code so he can merge it to test network easier, but I don't think it's good to do it right now, since it will cause loss of tracking to individual changes(I'm a bit bias here).

Current code is based on bitshares branch (the production branch). I may need to merge new codes from develop branch. Maybe need a new ticket for this? //Update: this job is finished.

Copied from original issue: cryptonomex/graphene#583

[vikramrajkumar]

From @xeroc on February 16, 2016 9:25

Another thing to consider is that for BitShares you need the hardfork logic, while you do not need it for the graphene code base .. (AFAIK)

Not sure how CNX see it, but IMHO it makes sense to have the hardfork logic be a separated commit from the actually feature.

[vikramrajkumar]

From @abitmore on February 16, 2016 9:37

@abitmore : In general, your operations should use an asset_id_type, then fetch the asset_object referred to by the ID in the evaluator.

This percentage transfer fee feature actually needs a great deal of review, as it breaks one of the design principles of the Graphene toolkit, which is that the fee for an operation can be determined only from the operation and the current fee schedule. Now transferring some assets will have a fee which cannot be determined from that information alone (as you'll need to know whether the asset issuer's enabled the percentage-based fee for their asset).

This BSIP also really doesn't "play nice" with the existing machinery which converts the fee. There's really no way for you to tell the blockchain "for this operation, don't convert the fee to BTS" -- it's already converted by the time the op knows about it.

I think the sanest way to do percentage-based transfer fees is to make them an extension on the transfer operation, which basically says "I'm including this extra BTS / asset to be paid according to the BSIP rules," and then have another extension in the asset_update_operation which allows an asset owner to say "transfer of my asset is disallowed unless the transfer op includes the extension with so much on it." Then the main fee's paid into network / referral system as normal and the BSIP is an extra fee can be handled by totally custom logic.

TLDR, use the extension system to add another field to the transfer op, instead of trying to jam it into the existing fee field and have the existing fee handling code get in your way (what you're doing is different enough that it will get in your way).

If I understood correctly, you suggested that we leave current set_fee and get_required_fees API unchanged, and add another API (for example calculate_additional_fee) for applications to calculate additional fees which can be added into extensions maybe with yet another API for example set_additional_fee, or maybe an API for example adjust_fee to adjust the fee field directly.

• A benefit of this is it probably can be used by all operations later.
• A drawback of this is it will require a bit more changes to client applications to set the additional fee field to extensions, or unnecessarily multiple calls to set a fee;
• another drawback is the calculate_fee loop called inside set_fee function become redundant when need to add more data to the transaction/operation after set the fee, but it's required if the client won't call set_additional_fee or adjust_fee.

So I propose that we extend the set_fee API and internally called calculate_fee function directly. With current implementation I added a asset_object parameter, but it probably better to add something smaller and more extendable instead, for example a flat_set<static_variant>. It also can be implemented better (than current implementation) if place a default implementation of extended_calculate_fee function in operation class and override it when needed.

Thoughts?

[vikramrajkumar]

From @theoreticalbts on February 26, 2016 15:43

@xeroc : We strongly prefer to have hardfork logic in the Graphene codebase for many reasons:

• Allows most testing to be done in Graphene unit tests
• Keep the diff between BitShares and Graphene relatively small
• Minimize merge work needed when updating BitShares with new code from Graphene
• Minimize possibility of bugs that manifest in BitShares but not in Graphene

Ideally a unit test will exist for new hardforking code, which checks the behavior both before and after the hardfork time.

[vikramrajkumar]

From @abitmore on February 29, 2016 3:1

Finished some code refactory, new code is here (will update OP accordingly)

• new code is based on develop branch, it can be merge to develop branch and bitshares branch easily
• with the help of virtual functions and templates, the code become cleaner, most of special operation checks are removed from generic code including evaluator.cpp, fee_schedule.cpp, database_api.cpp and wallet.cpp
• added a new get_operation_fee API to database_api, which is similar to but simpler than the old get_required_fees API

Things yet to be done:

• write unit testing code
• merge the 2 related branches mentioned in OP

Questions:

• is it possible and/or practicable to extend "transfer_operation" directly instead of adding a new operation "tranfer_v2_operation"?

[vikramrajkumar]

From @abitmore on February 29, 2016 21:41

Merged the 2 related branches mentioned in OP.

[vikramrajkumar]

From @theoreticalbts on March 1, 2016 18:31

• is it possible and/or practicable to extend "transfer_operation" directly instead of adding a new operation "tranfer_v2_operation"?

Yes, this is what the extensions field is for. We started defining extensions with the previous release, but currently there is a serialization bug #599 which does not allow them to be used.

[vikramrajkumar]

From @theoreticalbts on March 1, 2016 18:51

The history of this branch is a little messy as far as merge commits go. Some project management and git usage minutiae:

• They are not full of merge commits and merge conflicts, meaning you should start from a checkout of develop and either cherry pick commits, or create a fresh commit by pulling out files.
• Every commit compiles (meaning any commit fixing compiler errors should be combined with the commit that introduced the errors using the "fixup" or "squash" feature of git rebase -i).
• The committee_member_update_core_asset_operation should be its own ticket.
• The asset_update_operation CER semantics change should be its own ticket.
• As a rule of thumb, if you introduce new code, then immediately do a cleanup / refactor of the new code, it may be better to squash / fixup into a single commit. An exception to this rule of thumb is when a change can be decomposed into a cleanup / refactor of old code (retaining equivalent functionality) followed by new code (using extension points which were exposed by the refactoring of the old code).

@abitmore if you're reading this, hold off on fixing this stuff for now until I get a chance to review things more thoroughly.

[vikramrajkumar]

From @abitmore on March 1, 2016 19:3

• is it possible and/or practicable to extend "transfer_operation" directly instead of adding a new operation "tranfer_v2_operation"?

Yes, this is what the extensions field is for. We started defining extensions with the previous release, but currently there is a serialization bug #599 which does not allow them to be used.

The real issue is that transfer_operation::fee_parameters_type is lack of an extensions field.

[vikramrajkumar]

From @theoreticalbts on March 1, 2016 19:9

So we need to define a fee extension mechanism in order to implement this feature without adding a new transfer_v2_operation. Fortunately this is something we've already figured out how to do -- see #554 -- I just haven't gotten around to implementing it yet.

[vikramrajkumar]

From @abitmore on March 1, 2016 19:11

@abitmore if you're reading this, hold off on fixing this stuff for now until I get a chance to review things more thoroughly.

Sorry, I know the commit history is a bit messy. There are not only merges, but also a lot of code refactories -- some codes are added then removed. Please check #605 directly for the final status of code.

[vikramrajkumar]

From @theoreticalbts on March 1, 2016 19:25

I'm actually reviewing the diff which is much more readable than the commit history. I think the commit history is just too messy to spend effort on fixing, and what we should do instead is simply copy-paste code from the diff into brand-new better-organized commits.

A messy commit history isn't necessarily a bad thing. I personally usually have an extremely messy and verbose commit history full of tiny commits fixing compile errors, repeated merges, cherry picks, debugging stuff -- but I'm the only person who ever sees any of that, because I do a ton of clean-up work to make the commits I push to Github simple and readable. I also know when I'm writing the commits that I'll have to do this cleanup later, so over time I've developed a little notation system of hints I leave in commit messages which help me remember things I'll need to know in the later cleanup, and I also tend to do as I go the things that I know would be hardest / most confusing to clean up later (after I may have forgotten all the details of what I was doing).

[vikramrajkumar]

From @theoreticalbts on March 1, 2016 20:27

Hmm, there is a fundamental issue raised by this feature. When paying a percentage fee in BTS, you have to have an exchange rate, which means whatever you're using as the exchange rate [1] effectively becomes part of the fee schedule.

I originally thought that a violation of one of the Graphene design principles -- that the fee for an operation can be determined from only the operation and fee schedule -- was merely an architectural problem in the implementation which could be solved with some (possibly large) refactoring of the new code. But this violation is actually a fundamental part of the feature, and cannot be removed unless you want to remove the ability to use BTS to pay the transfer fee for such an asset.

So there are really two questions here:

• Should the specification be revised to require percentage fees to be paid in the asset being transferred?
• How do you solve the data flow problem of getting the asset-specific fee parameters into every place where fee parameters need to go?

The second question needs to be squarely addressed in order for both internal chain code and wallets to support percentage fees. The solution in the code is calculate_fee_extended(), which needs to be revised -- my thoughts on that revision will be forthcoming in another comment.

I should also note that even if the answer to the first question is "yes," the second question still needs to be answered if the percentage is an asset-specific parameter.

[1] The BSIP discusses this and eventually comes to the conclusion that the CER should be used as the exchange rate.

[vikramrajkumar]

From @theoreticalbts on March 1, 2016 20:48

• BIP-0062 Signature Canonization #554 should be implemented as a dependency of this feature.
• new get_operation_fee() API call is unnecessary, functionality is already provided by get_required_fees() API call immediately above it
• Changing get_required_fees_helper to take db as a parameter means that you're making fees dependent on chain state other than the fee schedule. One of the Graphene design principles is that the fee for an operation can be determined only by the operation and the fee schedule, without reference to any other chain state. This principle is an assumption baked into the design of the current wallets. More thoughts in a follow-up comment.
• Extensions should use extension<T> mechanism. This will greatly reduce the amount of needed boilerplate of the form "iterate over all the extensions and see if the one I care about is present", and make get_transfer_fee_mode(), set_transfer_fee_mode(), checking the extension is absent before the hardfork date, and . Note you will need to merge Asset update issuer operation #599 to fix a serialization bug which keeps extension<T> from working correctly in the BitShares release.
• build_extended_fee_parameters() should be a free-floating method which takes a const fee_schedule& instead of being a method on database.
• build_extended_fee_parameters() uses a variant which has poor performance and should be avoided in validation code. This should likely use fc::static_variant instead.
• Implementing a new committee_member_update_core_asset_operation is not the way to go. Instead we should change the owner of CORE to the committee, then verify that only the things we want to allow to be updated are updated.
• Any action which requires the committee authority is restricted to proposed tx's and review period, there is no need to enforce this specifically in an evaluator.
• For the new null core exchange rate semantics implementation here, copying options into a local variable is unnecessary. Instead simply set a local variable to the new exchange rate, copy the options to the object, then write the new exchange rate into the object.
• Substantial changes are made to account_statistics_object::process_fees() implementation. It seems that what's going on is pending_fees_to_network, pending_fees_to_non_network and pending_vested_fees_to_non_network are placed on the account object. This may be merely a refactoring of the existing functionality, if that is the case, there is no reason to have the fields on account_statistics_object when they are merely local bookkeeping for the process_fees implementation.
• Transfer fee mode is another thing that will probably be made redundant by BIP-0062 Signature Canonization #554. I.e. the implementation is cleaner if both percentage and flat fees are charged for all assets, but the percentage fee may be zero which should exactly emulate the previous functionality.

I left off about 2/3 of the way through the diff (I still need to get through transfer.cpp and some of the rest of the diff).

[vikramrajkumar]

From @abitmore on March 1, 2016 23:2

Should the specification be revised to require percentage fees to be paid in the asset being transferred?

This idea is perhaps due to haven't go through all features/code. Exchange rate is not only used to determine how much the fee should be, but also used to determine a cap and a bottom of the fee. Pay fee in the asset only without checking the rate will make the cap/bottom unable to be calculated, or need per-asset settings which is out of this BSIP's scope (if not well designed, it will cause justice issues between asset issuers and the committee).

new get_operation_fee() API call is unnecessary, functionality is already provided by get_required_fees() API call immediately above it

get_operation_fee is designed as a replacement of several db.current_fee_schdule.set_fee(op) calls inside cli_wallet code (I think it's not good to use an API in this way). The get_required_fees API recursively checks operations inside a proposal, thus is not suitable (and maybe is wrong).

build_extended_fee_parameters() uses a variant which has poor performance and should be avoided in validation code. This should likely use fc::static_variant instead.

Not impossible. Just need a good design and some coding.

Implementing a new committee_member_update_core_asset_operation is not the way to go. Instead we should change the owner of CORE to the committee, then verify that only the things we want to allow to be updated are updated.

Implementing a new special operation is much easier and less possible to mess things up. It's a white-listing feature, only options allowed to be updated can be updated. Current asset permission/flag design is more a black-listing feature, which is always needed to be expanded to define new rules.

Any action which requires the committee authority is restricted to proposed tx's and review period, there is no need to enforce this specifically in an evaluator.

It's just copied from committee_member_update_global_parameters_operation. These may belong to a new ticket.

For the new null core exchange rate semantics implementation here, copying options into a local variable is unnecessary. Instead simply set a local variable to the new exchange rate, copy the options to the object, then write the new exchange rate into the object.

The proposed way need to write database twice, is it good? Another possible way is changing the copy of the object to a member variable of the class, since it have been constructed once in do_evaluate() just some lines above.

Substantial changes are made to account_statistics_object::process_fees() implementation. It seems that what's going on is pending_fees_to_network, pending_fees_to_non_network and pending_vested_fees_to_non_network are placed on the account object. This may be merely a refactoring of the existing functionality, if that is the case, there is no reason to have the fields on account_statistics_object when they are merely local bookkeeping for the process_fees implementation.

No, it's not the case, it's a new feature described in the BSIP10 document: different fee cutting rules. I admit that code probably need to be refactored, but likely we need a new ticket for it.

Transfer fee mode is another thing that will probably be made redundant by #554. I.e. the implementation is cleaner if both percentage and flat fees are charged for all assets, but the percentage fee may be zero which should exactly emulate the previous functionality.

The required feature is not like this. Some assets charge percentage based fee, other assets charge flat fee, no per-asset fee settings right now except a "mode".

I have some thoughts about other questions, will reply after @theoreticalbts made more progresses.