Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: 1ct review modal #3900

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { useOneClickTradingSession, useTranslation } from "~/hooks";

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>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Add context to the displayed amount for better user understanding.
  2. Use proper number formatting for better readability and localization.
  3. 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.

};
Comment on lines +3 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Add a loading state to handle potential asynchronous operations.
  2. Consider adding a prop to handle cases where one-click trading is entirely disabled.
  3. 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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import { OneClickTradingTransactionParams } from "@osmosis-labs/types";
import classNames from "classnames";
import Image from "next/image";

import { Switch } from "~/components/ui/switch";
import { EventName } from "~/config";
import { useAmplitudeAnalytics, useTranslation } from "~/hooks";

interface Props {
onRequestOptions?: () => void;
spendLimit: OneClickTradingTransactionParams["spendLimit"];
isOneClickEnabled: OneClickTradingTransactionParams["isOneClickEnabled"];
sessionPeriod: OneClickTradingTransactionParams["sessionPeriod"];
setIsOneClickEnabled: (v: boolean) => void;
}

export const OneClickTradingCallToAction: React.FC<Props> = ({
onRequestOptions,
isOneClickEnabled,
sessionPeriod,
spendLimit,
setIsOneClickEnabled,
}) => {
const { t } = useTranslation();
const { logEvent } = useAmplitudeAnalytics();

const handleToggle = () => {
if (isOneClickEnabled) {
logEvent([EventName.OneClickTrading.enableOneClickTrading]);
}
setIsOneClickEnabled(!isOneClickEnabled);
};
Comment on lines +27 to +32
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
const handleToggle = () => {
if (isOneClickEnabled) {
logEvent([EventName.OneClickTrading.enableOneClickTrading]);
}
setIsOneClickEnabled(!isOneClickEnabled);
};
const handleToggle = () => {
const nextState = !isOneClickEnabled;
setIsOneClickEnabled(nextState);
if (nextState) {
logEvent([EventName.OneClickTrading.enableOneClickTrading]);
}
};

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch, I will update it.


return (
<div className="flex w-full flex-col gap-2 py-3">
<div
className={classNames(
"group flex cursor-pointer items-center justify-between gap-4 rounded-2xl bg-osmoverse-alpha-800/[.54] p-4"
)}
Comment on lines +37 to +39
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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"

onClick={handleToggle}
>
<div className="flex items-center gap-4">
<Image
src="/images/1ct-medium-icon.svg"
alt="1-Click trading icon"
Copy link
Contributor

@mattupham mattupham Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

width={48}
height={48}
/>
<div className="flex flex-col gap-1">
<p>{t("oneClickTrading.reviewSwapModal.title")}</p>
<p className="body2 text-osmoverse-300">
{t("oneClickTrading.reviewSwapModal.subtitle")}
</p>
</div>
</div>
<Switch checked={isOneClickEnabled} />
</div>
{isOneClickEnabled ? (
<p className="text-sm">
<span className="text-osmoverse-300">
{t("oneClickTrading.reviewSwapModal.details", {
time: t(
`oneClickTrading.settings.sessionPeriodScreen.periods.${sessionPeriod?.end}`
),
limit: spendLimit?.toString() || "$5,000 USD",
})}
</span>
<span
className="cursor-pointer text-wosmongton-300"
onClick={onRequestOptions}
>
{t("change")}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."
    }

</span>
</p>
) : null}
</div>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
import { OneClickTradingTransactionParams } from "@osmosis-labs/types";
import {
Dispatch,
forwardRef,
SetStateAction,
useCallback,
useImperativeHandle,
} from "react";

import { displayErrorRemovingSessionToast } from "~/components/alert/one-click-trading-toasts";
import { isRejectedTxErrorMessage } from "~/components/alert/prettify";
import { OneClickTradingSettings } from "~/components/one-click-trading/one-click-trading-settings";
import { useOneClickTradingSession } from "~/hooks";
import { useCreateOneClickTradingSession } from "~/hooks/mutations/one-click-trading";
import { useRemoveOneClickTradingSession } from "~/hooks/mutations/one-click-trading/use-remove-one-click-trading-session";
import { useStore } from "~/stores";
import { api } from "~/utils/trpc";

interface Props {
onGoBack: () => void;
transaction1CTParams: OneClickTradingTransactionParams | undefined;
setTransaction1CTParams: Dispatch<
SetStateAction<OneClickTradingTransactionParams | undefined>
>;
spendLimitTokenDecimals: number | undefined;
isLoading1CTParams?: boolean;
resetTransaction1CTParams: () => void;
}

export interface OneClickTradingInReviewModalRef {
onStartTrading: () => Promise<void>;
isLoading: boolean;
}

export const OneClickTradingInReviewModal = forwardRef<
OneClickTradingInReviewModalRef,
Props
>((props, ref) => {
const {
onGoBack,
transaction1CTParams,
setTransaction1CTParams,
spendLimitTokenDecimals,
isLoading1CTParams,
resetTransaction1CTParams,
} = props;

const { accountStore, chainStore } = useStore();
const { oneClickTradingInfo, isOneClickTradingEnabled } =
useOneClickTradingSession();
const removeSession = useRemoveOneClickTradingSession();
const create1CTSession = useCreateOneClickTradingSession();
const account = accountStore.getWallet(chainStore.osmosis.chainId);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const account = accountStore.getWallet(chainStore.osmosis.chainId);
const account = accountStore.getWallet(accountStore.osmosisChainId);


const shouldFetchSessionAuthenticator =
!!account?.address && !!oneClickTradingInfo;

const {
data: sessionAuthenticator,
isLoading: isLoadingSessionAuthenticator,
} = api.local.oneClickTrading.getSessionAuthenticator.useQuery(
{
userOsmoAddress: account?.address ?? "",
publicKey: oneClickTradingInfo?.publicKey ?? "",
},
{
enabled: shouldFetchSessionAuthenticator,
cacheTime: 15_000, // 15 seconds
staleTime: 15_000, // 15 seconds
retry: false,
}
);
Comment on lines +58 to +72
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid passing empty strings as default query parameters.

In the useQuery hook, userOsmoAddress and publicKey are defaulting to empty strings when undefined:

{
  userOsmoAddress: account?.address ?? "",
  publicKey: oneClickTradingInfo?.publicKey ?? "",
}

Passing empty strings may lead to unintended queries or cache collisions. Consider the following:

  • Adjust the enabled condition to ensure the query only runs when valid parameters are available.
  • Handle undefined values appropriately without defaulting to empty strings to prevent unnecessary API calls.


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,
]);
Comment on lines +74 to +99
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit

Suggested change
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,
]);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


