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

cw_multi_test: accept closures in new() #6

Open
dakom opened this issue Jul 17, 2022 · 3 comments
Open

cw_multi_test: accept closures in new() #6

dakom opened this issue Jul 17, 2022 · 3 comments
Labels
Milestone

Comments

@dakom
Copy link

dakom commented Jul 17, 2022

The current implementation makes two restrictive decisions:

  1. The only way to create a wrapper is to pass function pointers (can't pass closures or any ol impl Fn)
  2. All the fields are private

The internal new() creator is able to create a closure from the passed-in function pointer, but the current setup disallows the caller to do this too. e.g. it may want to wrap functions before passing them in.

Suggestion: change the new() functions to accept any impl Fn

edit: the Contract trait is pub so making the fields on ContractWrapper wouldn't add much. The real blocker is using the simple new() methods with closures being passed in

@dakom dakom changed the title cw_multi_test: make fields pub cw_multi_test: accept closures in new() Jul 17, 2022
@hashedone
Copy link
Contributor

The problem is that implementation if ContractWrapper is not trivial, it would require it to be generic over passed handlers types (right now it is generic over magic internals). I think it is doable, I will take a look at it next week, but personally I would suggest just implementing your custom type implementing Contract instead of using ContractWrapper.

@dakom
Copy link
Author

dakom commented Aug 1, 2022

Yup! totally agree @hashedone - looks like it would require adding more generics everywhere, which would be painful on the library side (but would make the API nicer for users)

Also agree, implementing our own custom type is totally workable, the issue here should be thought of more as improving developer experience rather than actually blocking anything important.

@ethanfrey
Copy link
Member

I would put a deep refactor of multi-test before this should be approached (or fix this as part of a refactor). We may be able to simplify the traits and generics if some big brains spend several days focused on it.

@uint uint transferred this issue from CosmWasm/cw-plus Nov 1, 2022
@DariuszDepta DariuszDepta added this to the 1.0.0 milestone Dec 4, 2023
@DariuszDepta DariuszDepta removed this from the 1.0.0 milestone Feb 21, 2024
@DariuszDepta DariuszDepta modified the milestones: 3.0.0, Backlog Mar 7, 2024
@DariuszDepta DariuszDepta added the backlog Backlog label Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants