Skip to content

Commit 4d4bc96

Browse files
VGR-GITVGR-GIT
committed
fix bug where rewards balance was overwriting balance perc change
1 parent 7a4fe52 commit 4d4bc96

16 files changed

+578
-116
lines changed

‎app/scripts/controllers/rewards/rewards-controller.test.ts‎

Lines changed: 189 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1075,6 +1075,34 @@ describe('RewardsController', () => {
10751075
);
10761076
});
10771077
});
1078+
1079+
it('should throw AuthorizationFailedError when subscription token is missing', async () => {
1080+
const state: Partial<RewardsControllerState> = {
1081+
rewardsSeasons: {
1082+
[MOCK_SEASON_ID]: {
1083+
id: MOCK_SEASON_ID,
1084+
name: 'Season 1',
1085+
startDate: new Date('2024-01-01').getTime(),
1086+
endDate: new Date('2024-12-31').getTime(),
1087+
tiers: MOCK_SEASON_TIERS,
1088+
},
1089+
},
1090+
};
1091+
1092+
await withController(
1093+
{ state, isDisabled: false },
1094+
async ({ controller }) => {
1095+
await expect(
1096+
controller.getSeasonStatus(MOCK_SUBSCRIPTION_ID, MOCK_SEASON_ID),
1097+
).rejects.toThrow(
1098+
`No subscription token found for subscription ID: ${MOCK_SUBSCRIPTION_ID}`,
1099+
);
1100+
await expect(
1101+
controller.getSeasonStatus(MOCK_SUBSCRIPTION_ID, MOCK_SEASON_ID),
1102+
).rejects.toBeInstanceOf(AuthorizationFailedError);
1103+
},
1104+
);
1105+
});
10781106
});
10791107

10801108
describe('optIn', () => {
@@ -1676,6 +1704,102 @@ describe('RewardsController', () => {
16761704
expect(result.addressesNeedingFresh).toEqual([MOCK_ACCOUNT_ADDRESS]);
16771705
});
16781706
});
1707+
1708+
it('should force fresh check for not-opted-in accounts checked more than 5 minutes ago', async () => {
1709+
const state: Partial<RewardsControllerState> = {
1710+
rewardsAccounts: {
1711+
[MOCK_CAIP_ACCOUNT]: {
1712+
account: MOCK_CAIP_ACCOUNT,
1713+
hasOptedIn: false,
1714+
subscriptionId: null,
1715+
perpsFeeDiscount: null,
1716+
lastPerpsDiscountRateFetched: null,
1717+
lastFreshOptInStatusCheck: Date.now() - 1000 * 60 * 6, // 6 minutes ago (exceeds 5 minute threshold)
1718+
},
1719+
},
1720+
};
1721+
1722+
await withController({ state, isDisabled: false }, ({ controller }) => {
1723+
const addressToAccountMap = new Map<string, InternalAccount>();
1724+
addressToAccountMap.set(
1725+
MOCK_ACCOUNT_ADDRESS.toLowerCase(),
1726+
MOCK_INTERNAL_ACCOUNT,
1727+
);
1728+
1729+
const result = controller.checkOptInStatusAgainstCache(
1730+
[MOCK_ACCOUNT_ADDRESS],
1731+
addressToAccountMap,
1732+
);
1733+
1734+
expect(result.cachedOptInResults).toEqual([null]);
1735+
expect(result.cachedSubscriptionIds).toEqual([null]);
1736+
expect(result.addressesNeedingFresh).toEqual([MOCK_ACCOUNT_ADDRESS]);
1737+
});
1738+
});
1739+
1740+
it('should use cached data for not-opted-in accounts checked within 5 minutes', async () => {
1741+
const state: Partial<RewardsControllerState> = {
1742+
rewardsAccounts: {
1743+
[MOCK_CAIP_ACCOUNT]: {
1744+
account: MOCK_CAIP_ACCOUNT,
1745+
hasOptedIn: false,
1746+
subscriptionId: null,
1747+
perpsFeeDiscount: null,
1748+
lastPerpsDiscountRateFetched: null,
1749+
lastFreshOptInStatusCheck: Date.now() - 1000 * 60 * 2, // 2 minutes ago (within 5 minute threshold)
1750+
},
1751+
},
1752+
};
1753+
1754+
await withController({ state, isDisabled: false }, ({ controller }) => {
1755+
const addressToAccountMap = new Map<string, InternalAccount>();
1756+
addressToAccountMap.set(
1757+
MOCK_ACCOUNT_ADDRESS.toLowerCase(),
1758+
MOCK_INTERNAL_ACCOUNT,
1759+
);
1760+
1761+
const result = controller.checkOptInStatusAgainstCache(
1762+
[MOCK_ACCOUNT_ADDRESS],
1763+
addressToAccountMap,
1764+
);
1765+
1766+
expect(result.cachedOptInResults).toEqual([false]);
1767+
expect(result.cachedSubscriptionIds).toEqual([null]);
1768+
expect(result.addressesNeedingFresh).toEqual([]);
1769+
});
1770+
});
1771+
1772+
it('should force fresh check for not-opted-in accounts without lastFreshOptInStatusCheck', async () => {
1773+
const state: Partial<RewardsControllerState> = {
1774+
rewardsAccounts: {
1775+
[MOCK_CAIP_ACCOUNT]: {
1776+
account: MOCK_CAIP_ACCOUNT,
1777+
hasOptedIn: false,
1778+
subscriptionId: null,
1779+
perpsFeeDiscount: null,
1780+
lastPerpsDiscountRateFetched: null,
1781+
lastFreshOptInStatusCheck: undefined,
1782+
},
1783+
},
1784+
};
1785+
1786+
await withController({ state, isDisabled: false }, ({ controller }) => {
1787+
const addressToAccountMap = new Map<string, InternalAccount>();
1788+
addressToAccountMap.set(
1789+
MOCK_ACCOUNT_ADDRESS.toLowerCase(),
1790+
MOCK_INTERNAL_ACCOUNT,
1791+
);
1792+
1793+
const result = controller.checkOptInStatusAgainstCache(
1794+
[MOCK_ACCOUNT_ADDRESS],
1795+
addressToAccountMap,
1796+
);
1797+
1798+
expect(result.cachedOptInResults).toEqual([null]);
1799+
expect(result.cachedSubscriptionIds).toEqual([null]);
1800+
expect(result.addressesNeedingFresh).toEqual([MOCK_ACCOUNT_ADDRESS]);
1801+
});
1802+
});
16791803
});
16801804

