chore: pass ClientHandleArc to handle_cli_command

[m1sterc001guy]

When developing ROASTR I found myself needing to execute commands against other modules (for example: to get the Bitcoin network of the wallet module). There might be a more elegant way to do this, but one easy way to accomplish this is to pass ClientHandlArc into handle_cli_command so that the cli inside other modules can execute functions from other modules.

[dpc]

When developing ROASTR I found myself needing to execute commands against other modules (for example: to get the Bitcoin network of the wallet module).

The problem is not passing the client as much as just making modules depend on each other, especialy the compile side aspect of it. If we start doing things this way then we will quickly find ourselves hitting circular dependencies between modules.

Modules already get access to the client via https://docs.fedimint.org/fedimint_client/module/struct.ClientContext.html passed to the during construction https://docs.fedimint.org/fedimint_client/module/init/trait.ClientModuleInit.html#tymethod.init .

On the ClientContext we are supposed to add any interactions that we want the Client Module to be able to perform on the Client as a whole. We could, when it make sense, have methods there to get access to some types of other modules etc. though it would require some careful design.

to get the Bitcoin network of the wallet module

The Bitcoin network should probably be a global property of the Federation. I don't think we are ever going to support Federation where each module is on a different network, so we can reasonably say that the network comes with the Federation. So in that case the roast server side module maybe should get passed the network, just like the wallet server module, possibly via https://docs.fedimint.org/fedimint_core/module/struct.ServerModuleInitArgs.html or from the server side config.

@elsirion

[m1sterc001guy]

If we start doing things this way then we will quickly find ourselves hitting circular dependencies between modules.

Perhaps the CLI should always be in a separate crate outside of the module's client crate? The CLI is essentially an integrated wallet/app, it seems natural to me that it should have access to multiple modules.

Agreed the bitcoin network should be a global property.

I just made this PR since this was the solution that unblocked me, but if we want to go with the cleaner route of adding it to ClientContext or making it a separate crate, feel free to close this and we can just make some follow up issues.

[dpc]

Perhaps the CLI should always be in a separate crate outside of the module's client crate? The CLI is essentially an integrated wallet/app, it seems natural to me that it should have access to multiple modules.

That might be a good idea, and it would solve the cyclic deps at compilation time at least.

But how are we going to impl handle_cli on a module, in an extern crate. We would probably need a separate trait, and registration system entirely. And there are still other issues that would make it unworkable outside of a fedimint worktree - e.g. version incompatibilities between modules, which would no longer be independent of each other.

But despite all of that, I still think that in principle if a module needs other modules, than it's not really its own command. IMO we should define interoperability points like this at ClientContext.

So e.g.:

impl ClientContext {
  fn get_primary_module(&self) -> Option<&dyn PrimaryModuleInterface>;
}

would allow any module get ahold of a primary module and mine itself some change etc.

impl ClientContext {
  fn get_bitcoin_network(&self) -> bitcoin::Network;
}

would allow modules get "wallet modules Bitcoin network", if it wasn't that it probably should just be global property.

Generally any interaction that we deemed worthwile and general enough can be done this way, and avoids modules hardwiring dependencies between each other.

I just made this PR since this was the solution that unblocked me, but if we want to go with the cleaner route of adding it to ClientContext or making it a separate crate, feel free to close this and we can just make some follow up issues.

Let's see what @elsirion thinks.

[elsirion]

tl;dr: let's make the network global and I don't see a use case for cross-module dependencies then.

The Bitcoin network should probably be a global property of the Federation. I don't think we are ever going to support Federation where each module is on a different network, so we can reasonably say that the network comes with the Federation.

Originally I wanted to avoid this since it would be awkward in use cases where BTC isn't needed, e.g. issuing toilet access tokens (the raison d'être for SCRIT, a real shitcoin and hugely influential on early Fedimint development). But realistically we will always have Bitcoin as an asset and might just add additional asset support eventually, so this makes sense.

Perhaps the CLI should always be in a separate crate outside of the module's client crate? The CLI is essentially an integrated wallet/app, it seems natural to me that it should have access to multiple modules.

I disagree with that. In general modules are self-contained. They may need money or want to store it, and use the (opaque to them) primary module abstraction for that, but assuming any module is available and thus coupling them is very bad long-term since it makes replacing old versions with new ones much harder and also requires way more context to maintain a module.

I recently chatted with @joschisan about the importance of keeping modules strictly separated to make reasoning about them independently possible and keeping complexity manageable.

[m1sterc001guy]

Made an issue: #5121

[dpc]

Perhaps the CLI should always be in a separate crate outside of the module's client crate? The CLI is essentially an integrated wallet/app, it seems natural to me that it should have access to multiple modules.

I disagree with that. In general modules are self-contained. They may need money or want to store it, and use the (opaque to them) primary module abstraction for that, but assuming any module is available and thus coupling them is very bad long-term since it makes replacing old versions with new ones much harder and also requires way more context to maintain a module.

In defense of it ... . The CLI interactions that do use multiple modules are not entangling the logic of modules themselves with each other. We do that a lot if fedimint-cli already. These are just UX things gluing together module interactions into user-friendly functionality.

Anyway I think the way to go, at least for now is in preference order:

• avoid the need for doing it, and try to make cli handlers only about their own modules
• if there's possibly generic interaction of a module with client that might be a common need, we should express it as ClientContext API
• If in the future it turns out to be really, really needed, maybe we could let modules register "something else", like automation-scripts that would be free to call fedimint-cli with some argument repeatedly and glue it together, or something like that to facilitate cross-module interactions etc. That's very much a stretch, and rough idea.

@m1sterc001guy @elsirion