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

Reorganizations of contracts in multi-test::test_utils #365

Merged
merged 3 commits into from
Jul 29, 2021

Conversation

hashedone
Copy link
Contributor

No description provided.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff. Let's get this in and rebase #363

Box::new(contract)
}

#[allow(dead_code)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good for completeness

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could just merge with the above? It is less efficient to add a transform from Empty -> Empty but this is only for test code? Does smell funny though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I didn't want to make this for now just because I was still thinking about this, but I see literally no reason to split those functions.

Also while we are at this point - I see no reason to make those returning Box<dyn Contract<C>> instead of impl Contract<C> - it may always be boxed when needed. I see we are in general overusing dynamic dispatch - I find them not working so well with typesystem in general.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we are in general overusing dynamic dispatch - I find them not working so well with typesystem in general.

I used to have 2-4 generics on almost every function. This was replaced with eg &dyn Storage and I found it made everything more readable and usable in general.

We can add in some more generics as you see fit, but I would like to keep the number relatively low.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no reason to make those returning Box<dyn Contract> instead of impl Contract

You can make a commit doing that change, and if it makes code simpler, then let's use it.

where
C: Clone + fmt::Debug + PartialEq + JsonSchema + 'static,
{
let contract = ContractWrapper::new_with_empty(execute, instantiate, query);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing the with_sudo line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True! I did just moving around and it was missing in original code, but should be there.

@hashedone hashedone merged commit d71f005 into main Jul 29, 2021
@hashedone hashedone deleted the 347-contracts-reorganization branch July 29, 2021 15:37
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff


#[allow(dead_code)]
pub fn contract_custom<C>() -> Box<dyn Contract<C>>
pub fn contract<C>() -> Box<dyn Contract<C>>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool

@@ -61,7 +61,10 @@ fn execute(
.add_attribute("action", "payout"))
}

fn sudo(deps: DepsMut, _env: Env, msg: SudoMsg) -> Result<Response, StdError> {
fn sudo<C>(deps: DepsMut, _env: Env, msg: SudoMsg) -> Result<Response<C>, StdError>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice way to make it more generic.
For test contracts we can do it a lot.

(In real contracts we need to specify C in the function signature, so we can compile down to a concrete wasm #[entry_point]. Good to show we can handle ones that don't return that as well.

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.

2 participants