Inheritance Assignment

Bank.sol
pragma solidity 0.7.5;
import “./Destroyable.sol”;
//inherits Destroyable which means also inherits Ownable
contract Bank is Destroyable {…

Ownable.sol
pragma solidity 0.7.5;

contract Ownable{
address payable public owner;
// public so the variable “owner” is reusable
// payable so the address can send or receive ether

constructor(){
owner = msg.sender;
}
modifier onlyOwner{
require(msg.sender==owner, “You are not the owner!”);
_;
}}

Destroyable.sol
pragma solidity 0.7.5;
import “./Ownable.sol”;
// inherits from Ownable so close() can inherit variable “owner” and modifier “onlyOwner”

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

1 Like

thank you for the feedback.

1 Like

Hi @Ben17,

Quite a few problems here …

Your close() function is successfully inherited by Bank, and so when Bank is deployed, the close function is available to call, selfdestruct() can be triggered to destroy the Bank contract, and the remaining funds in the Bank contract will be rescued by transferring them to an external address. However, anyone can call close() and they can call it with any address. You are right that selfdestruct() needs to be called with a payable address, so that this address can receive the transferred funds, but just calling the parameter owner does not restrict this to the contract owner’s address. And so, at the moment, any address can call close() and destroy the contract, and the caller can choose to transfer the contract’s remaining funds to any address they want (their own or any other) by inputting any address as the argument. I’m sure you can see that this is the opposite of secure :sweat_smile:

You need to ensure that only the contract owner can destroy the contract and that means only the contract owner’s address should be permitted to call close() and trigger selfdestruct(). You already have an onlyOwner modifier in Ownable_1 which can potentially apply this restriction. It is inherited by Destroyable, but will only restrict access to the contract owner if the contract owner’s address is assigned to the owner state variable on deployment; and for that you need to include a constructor in Ownable_1. Your current code isn’t making any use of the functionality which is inherited from Ownable_1, and so you may as well have omitted Ownable_1 from your inheritance structure completely.

Because only the contract owner’s address should be allowed to call close(), there is no need for a parameter, because this access restriction can be performed by the onlyOwner modifier. So, without the parameter, you need to find another way to ensure that the address passed to selfdestruct() is a payable address. See if you can work that out for yourself. Then, if you still need some help, take a look at some of the other students’ solutions, and the comments and feedback they’ve received, posted here in this discussion topic, to get some ideas.

In terms of how you’ve coded the inheritance relationships, this is mostly good. Just make sure you import the file using the file name and not the contract name. You have called your contract Ownable_1, so the Destroyable and Bank contract headers are correct. But according to your post, the file name is Ownable.sol, but your have imported Ownable_1.sol …

Just let us know if anything is unclear, or if you have any questions about how to make the necessary modifications to your code :slight_smile:

Hi @Saula_Atama,

Your Destroyable contract is well coded :ok_hand: But as you haven’t converted owner to a payable address locally in Destroyable, we need to see how you have coded that in Ownable.
In order for any remaining ether in the contract to be transferred to the address passed to selfdestruct(), this address needs to be a payable address. The compiler will throw an error if it’s not a payable address.

You are right that implementing 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. But your comment suggests you’ve structured this differently…

Can we also see the first few lines of your Bank.sol file, up to and including the contract header, so we can see how you’ve coded Bank’s inheritance relationship with the other contracts? That’s an important part of the solution as well, because it’s the Bank contract that we are deploying.

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

Nice solution @jCA :ok_hand:

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.

However, if you give the inherited function contractDestroyed() public instead of internal visibility, it can be called directly by the contract owner’s external address, without needing to add an additional public function in Bank via which to access it. This would provide exactly the same functionality as your solution, but more efficiently.

By adding the onlyOwner modifier to the withdraw() function, only the contract owner can withdraw funds up to their own individual balance. But the contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so it only seems fair that all users should be able to withdraw their funds as well, don’t you think? :sweat_smile: Your withdrawal() function header didn’t include the onlyOwner modifier before. I think during the course we were adding and removing onlyOwner from various functions to demonstrate various different scenarios, and I think it might have ended up being left in the withdraw() function in the solution code for this assignment, by mistake!

Just let us know if you have any questions. You’re making great progress! :muscle:

An excellent solution @Naoki_Takahashi :muscle:

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

Again, you’ve made a great effort with your comments :ok_hand: Just one observation …

An address only needs to be a payable address to receive ether from the smart contract. In your solution, the owner’s address only needs to be payable, because selfdestruct() requires a payable address argument, so that the remaining contract balance can be transferred to this address if it is destroyed. In fact, an Ethereum address that is saved in an address variable in a smart contract cannot be used to send ether from the smart contract — only the contract address can do that, because any ether sent from the contract will be deducted from the contract’s own account balance.

One final thing… Please format your code before posting it, so it’s clearer and easier to read. Follow the instructions in this FAQ: How to post code in the forum

Let me know if you have any questions :slight_smile:

1 Like

Ownable.sol:

pragma solidity 0.7.5;

contract Ownable {
address payable public owner;

modifier onlyOnwer{
    require(msg.sender == owner, "You do not have permision to do this action");
    _;
}
 
constructor(){
    owner = msg.sender;
}

}

Destroyable.sol:

pragma solidity 0.7.5;

import “./Ownable.sol”;

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

Bank.sol (Edited lines):

import “./Destroyable.sol”;

contract Bank is Destroyable { // Destroyable is enough due to multi-level inheritance

1 Like

Is it necessary to inherit Ownable in this line?

contract Bank is Ownable, Destroyable {

1 Like

Depends on your inheritance distribution, for example if Bank need access to Ownable methods, so Bank must inherit Ownable, while Destroyable do need Ownable, it also can be inherit from Bank.

For example. Taking your code has example:

owner is a public variable in the Ownable which is used in the Destroyable contract, but in case that Bank Contract also needs to access this variable or another method inside the Ownable contract, it will also needs to inherit it.

When you do:

you give access to Bank to Ownable and also Destroyable, at the same time, Destroyable will have access to Ownable.

Hope I explained my self clear, if not let me know to help you :nerd_face:

Carlos Z

destroyable.sol

pragma solidity >0.6.0;

import "./ownable.sol";

contract Destroyable is Ownable{

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

ownable.sol

pragma solidity 0.7.5;

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

helloworld.sol / Bank Contract

 pragma solidity 0.7.5;
  
 import "./destroyable.sol";
 import "./ownable.sol";

contract Bank is Destroyable {
1 Like

1)New File. Delete.sol. (Base contract)

pragma solidity 0.5.12;

Contract Delete {

address public owner;

modifier onlyOwner(){
require(msg.sender == owner); _;

function delete() public onlyOwner{ delete(owner);

}

  1. Helloworld.sol. ( Derived contract ) To inherit into any helloworld functions that require deleting owners.

import “./delete.sol”
pragma solidity 0.5.12;

contract helloworld is delete { -----++++

1 Like

This is quite a delayed follow up, but I have found some stuff in the breaking changes of Solidity v0.8.0, namely;
“Change msg.sender.transfer(x) to payable(msg.sender).transfer(x) or use a stored variable of address payable type.”

I was wondering if rather than having to remember to make the msg.sender payable in the SelfDestruct.sol contract - I know it is payable by default in 0.7.5 which the course uses - could I just make the state variable owner in Owner.sol payable as follows;

pragma solidity >=0.7.0 <0.9.0;

contract Owner {

    address payable public  owner;

Would this allow me to simply define the function body of self destruct as;

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

All based on v0.8.0 +
Perhaps that is obvious, or wrong… any practice is good practice though.

Appreciate any feedback.

1 Like

Yep, completely valid, the new syntax from version 0.8.0 of solidity is for any case that an address must be converted into a payable one, but if you already defined it as payable, there is no need for the convertion.

Carlos Z

I understand that Ownable is needed to be inherited in Bank contract but I think it is not necessary to write:

contract Bank is Ownable, Destroyable {

And it can be just written as below:

contract Bank is Destroyable {

Because if Bank inherits from Destroyable and Destroyable inherits from Ownable, it seems that Bank has also access to public variables defined in Ownable contract due to “multi-level inheritance”. Am I right?

1 Like

Are you saying you don’t necessarily need to assign address payable receiver to msg.sender? You can just streamline it as…

function destruction() public onlyOwner {
selfdestruct(msg.sender);

Also after I tested the functions… deposited money from several different accounts into the bank…, was the owner(msg.sender) suppossed to receive all the funds from the various deposits I made from other accounts after I hit destroy? Or does the account balance set to zero? I thought the owner account would be receiving the total of all the deposits made into the account after the destruct was performed but my balance shows zero after destructing. Thanks again for the clarification.

Indeed, I have made this code to show that Bank can inherit Ownable through Destroyable.

pragma solidity 0.7.5;
// SPDX-License-Identifier: UNLICENSED

// usual owner routines
contract Ownable {
    
    address public owner;
    
    constructor(){
       owner = msg.sender; 
    }
    
    modifier onlyOwner() {
        require(msg.sender == owner,"onlyOwner allowed");
        _;    
    }
}

// destroy contract, must access Ownable to use onlyOwner
contract Destroyable is Ownable{
    function closeContract() public onlyOwner{
        selfdestruct(msg.sender);
    }
}

// bank contract, inherit Ownable through Destroyable
contract Bank is Destroyable {
    
    function deposit() public payable{}
    
    function getContractBalance() public onlyOwner view returns(uint256){
        return address(this).balance;
    }
    
}

Carlos Z

OWNABLE

pragma solidity 0.8.6;

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

DESTROYABLE

pragma solidity 0.8.6;

import "./ownable.sol";

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

BANK

pragma solidity 0.8.6;
import "./destroyable.sol";

contract Bank is Destroyable{
//only change in bank
function withdraw(uint amount) public returns (uint){
        require(balance[msg.sender] >= amount);
        balance[msg.sender] -= amount;
//here (for solidity version 0.8.6)
        payable(msg.sender).transfer(amount);

        return balance[msg.sender];
    }
}
1 Like

contract Ownable{

address owner;

modifier onlyOwner{
    require(owner==msg.sender);
    _;
}

constructor () {
    owner=msg.sender;
}

}

import “./ownable.sol”;

contract Destroyable is Ownable{

function destroyContract() public onlyOwner { 
    selfdestruct(msg.sender); 
}

}

pragma solidity 0.7.5;

import “./destroyable.sol”;

contract Bank is Destroyable {
…
}

1 Like

// In hello world

import “./Destroyable.sol”;

contract Bank is Ownable, Destroyable {

// In Destroyable

import “./Ownable.sol”;

pragma solidity 0.7.5;

contract Destroyable is Ownable {

function destroy() public onlyOwner {
address payable receiver = msg.sender;
selfdestruct(receiver);
}
}

1 Like

Hi Filip! As I look at the solution I can´t recognize this line:

govermentInstance.addTransaction(msg.sender, recipient, amount);

And when I try to implement it I get an error message. Why is that? error message

1 Like