Transfer Assignment

[Klem]

Hi @jon_m ; thank you very much for the detailed explanations. I truly appreciate the love, care and attention to detail you all put in the academy

Thank you for your learning tips as well, I will most surely follow them. My learning style is to race forward to get the big picture, but always refer back to cement the individual details.

I’m hoping more exposure to code will quickly make me understand more. The Javascript course was absolutely wonderful ! Love all the resources listed there; I’m getting the recommended book for structuring code for sure !

I’m doing the multisig wallet task right now without referring ahead before trying on my own; I don’t think I’m there yet to finish, but it will be exciting to try for sure ! The bigger tasks are exciting !

Thank you again and take care

[jon_m]

Hey Ernest,

That’s great if you understand all of those points now, because a lot of what comes next in Solidity will build on these fundamentals. Understanding it is often harder than being able to just code it, but it’s worth spending the time to really think things through, because then you will make better progress with the more advanced material

Your modified solution is good. Just a couple of further comments…

• You’ve added an onlyOwner modifier (which wasn’t in your original solution), but this means that only the contract owner can withdraw funds up to their own individual balance. But the contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so it only seems fair that all users will be able to withdraw their funds as well

• Your assert statement now operates correctly, but you only need the arithmetic operator (-) instead of the -= assignment operator, because there is no point assigning a value to the local variable balanceBeforeWithdraw as well.

Let me know if you have any questions about these extra points

[jon_m]

Hey @Klem,

You’re very welcome! I’m glad you find the feedback and learning tips helpful

@Klem

My learning style is to race forward to get the big picture, but always refer back to cement the individual details.

Yes, we all learn in different ways, and the key is to know which learning style benefits you the most and suits you best.

@Klem

I’m hoping more exposure to code will quickly make me understand more.

Yes, exposure is key. It also needs to be productive exposure, meaning that you really think about and analyse some of the code you read, follow-up on new syntax you come across or things you don’t understand, and experiment with some of the new ideas you see used by others.

@Klem

I’m doing the multisig wallet task right now without referring ahead before trying on my own

Yes… really try hard to do as much as you possibly can on your own first (including some research). Then when you look at the code in the support and follow-up videos, if it’s different to your code, you should spend some time analysing and thinking about it. This is a really important stage in the learning process. You can learn a lot by working out what the suggested code does and how it does it, and also how to go about “bridging the gap” from what you managed to do (or not) on your own. However, it’s important to remember that there are nearly always several different alternative approaches and solutions, so if you’ve coded yours differently, but it still works, then it may well be valid. If you’re not sure, then post it here in the forum, and we’ll review it for you.

[jon_m]

Hey Joseph,

Your withdraw function looks great, and is now a work of art as well as a function

And it looks like you’ve been doing some great research. I’m glad my feedback has been helpful in pointing you in the right directions From what you’ve written in your post, your understanding and general coding awareness seems to be improving at a rapid pace

By the way the correct term is re-entrancy attack (not re-entry).

[michasuen]

function transfer(address recipiient, uint amount) public{
require[balance[msg.sender] >=amount;

[loso]

pragma solidity 0.7.5;

contract TakeMoney {
    mapping(address=>uint) balance;
    
    address owner;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    modifier onlyOwner {
        require(msg.sender== owner);
        _;
    }
    
    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){
        require(balance[msg.sender]>= amount,"Declined: Insufficient funds");
        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,"Get your money up");
        require(msg.sender!= recipient, "Stop sending money to yourself");
        
    }
}

[jon_m]

Hi @taodai,

@taodai

Idk why but in pragma solidity 0.8.0 msg.sender isn’t a payable address

That’s because from v0.8 msg.sender is by default a non-payable address, whereas in prior versions it is payable by default. You can find all of the changes that were implemented for each new version in the Solidity documentation, and this particular one is explained in the 9th bullet point in the New Restrictions section of Solidity v.0.8.0 Breaking Changes, which you will find here:  https://docs.soliditylang.org/en/v0.8.4/080-breaking-changes.html#new-restrictions.

@taodai

… do i have to declare an address and convert msg.sender to a payable address?

As you can see in the same bullet point in the documentation I’ve linked to above, the actual explicit conversion of msg.sender into a payable address is done as follows:

