feat: 1ct review modal #3900
feat: 1ct review modal #3900
Conversation
|
@Kyatros is attempting to deploy a commit to the OsmoLabs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces several new React components related to one-click trading functionality, including 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: 12
🧹 Outside diff range and nitpick comments (7)
packages/web/components/one-click-trading/one-click-remaining-amount.tsx (1)
8-14: LGTM: Expired session handling is correct. Consider adding aria-label for accessibility.The logic for handling an expired session is correct and the use of the translation function is good for internationalization.
Consider adding an
aria-labelto the paragraph for improved accessibility:- <p className="body1 text-osmoverse-300"> + <p className="body1 text-osmoverse-300" aria-label={t("oneClickTrading.profile.sessionExpired")}>packages/web/components/one-click-trading/one-click-trading-limit-remaining.tsx (1)
17-55: LGTM: Conditional rendering logic is clear and user-friendly.The component effectively handles both cases (limit exceeded and not exceeded) with appropriate UI elements. The early return pattern for the non-exceeded case improves code readability.
Consider extracting the rendered content for each case into separate functions to further improve readability, especially if the component grows in complexity. For example:
const renderNonExceededContent = () => ( <div className="body2 flex cursor-pointer items-center justify-end gap-1 text-wosmongton-300" onClick={onRequestEdit} > <OneClickTradingRemainingAmount /> <p>/</p> <OneClickTradingRemainingTime className="!body2 !text-wosmongton-300" /> </div> ); const renderExceededContent = () => ( <div className="body2 flex items-center justify-end gap-1 text-wosmongton-300"> {/* ... */} </div> ); return isSpendingLimitExceeded ? renderExceededContent() : renderNonExceededContent();This approach can make the main component body more concise and easier to understand at a glance.
packages/web/components/one-click-trading/one-click-trading-call-to-action.tsx (2)
27-32: Consider logging events for both enabling and disabling one-click trading.The
handleTogglefunction currently only logs an event when one-click trading is enabled. For consistency and better analytics, consider logging events for both enabling and disabling the feature.Here's a suggested improvement:
const handleToggle = () => { - if (isOneClickEnabled) { + if (!isOneClickEnabled) { logEvent([EventName.OneClickTrading.enableOneClickTrading]); + } else { + logEvent([EventName.OneClickTrading.disableOneClickTrading]); } setIsOneClickEnabled(!isOneClickEnabled); };Note: You'll need to add a new event name for disabling one-click trading in your
EventNameconfiguration.
34-57: LGTM: Well-structured render function with good accessibility.The main render function is well-implemented, using appropriate React patterns and Tailwind CSS for styling. The component is accessible, using semantic HTML and providing alt text for the image.
Minor suggestion: Consider extracting the text content into separate variables or constants at the top of the component for easier maintenance and potential internationalization.
packages/web/components/one-click-trading/profile-one-click-trading-settings.tsx (1)
58-58: AddedscopeKeyfor better context managementThe addition of the
scopeKeyparameter enhances the context management for one-click trading parameters. This aligns well with the PR objectives of improving the one-click trading feature.Consider adding a brief comment explaining the purpose of this
scopeKey, especially if it's used in other components or hooks for consistency checks.packages/web/hooks/one-click-trading/use-one-click-trading-params.ts (1)
67-67: Ensure uniqueness ofqueryKeyto prevent cache collisions.While using
[scopeKey, oneClickTradingInfo?.sessionKey]as thequeryKeyis effective, verify that thescopeKeyvalues are unique across different usages of the hook to avoid unintended cache sharing or collisions.packages/web/components/one-click-trading/one-click-trading-in-review-modal.tsx (1)
41-46: Consider standardizing naming conventions for consistency.In the destructured props, there is a mix of '1CT' and 'OneClickTrading' in the variable names (e.g.,
transaction1CTParams,isLoading1CTParams,resetTransaction1CTParams). Elsewhere in the code, the full termoneClickTradingis used (e.g.,useOneClickTradingSession). For clarity and maintainability, consider standardizing the naming convention by choosing either1CToroneClickTradingconsistently throughout the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/web/localizations/en.jsonis excluded by!**/*.jsonpackages/web/public/images/1ct-medium-icon.svgis excluded by!**/*.svg,!**/*.svg
📒 Files selected for processing (9)
- packages/web/components/one-click-trading/one-click-remaining-amount.tsx (1 hunks)
- packages/web/components/one-click-trading/one-click-trading-call-to-action.tsx (1 hunks)
- packages/web/components/one-click-trading/one-click-trading-in-review-modal.tsx (1 hunks)
- packages/web/components/one-click-trading/one-click-trading-limit-remaining.tsx (1 hunks)
- packages/web/components/one-click-trading/one-click-trading-settings.tsx (8 hunks)
- packages/web/components/one-click-trading/profile-one-click-trading-settings.tsx (1 hunks)
- packages/web/hooks/one-click-trading/use-one-click-trading-params.ts (2 hunks)
- packages/web/hooks/one-click-trading/use-one-click-trading-session.ts (5 hunks)
- packages/web/modals/review-order.tsx (7 hunks)
🧰 Additional context used
🔇 Additional comments (31)
packages/web/components/one-click-trading/one-click-remaining-amount.tsx (2)
1-1: LGTM: Imports are appropriate and consistent.The imports for
useOneClickTradingSessionanduseTranslationfrom "~/hooks" are correct and align with the component's requirements.
3-6: LGTM: Component declaration and hook usage are correct.The
OneClickTradingRemainingAmountcomponent is properly declared as a React functional component. The hooksuseTranslationanduseOneClickTradingSessionare correctly used at the top level, and the necessary functions and values are appropriately destructured.packages/web/components/one-click-trading/one-click-trading-limit-remaining.tsx (2)
1-9: LGTM: Imports and interface definition are well-structured.The imports are relevant to the component's functionality, and the use of a separate interface for props enhances type safety.
11-15: LGTM: Component definition and i18n setup are correct.The component is properly defined as a functional component with typed props, and the useTranslation hook is correctly implemented for internationalization.
packages/web/components/one-click-trading/one-click-trading-call-to-action.tsx (4)
1-15: LGTM: Imports and Props interface are well-defined.The imports cover all necessary dependencies, and the Props interface is comprehensive, including all required properties for the component's functionality.
17-25: LGTM: Component definition and hook usage are correct.The component is properly defined as a functional component with destructured props. The use of
useTranslationanduseAmplitudeAnalyticshooks is appropriate for handling internationalization and analytics, respectively.
1-78: Overall, the OneClickTradingCallToAction component is well-implemented and aligns with the PR objectives.The component successfully implements the one-click trading call-to-action feature as described in the PR objectives. It provides a clear and accessible interface for users to enable/disable one-click trading and view relevant details. The code is well-structured, uses appropriate React patterns, and follows good practices for accessibility and internationalization.
Minor improvements have been suggested for:
- Consistent event logging for both enabling and disabling one-click trading.
- Extracting text content for easier maintenance and internationalization.
- Adding error handling for potential undefined values.
These suggestions, if implemented, would further enhance the robustness and maintainability of the component.
58-75: LGTM: Good implementation of conditional rendering and internationalization.The conditional rendering of additional details when one-click trading is enabled is well-implemented. The use of translation functions for text content is good for internationalization, and the separate "change" option improves user experience.
Suggestion: Consider adding error handling or fallback values for
sessionPeriodandspendLimitin case they are undefined. This will prevent potential runtime errors and improve robustness.To verify the usage of
sessionPeriodandspendLimit, you can run the following script:packages/web/components/one-click-trading/profile-one-click-trading-settings.tsx (3)
57-57: Improved boolean conversion fordefaultIsOneClickEnabledThe use of double negation (
!!) to convertisOneClickTradingEnabledto a boolean is a concise and idiomatic way to ensure a boolean value. This change simplifies the code while maintaining the same functionality.
57-65: Overall assessment of changesThe modifications in this file effectively enhance the one-click trading functionality:
- Improved boolean conversion for
defaultIsOneClickEnabled.- Added
scopeKeyfor better context management.- Introduced reset functionality for transaction parameters.
These changes align well with the PR objectives of improving user engagement and conversion rates for the one-click trading feature. The code is clean, and the new additions provide more control and context to the trading settings.
65-65: Added reset functionality for transaction parametersThe addition of the
resetTransaction1CTParamsprop enhances the control over one-click trading settings, allowing for easy reset of transaction parameters. This aligns well with the PR objectives of improving user interaction with the feature.To ensure this new prop is properly utilized, please run the following script to check its implementation in the
OneClickTradingSettingscomponent:✅ Verification successful
Reset functionality for transaction parameters verified successfully
TheresetTransaction1CTParamsprop is correctly implemented and utilized across relevant components, ensuring proper functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of resetTransaction1CTParams in OneClickTradingSettings # Test: Search for the resetTransaction1CTParams prop in OneClickTradingSettings rg -A 10 'resetTransaction1CTParams.*OneClickTradingSettings'Length of output: 63
Script:
#!/bin/bash # Description: Search for all usages of resetTransaction1CTParams in the codebase rg 'resetTransaction1CTParams'Length of output: 953
packages/web/hooks/one-click-trading/use-one-click-trading-params.ts (7)
6-7: Necessary imports added foruseQueryanduseState.The addition of
useQueryfrom@tanstack/react-queryanduseStatefrom React is appropriate for the new implementation using React Query and state management.
51-55: Function signature updated with optionalscopeKeyparameter.Including
scopeKey?: stringas an optional parameter enhances the flexibility of the hook, allowing for more granular control over the query key and cache management.
65-66: Renamingrefetchtoresetimproves clarity.Assigning
refetchtoresetenhances readability and makes the intent clearer when resetting the parameters to their initial state.
68-85: Well-structuredqueryFnwith conditional logic for parameter fetching.The
queryFncorrectly handles both scenarios—whenoneClickTradingInfois provided and when it is not—ensuring that appropriate parameters are returned and merged as needed.
70-78: Verify consistency ofspendLimitTokenDecimalswhen merging data.When
oneClickTradingInfois provided,spendLimitTokenDecimalsis sourced fromdata, not fromoneClickTradingInfo. Ensure thatspendLimitTokenDecimalsfromdataaligns with the expectations based ononeClickTradingInfoto prevent potential mismatches or inconsistencies.
86-87: Appropriate use of caching options inuseQuery.Setting
cacheTimeto 10 minutes and disablingrefetchOnWindowFocusmakes sense for performance optimization and to prevent unnecessary data fetching upon window focus, especially after transaction acceptance.
88-90: EnsurespendLimitTokenDecimalsis updated correctly.In the
onSuccesscallback,spendLimitTokenDecimalsis destructured but not stored in state. SincespendLimitTokenDecimalsis returned from the hook, confirm that it reflects the latest value from the query result. If needed, consider storing it in state or adjusting the return value to ensure consistency.packages/web/hooks/one-click-trading/use-one-click-trading-session.ts (6)
1-2: New imports are necessary for added functionalitiesThe added imports from
@keplr-wallet/unitand@osmosis-labs/serverare required for handling decimal calculations and default currency settings in the new features introduced below.
12-12: Importingapifor tRPC queriesThe inclusion of
apifrom~/utils/trpcis appropriate and necessary for performing tRPC queries to fetch data related to the one-click trading session.
58-58: AddingaccountStore.oneClickTradingInfoto dependenciesIncluding
accountStore.oneClickTradingInfoin the dependency array of theuseAsynchook ensures that any changes to the one-click trading info trigger a re-execution of the asynchronous logic.
61-71: Fetching amount spent with conditional query executionThe use of
api.local.oneClickTrading.getAmountSpent.useQuerycorrectly fetches the amount spent in the one-click trading session. The query is conditionally enabled based on the presence ofvalue?.infoand whether one-click trading is enabled, which prevents unnecessary network requests when the data is not needed.
91-106: Accurate calculation of spending limit remainingThe
getSpendingLimitRemainingfunction correctly calculates the remaining spending limit by:
- Checking if
oneClickTradingInfoandamountSpentDataare available.- Calculating the
spendLimitwith proper handling of decimals usingDecandDecUtils.- Subtracting the
amountSpentfrom the totalspendLimitusingPricePretty.The implementation ensures precision in financial calculations.
129-137: Enhanced return object with additional data and loading stateUpdating the return object to include
amountSpentData,getSpendingLimitRemaining, and adjustingisLoadingInfoto account foramountSpentIsLoadingensures that components consuming this hook have access to the necessary data and accurate loading states.packages/web/components/one-click-trading/one-click-trading-in-review-modal.tsx (4)
83-85: Verify the type and conversion ofsessionAuthenticator.id.You are converting
sessionAuthenticator.idto aBigIntusingBigInt(sessionAuthenticator.id). Ensure thatsessionAuthenticator.idis of a type that can be accurately converted to aBigInt(e.g., a string or number). If it's already aBigInt, this conversion might be unnecessary or could cause an error.
101-108: ** useImperativeHandle correctly implemented.**The
useImperativeHandlehook is properly used to exposeisLoadingandonStartTradingmethods to parent components, allowing for controlled interactions.
35-38: Component definition is well-structured.The
OneClickTradingInReviewModalcomponent is correctly defined usingforwardRef, and it properly types its props and ref parameters, enhancing readability and maintainability.
110-161: Props passed toOneClickTradingSettingsare comprehensive.The props provided to
<OneClickTradingSettings>cover all necessary parameters and callbacks, ensuring the child component has all required data and functions to operate correctly.packages/web/components/one-click-trading/one-click-trading-settings.tsx (2)
396-402: Conditional rendering of the creation buttonThe addition of the
showCreationButtonprop and its usage effectively allows conditional rendering of the creation button, enhancing component flexibility.
117-120: Encapsulate session start logicThe
handleOnStartSessionfunction neatly encapsulates the logic for starting a session and resetting changes, improving code clarity.packages/web/modals/review-order.tsx (1)
753-753: EnsureoneClickTradingRef.currentis Defined Before AccessUsing
oneClickTradingRef.current?.isLoadingmay lead to issues ifoneClickTradingRef.currentisnullduring the initial render. Ensure that the ref is initialized before accessing its properties to prevent potential runtime errors.
| </p> | ||
| ); | ||
| } | ||
| return <p>{getSpendingLimitRemaining().toString()}</p>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance active session display with context and proper formatting.
While the logic for displaying the remaining amount is functional, there are several areas for improvement:
- Add context to the displayed amount for better user understanding.
- Use proper number formatting for better readability and localization.
- Add error handling in case
getSpendingLimitRemaining()fails.
Consider refactoring the return statement as follows:
import { formatCurrency } from "~/utils/number"; // Assume this utility exists
try {
const remainingAmount = getSpendingLimitRemaining();
return (
<p className="body1 text-osmoverse-300">
{t("oneClickTrading.profile.remainingAmount", {
amount: formatCurrency(remainingAmount),
})}
</p>
);
} catch (error) {
console.error("Failed to get remaining amount:", error);
return (
<p className="body1 text-osmoverse-300">
{t("oneClickTrading.profile.errorGettingAmount")}
</p>
);
}This suggestion assumes the existence of a formatCurrency utility and appropriate translation keys. Adjust as needed based on your project's actual utilities and translation setup.
| export const OneClickTradingRemainingAmount: React.FC = () => { | ||
| const { t } = useTranslation(); | ||
| const { isOneClickTradingExpired, getSpendingLimitRemaining } = | ||
| useOneClickTradingSession(); | ||
|
|
||
| if (isOneClickTradingExpired) { | ||
| return ( | ||
| <p className="body1 text-osmoverse-300"> | ||
| {t("oneClickTrading.profile.sessionExpired")} | ||
| </p> | ||
| ); | ||
| } | ||
| return <p>{getSpendingLimitRemaining().toString()}</p>; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the component with additional states and props.
The component handles the basic functionality well, but could be improved for better user experience and maintainability:
- Add a loading state to handle potential asynchronous operations.
- Consider adding a prop to handle cases where one-click trading is entirely disabled.
- Implement error boundaries for more robust error handling.
Here's a suggested structure for these improvements:
interface OneClickTradingRemainingAmountProps {
isOneClickTradingEnabled: boolean;
}
export const OneClickTradingRemainingAmount: React.FC<OneClickTradingRemainingAmountProps> = ({
isOneClickTradingEnabled
}) => {
const { t } = useTranslation();
const { isOneClickTradingExpired, getSpendingLimitRemaining, isLoading } =
useOneClickTradingSession();
if (!isOneClickTradingEnabled) {
return <p>{t("oneClickTrading.profile.disabled")}</p>;
}
if (isLoading) {
return <p>{t("oneClickTrading.profile.loading")}</p>;
}
if (isOneClickTradingExpired) {
return (
<p className="body1 text-osmoverse-300">
{t("oneClickTrading.profile.sessionExpired")}
</p>
);
}
try {
const remainingAmount = getSpendingLimitRemaining();
return (
<p className="body1 text-osmoverse-300">
{t("oneClickTrading.profile.remainingAmount", {
amount: formatCurrency(remainingAmount),
})}
</p>
);
} catch (error) {
console.error("Failed to get remaining amount:", error);
return (
<p className="body1 text-osmoverse-300">
{t("oneClickTrading.profile.errorGettingAmount")}
</p>
);
}
};This refactored version includes handling for disabled one-click trading, loading state, and maintains the existing functionality with improved error handling.
| export const OneClickTradingLimitRemaining: React.FC<Props> = ({ | ||
| isSpendingLimitExceeded, | ||
| onRequestEdit, | ||
| }) => { | ||
| const { t } = useTranslation(); | ||
|
|
||
| if (!isSpendingLimitExceeded) { | ||
| return ( | ||
| <div | ||
| className="body2 flex cursor-pointer items-center justify-end gap-1 text-wosmongton-300" | ||
| onClick={onRequestEdit} | ||
| > | ||
| <OneClickTradingRemainingAmount /> | ||
| <p>/</p> | ||
| <OneClickTradingRemainingTime className="!body2 !text-wosmongton-300" /> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <div className="body2 flex items-center justify-end gap-1 text-wosmongton-300"> | ||
| <button | ||
| type="button" | ||
| className="flex min-h-[2rem] items-center justify-center rounded-5xl border border-osmoverse-700 py-1 px-3" | ||
| onClick={onRequestEdit} | ||
| > | ||
| <span className="body2 sm:caption text-wosmongton-300"> | ||
| {t("edit")} | ||
| </span> | ||
| </button> | ||
| <Tooltip | ||
| rootClassNames="!max-w-[17.5rem]" | ||
| content={ | ||
| <div className="caption flex flex-col gap-1"> | ||
| <p>{t("oneClickTrading.tooltipExceeded.title")}</p> | ||
| <p className="text-osmoverse-300"> | ||
| {t("oneClickTrading.tooltipExceeded.description")} | ||
| </p> | ||
| </div> | ||
| } | ||
| > | ||
| <p className="body-2 text-rust-400">{t("exceeded")}</p> | ||
| </Tooltip> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoization for potential performance optimization.
While the current implementation should perform well in most cases, if the parent component re-renders frequently, memoizing this component could be beneficial.
Consider wrapping the component with React.memo:
export const OneClickTradingLimitRemaining = React.memo<Props>(({
isSpendingLimitExceeded,
onRequestEdit,
}) => {
// ... component body ...
});This optimization ensures that the component only re-renders when its props change, potentially improving performance in scenarios with frequent parent re-renders.
packages/web/components/one-click-trading/one-click-trading-limit-remaining.tsx
Show resolved
Hide resolved
| const [transaction1CTParams, setTransaction1CTParams] = | ||
| useState<OneClickTradingTransactionParams>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initializing transaction1CTParams state without a default value.
Currently, transaction1CTParams is initialized without a default value and remains undefined until the query succeeds. This could lead to issues in consuming components if they do not handle the undefined state. Consider initializing transaction1CTParams with a default value or ensuring that consuming components handle the undefined state appropriately.
| onStartTrading: () => void; | ||
| onEndSession?: () => void; | ||
| onClose: () => void; | ||
| onClose?: () => void; |
There was a problem hiding this comment.
Ensure 'onClose' is defined before calling it
Since onClose is now optional, ensure you check if it's defined before calling it to prevent potential runtime errors. In lines 209 and 215, onClose is called without checking for undefined.
Apply this diff to fix the issue:
// At line 209:
-onDiscard={onClose!}
+onDiscard={() => onClose?.()}
// At line 215:
-onClose();
+onClose?.();Also applies to: 209-209, 215-215
| prevParams: prevParams!, | ||
| nextParams: nextParams!, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Avoid non-null assertions on potentially undefined values
In setTransaction1CTParams, both prevParams and nextParams could be undefined, but you're using non-null assertions (prevParams!, nextParams!). This could lead to runtime errors if either is undefined.
Consider adding checks to ensure prevParams and nextParams are defined before using them:
+if (prevParams && nextParams) {
setChanges(
compare1CTTransactionParams({
- prevParams: prevParams!,
- nextParams: nextParams!,
+ prevParams,
+ nextParams,
})
);
+}📝 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.
| prevParams: prevParams!, | |
| nextParams: nextParams!, | |
| }) | |
| ); | |
| if (prevParams && nextParams) { | |
| setChanges( | |
| compare1CTTransactionParams({ | |
| prevParams, | |
| nextParams, | |
| }) | |
| ); | |
| } |
| useEffect(() => { | ||
| if (!isOneClickTradingEnabled || !shouldTriggerTx) return; | ||
| setShouldTriggerTx(false); | ||
| confirmAction(); | ||
| }, [confirmAction, isOneClickTradingEnabled]); |
There was a problem hiding this comment.
Add Missing Dependency shouldTriggerTx in useEffect Hook
The useEffect hook uses shouldTriggerTx in its condition but does not include it in the dependency array. This can cause the effect not to run when shouldTriggerTx changes, leading to inconsistent behavior.
Apply this diff to include the missing dependency:
}, [confirmAction, isOneClickTradingEnabled
+ , shouldTriggerTx
]);📝 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.
| useEffect(() => { | |
| if (!isOneClickTradingEnabled || !shouldTriggerTx) return; | |
| setShouldTriggerTx(false); | |
| confirmAction(); | |
| }, [confirmAction, isOneClickTradingEnabled]); | |
| useEffect(() => { | |
| if (!isOneClickTradingEnabled || !shouldTriggerTx) return; | |
| setShouldTriggerTx(false); | |
| confirmAction(); | |
| }, [confirmAction, isOneClickTradingEnabled, shouldTriggerTx]); |
| const handleOnClose = useCallback(() => { | ||
| setShow1CTSettings(false); | ||
| reset1CTParams(); | ||
| onClose(); | ||
| }, []); |
There was a problem hiding this comment.
Include Dependencies in handleOnClose's Dependency Array
The handleOnClose function uses setShow1CTSettings, reset1CTParams, and onClose, but they are not included in the dependency array of useCallback. This might lead to stale closures if any of these dependencies change.
Apply this diff to include the dependencies:
const handleOnClose = useCallback(() => {
setShow1CTSettings(false);
reset1CTParams();
onClose();
- }, []);
+ }, [setShow1CTSettings, reset1CTParams, onClose]);📝 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 handleOnClose = useCallback(() => { | |
| setShow1CTSettings(false); | |
| reset1CTParams(); | |
| onClose(); | |
| }, []); | |
| const handleOnClose = useCallback(() => { | |
| setShow1CTSettings(false); | |
| reset1CTParams(); | |
| onClose(); | |
| }, [setShow1CTSettings, reset1CTParams, onClose]); |
| if ( | ||
| !isOneClickTradingEnabled && | ||
| transaction1CTParams?.isOneClickEnabled && | ||
| oneClickTradingRef.current | ||
| ) { | ||
| await oneClickTradingRef.current.onStartTrading(); | ||
| setShouldTriggerTx(true); | ||
| } else { | ||
| confirmAction(); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Transaction Trigger Logic to Avoid Delay
Setting shouldTriggerTx to true and relying on the useEffect hook to call confirmAction() adds unnecessary complexity and might introduce delays in transaction execution.
Consider calling confirmAction() directly after onStartTrading():
await oneClickTradingRef.current.onStartTrading();
- setShouldTriggerTx(true);
+ confirmAction();Then, you can remove the useEffect hook at lines 210-214 related to shouldTriggerTx.
Committable suggestion was skipped due to low confidence.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| const handleToggle = () => { | ||
| if (isOneClickEnabled) { | ||
| logEvent([EventName.OneClickTrading.enableOneClickTrading]); | ||
| } | ||
| setIsOneClickEnabled(!isOneClickEnabled); | ||
| }; |
There was a problem hiding this comment.
Since it’s using the current state value, it will only log the event when the user toggles the switch off. If we want it to trigger whenever the switch is turned on, we need to use the next state value instead
| const handleToggle = () => { | |
| if (isOneClickEnabled) { | |
| logEvent([EventName.OneClickTrading.enableOneClickTrading]); | |
| } | |
| setIsOneClickEnabled(!isOneClickEnabled); | |
| }; | |
| const handleToggle = () => { | |
| const nextState = !isOneClickEnabled; | |
| setIsOneClickEnabled(nextState); | |
| if (nextState) { | |
| logEvent([EventName.OneClickTrading.enableOneClickTrading]); | |
| } | |
| }; |
There was a problem hiding this comment.
Thanks for the catch, I will update it.
| className={classNames( | ||
| "group flex cursor-pointer items-center justify-between gap-4 rounded-2xl bg-osmoverse-alpha-800/[.54] p-4" | ||
| )} |
There was a problem hiding this comment.
| className={classNames( | |
| "group flex cursor-pointer items-center justify-between gap-4 rounded-2xl bg-osmoverse-alpha-800/[.54] p-4" | |
| )} | |
| className="group flex cursor-pointer items-center justify-between gap-4 rounded-2xl bg-osmoverse-alpha-800/[.54] p-4" |
| useOneClickTradingSession(); | ||
| const removeSession = useRemoveOneClickTradingSession(); | ||
| const create1CTSession = useCreateOneClickTradingSession(); | ||
| const account = accountStore.getWallet(chainStore.osmosis.chainId); |
There was a problem hiding this comment.
| const account = accountStore.getWallet(chainStore.osmosis.chainId); | |
| const account = accountStore.getWallet(accountStore.osmosisChainId); |
| const onStartTrading = useCallback(async () => { | ||
| await create1CTSession.mutateAsync( | ||
| { | ||
| spendLimitTokenDecimals, | ||
| transaction1CTParams, | ||
| walletRepo: accountStore.getWalletRepo(chainStore.osmosis.chainId), | ||
| /** | ||
| * If the user has an existing session, remove it and add the new one. | ||
| */ | ||
| additionalAuthenticatorsToRemove: sessionAuthenticator | ||
| ? [BigInt(sessionAuthenticator.id)] | ||
| : undefined, | ||
| }, | ||
| { | ||
| onSuccess: onGoBack, | ||
| } | ||
| ); | ||
| }, [ | ||
| onGoBack, | ||
| accountStore, | ||
| chainStore.osmosis.chainId, | ||
| create1CTSession, | ||
| sessionAuthenticator, | ||
| spendLimitTokenDecimals, | ||
| transaction1CTParams, | ||
| ]); |
There was a problem hiding this comment.
Nit
| const onStartTrading = useCallback(async () => { | |
| await create1CTSession.mutateAsync( | |
| { | |
| spendLimitTokenDecimals, | |
| transaction1CTParams, | |
| walletRepo: accountStore.getWalletRepo(chainStore.osmosis.chainId), | |
| /** | |
| * If the user has an existing session, remove it and add the new one. | |
| */ | |
| additionalAuthenticatorsToRemove: sessionAuthenticator | |
| ? [BigInt(sessionAuthenticator.id)] | |
| : undefined, | |
| }, | |
| { | |
| onSuccess: onGoBack, | |
| } | |
| ); | |
| }, [ | |
| onGoBack, | |
| accountStore, | |
| chainStore.osmosis.chainId, | |
| create1CTSession, | |
| sessionAuthenticator, | |
| spendLimitTokenDecimals, | |
| transaction1CTParams, | |
| ]); | |
| const onStartTrading = useCallback(() => { | |
| create1CTSession.mutate( | |
| { | |
| spendLimitTokenDecimals, | |
| transaction1CTParams, | |
| walletRepo: accountStore.getWalletRepo(chainStore.osmosis.chainId), | |
| /** | |
| * If the user has an existing session, remove it and add the new one. | |
| */ | |
| additionalAuthenticatorsToRemove: sessionAuthenticator | |
| ? [BigInt(sessionAuthenticator.id)] | |
| : undefined, | |
| }, | |
| { | |
| onSuccess: onGoBack, | |
| } | |
| ); | |
| }, [ | |
| onGoBack, | |
| accountStore, | |
| chainStore.osmosis.chainId, | |
| create1CTSession, | |
| sessionAuthenticator, | |
| spendLimitTokenDecimals, | |
| transaction1CTParams, | |
| ]); |
There was a problem hiding this comment.
I see more advantages having async this function rather than sync because it could be reusable in multiples flows since we can wait until complete and not fire and forget.
packages/web/components/one-click-trading/one-click-trading-settings.tsx
Show resolved
Hide resolved
| @@ -47,74 +48,47 @@ export function getParametersFromOneClickTradingInfo({ | |||
| export const useOneClickTradingParams = ({ | |||
| oneClickTradingInfo, | |||
| defaultIsOneClickEnabled = false, | |||
| scopeKey = "oneClickTradingParams", | |||
There was a problem hiding this comment.
Why do we need a scopeKey?
There was a problem hiding this comment.
I added an scopeKey in order to make reusable in other scenarios in the future and prevent collisions. In most of the case and probably we never have to use the scopeKey but is nice to have it.
| const { | ||
| data: defaultTransaction1CTParams, | ||
| isLoading, | ||
| isError, | ||
| } = api.local.oneClickTrading.getParameters.useQuery(); | ||
|
|
||
| const [transaction1CTParams, setTransaction1CTParams] = useState< | ||
| OneClickTradingTransactionParams | undefined | ||
| >( | ||
| oneClickTradingInfo | ||
| ? getParametersFromOneClickTradingInfo({ | ||
| oneClickTradingInfo, | ||
| defaultIsOneClickEnabled, | ||
| }) | ||
| : undefined | ||
| ); | ||
| const [initialTransaction1CTParams, setInitialTransaction1CTParams] = | ||
| useState<OneClickTradingTransactionParams | undefined>(); | ||
|
|
||
| useEffect(() => { | ||
| const paramsToSet = oneClickTradingInfo | ||
| ? getParametersFromOneClickTradingInfo({ | ||
| oneClickTradingInfo, | ||
| defaultIsOneClickEnabled, | ||
| }) | ||
| : defaultTransaction1CTParams; | ||
| refetch: reset, | ||
| } = useQuery({ | ||
| queryKey: [scopeKey, oneClickTradingInfo?.sessionKey], | ||
| queryFn: async () => { | ||
| const data = await apiUtils.local.oneClickTrading.getParameters.fetch(); | ||
| if (oneClickTradingInfo) { | ||
| return { | ||
| ...getParametersFromOneClickTradingInfo({ | ||
| oneClickTradingInfo, | ||
| defaultIsOneClickEnabled, | ||
| }), | ||
| spendLimitTokenDecimals: data.spendLimitTokenDecimals, | ||
| }; | ||
| } | ||
|
|
||
| if (!paramsToSet || transaction1CTParams) return; | ||
|
|
||
| const nextTransaction1CTParams = { | ||
| isOneClickEnabled: defaultIsOneClickEnabled, | ||
| ...paramsToSet, | ||
| }; | ||
| setTransaction1CTParams(nextTransaction1CTParams); | ||
| setInitialTransaction1CTParams(nextTransaction1CTParams); | ||
| }, [ | ||
| defaultIsOneClickEnabled, | ||
| defaultTransaction1CTParams, | ||
| oneClickTradingInfo, | ||
| transaction1CTParams, | ||
| ]); | ||
|
|
||
| const reset = useCallback(() => { | ||
| const paramsToSet = oneClickTradingInfo | ||
| ? getParametersFromOneClickTradingInfo({ | ||
| oneClickTradingInfo, | ||
| defaultIsOneClickEnabled, | ||
| }) | ||
| : defaultTransaction1CTParams; | ||
| if (!paramsToSet && !initialTransaction1CTParams) return; | ||
|
|
||
| const nextTransaction1CTParams = paramsToSet | ||
| ? { | ||
| isOneClickEnabled: defaultIsOneClickEnabled, | ||
| ...paramsToSet, | ||
| } | ||
| : initialTransaction1CTParams; | ||
| setTransaction1CTParams(nextTransaction1CTParams); | ||
| }, [ | ||
| defaultIsOneClickEnabled, | ||
| defaultTransaction1CTParams, | ||
| initialTransaction1CTParams, | ||
| oneClickTradingInfo, | ||
| ]); | ||
| return { | ||
| ...data, | ||
| spendLimitTokenDecimals: data.spendLimitTokenDecimals, | ||
| isOneClickEnabled: defaultIsOneClickEnabled, | ||
| }; | ||
| }, | ||
| cacheTime: 1000 * 60 * 10, | ||
| refetchOnWindowFocus: false, // Prevents refetching after losing the focus when accepting the transaction | ||
| onSuccess: ({ spendLimitTokenDecimals, ...rest }) => { | ||
| setTransaction1CTParams(rest); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
We prefer not to manage query keys ourselves. It seems that this query hook was added to merge the default parameters with those from a previous session into a single hook. However, I don’t recommend this approach because it can lead to many edge cases, especially concerning cache values. It also adds complexity since we would need to use a scopeKey to prevent collisions. I suggest reverting these changes
There was a problem hiding this comment.
I consider this an improvement and would like to include it. I don't consider it could lead into edge cases, neither consider bad having a cache.
ScopeKey as I described before is not necessary to use in any case at the moment and most likely in the future.
That being said this hooks has a bug before and this is the approach I took to resolve it.
The bug is the following one:
Most of the time this hooks is used in combination with the session, and session is async so thats why oneClickTradingInfo param is also accepted as undefined
- When mounting the transaction1CTParams is undefined since the session is still undefined
- After that it does the useEffect as the session is undefined it use
defaultTransaction1CTParamsand set the values intransaction1CTParamsandinitialTransaction1CTParams - After that session is loaded updated the param and provided to the hook
- It does again the useEffect but this time has the
paramsToSethowever there is a condition in (line 81)[https://github.com/osmosis-labs/osmosis-frontend/blob/stage/packages/web/hooks/one-click-trading/use-one-click-trading-params.ts#L81C25-L81C45] where if it already hastransaction1CTParamsdoes the return making the hook not refreshing the state with the session.
There was a problem hiding this comment.
We want to retain the previous parameters to track changes, so we avoided overriding the state. Also, these modifications have introduced a bug. Please review the videos for more details.
Before
Screen.Recording.2024-10-22.at.11.58.57.AM.mov
After
Screen.Recording.2024-10-22.at.11.59.33.AM.mov
Initiating a transaction or any state change triggers a parameter update, causing the pending changes to be lost. This is unintended, as it prevents the discard dialog from appearing. Additionally, we want to avoid pre-emptive optimizations, as they tend to increase the risk of bugs and complicate the implementation for scenarios that are unlikely to occur.
| const getSpendingLimitRemaining = useCallback(() => { | ||
| const oneClickTradingInfo = value?.info; | ||
| if (!oneClickTradingInfo || !amountSpentData) { | ||
| return new PricePretty(DEFAULT_VS_CURRENCY, 0); | ||
| } | ||
|
|
||
| const spendLimit = new PricePretty( | ||
| DEFAULT_VS_CURRENCY, | ||
| new Dec(oneClickTradingInfo.spendLimit.amount).quo( | ||
| DecUtils.getTenExponentN(oneClickTradingInfo.spendLimit.decimals) | ||
| ) | ||
| ); | ||
|
|
||
| return spendLimit.sub(amountSpentData.amountSpent); | ||
| }, [value?.info, amountSpentData]); |
There was a problem hiding this comment.
Rather than splitting the logic into multiple steps to calculate the remaining spend limit, we can add a procedure to compute it directly in trpc. This approach prevents slowing down other components that don’t need this value.
| isLoading={oneClickTradingRef.current?.isLoading} | ||
| onClick={async () => { | ||
| if ( | ||
| !isOneClickTradingEnabled && | ||
| transaction1CTParams?.isOneClickEnabled && | ||
| oneClickTradingRef.current | ||
| ) { | ||
| await oneClickTradingRef.current.onStartTrading(); | ||
| setShouldTriggerTx(true); | ||
| } else { | ||
| confirmAction(); | ||
| } | ||
| }} |
There was a problem hiding this comment.
Using refs to trigger a transaction is not recommended because it can lead to race conditions and produce unexpected results. Additionally, isLoading won’t indicate a loading state unless the component re-renders, which may not happen unless an unrelated value changes.
I’m encountering an issue where I’m unable to initiate a swap immediately after a session is created because the 1CT session is still being set. When we attempt to start a 1CT transaction during this time, it results in an error because the session isn’t available yet.
To fix this issue, I believe we need to avoid relying on refs and ensure the session is properly set before initiating the swap.
There was a problem hiding this comment.
Using refs to trigger a transaction is not recommended because it can lead to race conditions and produce unexpected results. Additionally, isLoading won’t indicate a loading state unless the component re-renders, which may not happen unless an unrelated value changes.
I have to disagree with that statement.
It shouldn't lead to race conditions since we create a reference to the function which is async and we can wait for the function to finish.
Regarding the isLoading it should work even without re-render, it is a reference and actually works quite well.
You could notice this because when accepting the transaction in review-modal enabling 1ct shows a spinner from onStartTrading.
That button before didn't have a spinner/loader and now it show one. Ref works pretty well and one of the benefits is that we could have that logic encapsulated inside a component instead of having in the parent in this case review-modal.
I’m encountering an issue where I’m unable to initiate a swap immediately after a session is created because the 1CT session is still being set. When we attempt to start a 1CT transaction during this time, it results in an error because the session isn’t available yet.
That's not true it tries to submit the tx when it already have the session, after submitting the tx we change shouldTriggerTx state to true, but it only will only trigger when there is session. Here: https://github.com/osmosis-labs/osmosis-frontend/pull/3900/files#diff-25e66816c2ccd502684bb6420546938081ed0604faeb5dce40ad2277e8d237efR211
The problem is a different one related with this task is in the review modal which is something to be tackle in a different task thought.
There was a problem hiding this comment.
I understand your perspective, and I appreciate your detailed explanation. However, I’m still encountering an issue where I’m unable to initiate a swap immediately after a session is created because the 1CT session is still being set.
| <div className="flex items-center gap-4"> | ||
| <Image | ||
| src="/images/1ct-medium-icon.svg" | ||
| alt="1-Click trading icon" |
There was a problem hiding this comment.
We should internationalize the alt tag as well - we haven't done this everywhere in the codebase, but we should get in the habit of doing this
| className="cursor-pointer text-wosmongton-300" | ||
| onClick={onRequestOptions} | ||
| > | ||
| {t("change")} |
There was a problem hiding this comment.
We should add a key for this - similar to other modals, see example below from en.json
"activateUnverifiedAssetsModal": {
"activate": "Activate unverified assets",
"description": "is an unverified token. Do you wish to activate it? Be sure to research thoroughly before trading.",
"reminder": "You can always deactivate this setting in the settings modal."
}
| onClick={onRequestEdit} | ||
| > | ||
| <span className="body2 sm:caption text-wosmongton-300"> | ||
| {t("edit")} |
There was a problem hiding this comment.
We should also nest this under a key in en.json
| <OneClickTradingRemainingAmount /> | ||
| <p>/</p> | ||
| <OneClickTradingRemainingTime className="!body2 !text-wosmongton-300" /> | ||
| </div> |
There was a problem hiding this comment.
We might consider creating a button and passing the children depending on designs, but a ghost mode button might be reasonable here
|
@Kyatros translation is missing |
|
On mobile 1click trading is not getting enabled even after signed tx |
| @@ -105,13 +106,18 @@ export const OneClickTradingSettings = ({ | |||
| hasExistingSession, | |||
| onEndSession, | |||
| onClose, | |||
| resetTransaction1CTParams, | |||
There was a problem hiding this comment.
Maybe we should rename this to onDiscardChanges to make it more generic.
|
Going to review once changes are made from prior reviews |
|
@Kyatros looks like we still have some bugs on this feature. Are you able to come up with a working solution? |
What is the purpose of the change:
This PR introduces a new call to action (CTA) for the "1-click trading" feature within the Swap modal in the Osmosis frontend. The goal is to increase user engagement and conversion for this feature by adding a more visible and accessible CTA. Additionally, this PR includes functionality to track and display the length of a session, as required by the acceptance criteria in the provided Figma design.
Linear Task
Brief Changelog
-1-click trading CTA feature on Review Trade Modal.
-Improve functionallity and performance in 1CT hook.
-Feature of modifying 1ct session on Review Trade Modal.
Testing and Verifying
Tested locally
Use cases covered:
If the user does NOT have an active 1CT session:
If the user has an active 1CT session: