fedimint icon indicating copy to clipboard operation
fedimint copied to clipboard

Modularization refactoring synchronization plan

Open dpc opened this issue 3 years ago • 8 comments

Since initial feedback on #546 are somewhat positive it's probably a good time to think about how would we implement it. There's no way around the fact that such refactoring is going to be a large scope "stop-the-world" somewhat disruptive change. The best we can hope for is getting over it quickly, minimizing overhead and wasted effort.

Here is the plan (in order):

  • [ ] Get a consensus if something like #546 is what we want to pursue.

    • [ ] The faster you raise concern, the faster we can address it or even consider different approaches
  • [ ] Shuffle the code around to look more like real one, without actually changing any real code yet:

    • [ ] Amend #546 with "output finalization" api. (@dpc needs design help here)
    • [ ] Prepare the desired backend module types.
    • [ ] Move types into roughly right crates.
    • [ ] Implement any desired macros to eliminate boilerplate.
  • [ ] Warn the community that it is going to be a stop-the world change, so they don't start any important large scope work until it's done.

  • [ ] Get a final agreement that all the APIs are looking OK.

  • [ ] Land as many in progress PRs as possible into master, so the author's don't need to redo them.

  • [ ] Stop-the-world and make a big change:

    • No landing anything while this is going on. Rebasic things here is going to be messy, time consuming and make continuous review harder.
    • Gang on the work. Create a video call and work on refactoring together, as people are available.
    • Land commits frequently, to help people keep up with reviewing them, and make handing over work simple.
    • Maintain an append-only commit history, so it's easy to verify what was done, and continue working by just looking at the new commits.
    • Commits don't have to compile. They just have to continuously bring us into the desired destination.
    • End result: all the code is refactored, everything compiles, all tests are passing, and we have a PR with a lot of commits. Eric squashes it and pushes to master.
  • [ ] Resume normal operation.

dpc avatar Sep 12 '22 21:09 dpc

If possible, I think an incremental approach would be best. There is a lot of code that needs to be refactored and reviewed plus a high risk of introducing regressions. An estimate of 2-3 days could easily turn into 2-3 weeks.

We could break up the PRs into tasks like this, regardless of whether we want to stop-the-world or not:

  • Create the framework for modularization of existing enum types and FederationModule
  • Move types one-by-one in separate PRs, making appropriate changes on the federation / client side
  • Move the consensus logic / modules themselves last
  • Break the final dependencies of the "core" code on the modules
  • Refactor boilerplate into macros to make the writing of new modules as easy as possible for new devs

Current we have:

core --> wallet,mint,ln 
server --> core,wallet
client --> core

We need to get to:

wallet,mint,ln --> core 
server --> core
client --> core

jkitman avatar Sep 13 '22 11:09 jkitman

Also I'd like to finish up DKG before the FederationModule and configs get completely refactored since I've already spent a lot of time on it.

jkitman avatar Sep 13 '22 13:09 jkitman

After talking with @dpc I'm more on board with just forking the repo and doing it all at once after we have the traits and crate organization well-defined...probably the easiest way overall, but we have to be ready to freeze the main repo for a couple weeks.

jkitman avatar Sep 13 '22 19:09 jkitman

After talking with @dpc I'm more on board with just forking the repo and doing it all at once after we have the traits and crate organization well-defined...probably the easiest way overall, but we have to be ready to freeze the main repo for a couple weeks.

ACK, I think the current proposal would be to get the Demo out for Oct 1st and land all necessary PRs for that and then do a fork + freeze to get the modularization done. PRs that are done while the freeze is in place will need to either be based off the modularized fork or be rewritten after completion of the modularization (MVP). It seems like incremental rewriting of the whole system will not be feasible, as nearly everything will need to be moved or refactored in some way

Did I get that correct @dpc?

shadowcpy avatar Sep 14 '22 16:09 shadowcpy

I think we can accomplish this in 3 large refactors as outlined here: https://github.com/jkitman/minimint/blob/modularization-architecture/docs/modular-architecture.md#refactoring-plan

This gives us some milestones to work with and the ability to merge to master if unfreezing the world becomes necessary.

jkitman avatar Sep 14 '22 17:09 jkitman

Did I get that correct @dpc?

Yes, though ...

I think we can accomplish this in 3 large refactors as outlined here:

... we will give it a try before attempting a single big refactor.

dpc avatar Sep 15 '22 05:09 dpc

I definitely want to have a framework for modularization landed, even before we start to refactor any code to use it, with crates in roughly destination positions. Then 2 and 3 from https://github.com/jkitman/minimint/blob/modularization-architecture/docs/modular-architecture.md#refactoring-plan seem doable, so instead of having one big world-stop event, we would have two smaller ones: one for server, one for the client.

dpc avatar Sep 15 '22 05:09 dpc

After talking with Eric, the plan is as follows:

  1. Clean up the sketch PR and make it landable. Possibly land.

  2. Move forward with the thing and convert server-side code to use dyn trait from core/server.

  3. Refactor existing client code to use trait (not dynamic yet, but to iron out the API).

  4. Convert client-side code to use dyn trait (based on step 2).

Some quick and messy notes on what we chatted about:

  • Questions about ModuleKey being a self-assigned u16.
  • Move finalize to be a method on the module. Rename it poll_pending_output or something.
  • Preapre static trait(s) to be implemented by module authors, and then either impl DynModuleTrait for T where T: StaticModuleTrait (if possible) or a macro to generate the code to do.
  • The server side module trait can be mostly taken as is and there's some confidence it will work. One idea there is to rely on database transactions and rollback when checking double spends etc.
  • The client side module architecture/interfaces still a bit unclear, but possibly the current model there might work. The model there is to have modules mostly pure, and leave persisting data to the core client. Client would also subscribe to events (websockets) and drive modules to react to them.
  • Simulate linear types / use session types to force persisting data in the database before triggering side effects.

dpc avatar Sep 16 '22 22:09 dpc

Close this? @dpc @elsirion

justinmoon avatar Jan 20 '23 22:01 justinmoon