Inheritance Assignment

Ethereum Smart Contract Programming 101
#1720

Destroyable Contract Solution:
Ownable Contract:

pragma solidity 0.5.12;

contract Ownable{
    address public owner;

    modifier onlyOwner {
        require(msg.sender == owner, "Caller needs to be owner"        _; /
        _; //continues exec in function
    }

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

import “./Ownable.sol”;
pragma solidity 0.5.12;
contract Destroyable is Ownable {

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

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

contract HelloWorld is Destroyable{
…
#1721

I didn’t modify anything in my ownable.sol file.

this is my Destroyable.sol file:

pragma solidity 0.7.5;

import "./Ownable.sol";

contract Destroyable is Ownable{
    function destroy() public onlyOwner{
        selfdestruct(msg.sender);
    }
}

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.

1 Like
#1722

After looking at the solution I came across a piece of code and therefore I have one question:

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

Can you please explain to me what this piece of code functionality is in the bank.sol file?

Thanks!

1 Like
#1723

ok further in the course :laughing:

1 Like
#1724

Hi @martina,

You are nearly there, apart from a couple of errors …

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

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. These kinds of mistakes are easy to make, but the compiler will flag them for you, and the error messages generated by the compiler will help you to identify and correct them. If you haven’t already, turn on Auto compile by checking the first box under Compiler Configuration in the Solidity Compiler panel in Remix. The compiler will then compile your code as you write it. This will enable you to see and check any errors as soon as they arise.

Apart from this error, your inheritance structure is well coded. 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.

Once you’ve corrected your code for the above error, you will be able to compile and deploy Bank. However, you will now find that Bank cannot be destroyed…

All of the code in your Destroyable contract is correct. The problem is that, on deployment, the address that deploys Bank isn’t assigned to the owner state variable in Ownable. Can you see why?

At the moment the owner variable remains empty. So, when the contract owner calls destroy() the condition in the onlyOwner modifier’s require() statement is comparing the contract owner’s address (msg.sender) with a zero address (the default value of an empty address variable). This means that the condition will always evaluate to false and the require() statement will always fail and trigger revert.

See if you can correct this yourself. But let me know if you need any further help, or if you have any questions :slight_smile:

Just one final point …

When posting your code, format it as one single block. That way you won’t end up with bits of your code formatted, and other bits unformatted. This will make it easier to read and easier to spot any copy-and-paste errors.
Follow the instructions in this FAQ: How to post code in the forum

1 Like
#1725

Hi @stanmation,

Your solution meets all of the assignment objectives. Your implementation of a multi-level inheritance structure is also well coded and is the best approach: Bank only needs to explicitly inherit Destroyable, because it will implicitly inherit Ownable via Destroyable :ok_hand:

However, you haven’t taken full advantage of your inheritance structure, and you’ve included some code which isn’t necessary. The same objectives can be met in a more concise way …

(1) By giving your close() function in Destroyable internal visibility, it is inherited and available within Bank, but cannot be called externally by the contract owner. To resolve this you’ve added an additional public function (destroyMe) to Bank which in turn calls close(). However, an important reason for using inheritance is to reduce code duplication, and if you change the visibility of the close() function from internal to public, it will be inherited and available to be called from both within Bank and externally from Remix. You could also give it external visibility, which would enable it to be called externally from Remix, but not from within Bank, which for the purposes of this assignment would be enough. This removes the need for the additional destroyMe() function in Bank.

close() will now appear as a function-call button in the Remix panel, instead of destroyMe()

Additionally, by keeping all of the code and functionality related to contract destruction within a single, separate contract (Destroyable), you will improve the modularity and re-usability of your code (other important reasons for using inheritance) and keep your code more organised.

(2) 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 means there is no need to add any additional code for this.

In any case, the additional code you’ve added wouldn’t save the contract’s remaining Ether balance, or transfer it to the contract owner’s external address, anyway …

This line of code assigns the contract owner’s individual balance to the remaingBalance state variable in Ownable. But the Ether which needs to be transferred on destruction of the contract is the total amount of Ether held in the contract, and not just the contract owner’s share.

The individual user balances stored in the mapping perform an internal accounting role and record each user’s share of the contract’s total Ether balance. The contract address balance can be accessed with…

address(this).balance

… and can be retrieved at any point in time by adding a getter which contains this code e.g.

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

Whatever value is assigned within either the destroyMe() or the close() function to the remainingBalance state variable in Ownable will be immediately lost when selfdestruct is then triggered and destroys the contract in the same transaction. So, even if we did need to reference and store an amount during this final transaction (which we don’t), we would only need to do this temporarily using a local variable, and not with a state variable.

So, in summary, and as a result of all of the above points, instead of the two functions you currently have (close and destroyMe) all you need is the following one in Destroyable…

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

A couple of additional observations about the withdraw() function in Bank …

(3) You’ve modified the withdraw function() with the onlyOwner modifier, but this means that only the contract owner will be able to withdraw funds while the Bank contract is deployed and operating normally. The contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so, while the contract is still deployed, it only seems fair that all users should be able to withdraw their funds as well, don’t you think? :sweat_smile:
Your withdraw() 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 different scenarios, and I think it has ended up being left in the withdraw() function in the solution code for this assignment by mistake!

(4) Your withdraw() function now includes the missing return statement, but don’t forget to also add a line of code to adjust the individual user’s balance in the mapping for the withdrawal amount, as I’ve explained in my feedback for your Transfer Assignment.

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

#1726

Nice solution @nathanls78 :ok_hand:

In fact, Bank only needs to explicitly inherit Destroyable, 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 :slight_smile:

2 Likes
#1727

john you are now free @jon_m

1 Like
#1728

Hi @gunnarseay,

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

(1)

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 …

(3)

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

(4)

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

(5)

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 …

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

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

#1729

Nice solution @Heyns :ok_hand:

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?

#1730

Yes … this line of code shouldn’t be there in the example solution for this assignment :sweat_smile: 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!

#1731

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

}

}
#1732

Hi @martina,

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

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 …

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.

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

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.

You need to change this to …

contract Bank is Ownable, Destroyable { ... }

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

#1733

Hi there :raised_hands:
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);
    }
}
1 Like
#1734

Hi @xenmayer,

This is looking good :ok_hand:

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 …

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

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

1 Like
#1735

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

}

}

#1736

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

(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 :ok_hand:.

(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 :pray:

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 {
//...
}
1 Like
#1737 

// 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;
  }
}
1 Like
#1738

Hi @xenmayer,

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

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!

1 Like
#1739

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 …

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

1 Like