Inheritance Assignment

jon_m · May 3, 2021, 8:49pm

Your solution is well coded, apart from just one function:

However, your code still compiles successfully and provides all of the required functionality
You have made sure your owner address in Ownable is payable , which is what it needs to be so that selfdestruct() is called with a payable address and any remaining ether in the contract can be paid/transferred to the owner

By giving destroyContract() in Bank private visibility, this function cannot be called: private means it can only be called from within Bank (and not from an outside service such as Remix), and there are no such function calls in Bank (unless you’ve made some further modifications which you haven’t included in your posted solution).
However, even if you did change its visibility to public or external, there is in fact no need to include this additional function, anyway. The close() function is inherited by Bank from Destroyable*. So, when Bank is deployed, close() will be available for the owner to call and trigger selfdestruct().

We can also streamline the inheritance by having Bank just inherit Destroyable*, because it will then indirectly inherit Ownable via Destroyable*. This will give you a multi-level inheritance structure.

* Selfdesctuct in your code

jon_m · May 3, 2021, 9:46pm

Hi @shaikein11,

Your Destroyable contract is well coded but don’t forget to also import the file which contains Ownable.

Well done for making sure your owner address in Ownable is payable , so that, as you have correctly stated, selfdestruct() is called with a payable address This is so that any remaining ether in the contract can be paid/transferred to the owner when selfdestruct() is triggered.

We also need to see what modifications you’ve made to the start of your Bank.sol file, and Bank contract header, for the inheritance. This is a key part of your solution, because the aim of this assignment is for our Bank contract to inherit both the contract destruction and ownership functionalities, so that both are available when we just deploy Bank i.e. we should be able to deploy Bank, perform some transactions, then destroy the Bank contract, transferring any remaining ether balance to the caller of the selfdestruct function (here, the contract owner).

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

jon_m · May 3, 2021, 10:09pm

Hi @nutrina,

I’m glad you found the feedback helpful

Yes, that’s the most straightforward solution for v.0.8 syntax when you specifically want to use msg.sender to receive ether (instead of referencing a previously-defined payable address variable).

jon_m · May 3, 2021, 10:48pm

A very well-coded solution @xela

As you are calling selfdestruct() with owner, you have made sure it is defined as a payable address in Ownable selfdestruct() needs to be called with a payable address, so that any remaining ether in the contract can be paid/transferred to the owner when selfdestruct() is triggered.

As you are using the latest version of Solidity (v0.8.4), you have also ensured that, in the constructor, msg.sender is converted to a payable address when assigning it to the owner state variable Do you understand why we have to do this from v0.8, but didn’t with earlier versions?

Can we also see what modifications you’ve made to the start of your Bank.sol file, and Bank contract header, for the inheritance? The overall objective of this assignment is to be able to deploy Bank, perform some transactions, and then allow the owner to destroy the Bank contract, transferring any remaining ether balance to their own address.

jon_m · May 4, 2021, 1:31am

Nice solution @jtb

We can also streamline the inheritance by having HelloWorld just inherit Destroyable, because it will then indirectly inherit Ownable via Destroyable. This will give you a multi-level inheritance structure.

jon_m · May 4, 2021, 4:53am

dani88:

I saw that you wanted me to make “owner” payable. I honestly thought this code had filled that requirement, so I didn’t address it. I will use another method to make it payable.
dani88:
constructor() public{
     owner = msg.sender;
     address payable owner;
}

No … your owner state variable is already declared where it should be, at the top of your contract in the contract scope …

This is where you need to mark it as a payable address (if you decide to use this method — Method 3 described in this post).

The constructor is executed on deployment, and its msg.sender is the deployer of the contract. In the constructor owner = msg.sender; assigns the contract deployer’s address to the owner state variable, where it will be stored persistently in the contract state as a payable address (if you have marked the owner state variable as payable, as I’ve just mentioned).
Also adding address payable owner; to the constructor, is just declaring a temporary, local variable with the same name owner within the local scope of the constructor. Any values assigned to local variables are lost once the function (or, in this case, constructor) has finished executing. But after declaring this local variable, you haven’t even assigned it an address value. In the control flow you have in your constructor:

