Hoprd v4 #159
Open
Hoprd v4 #159
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughBumps package to 4.0.0; removes channel aggregation and node graph APIs and their tests; renames getPeers query param Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/account/getBalances.ts (1)
42-49: Guard non-string currency values before calling.includes.The schema (src/types/account.ts) defines all currency fields as required strings, but the code processes raw unvalidated JSON before
safeParsevalidation at line 53. If the API returns non-string values for any field—includingsafeHoprAllowance—calling.includes()will throw before schema validation can catch it.Line 42 checks for existence with optional chaining (
jsonResponse?.safeHoprAllowance &&), but this does not guard against non-string types (e.g., a number). Line 48 in the normalization loop has no guard at all. Addtypeofchecks to handle type violations defensively:🛠️ Proposed fix
- jsonResponse.token = jsonResponse?.safeHoprAllowance && jsonResponse.safeHoprAllowance.includes(' ') - ? jsonResponse.safeHoprAllowance.split(' ')[1] - : null; + jsonResponse.token = + typeof jsonResponse?.safeHoprAllowance === 'string' && + jsonResponse.safeHoprAllowance.includes(' ') + ? jsonResponse.safeHoprAllowance.split(' ')[1] + : null; for (let i = 0; i < currencies.length; i++) { const token = currencies[i] as string; - jsonResponse[token] = jsonResponse[`${currencies[i]}`].includes(' ') - ? jsonResponse[token].split(' ')[0] - : jsonResponse[token]; + const value = jsonResponse[`${currencies[i]}`]; + jsonResponse[token] = + typeof value === 'string' && value.includes(' ') + ? value.split(' ')[0] + : value; }
🤖 Fix all issues with AI agents
In `@src/api/account/getAddresses.spec.ts`:
- Around line 94-109: The test name currently claims "APIError" but the code
calls getAddresses and asserts .rejects.toThrow(Error) because getAddresses
throws a plain Error on 5xx; update the test description string in the test
block (the test(...) call) to accurately reflect the behavior (e.g., "should
throw Error on internal server error") or alternatively change the assertion to
expect an APIError and modify getAddresses to throw APIError; reference the test
wrapper and the getAddresses call to locate and update the test description or
the assertion/implementation so the name and actual behavior match.
In `@src/api/node/getPeers.ts`:
- Around line 13-16: In getPeers.ts the current conditional if (payload?.score)
skips valid score=0; change the check to explicitly test for presence (e.g.,
payload?.score !== undefined && payload?.score !== null) before calling
url.searchParams.set so a score of 0 is included; update the block around the
URL creation and the payload.score handling to use that explicit undefined/null
check referencing url and payload.score.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/api/account/getAddresses.ts`:
- Line 38: Remove the debug console.log from the SDK surface that prints
parsedRes in getAddresses; either delete the line "console.log('parsedRes',
parsedRes)" or replace it with a gated debug logger (e.g., use an existing
debug/info logger or a conditional that only logs in development) so user
address data is not printed to stdout. Locate the usage of parsedRes in the
getAddresses function and ensure any logging follows the project's structured
logging/debug pattern rather than raw console.log.
|
|
||
| const jsonResponse = await rawResponse.json(); | ||
| const parsedRes = GetAddressesResponse.safeParse(jsonResponse); | ||
| console.log('parsedRes', parsedRes); |
Contributor
There was a problem hiding this comment.
Remove debug logging from the SDK surface.
This prints parsed addresses and can leak user data or spam consumer logs. Prefer removing or gating behind a debug logger.
🧹 Proposed fix
- console.log('parsedRes', parsedRes);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.log('parsedRes', parsedRes); |
🤖 Prompt for AI Agents
In `@src/api/account/getAddresses.ts` at line 38, Remove the debug console.log
from the SDK surface that prints parsedRes in getAddresses; either delete the
line "console.log('parsedRes', parsedRes)" or replace it with a gated debug
logger (e.g., use an existing debug/info logger or a conditional that only logs
in development) so user address data is not printed to stdout. Locate the usage
of parsedRes in the getAddresses function and ensure any logging follows the
project's structured logging/debug pattern rather than raw console.log.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Breaking Changes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.