-
Notifications
You must be signed in to change notification settings - Fork 570
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
Fix #3486 "Order cancellation fee error" #3492
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the feeling that getPossibleFees2 and getFinalFeeAsset2 are really almost the same. What is the actual difference? It seems to create lots of duplicate code
@xiangxn I have introduced a parameter to getFinalFeeAsset that allows us to not create duplicate code, please have a look at it. |
@xiangxn please test my change again and if it still does what you wanted it to do, let's merge |
A reference parameter is required here, which will simplify the operation logic, so it is also an independent method to distinguish it from Moreover, the logic here is that a front-end accumulated fee is required, rather than a separate judgment on whether the account balance is sufficient. Therefore, your code must not meet the requirements of this logic. |
As I understood, for a transaction which contains multiple operations, we need to sum up fees of all the operations and check if the account balance and the fee pools are sufficient to pay all the fees. In addition, if an operation would cause a balance change, the change should be counted in when calculating fees. |
This line of code also causes subscript overflow: https://github.com/xiangxn/bitshares-ui/blob/fix-3486/app/lib/common/account_utils.js#L136 |
I reverted the branch and removed the logic of quoting parameters, and also dealt with the problem of not compiling. |
My English is terrible, don't know if you can understand what I'm saying?😄 |
I don't exactly understand yet why we need this complicated logic and duplicate code. I have restored my change to a new branch so allow comparison, I have also fixed the subscript overflow. I probably don't understand it correctly at the moment, can you please tell me In any case, we need to avoid duplicate code if it is at the end just a very small change to the business logic. |
bc2f1ad
to
d0a0996
Compare
In this problem, we need to take out the user's balance at one time and accumulate the required fees at the front end. The original method is to take the data on the chain every time, so it cannot achieve the purpose. I have used the code you provided, modified. @sschiessl-bcp |
Ah, now I understand. My way was doing the check only for one op, but we want it for ALL ops in the loop. |
Handle issues mentioned in #3486
Closes #3486