(1CT) Add events when creating 1CT session in swap review #3971
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several enhancements to the one-click trading functionality across multiple files. It adds a new function, Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/stores/src/account/base.tsOops! Something went wrong! :( ESLint: 8.50.0 ESLint couldn't find the config "prettier" to extend from. Please check that the name of the config is correct. The config "prettier" was referenced from the config file in "/packages/stores/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (6)
packages/web/hooks/mutations/one-click-trading/use-remove-one-click-trading-session.ts (2)
17-40: Consider refining event logging and error handling.The implementation could be improved in two areas:
- Event logging should be conditional on successful session cleanup
- Error handling for async operations should be added
Consider this improved implementation:
export async function onEnd1CTSession({ accountStore, authenticatorId, logEvent, }: { accountStore: ReturnType<typeof useStore>["accountStore"]; authenticatorId: string; logEvent: ReturnType<typeof useAmplitudeAnalytics>["logEvent"]; }) { - const oneClickTradingInfo = await accountStore.getOneClickTradingInfo(); + try { + const oneClickTradingInfo = await accountStore.getOneClickTradingInfo(); if (oneClickTradingInfo?.authenticatorId === authenticatorId) { accountStore.setOneClickTradingInfo(undefined); accountStore.setShouldUseOneClickTrading({ nextValue: false }); displayToast( { titleTranslationKey: "oneClickTrading.toast.oneClickTradingDisabled", captionTranslationKey: "oneClickTrading.toast.sessionEnded", }, ToastType.ONE_CLICK_TRADING ); + logEvent([EventName.OneClickTrading.endSession]); } - } - logEvent([EventName.OneClickTrading.endSession]); + } catch (error) { + console.error('Failed to end 1CT session:', error); + throw error; + } }
74-81: Enhance transaction error handling.The current error message "Transaction failed" doesn't provide enough context for debugging or user feedback.
Consider this improvement:
onFulfill: (tx) => { if (tx.code === 0) { resolve(tx); } else { - reject(new Error("Transaction failed")); + reject(new Error(`Transaction failed with code ${tx.code}: ${tx.rawLog || 'No error details available'}`)); } },packages/web/hooks/one-click-trading/use-one-click-trading-swap-review.ts (1)
230-230: LGTM with a minor suggestion for type safety.The addition of
authenticatorIdis correct and necessary for proper session tracking. While the non-null assertion is safe in this context (as this code path is only reached when removing an existing session), consider adding a runtime check for better type safety:-authenticatorId: oneClickTradingInfo!.authenticatorId, +authenticatorId: oneClickTradingInfo?.authenticatorId ?? (() => { + throw new Error('Attempting to remove 1CT session without authenticator ID'); +})(),packages/web/hooks/mutations/one-click-trading/use-create-one-click-trading-session.tsx (3)
248-254: Add error handling for event loggingWhile the event logging implementation captures the essential session parameters, consider wrapping it in a try-catch block to ensure that any logging failures don't affect the session creation flow.
- logEvent([ - EventName.OneClickTrading.startSession, - { - spendLimit: Number(transaction1CTParams.spendLimit.toDec().toString()), - sessionPeriod: transaction1CTParams.sessionPeriod.end, - }, - ]); + try { + logEvent([ + EventName.OneClickTrading.startSession, + { + spendLimit: Number(transaction1CTParams.spendLimit.toDec().toString()), + sessionPeriod: transaction1CTParams.sessionPeriod.end, + }, + ]); + } catch (error) { + console.warn('Failed to log one-click trading session start:', error); + // Session creation succeeded, so we don't want to throw the error + }
Line range hint
319-358: Consider using an enum for session periodsThe switch statement for session periods could benefit from using an enum to make the code more maintainable and type-safe.
enum SessionPeriod { FIVE_MIN = '5min', TEN_MIN = '10min', THIRTY_MIN = '30min', ONE_HOUR = '1hour', THREE_HOURS = '3hours', TWELVE_HOURS = '12hours' } const SESSION_PERIOD_DURATIONS: Record<SessionPeriod, { value: number; unit: dayjs.ManipulateType }> = { [SessionPeriod.FIVE_MIN]: { value: 5, unit: 'minute' }, [SessionPeriod.TEN_MIN]: { value: 10, unit: 'minute' }, // ... other periods };🧰 Tools
🪛 Gitleaks (8.21.2)
188-188: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Line range hint
411-447: Consider adding transaction error detailsThe error handling for transaction broadcasting could provide more specific error information to help with debugging.
- reject(new Error("Transaction failed")); + reject(new Error(`Transaction failed with code ${tx.code}: ${tx.rawLog}`));🧰 Tools
🪛 Gitleaks (8.21.2)
188-188: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/web/hooks/limit-orders/use-place-limit.ts(2 hunks)packages/web/hooks/mutations/one-click-trading/use-create-one-click-trading-session.tsx(4 hunks)packages/web/hooks/mutations/one-click-trading/use-remove-one-click-trading-session.ts(2 hunks)packages/web/hooks/one-click-trading/use-one-click-trading-swap-review.ts(1 hunks)packages/web/hooks/use-swap.tsx(3 hunks)
🔇 Additional comments (10)
packages/web/hooks/mutations/one-click-trading/use-create-one-click-trading-session.tsx (2)
186-186: LGTM: Well-typed event logging parameter addition
The addition of the logEvent parameter with proper typing ensures type safety and maintains consistency with the analytics system.
Also applies to: 201-201
483-483: LGTM: Proper event logging integration
The logEvent is correctly obtained from the analytics hook and properly passed to the session creation function.
packages/web/hooks/limit-orders/use-place-limit.ts (3)
20-20: LGTM: Clean import addition
The import statement for onEnd1CTSession is properly added and aligns with the PR's objective of implementing event logging for 1CT sessions.
406-407: LGTM: Event logging integration
The logEvent parameter is correctly passed to both session creation and termination functions, ensuring consistent event tracking.
408-416: Verify error handling for session termination
While the session termination logic is implemented correctly, we should ensure proper error handling for failed session terminations to maintain system stability.
Let's check for existing error handling patterns in the codebase:
Consider wrapping the session termination in a try-catch block to handle potential failures gracefully:
} else if (
shouldSend1CTTx &&
oneClickMessages &&
oneClickMessages.type === "remove-1ct-session"
) {
+ try {
onEnd1CTSession({
accountStore,
authenticatorId: oneClickMessages.authenticatorId,
logEvent,
});
+ } catch (error) {
+ console.error("Failed to end 1CT session:", error);
+ logEvent([EventName.OneClickTrading.sessionEndFailed, { error }]);
+ }
}packages/web/hooks/use-swap.tsx (5)
50-50: Import onEnd1CTSession Correctly Added
The import statement for onEnd1CTSession aligns with its usage in the code.
56-56: Import useAmplitudeAnalytics for Event Logging
The addition of useAmplitudeAnalytics is appropriate for analytics integration.
117-117: Extract logEvent from Analytics Hook
Initializing logEvent from useAmplitudeAnalytics is correctly implemented.
565-567: Pass logEvent to onAdd1CTSession
Passing logEvent ensures that session creation events are logged.
568-578: Pass logEvent to onEnd1CTSession
Including logEvent in onEnd1CTSession parameters ensures proper event logging when ending a 1CT session.
| }); | ||
| }); | ||
|
|
||
| onEnd1CTSession({ accountStore, authenticatorId, logEvent }); |
There was a problem hiding this comment.
Add error handling for session cleanup.
The onEnd1CTSession call should be wrapped in error handling to prevent silent failures and ensure consistent state.
Consider this improvement:
- onEnd1CTSession({ accountStore, authenticatorId, logEvent });
+ try {
+ await onEnd1CTSession({ accountStore, authenticatorId, logEvent });
+ } catch (error) {
+ console.error('Failed to cleanup 1CT session:', error);
+ // Optionally show error toast to user
+ displayToast(
+ {
+ titleTranslationKey: "oneClickTrading.toast.cleanupError",
+ captionTranslationKey: "oneClickTrading.toast.tryAgain",
+ },
+ ToastType.ERROR
+ );
+ }Committable suggestion skipped: line range outside the PR's diff.
JoseRFelix
changed the title
* feat: split txs for Ledger * fix: wrong override * feat: default to non ledger behavior * improvement: naming * improvement: typos
Testing and Verifying