[Wallet] Migrate app view functions to contractkit #1381

annakaz · 2019-10-17T00:38:56Z

Description

Use contractkit in app, and use it instead of walletkit to view exchange rate and fetch balances in wallet.

Tested

Tested on device using alfajores to ensure that exchange rate and balances can be fetched. Also tested from cold start to ensure fetchTokenBalanceWithRetry used in import works.

Other changes

Add contract decimal caching
Moved the logic in actions.ts to saga.ts to follow redux saga convention

Related issues

Partially addresses Migrate Mobile App to the new contractkit #475

Backwards compatibility

Yes

annakaz · 2019-10-17T00:48:10Z

Because of the file rename, it can be difficult to see the diff. The only function that changed is doFetchExchangeRate(), which I have compared in an online diff checker to make it easier to diff here:
https://www.diffchecker.com/A3OPDvZG

codecov · 2019-10-17T01:01:18Z

Codecov Report

Merging #1381 into master will decrease coverage by 0.15%.
The diff coverage is 25%.

@@            Coverage Diff             @@
##           master    #1381      +/-   ##
==========================================
- Coverage   65.49%   65.34%   -0.16%     
==========================================
  Files         271      271              
  Lines        8167     8160       -7     
  Branches      501      491      -10     
==========================================
- Hits         5349     5332      -17     
- Misses       2699     2714      +15     
+ Partials      119      114       -5

Flag	Coverage Δ
#mobile	`65.34% <25%> (-0.16%)`	⬇️

Impacted Files	Coverage Δ
packages/mobile/src/invite/saga.ts	`80.53% <ø> (ø)`	⬆️
packages/mobile/src/stableToken/saga.ts	`81.81% <ø> (ø)`	⬆️
packages/mobile/src/goldToken/saga.ts	`0% <ø> (ø)`	⬆️
packages/mobile/src/exchange/actions.ts	`100% <ø> (+71.84%)`	⬆️
packages/mobile/src/exchange/saga.ts	`0% <0%> (ø)`	⬆️
packages/mobile/src/exchange/reducer.ts	`66.66% <100%> (+6.66%)`	⬆️
packages/mobile/src/import/saga.ts	`87.75% <100%> (ø)`	⬆️
packages/mobile/src/web3/contracts.ts	`56.52% <100%> (+1.97%)`	⬆️
packages/mobile/src/tokens/saga.ts	`87.17% <81.81%> (-5.42%)`	⬇️
...kages/mobile/src/transactions/TransferFeedItem.tsx	`82.45% <0%> (-0.88%)`	⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 167d6c4...76228df. Read the comment docs.

annakaz · 2019-10-17T17:21:15Z

Updated with proper yield call, new diff can be seen here
https://www.diffchecker.com/A3OPDvZG

cmcewen

Nice!!!! As I'm looking, I see that we don't have any tests for these functions - would you mind adding one or two?

jmrossy

The changes mostly lgtm so far, thanks for moving this logic to the saga file.
Just a few questions below

jmrossy · 2019-10-19T07:53:17Z

packages/mobile/src/exchange/saga.ts

+
+    const goldTokenContract: GoldTokenType = yield call(getGoldTokenContract, web3)
+    const stableTokenContract: StableTokenType = yield call(getStableTokenContract, web3)
+    const exchangeContract: ExchangeType = yield call(getExchangeContract, web3)


Can this be converted to contact kit too?

These contracts are used for the exchange transaction. I plan to migrate these along with other state changing functions in a future PR

jmrossy · 2019-10-19T07:53:52Z

packages/mobile/src/exchange/saga.ts

