Inheritance Assignment

jon_m · October 25, 2021, 2:58am

You have made an excellent effort with this alternative solution, and the main thing is …

your code compiles;
your inheritance structure works;
only the contract owner can destroy the contract; and
you have successfully achieved your objective: on destruction of the contract, the remaining contract balance is transferred out of the contract and distributed to the external addresses of the individual account holders, each receiving an amount of ether equivalent to the amount they had deposited in the contract at the time of destruction.

We can also streamline the inheritance structure by having Bank explicitly inherit Destroyable only, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.

Unfortuantely, there is a flaw in your code which could potentially result in a user losing ether. If a customer calls the deposit function and sends an ether value, but without having previously registered by calling the registerAccount function, then this ether will still be deducted from their external address balance and deposited in the contract. However, as they haven’t previously registered, the amount deposited in the contract will not be recorded in the mapping as belonging to them. The problem arises, because there is no require statement in the deposit function to revert the transaction if the caller has not yet registered. I explain how you can correct this further below. The withdraw function should also revert if the caller has not yet registered, as at the moment the function still executes successfully, although the consequences are less serious than with the deposit function, because the amount requested is not withdrawn from the contract.

Even though Solidity v0.8 requires msg.sender to be explicitly converted to a payable address when used in the following line of code …

… this doesn’t mean that we have to make the withdraw function itself payable. Only functions that receive ether into the contract (adding it to the contract balance) should be marked payable. Functions that only send ether out of the contract (deducting it from the contract balance) should remain non-payable.

Marking the withdraw function 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. This is because if someone sends ether by mistake, this amount will still be transferred to the contract. As we are programming money, we need our code to be as risk-averse as possible.

In terms of your adaptations for the additional registerAccount function, and your solution for distributing the remaining contract balance amongst the individual account holders, here are some suggestions for improvements …

To avoid having to use a for-loop in nearly every function, you can include a bool property (e.g. bool registered;) in the Customer struct, which is assigned true whenever a new customer registers by calling the registerAccount function. Including this additional property set to true in each new account that is registered, has the following advantages:

You can replace several lines of code in registerAccount() with a single require statement that checks whether the caller has an instance in the mapping with a registered property value of true. If it does, then the caller is already registered, and the require statement should fail and trigger revert. If the caller doesn’t have an instance in the mapping yet, then the require statement should evaluate to true and allow the function to execute and create a new customer instance.
You don’t need to use the mapping like an array, iterating over the uint mapping keys as if they were array indices. You can make the customer’s address the mapping key again, and remove the address account property from the Customer struct.
In the functions where you now don’t need for-loops, you also don’t need to reference the customersCount variable. In fact, for another reason explained below, you can remove the customersCount variable altogether. This also means you can remove the id property from the Customer struct, which you don’t actually use anyway: it’s only there, because for each new customer you are using the assignment of the id property to increment customersCount +1 (this would be better done separately). This will leave the Customer struct with just two properties: uint balance and bool registered
In the registerAccount() function, you don’t need to place the final statement (which creates the new customer instance) within an if-statement, because this condition is now evaluated in the require statement.
In the deposit(), withdraw() and getBalance() functions, instead of using a for-loop to check that the caller is already registered, you can define a separate modifier which checks the registered property of msg.sender in the mapping to see if it is true, and then include this modifier in each of the three function headers. If you include an error message within the modifier’s require statement, there is also no need for the getBalance() function to return 0 if the caller hasn’t registered an account yet — in this particular case, triggering revert and generating an error message would be more appropriate than returning a balance of 0. The deposit and withdraw functions would also then revert and throw error messages if called by an unregistered address, instead of executing successfully and consuming gas unnecessarily. The risk that I mentioned above of an unregistered customer calling deposit() and losing ether would also be prevented.

So, the only place where you need a for-loop is in the closeBank function. If you define an address array (e.g. address[] customerLog) as well as the customers mapping, you can use the array to iterate over in the closeBank function. In the registerAccount function, you can push the caller’s address to the array, as well as creating the Customer instance and assigning it to the mapping. Having an array of customer addresses means that you don’t need the customersCount variable, nor do you need to use uint values for the mapping keys, because the array indices can perform both of these functions e.g.

mapping(address => Customer) customers;
address[] customerLog;

function closeBank() public onlyOwner {
    for (uint i = 0; i < customerLog.length; i++) {
        address toPay = customerLog[i];
        payable(toPay).transfer(customers[toPay].balance);
    } 
    permanentlyCloseBank();
}

In fact, as you are …

no longer using the in-built functionality of selfdestruct() to transfer the remaining contract balance to its single address argument; and
calling the permanentlyCloseBank() function internally from Bank …

… there really isn’t any point in including Destroyable within the inheritance structure. As destroying the contract is now the only operation performed by calling permanentlyCloseBank(), it would be more straightforward just to call selfdestruct() directly at the end of the closeBank function body.

Try implementing these modifications yourself. Just let me know if you need me to clarify anything, or if you have any questions.

You’re making great progress… keep it up!

jon_m · October 25, 2021, 3:29am

Hi @JJ_FX,

A couple of additional comments …

Adding a function to get the total contract balance (as well as the individual user balances with getBalance) makes testing easier e.g.

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

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.

I’ve also just noticed that you didn’t post any solutions for the Events Assignment, or the Transfer Assignment (which focuses on the withdraw function). If you haven’t done these, then I recommend you do, as they cover some important concepts.

josejalapeno · October 25, 2021, 5:31am

oh yes thats true, I havent thought of that because I was amazed on how I understood it and was eager to post my answer. thanks for that Jon!

JJ_FX · October 26, 2021, 6:50am

Thanks for thorough explanaition @jon_m! This will be super useful for me in the future, I even bookmarked your answer

Markus_Bielaszka · October 27, 2021, 2:09am

Bank.sol:

//SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.9;

import "./Destroyable.sol";

contract Bank is Destroyable { ... }

Ownable.sol:

//SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.9;

contract Ownable {

  address internal owner;
    
  constructor() {
      owner = msg.sender;
  }
    
  modifier onlyOwner {
      require(owner == msg.sender, "You are not the owner of this contract");
      _;
  }
}

Destroyable.sol:

//SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.9;

import "./Ownable.sol";

contract Destroyable is Ownable {
    
  function destroyContract() external onlyOwner {
      selfdestruct(payable(owner));
  }
}

jon_m · October 27, 2021, 4:46pm

Great solution, Markus

Your implementation of a multi-level inheritance structure is the best approach, because it is more streamlined. Bank only needs to explicitly inherit Destroyable because it will implicitly inherit Ownable via Destroyable.

By the way, using Solidity v0.8 means that you need to make a change to the syntax in your withdraw function in Bank (your solution to the Transfer Assignment). What’s the change?

13zebras · October 27, 2021, 6:42pm

I’m confused.

#1. What is the purpose of having bank.sol inherit ownable.sol if the state variable owner is never utilized? Why leave msg.sender in every function?

#2. Also, if complied under 0.8.9, I cannot get the withdraw function to work. I can get it to compile, but not execute (I left msg.sender in place):

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

I get the following error:

transact to Bank.withdraw errored: VM error: revert.
revert
The transaction has been reverted to the initial state.
Note: The called function should be payable if you send value and the value you send should be less than your current balance.
Debug the transaction to get more information.

I’ve looked through other’s posts of their code, but I can’t see how to fix this.

#3. Also, how does one debug in Remix? Can you point me to a doc showing this functionality?
EDIT: It’s amazing what a little searching will do:
https://remix-ide.readthedocs.io/en/latest/tutorial_debug.html

#4. Lastly, here is the required code for the assignment:

ownable.sol:

pragma solidity 0.8.9;

contract Ownable {
    
    address payable public owner;
    
    modifier onlyOwner {
        require(msg.sender == owner);
        _; // run the function
    }

    constructor() {
        owner = payable(msg.sender);
    }
}

destroyable.sol

pragma solidity 0.8.9;

import './ownable.sol';

contract Destroyable is Ownable {
    function diediedie() external onlyOwner { 
        selfdestruct(owner); 
    }
}

bank.sol

pragma solidity 0.8.9;

import './destroyable.sol';

contract Bank is Destroyable { ... }

THANKS! . . . Tom

jon_m · October 27, 2021, 11:36pm

Hi again, Markus,

The fact that you gave your destroyContract() function (in Destroyable) external visibility got me thinking … and testing …

I didn’t realise that when you deploy a derived contract, any external functions in the base contract will also be available to call externally. So it seems that I need to modify my feedback for your Inheritance Reading Assignment as follows (changes in bold) …

Inheritance Reading Assignment

The functions which are available for derived contracts (to call internally) are …

the public and/or internal functions defined in its parent/base contract;

any public and/or internal functions inherited by the base/parent contract from its own parent/base contract (the derived contract’s “grandparent”); and

public, private and internal functions (i.e. all except external functions) defined within the derived contract itself.

