Inheritance Assignment

[josejalapeno]

I am so glad to have finally arrived at my final answer, thank God I am really getting this, I am sorry for the deleted replies. Please verify if my code is true, I really hope to fully understand programming and to also write on my own in the future and to also make a living out of it.

So here’s the code:
Bank Contract:

pragma solidity 0.7.5;

import "./own.sol";
import "./Destroyable.sol";

contract Bank is Own, Destroyable

Own Contract: (changed it to own, cause there’s already an ownable JSON so I think its auto filling it)

pragma solidity 0.7.5;


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

Destroyable Contract:

pragma solidity 0.7.5;

import "./own.sol";

contract Destroyable is Own{

    function close() public onlyOwner {
    selfdestruct(owner);
    }
}

[jon_m]

Great solution @Lennart_Lucas

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

@Lennart_Lucas

Bank inherits ownable through destructible …

Keep up the great coding!

[jon_m]

Hi @zlr1984,

You have a couple of issues here, which are preventing your code from working as it should…

(1)

Your constructor in Ownable needs to assign msg.sender to the owner state variable, not compare them as it does here …

@zlr1984

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

At the moment, no one can call selfdestruct(), because the owner state variable hasn’t been assigned an address. So calling close() will always trigger revert and throw an error.

(2)

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. With Solidity v0.7 the constructor can assign msg.sender to owner without having to explicity convert it.

// Solidity v.0.7

address payable owner;

constructor() {
    owner = msg.sender;
}

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

// Solidity v.0.8.7

address payable owner;

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

Once you have corrected the operator in the constructor, you will get a red compiler error for this second issue.

What similar change do you have to make in your Bank contract when you use v0.8 instead of v0.7 ?
(Hint: you need to make an additional modification to your solution to the Transfer Assignment)

---

Everything else is well coded. 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

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

[jon_m]

This is a very good solution @josejalapeno

You’re making great progress!

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, as follows:

own.sol

contract Own { ... }

Destroyable.sol

import "./own.sol";
contract Destroyable is Own { ... }

Bank.sol

import "./Destroyable.sol";
contract Bank is Destroyable { ... }

Just let me know if you have any questions

[jon_m]

Hi @JJ_FX,

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

1. your code compiles;
2. your inheritance structure works;
3. only the contract owner can destroy the contract; and
4. 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 …

@JJ_FX

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

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

1. no longer using the in-built functionality of selfdestruct() to transfer the remaining contract balance to its single address argument; and
2. 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]

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]

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]

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

[Markus_Bielaszka]

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]

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]

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]

Hi again, Markus,

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

@Markus_Bielaszka

function destroyContract() external onlyOwner {
      selfdestruct(payable(owner));
  }

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) …

@jon_m

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]

Thanks Jon!

@jon_m

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?

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]

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

[PaulS96]

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]

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]

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.

---

@13zebras

#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?

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 …

@13zebras

#1. Why leave msg.sender in every function?

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]

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]

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]

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 {