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

feat: build on modular core-web #67

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

Conversation

okjodom
Copy link
Contributor

@okjodom okjodom commented Oct 4, 2024

  • Declares a Federation class that can be used standalone to connect and interact with the federation
  • FedimintWallet now uses Federation instead of FederationService while still sharing the same WorkerClient instance

- this provides common instantiation and state observation logic for
  when we create a fedimint clint like FedimintWallet
- an instance of this class can be used to interact with the federation,
without any of the other wallet or lightnign modules
- should probably be called `FederationAdmin`?
- Replaces `FederationService` with an instance of `Federation` within
  `FedimintWallet`.
- Showcases how standalone client instances can be composed into a logic
  set like the wallet, while sharing an indemponent `WorkerClient`
instance
Copy link

changeset-bot bot commented Oct 4, 2024

🦋 Changeset detected

Latest commit: 0c0b088

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@fedimint/core-web Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@alexlwn123
Copy link
Collaborator

Hey @okjodom, Thanks for the contribution!

Love the idea to further modularize things.

Few questions:

  • What's the benefit of allowing Federation to be instantiated as a standalone? The services are relatively tiny compared to the wasm instantiation, so I'd expect users to always instantiate the whole FedimintWallet, even if they only need a subset of the features.

  • What's the motivation for the Base class? By default, I much prefer composition over inheritance, so I'd like to avoid javascript's OOP features (abstract classes, interfaces, etc.) unless there's a really good reason

@okjodom
Copy link
Contributor Author

okjodom commented Oct 4, 2024

Hey @alexlwn123

What's the benefit of allowing Federation to be instantiated as a standalone? The services are relatively tiny compared to the wasm instantiation, so I'd expect users to always instantiate the whole FedimintWallet, even if they only need a subset of the features.

I am testing a scenario where I only need FedimintService, and FedimintWallet is not a good abstraction.

In this scenario, I totally could consume FedimintService and write custom logic for instantiating WorkerClient. This gives me more control to manage the worker state, but I am worried of missing out on 'best practices' for when to instantiate the worker, and keep track of the service having an active connection to a federation.

@okjodom
Copy link
Contributor Author

okjodom commented Oct 4, 2024

What's the motivation for the Base class?

This is an attempt at sharing what look like 'best practices' for when to instantiate the worker, and keep track of the service having an active connection to a federation. At a high level, the methods initialize, open, waitForOpen and cleanup state trackers make sense as what I'd need on a live instance of a service or wallet, that can be used with multiple federations, but only with one federation active at a time.

I much prefer composition over inheritance, so I'd like to avoid javascript's OOP features (abstract classes, interfaces, etc.) unless there's a really good reason

Aggreed on preference for composition over inheritance. If initialize, open, waitForOpen and cleanup methods are common enough, maybe we could make them part of WorkerClient, or somewhere else, composable into the services?

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