Skip to content

Fix #3486 "Order cancellation fee error"#3492

Merged
sschiessl-bcp merged 6 commits intobitshares:developfrom
xiangxn:fix-3486
Apr 23, 2022
Merged

Fix #3486 "Order cancellation fee error"#3492
sschiessl-bcp merged 6 commits intobitshares:developfrom
xiangxn:fix-3486

Conversation

@xiangxn
Copy link
Copy Markdown
Contributor

@xiangxn xiangxn commented Apr 12, 2022

Handle issues mentioned in #3486

Closes #3486

@abitmore abitmore changed the title fix 3486 Fix #3486 "Order cancellation fee error" Apr 12, 2022
Copy link
Copy Markdown
Contributor

@sschiessl-bcp sschiessl-bcp left a 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

@sschiessl-bcp
Copy link
Copy Markdown
Contributor

@xiangxn I have introduced a parameter to getFinalFeeAsset that allows us to not create duplicate code, please have a look at it.
Generally, your getPossibleFees was declared a getter method, but did manipulate the array or balance that the user fed into it. This should be avoided to not confuse the caller

@sschiessl-bcp
Copy link
Copy Markdown
Contributor

@xiangxn please test my change again and if it still does what you wanted it to do, let's merge

@xiangxn
Copy link
Copy Markdown
Contributor Author

xiangxn commented Apr 18, 2022

@xiangxn I have introduced a parameter to getFinalFeeAsset that allows us to not create duplicate code, please have a look at it. Generally, your getPossibleFees was declared a getter method, but did manipulate the array or balance that the user fed into it. This should be avoided to not confuse the caller

A reference parameter is required here, which will simplify the operation logic, so it is also an independent method to distinguish it from getPossibleFees.

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.

@abitmore
Copy link
Copy Markdown
Member

abitmore commented Apr 18, 2022

a front-end accumulated fee is required

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.

@xiangxn
Copy link
Copy Markdown
Contributor Author

xiangxn commented Apr 19, 2022

@xiangxn
Copy link
Copy Markdown
Contributor Author

xiangxn commented Apr 19, 2022

I reverted the branch and removed the logic of quoting parameters, and also dealt with the problem of not compiling.

@sschiessl-bcp @abitmore

@xiangxn
Copy link
Copy Markdown
Contributor Author

xiangxn commented Apr 19, 2022

getFinalFeeAsset2 is not compatible with getFinalFeeAsset, because one is for batch processing and the other is for single fee checking. If you want to force one, you may only leave getFinalFeeAsset2, and modify a lot of code used by the getFinalFeeAsset version.

My English is terrible, don't know if you can understand what I'm saying?😄

@sschiessl-bcp
Copy link
Copy Markdown
Contributor

sschiessl-bcp commented Apr 19, 2022

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.
https://github.com/bitshares/bitshares-ui/tree/bugfix/3486_cancel_default_fee_selection

I probably don't understand it correctly at the moment, can you please tell me
a) what behavior it is that you want to have
b) and which case I can test to see how my code does not fulfill it?

In any case, we need to avoid duplicate code if it is at the end just a very small change to the business logic.

@sschiessl-bcp sschiessl-bcp force-pushed the develop branch 2 times, most recently from bc2f1ad to d0a0996 Compare April 23, 2022 09:32
@xiangxn
Copy link
Copy Markdown
Contributor Author

xiangxn commented Apr 23, 2022

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

@sschiessl-bcp
Copy link
Copy Markdown
Contributor

Ah, now I understand. My way was doing the check only for one op, but we want it for ALL ops in the loop.

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.

Order cancellation fee error

3 participants