Conversation
1aa450a to
be885c9
Compare
src/v3/v2converter.ts
Outdated
| // Helper to add crypto asset | ||
| const addCrypto = (currencyCode: string, date: Date): void => { | ||
| const asset = currencyCodeToAsset(currencyCode, currencyCodeMaps) | ||
| const key = `${asset.pluginId}_${ |
There was a problem hiding this comment.
I can't because the key needs to include the date. Parsing would break with toCryptoKey since it doesn't put an _null for null tokenIds which would be hard to parse if I tack an _[date] at the end of it.
There was a problem hiding this comment.
I mentioned the wrong util. There's toDatedCryptoKey (and toDatedFiatKey) hiding in the src/v3/router file instead of utils. For the very issue you mentioned it puts the date in the front
| ): Promise<void> => { | ||
| const delayMs = frequencyToMs[frequency] | ||
| const delayMs = | ||
| typeof frequency === 'number' ? frequency * 1000 : frequencyToMs[frequency] |
There was a problem hiding this comment.
Use the frequency as-is without multiplying by 1000
There was a problem hiding this comment.
The frequency value is specified in seconds not ms.
There was a problem hiding this comment.
The function treats it as seconds but it isn't specified as such. This could be typed instead as
type Frequency = 'minute' | 'hour' | 'day' | 'week' | 'month'
type FrequencySeconds = number
then the definition becomes:
const createEngineLoop = async (
providerId: string,
engine: RateEngine,
frequency: Frequency | FrequencySeconds
): Promise<void> => { ): Promise<void> => {
| engine: tokenMapping | ||
| }, | ||
| { | ||
| frequency: 120, |
There was a problem hiding this comment.
hence why the engine multiplies by 1000. anything less than 1 sec is worthless as a frequency
There was a problem hiding this comment.
I understand why you used seconds here. I'm suggesting to pass ms here because I think the createEngineLoop definition is ambiguous. If you make the suggested type change then this comment is no longer relevant
be885c9 to
bdf5016
Compare
| ): Promise<void> => { | ||
| const delayMs = frequencyToMs[frequency] | ||
| const delayMs = | ||
| typeof frequency === 'number' ? frequency * 1000 : frequencyToMs[frequency] |
There was a problem hiding this comment.
The function treats it as seconds but it isn't specified as such. This could be typed instead as
type Frequency = 'minute' | 'hour' | 'day' | 'week' | 'month'
type FrequencySeconds = number
then the definition becomes:
const createEngineLoop = async (
providerId: string,
engine: RateEngine,
frequency: Frequency | FrequencySeconds
): Promise<void> => { ): Promise<void> => {
| engine: tokenMapping | ||
| }, | ||
| { | ||
| frequency: 120, |
There was a problem hiding this comment.
I understand why you used seconds here. I'm suggesting to pass ms here because I think the createEngineLoop definition is ambiguous. If you make the suggested type change then this comment is no longer relevant
src/v3/v2converter.ts
Outdated
| // Helper to add crypto asset | ||
| const addCrypto = (currencyCode: string, date: Date): void => { | ||
| const asset = currencyCodeToAsset(currencyCode, currencyCodeMaps) | ||
| const key = `${asset.pluginId}_${ |
There was a problem hiding this comment.
I mentioned the wrong util. There's toDatedCryptoKey (and toDatedFiatKey) hiding in the src/v3/router file instead of utils. For the very issue you mentioned it puts the date in the front
bdf5016 to
523b059
Compare
v2 requests into converter MUST have dates
523b059 to
d1f284c
Compare
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
none