Cleanup naming in module types.

[dpc]

• [ ] ModuleInit -> ModuleConstructor (or ModuleGen?)
• [ ] Find&replace all ConfigGen and config_gen to match.
• Consider I prefix in all module traits that we use in Arc<dyn Trait> and alikes. Just for consistency.
• Probably some other stuff. Go around and think about it.

[justinmoon]

I find the name ServerModulePlugin kind of weird. Plugin and module in my mind mean roughly the same thing. So seeing both next to each other is odd.

[dpc]

Yeap. This was a temporary meassure as we have:

struct Foo; // the dyn newtype wrapper

trait Foo; // type erased module trait

trait FooPlugin; // version of `Foo` with associated types that auto-impls to `Foo`

Better name suggestions appreciated.

[dpc]

@elsirion Maybe ModuleInit should be named ModuleKind or ModuleType. That's kind of a design that is build in my head.

The binary app has to just list all the module "types" it supports (ModuleKindRegistry). Then the federation members build the actual list of module instances with its configuration, where each instance is of type from the supported kinds.

[mxz42]

Not sure if it directly refers to this, but it's about naming, too:

I think as someone who wants to implement a module, it would "sound" and "feel right" to just write

impl ServerModule for MyGreatNewServerModule

naming convention may look like:

trait IServerModule;  // interface
struct DynIServerModule<Arc<dyn IServerModule>>;  // dyn new type
trait ServerModule; //  trait with associated types implemented by modules

---

I find the name ServerModulePlugin kind of weird. Plugin and module in my mind mean roughly the same thing.

I agree. With less work one could just rename ServerModulePlugin to ServerModuleTyped, though, that would carry the "unnecessary" information Typed for an implementor.

[elsirion]

@dpc Instead of a ModuleKindRegistry we could have a FedimintdBuilder and a DistributedgenBuilder to which these ModuleKind types could be mounted to respectively. When calling build() these would return an object on which one can call run() and that's all that has to happen in a custom federation main.rs.

fedimintd

#[tokio::main]
fn main() {
    FedimintServer::builder()
        .datadir("/var/lib/fedimintd")
        .with_module::<WalletInit>()
        .with_module::<MintInit>()
        .with_module::<LightningInit>()
        .build()
        .run()
        .await
}

distributedgen

#[tokio::main]
fn main() {
    …
    Distributedgen::builder()
        .config_dir("/var/lib/fedimintd")
        .with_module::<WalletInit>(wallet_params)
        .with_module::<MintInit>(mint_params)
        .with_module::<LightningInit>(ln_params)
        .build()
        .run()
        .await
}

[dpc]

@elsirion Ack on the builder, though I'm not sure about the composition of the .run and the UI and other misc code in the main() function now. We'll see what we can do.

@mxz42

I think I prefix for dyn-trait is easy to agree on. It's barely visible across the code anyway.

The dyn newtype is used all across the generic code, so it would be great if it had a neat short name. But if we put 3rd party developer DX first-most, maybe typed traits deserve the shortest name.

So then ... how to name the dyn-newtype (currently struct ServerModule(Arc<IServerModule>);)?

Either DynServerModule, or we could have both of them named ServerModule and just rely on module qualificator when there's a conflict. Not exactly unusual in the Rust ecosystem (how many Results there are). Plugin writers will only care about the typed-trait, while the generic code usually only cares about the dyn-newtype. Client code still sometimes touches both, but that should be minimized in the future, and they can prefix with fedimint_wallet::ServerModule to avoid conflicts and job done.

(I BTW. like the keeping the module name everywhere (so I try not to use anyhow::Result and just type anyhow::Result everywhere. In Go that's a requirement, BTW. One imports modules and needs to qualifies symbols by module all over the code. I think it helps readability)

@elsirion Thoughts?

[elsirion]

I like the Dyn* naming convention, it makes it easier to see that the type is a type-erased version of something else (I did initially call the Box<dyn …> types Any*, which seemed to be confusing, Dyn* is the more correct prefix it seems).

Letting the base trait have the shortest name seems like a nice benefit too.

[dpc]

@elsirion All right. I think I tried for too long to avoid the Dyn on a newtype, and it might be just a mistake.

@mxz42 Do you think you could do all the renames and other cleanups? It seems like you're on to the modularization already.

[mxz42]

@mxz42 Do you think you could do all the renames and other cleanups? It seems like you're on to the modularization already.

Yes, trying to understand it. :+1: Ok, I'll start renaming the symbols in my comment.

I haven't had a close look at ModuleInit or ConfigGen, so I'll likely need some hints there

[dpc]

I haven't had a close look at ModuleInit or ConfigGen, so I'll likely need some hints there

They are like all other types, methinks. Hit me on Discord with any questions.

[mxz42]

while creating #1292 I removed the Module in Lightning* symbols because I think it's redundant. The only place it's left now it the LightningModuleDecoder because currently it is consistent with the other modules such as MintModuleDecoder and WalletModuleDecoder.

might be worth to change it for all modules to MintDecoder etc.

thoughts?

[dpc]

@mxz42 :+1: from me.

BTW. I think I still see Plugin in associated types. Might want to go for these next.

[mxz42]

BTW. I think I still see Plugin in associated types. Might want to go for these next.

yes, good point. will do

[mxz42]

Another thought occurred while doing the renaming: all the Plugin* prefixes were a nice sign that this was the typed interface. That's now more or less lost. I guess another option would be to name them something like

trait IModInput; // interface module input
struct DynModInput; // new type
trait ModInput;  // typed trait. -> these would start with `Mod*`

[dpc]

Another thought occurred while doing the renaming: all the Plugin* prefixes were a nice sign that this was the typed interface.

Yes, but they were also confusing the user with their meaning.

I is erased-trait, Dyn is dyn newtype wrapping I, lack of prefix is just what it is ... what the user (module developer) has to interact with.

[justinmoon]

@elsirion Ack on the builder, though I'm not sure about the composition of the .run and the UI and other misc code in the main() function now. We'll see what we can do.

Eventually the admin UI will probably be a single page app that communicates via API vs server-side templated app it is today. That might work something like:

#[tokio::main]
fn main() {
    FedimintServer::builder()
        .datadir("/var/lib/fedimintd")
        .with_ui(Some("/path/to/ui/build/folder"))
        ...
}

If path is provided, fedimintd will serve those files. Otherwise, static UI files will need to be served elsewhere (e.g. by nginx). Calling with_ui could turn on all admin-related HTTP endpoints in the "core" (e.g. have we run dkg yet? which of my peers are offline?) and in the modules.

[elsirion]

If path is provided, fedimintd will serve those files. Otherwise, static UI files will need to be served elsewhere (e.g. by nginx). Calling with_ui could turn on all admin-related HTTP endpoints in the "core" (e.g. have we run dkg yet? which of my peers are offline?) and in the modules.

We could also statically bundle HTML+JS and allow modules to inject their own JS. But your idea is probably more practical in the near term.

One nit: .with_ui(Some("/path/to/ui/build/folder")) should be .with_ui("/path/to/ui/build/folder"), simply not calling it would be equal to None.

[justinmoon]

One nit: .with_ui(Some("/path/to/ui/build/folder")) should be .with_ui("/path/to/ui/build/folder"), simply not calling it would be equal to None.

I was thinking that calling it with None would enable the admin API endpoints (which paranoid users might want to disable) but not attempt to serve any static files.

[elsirion]

Bike shedding: just make it two separate functions, where calling one without the other panics on build. Or you could be fancy and use type-level boolean algebra to determine which combinations are legal.

[justinmoon]

Closing this due to inactivity