fix: safeguard against getEtherspotProvider crash in delegated mode#503
fix: safeguard against getEtherspotProvider crash in delegated mode#503
Conversation
📝 WalkthroughWalkthroughAdds a new hook Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Deploying x with
|
| Latest commit: |
0b8260f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e36bae58.x-e62.pages.dev |
| Branch Preview URL: | https://fix-delegated-eoa-signing.x-e62.pages.dev |
Deploying pillarx-debug with
|
| Latest commit: |
0b8260f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1f4c7781.pillarx-debug.pages.dev |
| Branch Preview URL: | https://fix-delegated-eoa-signing.pillarx-debug.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/apps/perps/hooks/useHyperliquid.ts`:
- Around line 79-80: The hook useHyperliquid contains a duplicated `else` token
causing a parse error; locate the conditional block inside useHyperliquid (the
`if ... else` chain around the lines with the duplicated `else`) and remove the
extra `else` so the control flow has a single matching `else` block (or merge
the intended logic into one `else` branch), ensuring braces remain balanced and
the function compiles.
github-actions
bot
temporarily deployed
to
Preview (fix/delegated-eoa-signing)
github-actions
bot
temporarily deployed
to
Preview (fix/delegated-eoa-signing)
github-actions
bot
temporarily deployed
to
Preview (fix/delegated-eoa-signing)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/apps/pillarx-app/hooks/useAlgoInsightsStats.ts`:
- Around line 23-27: The hook useAlgoInsightsStats currently ignores the error
returned by useTradingSignals; destructure the error (e.g., const { signals,
loading, error } = useTradingSignals({ enabled: true })) and propagate it in the
hook's return value so consumers can distinguish fetch failures from empty data
(update the returned shape to include error alongside stats and loading, and
ensure stats remains null when loading or error is present). Ensure references
to useAlgoInsightsStats and useTradingSignals are updated so callers can handle
error state.
🧹 Nitpick comments (3)
src/apps/pillarx-app/hooks/useAlgoInsightsStats.ts (3)
107-109: Remove leftover comment/question.This appears to be a development note that should be resolved and removed before merging.
🧹 Proposed cleanup
}); - // Ensure we have a starting point? Or just the trade history. - // If history is empty, return empty array return history; };
119-119: Track the placeholderriskLevelimplementation.The hardcoded
'Low Risk'value is noted as placeholder logic. Consider adding a TODO comment with context or creating an issue to track the proper implementation.📝 Suggested documentation
- riskLevel: 'Low Risk', // Placeholder logic + // TODO: Implement risk level calculation based on portfolio volatility/exposure + riskLevel: 'Low Risk',
126-136: Consider extracting 1w calculation for consistency.The
1wperiod calculates its value inline while1m,3m, and6muse pre-computed variables. This inconsistency slightly reduces readability and maintainability.♻️ Optional refactor for consistency
Add a pre-computed variable alongside the others at lines 36-39:
const now = Date.now(); + const oneWeekAgo = now - 7 * 24 * 60 * 60 * 1000; const oneMonthAgo = now - 30 * 24 * 60 * 60 * 1000;Add the 1w PnL calculation alongside lines 42-55:
+ const pnl1w = calculatePnL((s) => { + const closedAt = s.closed_at ? new Date(s.closed_at).getTime() : 0; + return closedAt > oneWeekAgo; + }); + const pnl1m = calculatePnL((s) => {Then simplify the return:
'1w': { - value: Number( - calculatePnL((s) => - s.closed_at - ? new Date(s.closed_at).getTime() > - now - 7 * 24 * 60 * 60 * 1000 - : false - ).toFixed(2) - ), - history: generateHistory(now - 7 * 24 * 60 * 60 * 1000), + value: Number(pnl1w.toFixed(2)), + history: generateHistory(oneWeekAgo), },
| export const useAlgoInsightsStats = () => { | ||
| const { signals, loading } = useTradingSignals({ enabled: true }); | ||
|
|
||
| const stats = useMemo<AlgoInsightsStats | null>(() => { | ||
| if (loading || signals.length === 0) return null; |
There was a problem hiding this comment.
Missing error state propagation.
The useTradingSignals hook returns an error state (per the relevant code snippet), but it's not destructured or exposed. If signal fetching fails, consumers cannot distinguish between "no data available" and "fetch error occurred" since both result in stats: null.
Consider exposing the error state:
🛡️ Proposed fix to propagate error state
export const useAlgoInsightsStats = () => {
- const { signals, loading } = useTradingSignals({ enabled: true });
+ const { signals, loading, error } = useTradingSignals({ enabled: true });
const stats = useMemo<AlgoInsightsStats | null>(() => {
// ...existing code...
}, [signals, loading]);
- return { stats, loading };
+ return { stats, loading, error };
};🤖 Prompt for AI Agents
In `@src/apps/pillarx-app/hooks/useAlgoInsightsStats.ts` around lines 23 - 27, The
hook useAlgoInsightsStats currently ignores the error returned by
useTradingSignals; destructure the error (e.g., const { signals, loading, error
} = useTradingSignals({ enabled: true })) and propagate it in the hook's return
value so consumers can distinguish fetch failures from empty data (update the
returned shape to include error alongside stats and loading, and ensure stats
remains null when loading or error is present). Ensure references to
useAlgoInsightsStats and useTradingSignals are updated so callers can handle
error state.
1 participant
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
Bug Fixes
New Features