-
Notifications
You must be signed in to change notification settings - Fork 401
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
Move active campaign concerns to campaign service #3794
Conversation
This makes it easier to manually clear notifications
379c87a
to
77f650b
Compare
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.
Ok did a pass, nothing blocking. Going to test it as part of the parent.
@@ -422,3 +422,7 @@ export const isEnrichedEVMTransactionRequest = ( | |||
transactionRequest: TransactionRequest, | |||
): transactionRequest is EnrichedEVMTransactionRequest => | |||
"annotation" in transactionRequest | |||
|
|||
export const isTransactionWithReceipt = ( |
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.
Should be isConfirmedEVMTransaction
, since that's the type we're asserting.
@@ -37,7 +37,7 @@ export const selectShowingActivityDetail = createSelector( | |||
) | |||
|
|||
export const selectActiveCampaigns = createSelector( | |||
(state: RootState) => state.ui.activeCampaigns, | |||
(state: RootState) => state.ui.campaigns, |
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.
Should above be selectCampaigns
now?
} | ||
|
||
await this.started() | ||
const accounts = await this.chainService.getAccountsToTrack() |
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.
This implies we also need to monitor for new accounts and run this check on those, fwiw.
MEZO_CAMPAIGN.notificationIds.canClaimNFT, | ||
) | ||
|
||
// fetch with uuid |
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.
Where are we fetching this from?
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.
Endpoint on mezo API here: https://github.com/thesis/mezo-portal/pull/1591 👀
type FilterValidCampaigns<T> = T extends { | ||
id: string | ||
data: unknown | ||
/** | ||
* The campaign is disabled if the user is e.g. not eligible | ||
*/ | ||
enabled: boolean | ||
} | ||
? T | ||
: never | ||
|
||
/** | ||
* Campaigns must use the following format | ||
* ```ts | ||
* { | ||
* id: "some-campaign-id" | ||
* data: {...} | ||
* disabled: boolean | ||
* } | ||
* ``` | ||
*/ | ||
export type Campaigns = FilterValidCampaigns<MezoCampaign> | ||
|
||
export type CampaignIds = Campaigns["id"] | ||
|
||
export type FilterCampaignsById<T, K> = T extends { id: K } ? T : never |
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.
Would love to have types like this directly in the service rather than in a types
file under it.
Separately, I think a better pattern for something like this might be:
type Campaign<Id extends string> = {
id: Id
data: unknown
/**
* The campaign is disabled if the user is e.g. not eligible
*/
enabled: boolean
}
const booyan: Campaign<"blap"> = {
id: "blap",
data: "bammer",
enabled: true
}
const booyak: Campaign<"bloop"> = {
id: "bloop",
data: "bammer",
enabled: true
}
const allCampaigns = [booyan, booyak] as const
type Campaigns = typeof campaigns[number]["id"]
This avoids the relatively complex conditional types in favor of interaction with simple types. The downside is if a new campaign isn't explicitly declared as Campaign<...>
, the Campaigns
type collapses to string
.
Anyway, this isn't a blocker.
) | ||
} else { | ||
// CLear up notifications |
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.
Just to check, is the intent here to clear all notifications if the user turns notifications off?
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.
Yes, I added this to dismiss all notifications since their click handlers were detached if notifications were turned off
commit 3b39af1 Author: Jorge Luis <[email protected]> Date: Tue Mar 11 10:00:17 2025 -0500 Rename campaigns selector commit 46214cb Author: Jorge Luis <[email protected]> Date: Tue Mar 11 09:59:58 2025 -0500 Rename type guard commit 1895a67 Author: Jorge Luis <[email protected]> Date: Tue Mar 11 09:57:24 2025 -0500 Change notifications toggler description commit 0c6b995 Author: Jorge Luis <[email protected]> Date: Tue Mar 11 09:48:26 2025 -0500 Remove testnet sorting, use hardcoded order commit 9c6bbd8 Author: Jorge Luis <[email protected]> Date: Tue Mar 11 09:47:45 2025 -0500 Guard safe switch of test network commit 23971ce Author: Jorge Luis <[email protected]> Date: Tue Mar 11 09:36:07 2025 -0500 Update copy commit 646ccf2 Merge: 4671a6e 62a2801 Author: Antonio Salazar Cardozo <[email protected]> Date: Tue Mar 11 08:47:08 2025 -0400 Move active campaign concerns to campaign service (#3794) This moves campaign related logic out of the main service into a dedicated service. It also causes dismissing campaign banners to clear and stop triggering notifications about the same topic. Campaign state is kept within the service db and updates are pushed to state in redux. Latest build: [extension-builds-3794](https://github.com/tahowallet/extension/suites/35331061341/artifacts/2708368343) (as of Fri, 07 Mar 2025 03:08:56 GMT).
This moves campaign related logic out of the main service into a dedicated service. It also causes dismissing campaign banners to clear and stop triggering notifications about the same topic.
Campaign state is kept within the service db and updates are pushed to state in redux.
Latest build: extension-builds-3794 (as of Fri, 07 Mar 2025 03:08:56 GMT).