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

Mezoification: Add base state and UI elements #3785

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

hyphenized
Copy link
Collaborator

@hyphenized hyphenized commented Feb 21, 2025

Adds base state, notification handlers and banner elements for upcoming campaign...

Banners and notifications should change based on the state of the campaign.

To Test

  • With SUPPORT_MEZO_NETWORK=true open the wallet, after a minute, a banner should appear
  • Clicking the banner should prompt for push notification permissions
  • If push notifications are enabled a notification should appear

Latest build: extension-builds-3785 (as of Thu, 06 Mar 2025 12:56:36 GMT).

@hyphenized hyphenized force-pushed the campaign-1 branch 3 times, most recently from 9d1947a to 4425ed7 Compare February 21, 2025 18:35
Base automatically changed from mezo-network to main February 28, 2025 12:54
This will control visibility and behaviour of banners and other UI elements
This sets a recurrent alarm to hit the API and check claim state unless the
wallet is not eligible or the campaign has already been finished. If eligible,
users receive a notification based on their claim completion.
Transactions must have been completed and target the borrower contract
on the mezo network.
This is used by posthog to distinguish users
- onDismiss is no longer passed as an options to the extension API
- Fixed a race condition in NotificationService state initialization
- Fixed the notifications settings toggler

As MV3 requires permissions to be requested during a user action,
the prompt for user permission no longer works if triggered through
a redux thunk that executed in the background.
Sets a visibility assertion to confirm element exists before synchronously
asserting value
@hyphenized hyphenized self-assigned this Feb 28, 2025
@hyphenized hyphenized marked this pull request as ready for review February 28, 2025 19:27
Exclusively used for newly added builtin networks
@Shadowfiend
Copy link
Contributor

Screenshot 2025-03-03 at 15 42 07

Alignment of the open-in-new-tab icon seems off; it also doesn't highlight on hover.

Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

Left a few initial thoughts, haven't made it all the way through yet.

