Transfer Assignment

Dustin_Gearhart · June 20, 2021, 6:36pm

pragma solidity 0.7.5;
    
    contract Bank_TransferFunctionExample{
        
        mapping(address => uint) balance;
        
        address owner;

        event depositDone (uint amount, address depositedTo);
        
        event balanceTransfered (uint amount, address transferedTo);
        
        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];
        }
        
//New code here; new withdraw function
        //Adding onlyOwner to function header, a require() condition, user balance update after withdrawl, and a return statement to update balance 
        //This prevents someone from withdrawing someone elses deposits
        function withdraw(uint amount) public onlyOwner returns (uint) {
            require(balance[msg.sender] >= amount);
            balance[msg.sender] -= amount;
            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, "Insufficent Balance");
            require(msg.sender != recipient, "You cannot send funds 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;
            emit balanceTransfered(amount, to);
        }
    }

Looks like onlyOwner was not needed but I’ll leave it as is since it works and I didn’t catch it until after.

jeanphi · June 20, 2021, 11:38pm

Here is my solution.
I’m curious about something. I’m not sure I understand the difference between msg.value and the amount of token one would like to deposit/withdraw if it wasn’t ETH.
In my solution below I call this xToken.
Is there a way for msg.value to be another sort of Token?
Would the withdraw/deposit functions takes args instead of reusing msg.value? @filip mixed both options in the previous video so I’m a little bit confused.
Thanks for your insights!

// SPDX-License-Identifier: GPL-3.0

pragma solidity >=0.7.0 <0.9.0;

/**
* @title Storage
* @dev Store & retrieve value in a variable
*/
contract Storage {

   mapping (address => uint) xTokenBalances;
   
   event moneyDeposited(uint);
   event moneyWithdrawn(uint);
   
   function deposit () public payable returns (uint){
       xTokenBalances[msg.sender]+=msg.value;
       emit moneyDeposited(msg.value);
       return xTokenBalances[msg.sender];
   }
   
   function withdraw () public payable returns (uint){
       require(xTokenBalances[msg.sender]>=msg.value,"Not enough X Token available");
       payable(msg.sender).transfer(msg.value);
       xTokenBalances[msg.sender]-=msg.value;
       emit moneyWithdrawn(msg.value);
       return xTokenBalances[msg.sender];
   }
   
}

jon_m · June 21, 2021, 10:28pm

Nice solution @Woland

You have added all of the additional lines of code needed to solve the problem with the withdraw function. Your additional assert statement is also appropriate and well coded.

However, it is only the address receiving the ether which needs to be payable — not the withdraw function itself. A function only needs to be marked payable when it needs to receive (not send) ether. So, unlike the deposit function, we don’t want to mark the withdraw function as payable in its function header. Marking it as payable doesn’t prevent your code from compiling, or the function from executing, but 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 if someone sends ether by mistake, this amount will still be transferred to the contract address, but won’t be added to the individual account balance of the user who transferred it, which means the user may experience difficulties in having the amount refunded.

Also, have a look at this post which explains an important security modification that should be made to the order of the statements within your withdraw function body.

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

jon_m · June 21, 2021, 11:17pm

Hi @Dustin_Gearhart,

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:

check inputs (require statements)
effects (update the contract state)
external interactions

Just as you have done, 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. But it’s great you’re already getting into good habits in terms of smart contract security

If you think it works with onlyOwner then you haven’t been thorough enough with your testing
The onlyOwner modifier restricts access to the withdraw function to the contract owner, meaning 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
You wouldn’t have noticed this if you only tested the withdraw function using the contract owner address to make withdrawals. This is an important lesson to learn regarding how important it is to test your completed solution under a variety of different scenarios to see if it performs as expected in all of them.

Just a couple of other minor points regarding your comments in your code above the withdraw function…

For the reasons I’ve explained above, the user’s individual balance in the mapping should be updated before the actual withdrawal of funds occurs. Your code does perform these steps in the correct order, but your comment doesn’t reflect what happens in your code.

The return statement doesn’t update the user’s balance (or the contract balance). It simply returns the user’s reduced balance after the withdrawal.

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

Dustin_Gearhart · June 22, 2021, 12:46am

That was incredibly informative! Thank you vey much for taking the time. The deeper understanding of what’s happening beneath the surface is definitely still fuzzy but points of clarification like that really help shape my understanding.

jon_m · June 22, 2021, 1:58am

Hi @jeanphi,

Looking now at your solution to this Transfer Assignment, I can clearly see where you misunderstood certain key concepts surrounding the transfer of ether between addresses, and how this is different to how a contract records its users individual balances (which could refer to either tokens, or the user’s share of the contract’s total ether balance).

After having written most of this post, I’ve just seen your reply in the other discussion topic, which seems to confirm that the information and explanations I’ve already given you there have now resolved a lot of the issues with your solution for this Transfer assignment. But I’ll still give you a brief summary of the points that arise from this assignment, so you can check to make sure you now understand what you should have done differently and why

(1)

msg.value references an ether value that is sent to a payable function and is automatically added to the contract address balance. Your withdraw() function is therefore a strange combination of a deposit and a withdraw function, which requires the caller to send ether to the contract in order to make a withdrawal. But the amount withdrawn from the contract balance and added to the user’s external wallet balance will always be exactly the same amount that is deducted from the user’s external wallet balance and added to the contract balance — meaning that the overall net effect in terms of both parties’ ether balances is zero. However, even though both the contract balance and the user’s external wallet balance do not change, the user’s balance in the mapping is still reduced by the amount of ether sent and then received back. So after each withdrawal the user will have effectively received zero ether, but their “entitlement” to a share of the total funds held in the contract will have decreased, while the contract balance doesn’t decrease. So I’m sure you can see who ends up gaining from this!

(2)

The individual user balances for this contract, which are stored in the mapping, reflect ether transactions, not token transactions. That’s why it’s not appropriate to name the mapping something related to tokens. However, we could have a contract which transacts with tokens. For example, these could be ERC-20 or ERC-721 tokens. Such token contracts are built according to rigorous standards and specifications, and are much more complex than our initial, simple HelloWorld contract with the addBalance() and transfer() functions. But the underlying principle is the same. The functions that execute transactions with tokens, and which don’t involve the exchange of ether…
(i) do not need to be payable
(ii) do not not need to reference msg.value
(iii) do not need to use the address member <address payable>.transfer(uint256 amount)
… all of which are only relevant when we are performing transactions with ether.

You need to make a clear distinction between how we use Solidity to program (i) movements of ether (the currency of the Ethereum ecosystem) and (ii) movements of other units of value, such as tokens we have created, and which are governed by their own smart contracts.

(3)

(4)

I wouldn’t use the Storage contract template from Remix for this Bank contract, as it’s more complex than a simple storage contract. Also, the comments at the top of the Storge contract template aren’t relevant to this contract. I would call this contract Bank, or something similar, because that’s a good description of what the contract does.

(5)

Your moneyDeposited and moneyWithdrawn events and emit statements are both coded well enough to the extent that they will work. You have also placed the emit statements in the correct position within their respective function bodies. However, the following improvements can be made:

As well as the deposit or withdrawal amount, both transactions involve another key piece of information — the address of the account holder who is either depositing or withdrawing ether. So this is also useful data to include in each event.
It is helpful to add an identifier (a name) to each event argument (e.g. _amount or _withdrawnBy etc., so that the data is easier to recognise and handle when it is logged or captured in the frontend of a dapp.

(5)

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

I hope you find this a useful summary, just to check that you have now understood all of these issues and concepts, or that you at least have a better understanding. You will need to feel comfortable with these aspects of Solidity before moving on to the 201 course, otherwise things could get a bit

Anyway, just let me know if you have any more questions

jon_m · June 22, 2021, 2:53am

I’ve just read through, again, the questions you asked at the beginning of your post for this assignment, and I think this last point about using arguments/parameters instead of msg.value could probably do with a little extra clarification.

When an address calls a function, this address is also effectively a parameter that can be referenced within the function. Solidity provides us with msg.sender, a “pre-defined parameter” that already comes “built-in” to every function we write. This makes sense because every function has a sender/caller address.

msg.value works in a similar way. Whenever ether is sent to a function, this too is effectively an additional parameter, but we just don’t have to explicitly declare it between the parentheses in the function header. When we make a function payable, Solidity automatically makes msg.value available to us as a way to reference any amount of ether that is sent to the function when it is called.

Other values that we want to pass to a function have to be declared in the usual way as parameters within the parentheses in the function header.

So, this is why the deposit() function doesn’t contain any parameters declared within the parentheses. The function is actually called with 2 values — the caller address, and the ether value sent. However, neither of these need to be declared explicitly as parameters, because they can be referenced within the function body as msg.sender and msg.value respectively.

When it comes to the withdraw() function, we have msg.sender available to us, but as ether is being sent from the contract, and not being received by it, the function should not be marked payable, and msg.value would not reference the amount deducted from the contract balance anyway. For this situation, Solidity provides us with the address member transfer which takes a parameter of data type uint256. As this refers to an amount of ether deducted and sent from the contract, a logical name for this parameter is amount, but we could actually call it anything we like. In order for the address member transfer to know how much ether to deduct from the contract balance and send to the external address it is called on, the withdraw function needs to be called with a uint256 value, and for that we need the parameter declared between the parentheses withdraw(uint256 amount) … but again we could call this parameter anything we want. All we need to do is pass the same identifier/name to the address member: transfer(myChosenIdentifier)

Hopefully, that makes things even clearer

CryptoDiddy · June 22, 2021, 1:03pm

pragma solidity 0.7.5;

contract Bank {

mapping(address => uint) balance;

address owner;

event depositDone(uint amount, address 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];
}

**//Need to make the withdraw button a payable function by using the payable function in the header**
**    function withdraw(uint amount) public payable returns (uint){**
**       **
**       // require will revert the payment if insufficient funds**
**       require(balance[msg.sender] >= amount, "Balance not sufficient");**
**       **
**       balance[msg.sender] -= amount;**
**       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, "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;
}

}

jeanphi · June 23, 2021, 1:33am

Thanks @jon_m for this extensive reply.
I think I made my head around the fundamentals yes, I’ll need to understand very soon how to leverage other tokens on Eth for the purpose of the startup I founded last year. Thanks for your insights. I recently discovered Moralis and it might be the way to go for us, maybe should we have further conversation aside.
I just posted my MultiSig assignment today, If you have a chance to have a look, I implemented it with no help so it is going to be easy for you to figure out if I now understand properly how things are supposed to work.
Cheers
JP

jon_m · June 24, 2021, 12:03am

Hey Jean-Philippe,

Great!

You’ll learn how to implement smart contracts for both ERC-20 tokens and ERC-721 NFTs in the Ethereum Smart Contract Programming 201 course. So all will soon be revealed!

Yeh, Moralis is definitely worth looking at to see if it meets the particular needs of your start-up. If you sign up on the Moralis website, I think you’ll have access to the Moralis Discord channel where you will be able to ask questions about its suitability for your particular use case.

One of my colleagues is currently reviewing the MultiSig assignments, so I’ll ask him to pay particular attention to your solution in terms of what we’ve been discussing

jon_m · June 24, 2021, 12:38am

Hi @CryptoDiddy,

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

check inputs (require statements)
effects (update the contract state)
external interactions

Just as you have done, 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 the courses which follow this one. But it’s great you’re already getting into good habits in terms of smart contract security

There is just one modification that needs to be made to your solution…

It is only the address receiving the ether which needs to be payable — not the withdraw function itself. A function only needs to be marked payable when it needs to receive (not send ) ether. So, unlike the deposit function, we don’t want to mark the withdraw function as payable in its function header. Marking it as payable doesn’t prevent your code from compiling, or the function from executing, but it is bad practice and poor risk management not to restrict a function to non-payable if it doesn’t need to receive ether. For example, if the caller sent an ether value to the withdraw function by mistake (as well as calling it with a withdrawal amount) then this ether would be added to the contract balance and potentially lost to the user, because such a deposit wouldn’t also be added to the user’s individual account balance in the mapping (only the withdrawal amount is deducted from their balance). However, the function is much securer if we leave it as non-payable, because then, if the caller made the same mistake, the function would revert, and the ether would not be sent.

Just let me know if anything is unclear, or if you have any questions

CryptoDiddy · June 24, 2021, 5:19am

Thanks once again for your insightful knowledge… much appreciated
I have updated the code just hope what each line means is correct

function withdraw(uint amount) public returns (uint){
// require will revert the payment if insufficient funds
require(balance[msg.sender] >= amount, “Balance not sufficient”);
// next is modify for reduction in balance
balance[msg.sender] -= amount;
// this will transfer balance
msg.sender.transfer(amount);
// balance returned after transaction
return balance[msg.sender];

szaslavsky · June 25, 2021, 4:30pm

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

2tijan7 · June 25, 2021, 7:11pm

Withdraw function:

function withdraw(uint amount) public returns(uint){
        require(balance[msg.sender] >= amount, "Not enough funds");
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }

AngSin17 · June 26, 2021, 9:44pm

Is this OK?

pragma solidity 0.7.5;

contract HelloWorld {
    mapping(address => uint) balance;
    
    modifier checkWithdrawLimit(uint amount) {
        require(balance[msg.sender] >= amount, "Withdraw limit exceeded");
        _;
    }
    
    event deposited(uint amount, address indexed depositedTo);
    
    function deposit() public payable returns(uint) {
        balance[msg.sender] += msg.value;
        emit deposited(msg.value, msg.sender);
        return getBalance();
    }
    
    function withdraw(uint amount) public checkWithdrawLimit(amount) returns(uint) {
        msg.sender.transfer(amount);
        balance[msg.sender] -= amount;
        return getBalance();
    }
    
    function getBalance() public view returns(uint) {
        return balance[msg.sender];
    }
}

jon_m · June 27, 2021, 3:02am

Hi @CryptoDiddy,

That’s all correct now

On the whole, your comments are good. The only one that I think could be explained a bit better is this one…

This line of code transfers the requested withdrawal amount from the contract address balance to the caller’s external wallet address.

The previous line balance[msg.sender] -= amount; adjusts the user’s individual balance in the mapping by the same amount in order to keep track of their share of the total pooled funds held in the smart contract.

jon_m · June 27, 2021, 3:10am

Your solutions are excellent @szaslavsky @2tijan7

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

check inputs (require statements)
effects (update the contract state)
external interactions

Just as you have done, 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 the courses which follow this one. But it’s great you’re already getting into good habits in terms of smart contract security

2tijan7 · June 27, 2021, 6:56pm

Thanks for feedback @jon_m . I am really excited for future assignments and courses.

jtb · June 28, 2021, 2:13am

}

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

KennethJP · June 28, 2021, 3:27pm

pragma solidity 0.7.5;
contract bank {
    
mapping(address => uint) balance;

    address owner;
    
    event withdrawalDone (uint amount, address indexed withdrawTo);
    event depositDone(uint amount, address indexed depositedTo);
    //Arguments that are indexed are actually indexed by the ethereum nodes... 
    //This means we can use these arguments to search and query for certain events that have happened in the past.
    event transferAdded(uint amount, address indexed depositedTo, address depositedFrom);
    
    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 payable returns (uint){
        require (balance [msg.sender] >= amount, "don't try to steal");
        msg.sender.transfer(amount);
        balance[msg.sender] -= amount;
        emit withdrawalDone (msg.value, msg.sender);
        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, "don't transfer money to yourself");
            
            uint previousSenderBalance = balance [msg.sender];
            emit transferAdded (amount, recipient, 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;
                  
        }
        
    }

think i got this right… any suggestions to better my code?