Inheritance Assignment

[jon_m]

Hi @gunnarseay,

You’re nearly there with this: just a few things that need modifying, and some syntax to tidy up…

(1)

@gunnarseay

pragma solidity 0.5.12;

Why are you using compiler v.0.5.12? This is quite out-dated now, so use either v0.7.5 (the Solidity version used in the course videos) or v0.8.

(2) If you use v0.5.12, you need to mark your constructor with public visibility or it will throw a compiler error…

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

It’s only from v0.7 that the visibility is ignored in constructors. So if you use v0.7 or above, your current constructor will compile as it is …

@gunnarseay

constructor(){
    owner = msg.sender;
}

(3)

@gunnarseay

require(msg.sender == owner, "Caller needs to be owner" _; /

You need to correct the characters at the end of this require() statement in your modifier.

(4)

@gunnarseay

}```}

This last line of your Ownable contract will also throw an error.

(5)

@gunnarseay

import “./Ownable.sol”;

You need to use these quotes: " "  in your code, and not: “ ”  otherwise your code won’t compile. You’ve used the wrong characters for your quotes in both import statements.

(6) If you read the compiler error message you get for this line of code …

@gunnarseay

selfdestruct(owner);

… you will see that owner needs to be explicity converted to a payable address. This is because owner is defined in your Ownable contract as a non-payable address, but the selfdestruct method requires a payable address argument.

You can easily fix this is several different ways. Which would you choose? If you’re not sure, have a look at some of the other students’ solutions, and the feedback and comments they’ve received, posted here in this discussion topic. You will find a lot of useful information here.

(7)

@gunnarseay

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

contract HelloWorld is Destroyable{

You are right that HelloWorld/Bank only needs to inherit Destroyable, because it will inherit Ownable implicitly via Destroyable. This gives you a multi-level inheritance structure. However, this means that this file needs to import the file that contains your Destroyable contract, not Ownable.sol

If you post your corrected version, we’ll take a look at it for you. But let us know if anything is unclear, if you need any more help, or if you have any questions

[jon_m]

Nice solution @Heyns

@Heyns

and then I changed the import in my bank.sol file to the destroyable.sol file, because in destroyable.sol the Ownable.sol file is already integrated.

Yes, that’s correct in terms of what you need to import into bank.sol … so, how have you modified the code in your Bank contract header for the inheritance structure?

[jon_m]

@Heyns

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

@Heyns

ok further in the course

Yes … this line of code shouldn’t be there in the example solution for this assignment As you’ve already realised, it only makes sense once you’ve moved on to calling functions in external contracts, in the next section of the course.

Sorry for the confusion!

[martina]

Hi @jon_m, sending destroyable contract, not sure if it is okay. Thank you!

Destroyable contract:

import "./ownable.sol";

pragma solidity 0.7.5;

contract Destroyable is Ownable {

function destroy() public onlyOwner {

    selfdestruct (payable(msg.sender));

}

}

[jon_m]

Hi @martina,

@jon_m

All of the code in your Destroyable contract is correct.

You didn’t need to change anything in destroyable.sol

@martina

selfdestruct (payable(msg.sender));

You are using Solidity v0.7.5. Prior to v0.8, msg.sender is already a payable address by default, and so doesn’t need to be explicitly converted whenever it needs to reference a payable address (such as here, as the selfdestruct method’s argument). What you originally had was correct …

@martina

selfdestruct (msg.sender);

However, your amendment would be correct when using Solidity v0.8, because msg.sender changed to a non-payable address by default with this version. This means that from Solidity v0.8 msg.sender needs to be explicitly converted whenever it needs to reference a payable address.

---

@jon_m

The problem is that, on deployment, the address that deploys Bank isn’t assigned to the owner state variable in Ownable.

This is because your constructor in Ownable isn’t assigning the deployer’s address (msg.sender) to the owner state variable…

@martina

constructor(){

 owner == msg.sender;

}

Instead, what you have is a condition, which is comparing the owner state variable (which is empty) with the deployer’s address. This will evaluate to false, but this Boolean is then lost, anyway, when the constructor finishes executing, because nothing else is done with it.

All you need to do is change the equality comparison operator (==) in the statement within the constructor to the assignment operator (=) …

constructor() {
    owner = msg.sender;
}

This will assign the deployer’s address to the owner state variable on deployment. So, now, when the destroy() function is called, the condition in the onlyOwner modifier’s require() statement will compare msg.sender with the deployer’s (or contract owner’s) address stored in the owner state variable (and not a zero address). This means that the condition will evaluate to true when the contract owner calls destroy(), but to false when any other address is the caller. And so, the contract owner can now call destroy() and trigger selfdestruct; but no other address can do this, which is what we want.

---

@jon_m

Based on the code you’ve posted, your Bank contract won’t compile because of an error in the contract header …

@martina

contract Bank is ownable, Destroyable {

All of the names of your three contracts start with a capital letter, and so you need to ensure you are consistent with this when coding your inheritance structure.

You need to change this to …

contract Bank is Ownable, Destroyable { ... }

@jon_m

However, Bank only needs to explicitly inherit Destroyable, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.

So, in fact, you can achieve the same result as follows …

// bank.sol

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

contract Bank is Destroyable { ... }

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

[xenmayer]

Hi there
I’ve decided that it would be better to keep owner var as payable from the declaration to prevent type casting.

contract Ownable {
    address payable internal owner;

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

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

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

[jon_m]

Hi @xenmayer,

This is looking good

Now consider the following …

(1) From your code, I’m assuming you are using a v0.8 compiler. Is that right? Always include the pragma statements for each file in your posted solutions. As well as pragma statements being an essential part of the overall code, they also help to avoid any confusion when reading and reviewing your code.

(2) As you haven’t included an import statement, I’m also assuming that your Destroyable contract is currently in the same file as Ownable. Is that right?
While this works, and means that Destroyable can inherit Ownable without the need to import a separate file, it’s better practice to place each contract in their own separate file, so that both Ownable and Destroyable can be re-used as independent modules. Improving the modularity and re-usability of our code are important reasons for using inheritance.

(3) Can you also post the first few lines of the file containing your Bank contract (up to and including the contract header)? It’s the Bank contract that we actually want to deploy, and the contract owner to later destroy, so seeing how you have included Bank within the overall inheritance structure is an important part of your solution.

(4) Unlike functions, the visibility of state variables defaults to internal when the visibility keyword is omitted. So, when defining a state variable with internal visibility, including the actual internal keyword is optional (unlike the public and private keywords). As a result of this, your state variable declaration in Ownable …

@xenmayer

address payable internal owner;

… can be written more concisely as …

address payable owner;

Both work, and your declaration is correct, but including the internal keyword here is unnecessary. It was probably included in the model solution in order to highlight the fact that owner has internal visibility, and so will be available in the derived contract. In practice, we would probably only want to explicitly include the internal keyword in a state variable definition for emphasis or clarity; for example, if omitting it risks anyone reading or working with the code thinking otherwise.

(5)

@xenmayer

I’ve decided that it would be better to keep owner var as payable from the declaration to prevent type casting.

Can you explain a bit more about what you mean, here? Your decision to define the owner state variable as payable is perfectly valid (instead of explicitly converting the address it holds to a payable address, locally, within the killSwitch function).
Whether we decide to define the owner state variable as a payable or non-payable address type will ultimately depend on how often we need to reference the address it holds as either a payable address or a non-payable address. Initially defining it as one or the other does not prevent us from converting it (from either payable to non-payable or vice versa) elsewhere in the code e.g.

// state variable declaration (defined explicitly as a payable address)
   address payable owner;
/* can be converted implicitly to a non-payable address
   if assigned to a non-payable address variable */
   address nonPayableOwner = owner;

// state variable declaration (defined implicitly as a non-payable address)
   address owner;
// can be converted explicitly to a payable address using  payable( )
   payable(owner)

Let us know if anything is unclear, or if you have any questions… and we’re looking forward to seeing your full solution code

[Rebe]

import “./Destroyable.sol”;

contract Ownable is Destroyable {

address owner;

modifier onlyOwner {

   

    require(msg.sender == owner);

    _;

}

constructor() {

    owner = msg.sender;

}

}

pragma solidity 0.7.5;

contract Destroyable {

function close() public {

    selfdestruct(msg.sender);

}

}

[xenmayer]

Hi @jon_m,
Thank you for an extending review . Let me fix and explain one by one

(1) From your code, I’m assuming you are using a v0.8 compiler. Is that right? Always include the pragma statements for each file in your posted solutions. As well as pragma statements being an essential part of the overall code, they also help to avoid any confusion when reading and reviewing your code.

Yes, you’re right. I’m using the 0.8.13. I’ll fix that in the code below.

(2) As you haven’t included an import statement, I’m also assuming that your Destroyable contract is currently in the same file as Ownable. Is that right?
While this works, and means that Destroyable can inherit Ownable without the need to import a separate file, it’s better practice to place each contract in their own separate file, so that both Ownable and Destroyable can be re-used as independent modules. Improving the modularity and re-usability of our code are important reasons for using inheritance.

Sure, thank you for mentioned that. I did the separated files but decide to place here only extracted solution. I’ll fix that below.

(3) Can you also post the first few lines of the file containing your Bank contract (up to and including the contract header)? It’s the Bank contract that we actually want to deploy, and the contract owner to later destroy, so seeing how you have included Bank within the overall inheritance structure is an important part of your solution.

Sure, I’ll place it in the code below .

(4) Unlike functions, the visibility of state variables defaults to internal when the visibility keyword is omitted. So, when defining a state variable with internal visibility, including the actual internal keyword is optional (unlike the public and private keywords). As a result of this, your state variable declaration in Ownable …

Thank you, it’s insightful for me because probably I didn’t mark it till previous lectures. I’ll fix that.

(5) Can you explain a bit more about what you mean, here? Your decision to define the owner state variable as payable is perfectly valid (instead of explicitly converting the address it holds to a payable address, locally, within the killSwitch function).
Whether we decide to define the owner state variable as a payable or non-payable address type will ultimately depend on how often we need to reference the address it holds as either a payable address or a non-payable address. Initially defining it as one or the other does not prevent us from converting it (from either payable to non-payable or vice versa) elsewhere in the code e.g.

You’re totally understood my idea. I did that just to prevent usage of payable() function more than once and the interface of the Ownable contract will help understood “payability” of its owner property. The creation of new non-payable variable from payable one is new for me thanks for mentioned that

And the fixed solution:

// Ownable.sol
pragma solidity 0.8.13;

contract Ownable {
    address payable owner;

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

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

// Destroyable.sol
pragma solidity 0.8.13;

import "./Ownable.sol";

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

// Bank.sol partially
pragma solidity 0.8.13;

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

//...

contract Bank is Ownable, Destroyable {
//...
}

[hardcod3dd]

// File: contracts/Ownable.sol

pragma solidity 0.8.7;

contract Ownable {
    address payable public owner;

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

    modifier onlyOwner() {
        require(msg.sender == owner);
        _;
    }
}
// File: contracts/Destroyable.sol

pragma solidity 0.8.7;


contract Destroyable is Ownable {



    function destroy() public onlyOwner {
        owner.transfer(address(this).balance);
        selfdestruct(owner);
    }
}
// File: contracts/Bank.sol

pragma solidity 0.8.7;


contract Bank is Ownable, Destroyable {

    receive() external payable {

    }

    function balance() public view returns (uint256){
    return payable(address(this)).balance;
  }
}

[jon_m]

Hi @xenmayer,

That’s looking great now!
… and I’m glad you found the comments and feedback helpful

Your inheritance structure is well coded and works. However, it’s also worth mentioning that Bank only needs to explicitly inherit Destroyable, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.

Keep up the great coding!

[jon_m]

Hi @Rebe,

Can you also post the first few lines of the file containing your Bank contract (up to and including the contract header)? It’s the Bank contract that we actually want to deploy, and which the contract owner later needs to be able to destroy. So, seeing how you have included Bank within the overall inheritance structure is an important part of your solution.

Your Destroyable contract will compile, but it will enable any address to call the close() function and destroy the derived contract that inherits Destroyable. Also, on destruction of the contract, whichever address has called close() will receive all of the contract’s remaining funds. In other words, any address can steal all of the funds in the contract at any time! To avoid this potential disaster, you need to restrict access to the close() function to the contract owner. We are assuming that the contract owner can be trusted to look after the funds in the event of a critical failure of the smart contract e.g a hack or attack etc.

As well as destroying the contract, selfdestruct also automatically transfers the remaining contract balance to the payable address argument it is called with. In your code, this is msg.sender …

@Rebe

selfdestruct(msg.sender);

So, if you ensure that only the contract owner’s external address can call the close() function, then the selfdestruct method’s msg.sender argument can only ever reference that address, which means that, on destruction of the contract, its remaining Ether balance can only ever be transferred to the contract owner’s external address.

Your Ownable contract already contains the code that can apply this access restriction to the close() function. However, with your current inheritance structure, Destroyable doesn’t inherit that code. To be able to re-use this code and avoid code duplication, you will also need to change your inheritance structure.

Let us know if anything is unclear, or if you need any more help correcting your solution. If you post your corrected version, we’ll review that as well

[jon_m]

Hi @hardcod3dd,

Your solution is nearly there …

Your inheritance structure is essentially well coded, but you are missing the necessary import statements. Without these, your code won’t compile and you won’t be able to deploy Bank. Where do you need to add them? Have you just forgotten to include them in the code you’ve posted?

It’s also worth mentioning that Bank actually only needs to explicitly inherit Destroyable, because it will implicitly inherit Ownable via Destroyable. This will give you a multi-level inheritance structure.

---

@hardcod3dd

owner.transfer(address(this).balance);

This line of code in your destroy() function (in Destroyable) achieves the desired result, and from a technical point of view it is well coded. However, as well as destroying the contract, selfdestruct also automatically transfers the remaining contract balance to the payable address argument it is called with (in our case, the contract owner’s address). This is all part of the pre-defined selfdestruct functionality, and there is no need to add any additional code to perform this transfer. So, you can remove the above line of code from your destroy() function and everything will work in exactly the same way …

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

---

@hardcod3dd

contract Bank is Ownable, Destroyable {

    receive() external payable {

    }

    function balance() public view returns (uint256){
    return payable(address(this)).balance;
  }
}

To test our solution, we need to be able to transfer Ether to the Bank contract address balance. This is because Bank needs to have an Ether balance when the contract owner triggers selfdestruct, so that we can check that this remaining Ether balance is transferred to the contract owner’s external address balance, and not lost.

We’ve been depositing Ether by calling our deposit() function, but are you transferring Ether to Bank using the Low level interactions Transfer button in Remix? I imagine you must be, and that you’re sending an Ether value with empty calldata, so that your receive Ether function will be able to receive the Ether and add it to the contract address balance. Is that right?

Adding a view function to retrieve the total contract balance is a good idea, and useful for testing. However, address(this) doesn’t need to be converted to a payable address …

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

An address only needs to be payable if it needs to receive Ether. In this function we are only reading the balance property of the contract address.

---

Let us know if any of the above points are unclear, or if you have any questions

[hardcod3dd]

i used remix flattener so it removed the imports. i needed it to be simple so i just added the receive function just to see if i understood the inheritance structure.thanks for the reply

[Taishi_Nammoto]

(1) Your full code from the 2 files containing your Ownable and Destroyable contracts;

Ownable.sol

contract Ownable {
    address public owner;

    modifier onlyOwner {
        require(msg.sender==owner);
        _; // run the function
    }

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

---

Destroyable.sol

import "./Ownable.sol";

contract Destroyable is Ownable {

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

(2) Only include your full code from the file containing your Bank contract if you have made modifications within the contract itself. Otherwise, just include the first few lines of code up to and including the contract header.

HelloWorld.sol

pragma solidity 0.7.5;

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

contract Bank is Ownable, Destroyable {

[Joe22]

contract ownable

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

contract destroyable

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

contract Destroyable is Ownable {

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

contract Bank

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

contract Bank is Ownable, Destroyable {

[SuperiorBlocks]

Took a while to figure out a couple of bugs in my contracts but here they are:

//BANK CONTRACT

import "./Inheritance_Ownable.sol";
import "./Inheritance_Destroyable.sol";

contract Bank_Inheritance is Ownable, Destroyable {...}



//OWNABLE
contract Ownable{

    //State variable(s)
    address public owner;

    //Modifier(s) is/are for restricting permissions like "only employees", "admins", or "owner" etc...
    modifier onlyOwner {
        require(msg.sender == owner);  
        _;

    }

    // Constructor to initialize var
    constructor(){
        owner = msg.sender;
    }
}



//DESTROYABLE

import "./Inheritance_Ownable.sol";

//Self-Destruct and Inheritance
contract Destroyable is Ownable {
    function selfDestruct() public onlyOwner {
        selfdestruct(msg.sender);
    }
}

[Rahul_Kumar]

Ownable.sol

contract Ownable {

    //default visibility is internal
    //if public is defined, automatically getter function is created for that variable
    address public owner;
    address public contractAddress;
  
    modifier onlyOwner {
        require(msg.sender == owner,"Only SC Owner can Call this");
        //run the real functions, Practically EVM will replace all of function statement here
        _; 
    }

    constructor(){
        //here owner becomes the creator of SC
        //mostly useful for restricting function access to admins
        owner = msg.sender;
        //storing contract address
        contractAddress = address(this);
    }
    function getContractBalance() public view returns(uint256){
        return contractAddress.balance;
    }
}

Destroyable.sol

import "./Ownable.sol";

contract Destroyable is Ownable {
    
    function destructContract() public onlyOwner returns(uint256) {
        uint256 _contractBalance = contractAddress.balance;
        payable(owner).transfer(_contractBalance);
        selfdestruct(payable(owner));
        return _contractBalance;
    } 
}

Bank.sol

pragma solidity 0.8.7;

//importing other contract
import "./Destroyable.sol";

contract Bank is Destroyable {

    mapping(address => uint) balances;

Added additional variables for contract address just to make it more legit, else logic can be done in shorter form without that (using transfer with this.balance and msg.sender.). Since destruct function can be called only be owner, either of msg.sender or owner can be used

[niclans]

Ownable_Bank.sol

pragma solidity 0.7.5;

contract Ownable_Bank {

    address public owner;

    modifier onlyOwner {
        require(msg.sender == owner);
        _; //run the function
    }

    constructor(){
        owner = msg.sender;
    }

}

Destroyable.sol

pragma solidity 0.7.5;

import "./Ownable_Bank.sol";

contract Destroyable is Ownable_Bank {

    function close () public { 
        selfdestruct (msg.sender); 
    }

}

[MichaelW]

Destroyable.sol: `// SPDX-License-Identifier: MIT
pragma solidity 0.7.5;

import ‘./Ownable’;

contract Destroyable is Ownable {

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

}

Ownable.sol:

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

contract Ownable {
    address internal owner;

    constructor() {
        msg.sender = owner;
    }

    modifier onlyOwner {
        require(msg.sender == owner, "You must be the owner to use this function");
        _;
    }
}

Why does the receiver line need to be payable?