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

Support submessages in multitest #323

Merged
merged 20 commits into from
Jul 14, 2021
Merged

Support submessages in multitest #323

merged 20 commits into from
Jul 14, 2021

Conversation

ethanfrey
Copy link
Member

@ethanfrey ethanfrey commented Jul 12, 2021

Closes #259

  • Add optional reply to contract wrapper contructor
  • Add sample contract that handles submessages
  • Add (failing) submessage tests
  • Support submessages

@ethanfrey ethanfrey marked this pull request as draft July 12, 2021 17:29
@ethanfrey ethanfrey changed the base branch from main to cleanup-multitest July 12, 2021 17:41
Base automatically changed from cleanup-multitest to main July 13, 2021 07:01
@ethanfrey ethanfrey marked this pull request as ready for review July 13, 2021 17:33
@ethanfrey ethanfrey requested a review from maurolacy July 13, 2021 17:33
@ethanfrey
Copy link
Member Author

I think I finally finished this.

Sometimes it felt like brute forcing to get the levels of caching working and I am sure it could use cleanup.

Happy for a review, even if only stylistic changes.

@ethanfrey
Copy link
Member Author

Okay, with #328 I fixed how we handle attributes to make it compatible with the events as returned with the SDK. And simplified/corrected how we handle SubMsg/reply.

I feel better about the functionality, but still think I need to organize the code better... especially all those traits I added to unify *Router and *Cache

id: msg.id,
result: ContractResult::Ok(SubMsgExecutionResponse {
events: r.events,
data: r.data,
Copy link

@yun-yeo yun-yeo Jul 14, 2021

Choose a reason for hiding this comment

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

For the better reply test, maybe it is possible to wrap the data with Protobuf wrappers depends on the message type (bank & wasm::execute, wasm::instantiate)

message MsgInstantiateContractResponse {
    // Address is the bech32 address of the new contract instance.
    string address = 1;
    // Data contains base64-encoded bytes to returned from the contract
    bytes data = 2;
}

https://github.com/terraswap/terraswap/blob/55c9ac2e54e95462c5c2ed3507cbdc27993b1b64/contracts/terraswap_factory/src/response.proto#L4-L9

https://github.com/CosmWasm/wasmd/blob/8bec411e4bd947cc7d872bd14bd1da865bf703bb/proto/cosmwasm/wasm/v1beta1/tx.proto#L69-L74

Then, we can properly test Protobuf decoding part at reply like

let res: MsgInstantiateContractResponse =
     Message::parse_from_bytes(msg.result.unwrap().data.unwrap().as_slice()).map_err(|_| {
         StdError::parse_err("MsgInstantiateContractResponse", "failed to parse data")
     })?;

https://github.com/terraswap/terraswap/blob/55c9ac2e54e95462c5c2ed3507cbdc27993b1b64/contracts/terraswap_factory/src/contract.rs#L152-L155

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch. I did something in init_response, but that was the old (pre-protobuf) version. Thanks for pointing this out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added #330 for this

Copy link
Member Author

Choose a reason for hiding this comment

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

And closing with #331

Copy link

@yun-yeo yun-yeo left a comment

Choose a reason for hiding this comment

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

LGTM 👏 👏 👏

I think Protobuf style data decoding testing at reply can be done later.

Thanks for making good test-kit for all CosmWasm contracts.

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Partial review of wasm.rs. lgtm.

Will continue reviewing later, as I have some errands to do now.

packages/multi-test/src/wasm.rs Outdated Show resolved Hide resolved
}
}

pub fn cache(&self) -> WasmCache<C> {
Copy link
Contributor

@maurolacy maurolacy Jul 14, 2021

Choose a reason for hiding this comment

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

I started reviewing this file, and this is a little above my head at this point, but:

I guess we need a multi-level (contract state) cache, to separate "server" contracts state from "client" contracts states. This, to mimic the transactional nature of the different contracts in the SDK.

Impressive piece of work, by the way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Before this PR we had a one-level cache in App.execute_multi. That is, all the messages are run together in a cache, and any error will cause the state changes to rollback. They will only be committed to the "real" storage if all return a success.

With submessages, each submessage will need it's own cache. They can write state and then fail, and then reply could catch the error and turn it to a success. That would mean the "parent contract" writes should be committed but the submessage writes not. These semanatics are described here: https://github.com/CosmWasm/cosmwasm/blob/main/SEMANTICS.md#submessages

This leads to the need of an arbitrarily multi-level caching technique. I feel my approach in this PR is a bit brute force and could probably use a bigger refactor to only use one Storage (rather than multiple HashMaps), and unify a lot on the caching in the end, but that would be much more work than needed to get a 0.7.0 release out. This package really should get some more refactors when we have capacity.

packages/multi-test/src/wasm.rs Show resolved Hide resolved
@ethanfrey ethanfrey merged commit 3356773 into main Jul 14, 2021
@ethanfrey ethanfrey deleted the multitest-submessages branch July 14, 2021 08:50
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.

Support submessages in multitest
3 participants