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] Remove fee calculation for CELO withdrawals #5042

Merged
merged 3 commits into from
Sep 16, 2020
Merged

Conversation

gnardini
Copy link
Contributor

@gnardini gnardini commented Sep 10, 2020

Description

The fee estimation was being done using cUSD so users were seeing a "Insufficient balance" error if they didn't have a cUSD balance even if the withdrawal was possible anyways.

In the exchange flow the fee is hard coded as a 0, we don't have fee estimation calculation for CELO yet, so I switched the calculation for a hard coded 0 here as well for now.

Tested

In the simulator

I'll add e2e tests soon for an account without cUSD balance that will cover this case. Depends on this PR: #4989

Copy link
Contributor

@tarikbellamine tarikbellamine left a comment

Choose a reason for hiding this comment

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

LGTM - left a small suggestion but could see it going either way.

@@ -29,7 +24,8 @@ export default function WithdrawCeloSummary({
recipientAddress,
}: WithdrawCeloProps) {
const { t } = useTranslation(Namespaces.exchangeFlow9)
const currentAccount = useSelector(currentAccountSelector)
// TODO: Estimate real fee.
const fee = new BigNumber(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead have this be a small non-zero number? My thinking is it's better to overestimate rather than underestimate when showing the user costs. I think this would also be good because it will ensure the user has enough CELO to pay for gas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll preface this by saying that I don't fully understand how the fees are calculated, but:

  • This number is for showing on the UI only, not the real one. I tried using the max number and there are no fee issues. Not sure why. Withdrawing without cUSD balance also works.
  • If we put something non-zero here, it would still be rounded to 0 since we only show three decimals, and whatever fee is charged is lower than that. So leaving it as 0 seems fine to me.
  • Also, the buy/sell CELO flow has a 0 too and I don't think we've received any complaints or issues from that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get what you mean - the number is only an estimate for display and doesn't impact the actual transaction at all. If the buy/sell CELO flow is 0 as well, I think leaving it at 0 here makes sense. We can keep it consistent for now and change all of them if/when we decide to.

@gnardini gnardini added the automerge Have PR merge automatically when checks pass label Sep 16, 2020
@mergify mergify bot merged commit 6dc16d1 into master Sep 16, 2020
@mergify mergify bot deleted the withdraw-celo-fee branch September 16, 2020 14:40
m-chrzan pushed a commit that referenced this pull request Sep 19, 2020
### Description

The fee estimation was being done using cUSD so users were seeing a "Insufficient balance" error if they didn't have a cUSD balance even if the withdrawal was possible anyways.

In the exchange flow the fee is hard coded as a 0, we don't have fee estimation calculation for CELO yet, so I switched the calculation for a hard coded 0 here as well for now.

### Tested

In the simulator

I'll add e2e tests soon for an account without cUSD balance that will cover this case. Depends on this PR: #4989
ewilz pushed a commit to ewilz/celo-monorepo that referenced this pull request Sep 29, 2020
### Description

The fee estimation was being done using cUSD so users were seeing a "Insufficient balance" error if they didn't have a cUSD balance even if the withdrawal was possible anyways.

In the exchange flow the fee is hard coded as a 0, we don't have fee estimation calculation for CELO yet, so I switched the calculation for a hard coded 0 here as well for now.

### Tested

In the simulator

I'll add e2e tests soon for an account without cUSD balance that will cover this case. Depends on this PR: celo-org#4989
@Lss-Ankit
Copy link

Lss-Ankit commented Oct 2, 2020

@tarikbellamine I have verified this issue on latest test flight build (25) & Android internal build Android internal build 1.2.0(1004294313) found that user is able to withdraw CELO even though user 0 cUSD.
Also, observed that esitmation fee is remain 0 while withdrawing CELO
Device: iPhone 11 (14iOS), Samsung Galaxy A70(9.0)

Please let me know if you want to test any thing else.

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 wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants