Thanks jon_m.
I had to go back to the previous lectures to better understand the matter but i get it now.
1 Like
Im sorry to ask so many questions but after testing the destroyable contract I noticed that if other addresses deposit fund in the bank contract and then the owner triggers the destroy function all their funds will be sent to him, right?
Then how can users trust a dapp with this capability?
1 Like
pragma solidity 0.7.5;
contract Ownable {
address owner;
constructor(){
owner = msg.sender;
}
modifier onlyOwner{
require(owner == msg.sender);
_;
}
}
pragma solidity 0.7.5;
import “./Ownable.sol”;
contract Destroyable is Ownable{
function destroy() public onlyOwner{
address payable reciever = msg.sender;
selfdestruct(reciever);
}
}
pragma solidity 0.7.5;
import “./Ownable.sol”;
import “./Destroyable.sol”;
contract Bank is Ownable, Destroyable {
mapping (address => uint) balance;
event depositDone(uint amount, address indexed depositedTo);
function deposit() public payable{
balance[msg.sender] = msg.value;
emit depositDone(msg.value, msg.sender);
}
function withdraw(uint amount) public payable onlyOwner{
require(balance[msg.sender] >= amount);
msg.sender.transfer(amount);
//balance[msg.sender] -= amount;
}
function getbalance() public view returns(uint){
return balance[msg.sender];
}
}
1 Like
Hi @guibvieira,
Can we see your other two contracts which are also part of this inheritance structure?
Don’t forget to also import the file which contains Ownable.
Passing owner instead of msg.sender to selfdestruct() relies on you having made an additional change in Ownable. That’s why we also need to see Ownable as part of your solution.
Apart from this, your Destroyable contract is looking good 
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.
Let us know if anything is unclear, or if you have any questions 
Nice work @Meriem 
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.
I couldn’t help noticing that you have a couple of issues in your Bank contract. I know this isn’t part of this assignment, so I’m wondering if this is just a copy-and-paste issue, and if the contract you’ve been deploying and testing doesn’t have these errors…
function deposit() public payable{
balance[msg.sender] = msg.value; // operator should be +=
emit depositDone(msg.value, msg.sender);
}
// When we deposit ether, we want to add it to our balance, not replace it!
/* This function doesn't need to be payable, because it's sending
not receiving ether */
function withdraw(uint amount) public payable onlyOwner{ //remove payable
require(balance[msg.sender] >= amount);
msg.sender.transfer(amount);
//balance[msg.sender] -= amount; //include this before transfer()
}
You’re not… questions are good! 
That’s a very good question, and shows you’re really thinking through what’s happening while doing your testing.
We would only actually use selfdestruct() in limited circumstances. And you have quite rightly pointed out one of its disadvantages. The point of selfdestruct() is to prevent or reduce the impact of a much worse scenario: an attacker finding a way to exploit an unforeseen vulnerability in the contract code and stealing all the funds. Having the funds go to an “owner” entity obviously relies on a certain degree of trust, but this is obviously preferable than losing everything to a malicious attacker, and we are much more likely to get a refund (or at least a partial one)! In our simple example, a single owner address triggers selfdestruct() and the transfer of the remaining funds to their own address. In reality, it might need the authorisation of more than one addresss, and the transfer of funds could be to a different contract address instead of a wallet address. However, my understanding is that, in practice, use of selfdestruct() is not encouraged, and other types of security mechanisms are built into smart contracts instead, such as the implementation of proxy contracts. You will learn more about the practicalities of this if you go on to take the Smart Contract Security course.
If you do some of your own research about selfdestruct() you will find that it is by no means a perfect security solution, and comes with specific weaknesses and disadvantages. However, its relatively straightforward and easy-to-understand functionality makes it ideal to use in an introductory example of how inheritance works. And to be honest, we’ve used it more as a way to demonstrate how inheritance works and is coded in Solidity, rather than to teach you selfdestruct() for its own particular merits.
I hope that goes some way towards answering your question
Noted. Your answers are very detailed and clear.
Something else I have noticed.
If the transfer function is onlyOwner then if an address other than the contract owner deposits funds into the bank contracts it won’t be able to withdraw. And since transfer only concerns msg.sender I think there is no issue if we make the withdraw function just public (not onlyOwner).
I have tested it and it works fine but I don’t know if this the best solution.
What do you think?
1 Like
Hi @Meriem,
Yes, you’re absolutely right. I think during the course we were adding and removing onlyOwner from various functions to demonstrate various different scenarios. Somehow it ended up being left in the withdraw() function. We would only want to restrict the withdrawal of funds to the owner, whilst allowing any address to deposit funds into the contract, in scenarios such as: donating funds to a project; users depositing funds within a dapp where, once deposited, are restricted to being spent on transactions related to that dapp only; or where there are other mechanisms for withdrawal provided by functions other than withdraw(). The different permutations are endless.
So, here, there isn’t really a “best” solution: it just depends on what scenario you want to cater for. You will get so much more out of these assignments by doing exactly what you’re doing — experimenting with the code, and modifying and testing it with a view to improving it, and adapting it to different scenarios and use cases that are meaningful to you.
… 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