-
Notifications
You must be signed in to change notification settings - Fork 52
fix: use URL API for proper query parameter encoding in BalanceService #372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Replace manual URL concatenation with the URL API and searchParams for more reliable and standards-compliant query parameter encoding, especially for the chainId parameter. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @ignaciosantise's task in 40s —— View job Code Review Complete
✅ No issues found Review Summary:
The change correctly replaces manual string concatenation with the standard URL API, which automatically handles proper encoding of all query parameters including special characters in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the URL construction in BalanceService to use the URL API and searchParams instead of manual string concatenation. The goal is to ensure proper encoding of query parameters and improve code maintainability.
Changes:
- Replaced manual string concatenation with URL constructor and searchParams API for building the balance API endpoint
- All query parameters (projectId, currency, chainId, st, sv) now use searchParams.set() for proper encoding
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const url = `${baseUrl}${BALANCE_API_PATH}/${address}/balance?projectId=${encodeURIComponent( | ||
| projectId, | ||
| )}¤cy=usd&chainId=${chainId}&st=walletkit&sv=1.0.0`; | ||
| const url = new URL(`${BALANCE_API_PATH}/${address}/balance`, baseUrl); |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential breaking change in URL construction. When BALANCE_API_PATH starts with / (which it does: '/v1/account'), it's treated as an absolute path from the origin when used with the URL constructor. This means:
- Old behavior:
${baseUrl}${BALANCE_API_PATH}- simple concatenation - New behavior: The leading
/in BALANCE_API_PATH makes it resolve from the origin
If ENV_BLOCKCHAIN_API_URL is something like https://example.com/api, the old code would produce https://example.com/api/v1/account/..., but the new code will produce https://example.com/v1/account/... (dropping the /api path component).
To maintain backward compatibility while using the URL API, either:
- Remove the leading
/from BALANCE_API_PATH and ensure baseUrl ends with/, or - Construct the full path first without the leading
/:new URL(\${BALANCE_API_PATH.slice(1)}/${address}/balance`, baseUrl)`, or - If baseUrl is always just an origin without path components, document this requirement clearly.
| const url = new URL(`${BALANCE_API_PATH}/${address}/balance`, baseUrl); | |
| const url = new URL(`${BALANCE_API_PATH.slice(1)}/${address}/balance`, baseUrl); |
Summary
Changes
Updated BalanceService to use the standard URL constructor and searchParams API when building the balance API URL. This replaces the previous approach of manually concatenating query parameters, which only applied encodeURIComponent to the projectId.
Fixes potential encoding issues with special characters in query parameters.