… also, just having looked at the code again, by using onlyOwner at least somewhere within Bank (even if the practicality of this is rather forced) allows us to demonstrate that the functionality of the onlyOwner modifier (defined in Ownable) is also available in Destroyable and Bank by inheritance. That might be why it was left there, although I totally agree with you that it doesn’t make a lot of practical sense in the overall context of this particular contract.
Hi @dani88,
It looks like you have your contracts mixed up here…
This function should be in Destroyable, because this is the functionality that will be used to destroy the contract that inherits it. Destroyable needs to inherit Ownable, in order for it to have access to the owner state variable and the onlyOwner modifier. So, if Destroyable inherits Ownable, it also needs to import the .sol file which contains Ownable (not helloworld.sol).
close() now doesn’t return a value (which is correct), but you’ve forgotten to also remove returns() from the function header. And as I mentioned before:
Without seeing your Ownable contract, I can’t see if you have defined the owner address as payable. If you haven’t, another option is to convert owner to a payable address when passing it to selfdestruct(). An even simpler way to do this, is to call selfdestruct() with msg.sender instead of owner, because in the version of Solidity you’re using, msg.sender is payable by default. Even if you use msg.sender, selfdestruct can still only be initiated by the contract owner, because access to the close() function is restricted by the onlyOwner modifier (inherited from Ownable).
Then, as I mentioned before…
Because of the inheritance structure, it will be easier for us to check your code if you post all 3 contracts. Did you try to attach a screen shot in your last post, as well as the code? All that shows is:
Anyway, instead of screen shots, please post your actual code, because this makes it much easier for us to check it.
I took a screenshot of the error message. I thought it might help. Thanks for taking the time to explain this.
I feel that I am getting closer to the answer. I appreciate your guidance. …thanks for not just giving me the answers. I have worked hard on this, and it’s been good for me. 
So, I realized that if “Ownable” was the owner, then “Destoryable” must be the victim!!, so I changed the name… Also, I put an empty function in “Destroyable”, it flagged it, so I deleted it.
Solidity is flagging "contract HelloWorld is Ownable, Destroyable {. line 5
says that definition of base has to precede definition of derived contract contract HelloWorld is Ownable, Destroyable {
Here’s the code that I have:
pragma solidity 0.5.12;
contract Ownable{
address public owner;
modifier onlyOwner(){
require(msg.sender == owner);
_; //Continue execution
}
constructor() public{
owner = msg.sender;
address payable owner;
}
}
`import "./Ownable.sol";
import "./Destroyable.sol";
pragma solidity 0.5.12;
contract HelloWorld is Ownable, Destroyable {
struct Person {
uint id;
string name;
uint age;
uint height;
bool senior;
}
event personCreated(string name, bool senior);
event personDeleted(string name, bool senior, address deletedBy);
uint public balance;
modifier costs (uint cost){
require(msg.value >= cost);
_;
}
mapping (address => Person) private people;
address[] private creators;
function createPerson(string memory name, uint age, uint height, bool senior) public payable costs(1 ether) {
require(age < 150, "Age needs to be below 150");
balance += msg.value;
//This creates a person
Person memory newPerson;
newPerson.name = name;
newPerson.age = age;
newPerson.height = height;
if(age >= 65){
newPerson.senior = true;
}
else{
newPerson.senior = false;
}
insertPerson(newPerson);
creators.push(msg.sender);
assert(
keccak256(
abi.encodePacked(
people[msg.sender].name,
people[msg.sender].age,
people[msg.sender].height,
people[msg.sender].senior
)
)
==
keccak256(
abi.encodePacked(
newPerson.name,
newPerson.age,
newPerson.height,
newPerson.senior
)
)
);
emit personCreated(newPerson.name, newPerson.senior);
}
function insertPerson(Person memory newPerson) private {
address creator = msg.sender;
people[creator] = newPerson;
// ln 78 creator.send(); does not work. one is address the other is address payable This forces the creator of the contract to think
//of whether or not the creator needs to receive money. It shouldn't in the example written.
//so, address creator = msg.sender; Problem is converting a payable to a non-payable address.
//to return = address payable test = address(uint160(creator));
}
function getPerson() public view returns(string memory name, uint age, uint height, bool senior){
address creator = msg.sender;
return (people[creator].name, people[creator].age, people[creator].height, people[creator].senior);
}
function deletePerson(address creator) public onlyOwner {
string memory name = people[creator].name;
bool senior = people[creator].senior;
delete people[creator];
assert(people[creator].age == 0);
emit personDeleted(name, senior, owner);
}
function getCreator(uint index) public view onlyOwner returns(address){
return creators[index];
}
function withdrawAll() public onlyOwner returns(uint){
uint toTransfer = balance;
balance = 0;
msg.sender.transfer(balance);
return toTransfer;
}
function close() public payable onlyOwner{
selfdestruct(owner);
}
}
`import "./helloWorld.sol";
pragma solidity 0.5.12;
contract Destroyable{
}`````Hi Donnie,
Firstly, well done for persevering with this!
Yes… you’ll learn more this way. I’m glad you’re finding this approach helpful
Line 5 defines which base (parent) contracts (Ownable and Destroyable) are inherited by the derived (child) contract (HelloWorld). You have correctly imported Destroyable.sol into helloWorld.sol, but you have also imported helloWorld.sol into Destroyable…
Only the base/parent contract should be imported into the derived/child contract, because it is the parent contract’s functionality that we want to have available in the child contract, and not vice versa.
When you remove the line import "./helloWorld.sol"; from Destroyable.sol the error will disappear.
But what is the point of HelloWorld inheriting Destroyable, if Destroyable is empty? None whatsoever. This is why I said in my last post that the close() function…
At the moment, you have the close() function in HelloWorld. You need to move it to Destroyable. You could leave it in HelloWorld and just not inherit Destroyable. But the whole point of this assignment is to demonstrate and practise inheritance. It is also good practice to keep separate functionality in separate modules (here, our separate modules are separate files).
You will then get an error for your selfdestruct() function call (now in Destroyable):
If you look at the error message, you should be able to see that the problem is that owner still isn’t a payable address. Making the close() function payable (as you have) doesn’t convert the owner address to a payable address — it only enables the close() function to receive ether, which it doesn’t need to do. It’s the owner address that needs to receive ether, so you need to either:
1) Convert owner to a payable address in close() either (i) using a local variable e.g.
address payable receiver = address(uint160(owner));
selfdestruct(receiver);
or (ii) directly within the function call itself:
selfdestruct(address(uint160(owner)));
Important note This conversion of a non-payable to a payable address is now easier in Solidity v0.6 and above, where you can just use payable(nonpayableAddress) The more up-to-date syntax is taught in the new Ethereum Smart Contract Programming 101 course where we use v0.7.5
or
2) As I mentioned before…
or
3) Define the owner address as payable within Ownable. This would be done in the state variable:
address payable public owner;
… and not in the constructor:
Implementing just one of the above 3 methods will remove the remaining compiler error, and also the addtional warnings (flagged in orange) in Ownable. You will then just be left with one orange warning in HelloWorld for this line:
See if you can work out from the warning message what it is you need to remove. Can you also work out why can you remove it?
Thanks so much for your patience with me. I appreciate it:
(2 ether) should read () or (msg.value). as per your instructions earlier…sorry I missed that one.
almost there!!
I am getting an error message on helloWorld
contract HelloWorld is Ownable, Destroyable {
This error is affecting all functions that are restricted to owner. When I delete “Destroyable”, the error message goes away, but the assignment isn’t complete. It is written just as it was in the video, and I am not sure what I did wrong.
I have auto compile checked, so remix compiles what I change.
Thanks again
pragma solidity 0.5.12;
contract Ownable {
address public owner;
modifier onlyOwner(){
require(msg.sender == owner);
_; //Continue execution
}
constructor() public{
owner = msg.sender;
address payable owner;
}
}
import "./Ownable.sol";
pragma solidity 0.5.12;
contract HelloWorld is Ownable, Destroyable {
struct Person {
uint id;
string name;
uint age;
uint height;
bool senior;
}
event personCreated(string name, bool senior);
event personDeleted(string name, bool senior, address deletedBy);
uint public balance;
modifier costs (uint cost){
require(msg.value >= cost);
_;
}
mapping (address => Person) private people;
address[] private creators;
function createPerson(string memory name, uint age, uint height, bool senior) public payable costs() {
require(age < 150, "Age needs to be below 150");
balance += msg.value;
//This creates a person
Person memory newPerson;
newPerson.name = name;
newPerson.age = age;
newPerson.height = height;
if(age >= 65){
newPerson.senior = true;
}
else{
newPerson.senior = false;
}
insertPerson(newPerson);
creators.push(msg.sender);
assert(
keccak256(
abi.encodePacked(
people[msg.sender].name,
people[msg.sender].age,
people[msg.sender].height,
people[msg.sender].senior
)
)
==
keccak256(
abi.encodePacked(
newPerson.name,
newPerson.age,
newPerson.height,
newPerson.senior
)
)
);
emit personCreated(newPerson.name, newPerson.senior);
}
function insertPerson(Person memory newPerson) private {
address creator = msg.sender;
people[creator] = newPerson;
// ln 78 creator.send(); does not work. one is address the other is address payable This forces the creator of the contract to think
//of whether or not the creator needs to receive money. It shouldn't in the example written.
//so, address creator = msg.sender; Problem is converting a payable to a non-payable address.
//to return = address payable test = address(uint160(creator));
}
function getPerson() public view returns(string memory name, uint age, uint height, bool senior){
address creator = msg.sender;
return (people[creator].name, people[creator].age, people[creator].height, people[creator].senior);
}
function deletePerson(address creator) public onlyOwner {
string memory name = people[creator].name;
bool senior = people[creator].senior;
delete people[creator];
assert(people[creator].age == 0);
emit personDeleted(name, senior, owner);
}
function getCreator(uint index) public view onlyOwner returns(address){
return creators[index];
}
function withdrawAll() public onlyOwner returns(uint){
uint toTransfer = balance;
balance = 0;
msg.sender.transfer(balance);
return toTransfer;
}
}
`
`import "./helloWorld.sol";
pragma solidity 0.5.12;
contract Destroyable is Ownable {
function close() public payable onlyOwner{
selfdestruct(owner);
}
}```Hi Donnie,
This was correct before… but now you’ve removed it…
The problem was this…
In summary:
If an inherited contract (one that you have included in the contract header after is ) is in a separate file, then you MUST IMPORT THE FILE WHERE THE INHERITED CONTRACT IS LOCATED, AS WELL AS declaring it in the contract header.
You haven’t done this in helloWorld.sol or in Destroyable.sol
After you’ve got your inheritance fixed, you can then start to address the issue with owner still not being a payable address, by following my earlier email from here:
No … those instructions were related to 1 ether being an invalid function parameter in your original leaseAgreement contract, which I suggested you changed to HelloWorld.
Using 1 ether as the argument in the modifier costs(1 ether) is correct and the warning you will get doesn’t refer to that. When you reach this stage of the compiling progress, read the actual error message you get and it will tell you what you need to remove. But can you work out why you need to remove it?
And just one other thing… if you send ether to a function, you don’t need to explicity state msg.value as a parameter. Solidity will automatically make msg.value available for you to reference within the function body (in the same way that it does with msg.sender).
1.- SelfDesctuct contract
pragma solidity 0.7.5;
import './Ownable.sol';
contract SelfDesctuct is Ownable{
function close() public onlyOwner{
selfdestruct(owner);
}
}
2.- ownable contract
pragma solidity 0.7.5;
contract Ownable{
address payable public owner;
constructor(){
owner = msg.sender;
}
modifier onlyOwner{
require(msg.sender == owner);
_;
}
}
3.- Added to Bank contract
import './Ownable.sol';
import './selfdestruct.sol';
contract Bank is Ownable, SelfDesctuct{
// all previous code...
function destroyContract() private{
close();
}
}
2 Likes
I will reuse the code from the class to build my destroyable class.
pragma solidity 0.7.5;
//Legacy code from the Inheritance Class. We should made owner address "payable", as selfdestruct requires a payable address
contract Ownable
{
address payable public owner;
modifier onlyOwner
{
require(msg.sender == owner);
_; // run the function
}
constructor()
{
owner = msg.sender;
}
}
//We will create a derived contract that will inherit behavior from the Ownable contract
contract Destroyable is Ownable
{
//Our close function will enforce contract ownership using the onlyOwner modifier
function close() public onlyOwner
{
//then we finally destroy the contract, since the proper validation was done by the modifier
selfdestruct(owner);
}
}
1 Like
Hello @jon_m,
Thank you for this feedback.
So first I have modified the pragma statements to: pragma solidity 0.8.0; in all files. I think it is good to use a newer compiler, also pinning a specific version of it. It is also clear with what compiler this is supposed to work.
Then whenever I need a payable sender address I use: payable(msg.sender).
I have tested this, and it works.
Thank you again for the constructive feedback.
1 Like
Destroyable.sol
pragma solidity ^0.8.4;
import "./Ownable.sol";
contract Destroyable is Ownable {
function close() onlyOwner public {
selfdestruct(owner);
}
}
Ownable.sol
pragma solidity ^0.8.4;
contract Ownable {
address payable owner;
modifier onlyOwner {
require(msg.sender == owner, "You need to be the owner!");
_; //runs the function
}
constructor(){
owner = payable(msg.sender);
}
}
1 Like
import './Ownable.sol';
import './Destroyable.sol';
pragma solidity 0.5.12;
contract HelloWorld is Ownable, Destroyable{
...
}
Destroyable.sol
import './Ownable.sol';
pragma solidity 0.5.12;
contract Destroyable is Ownable{
function destroy() public onlyOwner {
selfdestruct(msg.sender);
}
}
1 Like
Thanks so much for your help.
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.
pragma solidity 0.5.12;
contract Ownable {
address payable receiver = address(uint160(owner));
selfdestruct(receiver);
Throws a parserError: Expected identifier but got ‘(’
selfdestruct(receiver);
good to know!!
I will take that course when I finish this one. I figure that I may need the information in this contract for some reason, and besides, the repetition will do me good!!
If I comment out “onlyOwner”. the error goes away, so that would be my guess. I have no idea how I can do that and restrict the functions. BTW, If I do comment out those three functions’ “onlyOwner” feature, it leaves only one error it’s “contract HelloWorld is Ownable, Destyroyable {”
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;
}
}
1 Like
Hey Jon,
Here it is updated. I am now able to destroy the contract after updating the import statements and making the ownable address payable.
pragma solidity 0.7.5;
import "./ownable.sol";
import "./destroyable.sol";
interface GovernmentInterface{
function addTransaction(address _from, address _to, uint _amount) external;
}
contract Bank is Ownable, Destroyable{
GovernmentInterface GovernmentInstance = GovernmentInterface(0xC2F2b73709026aD4A31b22C95689E47ae8423913);
mapping (address => uint) balance;
event depositDone(uint amount, address 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 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, "Insufficient Funds");
require(msg.sender != recipient);
uint previousSenderBalance = balance[msg.sender];
_transfer(msg.sender, recipient, amount);
GovernmentInstance.addTransaction(msg.sender, recipient, amount);
assert(balance[msg.sender] == previousSenderBalance - amount);
}
function _transfer(address from, address to, uint amount) private {
balance[from] -= amount;
balance[to] += amount;
}
}
pragma solidity 0.7.5;
import "./ownable.sol";
contract Destroyable is Ownable{
function DestroyContact() public {
selfdestruct(owner);
}
}
pragma solidity 0.7.5;
contract Ownable{
address payable public owner;
modifier onlyOwner(){
require(msg.sender == owner);
_;
}
constructor(){
owner = msg.sender;
}
}
When I compile I am little confused to whether the transfer functionality is working as I seem to be getting a revert error. This may just be me not fully understanding how to work with the remix platform or it may be in the code but I can’t seem to figure it out. Other then that it compiles and I am able to use the self-destruct function within the Bank contract.
Thanks for all your help.
1 Like
Hi @Raf.PV7,
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
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
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).
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.