Solidity Payable Functions - Discussion

ah! got it!

totally makes sense: double boom!

to your last point: yeah, i followed up on those links in your other comments that i hadn’t seen, which were exactly for what i was looking.

again thanks for your thorough explanations; i’ll definitely be back for the 201 course at some point

1 Like

My Solution:

when i use the below code, I get this message:

transact to Bank.withdraw errored: VM error: revert.

revert
The transaction has been reverted to the initial state.
Reason provided by the contract: “Balance not sufficient”.
Debug the transaction to get more information.

    function withdraw (uint amount) public returns (uint) {
        require(balance[msg.sender]>=amount, "Balance not sufficient");
        payable(msg.sender).transfer(amount);
    }

I think this is the purpose. We don’t want to allow withdraw any amount of ether we don’t have in our balance. Require function will throw error.

1 Like

Hi @skawal22,

Am I right to assume that this solution is an earlier version of the final one you posted for the Transfer Assignment, which I’ve already given you feedback for?

Correct … If the address calling the withdraw function has an individual balance in the contract (recorded in the mapping) which is less than the amount requested for withdrawal, then the require() statement will fail and revert the transaction. We want this to happen in order to prevent a user withdrawing more than their share of the total contract balance (i.e. from effectively stealing other users’ funds). We know which specific require() statement has triggered revert, because the error message included as its 2nd argument is included within the error message displayed in the console.

Calling this function with a withdrawal amount, before having deposited any ether by calling the deposit() function with the same address, will always generate this error message, because the address doesn’t yet have any deposited ether to withdraw.

And as I’m sure you’ve already realised, for the require() statement to perform its check correctly, you need to add an additional line of code to reduce the user’s individual balance in the mapping by the same amount as the ether transferred out of the contract and added to the user’s external address balance. You included this in the other solution you posted (as shown above).

Just let me know if you still have any questions about this.

Hi @jon_m, I mistakenly posted this one here. Yes you’re right, its an earlier version of the final one for the Transfer assignment.

Thanks,
Sk

1 Like

Good day Filip and all,

I’m struggling to figure out the connection between the different accounts in solidity. for example, when I invoke the getAddress function, I’m getting the address of the account in box ‘1’. i.e. the msg.sender is the account in Box1. Does this mean that the account in Box 1 is actually ‘calling’ the getAddress function ?

Next question…

If I write the following function to transfer some Eth:

    function getMoney() external payable {

    }

I see that the amount in box 1 diminishes by the value in enter in the ‘VALUE’ box. but where does this Eth go to ?

If its going to the contract how can I check this value and transfer it back to the account shown in Box 1 ?

thank you in advance!

1 Like

Hi @Azmil_K,

… and welcome to the forum! :slight_smile:

Yes … you can change the address showing in the Account field (your “Box 1”) using the dropdown, to simulate different external addresses calling specific functions in your smart contract.

Ether/wei values sent to a function marked payable are automatically added to the contract address balance.

address(this).balance  will give you the contract balance. You can add a getter so that any external address can check this balance e.g.

function getContractBalance() public view returns(uint) {
    return address(this).balance;
}

You also need to keep a record of each individual user address’s share of the total contract balance in a mapping. Effectively, this records and tracks each user’s entitlement to withdraw funds from the contract. We show you how to start doing this, with ether transferred into the contract, in one of the lectures in this section of the course, using the deposit() function.

This is the withdraw() function task we give you in the Transfer Assignment near the end of this section of the course. Continue with the video lectures, complete the Transfer Assignment, and then, if you still have questions about how an external address can withdraw the ether they have deposited in the contract, let me know and I’ll be glad to help you further.

Also, don’t forget to post your solutions to all of the assignmnets, here in the forum. A link to where you need to post each of your solutions is provided in the relevant lesson of the course. This is important, because we will review your solutions and, where necessary, provide you with useful feedback. The assignments you should have already completed by the beginning of the Payable Functions section of the course are the Data Location Assignment and the Events Assignment.

Im not getting the same result as Philip on the lessons Payable and Transfer

I can not put a value in the “value” box, and then push the red deposit button. I need to wrtite the amount in the Deposit button as sown in picture.

The withdraw function also does not seem to deploy

Heres is the code im using

pragma solidity 0.8.7;

contract HelloWorld {

    mapping(address => uint) balance;

    address owner;

    event depositDone(uint amount, address depositedTo);

    modifier onlyOwner {
        require(msg.sender == owner );
        _;  //run the function
    }

    constructor() {
        owner = msg.sender;
    }

    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 returns (uint) {
        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, "Dont 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;
    }
}

Hi @Tomaage,

