(1CT) Set default session period to 7 days and update time periods #4000
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
|
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/web/hooks/one-click-trading/__tests__/use-one-click-trading-params.spec.tsOops! Something went wrong! :( ESLint: 8.50.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/packages/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces several modifications across various files related to one-click trading functionality. Key changes include updates to session period definitions and logic adjustments in several hooks and components to accommodate new session durations. A new utility function, Changes
Possibly related PRs
Suggested reviewers
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: 2
🧹 Outside diff range and nitpick comments (7)
packages/utils/src/date.ts (1)
23-56: LGTM! Consider minor improvements for robustness.The implementation is well-structured and handles the JavaScript setTimeout limitation correctly. A few suggestions to make it even more robust:
Consider these improvements:
- Add input validation for negative milliseconds
- Consider using
window.setTimeoutexplicitly for browser environments- Add a type for the return object
-export function safeTimeout(callback: () => void, milliseconds: number) { +export function safeTimeout(callback: () => void, milliseconds: number): { clear: () => void } { + if (milliseconds < 0) { + throw new Error('Timeout duration must be non-negative'); + } const MAX_TIMEOUT = 2 ** 31 - 1; - let timeoutId: NodeJS.Timeout | null = null; + let timeoutId: ReturnType<typeof setTimeout> | null = null; let cleared = false;packages/utils/src/__tests__/date.spec.ts (1)
1-114: Well-structured and comprehensive test suite!The test suite thoroughly covers various scenarios including direct timeouts, large timeout splitting, maximum timeout handling, premature callbacks, and timeout clearing. The tests are well-isolated with proper cleanup.
Consider adding a test case for negative timeout values to ensure proper error handling:
test("handles negative timeout values", () => { const callback = jest.fn(); const negativeTimeout = -1000; expect(() => safeTimeout(callback, negativeTimeout)).toThrow(); expect(callback).not.toHaveBeenCalled(); });packages/web/hooks/one-click-trading/use-one-click-trading-params.ts (2)
24-30: Document the session period migration strategyThe
OldHumanizedSessionPeriodsconstant defines deprecated session periods, but the migration strategy isn't documented.Add documentation explaining the migration strategy:
+/** + * List of deprecated session periods that are being migrated to "7days". + * @deprecated These periods are no longer supported and will be mapped to "7days" + */ const OldHumanizedSessionPeriods = [ "5min", "10min", "30min", "3hours", "12hours", ] as const;
39-43: Consider extracting the session period migration logicThe migration logic is embedded in the parameters function, making it harder to maintain and test independently.
Extract the migration logic into a separate function:
function migrateSessionPeriod(period: string): string { return OldHumanizedSessionPeriods.includes(period as any) ? "7days" : period; }Then use it in the parameters function:
- end: OldHumanizedSessionPeriods.includes( - oneClickTradingInfo.humanizedSessionPeriod as (typeof OldHumanizedSessionPeriods)[number] - ) - ? "7days" - : oneClickTradingInfo.humanizedSessionPeriod, + end: migrateSessionPeriod(oneClickTradingInfo.humanizedSessionPeriod),packages/web/hooks/one-click-trading/use-one-click-trading-swap-review.ts (2)
47-48: Use a more specific local storage keyThe current key "previous-one-click-enabled" could be more specific to avoid potential conflicts.
- useLocalStorage("previous-one-click-enabled", true); + useLocalStorage("osmosis-1ct-previous-enabled-state", true);
167-170: Improve readability of shouldSend1CTTx logicThe current implementation could be more explicit about its conditions.
- if (isOneClickTradingEnabled && (changes ?? []).length === 0) return false; - if (transaction1CTParams?.isOneClickEnabled) return true; - return false; + // Don't send if 1CT is enabled but no changes were made + if (isOneClickTradingEnabled && !changes?.length) { + return false; + } + + // Send if user wants to enable 1CT + return Boolean(transaction1CTParams?.isOneClickEnabled);packages/web/modals/review-order.tsx (1)
931-936: Consider simplifying the nested conditionsWhile the implementation is functionally correct, the nested conditions could be simplified for better readability.
Consider this alternative structure:
- {changes.includes("sessionPeriod") && - transactionParams?.sessionPeriod.end ? ( + {changes.includes("sessionPeriod") && transactionParams?.sessionPeriod.end ? ( <span className="text-bullish-400"> - {t( - oneClickTradingTimeMappings[transactionParams.sessionPeriod.end] - )} + {t(oneClickTradingTimeMappings[transactionParams.sessionPeriod.end])} </span> ) : ( <OneClickTradingRemainingTime className="inline" useShortTimeUnits /> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (17)
packages/web/localizations/de.jsonis excluded by!**/*.jsonpackages/web/localizations/en.jsonis excluded by!**/*.jsonpackages/web/localizations/es.jsonis excluded by!**/*.jsonpackages/web/localizations/fa.jsonis excluded by!**/*.jsonpackages/web/localizations/fr.jsonis excluded by!**/*.jsonpackages/web/localizations/gu.jsonis excluded by!**/*.jsonpackages/web/localizations/hi.jsonis excluded by!**/*.jsonpackages/web/localizations/ja.jsonis excluded by!**/*.jsonpackages/web/localizations/ko.jsonis excluded by!**/*.jsonpackages/web/localizations/pl.jsonis excluded by!**/*.jsonpackages/web/localizations/pt-br.jsonis excluded by!**/*.jsonpackages/web/localizations/ro.jsonis excluded by!**/*.jsonpackages/web/localizations/ru.jsonis excluded by!**/*.jsonpackages/web/localizations/tr.jsonis excluded by!**/*.jsonpackages/web/localizations/zh-cn.jsonis excluded by!**/*.jsonpackages/web/localizations/zh-hk.jsonis excluded by!**/*.jsonpackages/web/localizations/zh-tw.jsonis excluded by!**/*.json
📒 Files selected for processing (13)
packages/trpc/src/one-click-trading.ts(1 hunks)packages/types/src/one-click-trading-types.ts(1 hunks)packages/utils/src/__tests__/date.spec.ts(1 hunks)packages/utils/src/date.ts(1 hunks)packages/web/components/one-click-trading/screens/session-period-screen.tsx(1 hunks)packages/web/hooks/mutations/one-click-trading/use-create-one-click-trading-session.tsx(1 hunks)packages/web/hooks/one-click-trading/__tests__/use-one-click-trading-params.spec.ts(5 hunks)packages/web/hooks/one-click-trading/use-one-click-trading-params.ts(1 hunks)packages/web/hooks/one-click-trading/use-one-click-trading-session.ts(2 hunks)packages/web/hooks/one-click-trading/use-one-click-trading-swap-review.ts(6 hunks)packages/web/modals/profile.tsx(1 hunks)packages/web/modals/review-order.tsx(4 hunks)packages/web/utils/date.ts(2 hunks)
🧰 Additional context used
📓 Learnings (4)
packages/types/src/one-click-trading-types.ts (1)
Learnt from: greg-nagy
PR: osmosis-labs/osmosis-frontend#3954
File: packages/web/components/one-click-trading/screens/session-period-screen.tsx:17-22
Timestamp: 2024-11-15T09:22:45.377Z
Learning: Translation keys for 'oneClickTrading.sessionPeriods.*' are present in 'packages/web/localizations/en.json'.
packages/web/components/one-click-trading/screens/session-period-screen.tsx (1)
Learnt from: greg-nagy
PR: osmosis-labs/osmosis-frontend#3954
File: packages/web/components/one-click-trading/screens/session-period-screen.tsx:17-22
Timestamp: 2024-11-15T09:22:45.377Z
Learning: Translation keys for 'oneClickTrading.sessionPeriods.*' are present in 'packages/web/localizations/en.json'.
packages/web/modals/review-order.tsx (2)
Learnt from: greg-nagy
PR: osmosis-labs/osmosis-frontend#3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:205-208
Timestamp: 2024-11-15T12:43:19.973Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, within the `setTransaction1CTParams` function, it's acceptable to return the previous parameters if `initialTransaction1CTParams` or `nextParams` are undefined, as this situation is temporary and reusing the previous state is valid.
Learnt from: greg-nagy
PR: osmosis-labs/osmosis-frontend#3954
File: packages/web/modals/review-order.tsx:741-747
Timestamp: 2024-11-15T13:02:05.236Z
Learning: In `packages/web/modals/review-order.tsx`, when toggling one-click trading, if `transaction1CTParams` is undefined due to not retrieving default parameters from `api.local.oneClickTrading.getParameters`, we should keep `transaction1CTParams` undefined rather than providing a default state.
packages/web/hooks/one-click-trading/use-one-click-trading-swap-review.ts (1)
Learnt from: greg-nagy
PR: osmosis-labs/osmosis-frontend#3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:205-208
Timestamp: 2024-11-15T12:43:19.973Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, within the `setTransaction1CTParams` function, it's acceptable to return the previous parameters if `initialTransaction1CTParams` or `nextParams` are undefined, as this situation is temporary and reusing the previous state is valid.
🔇 Additional comments (11)
packages/trpc/src/one-click-trading.ts (1)
36-36: Verify security implications of extended session duration.
While the change to "7days" aligns with the PR objectives, the significant increase in session duration (from 1 hour to 7 days) increases the potential exposure window for the one-click trading feature.
Please document the security considerations for this change. Consider:
- Impact on user security
- Mitigation strategies (e.g., session revocation)
- User notifications about longer sessions
✅ Verification successful
Based on the code analysis, I can now generate the final response:
Extended session duration has built-in security controls
The 7-day session duration is part of a comprehensive security system that includes:
- Session authentication via
getSessionAuthenticator - Spend limits and loss protection controls
- Session validation and revocation mechanisms
- Network fee limits
The change aligns with the existing security architecture while providing better UX through longer sessions. The security implications are adequately handled by:
- Required wallet approval to initiate sessions
- Spend limit protections that remain active throughout the session
- Ability to revoke sessions via authenticator mechanisms
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing security documentation
rg -i "security|session duration|exposure" docs/
Length of output: 96
Script:
#!/bin/bash
# Let's check for security-related documentation in common documentation locations
fd -t f -e md -e mdx | xargs rg -i "security|session duration|exposure|one.?click.?trading"
# Also check for any configuration or constants related to session duration
ast-grep --pattern 'end: "$_" as const'
# Look for any security-related code or comments around session handling
rg -i "session|duration|expir" -A 3 -B 3
Length of output: 81875
Script:
#!/bin/bash
# Let's check for any security-related code or comments around the one-click trading implementation
rg -i "security|auth|session" packages/trpc/src/one-click-trading.ts -A 3 -B 3
# Also check for any session revocation or security mechanisms
ast-grep --pattern 'getSessionAuthenticator($$$)'
# Look for any tests related to session security
rg -i "test.*session|session.*security" -g "*.test.*" -g "*.spec.*"
Length of output: 5983
packages/web/utils/date.ts (1)
Line range hint 76-99: LGTM! Enhanced time display granularity
The addition of hours calculation for periods less than 30 days improves the precision of time display, which aligns well with the new session period options (1 hour, 1 day, 7 days, 30 days) introduced in this PR.
packages/web/components/one-click-trading/screens/session-period-screen.tsx (2)
13-18: LGTM! Well-structured translation mappings
Good practice to centralize the translation key mappings in a constant. The keys align with the learnings that indicate translations exist in packages/web/localizations/en.json.
29-32: LGTM! Updated session periods align with requirements
The session period options (1 hour, 1 day, 7 days, 30 days) match the PR objectives and provide a better range of durations for users.
packages/web/hooks/one-click-trading/use-one-click-trading-session.ts (2)
73-80: LGTM! Improved timeout handling with safeTimeout
Good improvements:
- Using
safeTimeoutensures proper handling of long durations (7 days, 30 days) - Proper cleanup with
clear()method - Clear variable naming with
timeRemainingSeconds
71-71: Verify handling of negative time remaining
The calculation sessionEndDate.unix() - dayjs().unix() could potentially return a negative value if the session has already ended.
Let's verify the behavior:
✅ Verification successful
Let me analyze the code further to understand the complete flow.
Let me search for the safeTimeout implementation to understand how it handles negative durations.
Let me get the complete implementation of safeTimeout to understand how it handles the duration.
Based on the complete context, I can now generate the final response:
Negative time remaining is properly handled by the implementation
The code is safe because:
- The
timeRemainingSecondscalculation is only used when!isExpired && value?.infois true - The
safeTimeoutimplementation handles the duration safely:- For negative durations, the timeout will execute immediately (browser behavior of setTimeout)
- The callback properly sets
isExpiredstate and triggersonExpire
- The expiration state is also checked server-side via
accountStore.isOneClickTradingExpired()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for handling of negative timeRemaining values
rg -A 5 "timeRemaining|sessionEndDate.unix()" --type ts
Length of output: 4085
Script:
#!/bin/bash
# Search for isExpired and setIsExpired usage
rg -A 5 "isExpired|setIsExpired" --type ts
Length of output: 12845
Script:
#!/bin/bash
# Search for safeTimeout implementation
ast-grep --pattern 'const safeTimeout = $_'
Length of output: 45
Script:
#!/bin/bash
# Search for any validation or checks before using timeRemainingSeconds
rg -B 3 -A 3 "timeRemainingSeconds" --type ts
Length of output: 1471
Script:
#!/bin/bash
# Search for safeTimeout implementation
rg -A 10 "export const safeTimeout|function safeTimeout" --type ts
Length of output: 794
Script:
#!/bin/bash
# Look for files that might contain the safeTimeout implementation
fd safeTimeout
Length of output: 16
Script:
#!/bin/bash
# Get the complete implementation of safeTimeout
rg -A 20 "export function safeTimeout" --type ts
Length of output: 1400
packages/web/modals/review-order.tsx (2)
133-133: LGTM: Clean addition of state setter
The addition of setPreviousIsOneClickEnabled follows React conventions and integrates well with the existing hook destructuring.
754-755: LGTM: Proper state management implementation
The implementation correctly manages state by updating the previous state before modifying the current state. The added empty line improves readability.
packages/web/hooks/mutations/one-click-trading/use-create-one-click-trading-session.tsx (1)
298-318: LGTM! Session period handling is well-implemented.
The switch statement correctly handles all session periods using appropriate dayjs operations and proper conversion to nanoseconds.
packages/web/hooks/one-click-trading/__tests__/use-one-click-trading-params.spec.ts (2)
150-150: LGTM! Test cases updated to reflect new session periods.
The test cases have been properly updated to use the new session period values ("30days" and "7days"), maintaining test coverage for the updated functionality.
Also applies to: 198-198, 236-236
263-420: LGTM! Comprehensive test coverage for getParametersFromOneClickTradingInfo.
The new test suite thoroughly covers:
- Default isOneClickEnabled behavior
- Network fee limit handling with type safety
- Session period mapping for both old and new periods
- Spend limit calculations
JoseRFelix
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/web/hooks/one-click-trading/__tests__/use-one-click-trading-params.spec.ts (1)
390-419: Consider adding edge cases to spend limit calculation testsWhile the current test case covers the basic spend limit calculation scenario, consider adding tests for:
- Zero amount
- Maximum allowed amount
- Invalid amounts (negative values)
Example test cases to add:
it("should handle edge cases in spend limit calculations", () => { const testCases = [ { amount: "0", decimals: 2, expected: "0" }, { amount: "999999999999", decimals: 2, expected: "9999999999.99" }, ]; testCases.forEach(({ amount, decimals, expected }) => { const params = getParametersFromOneClickTradingInfo({ oneClickTradingInfo: { // ... other required fields spendLimit: { amount, decimals }, }, defaultIsOneClickEnabled: true, }); expect(params.spendLimit.toDec().toString()).toBe(expected); }); }); it("should throw error for negative spend limit", () => { expect(() => getParametersFromOneClickTradingInfo({ oneClickTradingInfo: { // ... other required fields spendLimit: { amount: "-1000", decimals: 2 }, }, defaultIsOneClickEnabled: true, }) ).toThrow(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/web/hooks/one-click-trading/__tests__/use-one-click-trading-params.spec.ts(6 hunks)
🔇 Additional comments (3)
packages/web/hooks/one-click-trading/__tests__/use-one-click-trading-params.spec.ts (3)
7-7: LGTM: Import addition aligns with new test requirements
The addition of DecUtils is appropriate for the new spend limit calculation tests.
150-150: LGTM: Session period updates align with new requirements
The test cases have been appropriately updated to use the new session period values ("30days", "7days") as per the PR objectives.
Also applies to: 198-198, 222-222, 236-236
263-389: LGTM: Well-structured test suite with comprehensive coverage
The new test suite for getParametersFromOneClickTradingInfo is well-organized and covers essential scenarios:
- Default one-click trading enablement
- Network fee limit handling
- Session period mapping for both old and new values
What is the purpose of the change:
Linear Task
https://linear.app/osmosis/issue/FE-1277/1ct-default-values-improvements
Testing and Verifying