fix(fiat-services): fix fetching fiat rates for timestamps range #16754
Conversation
vytick
added
the
no-project
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
error Error: http://10.0.0.28:4873/@trezor%2feslint: no such package available WalkthroughThe pull request modifies the Assessment against linked issues
Note: The current code change seems to be a technical adjustment to time-based calculations, but it does not directly resolve the core issues raised in the linked issue about price updates and display standards. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-common/fiat-services/src/coingecko.ts (1)
Line range hint
157-157: Consider normalizing timestamp units.The function returns
ts: new Date().getTime()in milliseconds while working with seconds elsewhere. Consider converting this to seconds for consistency:-ts: new Date().getTime(), +ts: Math.floor(new Date().getTime() / 1000),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
suite-common/fiat-services/src/coingecko.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (3)
suite-common/fiat-services/src/coingecko.ts (3)
12-12: LGTM! Clear and accurate time constant.The constant is correctly defined with a clear name indicating the unit (seconds) and a readable calculation.
139-140: LGTM! Correct timestamp range adjustment.The range adjustment using
ONE_DAY_IN_Sis accurate and aligns with CoinGecko API's timestamp requirements.
139-140: Verify timestamp handling across the codebase.Let's ensure that timestamp units are handled consistently across the codebase.
✅ Verification successful
Timestamp handling is consistent with CoinGecko API requirements
The timestamp calculations are correctly implemented in seconds, which is consistent with CoinGecko API requirements and the rest of the file's timestamp handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for timestamp-related code to verify consistent handling of units # Look for getTime() usage which might expect milliseconds echo "Searching for getTime() usage:" rg "getTime\(\)" -A 2 -B 2 # Look for timestamp comparisons that might mix seconds and milliseconds echo -e "\nSearching for timestamp comparisons:" rg "timestamp[s]?\s*[<>=]" -A 2 -B 2 # Look for CoinGecko API related files that might handle timestamps echo -e "\nSearching for CoinGecko related files:" fd "coingecko" --type fLength of output: 58545
|
🚀 Expo preview is ready!
|
| @@ -136,8 +136,8 @@ export const getFiatRatesForTimestamps = async ( | |||
| const sortedTimestamps = [...timestamps].sort((ts1, ts2) => ts1 - ts2); | |||
|
|
|||
| // adjust from and to timestamps to get better range of data | |||
| const fromTimestamp = sortedTimestamps[0] - ONE_DAY_IN_MS; | |||
| const toTimestamp = sortedTimestamps[sortedTimestamps.length - 1] + ONE_DAY_IN_MS; | |||
| const fromTimestamp = sortedTimestamps[0] - ONE_DAY_IN_S; | |||
There was a problem hiding this comment.
Can you also rename it sortedTimestampsInSeconds?
When fetching fiat rates and picking the best one for given timestamp, the range was increased by wrong number. This caused wrong selection of the closest fiat rate for the timestamp and also wrong 24h change computation.
Related Issue
Related to #15440