semux-core icon indicating copy to clipboard operation
semux-core copied to clipboard

TRANSFER to a smart contract won't trigger the code execution

Open dinc334 opened this issue 5 years ago • 19 comments

🐛 Bug

Different behavior in semux and eth.

To reproduce

  1. Create a contract with function that change state and need more than 0.005 sem for execute
  2. Make transfer to contract with default fee
  3. Check state

Expected Behavior

In ETH your Tx will be reverted, but in Semux it works.

dinc334 avatar Sep 09 '19 08:09 dinc334

~This would be resolved once the network accepts the VOTING_PRECOMPILED_UPGRADE softfork (v2.1.0). For now, REVERTed transactions are not refunded for remaining gas.~

semuxgo avatar Sep 09 '19 09:09 semuxgo

It's not a reverting txs. Semux EVM thinks it's legit txs, but there are not.

dinc334 avatar Sep 09 '19 09:09 dinc334

I see, you're sending a direct TRANSFER to a contract. I misunderstood it.

semuxgo avatar Sep 09 '19 09:09 semuxgo

We can solve it on UI side in 2 possible ways:

  1. Make transactions from 'Send' tab to be CALL with non zero values instead of current TRANSFER type. So basically starting from the new wallet version we will stop using TRANSFER type in wallet at all, it will be possible to trigger it only via API. This will also allow to remove redundant 'Call smart contract' tab leaving only: 'Send' (now CALL) and 'Deploy a contract'. The 'Fee' from 'Send' tab should be replaced with gas, gasPrice of course.

  2. Just make a check in Send tab and alert something like 'You are about to transfer money to a smart contract. Regular transfers won't execute any code. Maybe you wanted to 'Call' it instead?'

honeycrypto avatar Sep 09 '19 09:09 honeycrypto

@honeycrypto 2 approach looks unreal, bc users can interact with contract via extensions, web wallet, not only gui wallet.

dinc334 avatar Sep 09 '19 09:09 dinc334

dinc, these are UI changes, so both 1 and 2 should be implemented by all wallet, extension etc devs (ping @witoldsz what do you think?)

honeycrypto avatar Sep 09 '19 09:09 honeycrypto

There are not many contracts in network, but even all bounty contracts has this type of "difficulty of using". There was a self-voting contract - it sucks due to the fact that the validator should send an empty call and not a transfer There was a token contract - it sucks due to the fact that if you wanna contribute to ICO, you should send an empty call and not a transfer There was a crowdfunding contract - it sucks because each donator should send an empty call and not a transfer Only Gods know how many contracts will be irrelevant bc of this "issue".

I think, we have only one way to fix it - is try to remove transfer types and make only calls like Eth make.

dinc334 avatar Sep 09 '19 10:09 dinc334

So, to summarize:

  • Ethereum can use regular transfer to the contract to trigger the anonymous function
  • The above does not work in Semux

Is that correct? If so, I would suggest fixing it, so Ethereum and Semux would work the same in this (very) basic regard.

witoldsz avatar Sep 09 '19 10:09 witoldsz

@witoldsz correct.

And that's why ETH use contracts to implement pos without core modifying and creating new types of txs.

dinc334 avatar Sep 09 '19 10:09 dinc334

I'd rather consider this as a different design rather than a bug.

You get the exact same Ethereum's behavior by calling a contract with a value from the "Smart Contract" panel. The "Send" panel is dedicated for direct, uninterpreted, value transfer.

semuxgo avatar Sep 09 '19 10:09 semuxgo

@semuxgo is it really different design or it just happened to work that way.

I must admit that I have lost lot of time trying to figure out why my transfers to contract "does not work" until I realized that problem.

If we leave it "as is" it will bring confusion to many others. We have to shed the responsibility to the end-user if he or she must use regular transfer or evm-call transfer each time they want to transfer some funds. Too bad if they choose wrong because their money will be lost. This is super user unfriendly.

My suggestion would be to translate regular transfers into evm-calls by the semux core (so users would see no difference) and then add a virtual anonymous function to every non-contract address which would just accept that funds (like regular transfer would).

witoldsz avatar Sep 09 '19 10:09 witoldsz

@semuxgo Because of this "design" - we loose the functionality that ETH is. Why people will use Sem instead of Eth, if their contract doesn't work correctly ? If Semux wanna use EVM - it's a bug. If no - it's a design feature.

dinc334 avatar Sep 09 '19 10:09 dinc334

Users are not loosing the functionality. As I said earlier, they have to go through the "Smart contract" tab and do a transfer there.

However, I agree that this is causing confusion to end users and adds extra complexity to user kernel interaction (the introduction of differentiation between transfer and call-with-value). Definitely considering an optimization to mitigate this.

semuxgo avatar Sep 09 '19 11:09 semuxgo

@witoldsz this is a compromise we had to made when integrating the EVM. Changing the behavior of existing TRANSFER transactions has many implications, specifically it would affect the transaction billing and require new definition of failure. I'm open to brilliant ideas, though.

semuxgo avatar Sep 09 '19 11:09 semuxgo

@semuxgo Users are loosing the functionality. As I said earlier you cannot create autovotercontract, crowdfunding contract, and ico distribution contract bc of Semux design. Noone use ETH's full node to make a call txs, everyone use services like MEW, Metamask, TrustWallet, etc. But semux design forces to use full node or any other service + additional understanding, when you need to use call or transfer tx type. ETH ICO

  • just send some ETH to contract address and you get tokens SEM ICO
  • pls send ONLY call txs with some sem and empty data, if not - you will lose all coins.

dinc334 avatar Sep 09 '19 11:09 dinc334

  • transaction billing: does not seem to be a problem (at least from my limited pov), options:
    • the fee will just change to whatever works in EVM
    • the case of transfer-to-not-a-contract would be detected and fixed fee would be applied (just as it's now)

Issue: of course, when executing some code by transfer, the user would have to choose max gas, gas price, etc… GUI can help figure it out (just as it does for Ethereum wallets)

  • failures… don't know much about it, but does not seem to be a blocker

The question is: do we agree it cannot stay that way or… How to fix the problems of regular users who probably do not want to have to think about two kinds of transfers and which to choose when?

witoldsz avatar Sep 09 '19 11:09 witoldsz

An alternative approach is to deprecate the TRANSFER transaction type. For the Semux Core GUI, merge the "Send" panel and "Smart Contract" panel. From the merged view, users only have to choose from two options:

  • Call a contract (or transfer coins);
  • Deploy a contract.

This would give us the same Ethereum behavior. Other user-facing clients can make similar changes.

The only drawback is that exchanges have to be based on the "CALL" transactions. Also, I do appreciate the single-fare property of the TRANSFER transactions. :(

semuxgo avatar Sep 09 '19 20:09 semuxgo

@semuxgo This is the only way how to fix it.

dinc334 avatar Sep 09 '19 20:09 dinc334

Updated the GUI to show warnings when a user tries to transfer to a smart contract, as a temporary solution. Commit: 961eeaa70aca173be6129ef80fbffdfa7c75dea4

More analysis and discussion would be required for a decent solution.

semuxgo avatar Sep 09 '19 21:09 semuxgo