Atomic swap

[maurolacy]

Implementation of atomic swaps, for native tokens.

[maurolacy]

Integration tests done. Except for query() integ tests, which are failing with:

thread 'test_query' panicked at 'VM error: CommunicationErr { source: InvalidRegion { source: LengthExceedsCapacity { length: 1931622946, capacity: 1919247470, backtrace: Backtrace(()) } } }', ...

The error is precisely when calling the query() VM wrapper. Will investigate more tomorrow; posting here in case you know something about it. Thanks.

Update: I'm assuming I'm doing something wrong, but cannot find the issue. Here's PoC code for the error: https://github.com/CosmWasm/cosmwasm-plus/tree/atomic-swap-integ_test_query

The error is only with the QueryMsg::List message type; the QueryMsg::Details message works fine.

I had to add the "iterator" feature to cosmwasm-vm, in order to use it. Maybe something related to that? I'll continue looking.

[ethanfrey]

You have to add the iterator feature to both cosmwasm-vm and cosmwasm-storage, but I assume you did.

This seems to show you are trying to send an enormous buffer or one with negative size. Can you link to the exact line that returns this error? I will eyeball it and see if I see anything obvious before digging down. You should never be able to create such an error with the higher-level APIs you use

[maurolacy]

You have to add the iterator feature to both cosmwasm-vm and cosmwasm-storage, but I assume you did.

Yes.

This seems to show you are trying to send an enormous buffer or one with negative size. Can you link to the exact line that returns this error? I will eyeball it and see if I see anything obvious before digging down.

There's now PoC code in the atomic-swap-integ_test_query branch.

The last line of integ_test_query() produces the error.

To reproduce:

cargo wasm
cargo integration-test --tests integ_test_query

Thanks.

[ethanfrey]

Interesting. Two points:

I don't see the equivalent test here: https://github.com/CosmWasm/cosmwasm-plus/blob/atomic-swap-integ_test_query/contracts/atomic-swap/src/contract.rs rather just a details query. Can you make a unit test on the ListQuery to see what it says? It will give you more detail info on any error.

Looking at this https://github.com/CosmWasm/cosmwasm-examples/blob/master/erc20/tests/integration.rs#L378 it seems you do the same thing (unless you want to just unwrap it there, but that is not a functional change).

[ethanfrey]

Also, I have been removing integration tests in this repo, as I hadn't found a bug with them before (besides the cosmwasm contracts that were there to debug it). I would like to hunt down what happens here, but if we figure out why this is broken, I would generally, just focus on the unit tests and maybe remove all these.

Also, good to add your contract to .circleci/config so this is covered by CI (maybe after removing the integration tests that slow things down a lot)

[maurolacy]

Interesting. Two points:

I don't see the equivalent test here: https://github.com/CosmWasm/cosmwasm-plus/blob/atomic-swap-integ_test_query/contracts/atomic-swap/src/contract.rs rather just a details query. Can you make a unit test on the ListQuery to see what it says? It will give you more detail info on any error.

I converted it to a minimal test, just to show / pinpoint the issue. I have the full integ test for query, and I can commit it if you like, for example in the original branch (it will break the build, though). Anyway, the unit test for query() has a QueryMsg::List (see https://github.com/CosmWasm/cosmwasm-plus/blob/95ba038f4403c64c2a30bb81e11ca0aa4e31f541/contracts/atomic-swap/src/contract.rs#L577), and it works fine. What also works (as I've checked manually) is QueryMsg::Details. It's only QueryMsg::List that's broken in the VM.

Looking at this https://github.com/CosmWasm/cosmwasm-examples/blob/master/erc20/tests/integration.rs#L378 it seems you do the same thing (unless you want to just unwrap it there, but that is not a functional change).

If I add the unwrap()it's the same; this is just the minimal way to reproduce the problem. In fact, I've checked and the query() implementation (the one in contract.rs) is never called... there seem to be an issue with arguments parsing in the VM, as far as I can tell at this point.

[maurolacy]

Also, I have been removing integration tests in this repo, as I hadn't found a bug with them before (besides the cosmwasm contracts that were there to debug it). I would like to hunt down what happens here, but if we figure out why this is broken, I would generally, just focus on the unit tests and maybe remove all these.

OK.

Also, good to add your contract to .circleci/config so this is covered by CI (maybe after removing the integration tests that slow things down a lot)

Will do.

[ethanfrey]

Huh, I ran this code and played with unit tests and get the same issue.

The all_swap_ids code seems proper as well. Maybe we have some vm error iterating over an empty range. I should add a test for that to the cosmwasm.

Update: I did update the test to List a non-empty bucket and the same issue.

[ethanfrey]

@maurolacy

I am off for a bit, but if you want to dig deeper, can you try to add an equivalent test to https://github.com/CosmWasm/cosmwasm/blob/master/contracts/queue/tests/integration.rs (which uses the iterator a bunch). You may just want to add a new query that works like you current List one here: https://github.com/CosmWasm/cosmwasm/blob/master/contracts/queue/src/contract.rs#L115-L124 and then test it with unit test and integration test.

If you get that to fail, then we are much closer to a fix - iterating in one repo. You can also import cosmwasm-storage for the same prefix store approach if it doesn't fail without it.

[maurolacy]

Will do.

[maurolacy]

OK, I've created unit and integration tests under the queue contract in cosmwasm, and the integration test fails.

The error is similar, but not exactly the same:

thread 'query_list' panicked at 'VM error: CommunicationErr { source: RegionLengthTooBig { length: 1663187490, max_length: 65536, backtrace: Backtrace(()) } }', ...

(Numbers are always the same).

I cannot push the branch as I don't have permission.

I'll create an issue to track this, and document stuff there.

[maurolacy]

Created issue: CosmWasm/cosmwasm#508

[maurolacy]

Pushing the query() integration test just for reference / completeness.

Will remove integrations tests later as agreed.

[ethanfrey]

Thanks, I will review tomorrow... been super busy the last days - sorry.

Can you rebase on master since I did merge a lot recently?

[CLAassistant]

All committers have signed the CLA.

[ethanfrey]

Thanks for building this out. I have also made some enhancements to the patterns in cosmwasm-plus the last week: pagination, new Expiration type, version calculation, etc. And make a lot of references to update to those newer specs. I think I only see one bug, and a few missing tests (and a few types that can be adjusted).

Please make the requested updates and this should be good to merge.

[ethanfrey]

Looks good. A few minor nitpics, I will merge this and you can address those in your next PR if you wish.

Also note #69 which you can gladly take on if you wish.