multi-test: Pass in API access to BankKeeper

[ethanfrey]

See TODO: https://github.com/CosmWasm/cosmwasm-plus/blob/15685ab98ad7a6785357ea4341dcb2a3b7999e44/packages/multi-test/src/bank.rs#L143

The ability to validate addresses is in Api and bank should have that as well (and any future keeper as well).

Find an easy way to pass this into BankKeeper as well. We either want to clone Api to provide multiple copies for each keeper (which is not implemented on the trait), or just pass in &dyn Api to each Keeper and hold Box<dyn Api> in the App. You only need &Api as it exposes pure functions, but I am not sure how these references play with lifetimes.

After getting Api into bank, then we can properly validate any input and reject sends to invalid addresses

[hashedone]

The borrow in bank approach would not work with safe Rust - it is typical example of self referential structure. It is possible to do with bunch of unsafe code, but I don't see how it is worth especially in tests. There are some crates hiding such unsafeness, but I didn't find clear winner here on the market and I am not sure about using some not well-known in community crate to thing like this. Also I suspect that at some time we would actually get Polonius, and if we are lucky it would allow for easier self references.

The other idea you proposed is cloning whole API - not bad idea, as it is just single usize, and it looks like it cannot change, so it basically doesn't look bad. My problem with that is that this idea bounds this implementation to an idea, that Api doesn't have its own state. It is true for now, but if in any point in the future there would be an idea to change this, then it would set us in the same spot as today, except it would be harder - it would require refactoring of already existing solution, and I assume tests pool would not shrink over time.

The third way is just to use Rc<dyn Api> instead of Box<dyn Api> - but I don't like this kind of shared ownership. In this context it would work and doesn't even so terrible, but still - I dont like it.

Way I would take is to take Api out of the App (and also WasmKeeper), and keep borrow even inside it. So test initialization would look like:

let api = mock_api();
let app = App::new(&api, block, bank, storage);

The downside is that api can never be moved, so it makes initializing UT in a single function harder - api has to be created outside of such function, and borrow has to be passed to it.

@ethanfrey I need your opinion on those four (basically I am debating between 3rd and 4rd approach).

[ethanfrey]

3 and 4 both seem reasonable.

This just needs to be usable by unit tests, so api outside app seems fine.

let api = mock_api();
let app = mock_app(&api);
// ... use app in tests

is only one more line in setup and quite easy to implement.

The third way is just to use Rc instead of Box - but I don't like this kind of shared ownership. In this context it would work and doesn't even so terrible, but still - I dont like it.

This would actually allow us to share lots of immutable references everywhere, and keep the same setup flow as now and expose it to many keepers. What do you not like about it? If you have a good reason not to, I am happy with (4)

[hashedone]

I just find Rc as a code smell. I know it is not so much, but in many cases Rc means "I don't know how to properly struct ownership in my code" - especially that semantically there is proper owner of an object, it is just difficult to express it in language (for now). My experience is that using Rc at some point spreads, then RefCell all added on top of it to mutate it, and at some point everything becomes shared everywhere without any logic. Such bad experience teach me, to avoid Rc unless there is actually use case of shared ownership on the same level and is basically impossible to find proper owner of an object (and to be honest I never found such case not involving threading, but then Arc is to be used, not Rc).

[ethanfrey]

Fair enough, let's go with case (4) and just pass in &api when creating the App