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]

Duplicate #173