Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify router args #422

Merged
merged 7 commits into from
Sep 19, 2021
Merged

Simplify router args #422

merged 7 commits into from
Sep 19, 2021

Conversation

ethanfrey
Copy link
Member

I tried to simplify some of these complex args and self types.

But I ended up with:

error[E0038]: the trait `CosmosExecutor` cannot be made into an object
   --> packages/multi-test/src/wasm.rs:684:17
    |
684 |         router: &dyn CosmosExecutor<ExecC = ExecC>,
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `CosmosExecutor` cannot be made into an object
    |
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
   --> packages/multi-test/src/app.rs:498:38
    |
497 | pub trait CosmosExecutor {
    |           -------------- this trait cannot be made into an object...
498 |     type ExecC: Clone + fmt::Debug + PartialEq + JsonSchema;
    |                                      ^^^^^^^^^ ...because it uses `Self` as a type parameter

I don't quite follow why it is not proper. Maybe you tried this path already and failed. Maybe you know how to fix it.

I think it will make Wasm cleaner as we add more types into Router (like Staking, etc). But maybe this approach is not possible.

@ethanfrey ethanfrey requested a review from hashedone September 14, 2021 20:16
@ethanfrey
Copy link
Member Author

If you have a solution or idea, please just push it on this branch

@ethanfrey
Copy link
Member Author

Update: this is compiling and passing tests.
Usage of the Router in wasm.rs is much simpler.

The biggest issue encountered was that struct CosmosMsg<T> requires T: Clone + Debug + PartialEq + JsonSchema + ... and it seems the PartialEq could not be used as a bound on the trait attribute/type. Ugh.

I demo a solution, I made a fake CosmosMsg, where the struct doesn't have any bounds on T (just the implementations). This solved the issue. We really should upstream this so as not to have multiple CosmosMsg types floating around.

Other than that, I think this does make usage of Router nicer and wasm.rs simpler. I would like some feedback

@ethanfrey
Copy link
Member Author

See CosmWasm/cosmwasm#1098 to simplify the types upstream.

@ethanfrey ethanfrey marked this pull request as ready for review September 15, 2021 14:09
@ethanfrey ethanfrey merged commit c884cab into main Sep 19, 2021
@ethanfrey ethanfrey deleted the simplify-router-args branch September 19, 2021 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant