Removing dyn from Router

[hashedone]

Closes #404

This is basically first iteration, however it makes maintaining App difficult - it requires almost always providing types for router/app which makes it unusable. I have ideas how to work it around, but still in progress.

Also I would like to remove dyn from entire App in the process (from internals, not interfaces) for consistency.

As a minor upgrade I want to include here is to provide helper proxy traits to make bounds way simpler (and don't have everywhere repeated the same compliacted bounds).

[ethanfrey]

First pass on this.

My impression is that you spent quite some time fighting with the Rust compiler, but the types do seem to fit together somehow.

I raised a few points about stronger connection between the WasmHandler and the CustomHandler types when building, as these are required to be aligned. Otherwise, it looks good to me.

The biggest open question is how this affects usage in the contracts. Hopefully, we don't need to define 10 generics to use it and can just eg. use BasicApp or such. It seems the contracts were not updated yet, but that would be my only other take... to ensure the API still makes sense (is easy to work with) when using it. That probably could be fixed with a few helper types/functions that make more assumptions for you (as you seem to have started doing)

[ethanfrey]

Huh, I have seen such in eg. Iterator, but I am not sure how they are different in practice than generic traits.

This is needed for the Sudoku?

[hashedone]

So difference is on the one hand subtle, on the other - fundamental.

When you have a generic trait, then it basically means, that this trait can be implemented on type multiple times, for different generics argument. So if CustomHandler is generic trait, you can make your very same type to be handler for multiple types. But what it also means is, that when you call a function on this trait, the if the function doesn't have all its generics in signature, type deduction my fail. Consider:

// Assume MyCustomHandler implements:
// * `CustomHandler<Empty, Empty>`
// * `CustomHandler<CustomMsg, Empty>`
let handler = MyCustomHandler::new();
// Now: which to call? Both `CustomHandler` implementations are valid here
handler.query(&api, &storage, &block, Empty);

And the problem is even worse than hinting for type in normal types - as when for types you basically typehint once and the type is known for whole context, for traits it is not true. Even if here you call query from particular implementation, it doesn't mean that in next line, you don't want to call the other query - therefore you need to typehint all your calls and it looks like: CustomHandler::<Empty, _>::query(&handler, &storage, &block, Empty).

On the other hand, associated types are induced from trait - so there is always one trait implementation for type, and types are delivered by this implementation. It is more limiting, but also way simpler for typesystem - types don't need to be ever elided. You see I often put type bounds on those types (ExecT = ExecT), but what it means is "if you already check that CustomHandler is implemented, make sure, that it is the same that other generic type" (which can also help type elision in case ExecT cannot be elided from other context, which is basically often case here).

Perfect example of difference between two approaches are AsRef and Deref traits which both give returns a borrow to underlying data, but they are different - AsRef is generic, Deref has associated Deref::Target. And they differ on semantics - AsRef means, that type can be used in place of borrow to other type (which makes sens to have multiple implementations - String can be used both as &String and &str, and there are more complex scenario, when type can have even more AsRef impls). Derefis designing for dereferencing and it means that it always may be dereferenced to single type, and it never makes sense to have multiple implementation of it. This is a reason, why you always can use*on smart pointer (which uses deref internally), but to use.as_ref` you always have to specify type you are wan to convert to (either directly by type hint, or by using in on unequivocal context).

Now an ancient question - when to use generic traits and when to use associated types? I often see rule of thumb: use generics unless they make you troubles, try associated when generics doesn't work. It is based on idea, that generics are somehow easier to think about I think, but I don't like this rule. My rule is: use generics if it makes sense to implement this trait with different types, otherwise use associated types to avoid type elision problems. Either way - in this very case, the generic implementation I took on first take proofed itself being problematic - because if in your whole test you chose to use only queries or execs, then type elision went crazy and spam of type hints were needed.

[ethanfrey]

Thanks for the explanation

[ethanfrey]

Rust question:

Why we need them both here when ExecC is fixed by codes type already?

[hashedone]

We don't overlook (I changed this type like all the times in the universe).

[ethanfrey]

Eyes starting to hurt....

You are a true soldier to make it to the end 🎖️

[hashedone]

This actually can be simplified - even if simply introducing marker trait:

trait CustomExecMsg: Clone + fmt::Debug + PartialEq + JsonSchema + DeserializeOwned + 'static {}

impl<C> CustomExecMsg for C where C: Clone + fmt::Debug + PartialEq + JsonSchema + DeserializeOwned + 'static {}

And then it can be used in all those long bounds. Also it is a trick for reducing all those app-traits to one, so instead of:

where
  BankT: Bank,
  CustomT: CustomHandler<ExecC = ExecC, QueryC = QueryC>,
  WasmT: Wasm<ExecC, QueryC>,

you would have:

where
  Self: App<BankT, CustomT, WasmT>

By extracting everything from impl App to separate trait App (and renaming App to some AppImpl) - bounds would be only in place of this trait impl. Same could be done for router (which is often bound in Wasm).

However I didn't want to include it in this review - it is big enough, and I didn't want to risk ruining something with those things.

[ethanfrey]

Huh.
I don't like so much disconnecting the Wasm<C::ExecC, C::QueryC> from they types used in CustomT (which is already defined).

So, you need to specify the proper (final) ExecC/QueryC when creating the (empty) builder, then can only install a new WasmKeeper that uses the same custom types.

That doesn't seem to be enforced here. But maybe I am just not following this.

[hashedone]

Basically the problem is, that you have to disconnect those types here. If you wont, then how would you even build the app for non-standard custom msgs? If you set custom handler first, it fails - your new custom handler types doesn't match wasm types. Same if you set wasm first, the again, you are in the same place (unless wasm can handle both types which is true for probably most cases, but in some edge it may not - at the and wasm may be substituted with custom one). The relation between those types is defined in build, as then both components have to be final.

[ethanfrey]

Let's discuss this in a future issue. It is good (and possibly complete correct) as it is. Maybe I try a small PR later on to address this, but definitely not a blocker to merge.

[ethanfrey]

Same as above. There is an implicit connection between the ExecC/QueryC types in Query and Wasm and it would be great if the builder enforced they are compatible.

It seems this will then cause some issue later on when trying to use the resulting app that some method is not available (as impl requires a match). But ideally the compile error would appear on the line that caused the issue (this one when setting an incompatible Custom handler)

[hashedone]

And as before, it is in build by bound WasmT: Wasm<CustomT::ExecC, CustomT::QueryC>.

[ethanfrey]

Does this also close CosmWasm/cw-multi-test#3 in the refactor?
Or is that a separate issue still?

[ethanfrey]

I checked this out and ran the contract tests that use multi-test and was amazed they worked without changes.

Such an overhaul of types and it didn't break the typical API usage. 👍

[hashedone]

I don't think it closes CosmWasm/cw-multi-test#3 - I didn't checked tests. And I am not sure if all tests would not need change (I think some if not all contracts are relating to multitest via cargo, not path), but I am sure there should not be drastic changes. App is still the same type (thanks to default - unless you use custom messages, when you should switch to BasicApp, but not the case for tests we already have). One place where things may have is building App, but it is minimal change (and basically comes to change mock_app implementation to App::new()).

[ethanfrey]

Good work.

Please rebase and merge it in.