Update token swap

[Art3miX]

This PR implements #649 feature

General design:

When funding the contract you can specify what messages you want to execute on completion of the swap.
in case of promised cw20, you can provide any binary message that will be fed as is into cw20::ExecuteMsg::Send message.
in case of native token, you can execute any amount of messages from the AcceptedMessages enum (bank send and burn, wasm execute and instantiate).

All types moved to types.rs.

This allows for a whole bunch of different use cases such as:

1. Fund a different swap contract
2. instantiate vesting contract
3. burn half native tokens and send other half to someone else

Improvements / questions

1. Currently promised cw20 tokens can only be sent, while native token can do more, should we also add more messages to cw20 cases? We can add Transfer and Burn as well.

[Art3miX]

@0xekez As always thanks for yet another great review!!!

I done the strategy change based on your 2 comments, and modified the tests, here is the new strategy.

We take all msgs we want to execute in the instantiate msg, the msgs are based on the promised token, so you can think of them as "What msgs I need to execute with those funds", so if someone promise native token, you set the on_completion field to empty vec and it will send it to the other party, or specify a specific msgs to execute (like init some other contract), same idea for cw20 token.

We validate the msgs by type enforcement and also by making sure the total funds sent in all msgs matches the total funds promised or else it will error.

1 security concern with this tho, because the msgs are not sent on funding, the party who instantiate this contract must be a trusted party, or else a malicious party can execute any msg he wants with your funds, and the only way to confirm this will be by querying the statues and verifying the specified msgs.

One thing that helps security is that those msgs are immutable, which also means that if you specify a wrong amount or msg you would need to create a new swap.

Maybe we should provide a msg that locks/invalidate this contract? so if a mistake happened when instantiating, it can be easily closed and refunded to make sure it won't be used in the future by mistake (new issue new PR of course)
Probably should be able to be closed by any party or by the the instantiate sender.

[0xekez]

looking good! a few comments on the new trait. I think it would make the code cleaner / safer to use an enum as we only have two variants.

[0xekez]

all of these variants have a Vec<Coin> parameter that we want to be sure to validate. currently, we match and validate on each branch:

match type {
   a => validate_coins(a.coins),
   b => validate_coins(b.coins),
}

what if we moved the amount to a top level field in a struct.

struct NativeSend {
  coins,
  msg,
}

then, we don't have to remember to validate in each match arm which makes it less likely we forget!

validate_coins(coins)
match msg {
  a => ...,
  b => ...,
}

[Art3miX]

I wish, but the whole point here is that we take all native msgs we allow as is, with the funds parameter, this also tells us how much exactly to send on each msg.
If we do the above, there is zero verification that the coins they sent in NativeSend , is the actual coins they set in each msg, so the check will be worthless because we don't verify the actual sent funds in the msgs.

[0xekez]

reading this code i my spidey sense was tingling. first, the iteration here can be done functionally with a fold over a tuple (no need for mut and map):

                    let (on_completion, tokens_sent) = on_completion.into_iter().try_fold(
                        (vec![], Uint128::zero()),
                        |(mut messages, total_sent), msg| -> Result<_, ContractError> {
                            let (sent, msg) = msg.into_checked_cosmos_msg(deps, &denom)?;
                            messages.push(msg);
                            Ok((messages, sent + total_sent))
                        },
                    )?;

second, only needing to check the length in the on_completion.len() > 0 case seems strange. the semantics of that suggest that nothing will be done on completion meaning... what? there exist both a bank and cw20 transfer option in the message types, so shouldn't a transfer to the other counterparty be represented as one of those? why have both ways to trigger a transfer to the other counterparty?

this is one of those weird corner cases of code where the logic gets weird and you realize you've got a wrong abstraction. how about making doing nothing on completion not allowed, and expressing the desire to transfer tokens with the transfer completion options?

[Art3miX]

1. Thats a neater way of doing it, I like it.
2. I look at it as having a default, at the end of the day we are talking about a swap contract, the default behavior is sending the tokens to the other party, this design allows having a basic swap in place without adding complexity to calling it, so providing an empty vector will result in a default swap behavior.
   If you need something more complex, you can provide the msgs you want in the on_completion field.

Im not against forcing all msgs on on_completion, I just think having this default makes a much better UI.

Thinking about it, I think I can set the default here instead of later, which will make it much clearer that we set default.

[0xekez]

(2) sounds like a good idea. the default can be to fill with a transfer message, maybe we just do that at instantiation time?

[0xekez]

might be nice to break this logic into a function as well so it can be reused in the cw20 case

[JakeHartnell]

Why even have this? Seems like it just risks someone accidentally using it?