Add fee estimation for Withdrawing CELO and pay for Withdrawing and Selling CELO fees in CELO #5345

gnardini · 2020-10-15T22:04:52Z

Description

Add a fee calculation for withdrawing CELO. This will be removed soonish when we do offline fee calculations (it's an epic in the upcoming milestone). This is meant as a quick fix in the meantime to not have a 0 hard-coded.
When withdrawing CELO or selling CELO. The fee is now paid for with CELO instead of cUSD.

I didn't add fee estimations for exchanging because it uses more than one transaction and made the calculation somewhat more complex, it will probably be improved doing offline estimations.

Tested

Tested it in an emulator, just looking at the estimate and paying attention to the exact CELO balance in the logs when doing different operations to see that the balance changes more than expected (meaning that gas was paid for in CELO) only when doing withdrawals and selling CELO.

Related issues

Fixes Exchange and Withdraw CELO transcations fail when moving full balance #5258

gnardini · 2020-10-15T22:06:48Z

packages/mobile/src/exchange/WithdrawCeloScreen.tsx


 type Props = StackScreenProps<StackParamList, Screens.WithdrawCeloScreen>

+const RANDOM_ADDRESS = '0xDCE9762d6C1fe89FF4f3857832131Ca18eE15C66'


We don't have an address here yet to do the estimation, and estimating requires an address, so I used a random one. Could that cause any issues?

This is fine, we already do this here as well.

gnardini · 2020-10-15T22:10:49Z

packages/mobile/src/transactions/send.ts

+  // TODO: Make it transparent for the user.
+  // TODO: Check for balance should be more than fee instead of zero.
+  const feeCurrency =
+    preferredFeeCurrency === CURRENCY_ENUM.DOLLAR && stableTokenBalance.isGreaterThan(0)


This was working like this before and I was worried about changing it in case someone only has CELO and needs to pay for gas for something, but I would understand not wanting to have it anymore.

Doesn't this create a potential discrepancy between what we show users and what we actually do? For example, if someone has both cUSD and CELO, we will always show them the fee in CELO but then will always deduct their fee from their cUSD balance.

If my understanding is correct, I think we need to get rid of this. It looks like you've set the default currency = cUSD for the param in the relevant function calls but I'd definitely test it out to be safe.

Gotcha, I totally misread this the first time. Sounds good!

jeanregisser

Great work! 👍

jeanregisser · 2020-10-16T15:15:34Z

packages/mobile/src/transactions/send.ts

+  const feeCurrencyAddress =
+    feeCurrency === CURRENCY_ENUM.DOLLAR
+      ? yield call(getCurrencyAddress, CURRENCY_ENUM.DOLLAR)
+      : undefined


nit: Would be nice to add a small comment explaining it's normal that we pass undefined when the feeCurrency is CELO.

jeanregisser · 2020-10-16T15:29:25Z

packages/mobile/src/exchange/WithdrawCeloScreen.tsx

+  useEffect(() => {
+    getSendFee(
+      accountAddress,
+      CURRENCY_ENUM.GOLD,
+      {
+        recipientAddress: RANDOM_ADDRESS,
+        amount: goldBalanceNumber,
+        comment: '',
+      },
+      false
+    )
+      .then((estimate: BigNumber) => {
+        setFeeEstimate(divideByWei(estimate))
+      })
+      .catch((error) => {
+        Logger.warn('WithdrawCeloScreen', `Error found while estimating gas fee: ${error}`)
+      })
+  }, [])
+


Async functions should be avoided in general in useEffect because they can trigger actions after the component has been unmounted.

useAsync from react-async-hook solves this.

Now we have CalculateFee which uses useAsync under the hood and mostly does what you want here.

I originally created it as a component because most components we had at the time, were class components.
But we could export a useSendFee hook from it now.

Added benefit is it provides loading/error states so we can display a spinner while it's calculating and it displays an error if it fails.

Thanks for this! I had run into this many times and always solved it in this inelegant way, the useAsync hook looks great.

jeanregisser · 2020-10-16T15:36:24Z

packages/mobile/src/exchange/WithdrawCeloScreen.tsx


 type Props = StackScreenProps<StackParamList, Screens.WithdrawCeloScreen>

+const RANDOM_ADDRESS = '0xDCE9762d6C1fe89FF4f3857832131Ca18eE15C66'


This is fine, we already do this here as well.

nategraf

All the parts that I am familiar with look good. I am less familiar with the react native components, so I am going to defer to @jeanregisser for final approval.

nategraf · 2020-10-16T19:42:01Z

packages/mobile/src/exchange/WithdrawCeloReviewScreen.test.tsx

@@ -14,6 +14,7 @@ const SAMPLE_AMOUNT = new BigNumber(5.001)
 const mockScreenProps = getMockStackScreenProps(Screens.WithdrawCeloReviewScreen, {
  recipientAddress: SAMPLE_ADDRESS,
  amount: SAMPLE_AMOUNT,
+  feeEstimate: new BigNumber(1),


Is this 1 wei, and if so, is that displayed to the user at all? If the fee was actually 1 wei, it would likely not be significant enough to warrant mentioning (unless they are trying to withdraw 100% of their balance, in which case we might help craft a request that won't leave any dust behind.)

Thanks for reviewing! The wei to CELO mapping happens in WithdrawCeloScreen, line 67. The converted fee is then passed to this review screen and this is a test of it where I put a != 0 number just to have something there.

tarikbellamine

Nice work! One comment I think needs to be addressed

tarikbellamine · 2020-10-16T20:53:43Z

packages/mobile/src/exchange/WithdrawCeloScreen.tsx

 function WithdrawCeloScreen({ navigation }: Props) {
  const [accountAddress, setAccountAddress] = useState('')
  const [celoInput, setCeloToTransfer] = useState('')

  const goldBalance = useSelector((state) => state.goldToken.balance)
-  const goldBalanceNumber = new BigNumber(goldBalance || 0)
+  const goldBalanceNumber = useMemo(() => new BigNumber(goldBalance || 0), [goldBalance])


Why use useMemo here?

My understanding for the best use cases of this is for when there are expensive calculations involved, otherwise the storage costs are not worth it.

Yeah, great catch, I'd like not to have it either, the problem is that we're creating a new BigNumber instance on each render and we pass this to the useSendFee hook, which uses it to see whether dependencies changed or not. If we pass different instances it assumes it changed and re-calculates the fee, which is undesired. The useMemo caches the instance of BigNumber so we use always the same unless the total balance changes for some reason.

Oh, that's an interesting problem and a creative solution.

I don't feel strongly about doing this now but an alternative is to pass an int value instead and convert to BigNumber within the useSendFee function. If this would require a bunch of other changes, I'd understand if you'd rather not do it. But if that's the case, please leave a "to-do" so we fix this later.

As a side note, do you think we are having this issue in other places we are doing fee estimates?

Missed this comment somehow. I changed the amount to a string since I saw it was being transformed to string right after anyways.

I checked the other places but didn't find something like this happening

tarikbellamine · 2020-10-16T21:43:11Z

packages/mobile/src/transactions/send.ts

+  // TODO: Make it transparent for the user.
+  // TODO: Check for balance should be more than fee instead of zero.
+  const feeCurrency =
+    preferredFeeCurrency === CURRENCY_ENUM.DOLLAR && stableTokenBalance.isGreaterThan(0)


Doesn't this create a potential discrepancy between what we show users and what we actually do? For example, if someone has both cUSD and CELO, we will always show them the fee in CELO but then will always deduct their fee from their cUSD balance.

If my understanding is correct, I think we need to get rid of this. It looks like you've set the default currency = cUSD for the param in the relevant function calls but I'd definitely test it out to be safe.

gnardini · 2020-10-16T21:58:41Z

Doesn't this create a potential discrepancy between what we show users and what we actually do? For example, if someone has both cUSD and CELO, we will always show them the fee in CELO but then will always deduct their fee from their cUSD balance.

If my understanding is correct, I think we need to get rid of this. It looks like you've set the default currency = cUSD for the param in the relevant function calls but I'd definitely test it out to be safe.

Before this change we used cUSD if there was enough balance and CELO otherwise.

The new parameter preferredFeeCurrency is passed as GOLD for the two cases where we would prefer to pay for gas with CELO (withdrawing and selling CELO) and DOLLAR otherwise. In those two cases, the condition would evaluate to false and we would get a feeCurrency of GOLD so we'd pay for gas with CELO.

The condition is to maintain the logic we used to have for all other cases (where preferredFeeCurrency is DOLLAR). I just didn't want to risk forcing an operation to fail if the user has no cUSD if they have enough CELO to pay for gas. Maybe that's not what we want though, I'm just keeping what was already there for all cases except the two I mentioned.

tarikbellamine

LGTM!

jeanregisser

Great! 👍

jeanregisser · 2020-10-20T15:07:20Z

packages/mobile/src/components/CeloAmountInput.tsx

    </TextInputWithButtons>
  )
 }

 const styles = StyleSheet.create({
  maxAmount: fontStyles.small600,
+  loadingContainer: {},


Suggested change

loadingContainer: {},

Lss-Ankit · 2020-11-06T08:00:24Z

Hi @jeanregisser , @gnardini Verified task using latest Test flight build v1.4.0 (28) and Android play store (internal build) v1.4.0 (1004294317) and found the following:

CELO is cut as fee while withdrawing CELO and Selling CELO.
Observed that cUSD balance is not effected due to withdraw and Sell CELO.
Observed that Fee .001 is shown while withdrawing CELO but Fee is not cut as CELO 0.001 it slight different. we have it checked from https://explorer.celo.org.
Also, Observed that user is able to withdraw full amount by tapping on MAX button while withdrawing CELO.

Can you please let us know 3rd and 4th point are expected and Also, let us know if we need to verify anything else.

Thanks!!

jeanregisser · 2020-11-24T13:08:33Z

All good thanks. The fee is probably rounded up for display.

Gonzalo Nardini added 3 commits October 15, 2020 17:19

Add fee estimation for withdrawals

e1218a0

Pay for withdrawal gas in CELO

ee4faf4

Use CELO for fees when selling CELO

4274aac

gnardini added the wallet label Oct 15, 2020

gnardini requested review from cmcewen, i1skn, jeanregisser and jmrossy as code owners October 15, 2020 22:04

gnardini commented Oct 15, 2020

View reviewed changes

Gonzalo Nardini added 2 commits October 15, 2020 19:15

Fix test

c830b74

Fix another test

b7d9d9a

jeanregisser reviewed Oct 16, 2020

View reviewed changes

Gonzalo Nardini added 3 commits October 16, 2020 14:08

Pr fixes

750d1f6

useSendFee hook

dc240a9

Fix test

00fd474

nategraf reviewed Oct 16, 2020

View reviewed changes

tarikbellamine reviewed Oct 16, 2020

View reviewed changes

tarikbellamine approved these changes Oct 16, 2020

View reviewed changes

jeanregisser approved these changes Oct 20, 2020

View reviewed changes

Gonzalo Nardini added 2 commits October 20, 2020 12:11

Remvoe unused style

2d614f0

Use string for amount to avoid useMemo

cea9876

gnardini added the automerge Have PR merge automatically when checks pass label Oct 20, 2020

Merge branch 'master' into withdraw-fees

9150ae9

mergify bot merged commit e931005 into master Oct 20, 2020

mergify bot deleted the withdraw-fees branch October 20, 2020 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fee estimation for Withdrawing CELO and pay for Withdrawing and Selling CELO fees in CELO #5345

Add fee estimation for Withdrawing CELO and pay for Withdrawing and Selling CELO fees in CELO #5345

gnardini commented Oct 15, 2020 •

edited

Loading

gnardini Oct 15, 2020 •

edited

Loading

jeanregisser Oct 16, 2020

gnardini Oct 15, 2020

tarikbellamine Oct 16, 2020

tarikbellamine Oct 16, 2020

jeanregisser left a comment

jeanregisser Oct 16, 2020

jeanregisser Oct 16, 2020

gnardini Oct 16, 2020

jeanregisser Oct 16, 2020

nategraf left a comment

nategraf Oct 16, 2020

gnardini Oct 16, 2020

tarikbellamine left a comment

tarikbellamine Oct 16, 2020

gnardini Oct 16, 2020

tarikbellamine Oct 16, 2020 •

edited

Loading

gnardini Oct 20, 2020 •

edited

Loading

tarikbellamine Oct 16, 2020

gnardini commented Oct 16, 2020

tarikbellamine left a comment

jeanregisser left a comment

jeanregisser Oct 20, 2020

Lss-Ankit commented Nov 6, 2020

jeanregisser commented Nov 24, 2020


		type Props = StackScreenProps<StackParamList, Screens.WithdrawCeloScreen>

		const RANDOM_ADDRESS = '0xDCE9762d6C1fe89FF4f3857832131Ca18eE15C66'

Add fee estimation for Withdrawing CELO and pay for Withdrawing and Selling CELO fees in CELO #5345

Add fee estimation for Withdrawing CELO and pay for Withdrawing and Selling CELO fees in CELO #5345

Conversation

gnardini commented Oct 15, 2020 • edited Loading

Description

Tested

Related issues

gnardini Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nategraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarikbellamine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarikbellamine Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

gnardini Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnardini commented Oct 16, 2020

tarikbellamine left a comment

Choose a reason for hiding this comment

jeanregisser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lss-Ankit commented Nov 6, 2020

jeanregisser commented Nov 24, 2020

gnardini commented Oct 15, 2020 •

edited

Loading

gnardini Oct 15, 2020 •

edited

Loading

tarikbellamine Oct 16, 2020 •

edited

Loading

gnardini Oct 20, 2020 •

edited

Loading