discussion: cw20-base methods should be nonpayable, or forward funds

[0xekez]

currently, funds sent in a cw20-base Send message are not forwarded, nor is an error returned if they are included. this isn't great UX. it would be nice to put together a coherent thought on what to do with native tokens sent to the cw20-base contract. i am happy to do the implementation work.

two ideas:

1. make all cw20-base methods nonpayable, or
2. make send forward native tokens, and the rest of the methods nonpayable.

obviously there are more permutations here, and I'd be interested in hearing arguments for other ones.

[0xekez]

cc @webmaster128 / @ethanfrey . I'd be curious if this has been discussed before and your thoughts.

[hashedone]

That is a common problem with what to do when unnecessary tokens are sent to contract with the method, and I remember we discussed it with Ethan. Sure, you can add a check if there is anything sent with the method (which is equivalent to making it nonpayable, I even think there is a function named this way in cw-utils failing if anything is sent), but there is a still a thing - what does it prevent? If one wants for any reason to prevent sending tokens to contract, it is impossible - it can always be done via Bank::Send message unless specific chains have some own way of blocking incoming funds. Therefore it prevents nothing besides accidentally sending tokens between contracts. But then - how often are you manually sending a message to the contract and assigning tokens to it? In the vast majority of cases, you do it via some front-end app which has to know contract API anyway - therefore, there is just no need to add the possibility to assign native tokens in your front-end.

Adding the additional nonpayable call on every message seems unnecessary to me, makes additional code which I mostly don't care about (so decreases readability - not in a significant way, but still, not the direction I would like to take), it adds additional overhead (probably unnoticeably, but again - with no reason), with the only improvement is making it slightly easier to test manually.

While back in the day, I was convinced more to very strictly limit the ability to accept tokens by contract, right now, I would do it only when it actually introduces some problems - for eg., if I accept exactly one native token on instantiation and use it to determine "denom" my contract is operating on, then I would actually assert there is exactly one token send (as if they are many, I could pick the wrong one which may lead to problems).

[0xekez]

@hashedone, thank you for the thoughts. I used to also hold your point about code like this being un-needed as frontends are the API consumers. What I have come to appreciate over time is that as we are using non-restrictive licenses, and writing infrastructure-like code, there will be many frontends and many users who do unexpected things with this code.

I wonder if, when the result is funds locked in the contract, it is good to air on the side of caution?

[webmaster128]

I agree with @hashedone here. If we want such a solution, it should include bank sends. But x/bank does not know about the existence of x/wasm, which is also fair from a design point of view. One could imagine a hook system where x/wasm registers for such events. But this would create also some overhead. Something similar was discussed in CosmWasm/cosmwasm#1515 before.

[0xekez]

cool. thanks for the thoughts @hashedone and @webmaster128 :) sounds good to me.