feat: address monitor and btc tx notifications

[alexp3y]

Try out Leather build 6cb5c9e — Extension build, Test report, Storybook, Chromatic

This PR adds a local extensible monitoring system that can detect BTC Txs involving wallet addresses.

BTC Tx monitoring consists of a single WebSocket connection running in the background service worker using regular stay alive messages to keep the worker running (per Chrome's suggestion).

Confirmed transactions trigger a native browser notification with account and amount information (screenshot).

The list address of addresses are synced from the wallet via internal message handling.

Addresses are cached in local storage and subsequently used to restore the monitor during extension initialization. This allows notifications to resume without requiring users to open and unlock the extension whenever they open a browser window.

A new notification configuration setting could be used to prevent / allow notifications. The address-monitor hooks could selectively return or broadcast an empty array instead of the actual wallet address list, which will close the monitor WebSocket connection and prevent BTC Tx notifications.

[kyranjamie]

Great work @alexp3y

[kyranjamie]

Suggested change

	} as MonitoredAddress;
	} satisfies MonitoredAddress;

?

[kyranjamie]

In hind sight, realise the memo isn't needed here, useSelector is already

[kyranjamie]

While we're moving to a more DI set up in the containers work, and that there's nothing inherently wrong with this solution, I find this hard to agree with given the context that none of the rest of the codebase uses classes and this can so easily be written with a plain fn.

function createAddressMonitor() {
  let monitors: AddressMonitor[] = [];

  return {
    init(addresses: MonitoredAddress[]) {
      monitors = [new BitcoinTransactionMonitor(addresses)];
    },
    sync(addresses: MonitoredAddress[]) {
      monitors.forEach(monitor => monitor.syncAddresses(addresses));
    }
  }
}

If our codebase were class-first, and the function approach were used, I'd think the same the other way around.

What are you thoughts on writing this without a class? And, where do you stand on wider coding style consistency?

[alexp3y]

Happy to rewrite these classes as factory functions, but I think its worth examining exactly what the value of forbidding the class keyword is.

If our codebase were class-first, and the function approach were used, I'd think the same the other way around.

IMO forbidding classes, just like enforcing classes everywhere in a class-first codebase, seems unnecessarily limiting. For example, many languages and codebases benefit from using multiple styles where they make sense - Rust is multi-paradigm out of the box, Java has increasingly introduced more and more FP concepts, etc... One massive strength of Typescript is that it supports both paradigms well and allows us to choose the right tool for the job.

In our codebase we already have cases of stateful API clients and services using factory functions / closures to introduce oop-like encapsulation patterns, and many libraries we use are class-based (including WebSocket). So, ultimately it's more a syntactical preference of not using class than a strict adherence to FP.

The current class-based implementation of the transaction monitor (BitcoinTransactionMonitor) is isolated outside of any functional concern and I would regard it as exactly the right tool for the job - It encapsulates a stateful object, defines its behavior and lifecycle, and explicitly implements the AddressMonitor interface. Converting it to a factory function would only cause us to lose some of that clarity without much benefit.

Anyway my opinion here is that consistency in a codebase is broadly good, but strictly enforcing a single paradigm and forbidding the class keyword specifically, especially as the scope of the project grows and becomes more diverse, actually adds more complexity than it solves for by forcing us to apply functional workarounds to inherently object-oriented problems.

[fbwoolf]

I am curious abt this discussion too. I recently questioned Will's use of a class. I do feel like it seems unnecessary in JS to introduce. For me, it isn't as legible. I don't think of it as being 'forbidden', but I'm not sure I understand why it needs to be used here over the already established pattern using functions? If classes are just syntactic sugar, why use?

[alter-eggo]

agree with Alex here tbh. not sure why we need to limit ourselves and not use classes even if we use fns in other places. for me personally class code provided feels better looking/organised than fn one.

[fbwoolf]

2-2, we need a tiebreaker. 🤣 @pete-watters

[kyranjamie]

Thanks for sharing your thoughts Alex, I think this is an important discussion and shouldn't distract from great work you've pumped out pretty quickly.

Great to see other opinions and I'd love to hear from the rest of the team too cc @tigranpetrossian @edgarkhanzadian @camerow

Making the changes suggested achieves nothing if we don't finish with a clear, shared philosophy on how we write code as a collective.

There are two discussions here: First, the trade offs between function and class in Javascript, and second the importance of consistency within a codebase.

Function vs class: _{Minimising this point. The conversation should focus on consistency, not specifically the class/fn js debate}

There's nothing wrong with classes, Angular, Nest and loads of other frameworks use them by convention. They're mature projects bundled with tonnes of examples and documentation to guide developers how to use them appropriately. I favour writing the function style, because of how it invites code being written, and that's the direction the extension has gone over the last 4 years I've been working on it. Classes invite OO patterns like inheritance that, by and large, I believe we should avoid in favour of composition. You might have the experience to know where not to use inheritance, but using classes tempt others (perhaps we less experience) to (ab)use them. Defining patterns like whether to favour classes or functions, maps the direction our code goes.

Consistency
Writing code as a team is difficult because there are so many ways to solve the same problem. We're an engineering team of 7, and in my experience, if each developers writes code the way they prefer, it's going to be an absolute shit show. Starting with @fbwoolf and I, we've tried to land on a consistent coding style which was easy when it was the two of us, and harder as the team grows. Following JS clean code as a baseline, and branching out to our own coding conventions, the goal has been to shut down some of the core ways coding style diverge. class isn't discussed here, pretty much because it's been out of the question we'd use it. In PRs use of class has been shot down in the past.

That JS and other languages support both patterns doesn't mean much to me personally. It shows that language like to give developers freedom to choose, not implicit permission to mix. Especially in Javascript, there are many things you quite undeniably shouldn't do.

When a developer writes a for loop where a .map() would work, there's nothing wrong with the for loop, but the codebase is more consistent and easier to read if map is used consistently. Developers don't have to jump between syntax, and future solutions are clear. Consistency acts as guidance for new engineers joining the team. Most importantly, the real productivity gain of having a rule is in the PR comment, that avoid a debate on the merits of imperative vs declarative coding styles. Yet, the same arguments that "it's too limiting" can be made.

We're going to build a backend soon. A greenfield project in a different context, serving a different set of functionalities. Fresh eyes, new ideas and different approaches are welcome. That project should set its own constraints. But, this is the extension. Myself, Fara and others have worked hard to keep things consistent (with mixed success). Deviating from this now would be a step back in my opinion. I laugh writing this because it's a kind of "not on my lawn" argument, but there's a context and history here that are important. 5 years into the project not using classes, is it absolutely crucial we use them for the first time today? I don't think so.

Where do we draw the line between giving developers the tools they need, and not having a free-for-all of use whatever language syntax you like? One might call rules limiting, but I'd argue that designing with constraints is a positive thing. I'd like to add way more. Like the discussion with @tigranpetrossian the other day leading to banning enums, we're taking something away, but it gives us more. Do we think we'd be more productive and happier if we removed rules and gave developers more autonomy on style? And, how do we define rules that live leather-wide, and those specific to a more narrow subset (react app, api) etc?

[pete-watters]

Thanks for summarising this 👍 I understand not allowing classes into the extension as there have never been any and I can get behind that.

In mono and when implementing more packages / server-side code we should allow developers more flexibility to to use the solution they feel best. We can always question usages when they crop up in code review if there are better ways.

There is a lot of great code in the extension but also a lot of legacy code to improve on. With our future roadmap, I see us moving more code out of the extension into mono so in that case I don't mind having a stricter set of rules for code added to the extension.

Overall I think code quality and consistency is important but it's just one aspect of a great product and not as impactful to our end users as what we could achieve allowing some more flexibility to coding style

[alexp3y]

Happy to drum up debate (as always) :) Agree with the points raised by @fbwoolf and @kyranjamie regarding the importance of context, history, and consistency in extension. The functional conventions here are well-established.

I updated the monitor classes here using factory functions and extracted some logic to pure functions as suggested.

[tigranpetrossian]

I have no strong opinions on isolated cases of class implementations, especially for library-level code, or various agent/observer/orchestrator implementations, but if the team finds that it will increase the cognitive load when it comes to both making decisions on which one to pick, and reading the code, I agree that we could stick to factory functions instead with little to no downsides.

Some more broad points about classes, unrelated to @alexp3y's implementation.

1. Combination of state & behavior is usually a path to a lot of frustration. It doesn't have to be the case, but classes by design almost encourage this.
2. Inheritance: without going into much detail, this isn't something we would want to do. I doubt anyone in our team was going to anyway.
3. Classes can be significantly more performant that factory functions. This really only shines when large amount of instantiations happen at once--classes are way more optimized by engines compared to closures. I've seen very valid implementations of stateful class instances passed as React props for performance reasons. This isn't something you encounter every day, but useful to keep in mind.

[camerow]

Personally I like classes and the consistent encapsulation / mental model of a class.
That said I also think codebase consistency > any one persons opinion. This is a really healthy discussion and it looks like we landed on keeping alive the historical pattern established which makes sense for the team. I don't see any dogmatic disdain for classes here, rather just a push to give ourselves some constraints for our own sanity.

[kyranjamie]

Could the websocket keep alive functionality be composed separately to the inbound tx handling?

As in, could we have a generic MempoolSpace websocket client responsible for handling the events & keep alive functionality, but the behaviour responding to the these events lives elsewhere.

something like, which separates the concerns of the websocket connection & the app behaviour

const client = createMempoolSpaceWebsocket({ 
  onTrackedAddressEvent(event) {
    // sendNotification()
  }  
});

onAnotherAddressToTrack(addr => client.trackAddress(addr))

[alexp3y]

Definitely can and likely should move toward something like that, but would recommend we wait until we have an additional use case for the MempoolSpace WS to do so with more perspective (and proper prioritization).

[kyranjamie]

⚠️ magic number 2 🤷🏼‍♂️

[kyranjamie]

Could this be exacted in a pure fn? Seems like logic we might reuse?

[alexp3y]

@markmhendrickson or @314159265359879 do you want to QA this before we merge?

[alter-eggo]

great work @alexp3y

[kyranjamie]

Nice work @alexp3y