Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix the fetching of exchange rates #255

Merged
merged 4 commits into from
Apr 20, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/mobile/src/exchange/reducer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Actions, ActionTypes } from 'src/exchange/actions'
import { getRehydratePayload, REHYDRATE, RehydrateAction } from 'src/redux/persist-helper'
import { RootState } from 'src/redux/reducers'

export const MAX_HISTORY_RETENTION = 30 * 24 * 3600 * 1000 // (ms) ~ 180 days
export const MAX_HISTORY_RETENTION = 30 * 24 * 3600 * 1000 // (ms) ~ 30 days
export const ADDRESS_LENGTH = 42

export interface ExchangeRatePair {
Expand Down
47 changes: 27 additions & 20 deletions packages/mobile/src/firebase/saga.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,55 +100,62 @@ function celoGoldExchangeRateHistoryChannel(lastTimeUpdated: number) {
}

const now = Date.now()
// timestamp + 1 is used because .startAt is inclusive
const startAt = Math.max(lastTimeUpdated + 1, now - MAX_HISTORY_RETENTION)

return eventChannel((emit: any) => {
const emitter = (snapshot: FirebaseDatabaseTypes.DataSnapshot) => {
const singleItemEmitter = (snapshot: FirebaseDatabaseTypes.DataSnapshot) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we define these outside of the eventChannel? It makes it a little more difficult to follow when all these functions are nested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't because it uses the emit thing that is returned in the eventChannel :/

emit([snapshot.val()])
}
let cancelFunction = () => {}
const listenForNewElements = (newElementsStartAt: number) => {
cancelFunction = firebase
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this called cancelFunction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was doing this wrongly. The idea was to return a function that is called when channel is canceled so you stop listening, but I was not doing it correctly, we need to call the off function in the Firebase node ref. Switched to the correct implementation

.database()
.ref(`${EXCHANGE_RATES}/cGLD/cUSD`)
.orderByChild('timestamp')
.startAt(newElementsStartAt)
.on('child_added', singleItemEmitter, errorCallback)
}
const fullListEmitter = (snapshot: FirebaseDatabaseTypes.DataSnapshot) => {
const result: ExchangeRate[] = []
snapshot.forEach((childSnapshot: FirebaseDatabaseTypes.DataSnapshot) => {
result.push(childSnapshot.val())
return undefined
})
emit(result)
if (result.length) {
emit(result)
listenForNewElements(result[result.length - 1].timestamp + 1)
} else {
listenForNewElements(startAt)
}
}

// timestamp + 1 is used because .startAt is inclusive
const startAt = lastTimeUpdated + 1 || now - MAX_HISTORY_RETENTION

const onValueChange = firebase
firebase
.database()
.ref(`${EXCHANGE_RATES}/cGLD/cUSD`)
.orderByChild('timestamp')
.startAt(startAt)
.on(VALUE_CHANGE_HOOK, emitter, errorCallback)

const cancel = () => {
firebase
.database()
.ref(`${EXCHANGE_RATES}/cGLD/cUSD`)
.orderByChild('timestamp')
.startAt(startAt)
.off(VALUE_CHANGE_HOOK, onValueChange)
}
.once(VALUE_CHANGE_HOOK, fullListEmitter, errorCallback)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this PR but why is VALUE_CHANGE_HOOK = 'value'? I would assume it would equal child_added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you pass value it returns the whole collection/object in one go. child_added is used so that the callback is called once for each child immediately and then whenever a new child is added it's called again. There's also a child_updated, child_moved and child_removed

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, so I find it confusing that VALUE_CHANGE_HOOK doesn't mean return values when there is a change but instead means return all values. Maybe just me though


return cancel
return cancelFunction
})
}

export function* subscribeToCeloGoldExchangeRateHistory() {
yield call(waitForFirebaseAuth)
const history = yield select(exchangeHistorySelector)
const chan = yield call(celoGoldExchangeRateHistoryChannel, history.lastTimeUpdated)
const channel = yield call(celoGoldExchangeRateHistoryChannel, history.lastTimeUpdated)
try {
while (true) {
const exchangeRates = yield take(chan)
const exchangeRates = yield take(channel)
const now = getRemoteTime()
yield put(updateCeloGoldExchangeRateHistory(exchangeRates, now))
}
} catch (error) {
Logger.error(`${TAG}@subscribeToCeloGoldExchangeRateHistory`, error)
} finally {
if (yield cancelled()) {
chan.close()
channel.close()
}
}
}
Expand Down
22 changes: 22 additions & 0 deletions packages/mobile/src/redux/migrations.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { DEFAULT_DAILY_PAYMENT_LIMIT_CUSD } from 'src/config'
import { initialState as exchangeInitialState } from 'src/exchange/reducer'
import { CicoProviderNames } from 'src/fiatExchanges/reducer'
import { migrations } from 'src/redux/migrations'
import { v0Schema, v1Schema, v2Schema, v7Schema, v8Schema, vNeg1Schema } from 'test/schemas'
Expand Down Expand Up @@ -177,4 +178,25 @@ describe('Redux persist migrations', () => {
expect(migratedSchema.app.bitfyUrl).toBeUndefined()
expect(migratedSchema.app.flowBtcUrl).toBeUndefined()
})
it('works for v11 to v12', () => {
const appStub = 'dont delete please'
const exchangeStub = 'also dont delete please'
const stub = {
exchange: {
otherExchangeProps: exchangeStub,
history: {
celoGoldExchangeRates: [1, 2, 3],
aggregatedExchangeRates: [4, 5, 6],
granularity: 0,
range: 0,
lastTimeUpdated: 100,
},
},
app: appStub,
}
const migratedSchema = migrations[12](stub)
expect(migratedSchema.app).toEqual(appStub)
expect(migratedSchema.exchange.otherExchangeProps).toEqual(exchangeStub)
expect(migratedSchema.exchange.history).toEqual(exchangeInitialState.history)
})
})
11 changes: 11 additions & 0 deletions packages/mobile/src/redux/migrations.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import _ from 'lodash'
import { DEFAULT_DAILY_PAYMENT_LIMIT_CUSD } from 'src/config'
import { initialState as exchangeInitialState } from 'src/exchange/reducer'
import { providersDisplayInfo } from 'src/fiatExchanges/reducer'
import { AddressToDisplayNameType } from 'src/identity/reducer'

Expand Down Expand Up @@ -166,4 +167,14 @@ export const migrations = {
app: _.omit(state.app, 'pontoEnabled', 'kotaniEnabled', 'bitfyUrl', 'flowBtcUrl'),
}
},
12: (state: any) => {
// Removing the exchange rate history because it's very likely that it's repeated a bunch of times.
return {
...state,
exchange: {
...state.exchange,
history: { ...exchangeInitialState.history },
},
}
},
}
2 changes: 1 addition & 1 deletion packages/mobile/src/redux/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ let lastEventTime = Date.now()

const persistConfig: any = {
key: 'root',
version: 11, // default is -1, increment as we make migrations
version: 12, // default is -1, increment as we make migrations
keyPrefix: `reduxStore-`, // the redux-persist default is `persist:` which doesn't work with some file systems.
storage: FSStorage(),
blacklist: ['geth', 'networkInfo', 'alert', 'fees', 'recipients', 'imports'],
Expand Down