+  try {
+    yield call(getConnectedAccount)
+
+    const contractKit = newKitFromWeb3(web3)


Should we have the kit live in web3/saga or somewhere else where more components can reach for it?

jmrossy · 2019-10-19T07:54:16Z

packages/mobile/src/exchange/saga.ts

+
+async function convertToContractDecimals(value: BigNumber | string | number, contract: any) {
+  // TODO(Rossy): Move this function to SDK and cache this decimals amount
+  const decimals = await contract.methods.decimals().call()


Can this use contract kit too?

annakaz · 2019-10-22T22:23:47Z

packages/mobile/src/tokens/saga.ts

+      tokenContract = await contractKit.contracts.getStableToken()
+      break
+    default:
+      throw new Error(`Could not fetch contract for unknown token ${token}`)


This error is caught in tokenFetchFactory

…norepo into annakaz/app-contractkit

cmcewen

woot!

cmcewen · 2019-10-22T22:57:06Z

packages/mobile/src/exchange/saga.ts

+    yield call(getConnectedAccount)
+    const exchange = yield call([contractKit.contracts, contractKit.contracts.getExchange])
+
+    const dollarMakerExchangeRate: BigNumber = yield call(


I think you could parallelize these calls for speed

cmcewen · 2019-10-22T22:57:42Z

packages/mobile/src/exchange/saga.ts

+  try {
+    const account: string = yield call(getConnectedUnlockedAccount)
+    const exchangeRatePair: ExchangeRatePair = yield select(
+      (state: RootState) => state.exchange.exchangeRatePair


nit - move to a selector

cmcewen · 2019-10-22T23:15:42Z

packages/mobile/src/web3/contracts.ts

@@ -13,6 +14,7 @@ import { Provider } from 'web3/providers'
 const tag = 'web3/contracts'

 export const web3: Web3 = getWeb3()
+export const contractKit = newKitFromWeb3(web3)


Will this be okay when toggling to zero sync?

We may need to update the contractkit provider as well, but I can deal with that in the zeroSync PR. Thanks for flagging this

Follow up on this, tested after merging in the ZeroSync PR and everything works as expected

…norepo into annakaz/app-contractkit

annakaz · 2019-10-24T20:27:47Z

packages/mobile/src/stableToken/saga.test.ts

@@ -93,16 +108,6 @@ describe('stableToken saga', () => {
      .run()
  })

-  // TODO(cmcewen): Figure out how to mock this so we can get actual contract calls


Note that I removed this test as a getStableToken call is already tested in the 'should fetch the balance and put the new balance' test above.

jmrossy · 2019-10-25T16:27:13Z

packages/mobile/src/tokens/saga.ts

+
+async function getWeiPerUnit(token: CURRENCY_ENUM) {
+  let weiPerUnit = contractWeiPerUnit[token]
+  if (!weiPerUnit) {


Nice to see this cached 👍

jmrossy · 2019-10-25T16:27:58Z

packages/mobile/src/tokens/saga.ts

 import { getConnectedAccount, getConnectedUnlockedAccount } from 'src/web3/saga'
 import * as utf8 from 'utf8'

 const TAG = 'tokens/saga'

+// The number of wei that represent one unit in a contract
+const contractWeiPerUnit: { [key in CURRENCY_ENUM]: BigNumber | null } = {


Can a wei per unit differ for two tokens? If not probably not need to cache two diff values for it

Yes it can differ and is set different for gold and stabletokens, it's a configurable parameter here: https://github.com/celo-org/celo-monorepo/blob/master/packages/protocol/contracts/stability/StableToken.sol#L113

jmrossy · 2019-10-25T16:28:27Z

packages/mobile/src/tokens/saga.ts

+
+export async function getTokenContract(token: CURRENCY_ENUM) {
+  Logger.debug(TAG + '@getTokenContract', `Fetching contract for ${token}`)
+  await getConnectedAccount()


Why does the account need to be connected for this?

Switched to wait for web3 sync, which is needed for contractkit to get the token contracts

…repo into aaronmgdr/invite-page * 'aaronmgdr/invite-page' of github.com:celo-org/celo-monorepo: (63 commits) Fix compile error during local clean build (#1506) Update to RN 61 and AndroidX (#1343) Set usage of shuffled round robin in the genesis block (#1464) Add spanish backup key for backup key flow (#1500) Fix sync tests by pulling genesis block to determine epoch length (#1504) [Wallet] fix missing full name error alert (#1496) [Wallet + Verification Pool] Add details about generating an app-signature (#1482) Deploy celo's image of ethstats (#1421) Storing previous randomness values (#1197) [Wallet] Fix wei invite bug (#1489) Point all packages to latest ganache-cli master (#1488) Point end-to-end tests back to master (#1469) [Wallet] Migrate app view functions to contractkit (#1381) [Wallet] Add script to translate locale strings (#1485) [Wallet] Update wallet celo client version and add missing translations for backup flow (#1483) [Wallet] Hotfix local currency (#1481) [Wallet] Remove QR debouncing to improve responsiveness (#1480) [Wallet] Upgrade app version to v1.5.1 (#1463) Update governance end-to-end tests to work with changed precompile (#1476) Fixes key_placer.sh when encrypting files (#1465) ...

annakaz added 8 commits October 16, 2019 12:11

WIP

12316b9

Fix confiq for contractkit import

0b69fe3

Add comment to replicate self reference bug

e339991

Remove unused code for debuggin

6ba7a93

Make this runnable

36d784c

Get view exchange working

db50930

Move exchange logic to saga

6ec179e

Move exchange logic to saga not actions

e27424f

annakaz requested review from cmcewen and jmrossy as code owners October 17, 2019 00:38

Merge branch 'master' into annakaz/app-contractkit

6f1d0f4

annakaz assigned jmrossy and cmcewen Oct 17, 2019

annakaz and others added 3 commits October 17, 2019 09:11

Merge branch 'master' into annakaz/app-contractkit

6e4a628

Use proper yield call

48763fa

Merge branch 'master' into annakaz/app-contractkit

86696e5

cmcewen reviewed Oct 17, 2019

View reviewed changes

jmrossy reviewed Oct 19, 2019

View reviewed changes

jmrossy assigned annakaz and unassigned jmrossy and cmcewen Oct 19, 2019

annakaz added 2 commits October 22, 2019 09:27

WIP adding exchange/saga tests

2f9e91f

Move balance fetching to ck. Tested on alfajores

2301cb9

annakaz commented Oct 22, 2019

View reviewed changes

annakaz added 2 commits October 22, 2019 15:38

Add contract decimals caching

8a157d9

add comments

15be5e1

annakaz removed their assignment Oct 22, 2019

annakaz assigned jmrossy Oct 22, 2019

annakaz and others added 4 commits October 22, 2019 15:54

Add decimal caching

d18cacd

Merge branch 'master' into annakaz/app-contractkit

f982a9f

Revert .env file

f5a0917

Merge branch 'annakaz/app-contractkit' of github.com:celo-org/celo-mo…

fd38798

…norepo into annakaz/app-contractkit

cmcewen approved these changes Oct 22, 2019

View reviewed changes

annakaz and others added 2 commits October 22, 2019 16:45

Merge branch 'master' into annakaz/app-contractkit

3daa8c5

WIP fixing tests

ca81485

annakaz assigned annakaz and unassigned jmrossy and cmcewen Oct 23, 2019

annakaz added 6 commits October 23, 2019 09:21

Merge branch 'annakaz/app-contractkit' of github.com:celo-org/celo-mo…

028dd5d

…norepo into annakaz/app-contractkit

Merge branch 'master' into annakaz/app-contractkit

8aec57b

WIP fixing tests

6fec476

WIP with saga tests

750a6af

Fix invite saga tests

821b5c9

Update tests to work with contractkit

d59e21a

annakaz commented Oct 24, 2019

View reviewed changes

Merge branch 'master' into annakaz/app-contractkit

80416ae

jmrossy approved these changes Oct 25, 2019

View reviewed changes

annakaz and others added 2 commits October 25, 2019 09:59

Wait for web3 sync instead of connectedAccount

cbc4e86

Merge branch 'master' into annakaz/app-contractkit

76228df

annakaz merged commit a6a7269 into master Oct 25, 2019

mcortesi deleted the annakaz/app-contractkit branch December 4, 2019 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Wallet] Migrate app view functions to contractkit #1381

[Wallet] Migrate app view functions to contractkit #1381

annakaz commented Oct 17, 2019 •

edited

Loading

annakaz commented Oct 17, 2019 •

edited

Loading

codecov bot commented Oct 17, 2019 •

edited

Loading

annakaz commented Oct 17, 2019

cmcewen left a comment

jmrossy left a comment

jmrossy Oct 19, 2019

annakaz Oct 22, 2019

jmrossy Oct 19, 2019

jmrossy Oct 19, 2019

annakaz Oct 22, 2019

annakaz Oct 22, 2019

cmcewen left a comment

cmcewen Oct 22, 2019

annakaz Oct 22, 2019

cmcewen Oct 22, 2019

cmcewen Oct 22, 2019

annakaz Oct 22, 2019

annakaz Oct 24, 2019

annakaz Oct 24, 2019

jmrossy Oct 25, 2019

jmrossy Oct 25, 2019

annakaz Oct 25, 2019

jmrossy Oct 25, 2019

annakaz Oct 25, 2019

[Wallet] Migrate app view functions to contractkit #1381

[Wallet] Migrate app view functions to contractkit #1381

Conversation

annakaz commented Oct 17, 2019 • edited Loading

Description

Tested

Other changes

Related issues

Backwards compatibility

annakaz commented Oct 17, 2019 • edited Loading

codecov bot commented Oct 17, 2019 • edited Loading

Codecov Report

annakaz commented Oct 17, 2019

cmcewen left a comment

Choose a reason for hiding this comment

jmrossy 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmcewen 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annakaz commented Oct 17, 2019 •

edited

Loading

annakaz commented Oct 17, 2019 •

edited

Loading

codecov bot commented Oct 17, 2019 •

edited

Loading