Add USD ROSE prices #6
Conversation
ptrus
force-pushed
the
ptrus/feature/usd-prices
branch
3 times, most recently
from
b385ce2 to
268047b
Compare
ptrus
force-pushed
the
ptrus/feature/usd-prices
branch
from
4e57e50 to
d0a4079
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds USD pricing functionality for ROSE tokens in both staking rewards and accounting events exports. The implementation fetches historical ROSE/USD price data from a cached JSON file in the repository, which is automatically updated daily via a GitHub Actions workflow that pulls data from Binance.
Key Changes:
- Added USD price columns (
usd_priceandusd_amount/usd_reward) to CSV exports for ROSE and wROSE transactions - Implemented automated price data updates from Binance API via scheduled GitHub Actions workflow
- Added comprehensive test coverage for price calculations and data handling
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/prices/rose-usd.json | Historical ROSE/USD price data from November 2020 to December 2025 (1859 daily prices) |
| src/utils.js | Added retry logic, price fetching utilities, and progress callback support for paginated fetching |
| src/fetchStakingRewards.js | Integrated USD price calculations for staking rewards based on period end dates |
| src/fetchEvents.js | Added USD pricing for ROSE and wROSE token transactions in accounting events |
| src/pages/StakingRewards.jsx | Updated UI to include new USD price fields in CSV export and documentation |
| src/pages/AccountingEvents.jsx | Updated UI to include new USD price fields in CSV export and documentation |
| src/fetchStakingRewards.test.js | Added comprehensive test suite for USD price calculations and field ordering |
| src/fetchEvents.test.js | Added test coverage for USD price lookups and amount calculations |
| scripts/update-prices.js | Node script to fetch latest prices from Binance and merge with existing data |
| scripts/update-prices.test.js | Test suite for the price update script functionality |
| scripts/backfill-prices.cjs | One-time script to backfill historical price data from Binance |
| .github/workflows/update-prices.yml | Daily scheduled workflow to automatically update price data |
| .github/workflows/ci-build.yml | Added test execution step to CI pipeline |
| .changelog/6.feature.md | Changelog entry for the new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/fetchEvents.js
Outdated
| const isRoseOrWrose = (row) => { | ||
| if (row.symbol === "ROSE") return true; | ||
| if (row.contract && normalizeAddress(row.contract) === WROSE_CONTRACT) return true; |
There was a problem hiding this comment.
The normalizeAddress function is called on the contract address within isRoseOrWrose, but the WROSE_CONTRACT constant is already normalized. This results in double normalization which is redundant. Consider removing the normalization call in the isRoseOrWrose function since WROSE_CONTRACT is already normalized at line 14.
src/fetchEvents.js
Outdated
| * @returns {string} - USD amount formatted to 2 decimal places | ||
| */ | ||
| const calculateUsdAmount = (amount, decimals, price) => { | ||
| if (!price) return null; | ||
| const tokenAmount = parseFloat(amount) / Math.pow(10, decimals); |
There was a problem hiding this comment.
The calculateUsdAmount function returns null when price is falsy, but doesn't validate that the amount parameter is valid. If amount is not a valid number string, parseFloat will return NaN, which will then be used in the multiplication, resulting in "NaN" being returned by toFixed(2). Consider adding validation for the amount parameter similar to how calculateUsdFromRose handles it in fetchStakingRewards.js.
| * @returns {string} - USD amount formatted to 2 decimal places | |
| */ | |
| const calculateUsdAmount = (amount, decimals, price) => { | |
| if (!price) return null; | |
| const tokenAmount = parseFloat(amount) / Math.pow(10, decimals); | |
| * @returns {string|null} - USD amount formatted to 2 decimal places, or null if inputs are invalid | |
| */ | |
| const calculateUsdAmount = (amount, decimals, price) => { | |
| if (!price) return null; | |
| const parsedAmount = parseFloat(amount); | |
| if (Number.isNaN(parsedAmount)) { | |
| return null; | |
| } | |
| const tokenAmount = parsedAmount / Math.pow(10, decimals); |
src/pages/StakingRewards.jsx
Outdated
| </li> | ||
| <li> | ||
| <strong>usd_price, usd_reward</strong> - USD price at period end and reward value | ||
| (prices from Binance) |
There was a problem hiding this comment.
The documentation states prices are from Binance, but this is inconsistent with the actual implementation which fetches prices from a GitHub raw URL. While the prices in GitHub may originally come from Binance, the documentation should be more accurate about where the CSV exporter gets the prices from. Consider clarifying that prices are cached in the repository and sourced from Binance.
| (prices from Binance) | |
| (USD prices cached in this repository, originally sourced from Binance) |
src/pages/AccountingEvents.jsx
Outdated
| </li> | ||
| <li> | ||
| <strong>usd_price, usd_amount</strong> - USD price and value at time of transaction | ||
| (ROSE and wROSE only, prices from Binance) |
There was a problem hiding this comment.
The documentation states prices are from Binance, but this is inconsistent with the actual implementation which fetches prices from a GitHub raw URL. While the prices in GitHub may originally come from Binance, the documentation should be more accurate about where the CSV exporter gets the prices from. Consider clarifying that prices are cached in the repository and sourced from Binance.
| (ROSE and wROSE only, prices from Binance) | |
| (ROSE and wROSE only, prices cached in this repository and originally sourced from Binance) |
.changelog/6.feature.md
Outdated
| @@ -0,0 +1 @@ | |||
| Add USD ROSE prices at the day of the transacitons | |||
There was a problem hiding this comment.
There is a spelling error in the changelog. "transacitons" should be "transactions".
| Add USD ROSE prices at the day of the transacitons | |
| Add USD ROSE prices at the day of the transactions |
| const fetchWithRetry = async (fetchFn, maxRetries = 3, baseDelay = 1000) => { | ||
| let lastError; | ||
| for (let attempt = 0; attempt <= maxRetries; attempt++) { | ||
| try { | ||
| return await fetchFn(); | ||
| } catch (error) { | ||
| lastError = error; | ||
| if (attempt < maxRetries) { | ||
| const delay = baseDelay * Math.pow(2, attempt); | ||
| await sleep(delay); | ||
| } | ||
| } | ||
| } | ||
| throw lastError; | ||
| }; |
There was a problem hiding this comment.
The fetchWithRetry function should include logging for better observability. When a request fails and is retried, it would be helpful to log which attempt is being made. Consider adding a console log or debug statement that indicates the retry attempt number and delay being used.
ptrus
force-pushed
the
ptrus/feature/usd-prices
branch
from
d0a4079 to
41fef8c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 17 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/fetchEvents.js
Outdated
| * @param {string} amount - Amount in base units | ||
| * @param {number} decimals - Token decimals | ||
| * @param {number} price - USD price per token | ||
| * @returns {string} - USD amount formatted to 2 decimal places |
There was a problem hiding this comment.
The JSDoc indicates the return type is string, but the function can also return null when price or amount is missing. The return type should be documented as string|null to accurately reflect all possible return values.
| * @returns {string} - USD amount formatted to 2 decimal places | |
| * @returns {string|null} - USD amount formatted to 2 decimal places, or null if calculation is not possible |
ptrus
force-pushed
the
ptrus/feature/usd-prices
branch
from
41fef8c to
ca58506
Compare
Fixes: #5