It’s normally the derived contract that we deploy, not the base contract. In this case, the following functions can be called either …

EXTERNALLY (i) public or external functions defined in the derived contract itself; (ii) public functions which have been inherited; and (iii) external functions defined in the base contract(s);

INTERNALLY from within the derived contract: (i) private, internal or public functions defined in the derived contract itself; and (ii) public or internal functions which have been inherited.

(iii) external functions defined in the base contract(s)
This applies to your destroyContract function in Destroyable, which you have marked with external visibility. On deploying Bank, I would have expected only to be able to call this function externally if it had public visibility. But this clearly isn’t the case.

Thanks for teaching me something new!

Note: I’ve also edited my original post in the Inheritance Reading Assignment discussion topic for these changes.

Markus_Bielaszka · October 28, 2021, 12:39am

Thanks Jon!

Yes thanks, I changed it in my code, but forgot to change it in the assignment post
It’s: payable(msg.sender).transfer(_amount);

Markus_Bielaszka · October 28, 2021, 2:03am

Hi again, I’m glad!
I wasn’t sure of this either, so thanks for explaining it for me, it makes sense

PaulS96 · October 28, 2021, 2:31pm

Ownable.sol

pragma solidity 0.7.5;

contract ownable{
    address payable public owner;
    
    modifier onlyOwner{
        require(msg.sender == owner);
        _;
    }
    constructor(){
        owner = msg.sender;
    }
    
}

Destroyable.sol

pragma solidity 0.7.5;

import "./ownable.sol";

contract destroyable is ownable {
  
    function destroy() public onlyOwner{ 
        selfdestruct(payable(owner));
        
      }
}

Bank.sol

pragma solidity 0.7.5;

import "./destroyable.sol";

contract bank is destroyable{
    
}

jon_m · October 28, 2021, 3:01pm

Another very good solution @PaulS96

Your implementation of a multi-level inheritance structure is the best approach, because it is more streamlined. Bank only needs to explicitly inherit Destroyable because it will implicitly inherit Ownable via Destroyable.

A couple of observations …

You’ve declared your owner state variable in contract Ownable with a payable address type. This means that, in Destroyable, when you call selfdestruct() with owner it is already a payable address, and so doesn’t need to be explicity converted using payable(). Your code still compiles and executes correctly, but adding payable() here is redundant.
However, you would need to convert it to a payable address, as you have done, if you declare owner as a non-payable address in Ownable (i.e. without adding the keyword payable). Which of these two alternatives you choose, depends on how often you need to reference owner as a payable address throught all of the contracts in your inheritance structure.
The convention is to start contract names with a capital letter — the same as with events, structs and interfaces. The names of variables, mappings, arrays, arguments, parameters and functions usually start with a lower case letter. Doing the opposite will not prevent your code from compiling, but following this generally accepted convention does apply a consistency to our code, which can make it more readable.

Let me know if you have any questions.

Keep up the great coding!

jon_m · October 28, 2021, 5:09pm

Hi Tom,

Firstly, the solution you’ve posted at #4 is very good, and all of the code is correct

Your implementation of a multi-level inheritance structure is the best approach, because it is more streamlined. Bank only needs to explicitly inherit Destroyable because it will implicitly inherit Ownable via Destroyable.

You are right that your Bank contract doesn’t need to directly utilize the owner state variable in Ownable. It also doesn’t need to have the onlyOwner modifier available, because this isn’t applied to any of its functions. However, when Bank is deployed, the address that deploys it needs to be assigned to the owner state variable, and for this to happen Bank needs to be able to execute the constructor in Ownable, and for this to happen it needs to inherit it.

Your Destroyable contract needs to inherit both the onlyOwner modifier (to restrict access to your diediedie function) and the owner state variable (to pass to selfdestruct) from Ownable. Passing msg.sender to selfdestruct is equivalent to passing owner, because the onlyOwner modifier restricts access to the diediedie function to the owner address, and so msg.sender can only be the owner address in this function, anyway. But even if we pass msg.sender to selfdestruct() instead of owner, Destroyable still needs to inherit the onlyOwner modifier from Ownable.

Does this answer your Q1? I’m not entirely sure what you mean by …

We still need to reference msg.sender in our Bank contract functions because we want any address to be able to make deposits, withdrawals and transfers. If we referenced owner instead, then only the contract owner would be able to perform these transactions, which would be pretty useless for a bank! By contrast, we only want the contract owner to be able to trigger selfdestruct() and have the ether remaining in the contract transferred to their external address.

