semuxproject / semux-core

Semux Core
https://www.semux.org
MIT License
76 stars 32 forks source link

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

Open dinc334 opened 4 years ago

dinc334 commented 4 years ago

šŸ› 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.

semuxgo commented 4 years ago

~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.~

dinc334 commented 4 years ago

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

semuxgo commented 4 years ago

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

honeycrypto commented 4 years ago

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?'

dinc334 commented 4 years ago

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

honeycrypto commented 4 years ago

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?)

dinc334 commented 4 years ago

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.

witoldsz commented 4 years ago

So, to summarize:

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

dinc334 commented 4 years ago

@witoldsz correct.

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

semuxgo commented 4 years ago

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.

witoldsz commented 4 years ago

@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).

dinc334 commented 4 years ago

@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.

semuxgo commented 4 years ago

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 commented 4 years ago

@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.

dinc334 commented 4 years ago

@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

witoldsz commented 4 years ago

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)

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?

semuxgo commented 4 years ago

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:

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. :(

dinc334 commented 4 years ago

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

semuxgo commented 4 years ago

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.