16811805
describe('shouldSkipSilentAuth', () => {
@@ -1724,6 +1848,30 @@ describe('RewardsController', () => {
17241848
});
17251849
});
17261850

1851+
it('should skip for not-opted-in accounts checked within 5 minutes', async () => {
1852+
const state: Partial<RewardsControllerState> = {
1853+
rewardsAccounts: {
1854+
[MOCK_CAIP_ACCOUNT]: {
1855+
account: MOCK_CAIP_ACCOUNT,
1856+
hasOptedIn: false,
1857+
subscriptionId: null,
1858+
perpsFeeDiscount: null,
1859+
lastPerpsDiscountRateFetched: null,
1860+
lastFreshOptInStatusCheck: Date.now() - 1000 * 60 * 2, // 2 minutes ago (within 5 minute threshold)
1861+
},
1862+
},
1863+
};
1864+
1865+
await withController({ state, isDisabled: false }, ({ controller }) => {
1866+
const result = controller.shouldSkipSilentAuth(
1867+
MOCK_CAIP_ACCOUNT,
1868+
MOCK_INTERNAL_ACCOUNT,
1869+
);
1870+
1871+
expect(result).toBe(true);
1872+
});
1873+
});
1874+
17271875
it('should not skip for stale not-opted-in accounts', async () => {
17281876
const state: Partial<RewardsControllerState> = {
17291877
rewardsAccounts: {
@@ -1733,7 +1881,7 @@ describe('RewardsController', () => {
17331881
subscriptionId: null,
17341882
perpsFeeDiscount: null,
17351883
lastPerpsDiscountRateFetched: null,
1736-
lastFreshOptInStatusCheck: Date.now() - 1000 * 60 * 60 * 24 * 2, // 2 days ago
1884+
lastFreshOptInStatusCheck: Date.now() - 1000 * 60 * 6, // 6 minutes ago (exceeds 5 minute threshold)
17371885
},
17381886
},
17391887
};
@@ -2532,11 +2680,14 @@ describe('Additional RewardsController edge cases', () => {
25322680
});
25332681

25342682
describe('getCandidateSubscriptionId - error scenarios', () => {
2535-
it('should return subscription ID from cache if session token exists', async () => {
2683+
it('should return subscription ID from cache if session token and subscription exist', async () => {
25362684
const state: Partial<RewardsControllerState> = {
25372685
rewardsSubscriptionTokens: {
25382686
[MOCK_SUBSCRIPTION_ID]: MOCK_SESSION_TOKEN,
25392687
},
2688+
rewardsSubscriptions: {
2689+
[MOCK_SUBSCRIPTION_ID]: MOCK_SUBSCRIPTION,
2690+
},
25402691
};
25412692

25422693
await withController(
@@ -2579,6 +2730,42 @@ describe('Additional RewardsController edge cases', () => {
25792730
},
25802731
);
25812732
});
2733+
2734+
it('should continue to silent auth when subscription exists but session token is missing', async () => {
2735+
const state: Partial<RewardsControllerState> = {
2736+
rewardsSubscriptions: {
2737+
[MOCK_SUBSCRIPTION_ID]: MOCK_SUBSCRIPTION,
2738+
},
2739+
};
2740+
2741+
await withController(
2742+
{ state, isDisabled: false },
2743+
async ({ controller, mockMessengerCall }) => {
2744+
mockMessengerCall.mockImplementation((actionType) => {
2745+
if (actionType === 'AccountsController:listMultichainAccounts') {
2746+
return [MOCK_INTERNAL_ACCOUNT];
2747+
}
2748+
if (actionType === 'RewardsDataService:getOptInStatus') {
2749+
return Promise.resolve({
2750+
ois: [true],
2751+
sids: [MOCK_SUBSCRIPTION_ID],
2752+
});
2753+
}
2754+
if (actionType === 'KeyringController:signPersonalMessage') {
2755+
return Promise.resolve('0xmocksignature');
2756+
}
2757+
if (actionType === 'RewardsDataService:login') {
2758+
return Promise.resolve(MOCK_LOGIN_RESPONSE);
2759+
}
2760+
return undefined;
2761+
});
2762+
2763+
const result = await controller.getCandidateSubscriptionId();
2764+
2765+
expect(result).toBe(MOCK_SUBSCRIPTION_ID);
2766+
},
2767+
);
2768+
});
25822769
});
25832770

25842771
describe('linkAccountsToSubscriptionCandidate - error handling', () => {

‎app/scripts/controllers/rewards/rewards-controller.ts‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,8 @@ const SEASON_STATUS_CACHE_THRESHOLD_MS = 1000 * 60 * 1; // 1 minute
5151
// Season metadata cache threshold
5252
const SEASON_METADATA_CACHE_THRESHOLD_MS = 1000 * 60 * 10; // 10 minutes
5353

54-
// Opt-in status stale threshold for not opted-in accounts to force a fresh check
55-
const NOT_OPTED_IN_OIS_STALE_CACHE_THRESHOLD_MS = 1000 * 60 * 60 * 24; // 24 hours
54+
// Opt-in status stale threshold for not opted-in accounts to force a fresh check (less strict than in mobile for now)
55+
const NOT_OPTED_IN_OIS_STALE_CACHE_THRESHOLD_MS = 1000 * 60 * 5; // 5 minutes
5656

5757
/**
5858
* State metadata for the RewardsController
@@ -847,7 +847,6 @@ export class RewardsController extends BaseController<
847847
// Update state with successful authentication
848848
subscription = loginResponse.subscription;
849849

850-
// TODO: Re-enable multi-subscription token vault when implemented
851850
// Store the session token for this subscription
852851
this.#storeSubscriptionToken(subscription.id, loginResponse.sessionId);
853852

@@ -1265,11 +1264,6 @@ export class RewardsController extends BaseController<
12651264
if (!cached) {
12661265
return undefined;
12671266
}
1268-
log.info(
1269-
'RewardsController: Using cached season status data for',
1270-
subscriptionId,
1271-
seasonId,
1272-
);
12731267
return { payload: cached, lastFetched: cached.lastFetched };
12741268
},
12751269
fetchFresh: async () => {
@@ -1281,7 +1275,7 @@ export class RewardsController extends BaseController<
12811275
);
12821276
const subscriptionToken = this.#getSubscriptionToken(subscriptionId);
12831277
if (!subscriptionToken) {
1284-
throw new Error(
1278+
throw new AuthorizationFailedError(
12851279
`No subscription token found for subscription ID: ${subscriptionId}`,
12861280
);
12871281
}
@@ -1806,7 +1800,11 @@ export class RewardsController extends BaseController<
18061800
const sessionToken = subscriptionId
18071801
? this.state.rewardsSubscriptionTokens[subscriptionId]
18081802
: undefined;
1809-
if (subscriptionId && Boolean(sessionToken)) {
1803+
if (
1804+
subscriptionId &&
1805+
Boolean(sessionToken) &&
1806+
this.state.rewardsSubscriptions[subscriptionId]
1807+
) {
18101808
return subscriptionId;
18111809
}
18121810
try {

‎shared/types/background.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ import type { OnboardingControllerState } from '../../app/scripts/controllers/on
7070
import type { MetaMetricsControllerState } from '../../app/scripts/controllers/metametrics-controller';
7171
import type { AppMetadataControllerState } from '../../app/scripts/controllers/app-metadata';
7272
import type { SwapsControllerState } from '../../app/scripts/controllers/swaps/swaps.types';
73+
import type { RewardsControllerState } from '../../app/scripts/controllers/rewards/rewards-controller.types';
7374

7475
export type ControllerStatePropertiesEnumerated = {
7576
internalAccounts: AccountsControllerState['internalAccounts'];
@@ -310,6 +311,12 @@ export type ControllerStatePropertiesEnumerated = {
310311
isAccountSyncingEnabled: UserStorageController.UserStorageControllerState['isAccountSyncingEnabled'];
311312
isContactSyncingEnabled: UserStorageController.UserStorageControllerState['isContactSyncingEnabled'];
312313
isContactSyncingInProgress: UserStorageController.UserStorageControllerState['isContactSyncingInProgress'];
314+
rewardsActiveAccount: RewardsControllerState['rewardsActiveAccount'];
315+
rewardsAccounts: RewardsControllerState['rewardsAccounts'];
316+
rewardsSubscriptions: RewardsControllerState['rewardsSubscriptions'];
317+
rewardsSeasons: RewardsControllerState['rewardsSeasons'];
318+
rewardsSeasonStatuses: RewardsControllerState['rewardsSeasonStatuses'];
319+
rewardsSubscriptionTokens: RewardsControllerState['rewardsSubscriptionTokens'];
313320
};
314321

315322
type ControllerStateTypesMerged = AccountsControllerState &
@@ -370,7 +377,8 @@ type ControllerStateTypesMerged = AccountsControllerState &
370377
TokenRatesControllerState &
371378
TransactionControllerState &
372379
UserOperationControllerState &
373-
UserStorageController.UserStorageControllerState;
380+
UserStorageController.UserStorageControllerState &
381+
RewardsControllerState;
374382

375383
// TODO: Fix in https://github.com/MetaMask/metamask-extension/issues/31860
376384
// eslint-disable-next-line @typescript-eslint/naming-convention

‎ui/components/app/assets/account-group-balance-change/account-group-balance-change.test.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ describe('AccountGroupBalanceChange', () => {
3838

3939
const renderComponent = () =>
4040
renderWithProvider(
41-
<AccountGroupBalanceChange period="1d" portfolioButton={() => null} />,
41+
<AccountGroupBalanceChange period="1d" trailingChild={() => null} />,
4242
mockStore,
4343
);
4444

‎ui/components/app/assets/account-group-balance-change/account-group-balance-change.tsx‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,14 @@ import { useAccountGroupBalanceDisplay } from './useAccountGroupBalanceDisplay';
1818

1919
export type AccountGroupBalanceChangeProps = {
2020
period: BalanceChangePeriod;
21-
portfolioButton: () => JSX.Element | null;
21+
trailingChild: () => JSX.Element | null;
2222
};
2323

2424
const balanceAmountSpanStyle = { whiteSpace: 'pre' } as const;
2525

2626
const AccountGroupBalanceChangeComponent: React.FC<
2727
AccountGroupBalanceChangeProps
28-
> = ({ period, portfolioButton }) => {
28+
> = ({ period, trailingChild }) => {
2929
const { privacyMode, color, amountChange, percentChange } =
3030
useAccountGroupBalanceDisplay(period);
3131
const { formatCurrency, formatPercentWithMinThreshold } = useFormatters();
@@ -61,7 +61,7 @@ const AccountGroupBalanceChangeComponent: React.FC<
6161
{`(${formatPercentWithMinThreshold(percentChange, { signDisplay: 'always' })})`}
6262
</SensitiveText>
6363
</Box>
64-
{portfolioButton()}
64+
{trailingChild()}
6565
</Skeleton>
6666
);
6767
};

‎ui/components/app/rewards/RewardsPointsBalance.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ const RewardsBadge = ({ text }: { text: string }) => {
1111

1212
return (
1313
<Box
14-
className="flex items-center gap-2 px-2 bg-background-muted rounded"
14+
className="flex items-center gap-1 px-1.5 bg-background-muted rounded"
1515
data-testid="rewards-points-balance"
1616
>
1717
<img

0 commit comments

Comments
 (0)