From the screen shot it looks like you still have a different contract deployed to the one actually displayed in the text editor, and you’re calling functions on that other contract instead.

If you look at the panel with the function-call buttons for your deployed contract, your red deposit button has an input field for a uint _toAdd argument. This must be from the previous contract we coded which had the addBalance() function, and where we input unsigned integer values instead of ether values. If you still have that contract deployed, then this will be why it’s not letting you call the deposit function with an ether value in the Value field. As you’ve realised, it requires a uint argument in the input field instead. When you’ve deployed the contract you want to interact with, the red deposit button should appear without any input field next to it, because the deposit() function in this contract doesn’t have any parameters defined within the parentheses immediately after the function name in the function header.

You can also see that your displayed contract has a withdraw() function, but the transfer() function-call button is the only other one displayed in the panel for the contract which is actually deployed.

It’s a good idea to have Auto-compile turned on: mark this option under Compiler Configuration in the Solidity Compiler panel. This will make sure that any errors are highlighed by the compiler while you’re coding. If you don’t have Auto-compile turned on then you must remember to click the blue Compile button, and check that your contract has successfully compiled, before trying to deploy it. If you still have compiler errors then you won’t be able to deploy your contract until you resolve them.

From looking at your code, I can see that the contract you want to deploy won’t compile, even though you’ve used the same code from the video. This is because you’ve chosen to use the more up-to-date Solidity v0.8 (which is only to be encouraged) instead of v0.7 (the course uses v0.7.5). If you try to compile your contract, you will notice that you get a red compiler error for this line of code …

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, here, with the transfer method). That’s why the code in the video (based on Solidity v0.7 syntax) uses  msg.sender.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. So, in order to get your contract to compile you need to make the following modification to this line of code …

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

You will find that the only difference in syntax between these two versions of Solidity, which affects the code covered in this course, concerns msg.sender. Paying close attention to any compiler errors that you get will help you to easily identify when you need to explicity convert it using payable()

Also, make sure you always remove your previously deployed contract(s) by clicking on the bin icon to the right of where it says Deployed Contracts; or if you only have one contract deployed, clicking on the cross just below the bin icon will also remove the contract. Then, just before you deploy your new contract, make sure it is the one displayed in the Contract field just below the Value field.

Let me know if you experience any further difficulties, or if you have any questions about any of the issues I’ve raised.

1 Like

Thanks

The

made it work.

1 Like

When I deposit 2 eth from adress A, into a function in the smart contract, the ETH arrvies at the smart contract Adress B.
Then I make a call to withdraw the same 2 ETH. I see it arrives at adress A.
Why does the get balance function continue to show 2 ETH? Does it not check the balance that the msg.sender has in the smart contract?

pragma solidity 0.8.11;

import "./Destroyable.sol";

contract Bank is Destroyable {

    mapping(address => uint) balance;

    event depositDone(uint amount, address 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 onlyOwner returns (uint) {
        require(balance[msg.sender] >= amount);
        payable(msg.sender).transfer(amount);
        return balance[msg.sender];
    }

    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, "Dont 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;
    }
}

Hi @Tomaage,

I’ve answered this question in my feedback for your Inheritance Assignment (see below), because I saw your withdraw() function there first. Point (2) specifically addresses the issue you’ve raised here…

Pay particular attention to the sentences that I’ve highlighted.

Point (1) addresses another issue with your withdraw() function that you should also consider.

I think you will find it helpful to also add the following getter to your Bank contract, which will return the contract address balance. You can then see more clearly how this changes in relation to the individual user balances in the mapping (which are returned by the getBalance function).

function getContractBalance() public view returns(uint) {
    return address(this).balance;
}

When you’re ready, post your corrected solution and I’ll check that you’re made the right changes. And just let me know if you have any further questions.


And don’t forget to post your solution to the Events Assignment here. It looks like you may have missed it, because you are missing a Transfer event in your code, and a corresponding emit statement in the transfer() function. This assignment is from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.

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. So remove onlyOwner

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?

Some more question for my understanding. 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? 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? They would not know that you sent it to them in this contract unless you told them, I they would not have the money should the smart contract be hacked.
To make it transfer out of the contract to the recipient external wallet; I would have to make it a payable function? I tried messing around, but could not really make it work

1 Like

Hi @Tomaage,

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 :muscle:

  • 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

:money_mouth_face:

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

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.

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 …

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.

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.

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 :muscle:

2 Likes

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.

1 Like

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 :muscle:

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! :smiley:

1 Like

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

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 ?

1 Like

Hi @seybastiens,

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!

1 Like

Hi again @seybastiens,

Yes, it is.

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 :slight_smile:

1 Like

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

1 Like