(1CT) Create single tx to activate 1-Click Trading from swap review modal #3970
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
WalkthroughThis pull request involves significant updates across multiple components and hooks related to one-click trading functionality. Key changes include the refactoring of import statements, the introduction of new hooks and state management for one-click trading sessions, and enhancements to error handling. The Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@JoseRFelix has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: 7
🧹 Outside diff range and nitpick comments (15)
packages/web/modals/wallet-select/use-connect-wallet.ts (1)
Line range hint
147-157: Simplify error handling logic.While the error handling is functionally correct, it could be more concise. The nested error handling with type checking can be simplified.
Consider this more streamlined approach:
- await onCreate1CTSession(); - } catch (e) { - const error = e as CreateOneClickSessionError | Error; - - if (error instanceof Error) { - throw new CreateOneClickSessionError(error.message); - } - - throw e; + await onCreate1CTSession(); + } catch (error) { + if (!(error instanceof CreateOneClickSessionError)) { + throw new CreateOneClickSessionError( + error instanceof Error ? error.message : 'Unknown error' + ); + } + throw error;This change:
- Eliminates unnecessary type assertion
- Simplifies the error handling flow
- Maintains the same error propagation behavior
packages/web/modals/wallet-select/index.tsx (2)
Line range hint
146-156: Consider adding explicit error handlingWhile the function handles the happy path well, consider adding explicit error handling to gracefully handle failures during session creation.
const onCreate1CTSession = async ({ transaction1CTParams, }: { transaction1CTParams: OneClickTradingTransactionParams | undefined; }) => { + try { create1CTSession.reset(); setIsInitializingOneClickTrading(true); return create1CTSession.mutate({ transaction1CTParams, spendLimitTokenDecimals: spendLimitTokenDecimals, }); + } catch (error) { + setIsInitializingOneClickTrading(false); + // Re-throw to let error boundary handle it + throw error; + } };
Line range hint
1-285: Architecture successfully simplified for single-transaction flowThe removal of direct wallet repository dependency and consolidation of session creation logic successfully supports the PR's objective of enabling single-transaction 1CT activation. The component maintains proper separation of concerns while reducing complexity.
Consider documenting this architectural change in the component's comments to explain the single-transaction design decision.
packages/web/components/swap-tool/index.tsx (2)
Line range hint
590-601: Consider extracting complex loading state logicThe loading state condition for 1-Click trading is correctly implemented, but the nested conditions could be extracted into a separate function for better maintainability.
Consider refactoring like this:
+const getIsOneClickLoadingState = ( + isOneClickTradingEnabled: boolean, + isLoadingNetworkFee: boolean, + isInputEmpty: boolean +) => + isOneClickTradingEnabled && + isLoadingNetworkFee && + !isInputEmpty; <Button disabled={/* ... */} - isLoading={ - isOneClickTradingEnabled && - swapState.isLoadingNetworkFee && - !swapState.inAmountInput.isEmpty - } + isLoading={getIsOneClickLoadingState( + isOneClickTradingEnabled, + swapState.isLoadingNetworkFee, + swapState.inAmountInput.isEmpty + )} loadingText={buttonText} /* ... */ >
Line range hint
1-724: Consider breaking down the SwapTool componentThe SwapTool component has grown quite large and handles multiple responsibilities including:
- Asset input/output fields
- Token selection
- Swap execution
- Modal management
- Loading states
This complexity makes it harder to maintain and test, especially with the addition of 1-Click trading features.
Consider breaking it down into smaller, more focused components:
SwapInputFields- Handle the input/output fieldsSwapActions- Handle the swap button and related actionsSwapModals- Manage the various modalsSwapState- Custom hook for state managementThis would improve:
- Code maintainability
- Testing isolation
- Feature addition (like 1-Click trading)
- State management
packages/web/hooks/mutations/one-click-trading/use-remove-one-click-trading-session.ts (1)
54-54: Provide detailed error information upon transaction failureThe error message
"Transaction failed"is generic. Including thetx.rawLogprovides more context for debugging.Updated error handling:
if (tx.code !== 0) { - throw new Error("Transaction failed"); + throw new Error(`Transaction failed: ${tx.rawLog}`); }packages/web/hooks/one-click-trading/use-one-click-trading-swap-review.ts (2)
83-112: Consolidate multipleuseEffecthooks for cleaner state managementCurrently, there are four separate
useEffecthooks updating the store whenisModalOpenchanges. Consolidating them into a singleuseEffectenhances readability and maintainability.Consider this refactor:
-useEffect(() => { - if (isModalOpen) { - use1CTSwapReviewStore - .getState() - .setTransaction1CTParams(transactionParams); - } -}, [transactionParams, isModalOpen]); -useEffect(() => { - if (isModalOpen) { - use1CTSwapReviewStore - .getState() - .setSpendLimitTokenDecimals(spendLimitTokenDecimals); - } -}, [isModalOpen, spendLimitTokenDecimals]); -useEffect(() => { - if (isModalOpen) { - use1CTSwapReviewStore - .getState() - .setInitialTransactionParams(initialTransactionParams); - } -}, [isModalOpen, initialTransactionParams]); -useEffect(() => { - if (isModalOpen) { - use1CTSwapReviewStore.getState().setChanges(changes); - } -}, [isModalOpen, changes]); +useEffect(() => { + if (isModalOpen) { + const state = use1CTSwapReviewStore.getState(); + state.setTransaction1CTParams(transactionParams); + state.setSpendLimitTokenDecimals(spendLimitTokenDecimals); + state.setInitialTransactionParams(initialTransactionParams); + state.setChanges(changes); + } +}, [ + isModalOpen, + transactionParams, + spendLimitTokenDecimals, + initialTransactionParams, + changes, +]);
321-325: Handle undefinedpriceinformatSpendLimitto prevent inaccurate outputsWhen
priceisundefined, theformatSpendLimitfunction may return"undefined"as part of the output. To avoid misleading outputs, consider returning an empty string or a default value whenpriceis not provided.Apply this adjustment:
export function formatSpendLimit(price: PricePretty | undefined) { - return `${price?.symbol}${trimPlaceholderZeros( - price?.toDec().toString(2) ?? "" - )}`; + if (!price) return ""; + return `${price.symbol}${trimPlaceholderZeros( + price.toDec().toString(2) + )}`; }packages/web/modals/review-order.tsx (7)
Line range hint
174-198: Inconsistent comment and dependency array inuseMemoThe comment indicates that dependencies are disabled for this hook, but the dependency array includes
amountWithSlippageandslippageConfig. Please verify if dependencies should be included or if the comment needs updating to reflect the current implementation.
Line range hint
169-172: Avoid disabling ESLint rules without necessityDisabling the
react-hooks/exhaustive-depsrule may hide potential issues with hook dependencies. IfinitialOutputdepends on external variables, consider specifying the dependencies explicitly to ensure the hook behaves as expected.
769-774: SimplifyconfirmActioncall by omitting undefined propertiesSince both
create1CTSessionMsgandremove1CTSessionMsgare optional and set toundefined, you can simplify the function call by passing an empty object:-onClick={() => { - confirmAction({ - create1CTSessionMsg: undefined, - remove1CTSessionMsg: undefined, - }); -}} +onClick={() => { + confirmAction({}); +}}
Line range hint
174-198: Potential misuse ofuseMemowith mutable variableInside the
useMemo,originalValueis reassigned in therestartfunction:restart: () => { originalValue = amountWithSlippage ?? new IntPretty(0); },Mutating a variable within
useMemocan lead to unexpected behaviors. Consider usinguseRefif you need a mutable value that persists across renders.
Line range hint
860-872: Safe access oftransactionParamspropertiesIn
OneClickTradingActiveSessionParamsEdit, accessing properties oftransactionParamswithout checking if they exist could lead to runtime errors.{transactionParams?.spendLimit.toString()}If
transactionParamsortransactionParams.spendLimitisundefined, calling.toString()will cause an error. Use optional chaining and provide a fallback:{transactionParams?.spendLimit?.toString() ?? 'Default Spend Limit'}
Line range hint
870-880: Ensure safe access tosessionPeriod.endpropertySimilarly, when accessing
transactionParams?.sessionPeriod.end, ensure thatsessionPeriodis defined:{transactionParams?.sessionPeriod?.end ?? 'Default Session Period'}This prevents errors if
sessionPeriodisundefined.
Line range hint
169-172: Consider specifying dependencies foruseMemoInstead of disabling ESLint, explicitly include necessary dependencies for
useMemoto ensure thatinitialOutputupdates correctly when dependencies change. IfinitialOutputshould remain constant, consider usinguseRefinstead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
packages/web/components/one-click-trading/one-click-trading-settings.tsx(1 hunks)packages/web/components/one-click-trading/profile-one-click-trading-settings.tsx(0 hunks)packages/web/components/place-limit-tool/index.tsx(1 hunks)packages/web/components/swap-tool/index.tsx(1 hunks)packages/web/hooks/limit-orders/use-place-limit.ts(8 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/index.ts(1 hunks)packages/web/hooks/one-click-trading/use-one-click-trading-session-manager.ts(0 hunks)packages/web/hooks/one-click-trading/use-one-click-trading-swap-review.ts(1 hunks)packages/web/hooks/use-swap.tsx(9 hunks)packages/web/modals/profile.tsx(0 hunks)packages/web/modals/review-order.tsx(5 hunks)packages/web/modals/wallet-select/index.tsx(2 hunks)packages/web/modals/wallet-select/use-connect-wallet.ts(2 hunks)
💤 Files with no reviewable changes (3)
- packages/web/components/one-click-trading/profile-one-click-trading-settings.tsx
- packages/web/hooks/one-click-trading/use-one-click-trading-session-manager.ts
- packages/web/modals/profile.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/web/hooks/one-click-trading/index.ts
🧰 Additional context used
🪛 Gitleaks (8.21.2)
packages/web/hooks/mutations/one-click-trading/use-create-one-click-trading-session.tsx
187-187: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (28)
packages/web/modals/wallet-select/index.tsx (3)
1-1: LGTM: Import cleanup aligns with architectural changes
The removal of WalletRepo from imports reflects the architectural change to simplify one-click trading activation, reducing unnecessary coupling.
Line range hint 157-285: LGTM: Render logic handles all states appropriately
The component's render logic properly manages all states including one-click trading initialization, errors, and loading states. The cleanup on modal close is handled correctly.
96-96: Verify error handling after walletRepo removal
The simplified callback signature aligns with the PR objective. However, we should verify that error handling remains robust without the wallet repository context.
✅ Verification successful
Let me also search for the usage of onCreate1CTSession to understand the error handling context:
Based on the search results, I can now generate the final response. The error handling for the onCreate1CTSession callback remains robust even without the walletRepo parameter:
Error handling is properly maintained after walletRepo removal
The error handling for one-click trading session creation is well-implemented in the codebase:
- Errors are caught and properly typed via
CreateOneClickSessionError - Parameter validation ensures
transaction1CTParamsis defined before proceeding - Error states are properly propagated to the UI through modal views like
initializeOneClickTradingErrorandinitializeOneClickTradingErrorInsufficientFee
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in wallet-related code
rg -A 5 "catch|error|Error" --type typescript packages/web/
Length of output: 96
Script:
#!/bin/bash
# Search for error handling patterns in wallet-related code, using ripgrep without file type filter
rg -A 5 "catch|error|Error" packages/web/modals/wallet-select/
Length of output: 38432
Script:
#!/bin/bash
# Search for onCreate1CTSession implementation and usage
ast-grep --pattern 'onCreate1CTSession = async ($$$) => { $$$ }'
Length of output: 66
Script:
#!/bin/bash
# Search for any error handling in the transaction context
rg -A 5 "transaction1CTParams" packages/web/
Length of output: 57177
packages/web/components/one-click-trading/one-click-trading-settings.tsx (2)
Line range hint 1-603: Implementation LGTM!
The component implementation is robust with:
- Proper TypeScript types and interfaces
- Comprehensive error handling and loading states
- Clear user feedback for actions
- Clean separation of concerns
35-35: Verify the new import path
The import path change aligns with the PR objective of consolidating 1CT activation into a single transaction. However, we should verify that the new module maintains the same function signature to prevent runtime errors.
✅ Verification successful
Let me verify the old module to ensure the function signature matches.
Import path change is valid
The formatSpendLimit function has been successfully moved to the new module use-one-click-trading-swap-review.ts with a consistent signature that accepts a PricePretty parameter. The old module use-one-click-trading-session-manager.ts no longer exists, indicating this is part of the intended consolidation of one-click trading functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new module exists and maintains the same function signature
# Check if the new module exists
fd "use-one-click-trading-swap-review.ts"
# Check the function signature in both old and new files
rg -A 5 "export.*formatSpendLimit"
Length of output: 685
Script:
#!/bin/bash
# Check the old module's existence and function signature
fd "use-one-click-trading-session-manager.ts"
rg -A 5 "formatSpendLimit" packages/web/hooks/one-click-trading/use-one-click-trading-session-manager.ts
Length of output: 268
packages/web/components/swap-tool/index.tsx (1)
296-297: LGTM: Loading state properly handles 1-Click Trading messages
The addition of isLoadingOneClickMessages to the loading condition ensures proper UI feedback during 1-Click Trading message preparation.
packages/web/components/place-limit-tool/index.tsx (1)
879-881: LGTM! Verify loading state behavior.
The addition of swapState.isLoadingOneClickMessages to disable the confirmation button while loading one-click messages is a good safeguard. This prevents potential race conditions and aligns with the PR's objective of consolidating 1CT activation into a single transaction.
✅ Verification successful
Loading state implementation verified and correctly used
The isLoadingOneClickMessages state is properly implemented in the useOneClickTradingSwapReview hook, where it tracks the loading state of async operations for creating or removing 1CT sessions. The state is appropriately used in the place-limit-tool component to disable the confirmation button during these operations, preventing race conditions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of isLoadingOneClickMessages across other components
ast-grep --pattern 'isLoadingOneClickMessages'
# Check for similar loading state patterns in review modals
ast-grep --pattern 'ReviewOrder.*isConfirmationDisabled'
Length of output: 372
Script:
#!/bin/bash
# Let's check the implementation of ReviewOrder component to understand the isConfirmationDisabled prop usage
ast-grep --pattern 'ReviewOrder = ({$$$}) => {$$$}'
# Let's also check where else the hook is being used
rg -l "useOneClickTradingSwapReview"
# And check the actual implementation of the loading state in the hook
ast-grep --pattern 'const { value: oneClickMessages, loading: isLoadingOneClickMessages }'
Length of output: 5028
packages/web/hooks/mutations/one-click-trading/use-remove-one-click-trading-session.ts (1)
44-44: Confirm the message type in signAndBroadcast
The message type is specified as "addOrRemoveAuthenticators". Ensure that this string matches the expected message type for removing an authenticator, especially if the message was changed to makeRemoveAuthenticatorMsg.
packages/web/hooks/limit-orders/use-place-limit.ts (6)
17-20: Imports added for one-click trading and translation functionality
The added imports for useTranslation, onAdd1CTSession, and use1CTSwapReviewMessages are necessary for implementing one-click trading features and localization.
73-74: Initialize apiUtils and t for API utilities and translations
Initializing apiUtils and t ensures that API utilities and translation functions are available within the usePlaceLimit hook.
287-289: Retrieve one-click trading messages using use1CTSwapReviewMessages hook
The use of the use1CTSwapReviewMessages hook to retrieve one-click trading messages is appropriate and integrates well with the existing logic.
290-294: Combine encodedMsg with one-click messages in limitMessages
The logic to include both the encodedMsg and any available one-click trading messages into limitMessages is correct and ensures all relevant messages are included in the transaction.
628-628: Pass limitMessages to useEstimateTxFees correctly
Providing limitMessages to useEstimateTxFees is appropriate for accurate estimation of transaction fees based on the messages to be sent.
685-685: Include isLoadingOneClickMessages in returned state
Adding isLoadingOneClickMessages to the returned state allows components to handle loading states related to one-click trading messages effectively.
packages/web/modals/review-order.tsx (4)
10-14: Imports updated to include necessary types
The import statements correctly include makeAddAuthenticatorMsg, makeRemoveAuthenticatorMsg, and QuoteDirection types from @osmosis-labs/tx. This ensures that the types are available for use in the component.
56-63: Updated confirmAction prop to accept session messages
The confirmAction prop in the ReviewOrderProps interface now accepts an object with optional create1CTSessionMsg and remove1CTSessionMsg. This change aligns with the new functionality and allows for more flexible session management.
143-143: Using useOneClickTradingSwapReview hook appropriately
The useOneClickTradingSwapReview hook is correctly initialized with { isModalOpen: isOpen }, which ensures that the one-click trading session is managed based on the modal's open state.
775-775: Ensure wouldExceedSpendLimit is defined before use
When setting the disabled prop:
disabled={isConfirmationDisabled || wouldExceedSpendLimit}Ensure that wouldExceedSpendLimit is always defined as a boolean to prevent unintended behavior when it is undefined.
packages/web/hooks/use-swap.tsx (10)
31-31: Imports Update Approved
The addition of React hooks (useCallback, useEffect, useMemo, useState) is appropriate for the enhanced component logic.
49-53: One-Click Trading Hooks Imported Successfully
The imports of onAdd1CTSession, use1CTSwapReviewMessages, and useOneClickTradingSession correctly integrate the One-Click Trading functionality into the swap hook.
115-115: API Utilities Initialized
Initialization of apiUtils with api.useUtils() is correctly implemented for accessing API utility functions.
331-336: Messages Consolidation Confirmed
The combination of quote messages and One-Click Trading messages ensures all necessary transaction messages are included. The use of useMemo optimizes performance.
344-344: Transaction Fee Estimation Messages Updated
Including the consolidated messages array in the useEstimateTxFees hook enhances the accuracy of fee estimation.
442-445: Conditional Logic for One-Click Trading Verified
The computation of messageCanBeSignedWithOneClickTrading appropriately checks message availability and One-Click Trading eligibility.
519-570: Transaction Signing and Broadcasting Logic Reviewed
The updated signAndBroadcast method correctly handles transaction messages, fee options, and handles One-Click Trading session creation when necessary.
581-595: Dependencies Array Updated for Accurate Memoization
Including apiUtils.local.oneClickTrading.getSessionAuthenticator.fetch and other dependencies ensures that the sendTradeTokenInTx function updates correctly when these values change.
1044-1045: One-Click Messages Accessed in Amount Input Hook
Retrieving oneClickMessages within useSwapAmountInput aligns with the need to include these messages in fee estimation.
1056-1059: Messages Array Expanded for Fee Estimation
Merging quoteForCurrentBalance.messages with oneClickMessages.msgs ensures comprehensive fee estimation for current balance checks.
packages/web/hooks/mutations/one-click-trading/use-remove-one-click-trading-session.ts
Show resolved
Hide resolved
packages/web/hooks/mutations/one-click-trading/use-remove-one-click-trading-session.ts
Show resolved
Hide resolved
packages/web/hooks/one-click-trading/use-one-click-trading-swap-review.ts
Outdated
Show resolved
Hide resolved
packages/web/hooks/mutations/one-click-trading/use-create-one-click-trading-session.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/web/modals/review-order.tsx (1)
Line range hint
47-58: UpdateconfirmActionprop type to match the new function signatureThe
confirmActionprop in theReviewOrderPropsinterface is still defined as() => void, but according to the changes, it now expects parameters:{ create1CTSessionMsg: Awaited<ReturnType<typeof makeAddAuthenticatorMsg>> | undefined; remove1CTSessionMsg: Awaited<ReturnType<typeof makeRemoveAuthenticatorMsg>> | undefined; }This mismatch will lead to TypeScript errors. Please update the prop type to reflect the new function signature.
Apply this diff to fix the issue:
interface ReviewOrderProps { isOpen: boolean; onClose: () => void; - confirmAction: () => void; + confirmAction: (params: { + create1CTSessionMsg: Awaited<ReturnType<typeof makeAddAuthenticatorMsg>> | undefined; + remove1CTSessionMsg: Awaited<ReturnType<typeof makeRemoveAuthenticatorMsg>> | undefined; + }) => void; isConfirmationDisabled: boolean; slippageConfig?: ObservableSlippageConfig; // ... rest of the interface }packages/web/hooks/use-swap.tsx (1)
442-445: Refactor async ternary expression for better readabilityUsing
awaitinside a ternary operator can reduce code readability. Consider refactoring the code to avoid this pattern.Proposed code:
- const messageCanBeSignedWithOneClickTrading = !isNil(messages) - ? isOneClickTradingEnabled && - (await accountStore.shouldBeSignedWithOneClickTrading({ - messages, - })) - : false; + let messageCanBeSignedWithOneClickTrading = false; + if (!isNil(messages)) { + const canBeSignedWith1CT = await accountStore.shouldBeSignedWithOneClickTrading({ + messages, + }); + messageCanBeSignedWithOneClickTrading = isOneClickTradingEnabled && canBeSignedWith1CT; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/stores/src/account/osmosis/index.ts(0 hunks)packages/web/hooks/use-swap.tsx(9 hunks)packages/web/modals/review-order.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- packages/stores/src/account/osmosis/index.ts
🧰 Additional context used
📓 Learnings (1)
packages/web/modals/review-order.tsx (1)
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.
🔇 Additional comments (10)
packages/web/hooks/use-swap.tsx (10)
31-31: Import statements updated appropriately
The necessary React hooks have been imported correctly.
49-53: Added imports for one-click trading hooks
The imports for onAdd1CTSession, use1CTSwapReviewMessages, and useOneClickTradingSession are correctly added.
331-333: Usage of use1CTSwapReviewMessages hook is appropriate
The hook use1CTSwapReviewMessages is properly utilized to extract necessary variables.
334-336: Combining quote messages with one-click trading messages
The messages are correctly combined to include both quote messages and one-click trading messages.
344-347: Sign options reflect one-click trading state appropriately
The useEstimateTxFees hook is correctly provided with the useOneClickTrading flag based on isOneClickTradingEnabled.
519-574: Signing and broadcasting transaction with appropriate conditions
The signAndBroadcast function is correctly utilized, including the handling of one-click trading session creation when applicable.
585-599: Dependency array for useCallback is appropriate
All necessary dependencies are included in the dependency array of sendTradeTokenInTx.
731-731: Passing loading state of one-click trading messages
The isLoadingOneClickMessages state is correctly passed, ensuring proper handling of loading states.
1048-1049: Incorporation of one-click trading messages in fee estimation (useSwapAmountInput)
The oneClickMessages are correctly retrieved to include in fee estimation within useSwapAmountInput.
1060-1063: Combining messages for fee estimation in useSwapAmountInput
The messages array is correctly constructed to include both quoteForCurrentBalance messages and oneClickMessages, ensuring accurate fee estimation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/web/hooks/one-click-trading/use-one-click-trading-swap-review.ts (3)
83-111: Consider consolidating multiple useEffect hooksThese four separate useEffect hooks could be consolidated into a single effect to reduce the number of re-renders and improve performance.
- useEffect(() => { - if (isModalOpen) { - use1CTSwapReviewStore - .getState() - .setTransaction1CTParams(transactionParams); - } - }, [transactionParams, isModalOpen]); - - useEffect(() => { - if (isModalOpen) { - use1CTSwapReviewStore - .getState() - .setSpendLimitTokenDecimals(spendLimitTokenDecimals); - } - }, [isModalOpen, spendLimitTokenDecimals]); - - useEffect(() => { - if (isModalOpen) { - use1CTSwapReviewStore - .getState() - .setInitialTransactionParams(initialTransactionParams); - } - }, [isModalOpen, initialTransactionParams]); - - useEffect(() => { - if (isModalOpen) { - use1CTSwapReviewStore.getState().setChanges(changes); - } - }, [isModalOpen, changes]); + useEffect(() => { + if (isModalOpen) { + const state = use1CTSwapReviewStore.getState(); + state.setTransaction1CTParams(transactionParams); + state.setSpendLimitTokenDecimals(spendLimitTokenDecimals); + state.setInitialTransactionParams(initialTransactionParams); + state.setChanges(changes); + } + }, [isModalOpen, transactionParams, spendLimitTokenDecimals, initialTransactionParams, changes]);
175-176: Consider extracting magic numbers into named constantsThe cache and stale time values would be more maintainable if extracted into named constants.
+ const SESSION_CACHE_TIME = 15_000; // 15 seconds + const SESSION_STALE_TIME = 15_000; // 15 seconds { enabled: isOneClickTradingEnabled && shouldFetchExistingSessionAuthenticator, - cacheTime: 15_000, // 15 seconds - staleTime: 15_000, // 15 seconds + cacheTime: SESSION_CACHE_TIME, + staleTime: SESSION_STALE_TIME, retry: false, }
290-312: Consider simplifying the wouldExceedSpendLimit calculationThe function could be simplified by extracting the spend limit calculation logic and using early returns for better readability.
const wouldExceedSpendLimit = useCallback( ({ wantToSpend, maybeWouldSpendTotal, }: { wantToSpend: Dec; maybeWouldSpendTotal?: Dec; }) => { if (wantToSpend.isZero()) return false; + const getSpendLimit = () => transactionParams?.spendLimit?.toDec() ?? new Dec(0); + const getAmountSpent = () => amountSpentData?.amountSpent?.toDec() ?? new Dec(0); + - const spendLimit = transactionParams?.spendLimit?.toDec() ?? new Dec(0); - const amountSpent = amountSpentData?.amountSpent?.toDec() ?? new Dec(0); /** * If we have simulation results then we use the exact amount that would be spent * if not we provide a fallback by adding already spent amount and the next spending * (the fallback ignores the fact that for some tokens, the value is not included in the spend limit) */ - const wouldSpend = maybeWouldSpendTotal ?? amountSpent.add(wantToSpend); + const wouldSpend = maybeWouldSpendTotal ?? getAmountSpent().add(wantToSpend); - return wouldSpend.gt(spendLimit); + return wouldSpend.gt(getSpendLimit()); }, [amountSpentData, transactionParams] );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/web/hooks/limit-orders/use-place-limit.ts(8 hunks)packages/web/hooks/one-click-trading/use-one-click-trading-swap-review.ts(1 hunks)
🔇 Additional comments (7)
packages/web/hooks/one-click-trading/use-one-click-trading-swap-review.ts (2)
20-47: LGTM: Well-structured Zustand store implementation
The store implementation follows best practices with proper TypeScript types and clean state management.
321-325: LGTM: Clean utility function implementation
The function properly handles undefined values and follows good practices for string formatting.
packages/web/hooks/limit-orders/use-place-limit.ts (5)
17-20: Imports for One-Click Trading Functionality Added Correctly
The newly added imports for useTranslation, onAdd1CTSession, and use1CTSwapReviewMessages are correctly implemented to integrate one-click trading features.
287-295: Integration of One-Click Trading Messages
The use of use1CTSwapReviewMessages() and the construction of limitMessages ensure that one-click trading messages are properly incorporated when placing limit orders.
352-352: Proper Handling of Empty Limit Messages
The check if (!limitMessages || limitMessages.length === 0) return; correctly prevents proceeding when there are no messages to send, avoiding potential errors.
377-408: Secure Session Initialization After Transaction Success
The transaction callback handles success appropriately by checking shouldSend1CTTx and initializing the one-click trading session with onAdd1CTSession. Ensure that sensitive information like privateKey is managed securely within onAdd1CTSession to maintain security best practices.
685-685: Including isLoadingOneClickMessages in Return Object
Adding isLoadingOneClickMessages to the returned state facilitates proper loading state management in consuming components.
JoseRFelix
deleted the
jose/fe-1249-create-a-single-tx-to-activate-1ct-from-swap-review-modal
branch
What is the purpose of the change:
After merging #3954, a CTA to start a 1CT session is now displayed in the swap review modal. However, during activation, two separate transactions are being executed, resulting in higher fees for the user and displaying two success toasts. Additionally, there’s a hacky implementation in this section of the code to ensure the 1CT session is created.
This PR addresses the issue by combining the 1CT and swap/limit operations into a single transaction. This approach improves the user experience and simplifies the implementation.
Linear Task
https://linear.app/osmosis/issue/FE-1249/create-a-single-tx-to-activate-1ct-from-swap-review-modal
Testing and Verifying