Transfer Assignment

[jon_m]

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)

@jon_m

We should not mark the withdraw function as payable , because it does not need to receive ether from an external address in order to update the contract balance. If we made it payable , then, 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 there is no accounting adjustment made to the mapping in the withdraw function for such a deposit (only for withdrawals). 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.

---

(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]

@jeanphi

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.

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]

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]

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]

Hey Jean-Philippe,

@jeanphi

I think I made my head around the fundamentals yes

Great!

@jeanphi

I’ll need to understand very soon how to leverage other tokens on Eth for the purpose of the startup I founded last year.

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!

@jeanphi

I recently discovered Moralis and it might be the way to go for us

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.

@jeanphi

I just posted my MultiSig assignment today, If you have a chance to have a look

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]

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:

1. check inputs (require statements)
2. effects (update the contract state)
3. 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]

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]

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]

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]

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]

Hi @CryptoDiddy,

That’s all correct now

@CryptoDiddy

I have updated the code just hope what each line means is correct

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

@CryptoDiddy

// this will transfer balance
msg.sender.transfer(amount);

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]

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:

1. check inputs (require statements)
2. effects (update the contract state)
3. 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]

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

[jtb]

}

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

[KennethJP]

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?

[HemalT]

function withdraw(uint amount) public returns(uint){
require(balance[msg.sender] >= amount, “Insufficient funds”);
balance[msg.sender] -= amount;
msg.sender.transfer(amount);
return balance[msg.sender];

[apmfree]

function withdraw(uint amount) public returns (uint) {

    require(balance[msg.sender] >= amount,"Balance not sufficient");
    msg.sender.transfer(amount);
    balance[msg.sender] -= amount;
    
}

[MattPerry]

added a require() statement to fix the issue.

pragma solidity 0.7.5;

contract Bank {
    
    mapping(address => uint) balance;
    
    address owner;
    
    event depositDone(uint amount, address indexed depositedTo);
    event transferEvent(uint amount, address toAddress, address fromAddress);
    
    
    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) {
       
        require(balance[msg.sender] >= amount, "insufficient funds!");
        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);
        require(msg.sender != recipient);
        
        uint previousSenderBalance =  balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);

        
        assert(balance[msg.sender] == previousSenderBalance - amount);
        
        //event logs and further checks
        emit transferEvent(amount, recipient, msg.sender);
    }
 
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
}

[Flippy]

This is my solution to the assignment.

pragma solidity 0.8.4;

    contract BinanceSmartChain {
            mapping(address => uint) balance;
            address owner;
            
            event depositDone(uint amount, address indexed depositedTo);
            
            modifier onlyOwner {
                require(msg.sender == owner,"You are not allowed to withdraw!!!");
                
                _;
            }
            
            constructor (){
                owner = msg.sender;
            }
            function deposit()public payable returns (uint){
                
                // balance [msg.sender] = balance[msg.sender] + _toAdd; or 
                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,"Insufficient balance.");
                payable (msg.sender).transfer(amount);
                balance[msg.sender] -= 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, "Your balance is insufficient");
                require(msg.sender != recipient,"Error transfering to yourself is not allowed");
                uint previousBalance = balance[msg.sender];
                
                _transfer (msg.sender,recipient, amount);
                
                assert (balance[msg.sender] == previousBalance - amount);
            } 
            function _transfer(address from,address to, uint amount) private {
                
                balance [from] -= amount;
                balance[to] += amount;
            }
    }

[Gomez1333]

function withdraw(uint amount) public returns (uint){
require(amount <= balance[msg.sender],“balance not sufficient”);
uint previousSenderBalance = balance[msg.sender];
msg.sender.transfer(amount);
balance[msg.sender] -= amount;
assert(balance[msg.sender] == previousSenderBalance - amount);
}