-
Notifications
You must be signed in to change notification settings - Fork 105
feat: add vip telegram notification for high-value users #1901
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
base: main
Are you sure you want to change the base?
feat: add vip telegram notification for high-value users #1901
Conversation
|
@fionn223 is attempting to deploy a commit to the dYdX Trading Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| const turnkeyFF = useStatsigGateValue(StatsigFlags.ffTurnkeyWeb); | ||
|
|
||
| return forcedTurnkey || turnkeyFF; | ||
| return forcedTurnkey ?? turnkeyFF; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this failing lint or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably best to undo this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forcedTurnkey will never be nullish
|
|
||
| const title = stringGetter({ | ||
| key: STRING_KEYS.YOURE_INVITED, | ||
| fallback: "You're Invited", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove fallbacks for now, it might actually be a good idea but it's best to follow the practices in the rest of the app
| isToast, | ||
| notification, | ||
| portfolioValue: _portfolioValue, | ||
| telegramUrl = 'https://t.me/+NLOpf5jiAEVjODFh', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaredvu do you think we should make this a config of some sort? Any risk here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove the default telegrameUrl here and instead add it to
public/configs/v1/env.json under links for all environments except [mainnet_chain_id] as telegramVipUrl.
then you have to update useURLConfigs to return telegramVipUrl
| export const VipTelegramNotification = ({ | ||
| isToast, | ||
| notification, | ||
| portfolioValue: _portfolioValue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why start with _?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed?
| ); | ||
| }; | ||
|
|
||
| const $Icon = styled.img` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just inline all of these with tw=''
| return; | ||
| } | ||
|
|
||
| const totalValue = equity != null ? equity + (vaultBalance ?? 0) : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could probably just be (equity ?? 0) + (vaultBalance ?? 0)
|
|
||
| const title = stringGetter({ | ||
| key: STRING_KEYS.YOURE_INVITED, | ||
| fallback: "You're Invited", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably don't need fallbacks here either
| sessionStorage.setItem('vip-telegram-notification-shown', 'true'); | ||
|
|
||
| trigger({ | ||
| id: `vip-telegram-invitation-${Date.now()}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why include date in the id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to remove it to get the notification localstorage behavior working
| isToast={isToast} | ||
| notification={notification} | ||
| portfolioValue={totalValue} | ||
| telegramUrl="https://t.me/+NLOpf5jiAEVjODFh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird that this is hardcoded in two places
| }, [equity, isConnected, stringGetter, trigger, vaultBalance]); | ||
| }, | ||
|
|
||
| useNotificationAction: () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can probably just remove this or remove the comment at least
| const equity = useAppSelector(getSubaccountEquity, shallowEqual); | ||
| const { data: vaultAccount } = useLoadedVaultAccount(); | ||
| const vaultBalance = vaultAccount?.balanceUsdc; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After following steps in https://github.com/dydxprotocol/v4-web/pull/1901/files/50fdb651b95914e94f9965d01573902e02f13b37#r2383062316
You can use const { telegramVipUrl } = useUrlConfigs() and return if it doesn't exist, otherwise pass it into the notification being triggered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason we want to go through the trouble of adding it to the env config is so third party deployers that fork this repo are not forced to use these links
Changes