Skip to content
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

Add fee calculator to Bitshares object. #91

Closed
wants to merge 1 commit into from

Conversation

jhtitor
Copy link
Contributor

@jhtitor jhtitor commented Apr 23, 2018

This should NOT be used instead of get_required_fees API call (although it should be possible in the future). The main purpose here is to aid Blind Transfers.

@codecov-io
Copy link

codecov-io commented Apr 23, 2018

Codecov Report

Merging #91 into develop will increase coverage by 7.68%.
The diff coverage is 7.4%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #91      +/-   ##
===========================================
+ Coverage    59.56%   67.25%   +7.68%     
===========================================
  Files           37       51      +14     
  Lines         3539     4113     +574     
===========================================
+ Hits          2108     2766     +658     
+ Misses        1431     1347      -84
Impacted Files Coverage Δ
bitshares/bitshares.py 43.51% <7.4%> (-2.83%) ⬇️
bitshares/account.py 63.46% <0%> (-23.08%) ⬇️
bitshares/committee.py 68.18% <0%> (-18.19%) ⬇️
bitshares/blockchainobject.py 87.85% <0%> (-5.96%) ⬇️
bitshares/utils.py 40% <0%> (-3.34%) ⬇️
bitshares/transactionbuilder.py 66.09% <0%> (-0.72%) ⬇️
bitsharesbase/operations.py 74.48% <0%> (-0.71%) ⬇️
bitsharesbase/memo.py 90.38% <0%> (-0.53%) ⬇️
bitshares/exceptions.py 100% <0%> (ø) ⬆️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb85138...bec15bd. Read the comment docs.

@xeroc
Copy link
Member

xeroc commented Apr 27, 2018

Why would you not always want to obtain the required fee from the API server? What's the usecase (other than removing one API call)?

@jhtitor
Copy link
Contributor Author

jhtitor commented Apr 27, 2018

  1. get_required_fees doesn't work correctly for Transfer_from_blind and Blind_transfer operations in bitshares-core. Instead a local fee calculator is used. And yes 5 * GRAPHENE_PRECISION is hard-coded, on the wallet side!

  2. When doing Transfer_from_blind, you have to include the fee INTO Blind_transfer amount, meaning you have to pre-calculate the fee for the operation, before it is fully constructed. This is not a serious issue, as long you only have to do it once, however...

  3. ...there is yet another bug/issue in bitshares-core, which forces you to precalculate this fee TWO times, for one and for two outputs, so we can actually avoid TWO round-trips to server.

Plus: I think it's nice to have such a feature. You cache fee definitions, and then you can re-calculate fees locally.

@jhtitor jhtitor mentioned this pull request May 8, 2018
6 tasks
@xeroc
Copy link
Member

xeroc commented May 28, 2018

Since this is stealth related, can we have a separated utils file or even a Utils class that can deal with those methods?

@jhtitor
Copy link
Contributor Author

jhtitor commented Jun 22, 2018

Are you asking if there is any specific reason for this to be part of BitShares class and not in a separate class? No, there's no specific reason for that, I just had no idea where to put it. I'll gladly move it out, however, I've also noticed that latest develop has Fee class. Should this patch be rewritten to utilize that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants