Refactoring ContractWrapper proposal

[hashedone]

Context:

When I was figuring out what was to be done for IBC, I faced a bit of a wall in the form of a ContractWrapper. As this structure works, it is a serious boilerplate that would quickly go out of control. At this moment, there are 13 generics and the same amount of non-trivial trait bounds (by non-trivial, I mean - more than one bound in a single line). For now, we have six supported entry points in MT, and IBC adds another 6, which makes me think the number of those generics with names in the form of letter-number would roughly double (maybe not entirely, as messages for IBC are not custom, but errors would double for sure).

Additionally, many types of aliases would also double in the count - https://github.com/CosmWasm/cw-multi-test/blob/main/src/contracts.rs#L45-L55.

Also the approach taken leads to severe limitations - as all the builder functions (new or with_sudo) are taking fn types instead of some generics, it is impossible to pass there any callable types not being trivial - obviously, for 99% cases it is ok, as we pass there entry point functions, but I can imagine one could like to use this utility to build ad-hoc fake/mock/stub contract passing there some closures - and it won't work if those closures capture any context.

The more severe limitation is related to how _empty functions are defined - they take functions which sometimes take Q being empty, sometimes C`, and sometimes both.

For example - the new_with_empty assumes all passed functions to have both C and Q an Empty, while migrate assumes C being empty, but Q already being of the expected type. I assume it is an oversight, but it leads to inconsistencies and messy API. We want some consistent API for this, but on the other hand - I would like to leave those functions mostly as they are to minimize API changes (or make them more generic so they still can be called the old way).

Proposal

I want to do a couple of things about the contract.rs. The first one is obvious - split the file. ContractWrapper in whatever form deserves its own module, as Contract trait is. Contract trait by its nature has to contain all the entry points and it is not a thing to be split in any way, but it will grow (now with IBC entry points, no one knows about the future).

The other thing is a bit more complex but should lead to way simpler maintenance of a ContractWrapper. I started POC of it on contract_wrapper_refactor branch. The core idea is to switch from Box<dyn Fn(...) -> ...> functions to generics. In the end ContractWrapper would be defined as something like:

pub struct ContractWrapper<ExecuteFn, InstantiateFn, QueryFn, SudoFn, ReplyFn, MigrateFn> {
  execute_fn: ExecuteFn,
  instantiate_fn: InstantiateFn,
  query_fn: QueryFn,
  sudo_fn: SudoFn,
  reply_fn: ReplyFn,
  migrate_fn: MigrateFn,
}

Then all bounds would be reduced in impl-block as follows:

impl<Q, C, ExecuteFn, InstantiateFn, QueryFn>
ContractWrapper<ExecuteFn, InstantiateFn, QueryFn, DefaultSudoFn<Q, C>, DefaultReplyFn<Q, C>, DefaultMigrateFn<Q, C>>
where:
    ExecuteFn: ContractFnT<Q, C>,
    InstantiateFn: ContractFnT<Q, C>,
    QueryFn: QueryFnT<Q>,
{
  pub fn new<Q, C>(exec_fn: ExecuteFn, instantiate_fn: InstantiateFn, query_fn: QueryFn) -> Self
  {
    Self {
      exec_fn,
      instantiate_fn,
      query_fn,
      // Defaults just returns an error
      sudo_fn: default_sudo_fn,
      reply_fn: default_reply_fn,
      migrate_fn: default_migrate_fn,
    }
  }
  // ...
}

impl<ExecuteFn, InstantiateFn, QueryFn, SudoFn, ReplyFn, MigrateFn>
ContractWrapper<ExecuteFn, InstantiateFn, QueryFn, SudoFn, ReplyFn, MigrateFn>
{
  pub fn with_sudo<NewSudoFnT, Q, C>(self, sudo_fn: NewSudoFnT) -> Self
  where:
    // This magic verifies we have some common Q and C, and in general - we don't mess up our type
    // after all
    ContractWrapper<ExecuteFn, InstantiateFn, QueryFn, NewSudoFn, ReplyFn, MigrateFn>: Contract<C, Q>,
  {
    let Self {
      exec_fn,
      instantiate_fn,
      query_fn,
      reply_fn,
      migrate_fn,
      ..
    } = self;
    
    ContractWrapper {
      exec_fn,
      instantiate_fn,
      query_fn,
      sudo_fn,
      reply_fn,
      migrate_fn,
    }default_migrate_fn
  }
  // ...
}

Because of trait bounds in impl-block it would be impossible to create an instance of an invalid wrapper, and it would look way clearer.

Before defining the function traits, I would like to point out a problem - the change is breaking. If at any point anyone used the ContractWrapper spelling out his generics, he will have to get rid of it - however, I doubt anyone did it. In our examples, we always box it and return it as Box<dyn Contract>, and it is how it should be used - and if one wants to avoid dynamic dispatch for his case (don't see a use case as this is only to be stored in MT which keeps it as trait object, but maybe), then he should use the wrapper as impl Contract. Therefore it is reasonable to do this break, and it is best to do it before IBC implementation because it would make it saner to work on doubling the entry points count.

Now let me move to the last important part of the refactor - the functions trait. Those fellows are just extension traits for Fn, which:

• Makes usage of them cleaner (trait bounds are very verbose)
• Takes care of commonizing error types into anyhow

The example trait is the ContractFnT:

pub trait ContractFnT<Q, C>
where:
  Q: CustomQuery
{
  type Msg;
  
  fn call(&self, deps: DepsMut<Q>, env: Env, info: MessageInfo, msg: Self::Msg) -> AnyResult<Response<C>>;
}

impl<T, Q, C, E> ContractFn<Q, C> for fn(DepsMut<Q>, Env, MessageInfo, Msg) -> Result<Response<C>, E>
where:
  E: Info<anyhow::Error>,
  Q: CustomQuery,
{
  fn call(...) -> AnyResult<Response<C>> {
    self(deps, env, info, msg).map_err(Into::into)
  }
}

This would allow passing any simple function (just like right now), but it still doesn't enable passing function-like objects. There is trickery to allow this also; the problem to overcome is that such an object could have multiple Fn implemented, in particular with different msg arguments - a rough idea of how to fix it by allowing passing arbitrary function-like argument by turbo fishing the msg-type if needed can be found here, on more straightforward example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7843098bcff9460cfd56aadf75f30375. With some GAT`s love this can be done even nicer, but not sure about updating MRCV for this (not sure if the current one is including GATs).

Finally Contract trait has to be defined on ContractWrapper, which is trivial.

The final touch is about commonizing the (C, Q) -> (Empty, Empty) generalization. I find that decostumize_deps and so are supposed to be extension traits for their types (Deps in this case). Reasoning for that is that it would enable trait-binding on the fact that casting from one Deps to another is possible. It should then provide a bit nicer APIs, and also allow for more generic transformation (as "I want to test my contract on different chain supporting superset of messages I use"). This would lead to adding functions on our contract function traits, let say:

pub trait ContractFnT<Q, C>
where:
  Q: CustomQuery
{
  // ...
  fn query<NewQ>(self) -> impl ContractFnT<NewQ, C, Msg = Self::Msg>
  where
    Deps<Q>: CustomizeDeps<NewQ>,
  {
    // Don't want to go into implementation - very much possible
    todo!()
  }
  
  fn msg<NewC>(self) -> impl ContractFnT<Q, NewC, Msg = Self::Msg>
  where
    Response<C>: CustomizeResponse<NewC>,
  {
    todo!()
  }
  
  fn custom<NewQ, NewC>(self) -> impl ContractFnT<NewQ, NewC, Msg = Self::Msg>
  where
    Deps<Q>: CustomizeDeps<NewQ>,
    Response<C>: CustomizeResponse<NewC>,
    {
      todo!()
    }
}

This is to be added later, but it would allow one to convert entry-point functions between compatible chains arbitrarily:

fn my_contract() -> Box<dyn Contract<CustomMsg, QueryMsg>> {
  // Here for some reason my `exec` already uses proper `QueryMsg`
  ContractWrapper::new(exec.msg(), instantiate.custom(), query.custom())
}

All the _with_empty functions would be left (possibly marked as deprecated at some point) to minimize any changes in code using MT right now.

[hashedone]

@ethanfrey @webmaster128 @ueco-jb

[ethanfrey]

I would like to point out a problem - the change is breaking. I

This is why multi-test is the only non-1.0 package we have in the CosmWasm rust stuff... it may need breaking.

I didn't follow all the details, but we can definitely think of a better formulation of this. In the end, all these generic things should be wrapped by Box<dyn Contract> when stored, so they don't need any more dyn inside them and can all be concrete instances of different types.

For now, we have six supported entry points in MT, and IBC adds another 6,

I envision a trait IbcContract that extends trait Contract, so those can usually be hidden.
And that means we will also need a different wrapper type that implements this (embedding the normal one), as many contracts will not fulfill IbcContract interface and should not have that. I see less of an issue with sudo, reply (internal detail) or migrate not being type-checked.

Also, I took a look at the diff of your refactor branch: main...contract-wrapper-refactor and I don't see the changes. Maybe you can make a draft PR and add some comments to explain how this issue relates to that code.

[hashedone]

This is why multi-test is the only non-1.0 package we have in the CosmWasm rust stuff... it may need breaking.

Sure, just wanted to point it out.

I envision a trait IbcContract that extends trait Contract, so those can usually be hidden.
And that means we will also need a different wrapper type that implements this (embedding the normal one), as many contracts will not fulfill IbcContract interface and should not have that. I see less of an issue with sudo, reply (internal detail) or migrate not being type-checked.

That won't actually work in the current multitest design because of a single reason - we store contract code as Box<dyn Contract> and use it in API. And it would get complicated if we had two different types. Probably we would end up using IbcContract trait everywhere as it is more "general", and then the original Contract becomes basically obsolete. I just don't see those two entities coexist unless we switch to completely abandoning Box<dyn Trait>, and switch to static dispatch everywhere. That may give some compensation (better type-level utilities, like possibilities for where-enabled functions based on the fact that IBC is implemented for the contract), but I think it would require a way bigger refactor of the whole multitest on the very principal design level. I find Contract trait as just a front-end facade, and the idea is to make it as easy to implement as possible.

I would even debate on giving all the methods default failing implementations, so one creating their own impl for any reason (I can imagine such a use-case) have an easier time not implementing IBC-related endpoints and/or reply/migrate.

Also, I took a look at the diff of your refactor branch: main...contract-wrapper-refactor and I don't see the changes. Maybe you can make a draft PR and add some comments to explain how this issue relates to that code.

I see the whole contracts folders there, in particular, contracts/entry_points.rs, and a commented chunk of "new" wrapper... However, it is slightly different from what I wrote here (I was still experimenting with that). I can take a day or two to prepare a more carefully prepared PR (or update this one, probably).

[ethanfrey]

Makes sense.