Transfer Assignment

Hi @11lll,

You have added both of the essential lines of code needed to solve the problem with the withdraw function :ok_hand:

Your withdrawn event declaration and corresponding emit statement are also both well coded. The emit statement is in the correct position in the withdraw() function body and will log appropriate data when the withdraw() function is successfully executed.

However, 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 you’ve included it in your function header, you should also include a return statement in your function body. You should have got a yellow/orange compiler warning about this.

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

Just let me know if you have any questions :slight_smile:

Hi @_Vincent,

This is great that you’ve come back to experiment and test the code in detail. You’ll learn a lot this way.

Yes, it will work.

These are well coded, and Solidity will automatically create getters for both state variables, which you can call from Remix to retrieve the contract address and the contract balance :ok_hand:

Not sure what you mean by “null value”…?
In the contract code there is no constructor assigning the deployer’s address (msg.sender) to a state variable (e.g. owner). So for the purposes of transacting with this particular contract, the deployer’s address is irrelevant.

If you are calling the transfer function, then you are right to copy one of the addresses from the Account field dropdown for the recipient argument. The uint value you enter for the amount argument is in wei not ether. So, to transfer an amount in ether you need to add 18 zeros! It is annoying but that’s the way it is I’m afraid. We have to do it like this for the amount arguments when we call the transfer() function and the withdraw() function. Solidity can only process integers (i.e.whole numbers with no decimals) so all ether values are in units of wei. When an address sends ether to the deposit() function, Remix gives us the option of entering the value in wei, gwei, finney or ether. This is just for convenience; the actual value sent to the function (and referenced by msg.value) will always be converted to units of wei — in other words, if you enter 1 in the Value field and set the adjacent dropdown to ether, Remix will add on the 18 zeros for you when this value is sent to the function :raised_hands:

After you’ve copied and pasted an address for the recipient argument, you need to make sure that you change the address displayed in the Account field to one which already has funds deposited in the Bank contract and therefore has a positive individual balance recorded in the mapping. When you click the transfer button, the address displayed in the Account field is the msg.sender address which calls the transfer() function and transfers an amount in wei from its own individual balance (its share of the total contract balance) to the recipient’s individual balance. If the displayed address has a zero balance in the Bank contract, this is probably why your transfer() function is reverting and throwing the error “Balance not sufficient”.

I don’t think this will be the problem.

I hope this helps to resolve things, but let me know if you still experience problems, or if you have any further questions.

1 Like

Hi @jon_m, many thanks for your time !
Indeed, after perform some tests, I can confirm this is wei/ether the “issue”, but it’s amazing to see that with transfer() function here, a value of “2” is well transfered, but “1” give the error “Balance not sufficient”.

It helps me a lot too to check the documentation of https://docs.soliditylang.org/en/v0.8.10/index.html and https://remix-ide.readthedocs.io/en/latest/index.html. I encourage everyone to do the same in addition. Courses very well suited, but some details are in official docs.

A last trouble from my own : the contract balance (address(this).balance) is initialized at the deployment of contract at “1 ETH” and it’s impossible by code to update this amount.

At the end, If then, I use deposit function for put 1 ETH in the msg.value of msg.sender, it works.
But, in my mind, I thought, once this msg.value is increased at 1ETH, then the contract balance will be equals to 2 ETH, but it’s not the case.

I think at this level, we have to manage a library with the good mapping of all users belonging to the contract and see if the sum of all balances really equals to the adress(this).balance of our contract.

1 Like

Hi @_Vincent,

Sorry, I misinformed you when I said…

As you say, both of the values for these 2 state variables are set on deployment. This means that the contractAddress getter will correctly return the contract address because this never changes. However, contractBalance will be set and remain at 0, because address(this).balance is only evaluated on deployment. In order to return the contract balance at specific points in time, you need to add the function that I gave you before, which contains address(this).balance in its local scope. This piece of code will then only be evaluated whenever this function is called…

With the code you have at the moment, the contractBalance getter should always return 0 wei (the contract balance on deployment), not the 1000000000000000000 wei (1 ETH) you say it’s returning when you call it… ?