13zebras:

#2. Also, if complied under 0.8.9, I cannot get the withdraw function to work. I can get it to compile, but not execute (I left msg.sender in place):
function withdraw(uint amount) public payable returns (uint){
        require(balance[msg.sender] >= amount);
        balance[msg.sender] -= amount;
        payable(msg.sender).transfer(amount);
        return balance[msg.sender];
    }

Apart from the withdraw() function not needing to be marked payable (see below for an explanation of why), your code is correct. But, even though it’s bad practice, marking your withdraw function as payable won’t prevent your code from compiling, or the function from executing, so I can’t see any reason why you are getting the error message. The error message itself is a generic, default one, and that’s why it isn’t helpful in identifying what needs to be corrected. If you add an error message to the require statement, then if the error message you get in the Remix console on revert doesn’t contain this specific text, we can rule out the transaction failing because the withdrawal amount is greater than the caller’s current balance.

If you’re still experiencing the same problem after trying this, then post your full Bank contract so I can deploy and test it myself to find out what’s causing it.

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

Let me know if anything is still unclear, if any of your questions remain unanswered, or if you have any further ones

Flippy · October 29, 2021, 11:02am

Bank.sol

pragma solidity 0.7.5;

import "./Destroyable.sol";

contract Bank is Destroyable {

Ownable.sol

contract Ownable{
    address payable public owner;
    
    modifier onlyOwner {
        require(msg.sender == owner);
        _;
    }
    
    constructor(){
        owner = payable (msg.sender);
    }
}

Destroyable.sol

pragma solidity 0.7.5;

import "./Ownable.sol";

contract Destroyable is Ownable{
    function destroy() external onlyOwner{
        selfdestruct(owner);
    }
}

jon_m · October 29, 2021, 2:35pm

This is a very good solution @Flippy

Your implementation of a multi-level inheritance structure is the best approach, because it is more streamlined. Bank only needs to explicitly inherit Destroyable because it will implicitly inherit Ownable via Destroyable.

Just one observation …

Prior to Solidity v0.8 msg.sender is already a payable address by default. This means that whenever you want to use msg.sender as a payable address you don’t have to explicitly convert it. For example, in Ownable, your owner state variable is declared with a payable address type. As you are using Solidity v0.7.5, the constructor can assign msg.sender to owner without having to explicity convert it …

// Solidity v0.7.5

address payable public owner;

constructor() {
    owner = msg.sender;
}

However, from v0.8 msg.sender is non-payable by default, and so whenever you are using v0.8 and need to use msg.sender as a payable address, this does require an explicit conversion …

// Solidity v0.8

address payable public owner;

constructor() {
    owner = payable(msg.sender);
}

Just let me know if you have any questions.

belinox · October 31, 2021, 3:02pm

pragma solidity 0.7.5;

contract Ownable {
    address payable internal owner;
    
    modifier onlyOwner {
        require(msg.sender == owner);
        _; //run the function
    }
    
    constructor(){
        owner = msg.sender;
    }
}

pragma solidity 0.7.5;

import "./Ownable.sol";

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

pragma solidity 0.7.5;
import "./Ownable.sol";
import "./Destroyable.sol";

contract Bank is Ownable, Destroyable {

jon_m · October 31, 2021, 10:24pm

Nice solution @belinox

We can also streamline the inheritance structure by having Bank explicitly inherit Destroyable only, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.

Let me know if you have any questions

AU-Stephen · November 1, 2021, 9:57am

I had the same solution as @belinox. Good to know that we can just declare Bank is Destroyable if Destroyable is Ownable!

zlr1984 · November 1, 2021, 10:33am

Thanks Jon for guidiance.

I am not sure what modifications I have to make in this case, and there are 2 red errors:

For the Bank.sol I get the red error at:

msg.sender.transfer (amount);

For the Destroyable.sol, I get the red error at:

selfdestruct(owner);

jon_m · November 2, 2021, 2:09am

Hi @zlr1984,

If you are using Solidity v0.8, msg.sender is non-payable by default. The transfer method is called on a payable address …

<address payable>.transfer(uint amount);

… so you need to explicity convert msg.sender …

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

… using the same payable() syntax that you need to use in the constructor in Ownable, if you want to assign the contract deployer’s address to a state variable defined with a payable address type…

This might be because your owner address state variable (in Ownable) is no longer defined as payable: selfdestruct() takes a payable address argument. But I will need to see your latest versions of both the Ownable and the Destroyable contracts to be able to say for certain what is causing this compiler error.