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

Eliminate online gas estimation from send and invite flows #117

Merged
merged 66 commits into from
Mar 18, 2021

Conversation

nategraf
Copy link
Contributor

Description

As described in celo-org/celo-monorepo#5432, gas estimation is a costly
operation for the mobile client. The most promising short-term solution is to simply hard-code the
gas value to be used in for simple transactions along common code paths. The most common transaction
sent by a typical user is to send cUSD, either by a direct transfer or an invite.

This PR adds the required plumbing to use a single gas value, price and currency throughout the send
and invite flows. As a result, a single constant gas value can be used from start to finish. The
major result of this is that instead of estimating gas online twice, as the app currently does, the
static value is used and online gas estimation is never used.

This represents a large improvement to the experienced transaction speed, especially in less than
ideal network conditions. Additional effects of this are that errors caused by gas estimation should
be reduced, it's possible to give a fee estimate while offline, and the fee estimate shown the user
will be respected as a maximum fee value.

Other changes

  • Remove the unused EstimateFee.tsx
  • Refactor cancellation for sendTransaction
  • Fix gas price cache to handle multiple currencies.
  • Propagate receipts back up from sendTransaction
  • Use a Lock for getting contractkit instance to avoid reliance on polling and flag-based locking.
  • Don't clear the transaction feed when the call to the blockchain API fails, such as when the device is offline.
  • Add networkConnectedSelector to access the network connectivity state reported by the device.

Tested

  • Updated snapshot and unit tests to cover new functionality
  • Manually tests effected user flows: Sending cUSD, escrow, and reclaiming escow

Related issues

Backwards compatibility

No backwards compatibility concerns.

Victor Graf and others added 30 commits October 5, 2020 10:13
…celo-monorepo into hackathon/offline-transactions
…celo-monorepo into hackathon/offline-transactions
@nategraf
Copy link
Contributor Author

Just noticed in the logs the following message.

[Fri Mar 12 2021 17:09:41.193]  INFO     Warning: Cannot update a component from inside the function body of a different component.%s ["
    in CalculateSendFee (at CalculateFee.tsx:138)
    in CalculateFee (at SendConfirmation.tsx:451)
    in SendConfirmation (at SceneView.tsx:122)
    in StaticContainer

I have a feeling it's related to this line below, but I'm not exactly sure what to do differently. Maybe a helpful reviewer could suggest a fix!
https://github.com/celo-org/wallet/blob/ecfa456aa4e2ba7a4b102b4b58a6aa196016b945/packages/mobile/src/send/SendConfirmation.tsx#L244

@etuleu
Copy link
Contributor

etuleu commented Mar 16, 2021

Warning: Cannot update a component from inside the function body of a different component

Sounds like this: https://reactjs.org/blog/2020/02/26/react-v16.13.0.html#warnings-for-some-updates-during-render

Maybe we can fix these in a different PR though unless it's easy to make the change. It's not a huge problem for now, but we should fix these warnings at some point.

Copy link
Contributor

@etuleu etuleu left a comment

Choose a reason for hiding this comment

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

I looked over the changes but for me at this point this is mostly a learning exercise. Would be good to get Jean's eyes over this as well for sure.

@nategraf nategraf added the automerge Have PR merge automatically when checks pass label Mar 18, 2021
@mergify mergify bot merged commit a28dd47 into main Mar 18, 2021
@mergify mergify bot deleted the victor/eliminate-double-estimation branch March 18, 2021 00:59
@ValoraQA
Copy link

Hi @nategraf I have verified this issue using Android Internal Build V1.12.0(1004294340) & observed following:
Observations:

  • 4G Network: Observed that, app takes 25 sec to load sending screen with estimation after tapping on Send button. Also it is observed that more than one gas fees are displaying.
  • 3G Network: Observed that, app takes more that a minute to load estimation on sending screen. Also observed user multiple fees are applied.
  • 512 kbps speed:(using Charles) Observed that, continuous loading indicator displayed for longer time & user not able to proceed from Send screen after tapping on Review button.
    Verified on devices: Samsung Galaxy A5(7.0), iPhone XR (14.2)

Let me know if anything else need to test.
Thanks..!!

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.

5 participants