msg.sender assigned to owner state variable;
Local owner variable declared;
No address assigned to local variable;
Constructor finishes executing.

jon_m · May 4, 2021, 3:01am

This code (for Method 1) goes in Destroyable (not in Ownable) within the close() function body.
With Method 1 you keep the owner’s address stored in the state variable called owner (in the contract Ownable) as non-payable. If you only need this address to be payable for the purposes of executing selfdestruct(), then you can convert it to payable locally, within the function where you actually need to use it as payable… either:
(i) using a local variable (i.e. within a function), and just before you actually use it…

function close() public onlyOwner {
     address payable receiver = address(uint160(owner));
     selfdestruct(receiver);
}

or (ii) directly within the selfdestruct() function call itself…

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

jon_m · May 4, 2021, 3:12am

That’s a great idea.

Absolutely!.. nearly all of the videos are re-recorded, and some of the assignments are quite different. This variety in the material, a few changes in syntax for v.0.7, but the fact we are still covering all of the same concepts and techniques, will mean if you do it immediately after this one, it will be a great way to review and consolidate what you’ve being learning in this course, while still being engaging and not too repetitive. I would highly recommend you do that before moving on to Ethereum Smart Contract Programming 201

jon_m · May 4, 2021, 4:39am

dani88:

The owner’s contract is below:

pragma solidity 0.5.12;

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

This Ownable contract will now work for Method 3, Donnie
You have already made the correct changes to the constructor and the owner state variable (which I mentioned in the first of my replies to you today).

By making the address stored in the owner state variable payable, you don’t need to perform any of the local conversions which we’ve been discussing for Method 1. So, in the close() function (in Destroyable, not Ownable) you can just go ahead and call selfdestruct() with owner like you did here:

But, there is still one error in this code: the wrong file has been imported. Look again, at what I have explained in this post, and get your import statements right at the top of both remaining files.
Also, as I’ve mentioned before, you don’t need to make the close() function payable…

If you do this correctly, then you won’t need to remove onlyOwner like you’ve said here…

This line won’t throw an error if you import the correct files into this file. And, obviously, you must keep your spelling consistent…

When you’ve got that working, you can then experiment with removing payable from the owner state variable in Ownable, and using Methods 1 & 2 to ensure the owner address is payable for selfdestruct() — I’ve explained how to get Method 1 right by using the code in Destroyable, in this post).

xela · May 4, 2021, 9:28am

Hey @jon_m and thanks for reviewing my code,

I have just checked the changelogs of v0.8 and it says:

Address literals have the type address instead of address payable . They can be converted to address payable by using an explicit conversion, e.g. payable(0xdCad3a6d3569DF655070DEd06cb7A1b2Ccd1D3AF) .

So basically this means if you just use a normal address like this:

address a = 0xdCad3a6d3569DF655070DEd06cb7A1b2Ccd1D3AF;

by default it is NOT payable since v0.8 but can be explicitly converteted like this:

address payable a = payable(0xdCad3a6d3569DF655070DEd06cb7A1b2Ccd1D3AF);

Have I understood this correctly?

Here is the code for my Bank. Since Destroyable is inheriting from Ownable and EtherBank is inheriting from Destroyable, I don’t need my EtherBank to inherit from Ownable, right?
EtherBank.sol

pragma solidity ^0.8.4;

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

interface GovernmentInterface {
    function addTransaction(address _from, address _to, uint _amount) external payable;
    
}

contract EtherBank is Destroyable {

    GovernmentInterface GovernmentInstance = GovernmentInterface(0xC5862Ba753F3DabC4120E9803c01e0B13D7905F2);
    
    mapping(address => uint) balances;
    
    
    event deposited(address sender, uint amount);
    event withdrawn(address withdrawer, uint amount);
    event transfered(address sender, address receiver, uint amount);


    modifier checkFunds(uint requiredAmount){
        require(balances[msg.sender] >= requiredAmount, "Not enough funds for transaction!");
        _; //runs the function
    }
    

    modifier checkValueForZero(uint value){
        require(value > 0, "Value needs to be greater than 0.");
        _;
    }
    
    function deposit() public payable checkValueForZero(msg.value) returns(uint) {
        uint balanceBefore = balances[msg.sender];
        balances[msg.sender] += msg.value;
        emit deposited(msg.sender, msg.value);
        assert(balances[msg.sender] == balanceBefore + msg.value);
        return balances[msg.sender];
    }
    
    function withdraw(uint amount) public payable checkFunds(amount) checkValueForZero(amount) returns(uint){
        uint balanceBefore = balances[msg.sender];
        balances[msg.sender] -= amount;
        payable(msg.sender).transfer(amount);
        emit withdrawn(msg.sender, amount);
        assert(balances[msg.sender] + amount == balanceBefore);
        return balances[msg.sender];
    }
    
    function getBalance() public view returns(uint){
        return balances[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        uint previousSenderBalance = balances[msg.sender];
        _transfer(msg.sender, recipient, amount);
        GovernmentInstance.addTransaction{value: 1 gwei}(msg.sender, recipient, amount);
        emit transfered(msg.sender, recipient, amount);
        assert(balances[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer(address from, address to, uint amount) private checkFunds(amount){
        balances[from] -= amount;
        balances[to] += amount;
    }
}

dani88 · May 4, 2021, 4:23pm

Hey Jon,
I you have put a lot of time into helping me with this. I appreciate it. I am still getting an error with helloWorld. here is the relevant code:

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

contract HelloWorld is Ownable, Destroyable {

import "./Ownable.sol";

pragma solidity 0.5.12;

contract Destroyable is Ownable {

    function close() public payable onlyOwner{
         selfdestruct(owner);
     //option 1
     //address payable receiver = address(uint160(owner));
     //  selfdestruct(receiver);
    // option2
   // function close() public onlyOwner {
  //  selfdestruct(address(uint160(owner)));
     //  selfdestruct(Owner);
       
    }

}

What am I missing???

kopino4 · May 5, 2021, 12:14pm

This is working but is there a standart way to make owner address payable?

contract Destroyable {
    address payable owner;
    
    constructor() {
        owner = payable(msg.sender);
    }
    
    function destroy() internal {
        require(msg.sender == owner);
        selfdestruct(owner);
    }
}

jon_m · May 5, 2021, 6:39pm

Your code is now looking good @Michael_Gerst

Well done for perservering with this Hopefully you’ve learnt a lot in the process

Corrected

Corrected, by defining owner as a payable address in Ownable

The only thing you haven’t modified is …

To use the onlyOwner modifier defined in Ownable (and inherited by Destroyable and Bank), you just need to add the name onlyOwner to the function header.

Also, have a look at this post which explains why you need to make an important security modification to the order of the statements within your withdraw function body.

This is probably caused by something related to calling the external Government contract function addTransaction. When you say you are getting a revert error when compiling, do you actually mean you are getting a transaction revert error when you call transfer() after you’ve already compiled and deployed your contract? Compiler errors are the ones you get before deployment, and are indicated by a red mark over the line number. Your code should now be compiling without any errors, and your code for this assignment should now work. So try removing all of the code which you’ve added afterwards for the lessons on external contracts: the Government interface; declaration of the governmentInstance variable; and the call to addTransaction in transfer(). You should find that your transfer function now works. That will also prove to you that the revert is thrown by an issue with the external function call.
If that’s the case, then have a look at the discussion topic related to Inheritance & External Contracts, and you’ll probably find the solution there. I suspect that this could be answer.

If this doesn’t resolve the problem, then post any further questions about the Government external contract in the discussion post related to that. But let us know here if you have any more questions about the Inheritance assignment

jon_m · May 6, 2021, 12:19am

Hey @xela,

Correct…this gives you a multi-level inheritance structure. It also means you don’t need to import Ownable.sol into EtherBank’s .sol file. Apart from that, your EtherBank contract and file are correctly coded for your inheritance structure.

In terms of the rest of your EtherBank code, the comments in my feedback for your Transfer Assignment apply.

Just one additional point…

As you know, when a derived contract inherits functionality from one or more parent contracts, we only need to deploy the derived contract in order to have the inherited functionality available. But if the parent contract(s) are in separate files, then we need to import them into the derived contract’s file.

The situation is different when calling an external function. Unlike an inherited contract, the external contract also needs to be deployed in order to be able to call its functions from another contract. The external function call can be made on an instance of the external contract (based on an interface and the address of the external contract). Because of this, the external contract’s file does not need to be imported. We would only need to import the interface (not the external contract) if this was located in a separate file. So you can also remove…

import "./Government.sol";

xela:

So basically this means if you just use a normal address like this:
address a = 0xdCad3a6d3569DF655070DEd06cb7A1b2Ccd1D3AF;
by default it is NOT payable since v0.8 but can be explicitly converteted like this:
address payable a = payable(0xdCad3a6d3569DF655070DEd06cb7A1b2Ccd1D3AF);
Have I understood this correctly?

Correct
However, you were using msg.sender and not an address literal. But msg.sender underwent the same change from v0.8 as address literals. You can see this in the 9th bullet point of the New Restrictions section in the Solidity v0.8.0 Breaking Changes you linked to and quoted from:
https://docs.soliditylang.org/en/v0.8.4/080-breaking-changes.html#new-restrictions

e.g.

v0.7.0

pragma solidity 0.7.0;

contract PayableTest {
    /* address literal is payable by default */
    address payable a = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4; //compiles
    /* and can be implicitly converted to a non-payable address*/
    address b = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4; //compiles

    /* msg.sender is payable by default */
    address payable c = msg.sender; //compiles
    /* and can be implicitly converted to a non-payable address*/
    address d = msg.sender; //compiles
    
   function withdraw(uint amount) external {
      /* msg.sender is payable by default */
      msg.sender.transfer(amount); // compiles

      /* address literal is payable by default */
      0x5B38Da6a701c568545dCfcB03FcB875f56beddC4.transfer(amount); //compiles
   }
}

v0.8.0

pragma solidity 0.8.0;

contract PayableTest {
   /* address literal is non-payable by default */
   address payable a = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4; //ERROR
   address b = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4; //compiles
   /* can be explicitly converted to a payable address */
   address payable x = payable(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4); //compiles
   
   /* msg.sender is non-payable by default */
   address payable c = msg.sender; //ERROR
   address d = msg.sender; //compiles
   /* can be explicitly converted to a payable address*/
   address payable y = payable(msg.sender); //compiles

   function withdraw(uint amount) external {
      /* msg.sender is non-payable by default */
      msg.sender.transfer(amount); //ERROR
      /* can be explicitly converted to a payable address */
      payable(msg.sender).transfer(amount); //compiles
      y.transfer(amount); //compiles

      /* address literal is non-payable by default */
      0x5B38Da6a701c568545dCfcB03FcB875f56beddC4.transfer(amount); //ERROR
      /* can be explicitly converted to a payable address */
      payable(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4).transfer(amount);
      x.transfer(amount); //compiles
   }
}

StanleyGM · May 9, 2021, 5:35pm

Destroyable.sol

pragma solidity 0.7.5;

import './Ownable.sol';

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

Ownable.sol

pragma solidity 0.7.5;

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

Bank.sol

pragma solidity 0.7.5;


import "./Destroyable.sol";

contract Bank is Destroyable {
    
    mapping(address => uint) balance;

    event depositDone(uint amount, address indexed depositedTo);
    
    
    function deposit() public payable returns (uint) {
        balance[msg.sender] += msg.value;
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public onlyOwner returns (uint){
    require(balance[msg.sender] >= amount);
        msg.sender.transfer(amount);
        balance[msg.sender] -= amount;
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        require (balance[msg.sender] >= amount);
        require (msg.sender != recipient);
        uint previousSenderBalance = balance[msg.sender];
        balance[recipient] += amount;
        previousSenderBalance -= amount;
        
    }
}

Lehigr2 · May 7, 2021, 2:41am

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

jon_m · May 7, 2021, 10:24am

Hi Donnie,

Sorry for the delay in replying.

Nearly there now!

You are inheriting Ownable and Destroyable, but only importing Ownable. Remember…

Can you see what you need to add?

Your Destroyable is looking good now, and it’s nice to see you’ve got the other options for calling selfdestruct() in comments
Just a couple of points…

It will still execute, but close() does not need to be payable with any of the options, because the function itself doesn’t need to receive any ether. Only our owner address needs to be payable because it’s receiving ether (the remaining contract balance).

You don’t need this line for your Option 2…

The other option would be simply…

selfdestruct(msg.sender);

I explained why this works, here…

And remember… when you run your options, the owner variable doesn’t need to be defined as payable in Ownable — only when you use selfdestruct(owner);

Once you resolve the error in HelloWorld…

Let me know when I can crack open the champagne!

jon_m · May 7, 2021, 11:07am

Hi @kopino4,

If you are using v0.8 instead of v0.7.5, then this is correct…

From v.0.8, msg.sender is non-payable by default, which is why you need to explicitly convert it to payable when assigning it to your payable address variable owner in the constructor. Prior to v.0.8 msg.sender was payable by default, and so this explicit conversion wasn’t necessary.

If you only need your owner address to be payable for the purposes of selfdestruct(), then another option is to leave the address variable non-payable, and then explicitly convert owner when passing it to selfdestruct()…

selfdestruct(payable(owner));

There isn’t really a “standard way”, as each situation is different and it’s up to us as developers to work within the confines of the language’s defined syntax rules and come up with the most appropriate solution. The syntax is standardised though, and that is what you need to learn. For example, what I’ve explained to you above about msg.sender in v.0.8 can be found in the Solidity documentation, here (9th bullet point in the New Restrictions section of Solidity v.0.8.0 Breaking Changes):
https://docs.soliditylang.org/en/v0.8.4/080-breaking-changes.html#new-restrictions.

Your destroy() function is looking good, but why have you given it internal visibility? Can we see your complete solution in order to see how you have coded your inheritance structure?
Just to make the code more interesting in terms of the inheritance, how would your solution look if you separated your ownership functionality and contract destruction functionality into 2 separate files — giving you 3 files in total?

dani88 · May 7, 2021, 3:06pm

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

contract HelloWorld is Ownable, Destroyable {

I thought you said that this was wrong in an earlier post. Maybe I misunderstood.

Now, the only error that I get is after the deployment. it states. “currently you have no contract instances to interact with.”

I also changed this:

import "./Ownable.sol";

pragma solidity 0.5.12;

contract Destroyable is Ownable {

    function close() public onlyOwner{
         selfdestruct(owner);
     //option 1
     //address payable receiver = address(uint160(owner));
     //  selfdestruct(receiver);
    // option2
   // function close() public onlyOwner {
  //  selfdestruct(address(uint160(owner)));
  //option next: 
  //function()close public onlyOwner{
    //selfdestruct(msg.sender);  
  //}
       
    }

}

it seems to automatically default to “Destroyable”. even though I told it to deploy helloWorld.
because I don’t know why!

decrypter · May 7, 2021, 3:13pm

pragma solidity 0.7.5;

contract Destroyable{

address payable private owner;

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

constructor(){
    owner=msg.sender;
}

function close() internal onlyOwner { //Internal to allow calls only from derived contracts
    selfdestruct(owner);  
}

}