@@ -47,6 +54,13 @@ export type UIState = {
routeHistoryEntries?: Partial<Location>[]
slippageTolerance: number
accountSignerSettings: AccountSignerSettings[]
activeCampaigns: {
"mezo-claim"?: {
dateFrom: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Guessing if we make this Date it won't (de)serialize?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, it will remain a string

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should give our serializer/deserializer the ability to deal with dates tbh (not as a blocker though).

@@ -134,6 +137,15 @@ export default class AnalyticsService extends BaseService<Events> {
await super.internalStopService()
}

get analyticsUUID() {
Copy link
Contributor

Choose a reason for hiding this comment

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

From below it looks like we can lose this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you're referring to the error below, that's just to block access until service has initialized. This could happen during tests or if there's an attempt to get this data from within another service's initialization without awaiting service.started()

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I meant that it doesn't look like we're accessing this as a property anywhere—which is good, as leaking the UUID feels like leaking the private state of the analytics service and leaking the implementation detail of using PostHog.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm I'm not following, we need this id to get the wallet's campaign state 🤔

Comment on lines +1726 to +1856
) {
return
}

const shownItems = new Set(
await this.preferenceService.getShownDismissableItems(),
)

// const uuid = this.analyticsService.analyticsUUID
const hasSeenEligibilityPush = shownItems.has("mezo-eligible-notification")
const hasSeenBorrowPush = shownItems.has("mezo-borrow-notification")
const hasSeenNFTNotification = shownItems.has("mezo-nft-notification")

// fetch with uuid
const campaignData = {
dateFrom: "2025-02-21",
dateTo: "2025-03-28",
state: "eligible" as MezoClaimStatus,
}

const isActiveCampaign = dayjs().isBetween(
campaignData.dateFrom,
campaignData.dateTo,
"day",
"[]",
)

if (
isActiveCampaign &&
campaignData.state === "eligible" &&
!hasSeenEligibilityPush
) {
this.notificationsService.notify({
options: {
title:
"Enjoy 20,000 sats on Mezo testnet. Try borrow for an exclusive Mezo NFT!",
message: "Login to Mezo to claim",
onDismiss: () =>
this.preferenceService.markDismissableItemAsShown(
"mezo-eligible-notification",
),
},
callback: () => {
browser.tabs.create({ url: "https://mezo.org/matsnet" })
this.preferenceService.markDismissableItemAsShown(
"mezo-eligible-notification",
)
},
})
}

if (
isActiveCampaign &&
campaignData.state === "claimed-sats" &&
!hasSeenBorrowPush
) {
this.notificationsService.notify({
options: {
title: "Borrow mUSD with testnet sats for an exclusive Mezo NFT!",
message: "Click to borrow mUSD ",
onDismiss: () =>
this.preferenceService.markDismissableItemAsShown(
"mezo-borrow-notification",
),
},
callback: () => {
browser.tabs.create({ url: "https://mezo.org/matsnet/borrow" })
this.preferenceService.markDismissableItemAsShown(
"mezo-borrow-notification",
)
},
})
}

if (
isActiveCampaign &&
campaignData.state === "borrowed" &&
!hasSeenNFTNotification
) {
this.notificationsService.notify({
options: {
title:
"Spend testnet mUSD in the Mezo store for an exclusive Mezo NFT!",
message: "Click to visit the Mezo Store",
onDismiss: () =>
this.preferenceService.markDismissableItemAsShown(
"mezo-borrow-notification",
),
},
callback: () => {
browser.tabs.create({ url: "https://mezo.org/matsnet/borrow" })
this.preferenceService.markDismissableItemAsShown(
"mezo-borrow-notification",
)
},
})
}

this.store.dispatch(updateCampaignState(["mezo-claim", campaignData]))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Fine for our quick implementation here, but we really need to move this either into redux itself or out to a service—the ReduxService should be doing the minimum possible to coordinate between the two. More like what the island service hookup at https://github.com/tahowallet/extension/blob/main/background/services/redux/index.ts#L1567-L1569 does.

Need to do some deeper arch work to figure that out before we can clean up since these are more complex interactions though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for giving this a look, wasn't confident enough to put this into it's own service without receiving some feedback on the approach first 🕵️

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a few ways in my head we should factor it out:

  • Mezo service that handles Mezo-related things including these kinds of campaigns.
  • Notification service is directly responsible for stuff like this (not how we originally envisioned it, but may make sense).
  • Campaigns service to handle these kinds of campaigns, Mezo service would handle more network-level/API stuff like mats, Passport, etc.

Moves test network visibility user setting to preference service and sets force toggles it to display on all new and existing installs

type TopMenuProtocolListProps = {
onProtocolChange: (network: EVMNetwork) => void
}

/**
* Places Ethereum and Mezo network above other networks
* Places Mezo network above other networks
Copy link
Contributor

Choose a reason for hiding this comment

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

Ngl I would love to kill this sort function. Its main purpose is to boost matsnet in the list if people have previously added it, is that right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup that's pretty much all it does. Currently testnets are hardcoded so we could remove it and use the hardcoded order but that seems a bit dirty 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it work even if the user had previously added it as a custom network? If so, then let's rely on the hardcoded order for now.

isSelected={sameNetwork(currentNetwork, network)}
key={network.name}
network={network}
info={testNetworkInfo[network.chainID]}
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is descriptively named info lol—is this the description that lives under the network name? If so maybe let's rename to description (not a blocker).

"function openTrove(uint256 _maxFeePercentage, uint256 debtAmount, uint256 _assetAmount, address _upperHint, address _lowerHint)",
])

// eslint-disable-next-line import/prefer-default-export
Copy link
Contributor

Choose a reason for hiding this comment

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

Lint disable is best accompanied by an explanation.

In this case my thought is that we probably move this into a Mezo or Mezo campaign service, which is where we deal with the Redux service's complexity as well (after we ship this, I mean). Wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point, this is only used for the campaign. I initially put this under /lib because I thought this would be also used by the enrichment service.

Copy link
Contributor

Choose a reason for hiding this comment

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

For sure. I think once we're enriching we'll probably export more anyway—and I wonder if the better approach here is actually making this file an ABI export file (background/lib/mezo/musd-trovemanager-abi.ts or so) and then we implement checkIsBorrowingTx directly where we need it and let enrichment do its own things with the ABI.

Anyway, none of that is blocking.

This PR moves the testnet visibility setting to the preference service,
and sets a migration to enable it on all existing and new installs.

## Testing
- [ ] On a new install, check testnets shows up by default on network
switcher
- [ ] On existing install, after update testnets should show up

Latest build:
[extension-builds-3793](https://github.com/tahowallet/extension/suites/35160394180/artifacts/2686423977)
(as of Tue, 04 Mar 2025 05:05:23 GMT).
Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

Another round, still cookin'.

@@ -222,6 +237,22 @@ const uiSlice = createSlice({
...state,
settings: { ...state.settings, autoLockInterval: payload },
}),
updateCampaignState: <T extends keyof UIState["activeCampaigns"]>(
Copy link
Contributor

Choose a reason for hiding this comment

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

When we start pulling typeofs like this it's usually a sign to me that we should name the type, fwiw.

@@ -47,6 +54,13 @@ export type UIState = {
routeHistoryEntries?: Partial<Location>[]
slippageTolerance: number
accountSignerSettings: AccountSignerSettings[]
activeCampaigns: {
"mezo-claim"?: {
dateFrom: string
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should give our serializer/deserializer the ability to deal with dates tbh (not as a blocker though).

@@ -134,6 +137,15 @@ export default class AnalyticsService extends BaseService<Events> {
await super.internalStopService()
}

get analyticsUUID() {
Copy link
Contributor

Choose a reason for hiding this comment

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

No, I meant that it doesn't look like we're accessing this as a property anywhere—which is good, as leaking the UUID feels like leaking the private state of the analytics service and leaking the implementation detail of using PostHog.

@@ -277,18 +276,10 @@ export default class PreferenceService extends BaseService<Events> {
}

async setShouldShowNotifications(shouldShowNotifications: boolean) {
if (shouldShowNotifications) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would love to still make it so that this can only set the db state if the permission is actually granted, rather than having it be a blind state updater. i.e., it would be good if we could guard against setShouldShowNotifications(true) successfully updating the db if in fact the user denied the request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm we can verify if permission has indeed been granted and guard that I think

shouldShowNotifications,
)
this.store.dispatch(toggleNotifications(isPermissionGranted))
await this.preferenceService.setShouldShowNotifications(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is currently hooked up to the Show Subscape notifications settings toggle. Two problems I think:

  • By removing the permissions.request from the service, we ensure that turning that toggle on will not work, as there's no permission request prompt on the click path.
  • We should probably rename that setting to Show Mezo notifications.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I understand, I had to move permissions.request to the UI because in mv3 permission requests have to trigger in response to a user interaction and that detail seems to get lost on the way to the service worker 😕

@@ -550,7 +550,8 @@
"l2": "L2 scaling solution",
"compatibleChain": "EVM-compatible blockchain",
"avalanche": "Mainnet C-Chain",
"connected": "Connected"
"connected": "Connected",
"featuredNetwork": "new"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it featured or new? 😉

@Shadowfiend
Copy link
Contributor

Screenshot 2025-03-03 at 15 42 07 Alignment of the open-in-new-tab icon seems off; it also doesn't highlight on hover.

Want to bring this back to the fore, I think this is a blocker.

Also, I think if you hit the Login to Claim link, that should dismiss the notification—wdyt?

@hyphenized
Copy link
Collaborator Author

Screenshot 2025-03-03 at 15 42 07 Alignment of the open-in-new-tab icon seems off; it also doesn't highlight on hover.

Want to bring this back to the fore, I think this is a blocker.

Ok this should be fixed now

Also, I think if you hit the Login to Claim link, that should dismiss the notification—wdyt?

Good catch! That should also prevent additional "login to claim" notifications from appearing right?

@Shadowfiend
Copy link
Contributor

Yeah should be a full dismissal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants