app: Fix swap fee summary #2057
Conversation
The fee estimates for tokens on the UI was not accounting for the existance of tokens. When trading tokens, the fee is paid in the parent asset, so it doesn't make sense to display the fee as a percentage of the order amount. The fee summary pane now displays the actual fee estimate ranges instead of the percentage, and the fee details pane only shows percentages for non-token assets.
There was a problem hiding this comment.
On the form, I think there could be an explanation for what the (%) is displaying for non-tokens. I don't think it is very obvious that it is the percent of total trade.
On simnet, eth will say "Order estimates and options unavailable" because "gas required exceeds allowance (big number)". Why was this?
This comment was marked as resolved.
This comment was marked as resolved.
It seems like somehow the updates to the |
|
Let me try building again? Oh yeah, looks ok after building again. Must have been a mistake on my side. Will hide the comment. |
| "fee_projection_tooltip": "如果在您的订单匹配之前网络条件没有改变，则总费用（作为交易的百分比）应该在这个范围内。", | ||
| "fee_projection_tooltip": "如果在您的订单匹配之前网络条件没有改变，则总费用应该在这个范围内。", |
There was a problem hiding this comment.
Haha no spaces in Chinese.
| <div class="d-flex flex-row align-items-center justify-content-center"> | ||
| <img class="micro-icon mx-1" data-icon="from"> | ||
| <span id="summarySwapFeesLow"></span> | ||
| - |
There was a problem hiding this comment.
How about <span class="px-1">–</span>
| <td><span id="vSwapFeesLow"></span> (<span id="vSwapFeesLowPct"></span>)</td> | ||
| <td><span id="vRedeemFeesLow"></span> (<span id="vRedeemFeesLowPct"></span>)</td> | ||
| <td><span id="vSwapFeesLow"></span> <span id="vSwapFeesLowPct"></span></td> | ||
| <td><span id="vRedeemFeesLow"></span> <span id="vRedeemFeesLowPct"></span></td> |
There was a problem hiding this comment.
If you are doing another commit, can you remove the weird extra space after span in this element tag?
I don't have time to fix this right now. I think this PR can go in without it though, because that issue was already existing.
This is happening because when doing |
Yup, all good here. |
There was a problem hiding this comment.
LGTM. @buck54321 pls merge when you're good with it, or we can push a fix to it if needed.
| for (const icon of Doc.applySelector(page.vDetailPane, '[data-icon]')) { | ||
| const setIcon = (icon: PageElement) => { | ||
| switch (icon.dataset.icon) { | ||
| case 'from': | ||
| icon.src = Doc.logoPath(fromAsset.symbol) | ||
| if (fromAsset.token) { | ||
| const parentAsset = app().assets[fromAsset.token.parentID] | ||
| icon.src = Doc.logoPath(parentAsset.symbol) | ||
| } else { | ||
| icon.src = Doc.logoPath(fromAsset.symbol) |
There was a problem hiding this comment.
This new setIcon assumes the icon will only be used to represent the fee asset, not the traded asset. If the order verification form wants to put the icons for the traded assets, this helper won't be of help. Seems like we need [data-icon] for the traded asset and [data-fee-icon] for the fee asset icon. Fine in this PR though.
The fee estimates for tokens on the UI was not accounting for the existance of tokens. When trading tokens, the fee is paid in the parent asset, so it doesn't make sense to display the fee as a percentage of the order amount. The fee summary pane now displays the actual fee estimate ranges instead of the percentage, and the fee details pane only shows percentages for non-token assets.
Closes #2041