Closed sschiessl-bcp closed 5 years ago
sschiessl-bcp
commented
5 years ago There will be massively many places in the UI where fee is used and selected. Please do a quick browse beforehand and find out in what ways the code normally implements the fee and report back here.
We should refactor that to a single piece of fee-defining code.
VerevkinAlexander
commented
5 years ago @sschiessl-bcp I can try to inject FeeSelect modal to the Transactionconfirm component and call it before calling Transactionconfirm modal. What do you think about it?
sschiessl-bcp
commented
5 years ago @Metizik
Like agreed in the call please implement it first for the SendModel and then we will generalize it to the other places in the UI.
I think it would make sense if you create a new FeeSelector component that merges all functionality needed for https://github.com/bitshares/bitshares-ui/blob/a025cf026434a044625229af841f5308985d912d/app/components/Modal/SendModal.jsx#L834 .
If the fee is insufficient, please add a clickable text (analog to the quantity) that opens the new FeeModal.
mettizik
commented
5 years ago I've got acquainted with code around SendModal and AmountSelecter and related storages, got implemented a simple modal UI-part for selection of asset according to wirefames in bitshares#1117. But as for now got a bit stuck on few moments, there are a few issues I would like to discuss:
Regarding point 3 - in case, when user does not have enough default asset to pay fee we could possibly change UI of AmountSelector, though that it had label with currently selected asset and a button called 'Switch to another asset', which will open dialog described in original bitshares#1117 issue. It will be more transparent for user, since he would have an only possibility to change asset - using modal, not drop-down, or other widgets. @sschiessl-bcp , what do you think?
sschiessl-bcp
commented
5 years ago There is the txHelper class as seen in SendModal (). Essentially, you can get account balance like this:
const account = await FetchChain("getAccount", accountName);
const balanceID = account.getIn(["balances", assetId]);
let balanceAmount = (await FetchChain("getObject", balanceID)).get("balance");In the first step I'd suggest to simple extract the existing logic from SendModal in a new FeeSelector componenet (all the logic concerning fees). That should also help get some more clarity what the code is doing. It is quite messy, don't take it as the best way. txHelper is quite confusing.
Modal has a visibility flag (https://github.com/bitshares/bitshares-ui/blob/0edb22253c245b9ec25ff154a8dac03dddc47a21/app/components/Modal/SendModal.jsx#L674). The use of ZfApi is deprecated.
I like the idea with the button, but the asset selector should remain for the experienced user. The inexperienced can click on the button to open the modal and get more information
mettizik
commented
5 years ago I extracted a component for FeeAssetSelector and started to move fee related logic out from SendModal. As for now there are a few questions, found out during refactoring: 1 - What does feeStatus stands for? In all my tests it was an empty object returned here - https://github.com/bitshares/bitshares-ui/blob/0edb22253c245b9ec25ff154a8dac03dddc47a21/app/components/Modal/SendModal.jsx#L294, what made a huge part of code unnecessary in SendModal. 2 - there is an error in SendModal (https://github.com/bitshares/bitshares-ui/blob/0edb22253c245b9ec25ff154a8dac03dddc47a21/app/components/Modal/SendModal.jsx#L356), which also messes lifecycle and logic of the component (it is not necessary a bug, but it looks strange). I think it should be fixed, but since component was working for more than a year already - my assumptions may be falsy. 3 - is fee calculated depending only on memo field? It seems so for me regarding this implementation https://github.com/bitshares/bitshares-ui/blob/0edb22253c245b9ec25ff154a8dac03dddc47a21/app/components/Modal/SendModal.jsx#L404
I am now going to extract all the fee calculation to FeeAssetSelector and output a calculated fee to parent component. Depending on question 3 it will receive account and either memo or other required parameters as input props.
I also added a button which opens SetDefaultFeeAsset modal to FeeAssetSelector component, as it was agreed - without removing drop-down list.
mettizik
commented
5 years ago @sschiessl-bcp , I finished refactoring and modal UI, intermediate results are in branch https://github.com/bitshares/bitshares-ui/compare/develop...blockchainprojects:1117-allow-user-to-select-asset-for-fee
There are a few things I would like to clarify regarding this implementation - I removed some useless parts of code and found an issue in trxHelper (described in comment). But I am still not sure about feeStatus checks, since it did not work in my tests, but it may behave in a different way on production network.
Left to do for now:
sschiessl-bcp
commented
5 years ago 1 - What does feeStatus stands for?
I don'tr remember as well atm, we will need to test the UX and see if it was really necessary
2 - there is an error in SendModal
What is the error?
3 - is fee calculated depending only on memo field?
Check the default values of checkFeeStatusAsync. It defaults to the transfer operation. I checked the current implementation, it does not update the fee when the memo is filled in. Fee is dependent on memo size as well.
But I am still not sure about feeStatus checks, since it did not work in my tests, but it may behave in a different way on production network
How are you currently testing? You can create an account for free on the mainnet and also on the testnet. I can send you some test tokens as well if desired.
found an issue in trxHelper (described in comment)
The naming there is confusing as well .. ultimately shouldPayFeeWithAssetAsync checks if the user has enough balance of the fee asset to the fee amount. It returns true properly, the Promise us resolved while being returned and answers true in line 348, it's not easy readable code though especially mixed with usage of checkBalance
mettizik
commented
5 years ago 1 - I checked it now - it works fine from my point of view on expected UX without feeStatus evaluation
2 - getAvailabeAssets was referring to this.state instead of passed state to get feeStatus. Which may lead to issues, if feeStatus in explicitly passed state is different
3 - Thanks. I believe fee is not updated because fee charge for memo is free ( https://gyazo.com/ed90f60abd356372e5c874f0c38a33dc )
shouldPayFeeWithAssetAsync I see where I was wrong here - I missed that it is returning correct promise on 1.3.0 asset. Since I had enough balance always - it returned undefined. Anyway - I removed it usage in fee selector, since it was not needed due to calculations performed in SendModal, which were much more accurate.
Metizik
commented
5 years ago @sschiessl-bcp , PR with changes https://github.com/blockchainprojects/bitshares-ui/pull/117
sschiessl-bcp
commented
5 years ago All fee calculation should happen within the new component such that it can be re-use anywhere in the UI with no additional business logic for fee calculation. Add callbacks / hooks and props where needed.
Next step: find all separate spots in the UI where fees are calculated and report them here. Following question need to be answered for tech spot: can the new fee selector cover everything? Is any different input needed for new component? Does the visualization match or should it be configurable? Is there any reason the fee calculation logic can not be only within new feeselector component?
There is also the transaction confirm modal that shows the complete payload and the total final fee.
Metizik
commented
5 years ago @sschiessl-bcp , passed a few iterations over all the code related to Fee calculations and displaying.
Regarding code review performed (described below) I believe the best would be to implement following changes:
app/components/Transfer/Transfer.jsx since it has a lot of duplication against previous SendModal revision and FeeAssetSelector can be added easily and will improve code quality significantly. Refactoring would be exactly the same as for SendModal earlierapp/components/Exchange/BuySell.jsx and replace amount selector with new component to preserve latest changes and meet new requirementsFeeAssetSelector to app/components/Blockchain/TransactionConfirm.jsx since it is a place where user may want to change fee asset and proceed. Adding new component there will allow user to choose asset, depending on available balance in different assets, and on my opinion - this will make a minor improvement to UX on this step.trxHelper is used) and remove duplicated logic among UI components.Overall picture is the following:
There are few places which definitely require some modifications regarding new component implemented for Fee asset selection:
I believe it would be a good from UX point of view to add this component to app/components/Blockchain/TransactionConfirm.jsx, since it is a place where user may decide to change Fee asset, but it is optional and not necessarily true.
There are a few components which I could not locate on the first run, which use Fee calculation utils:
app/components/Account/FeePoolOperation.jsx - IMO has no need to integrate fee asset selector, since it is more like AmountSelector (displaying fee), not related to calculation.
And there are lots of places, where fee calculations take place, but those are not so obvious to me and may need just minor refactoring and extra utils implementation for duplication removing:
Fee related code is 90% duplicate of the removed one in SendModal and should be removed.
sschiessl-bcp
commented
5 years ago app/components/Transfer/Transfer.jsx can be removed since its deprecated. Please go ahead with app/components/Blockchain/TransactionConfirm.jsx integration.
Make sure double check that
I saw that you use memo as props for new component. In general, you need a list of operations, or a TransactionBuilder object as props because memo is specific for only the transfer component (which is the default in the fee calc utils). This component should work in general for any operation / transaction containting operaitons
Metizik
commented
5 years ago @sschiessl-bcp , Removed Transfer component. Since Transaction confirm is more informational modal I decided not to integrate there FeeAssetSelector, because this component won't (if I got it right) be able to handle change of fee asset completely by itself and though will mess responsibilities as well. I added a button to change default Fee asset though. Please correct me if I am wrong here Tested fee calculation against mainnet and fixed issues found, thanks Removed all unnecessary rerendering and blockchain fetching Documented proptypes and set defaults where required
Refactored props for FeeAssetSeleector, to receive all options required for checkFeeStatusAsync. I did not found any places where list of operations was used for fee calculation, as I understood count of operations can be passed to checkFeeStatusAsync as option and is used only in ScaledOrders. Can you please point, where I can see usage of operations list is used for fee calculation ?
sschiessl-bcp
commented
5 years ago Since Transaction confirm is more informational modal I decided not to integrate there FeeAssetSelector
Agreed.
Please correct me if I am wrong here
Will see how it feels
Can you please point, where I can see usage of operations list is used for fee calculation ?
Would not be surprised if not used anywhere else
sschiessl-bcp
commented
5 years ago When opening the modal in the settings it does not show any assets, async loading error?
sschiessl-bcp
commented
5 years ago When I'm in Send Modal and click the button to open modal, and also click to change default, the selected new default is not entered as the new fee asset in the send modal
sschiessl-bcp
commented
5 years ago Please remove the button again here, but introduce a question mark icon in the row of the view after BTS with a tooltip "The default asset used to pay transaction fees can be changed in the settings"
Metizik
commented
5 years ago Fixed three issues listed above, now switching to refactoring of TODOs left
mettizik
commented
5 years ago @sschiessl-bcp , could you please review latest changes in PR ? I think according to original issue the next step would be to use saved in settings default asset as default for fee payments, where there are feed operations, isn't it?
sschiessl-bcp
commented
5 years ago @sschiessl-bcp , could you please review latest changes in PR ? I think according to original issue the next step would be to use saved in settings default asset as default for fee payments, where there are feed operations, isn't it?
Not sure what you mean with that. Can you please re-formulate?
mettizik
commented
5 years ago I mean, now when we have a default fee asset setting present I believe user would expect, that selected default fee asset will be used for fee payment in all places where fee is involved.
sschiessl-bcp
commented
5 years ago I mean, now when we have a default fee asset setting present I believe user would expect, that selected default fee asset will be used for fee payment in all places where fee is involved.
A yes, the new FeeComponent should be doing that yes. Are there any places that use the fee, but that don't use the FeeComponent?
mettizik
commented
5 years ago @sschiessl-bcp , yes, lots of - most of them are listed here https://github.com/blockchainprojects/bitshares-ui/issues/111#issuecomment-500469007
https://github.com/bitshares/bitshares-ui/issues/1117
Description of the task is here https://github.com/bitshares/bitshares-ui/issues/1117#issuecomment-492209474
Please make sure that