-
Notifications
You must be signed in to change notification settings - Fork 337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code review of [BSIP10] percentage based transfer fee #583
Comments
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. |
If I understood correctly, you suggested that we leave current
So I propose that we extend the Thoughts? |
…f fee parameter names. Conflicts: libraries/chain/include/graphene/chain/hardfork.hpp libraries/chain/transfer_evaluator.cpp
@xeroc : We strongly prefer to have hardfork logic in the Graphene codebase for many reasons:
Ideally a unit test will exist for new hardforking code, which checks the behavior both before and after the hardfork time. |
Finished some code refactory, new code is here (will update OP accordingly)
Things yet to be done:
Questions:
|
Merged the 2 related branches mentioned in OP. |
Yes, this is what the |
The history of this branch is a little messy as far as merge commits go. Some project management and git usage minutiae:
@abitmore if you're reading this, hold off on fixing this stuff for now until I get a chance to review things more thoroughly. |
The real issue is that transfer_operation::fee_parameters_type is lack of an extensions field. |
So we need to define a fee extension mechanism in order to implement this feature without adding a new |
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. |
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). |
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:
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 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. |
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). |
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).
Not impossible. Just need a good design and some coding.
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.
It's just copied from
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
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.
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. |
This issue was moved to bitshares/bitshares-core#173 |
... even if it's still in margin call territory. cryptonomex#583
As @theoreticalbts mentioned in #570 (comment) I'm now creating this ticket.
herehere.asset_update_operation
thenew_options
can have a nullCER
, in this caseCER
ofasset_to_update
won't change. It addresses the No. 5 known issue in BSIP10. //Update: branch merged to main develop branchcommittee_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.The text was updated successfully, but these errors were encountered: