Solidity Payable Functions - Discussion

[jon_m]

Hi @Tomaage,

@Tomaage

function withdraw(uint amount) public returns (uint) {
    require(balance[msg.sender] >= amount);
    balance[msg.sender]-=amount;
    payable(msg.sender).transfer(amount);
    return balance[msg.sender];
}

You now have all of the lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body to reduce security risk …

• Check inputs (require statements)
• Effects (update the contract state)
• External Interactions (e.g. transfer method)

Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract to the external address…

payable(msg.sender).transfer(amount);

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent it, in the courses which follow this one.

---

Answers to your questions, and additional comments

@Tomaage

1 A Bank that anybody can deposit to, but only I can withdraw from? I kinda like it, but I see why it would not be popular.

@Tomaage

2 So since I was missing the code

balance[msg.sender]-=amount;

in my withdraw function, I was not telling the function to update the smart contracts internal balance of this particular msg.sender . So they could withdraw all the eth stored in the contract. Even eth from other adresses. When one of those adresses came and wanted to withdraw their ETH, the contract balance would be 0, but would their getBalance function still show what they had put in?

Yes, that’s correct. The getBalance() function would return what has been updated in the mapping. This would be the cumulative effect of …

• balance increases for deposits, updated with …
  balance[msg.sender] += msg.value;

   and

• balance increases (recipients) and balance decreases (senders) for internal transfers executed by the transfer() function, and updated with the _transfer() helper function …

    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }

   but not any

• balance decreases for withdrawals

Basically, the internal accounting function of the balance mapping would no longer work, would no longer provide a true record of each user’s share of the total contract balance, and would no longer serve as a means to prevent users from withdrawing more than they are entitled to.

As time went by and more and more transactions took place, including withdrawals, the difference between (i) the sum of all of the individual user balances in the mapping, and (ii) the total contract balance, would become greater and greater. A situation could quickly arise, where any ether deposited in the contract could be withdrawn by anyone who could “get there first”. As the ether held in the contract is finite, and the contract balance is always correct and accurate, where there were “winners” (“thieves”), there would be equal and opposite “losers” (“victims”).

@Tomaage

That transfer function I have there. It only transfers the value internaly in the smart contract, and updates the internal balance. It does not send it out to that recipients external wallet?

Yes … but it only performs an internal transfer of funds (effectively, a reallocation of funds) between two individual users of the Bank contract. Because no ether is entering or leaving the contract, the net effect to the contract address balance is zero, and the only change is to the individual user balances in the mapping, in order to adjust the amount each of the parties involved in the internal transfer is entitled to withdraw from the contract (their share of the total pooled funds) i.e. the recipient’s entitlement (balance) is increased by the same amount the sender’s entitlement is reduced.

@Tomaage

So this bank only lets you use its internal system to send to somebody else. If they want the money, they neeed to call the bank with the withdrawl function?

Correct … but we could easily add an additional function which allows users to transfer ether (part of their individual share as recorded in the mapping) out of the contract to an external address that is different to their own. I can see you’ve already tried this …

@Tomaage

To make it transfer out of the contract to the recipient external wallet; I would have to make it a payable function?

No… a function only needs to be marked payable when it needs to receive ether from an external address and add it to the contract address balance (e.g. our deposit function). It might help to think of payable as the code that enables msg.value to be added to the contract address balance, without us having to add any further code for this. In the deposit function this happens automatically because we have marked it payable. The line  balance[msg.sender] += msg.value;  then adds the same value to the individual user’s balance in the mapping, in order to keep track of their share of the total funds held in the contract.

Instead, to create a function that allows users to transfer ether out of the contract to an external address that is different to their own, we would just need to make a minor adjustment to the withdraw() function e.g.

function externalTransfer(uint amount, address recipient) public {
   require(balance[msg.sender] >= amount);
   balance[msg.sender] -= amount;
   payable(recipient).transfer(amount);
}

/* Notice, we don't actually need to return a value in either the
   withdraw() function, this new function, or the deposit() function */

If we make a function payable when it doesn’t need to be, and the caller sends an ether value to the function by mistake (as well as calling it with the arguments that it does require), then this ether would be added to the contract balance and potentially lost to the user. So, if a function doesn’t need to receive any ether, it is much securer if we leave it as non-payable, because then, if the caller makes the same mistake, the function will revert, and the ether won’t be sent.

@Tomaage

They would not know that you sent it to them in this contract unless you told them

That’s what events are for. We would have a dapp, with a front-end and a user interface. In other courses you can learn how the Web3 client can use a Web3.js library to listen out for events and retrieve the data. It would then be for the front-end code to handle how the end user is notified of the captured event data that is relevant to them.

@Tomaage

they would not have the money should the smart contract be hacked.

Unfortunately, that’s correct … and why smart contract security is critically important!

As always, just let me know if anything is unclear, or if you have any further questions

[Tomaage]

Thanks @jon_m

Just want to say that feedback like this is super valuable for my understanding . I am totaly new to coding, so lots of basics that I dont have a understanding of yet that slows my progress. But knowing that this kind of help is waiting in the forum is a big boost to my motivation to continue on even though there is a big mountain of stuff infront of me of stuff I dont understand yet, but have to learn if I want to contribute to the decentralised world of web 3.

[jon_m]

Yes, I totally understand!

But you’re asking all the right questions, and they show that you are really wrestling with the new concepts. As a beginner, you may well feel like your progress is slow, but by working through things carefully and methodically, like I can see you are, you’re building the solid foundations which, in the long run, you’ll really benefit from

By the way, just one word of advice. Especially when you’re starting out, no matter how much time you spend trying to understand certain concepts, there’ll always be a few loose ends and further questions that you just can’t seem to resolve. That’s perfectly normal. Some things you won’t fully understand until you have a few more building blocks in place, or have simply spent more time being exposed and working with the code.

Obviously you do need to put the time in asking questions, doing your own research and experimenting with the code, to see if you can resolve things in the here-and-now, but some things you should just make a note of, so that they stay on your radar, and then come back to them later on. You’ll find that some questions get answered indirectly, while you’re working on something else, because you learn about something that was preventing you from answering certain questions before. So it can be a kind of knock-on-effect, which you benefit from by being patient.

Anyway, I hope there’s something there that you find helpful and encouraging.

You’re making great progress!

[seybastiens]

If a variable used/declared in a function is payable, then the function has to be payable too right ?

[seybastiens]

In the Transfer video where you explain about the withdrawal and write :
msg.sender.transfer(amount) ;
The transfer function is a pre-built one right ?

I mean it’s not the function we created earlier in the course that has the same exact name right ?

If it’s a pre-built one, how can we see the exact code behind it ?

[jon_m]

Hi @seybastiens,

@seybastiens

If a variable used/declared in a function is payable, then the function has to be payable too right ?

No … a function is only marked payable when it needs to receive Ether from an external address and add it to the contract address balance e.g. the deposit() function in our Bank contract.

If a function needs to send Ether (but not receive it) then it shouldn’t be marked payable. It is bad practice and poor risk management not to restrict a function to non-payable if it doesn’t need to receive Ether, because it won’t prevent Ether being sent to the function and added to the contract address balance by mistake. The sender could then experience difficulties in getting this Ether refunded, or they may even lose it!

[jon_m]

Hi again @seybastiens,

@seybastiens

msg.sender.transfer(amount) ;
The transfer function is a pre-built one right ?

Yes, it is.

@seybastiens

I mean it’s not the function we created earlier in the course that has the same exact name right ?

No, it’s not.

I think it’s clearer to call this transfer a method rather than a function.

Solidity’s in-built transfer method is called on a payable address. It has the following pre-defined syntax:

<address payable>.transfer(uint amount)

Just as the execution of a function marked with the payable keyword results in the EVM performing low level operations which add any Ether value sent to the function to the contract address balance …
… when Solidity’s in-built transfer method is executed, it results in the EVM performing low level operations which (i) deduct an amount of Ether from the contract address balance which is equivalent to the unsigned-integer argument the transfer method is called with, and (ii) transfer and add this amount of Ether to the external address the transfer method is called on.

Let me know if you have any further questions about this

[seybastiens]

Ok makes more sense thanks.

Is it normal though that I get a :
"TypeError: Type address is not implicitly convertible to expected type address payable. "
on

address payable toSend; 
toSend = msg.sender;

or address payable toSend = msg.sender; ?

It seems like on my end msg.sender isn’t recognized as a payable address by default.

Solution (found on StackExchange):

From Solidity 8.0.0 the transfer structure should be :
payable(msg.sender).transfer(amount);

[jon_m]

@seybastiens

From Solidity 8.0.0 the transfer structure should be :
payable(msg.sender).transfer(amount);

Yes, that’s right

But the syntax change itself doesn’t relate to the transfer method, which has always required a payable address. Instead, it relates specifically to msg.sender, as you first suspected …

@seybastiens

It seems like on my end msg.sender isn’t recognized as a payable address by default.

Prior to Solidity v0.8 msg.sender is already a payable address by default, and so doesn’t need to be explicitly converted whenever the syntax requires it to reference a payable address, such as the address on which the transfer method is called …

// Solidity v0.7
msg.sender.transfer(amount);

… or when we want to assign it to a payable address variable …

// Solidity v0.7
address payable toSend; 
toSend = msg.sender;
toSend.transfer(amount);

// or

address payable toSend = msg.sender;
toSend.transfer(amount);

However, from Solidity v0.8, msg.sender is non-payable by default, which means having to explicity convert it to a payable address when necessary. And this is why we need to modify the code in the above examples as follows …

// Solidity v0.8
payable(msg.sender).transfer(amount);

… or when we want to assign msg.sender to a payable address variable …

// Solidity v0.8
address payable toSend; 
toSend = payable(msg.sender);
toSend.transfer(amount);

// or

address payable toSend = payable(msg.sender);
toSend.transfer(amount);

[patgeek]

hello

how can we know the total amount of wei deposited in the smart contract after
users deposited wei with a deposit function

i mean how can we know the total amount of money a smart contract has

[Gry]

You can see the total amount of money in smart contract by including line “return address(this).balance;” in the smart contract and making getBalance() function for it as below:

function getBalance() public view returns (uint) {

        return address(this).balance;

    }

Also remember to have a mapping tracking addresses / their deposited amounts to balance somewhere

mapping(address => uint) balance;

[patgeek]

i have declared a recipient address as payable ====> address payable _toAddress
i do not understand why this payable address does not receive the amount withdrawed from
the address msg.sender

  function withdraw(uint _amount, address payable _toAddress) public payable returns(uint){
              //  require(balance[msg.sender] >= _amount, "insufficient Balance");

               // uint fromaddressbalanceBeforeWithdraw = balance[msg.sender];
              //  uint toAddressBalanceBeforeWithdraw = balance[_toAddress];

                _toAddress.transfer(_amount);
                msg.sender.transfer(_amount);
                
                  balance[msg.sender] -= _amount;
                  balance[_toAddress] += _amount;

               // assert( balance[_toAddress] == toAddressBalanceBeforeWithdraw + _amount);
              //  assert(balance[msg.sender] == fromaddressbalanceBeforeWithdraw - _amount);

                emit withdrawDone(_amount, msg.sender, _toAddress);


                return msg.value;

        }

[Gry]

I assume you are attempting to spend funds from a wallet to another wallet. If that´s the case, there is no way to program a contract to spend from another wallet.

You can how ever deposit funds to contract with a wallet and withdraw from the contract with another wallet. Remember to deduct balances before doing the transfers/interactions by the way, for security reasons. (to prevent re-entrancy attack)

[patgeek]

i have tried the following, and i manage to send money from one account to another

function send(address payable _receiver) public payable {
                        _receiver.send(msg.value);
                }

[thecil]

You can also just do the following, keep in mind that from solidity version 0.8, you have to convert an address into payable to be able to transfer to it.

function send(address _receiver) public payable {
                        payable(_receiver).transfer(msg.value);
                }

Carlos Z

[Ben17]

Hi, I have a quick question about the transfer function. My code was working perfectly fine before I tried to make it payable and transfer ether with it. Now even though I have set it to payable in the function header it still keeps giving a message saying the function needs to be payable to send value. Below is my current relevant code and a screenshot of the error revert message Remix keeps giving me. Any help would be greatly appreciated, thanks!

function transfer(address recipient, uint amount) public payable {
       require(balance[msg.sender] >= amount, "Insuficient Balance"); 
       require(msg.sender != recipient, "Cannot Transfer to Yourself"); 
       uint previousSenderBalance = balance[msg.sender];    
       _transfer(msg.sender, recipient, amount);
       GovernmentInstance.addTransaction(msg.sender, recipient, amount);
       assert(balance[msg.sender] == previousSenderBalance - amount);
    } 

    function _transfer(address from, address to, uint amount) private {
       balance[from] -= amount; 
       balance[to] += amount; 
    } 

[thecil]

There is a difference when sending eth, is not the user who pay to transfer to another user, its the contract who will.

Check this simple contract I made to show you the difference between transfer from a user to another inside the contract and withdraw or deposit eth on the contract.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.7;

contract contractPayable{

mapping(address => uint) balance;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    function deposit() public payable returns (uint)  {
        balance[msg.sender] += msg.value;
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public {
        require(balance[msg.sender] >= amount, "not enough balance to withdraw");
        balance[msg.sender] -= amount;
        payable(msg.sender).transfer(amount); 
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
                
        assert(balance[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }

}

[Ben17]

Oh ok, I got it now and it is working fine. Thank you!

[Lamasticot]

Can we put the msg.value as the total balance of an wallet, like when someone is connected on my SC and he want to send money, the amount must be the balance of the wallet. Thanks

[JohnVersus]

Hi @Lamasticot
That would cause an insufficient gas error. There should be some balance left in the wallet for gas payment.