Inheritance Assignment

RobynFitzooth · July 28, 2021, 6:54pm

Ownable.sol

pragma solidity 0.7.5;

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

Destroyable.sol

pragma solidity 0.7.5;

import "./Ownable.sol";

contract Destroyable is Ownable {
    address payable private owner;

    constructor() {
        owner = msg.sender;
    }

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

}

Bank.sol

pragma solidity 0.7.5;

import "./Ownable.sol";
import "./Destroyable.sol";

contract Bank is Ownable, Destroyable {
...

Saula_Atama · July 28, 2021, 10:15pm

pragma solidity 0.7.5;
import “./Ownable.sol”;

contract Destoryable is Ownable {

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

// multi-layer structure through Bank > Ownable > Destroyable
// self-destruct written with owners address in the function, should there be any issues, it will return whatever the balance is left back to the owner

jon_m · July 29, 2021, 1:27am

An excellent solution @jacobo

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’re making great progress!

jon_m · July 29, 2021, 2:38am

Hi @runHikeClimb,

Your code compiles

You have coded an inheritance relationship which makes the selfdestruct() functionality in Destroyable available within Bank when Bank is deployed.

When close() is called, the Bank contract is successfully destroyed.

You have defined the owner state variable with a payable address type, so that selfdestruct() is called with a payable address and, therefore, any remaining ether in the contract is transferred to the contract owner when the contract is destroyed.

However, even though any remaining ether in the contract will only be transferred to the contract owner, at the moment anyone can call close(), trigger selfdestruct() and destroy the contract. Part of the assignment is to ensure that only the contract owner can destroy the contract. We already have an onlyOwner modifier defined in Ownable which will provide this restriction, so if Destroyable inherits Ownable we can just apply onlyOwner to close() without having to define the modifier again in Destroyable. Also, when Destroyable inherits Ownable, if you change the visibility of the owner state variable in Ownable from private to internal or public, you won’t need to duplicate it in Destroyable either, because it will be inherited. The constructor also won’t need to be repeated. This is the whole point of inheritance — to make our code more modular and avoid code duplication.

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 about how to make the necessary modifications to your code

Ben17 · July 30, 2021, 1:31am

Bank.sol

pragma solidity 0.7.5;

import “./Ownable_1.sol”;
import “./Destroyable.sol”;

contract Bank is Ownable_1, Destroyable {
…

Ownable.sol

pragma solidity 0.7.5;

contract Ownable_1 {

address owner;

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

}

Destroyable.sol

pragma solidity 0.7.5;

import “./Ownable_1.sol”;

contract Destroyable is Ownable_1 {

function close(address payable owner) public {

selfdestruct(owner);
}

}

jon_m · July 30, 2021, 2:55pm

Hi @RobynFitzooth ,

Your code compiles

You have coded an inheritance relationship which makes the selfdestruct() functionality in Destroyable available within Bank when Bank is deployed.

When close() is called, the Bank contract is successfully destroyed.

You have defined the owner state variable with a payable address type, so that selfdestruct() is called with a payable address and, therefore, any remaining ether in the contract is transferred to the contract owner when the contract is destroyed.

However, even though any remaining ether in the contract will only be transferred to the contract owner, at the moment, anyone can call close(), trigger selfdestruct() and destroy the contract. Part of the assignment is to ensure that only the contract owner can destroy the contract. We already have an onlyOwner modifier in Ownable which is inherited by Destroyable, so you can just apply onlyOwner to close() without having to define the modifier again in Destroyable. And because Destroyable inherits Ownable, if you change the visibility of the owner state variable in Ownable from private to internal or public, and also define it with a payable address type, you won’t need the additional owner state variable in Destroyable, because it will be inherited. You will be able to remove the duplicate constructor you’ve added to Destroyable, as well. This is the whole point of inheritance — to make our code more modular and avoid code duplication.

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 about how to make the necessary modifications to your code

jCA · July 31, 2021, 12:32am

Ownable - PARENT:

// SPDX-License-Identifier: MIT
pragma solidity 0.7.5;  // Tells compiler which version of Solidity being used

contract Ownable {
    address public owner;
    
    modifier onlyOwner {
        require(msg.sender == owner, "Not authorized");
        _; // allows remaining code to run if the above is true
    }
    
    constructor() {
        owner = msg.sender;
    }
}

DESTROYABLE - CHILD:

// SPDX-License-Identifier: MIT
pragma solidity 0.7.5;  // Tells compiler which version of Solidity being used

import "./Parent.sol";

contract Destroyable is Ownable {
    
    function contractDestroyed() internal onlyOwner  {
        selfdestruct(msg.sender);
    }
}

Bank : CHILD - See Bottom of Contract for Destroyable inheritance implementation:

// SPDX-License-Identifier: MIT
pragma solidity 0.7.5;  // Tells compiler which version of Solidity being used

import "./Destroyable.sol";

contract Bank is Destroyable {
    mapping(address => uint) balances;
    
    // events
    event _depositEvent(uint _amount, address indexed _depositedTo);
    event transferredDetails(uint _amount, address indexed _to, address indexed _from);
    event _withdrawEvent(uint _amount, address indexed _to);
    
    
    modifier onlyAddressOwnerFunds(uint _amount) {
        bool isSufficient = balances[msg.sender] >= _amount;
        require(isSufficient, "Insufficient Balance");
        _;
    }
    
    
    function deposit() public payable returns(uint) {
        balances[msg.sender] += msg.value;
        emit _depositEvent(msg.value, msg.sender);
        return balances[msg.sender];
    }
    
    function withdraw(uint _amount) public onlyOwner onlyAddressOwnerFunds(_amount) returns (uint) {
        uint previousBalance = balances[msg.sender];
        balances[msg.sender] -= _amount;
        msg.sender.transfer(_amount);
        assert(balances[msg.sender] == previousBalance - _amount);
        emit _withdrawEvent(_amount, msg.sender);
        return balances[msg.sender];
    }
    
    function getBalance() public view returns(uint) {
        return balances[msg.sender];
    }
    
    function transfer(address _recipient, uint _amount) public {
        require(balances[msg.sender] >= _amount, "Balance insufficient");
        require(msg.sender != _recipient, "Cannot transfer to self");
        
        uint previousBalance = balances[msg.sender];
        _transfer(msg.sender, _recipient, _amount);
        
        // assert should always be true unless the code it is testing against is defective
        assert(balances[msg.sender] == previousBalance - _amount);
        
        //event logs and additional validation
        emit transferredDetails(_amount, _recipient, msg.sender);
    }
    
    function _transfer(address _from, address _to, uint _amount) private {
        balances[_from] -= _amount;
        balances[_to] += _amount;
    }
    
    // Inherited call to Destroyable Contract
    function DestroyAndWithdraw() public {
        contractDestroyed();
    }
}

Naoki_Takahashi · August 1, 2021, 2:04am

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);
}
}

runHikeClimb · August 1, 2021, 2:16am

thank you for the feedback.

jon_m · August 1, 2021, 3:10am

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

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

jon_m · August 1, 2021, 3:31am

Hi @Saula_Atama,

Your Destroyable contract is well coded 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.

jon_m · August 1, 2021, 4:09am

Nice solution @jCA

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

jon_m · August 1, 2021, 5:05am

An excellent solution @Naoki_Takahashi

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

xRave110 · August 1, 2021, 12:34pm

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

xRave110 · August 1, 2021, 12:39pm

Is it necessary to inherit Ownable in this line?

contract Bank is Ownable, Destroyable {

thecil · August 2, 2021, 9:46am

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

Carlos Z

Konzaih · August 2, 2021, 11:44am

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 {

ecbwong · August 3, 2021, 3:38am

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

}

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 { -----++++

Jackthelad · August 3, 2021, 4:42am

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.

thecil · August 3, 2021, 2:45pm

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