Anyway, if you replace the contractBalance state variable with the function above, you should now be able to do what you say here …

It’s great you’re starting to use and check things in the Solidity documentation. It can take a while to get used to, and it can be difficult to read and understand at first, but with patience and practice you’ll find it a valuable resource.

This shouldn’t be happening. If you check just before transferring 1 ETH (by calling getBalance) that the caller of the transfer() function (msg.sender) has an individual balance in the mapping of at least 1000000000000000000 wei (1 ETH) after any previous transactions, then require() shouldn’t fail and throw the error messge “Balance not sufficient”. Are you sure you’re inputting a transfer amount of 1 + 18 zeros, and no more? It’s easy to make a mistake with the zeros, and if you add 19 instead of 18, it will be trying to transfer 10 ETH instead 1 ETH.

Let me know how you get on resolving these issues.

1 Like

Hi @jon_m,
I made a video on the error, so maybe you’ll understand what happens : https://www.youtube.com/watch?v=r1UKk8aOmwg

This is very strange, because the same actions worked well, unless I made a mistake.
I perform just a transfer, with the right number of zero. And no “payable” term to add in the official source code for transfer() function (I write that, because sometimes error message caught and it relates a missing payable term).

Thanks,
Vincent.

Hi @_Vincent,

Can you post an exact copy of the full code in the contract file you are deploying and interacting with? I also want to deploy and test your actual code myself :slight_smile:

And just to confirm, you should be doing and seeing something like the following…
(I’m going to use 2 different addresses from the Account field dropdown — let’s call them A and B)

  1. Call deposit() function with Address A showing in Account field, and 5 ether in Value field. Make sure the dropdown next to the Value field is set to ether (not wei). No other inputs.
  2. You see Address A’s external account balance (showing in the Account field) reduce by 5 ether (e.g. from 99.9999 to 94.9999)
  3. Call getBalance() with Address A, output 5000000000000000000
  4. Call getContractBalance(), output 5000000000000000000
    EFFECT: Address A’s external balance -5 ether; Contract balance +5 ether (all belongs to Address A)
  5. Call transfer() function with Address A showing in Account field, and 2 arguments: recipient Address B, amount 2000000000000000000
    After you’ve copy-and-pasted Address B to input as the recipient argument, don’t forget to change the address showing in the Account field back to Address A before calling the function
    Do not input a value in the Value field. Your transfer() function should not be marked payable.
  6. The external account balances of both addresses do not change.
  7. Call getBalance() with Address A, output 3000000000000000000
  8. Call getBalance() with Address B, output 2000000000000000000
  9. Call getContractBalance(), output 5000000000000000000
    CUMULATIVE EFFECT: Address A’s external balance -5 ether; Contract balance +5 ether (3 ether belongs to Address A, and 2 ether belongs to Address B)
  10. Call withdraw() function with Address B showing in Account field, and 1 argument: amount 1000000000000000000
  11. You see Address B’s external account balance (showing in the Account field) increase by 1 ether (e.g. from 99.9999 to 100.9999)
  12. Call getBalance() with Address B, output 1000000000000000000
  13. Call getBalance() with Address A, output 3000000000000000000
  14. Call getContractBalance(), output 4000000000000000000
    CUMULATIVE EFFECT: Address A’s external balance -5 ether; Address B’s external balance +1 ether; Contract balance +4 ether (3 ether belongs to Address A, and 1 ether belongs to Address B)
  15. Call transfer() function with Address B showing in Account field, and 2 arguments: recipient Address A, amount 1000000000000000000
    After you’ve copy-and-pasted Address A to input as the recipient argument, don’t forget to change the address showing in the Account field back to Address B before calling the function
    Do not input a value in the Value field. This transaction should not revert, because the require statement should not fail if it is coded correctly).
  16. The external account balances of both addresses do not change.
  17. Call getBalance() with Address B, output 0
  18. Call getBalance() with Address A, output 4000000000000000000
  19. Call getContractBalance(), output 4000000000000000000
    CUMULATIVE EFFECT: Address A’s external balance -5 ether; Address B’s external balance +1 ether; Contract balance +4 ether (all belongs to Address A)

You can then try the same thing with different amounts, and then different transaction combinations.

2 Likes

Hi @jon_m,
many thanks for your process.

Unfortunately, I stuck at the step 5, with following output :
"Note: The called function should be payable if you send value and the value you send should be less than your current balance."
Debug infos : "Transaction mined but execution failed"

Here is my code :

OWNABLE.SOL :

pragma solidity 0.7.5;

contract Ownable
{
    address payable public owner;
    
    address public contractAddress;
    uint256 public contractBalance;
    
    constructor() payable
    {
        owner = msg.sender;
        //contractBalance = address(this).balance;
        contractAddress = address(this);
    }
    
    
     //MODIFIERS
    modifier onlyOwner()
    {
        require(msg.sender == owner);
        _;       
    } 
}

DESTROYABLE.SOL :

pragma solidity 0.7.5;

import "./Ownable.sol";

contract Destroyable is Ownable
{
    function close() public onlyOwner
    { 
        selfdestruct(owner); 
    }
}

BANK.SOL :

pragma solidity 0.7.5;

import "./Destroyable.sol";

  interface GovernmentInterface{
        function addTransaction(address _from, address _to, uint _amount) external;
    }

contract Bank is Destroyable
{
    GovernmentInterface govInstance = GovernmentInterface(0xd9145CCE52D386f254917e481eB44e9943F39138);
    
    constructor() payable {
    }
    
    
    mapping(address => uint) balance;
    
    //EVENTS
    event balanceAdded(uint amount, address indexed depositedTo);
    event depositDone(uint value, address indexed sender);
    event closureDone(uint value, address indexed sender);
    

    //BALANCE
    function addBalance(uint _amount) public onlyOwner returns(uint)
    {
        balance[msg.sender] += _amount;
        emit balanceAdded(_amount,msg.sender);
        
        return balance[msg.sender];
    }
    
    function getBalance(address _address) public view returns(uint)
    {
       return balance[_address];
    }
            
    //address that originally deploys the contract
    function getOwnerBalance() public view returns(uint)
    {
       return balance[msg.sender];
    }
    
    
    function getContractBalance() public view returns(uint) {
    return address(this).balance;
}
    
    //DEPOSIT 
    function depositToOwner() public payable returns(uint)
    {
        balance[msg.sender] += msg.value;
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    //WITHDRAW
    function withdraw(uint amount) public payable onlyOwner returns(uint)
    {
        require(balance[msg.sender] >= amount);
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }

    
    //TRANSFER
    function transfer(address _recipient,uint _amount) public {
        //check balance of msg.sender
        require(balance[msg.sender] >= _amount, "Balance not sufficient");
        require(_recipient != owner, "Don't transfer money to yourself");
        
        uint previousBalance = balance[msg.sender];
        
        _transfer(msg.sender, _recipient, _amount);
        
        govInstance.addTransaction(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;
    }
}

My Solution:

pragma solidity 0.8.9;

contract Bank {

mapping(address => uint) balance;
address owner; 
event depositDone(uint amount, address indexed depositedTo);
//event balanceTransferred(uint amount, address indexed depositedFrom, 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, "Balance not sufficient");
    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, "Balance not sufficient");
     require(msg.sender != recipient, "Don't transfer money to yourself");
     
     uint previousSenderBalance = balance[msg.sender];
     
     _transfer(msg.sender, recipient, amount);
     
    // emit balanceTransferred(amount, msg.sender, recipient);
     
     assert(balance[msg.sender] == previousSenderBalance - amount);
 }
 
 function _transfer(address from, address to, uint amount) private {
     balance[from] -= amount;
     balance[to] += amount;
 }

}

1 Like

Hi @_Vincent,

Good job I asked to see your full code, because I didn’t realise you were including the external function call to the Government contract…

I think what’s probably causing your transfer() function to revert is not changing the Government contract’s address assigned to your govInstance in Bank, after you’ve deployed Government. Have a look at this post, which explains the issue and how to resolve it. It also explains why the error message you’re getting isn’t very helpful.

If that doesn’t work, post your Government contract as well, as that is part of your solution (you should be deploying it before Bank).

Hi @_Vincent,

Other comments and issues with your contracts…

Ownable

(1)

This code can be removed, because it serves no purpose. No value is assigned to it. Only the Bank contract needs to be deployed, because it inherits the necessary functionality from Destroyable and Ownable. So, the getContractBalance() function you’ve included in Bank is sufficient to retrieve the deployed Bank contract’s balance at any specific point in time.

(2) You don’t need to use the constructor to assign the contract address to the contractAddress state variable on deployment. The following line of code will do this more concisely…

address public contractAddress =  address(this);

Because this state variable is inherited, address(this) will capture the address at which the derived contract (which inherits this state variable) is deployed.

(3)  The constructor doesn’t need to be marked payable

Destroyable  :white_check_mark:

Bank

(1) You don’t need another constructor in Bank.

(2) This getter …

… will return the balance of any address that calls it, and not just the contract owner’s balance. To allow anybody to check the contract owner’s balance, you would have to look up the owner 's balance in the mapping (not msg.sender 's) …

return balance[owner];

(3) The addBalance() function is not right in this contract, and should have been removed when the deposit() function was added. The addBalance() function was for adding non-ether amounts before we learnt how to send ether to a contract using a payable function. The increase to a user’s individual balance in the mapping, by the same amount as the ether sent to the function and deposited in the contract, is now being performed in the deposit() function. Having both functions allows the contract owner to inflate their individual balance in the mapping, by calling the addBalance() function, and then to withdraw more than their share of the total contract balance, effectively stealing other users’ funds.
The associated balanceAdded event and corresponding emit statement should, therefore, also be removed.

(4) The closureDone event is not implemented anywhere in the contract, and so it’s redundant code.

(5) 

Additional information

(6) By adding the onlyOwner modifier to the withdraw() function, only the contract owner can withdraw their funds. 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 should be able to withdraw their funds as well, don’t you think? :sweat_smile:
On this note, you should change the name of your deposit() function. At the moment it is depositToOwner(), which isn’t true. The function caller deposits funds to their own individual balance (not to the contract owner’s).

(7) 

Any address with funds deposited in the contract can make an internal transfer, so we want this require statement to prevent the function caller from transferring funds to themselves. But your condition doesn’t do that; it prevents any function caller from transferring funds to the contract owner.

(8)  If you want to end up with a complete contract, then you are missing the transfer event from the Events Assignment. I think you may have missed it. The task was to add code, so that each successful transfer logs an event with appropriate data.

1 Like

Nice solution @skawal22 :ok_hand:

You have added all of the additional lines of code needed to solve the problem with the withdraw function.

Now 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.

Just let me know if you have any questions :slight_smile:

1 Like

@jon_m, Thanks again. And i appreciate your suggestion. I read the link post and now understand why you added suggestions there. :slight_smile:

1 Like

Hi @jon_m,
thanks for your steps.

Tried again and it works with some adjustments :
Step 5 resolved but removing government instance part (not necessary in my case, because this training happens before using governmentinstance in ETH courses).

Got issue with withdrawal and send transfer operation, but it’s logic and the issue coming from modifier onlyOwner.
This is not an issue, but we can’t withdrawal if we are not the owner, and we can’t perform a “reverse” transfer, cause to onlyOwner modifier.

Thanks again for your time @jon_m,
some feedbacks :
Ownable (3) I set the constructor with “payable”, because without that, we can’t deploy the contract with a value in “Value” fields. I notified it during my first tests.

Bank (6) Hum… it’s my new bank concept : only Owner can withdraw funds :sweat_smile:, you right ! I checked that, in my previous post with my tests :ok_hand:t2:

Thanks for all your taughts, missing notions are now acquired :ok_hand:t2:

1 Like

My solution for the assignment.

pragma solidity 0.8.9;

contract Bank {
    mapping(address => uint) balance;
    address owner;
    event DepositDone(uint amount, address indexed depositedTo);
    event TransferDone(uint amount, uint balance, address indexed transferedTo);
    event WithdrawDone(uint amount, uint balance, address indexed withdrawnTo);
    
    modifier onlyOwner {
        require(msg.sender == owner, "You are not the 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 getBalance() public view returns (uint) {
        return balance[msg.sender];
    }
    
    function transfer(address _recipient, uint _amount) public {
        require(balance[msg.sender] >= _amount, "Insufficient balance");
        require(msg.sender != _recipient, "Don't send money to yourself.");
        
        balance[msg.sender] -= _amount;
        balance[_recipient] += _amount;
        
        payable(_recipient).transfer(_amount);
        
        emit TransferDone(_amount, balance[msg.sender], _recipient);
    }
    
    function withdraw(uint _amount) public {
        require(balance[msg.sender] >= _amount, "Insufficient balance");
        
        balance[msg.sender] -= _amount;
        
        payable(msg.sender).transfer(_amount);
        
        emit WithdrawDone(_amount, balance[msg.sender], msg.sender);
    }
}
1 Like
pragma solidity 0.7.5;

contract Bank{
    
    mapping(address => uint) balance;
    
    address owner;
    
    event depositeDone(uint amount, address indexed depositedTo);
    event withdrawDone(uint amount, address indexed withdraw);

    
    modifier onlyOwner {
        msg.sender == owner;
        _;
    }
    
    constructor(){
        owner = msg.sender;
    }
    
    function deposit() public payable returns(uint){
        balance[msg.sender] += msg.value;
        emit depositeDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public{
        require(amount <= balance[msg.sender], "You haven't sufficient balance");
        msg.sender.transfer(amount);
        emit withdrawDone(amount, msg.sender);
        balance[msg.sender] -= amount;
    }
    
    function getBalance() public view returns(uint){
        return balance[msg.sender];
    }
    
    function transferTo(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't send 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;
    }
}
1 Like

Hi @_Vincent,

Yes, it will be easier to get it all working first without the extra complication of the Government instance and external function call. You can add this in afterwards.

You should remove onlyOwner from the withdraw function header for the following reason …

Do you mean the transfer of ether out of the contract to the external address, which happens as part of the withdraw transaction, i.e. msg.sender.transfer(amount) ?

By “reverse” transfer do you mean step 15? …

There is no reason why this shouldn’t work, if your first transfer now works in step 5. The onlyOwner modifier isn’t applied to the transfer() function. Make sure you have Address A in the recipient argument field, and Address B showing in the Account field, before calling the function. Or is the issue something else?

1 Like

Hello @jon_m,

Yes, it was not possible for I doing a transfer as a non Owner, but logic regarding the onlyOwner modifier.

Exactly, even it’s not the right term to use “reverse”.

Issue something else finally, but to be honest I don’t remember cause I’m working on the final project for ETH 101.
Sure, it was a little trick :).

Many thanks Jon :ok_hand:t2:

1 Like

The idea isn’t for the owner to send ether to the contract on deployment. If you do that, you will also need to include a line of code in the constructor which increases the owner’s individual balance in the mapping by the same amount, otherwise the owner won’t be able to withdraw those funds (unless they destroy the contract) or transfer them internally to another user. We have the deposit() function for all users (including the owner) to deposit ether in the contract. You should use this for deposits, because it contains all of the correct functionality for this to be done correctly and securely.

So, you can deploy the contract with Address A (which will be the owner). Then Address A can immediately call the deposit() function to deposit the same amount of ether you were previously depositing via the constructor.

:joy:  Ahhh … OK … so, does this now clear up the following issue you raised in your first post?

1 Like

Thanks for first tip about preventing to deploy with an Ether value and, instead of use deposit() function.

All is right now :ok_hand:t2: :ok_hand:t2:

Thanks @jon_m.

1 Like