payable(msg.sender);

This synatx has been used to convert non-payable addresses to payable ones since Solidity v0.6. You could then assign this to a variable, but it is often not necessary. For example, in your withdraw() function you can use it directly as follows:

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

---

Apart from this, your solution to the assignment is very well coded You have added all of the additional lines of code needed to solve the problem with the withdraw function, and all of the statements in the function body are in the correct order to reduce security risk:

1. check inputs (require statements)
2. effects (update the contract state)
3. external interactions

It’s important to modify the contract state for the reduction in the balance…

balance[msg.sender] -= amount;

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

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

… just in case there is an attack after the transfer, but before the state is modified to reflect this operation. You’ll learn about the type of attack this prevents, and how it does it, in later courses. We wouldn’t expect you to know this at this early stage, though, so I’m just telling you for extra information, in case you haven’t already seen this explanation in some of the other posts in this discussion topic. But it’s great that you’re already getting into good habits in terms of smart contract security

Let me know if anything is unclear, or if you have any further questions.

You’re making great progress! 

[jon_m]

@taodai

It says “send” and “transfer” are only available for objects of type “address payable”.

That’s correct …and which is why…

msg.sender.transfer(amount); // CORRECT before v0.8 but INCORRECT from v0.8

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

[Ernest_SG]

Hey Jon,

Thanks a lot for the feedback, truly helps a lot. Well to be honest I think I didn’t notice that the onlyOwner modifier was there, obviously it’d be very shitty bank if people were never able to withdraw their money . Regarding the second point you made, I think is force of habit, I should probably stop more often and reflex on the use of even this operators.

Once again appreciate your help a ton!

[jon_m]

Hi @michasuen,

This assignment involves completing the withdraw() function, not the transfer() function. It is called the Transfer Assignment because the withdrawal amount is transferred from the contract address to the caller’s external wallet address using   msg.sender.transfer(amount);   This is the code that was given to you in the introductory video. The withdraw() function code that you need to start with is…

function withdraw(uint amount) public returns(uint) {
    msg.sender.transfer(amount);
}

Your require statement is nearly correct…

@michasuen