useImperativeHandle(
ref,
() => ({
isLoading: create1CTSession.isLoading,
onStartTrading,
}),
[onStartTrading, create1CTSession]
);

return (
<OneClickTradingSettings
showCreationButton={false}
transaction1CTParams={transaction1CTParams}
setTransaction1CTParams={setTransaction1CTParams}
resetTransaction1CTParams={resetTransaction1CTParams}
isLoading={
isLoading1CTParams ||
(shouldFetchSessionAuthenticator
? isLoadingSessionAuthenticator
: false)
}
isSendingTx={create1CTSession.isLoading}
onStartTrading={onStartTrading}
onGoBack={onGoBack}
hasExistingSession={isOneClickTradingEnabled}
onEndSession={() => {
const rollback = () => {
if (!transaction1CTParams) return;
setTransaction1CTParams({
...transaction1CTParams,
isOneClickEnabled: true,
});
};

if (!oneClickTradingInfo) {
displayErrorRemovingSessionToast();
rollback();
throw new Error("oneClickTradingInfo is undefined");
}

removeSession.mutate(
{
authenticatorId: oneClickTradingInfo?.authenticatorId,
},
{
onSuccess: () => {
onGoBack();
},
onError: (e) => {
const error = e as Error;
rollback();
if (!isRejectedTxErrorMessage({ message: error?.message })) {
displayErrorRemovingSessionToast();
}
},
}
);
}}
Comment on lines +126 to +158
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure proper error handling in onEndSession.

In the onEndSession callback, consider the following:

  • In the onError handler, you're casting the error to Error with const error = e as Error;. To ensure type safety, check if e is actually an instance of Error before casting or use a type guard.
  • When calling displayErrorRemovingSessionToast(), ensure that this function adequately informs the user of the error, and consider logging the error details for debugging purposes.

isEndingSession={removeSession.isLoading}
/>
);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { OneClickTradingRemainingAmount } from "~/components/one-click-trading/one-click-remaining-amount";
import { OneClickTradingRemainingTime } from "~/components/one-click-trading/one-click-remaining-time";
import { Tooltip } from "~/components/tooltip";
import { useTranslation } from "~/hooks";

interface Props {
isSpendingLimitExceeded: boolean;
onRequestEdit: () => void;
}

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}
>
mattupham marked this conversation as resolved.
Show resolved Hide resolved
<OneClickTradingRemainingAmount />
<p>/</p>
<OneClickTradingRemainingTime className="!body2 !text-wosmongton-300" />
</div>
Copy link
Contributor

@mattupham mattupham Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might consider creating a button and passing the children depending on designs, but a ghost mode button might be reasonable here

);
}

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")}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also nest this under a key in en.json

</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>
);
};
Comment on lines +11 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Loading