solidity icon indicating copy to clipboard operation
solidity copied to clipboard

Allow function call option on `.transfer` and also `{gas: max}` as a special function call option

Open chriseth opened this issue 4 years ago • 26 comments
trafficstars

This would allow msg.sender.transfer{gas: max}(2 ether);, which would then be the recommended way to implement the withdraw pattern.

chriseth avatar Feb 25 '21 14:02 chriseth

Or maybe msg.sender.transfer{gas: all}(2 ether);?

cameel avatar Feb 25 '21 15:02 cameel

The downside of all is that it might lead to people thinking that it forwards gas(), but it actually only forards gas() * 63/64.

chriseth avatar Feb 25 '21 15:02 chriseth

Why do we need max and not just {gas: gasleft()}?

axic avatar Feb 25 '21 15:02 axic

I think something like max is more explanatory and it also allows us to remove gasleft at some point.

chriseth avatar Feb 25 '21 16:02 chriseth

I wonder why allow specifying the gas in the first place and not just passing all gas available? If we want to get rid of gasleft() that means we perhaps want to also get rid of gas observability, and as such why have {gas: ...} in the first place.

axic avatar Feb 25 '21 16:02 axic

While this would be better than the current situation, it leaves the path of least resistance as "the wrong path" and the path of most resistance as "the right path". I would rather see .transfer and .send deprecated (meaning they emit a compiler warning or they simply don't exist anymore) and we introduce a new method.


Re: naming: I feel like .receive() is a great choice for today if all we are doing is setting gas cost to max. It aligns exactly with what I would expect as a contract developer since contracts can have a function receive() payable on them and it is well documented what it does.

If/when SELFDESTRUCT gets replaced with SENDETH, we can create a new function for .sendeth() which will do exactly what it says on the tin and nothing more.

MicahZoltu avatar Feb 25 '21 16:02 MicahZoltu

@axic so you would be open to a silent breaking change of transfer?

@MicahZoltu I would prefer to keep the symmetry of send and receive. We could use some other words like "remit" - although I'm not too firm in english to make such a decision.

I also see the problem that the "right path" is more complicated, but at least it is less complicated than

(bool success,) = x.call{value: amount}("");
require(success);

chriseth avatar Feb 25 '21 16:02 chriseth

@axic so you would be open to a silent breaking change of transfer?

@MicahZoltu I would prefer to keep the symmetry of send and receive. We could use some other words like "remit" - although I'm not too firm in english to make such a decision.

I also see the problem that the "right path" is more complicated, but at least it is less complicated than

(bool success,) = x.call{value: amount}("");
require(success);

I agree that a silent breaking change is dangerous because it confuses people, and people might be surprised when they find the behavior of transfer has changed.

Adding an option of {gas: max} looks like a good choice. But a potential problem is that this syntax may encourage people to setting the gas to a fixed value when transferring, e.g. {gas: 2300}, which should be avoided. (See documents)

lonelyenvoy avatar Feb 25 '21 16:02 lonelyenvoy

By the way, we should allow try / catch with transfer: Chriseth's post on Solidity Forum This is a backward compatible change, does not require a new function name, and is natural.

lonelyenvoy avatar Feb 25 '21 16:02 lonelyenvoy

We could use some other words like "remit" - although I'm not too firm in english to make such a decision.

I would prefer remit (or really any name) over the current situation or over the current proposal (contract.transfer{ gas: max }).

Another option, which I like less than a new named function but more than the current proposal, would be to do address.transfer{ gas: max } but disallow anything other than gas: max in there and require it for anyone who uses .transfer. If you want to limit gas, you have to do it via the much more complicated x.call{...} syntax, making the bad path the hard path.

I would prefer to keep the symmetry of send and receive.

There isn't symmetry like this anywhere else in the language when you call a contract. For example, if you want to call function getFoo() on a contract you don't call Contract.setFoo(). I think for software developers, it is natural to expect that contract.receive() will call the receive function on contract. Seasoned Solidity developers will know that receive is a special function, and newbies will hopeful get a link to details about the receive function when they read the docs about how to send ETH.

If we call it remit, a newbie contract developer will go looking for a remit function on the recipient contract and may even write a function remit() payable function on the contract, because that is what one would expect to happen when you call a function on an address.

MicahZoltu avatar Feb 25 '21 17:02 MicahZoltu

I think something like max is more explanatory and it also allows us to remove gasleft at some point.

I wonder why allow specifying the gas in the first place and not just passing all gas available? If we want to get rid of gasleft() that means we perhaps want to also get rid of gas observability, and as such why have {gas: ...} in the first place.

@chriseth @axic Is it ok to get rid of gas observability? Some mass operations will become impossible. For example, according to this research paper, many contracts are using this pattern, which is bad:

contract NaiveBank {
  struct Account { 
    address addr;
    uint balance;
  }
  Account accounts[];
  function applyInterest() returns (uint) {
    for (uint i = 0; i < accounts.length; i++) {
      // apply 5 percent interest
      accounts[i].balance = accounts[i].balance * 105 / 100;
    }
    return accounts.length; 
  }
}

The function will not be able to finish if accounts holds too many elements; it will throw an out-of-gas error. The paper suggested the way below:

  Account accounts[];
  uint nextAccount;
  function applyInterest() returns (uint) {
    for (uint i = nextAccount; i < accounts.length && msg.gas > 100000; i++) {
      // apply 5 percent interest
      accounts[i].balance = accounts[i].balance * 105 / 100; 
    }
    nextAccount = i < accounts.length ? i : 0;
    return accounts.length; 
  }

I understand it's better to use a "pull"-style than a "push"-style. However, it may be essential for some contracts to do such unbounded mass operation. How can they achieve this if gasleft is removed?

lonelyenvoy avatar Feb 25 '21 17:02 lonelyenvoy

They can achieve that by having the function take in a page size and having the caller provide enough gas to execute that many iterations.

MicahZoltu avatar Feb 25 '21 17:02 MicahZoltu

They can achieve that by having the function take in a page size and having the caller provide enough gas to execute that many iterations.

@MicahZoltu The gas required by a function can go above the block gas limit, which should be 12,500,000 currently. A simple combination of SLOAD and SSTORE costs 20,800 gas. If this combination is placed inside a loop, then the function can at most run 600 loops before hitting the block gas limit. It then renders this function not usable anymore, which is the root cause of some Denial of Service problems.

lonelyenvoy avatar Feb 26 '21 01:02 lonelyenvoy

I'm not sure how your latest point is related to the topic at hand? Also, for the specific example given you should definitely not be handling interest that way, and really you should never have unbounded loops in code running on the EVM. Even paged loops should be avoided whenever possible.

MicahZoltu avatar Feb 26 '21 05:02 MicahZoltu

I'm not sure how your latest point is related to the topic at hand? Also, for the specific example given you should definitely not be handling interest that way, and really you should never have unbounded loops in code running on the EVM. Even paged loops should be avoided whenever possible.

Ok, I understand. A paged loop can solve the problem, even though it's not a good practice.

lonelyenvoy avatar Feb 26 '21 05:02 lonelyenvoy

Maybe in the document it should be more clear that writing code running on EVM is different from using traditional general-purpose languages due to the gas limitation; otherwise people won't notice such problems and their contracts will be error-prone and vulnerable.

lonelyenvoy avatar Feb 26 '21 05:02 lonelyenvoy

I mostly agree with @axic and @MicahZoltu here. I think the transfer function should not specify the gas at all. But a silent breaking change sounds dangerous. Maybe we can deprecate transfer and have a new function, say msg.sender.pay that is equivalent to call(gas(), ...). It's unfortunate that developers use transfer to a contract where the gas stipend is not enough and have their funds stuck forever.

However, the problem with transfer, remit or pay is that the name is too generic and hides all the gas subtleties. The call option, {gas: max} solves this problem. Perhaps there should also be a {gas: stipend} option.

Maybe an alternative solution is to deprecate both transfer and send and have functions with more descriptive names in our planned standard library. For example, transfer_allGas(address _a, uint _value) returns (bool success) and transfer_onlyStipend(address _a, uint _value) returns (bool success).

hrkrshnn avatar Feb 26 '21 12:02 hrkrshnn

For a transitional period, we could require a gas option for transfer and then make "max" the default in a breaking change. The values could be {gas: min} (stipend), {gas: max}, or a specific number.

chriseth avatar Mar 01 '21 12:03 chriseth

The values could be {gas: min} (stipend)

What problem do you believe is being solved by providing this as an option at all? The goal is to get rid of the stipend, not just change the syntax people use to get the stipend. If you have some contract that absolutely much use the stipend, then you can do .call{ gas: 2300 }(bytes). This is intentionally the hard path because you really should not be doing it.

I would prefer to keep the symmetry of send and receive.

I would still like to see more discussion on this point. Almost nowhere else in programming, and I don't think anywhere else in Solidity, does xxx.apple(...) result in anything other than the apple function getting called on xxx. I feel like xxx.receive() is perfect here because it behaves exactly like the developer expects, which is that a function named receive will be called on xxx, which (from the Solidity language standpoint) is what will happen.

I would expect send(source, destination) for the syntax of a transfer where the recipient may receive a different callback method being called, or maybe even send(source, destination.receive)

MicahZoltu avatar Mar 02 '21 14:03 MicahZoltu

@MicahZoltu Solidity doesn't hardcode the stipend (if the value is known to be non-zero.) The equivalent would be .call{gas: 0, value: non-zero}(...) and .call{gas: 2300, value: 0}(...). I guess one advantage for {gas: min} for stipend would be to prevent users from hardcoding the stipend. Also, it allows for an easy replacement for users upgrading their contracts and were relying on transfer as a re-entrant safe way to send Ether.

Also, why do you think people should not be using transfer with only stipend?

hrkrshnn avatar Mar 02 '21 14:03 hrkrshnn

Also, why do you think people should not be using transfer with only stipend?

It makes it so often the calls don't work with contract wallets, which is something we want to encourage usage of rather than discourage usage of. Also, gas prices change so 2300 might be enough to call a specific contract wallet today but not tomorrow, or potentially even not enough to call any address in the future.

MicahZoltu avatar Mar 02 '21 16:03 MicahZoltu

It make the wold dance :)

KunKax avatar Mar 02 '21 16:03 KunKax

I am a bit worried that we are introducing now introducing a lot special parser cases like gas: min, gas: max, etc.

How about we introduce a gas "global" with the fields .stipend, .left (== gasleft()), .max?

axic avatar Mar 10 '21 13:03 axic

Could also be tx.gas (tx.gas.left, tx.gas.stipend, tx.gas.max), etc. Though it would make names longer.

cameel avatar Mar 10 '21 13:03 cameel

Made a forum post asking for use cases: https://forum.soliditylang.org/t/feedback-on-address-payable-transfer/164

hrkrshnn avatar Mar 10 '21 18:03 hrkrshnn

Here is a blog post https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/ recommending to stop using transfer for solidity. OZ's https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L53 address library and sendValue seem to be used instead.

hrkrshnn avatar Mar 11 '21 11:03 hrkrshnn

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

github-actions[bot] avatar Mar 17 '23 12:03 github-actions[bot]

Hi everyone! This issue has been automatically closed due to inactivity. If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen. However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.

github-actions[bot] avatar Mar 25 '23 12:03 github-actions[bot]