#2973 simplified order form for ordinary users

sschiessl-bcp commented 5 years ago

General

Closes #2973

I've implemented a new component QuickTrade. To open it you can go to http://localhost:8080/quick-trade or you can push the button QuickTrade in Dashboard->Activity->click on any currency pair:

quicktrade2

quicktrade

General

Please make sure the following is done:

Pull request is onto develop
If you haven't already, have a look at
- https://github.com/bitshares/bitshares-ui/blob/develop/CODE_OF_CONDUCT.md
- https://github.com/bitshares/bitshares-ui/blob/develop/CONTRIBUTING.md

Code Preparation

Please review all your changes one last time before committing

[ ] Check for unused code
[ ] No unrelated changes are included
[ ] None of the changed files are reformatting only
[ ] Code is self explanatory or documented
[ ] All written text is properly translated (english language)

Testing

The branch has been tested on the following browsers (desktop and mobile view)

[ ] Chrome
[ ] Opera
[ ] Firefox
[ ] Safari

User interface changes

Delete this section if there weren't any UI changes. Please make sure you tested your changes in all themes

[ ] Dark
[ ] Light
[ ] Midnight

Please provide screenshots/licecap of your changes below

sschiessl-bcp commented 5 years ago

Check comment here https://github.com/bitshares/bitshares-ui/pull/3037#issuecomment-522066143

sschiessl-bcp commented 5 years ago

You accidently opened the PR against bitshares repo, I opened a new one here

Metizik commented 5 years ago

I have performed a components replacement for those that use style-guide. I also performed dev testing: 1) I have compared several markets in my new UI with the values that the Exchange tab shows 2) I entered zero and large values in the fields for entering the quantity. When entering zero, zero is displayed in the opposite amount field When entering a large value in the sell amount field, the market depth is displayed in the reсeive amount field When entering a large value in the reсeive amount field, Infinity is now displayed in the sell amount field

Sell Asset selector displays a list of assets that are on the balance of the current user. Receive Asset selector initially displays the same list, but the user can write in the search field the asset he is interested in. I decided to do so that the whole list of assets would not load at once. The user needs to enter at least three characters to start the search. I have added a tooltip that tells this.

The total fee percentage is calculated as the sum of the market fee as a percentage and the transaction fee as a percentage. Is this correct? @sschiessl-bcp

sschiessl-bcp commented 5 years ago

if only one asset is selected, selector should not disable automatically
have a look at the margins hear and streamline
if user selects as asset to sell the asset that is currently locked in as to receive, switch assets
is the liquidity penalty calculated correctly?
please enable a switch like we have in debt column of margin position overview, if I click on an entry in the row "AMOUNT (CNY)" in orders it switches to " AMOUNT (CNY)" and shows cumulative sums.
add price base and quote in the header of orders and in the price collapser
the down arrow of the collapser component is hidden by the ttext
i added the icon "instant-trade". please create a new Spotlight tile (use dummy translations)

Metizik commented 5 years ago

@sschiessl-bcp , working on. Can you, please, clarify these points:

if user selects as asset to sell the asset that is currently locked in as to receive, switch assets

I don't actually understand - we want to reset fields of asset to receive, if user selects the same asset to sell, right?

is the liquidity penalty calculated correctly?

I used the formulas you provided to calculate the liquidity penalty: https://github.com/bitshares/bitshares-ui/issues/2973#issuecomment-518204343 It seems to be ok. If there is an specific way to check this - I will recheck.

please enable a switch like we have in debt column of margin position overview, if I click on an entry in the row "AMOUNT (CNY)" in orders it switches to " AMOUNT (CNY)" and shows cumulative sums.

I am not sure I will be able to complete this point today.

Rest of points is understood and now in progress

sschiessl-bcp commented 5 years ago

Assume transaction fee is paid in sellAsset.

Please refactor to reduce code duplication:

add method _getTransactionFee(denominationAsset) and use where transaction fee needs to be calculated. If denomincationAsset is sellAsset object, then return fee as denominated in sellAsset. If it is receiveAsset, then return transaction fee as denominated in receiveAsset. For that, calculate sellAsset fee, and use last market price to do the conversion (because we pay the actual fee in sellAsset, this is just a guess via market price). default is sellAsset denomination
add method _getMarketFee(denomindatedAsset) with the same logic (default denomination is receiveAsset, since market fee only applies to receive asset)
refactor such that view and internal numbers are streamlines. For example: variables of percent should always carry a value from 0 to 1 (and not 100). Do the covnersion into string % only in the render parts (e.g. getTotalPercentFee should return a value between 0 and 1, conversion into string % happens in the view
getTotalPercentFee also needs to include the liquidty penalty (use the one with respect to market price)
something weird is happening when calculating amount Notice how the second picture has a higher price, and then price goes down again in third picture. Matching of orders needs to be done sorted from best to worst price.
if there is no exact asset match, please display the icon "unknown" (i just added it)

sschiessl-bcp commented 5 years ago

@sschiessl-bcp , working on. Can you, please, clarify these points:

if user selects as asset to sell the asset that is currently locked in as to receive, switch assets

I don't actually understand - we want to reset fields of asset to receive, if user selects the same asset to sell, right?

If user has: to sell BTS, to receive bitCNY, and now selects bitCNY in the to sell combobox, swap assets. Please implement.

is the liquidity penalty calculated correctly?

I used the formulas you provided to calculate the liquidity penalty: bitshares#2973 (comment) It seems to be ok. If there is an specific way to check this - I will recheck.

I fixed it, was visualization problem

Metizik commented 5 years ago

@sschiessl-bcp , will do. Can you please explain, what is the reason behind points 1 and 2, so that I did it right next time on the first pass?

sschiessl-bcp commented 5 years ago

Can you please explain, what is the reason behind points 1 and 2, so that I did it right next time on the first pass?

The reason is the ability to display the cost of the transaction in whatever asset the user pleases, while the actual fees are still paid in either the receiving asset for market fee and mostly BTS for transaction fee. I want to be able to carry a notion of X% of total fee, and then it makes sense only if all fees can be denominated in the same asset

sschiessl-bcp commented 5 years ago

Assume I have asset to sell and receive selected, and amounts entered.
- If I change the asset to sell, the amount of asset to receive should remain and not be reset, and the correct amount for asset to sell recalculated. Same goes other way around
- When swapping assets, amount should also be persisted (amounts are also swapped, and not removed)
only show feedprice if the chosen assets are bitassets with its collateral (e.g. bitCNY and BTS)
the orders should per default subscribe to the market and update itself when order book is updated (just like in exchange). implement a toggle "subscribe to market" within the collapsed component that user can uncheck and subscription is halted and he can inspect the orders

sschiessl-bcp commented 5 years ago

Forwarded.

blockchainprojects / bitshares-ui