Use builder pattern for App init

[ethanfrey]

Extending #387

With the generic modules, we no longer have generic initialisation functions (like setting the balance).

I propose the following builder pattern.

Router::new() // this will set with BankKeeper, WasmKeeper, and the rest unimplemented
.with_staking(stakingKeeper) // so we can extend these later
.build_app() // this will create an app with MockApi and MockStorage

The main question I have is how to call the setup/init methods on custom keepers that go beyond the default module types (eg. init_balance). Here are some examples:

    // Add a new contract. Must be done on the base object, when no contracts running
    fn store_code(&mut self, code: Box<dyn Contract<C>>) -> usize;

    // Admin interface
    fn init_balance(
        &self,
        storage: &mut dyn Storage,
        account: &Addr,
        amount: Vec<Coin>,
    ) -> Result<(), String>;

We can either just set these on the router when building (and then pass the storage into the build_app constructor:

let mut router = Router::new();
let mut storage = MockStorage::new();
router.bank.init_balance(&mut storage, &owner, coins(1000, "etc"));
let mut app = router.build_app(storage);

Or we can expose these with some closure in the app.

let mut app = Router::new().build_app();
app.setup(|router: &Router, storage: &mut dyn Storage| {
  router.bank.init_balance(storage, &owner, coins(1000, "etc"));
  router.bank.init_balance(storage, &trader, coins(10, "btc"));
})

In either case, we must make the different keepers public on the router interface. I lean towards the second one.

[hashedone]

Why not to just build Router dependencies separately and inject them?:

let storage = StorageBuilder::new()
  .with_item(...)
  .build()
  .unwrap();
let bank = BankBuilder::new()
  .with_balance(&mut storage, &owner, coins(1000, "etc"))
  .build()
  .unwap();
let router = RouterBuilder::new(bank)
  .some_router_specific_configuration(...)
  .build()
  .unwrap();
let app = App::new(router, storage);

I possibly made some unreasonable things with Api right here as I didn't work with code, but basically demonstrated the idea.

[ethanfrey]

I think this is much more complex than needed. As they don't own the storage. But let's talk more later.

[hashedone]

What I don't like is exposing internals, or being able to call configuration function after setup is complete. But maybe just using actual idea about passing arguments to closure is not this bad:

let mut app = Router::new().build_app(|bank: &BankKeeper, storage: &mut dyn Storage);
app.setup(|router: &Router, storage: &mut dyn Storage| {
  bank.init_balance(storage, &owner, coins(1000, "etc"));
  bank.init_balance(storage, &trader, coins(10, "btc"));
})

[ethanfrey]

Yeah, nice idea. How about that setup call can only be done as part of the build step? When we go from (stateless) Router to (stateful) App.

let mut app = Router::new()
  .with_staking(stakingKeeper)
  .setup(|router: &Router, storage: &mut dyn Storage| {
    router.bank.init_balance(storage, &owner, coins(1000, "etc"));
    router.bank.init_balance(storage, &trader, coins(10, "btc"));

    router.staking.set_validator_weight(storage, val1, 1000);
  });

[ethanfrey]

So, we can call all the generic helpers only at the initial phase.
Rather than now, where we can call init_balance anytime.

[hashedone]

I basically spend too much time on this without reasonable proposal. It turns out, that with current multitest design this approach is in my opinion pointless - it is up to test developer to properly structure test and just not intersperse test logic with setup. The problem is that, in one hand, we don't want to create app unless setup is completed (as if app is created it is no reason to use any "setup" functionality), but on the other hand, some tests requires app to be exsisting for proper setup (in cw3-flex tests funds are initialized on fully initialized contract).

So instead of overcomplicating setup I propose just add mocks for all components, and probably default App constructor so all those mocks doesn't need to be passed by hand.

[maurolacy]

I'm now reviewing this. Looks good (the builder pattern for App looks great) but, I cannot take the decision to introduce a crate for mocking the custom message handler.

I would say, let's wait for @ethanfrey 's take on this.

[hashedone]

In terms of mockall - as I told in call - I like generality and configurability of generated mocks. I don't think adding additional dependency for testing is an issue. However I understand that it might be an issue that using it forces one to get familiar with aditional crate, but actually any way of mocking introduces an API. Anyway - I understand concerns.