require[balance[msg.sender] >=amount;

…but you need to pay more attention to the brackets/parentheses. A require() statement is a function call, and so its arguments need to be enclosed in parentheses…

require(balance[msg.sender] >= amount);
    ^                           ^

As well as the require statement, there are two other important lines of code that you need to add. Watch the introductory video again, and have another go. If you get stuck you can have a look at some of the other students’ solutions, and the comments and feedback they’ve received, here in this discussion topic.

Let me know if anything is unclear, or if you have any questions about what you need to do

[jon_m]

Hi @loso,

The require statement you’ve added is correct, but your withdraw function is missing 2 other important lines of code…

• If you deploy your contract, make a deposit, and then call your withdraw function as it is, you will notice that the caller’s external wallet address receives the requested withdrawal amount, but their balance in the mapping is not reduced! I’m sure you can see that this is another very serious bug!
  msg.sender.transfer(amount) transfers ether from the contract address balance to the caller’s external wallet address, but it doesn’t adjust the individual user balances in the mapping. These balances perform an internal accounting role, and record each user’s share of the contract’s total ether balance. So, just as we increase a user’s individual balance when they deposit ether in the contract, we need to do the opposite whenever they make a withdrawal.

• Notice that the withdraw function header also includes returns(uint) . This is not mandatory to include, and the function can still operate effectively without returning a value. But as it’s been included in the function header, you should also include a return statement in the function body. You should have got a yellow/orange compiler warning for this.

Once you’ve made the above modifications to your code, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

By the way, your transfer function is missing a lot of code…

Let me know if anything is unclear, or if you have any questions about these points

[Acex13]

function withdraw(uint amount) public returns(uint) {
        
        require(amount <= balance[msg.sender], "Cannot withdraw more than your current balance. First, add more crypto");
    
        msg.sender.transfer(amount);
        emit withdrawAmt(amount, msg.sender);
        return balance[msg.sender];

[Acex13]

My last reply was incorrect. I was getting the error handling correct, but the balance was not working. I was unclear as to whether the “transfer” function automatically did the math (which didn’t make sense to me). Added the math and now the balances work out:

function withdraw(uint amount) public returns(uint) {
        
        require(amount <= balance[msg.sender], "Cannot withdraw more than your current balance. First, add more crypto");
    
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        
        emit withdrawAmt(amount, msg.sender);
        return balance[msg.sender];
    }

[andrewwy]

In my code below, I have deducted the balance from the sender’s address. The “require” statement prevents the user from withdrawing an amount more than he/she has. I also used emit to fire an event and added an assert statement as a check.

event withrawalDone(uint amount, address indexed addressWithrawnFrom);

function withdraw(uint amount) public returns (uint) {
        require(balance[msg.sender] >= amount, "Cannot withdraw. Insufficient balance.");
        uint prevBalance = balance[msg.sender];
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        emit withrawalDone(amount, msg.sender);
        assert(balance[msg.sender] == prevBalance - amount);
        return balance[msg.sender];
    }

[Anvar_Xadja]

Hey guys. So in my code here, I allowed anyone to withdraw to any specific address with the amount that not less than user has in balance like in many wallets or exchanges. Also I added events to the withdraw function.
I feel I’m growing here. Thank you @filip , you are explaining really well!

pragma solidity 0.7.5;

contract Bank {
    
    mapping(address => uint) balance;
    address owner;
    
    event depositDone(uint amount, address indexed sender);
    event transferDone(uint amount, address indexed sender, address indexed recipient);
    event withdrawDone(uint amount, address indexed sender, address indexed recipient);
    
    modifier onlyOwner() {
        require(msg.sender == owner, "Only owner of the contract can add balance");
        _;
    }
    
    modifier sufficient(uint amount) {
        require(balance[msg.sender] >= amount, "Insufficient funds");
        _;
    }
    
    modifier isSenderEqualReceiver(address recipient) {
        require(msg.sender != recipient, "Can not transfer to the address you are sending from!");
        _;
    }
    
    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, address payable recipient) public sufficient(amount) returns(uint) {
        // msg.sender is an address
        
        recipient.transfer(amount);
        balance[msg.sender] -= amount;
        emit withdrawDone(amount, msg.sender, recipient);
        
    }

[jon_m]

An excellent assignment solution @Acex13

Well done for working out the issue with your first attempt. That’s really good problem-solving!

@Acex13

I was unclear as to whether the “transfer” function automatically did the math (which didn’t make sense to me).

As I’m sure you’ve already worked out,  msg.sender.transfer(amount)  transfers ether from the contract address balance to the caller’s external wallet address, but it doesn’t adjust the individual user balances in the mapping. These balances perform an internal accounting role, and record each user’s share of the contract’s total ether balance. So, just as we increase a user’s individual balance when they deposit ether in the contract, we need to do the opposite whenever they make a withdrawal.

---

You have added all of the additional lines of code needed to solve the problem with the withdraw function, and all of the statements in the function body are in the correct order to reduce security risk:

1. check inputs (require statements)
2. effects (update the contract state)
3. external interactions

It’s important to modify the contract state for the reduction in the balance…

balance[msg.sender] -= amount;

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

msg.sender.transfer(amount);

… just in case there is an attack after the transfer, but before the state is modified to reflect this operation. You’ll learn about the type of attack this prevents, and how it does it, in later courses. We wouldn’t expect you to know this at this early stage, though, so I’m just telling you for extra information, in case you haven’t already seen this explanation in some of the other posts in this discussion topic. But it’s great that you’re already getting into good habits in terms of smart contract security

The withdrawAmt event you are emitting is also a nice addition. It’s in the correct position within your withdraw function body, and it will log relevant information when a call to the withdraw function has executed successfully. The emit statement looks to be coded correctly. Can we also see the event declaration that it’s based on?

Let me know if you have any questions

[jon_m]

An excellent solution @andrewwy

You have added all of the additional lines of code needed to solve the problem with the withdraw function, and they are in the correct order to reduce security risk:

1. check inputs (require statements)
2. effects (update the contract state)
3. external interactions

It’s important to modify the contract state for the reduction in the balance…

balance[msg.sender] -= amount;

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

msg.sender.transfer(amount);

… just in case there is an attack after the transfer, but before the state is modified to reflect this operation. You’ll learn about the type of attack this prevents, and how it does it, in later courses. We wouldn’t expect you to know this at this early stage, though, so I’m just telling you for extra information, in case you haven’t already seen this explanation in some of the other posts in this discussion topic. But it’s great that you’re already getting into good habits in terms of smart contract security

The additional assert statement and withdrawal event (with corresponding emit statement), which you’ve added, are also appropriate and well coded. The emit statement will log relevant information when a call to the withdraw function has executed successfully. The only thing I would say, is that the emit statement is probably better positioned after the assert() statement, rather than before.

Let me know if you have any questions

[jon_m]

Hi @Anvar_Xadja,

@Anvar_Xadja

So in my code here, I allowed anyone to withdraw to any specific address … like in many wallets or exchanges.

Very nice adaptation/extension Also, well done for realising that the external address, which the contract transfers the ether to, needs to be a payable address:

<address payable>.transfer(uint256 amount);

BeforeSolidity v0.8 msg.sender is payable by default, which is why we don’t have to convert msg.sender to a payable address when the user withdraws ether from the contract to their own external wallet. However, from v0.8 msg.sender is non-payable by default…

msg.sender.transfer(amount); // CORRECT before v0.8 but INCORRECT from v0.8

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

 require() statement — effective use of the sufficient modifier to implement the same require() statement used in the transfer() function.

  balance[msg.sender] -= amount;   correctly adjusts the sender’s individual balance recorded in the mapping, which represents their share of the contract’s total ether balance.

  Notice that the withdraw function header also includes returns(uint). This is not mandatory to include, and the function can still operate effectively without returning a value. But as it’s been included in the function header, you should also include a return statement in the function body. You should have got a yellow/orange compiler warning for this. Can you add an appropriate return statement?

  The withdrawal event (with corresponding emit statement), which you’ve added, is also appropriate and well coded. The emit statement will log relevant information when a call to the withdraw function has executed successfully.

Also, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

Let me know if anything is unclear, or if you have any questions.

You’re making great progress

[ibn5x]

Here is my proposed solution to the transfer assignment.

pragma solidity 0.8.1;

contract TwentySevenSoundsBank
{
    mapping(address => uint) balance;
  
    address owner;
    uint contractValue;
    
    constructor () 
        {
            owner = msg.sender;
        }
        
        modifier onlyOwner
            {
                require(msg.sender == owner);
                _;
            }
            
        event depositAdded(uint amount, address indexed depositedToo); //this defines our event    
        event amountTransfered(address indexed sentFrom, uint amountSent, address sentToo);
        
    function getContractsBalance()public view returns(uint)
        {
            return address(this).balance;
        }    
    
    function deposit() public payable returns(uint) 
        {
            
            balance[msg.sender] += msg.value; //not needed we used to internally track who we owe funds too. 
              
            emit depositAdded(msg.value, msg.sender);
            return balance[msg.sender];
        }
        

        
    function withdraw(uint amount) public returns(uint)
        {
           require(balance[msg.sender] >= amount, "balance insufficient.");
          
          
          address payable userAddress = payable(msg.sender);
          balance[msg.sender] -= amount;
          userAddress.transfer(amount);
          return balance[msg.sender];
        }
        
        
    function getBalance() public view returns(uint)
        {
            return balance[msg.sender];
        }  
        
  /*  function removeBalance(uint _subNum) public returns(uint)
        {
            balance[msg.sender] -= _subNum;
            return  balance[msg.sender];
        } 
  */
        
    function transfer(address recipient, uint amount) public 
        {
                //check balance of msg.sender 
                require(balance[msg.sender] >= amount, "balance insufficient.");
                require(msg.sender != recipient, "cannot money to yourself");
                
                uint previousSenderBalance = balance[msg.sender];
                
                
                _transfer(msg.sender, recipient, amount);
           
                //event logs  and further checks 
                emit amountTransfered(msg.sender, amount, recipient);
                assert(balance[msg.sender] == previousSenderBalance - amount);
            }
            
    function _transfer(address from, address too, uint amount) private 
        {
                balance[from] -= amount;
                balance[too] += amount; 
            }  
       
}

[Anvar_Xadja]

Ok I will update my code. Thank you for reviewing!