Skip to content
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

[Wallet] Implement explanations of the fees and use fee drawer on exchanges #4429

Merged
merged 2 commits into from
Jul 20, 2020

Conversation

i1skn
Copy link
Contributor

@i1skn i1skn commented Jul 15, 2020

Description

Implement Dialog with fee explanation when use clicks on "?" icon next to a fee.

Other changes

  • Implement <FeeDrawer /> for Exchanges

Tested

iOS Simulator

Backwards compatibility

Yes

Screenshots

Simulator Screen Shot - iPhone 11 Pro - 2020-07-15 at 16 40 57
Simulator Screen Shot - iPhone 11 Pro - 2020-07-15 at 16 41 00
Simulator Screen Shot - iPhone 11 Pro - 2020-07-15 at 17 31 00

@i1skn i1skn requested review from cmcewen and jmrossy as code owners July 15, 2020 14:44
@i1skn i1skn changed the title Implement explanations of the fees and use fee drawer on exchanges [Wallet] Implement explanations of the fees and use fee drawer on exchanges Jul 15, 2020
@i1skn i1skn requested a review from a team July 15, 2020 14:44
Copy link
Contributor

@annakaz annakaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! Comment on how very small fees are displayed

@@ -39,7 +39,8 @@ export const getExchangeRateDisplayValue = (value: BigNumber.Value): string => {

export const getFeeDisplayValue = (value: BigNumber.Value | null | undefined): string => {
return value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does value evaluate to false for any non-zero values? Wondering whether the case below is ever used for non-zero values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When value is a BigNumber it would evaluate to true independently of the value in it.

@@ -1672,7 +1672,7 @@ exports[`TransferConfirmationCard renders correctly for sent escrow transaction
>

$
0.001
0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming that this change is expected as we now only round up to 0.001 if it is non zero?

Copy link
Contributor Author

@i1skn i1skn Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this is a tough question. When fee is precisely zero, do we want still to show 0.001? cc: @cmcewen @jmrossy @jeanregisser

Copy link
Contributor Author

@i1skn i1skn Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After talking with @cmcewen I'm proposing the following: fees should not be zero in the long run, but now we have tobin tax set to zero and transaction fee is not present in the blockchain api for exchanges. @nityas also has agreed to if the value is exactly 0 - show zero. In the future these fees should be non-zero. Let me know what do you think? cc: @annakaz

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sgtm!

Copy link
Contributor

@annakaz annakaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@i1skn i1skn added the automerge Have PR merge automatically when checks pass label Jul 20, 2020
@mergify mergify bot merged commit c056451 into master Jul 20, 2020
@mergify mergify bot deleted the i1skn/fee-icons branch July 20, 2020 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants