Fix/safe price sorting - #474
Conversation
…egistered_at field in tests
|
@otobongdev Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughThe change adds fixed-point USDC price parsing and applies it to service sorting. Invalid prices sort after valid prices without producing ChangesService price sorting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Hi @otobongdev, This PR could not be merged because it has merge conflicts with the target branch. Please resolve the merge conflicts, push the updated changes, and the PR can be reviewed and merged. Thank you! |
|
done |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/lib/sort.test.ts`:
- Around line 4-13: Update the makeAgent fixture to include defaults for the
required AgentEntry fields name, description, owner, successful_payments,
failed_payments, total_volume_stroops, last_active, flagged, and flag_reason,
while preserving the existing overrides spread so callers can replace any
default.
In `@frontend/lib/sort.ts`:
- Around line 15-21: Update the price parsing logic around match[2] and the
fixed-point conversion so fractional parts longer than seven digits return null
instead of being truncated by slice(0, 7). Preserve zero-padding for fractions
with seven or fewer digits and keep the existing safe-integer validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68d465dd-bbc8-41dd-8a31-d86678b469f8
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
frontend/lib/sort.test.tsfrontend/lib/sort.ts
| function makeAgent(overrides: Partial<AgentEntry> = {}): AgentEntry { | ||
| return { | ||
| address: 'GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWFL', | ||
| score: 0, | ||
| total_payments: '0', | ||
| registered_at: '100', | ||
| active: true, | ||
| ...overrides, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the factory and its declared return contract.
ast-grep outline frontend/lib/sort.test.ts --items all --match makeAgent
sed -n '4,14p' frontend/lib/sort.test.ts
sed -n '87,102p' frontend/lib/types.tsRepository: Stellar-Ecosystem/lodestar
Length of output: 951
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Package version and tsconfig(s):"
if [ -f package.json ]; then
jq -r '.devDependencies.typescript // .dependencies.typescript // empty' package.json
fi
fd -a 'tsconfig\.json$' . | sed 's#^\./##' | sort
echo
echo "TS availability:"
if command -v npx >/dev/null 2>&1; then
npx tsc --version 2>/dev/null || true
fi
if command -v tsc >/dev/null 2>&1; then
tsc --version 2>/dev/null || true
fi
echo
echo "Check TS diagnostic for sort.test.ts (read-only):"
if command -v tsc >/dev/null 2>&1; then
tsc --noEmit --strict frontend/lib/sort.test.ts frontend/lib/types.ts 2>&1 || true
fiRepository: Stellar-Ecosystem/lodestar
Length of output: 7318
Make the fixture satisfy AgentEntry.
makeAgent returns AgentEntry, but the object omits required fields: name, description, owner, successful_payments, failed_payments, total_volume_stroops, last_active, flagged, and flag_reason. Add defaults so overrides: Partial<AgentEntry> = {} can still produce a complete AgentEntry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/lib/sort.test.ts` around lines 4 - 13, Update the makeAgent fixture
to include defaults for the required AgentEntry fields name, description, owner,
successful_payments, failed_payments, total_volume_stroops, last_active,
flagged, and flag_reason, while preserving the existing overrides spread so
callers can replace any default.
| const intPart = match[1]; | ||
| const fracPart = (match[2] ?? '').padEnd(7, '0').slice(0, 7); | ||
| const combined = `${intPart}${fracPart}`; | ||
| const normalized = combined.replace(/^0+/, '') || '0'; | ||
| const result = Number(normalized); | ||
|
|
||
| return Number.isSafeInteger(result) && result >= 0 ? result : null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject prices with more than seven fractional digits.
Line 16 silently truncates excess precision. For example, "1.00000009" becomes the valid value "1.0000000" instead of sorting as an invalid price. Reject fractions longer than seven digits before fixed-point conversion.
Proposed fix
const intPart = match[1];
- const fracPart = (match[2] ?? '').padEnd(7, '0').slice(0, 7);
+ const fraction = match[2] ?? '';
+ if (fraction.length > 7) return null;
+ const fracPart = fraction.padEnd(7, '0');📝 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.
| const intPart = match[1]; | |
| const fracPart = (match[2] ?? '').padEnd(7, '0').slice(0, 7); | |
| const combined = `${intPart}${fracPart}`; | |
| const normalized = combined.replace(/^0+/, '') || '0'; | |
| const result = Number(normalized); | |
| return Number.isSafeInteger(result) && result >= 0 ? result : null; | |
| const intPart = match[1]; | |
| const fraction = match[2] ?? ''; | |
| if (fraction.length > 7) return null; | |
| const fracPart = fraction.padEnd(7, '0'); | |
| const combined = `${intPart}${fracPart}`; | |
| const normalized = combined.replace(/^0+/, '') || '0'; | |
| const result = Number(normalized); | |
| return Number.isSafeInteger(result) && result >= 0 ? result : null; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/lib/sort.ts` around lines 15 - 21, Update the price parsing logic
around match[2] and the fixed-point conversion so fractional parts longer than
seven digits return null instead of being truncated by slice(0, 7). Preserve
zero-padding for fractions with seven or fewer digits and keep the existing
safe-integer validation.
2 participants
Closes #375
Summary by CodeRabbit