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

Remove use of dyn in multitest Router #404

Closed
ethanfrey opened this issue Sep 7, 2021 · 1 comment · Fixed by #410
Closed

Remove use of dyn in multitest Router #404

ethanfrey opened this issue Sep 7, 2021 · 1 comment · Fixed by #410
Assignees
Milestone

Comments

@ethanfrey
Copy link
Member

for Storage, Api, Querier, we should stick with dyn, as that is what we pass into Deps and we have no real need for more details. For the other, it can be useful to get the original types when setting up test code.

I am talking about:

pub struct Router<ExecC, QueryC> {
    pub(crate) wasm: Box<dyn Wasm<ExecC, QueryC>>,
    pub(crate) bank: Box<dyn Bank>,
    pub(crate) custom: Box<dyn CustomHandler<ExecC, QueryC>>,
}

https://github.com/CosmWasm/cw-plus/blob/mt-msg-handling/packages/multi-test/src/app.rs#L254-L258

I would propose something like the follow:

pub struct Router<ExecC, QueryC, W: Wasm<ExecC, QueryC>, B: Bank, C: CustomHandler<ExecC, QueryC>> {
    pub(crate) wasm: Box<W>,
    pub(crate) bank: Box<B>,
    pub(crate) custom: Box<C>,
}

It would require some changes to the builder pattern (but we have a similar setup in ContractWrapper where we change types during the builder.

@ethanfrey ethanfrey added this to the v0.9.0 milestone Sep 7, 2021
@hashedone
Copy link
Contributor

Basically I very much like this approach. Two things which I would already note:

  • If you don't have dyn Traits, you nevermore need boxing - just keep stuff by ownership. You need box to handle unsized types, but there is no need to accept unsized generics.
  • I really don't like trait bounds in types. They doesn't give anything, but propagates quickly (if I use the type anywhere, I immediately have to add bounds - even if I don't need to use impl of it). Basically I find it way better to add bounds only to impl block (where you have to add it anyway). There is an argumentation that maybe at some point in future implied bounds would be there, but it doesn't look like it would happen anytime soon if ever.

@hashedone hashedone self-assigned this Sep 9, 2021
@ethanfrey ethanfrey modified the milestones: v0.9.0, v0.10.0 Sep 13, 2021
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 a pull request may close this issue.

2 participants