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.

Sign up for GitHub

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

Jump to bottom

[CFDs] Hirad/ Adding the current_list hook #11358

Merged
merged 6 commits into from
Nov 17, 2023
Merged

[CFDs] Hirad/ Adding the current_list hook #11358

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
844e5b3
chore: adding the current_list hook
hirad-deriv Nov 13, 2023
4b489b0
fix: making changes based on the comments
hirad-deriv Nov 14, 2023
040e28c
fix: changed back to the previous state because of the type conflicts
hirad-deriv Nov 14, 2023
d456b1e
chore: getting the latest changes of master
hirad-deriv Nov 15, 2023
1128c09
fix: used api types instead of constant types
hirad-deriv Nov 17, 2023
12e7f6b
chore: pulled the latest changes of master
hirad-deriv Nov 17, 2023
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
8 changes: 5 additions & 3 deletions packages/cfd/src/Components/props.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ export type TCFDPlatform = 'dxtrade' | 'mt5' | 'ctrader' | 'derivez';

export type TCFDsPlatformType = 'dxtrade' | 'derivez' | 'mt5' | 'ctrader' | '';

export type TShortcode = 'svg' | 'bvi' | 'labuan' | 'vanuatu' | 'maltainvest';
Copy link
Contributor

Choose a reason for hiding this comment

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

We can get the types from the api itself.

Copy link
Contributor Author

@hirad-deriv hirad-deriv Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We have a redundant Shortcode in the API (seychelles). I can either get it from the API and Omit that type or just hard code it myself. Let me check the api types one more time

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use Partial or DeepPartial if you face errors

Copy link
Contributor

Choose a reason for hiding this comment

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

can we try this type from @deriv/api-types to be more type safe

Suggested change
export type TShortcode = 'svg' | 'bvi' | 'labuan' | 'vanuatu' | 'maltainvest';
export type TShortcode = DetailsOfEachMT5Loginid['landing_company_short'];

Copy link
Contributor

Choose a reason for hiding this comment

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

Better get it from the hook. Lets avoid getting it from the api-types

Copy link
Contributor Author

@hirad-deriv hirad-deriv Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I face a lot of type conflicts after using the type from the API. We'll work on this fix at the next phase of improving our types since those changes are out of scope of this PR


export type TCFDAccountCopy = {
text: string | undefined;
className: string;
Expand Down Expand Up @@ -87,10 +89,10 @@ export type TTradingPlatformAvailableAccount = {
};
signup: string[];
};
shortcode: 'bvi' | 'labuan' | 'maltainvest' | 'svg' | 'vanuatu';
shortcode: TShortcode;
sub_account_type: string;
account_type?: 'real' | 'demo';
landing_company_short?: 'bvi' | 'labuan' | 'svg' | 'vanuatu';
landing_company_short?: TShortcode;
};

export type TModifiedTradingPlatformAvailableAccount = Omit<TTradingPlatformAvailableAccount, 'market_type'> & {
Expand Down Expand Up @@ -279,7 +281,7 @@ export type TJurisdictionData = {

export type TDetailsOfEachMT5Loginid = DetailsOfEachMT5Loginid & {
display_login?: string;
landing_company_short?: string;
landing_company_short?: TShortcode;
short_code_and_region?: string;
mt5_acc_auth_status?: string | null;
selected_mt5_jurisdiction?: TOpenAccountTransferMeta &
Expand Down
2 changes: 1 addition & 1 deletion packages/cfd/src/Containers/props.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export type TCFDPersonalDetailsContainerProps = {
onSubmit: (index: number, value: { [key: string]: string }) => void;
};

type CFD_Platform = 'dxtrade' | 'mt5' | 'derivez' | 'ctrader';
export type CFD_Platform = 'dxtrade' | 'mt5' | 'derivez' | 'ctrader';
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can refer to TPlatforms namepace in types.ts in wallets package as reference. We let the api do the typing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A round of type improvement is done in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hirad-deriv better to don't add more technical debt for the future, please update the types here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating the types here requires update of at least 10-15 files.
Should I do it ? In my opinion making all of these changes in this PR is out of scope of it


export type TCFDChangePasswordConfirmationProps = {
confirm_label?: string;
Expand Down
51 changes: 51 additions & 0 deletions packages/cfd/src/features/hooks/useCurrentList.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { useStore } from '@deriv/stores';
import { useMT5AccountsList, useDxtradeAccountsList, useCtraderAccountsList } from '@deriv/api';
import { getAccountListKey } from '@deriv/shared';

type MT5AccountsList = NonNullable<ReturnType<typeof useMT5AccountsList>['data']>[number];
type DxtradeAccountsList = NonNullable<ReturnType<typeof useDxtradeAccountsList>['data']>[number];
type CtraderAccountsList = NonNullable<ReturnType<typeof useCtraderAccountsList>['data']>[number];

type TCurrentList = {
[key: string]: MT5AccountsList | DxtradeAccountsList | CtraderAccountsList;
};

/*
This hook is used to get the existing_accounts_data
*/
const useCurrentList = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

But i have a question in general. Why we doing this? We can just call the hooks in the components we wanna use and be done with it. Why another transformation ? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently we are facing issue with the Context that which account we need from the response. Moreover in legacy code we ae using it like mt5.synthetic_all@ps+01 as the key for the account response. Thats why we are transforming to the legacy code because we cannot use Context as we want to separate CFD from Appstore. The context can be provided in Appstore but how we use it inside the CFD. If we pass it again we are introducing a new dependency.

If you have any solution please bring it on we will be happy to have that as our main goal is to have a better code in the deriv-app

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to what Hamza mentioned, we need to stick with the current logic that we have, add related hooks to do the removal of stores first and then decide if refactoring those logics are necessary. This is the main reason of adding some hooks to the CFD package.

const current_list: TCurrentList = {};
const { traders_hub } = useStore();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this step we're removing the logics from the stores and need to make use of some of the states in the store. What do you guys use for EU users ? We can handle it in the useMT5AccountsList as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have handled this for wallet in useWalletAccountsList. We have is_malta_wallet flag. You can add this same flag in useAccountsList and it will be exposed to useActiveTradingAccount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

show_eu_related_content is based on the content_flag and not is_eu. These 2 are not the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrmm we dont need to use that. Maybe another PR

const { data: mt5_accounts } = useMT5AccountsList();
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use the CFDAccountsList because here we are using every platform so combined one make more sense.
If we want we can destructure from the cfd_accounts.
Just a suggestion.

Suggested change
const { data: mt5_accounts } = useMT5AccountsList();
const { data: cfd_accounts } = useCFDAccountsList();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both are possible but it's a matter of preference.

const { data: dxtrade_accounts } = useDxtradeAccountsList();
const { data: ctrader_accounts } = useCtraderAccountsList();
const show_eu_related_content = traders_hub.show_eu_related_content;

mt5_accounts
?.filter(acc =>
show_eu_related_content
? acc.landing_company_short === 'maltainvest'
Copy link
Contributor

Choose a reason for hiding this comment

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

Make use of config file here. Instead of hardcoded value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be done at the stage of removal of dependencies of CFD package. Tried to avoid using shared package as much as possible but seems like we need to use it anyway.

: acc.landing_company_short !== 'maltainvest'
)
.forEach(account => {
current_list[getAccountListKey(account, 'mt5', account.landing_company_short)] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Platform names from config file

...account,
};
});

dxtrade_accounts?.forEach(account => {
current_list[getAccountListKey(account, 'dxtrade', account.landing_company_short)] = {
...account,
};
});

ctrader_accounts?.forEach(account => {
current_list[getAccountListKey(account, 'ctrader', account.landing_company_short)] = {
...account,
};
});

return { current_list };
};

export default useCurrentList;