Inheritance Assignment

[jon_m]

Hi @omkar_c,

@omkar_c

why do we need to create a new payable variable name receiver and assign it to msg.sender and then destroy the receiver instead of destroying the owner variable directly.

You don’t need to do it that way. It is a more long-winded (or some would say more clearly broken-down step-by-step) version of the following equally valid solution:

selfdestruct(msg.sender);

We can use msg.sender to reference the contract owner’s address, because the onlyOwner modifier restricts access to the destroy() function to the contract owner, and so msg.sender can only ever be the contract owner address anyway.

In Solidity v0.7, msg.sender is a payable address type by default, and so there is no need to explicitly convert it whenever we need it to be payable, e.g. in selfdestruct() . However, in Solidity v0.8, msg.sender is now non-payable by default, but instead of converting it to a payable address via a separate variable, we can perform the conversion within the selfdestruct() function call itself…

selfdestruct(payable(msg.sender));

Alternatively, you can reference the owner variable by either:

(1) Declaring the owner state variable in Ownable with a payable address type, instead of with the default, non-payable address type; or

(2) If you only need this address to be payable for the purposes of executing selfdestruct(), you can leave the owner state variable as non-payable, and explicitly convert the address to payable locally, within the function where you actually need to use it as payable. You can even do this directly within the selfdestruct() function call itself, as follows:

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

Have a look at this post for further details about the use of a separate receiver variable in the model solution.

---

And just to clarify… we are not destroying the contract owner’s address. We’re destroying the smart contract. The owner’s address is passed to selfdestruct() because, as well as destroying the contract, calling selfdestruct() will transfer the contract’s remaining ether balance to its mandatory payable address parameter. To receive ether an address must be payable.

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

[jon_m]

Hi @simfin94,

Your Destroyable contract is looking good but can we also see your code for the other two contracts in your inheritance structure?

We especially 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 an important part of the solution, because the aim of this assignment is for our Bank contract to inherit both the selfdestruct() and contract 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).

It would also help to see your Ownable contract, because you’ve used the same name owner for your local payable address variable (in the destroy function in Destroyable) as the state variable in Ownable — unless of course you’ve changed the name of the state variable… Even if they do have the same name, your solution still works, but if you’re going to assign msg.sender (as the owner address) to a separate variable before passing it to selfdestruct(), then I think it would be much clearer to give this variable a different name.

Alternatively, you could use one of the other methods for passing the contract owner address to selfdestruct() , which I’ve outlined in this post.

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

[chen]

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

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

pragma solidity 0.7.5;

contract Ownable{
    address payable public owner;
    
    constructor(){
        owner = msg.sender;
    }
}

pragma solidity 0.7.5;

import "./Destroyable.sol";
import "./Ownable.sol";
contract Bank is Ownable,Destroyable{
   mapping(address => uint) balances;
   
  function withdraw(uint amount) public {
     
      owner.transfer(amount);
      balances[owner] += amount;
      close();
  }
 
}

[chen]

I don’t see Destroyable 's destroy function is called in Bank contract.
If destroy is not called, then why you defined the destroy function in parent contract?

[blockchain.PE]

ownable.sol

contract Ownable {

address owner;

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

constructor() {
    owner = msg.sender;
}

}

destroyable.sol

import “./ownable.sol”;

contract Destroyable is Ownable {

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

}

bank.sol

pragma solidity 0.7.5;

import “./ownable.sol”;
import “./destroyable.sol”;

contract Bank is Ownable, Destroyable {
…
…
…

[jon_m]

Overall, a very nice solution @gaio

A few comments…

(1) Your inheritance structure works, but it is not multi-level inheritance. We can streamline the inheritance structure by having Bank explicitly inherit Destroyable only, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.

(2)

@gaio

2 - in Bank (which is a bit messy code, because I’m playing around with some potential modifiers), since Ownable is being derived through Destroyable, is the constructor required? I just tried it in Remix with and without the constructor, and both ways worked. I guess I would only need a constructor here if I was adding in addition state variable definitions only needed in Bank.

Exactly right You are right to remove the owner state variable from Bank, because this is now inherited from Ownable.

@CryptoKnight84

Definitely not required to define the same variable again.

This means that the constructor is also now redundant in Bank and should also be removed to avoid unnecessary code duplication (which is one of main reasons for using inheritance). On deployment, the constructor in Ownable assigns the contract owner’s address to the owner state variable, and so Bank, as well as Destroyable, can both access the owner’s address by referencing owner (which you’ve done appropriately without a constructor in Destroyable).

(3) You are missing the require statement in withdraw(), which is an essential check to ensure the caller has enough ether to cover the requested amount. You could either add it directly to the function body, or implement your enoughBalance modifier like @thecil explained here. The advantage of using the modifier is that it can also be used in transferTo(), which would avoid code duplication. However, if you’re not going to use your modifiers, then you should comment our their code entirely (not just their bodies).

(4) Have a look at this post which explains the importance of the order of the statements within your withdraw function body.

(5) If you want to import Buidler’s console.log then you need to correct the spelling of “buidler” in the import statement. Have you downloaded the Buidler EVM with npm, using the instructions in the article you linked to? I would probably wait until you’ve completed the 201 course before you try experimenting with it. In the 201 course you’re going to use Truffle and the local Ethereum network Ganache. From reading the article it seems the Builder EVM is an alternative to Ganache, so I would probably avoid using both to start with, which could lead to confusion…

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

[jon_m]

@gaio

Questions (about my contract code above):

1 - why in Destroyable can’t I just include the owner state variable in selfdestruct(owner) ?
I saw most everyone using selfdestruct(msg.sender). Why? If owner is already declared and required to be payable msg.sender in Ownable, why not just refer to owner?

@CryptoKnight84

I think its clearer to read if its the owner. In the 0.8.6 version there are no implicit conversions from address to address payable. I would think explicitly stating an address is payable improves readaility.

Just to be clear on these alternative solutions…

It is correct to use either msg.sender or owner to reference the owner’s address as the argument passed to selfdestruct(). Which one you use depends on (i) considering how you may need to use the owner state variable elsewhere in the smart contract (throughout the whole inheritance structure), (ii) code readability, and (iii) personal preference (different developers may have differing opinions about what they consider to be clear, readable code etc. However, before any personal preference, it is important to consider both code readability and re-usability for all developers who may potentially need to work with your code.

We also need to ensure that the address argument passed to selfdestruct() is a payable address type, in order for the contract’s remaining ether balance to be transferred to that address when the contract is destroyed. To receive ether an address must be payable.

(1) As you have, you can declare the owner state variable in Ownable with a payable address type, instead of with the default, non-payable address type.

address payable public owner;

You will then be able to use owner as the argument in selfdestruct() …

selfdestruct(owner);

(2) However, if you only need the contract owner’s address to be payable for the purposes of executing selfdestruct(), you can leave the owner state variable as non-payable, and explicitly convert the address to payable locally, within the function where you actually need to use it as payable…

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

But you can also do this directly within the selfdestruct() function call itself, which I think is more straightforward and concise …

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

(3) We can also use msg.sender to reference the contract owner’s address, because this is the only address that it can ever represent in the destroy() function (as a result of the onlyOwner modifier restricting who can access this function in the first place). Prior to Solidity v.0.8, msg.sender is payable by default, and so doesn’t need to be explicitly converted…

selfdestruct(msg.sender);

However, from v.0.8, msg.sender is non-payable by default, and so whenever you are using v.0.8 and need to use msg.sender as a payable address, this requires an explicit conversion e.g.

selfdestruct(payable(msg.sender));

Using msg.sender means that we don’t have to declare the owner state variable as a payable address, if this is unnecessary.

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

[jon_m]

@gaio

why is owner made internal?

contract Ownable {
    address internal owner;

@CryptoKnight84

If its internal it’s only accessible to the contract. I think anyone should be able to query which address deployed the contract, so making it a secret is kind of pointless.

@CryptoKnight84 makes a valid point  However, as is often the case, there is no right or wrong answer here, especially in our simplified example. In reality it will depend on the specific use case of the smart contract, which may require more or less visibility (restrictiveness) for certain information and operations. Less visibility usually means more security. Because smart contract security is of upmost importance for their success and adoption, it is best practice to keep visibility restricted to only what is necessary.

In actual fact, state variables have internal visibility by default (unlike functions which must always have their visibility stated explicitly). It’s not wrong to add internal to the state variable declaration, but it’s unnecessary, and   address owner;   will still define it with internal visibility. It may be that the keyword internal has been included in the solution code to highlight the fact that state variables (as well as functions) with either public or internal visibility are inherited.

[jon_m]

Hi @chen,

@chen

I don’t see Destroyable 's destroy function is called in Bank contract.
If destroy is not called, then why you defined the destroy function in parent contract?

When you deploy Bank, you will notice that the close() function is available to call externally from Remix. This is because it has public visibility. If we gave it internal visibility, then it would still be available in Bank, but could only be called internally from another function within the deployed Bank contract. When close() is executed, this triggers selfdestruct().

Destroyable’s close() function is inherited by the Bank contract, and so it is available to call when Bank is deployed. With this inheritance structure we only need to deploy the derived contract Bank, and the functionality inherited from Ownable and Destroyable will be available as if they were coded within Bank.

Apart from…

@chen

import "./Destroyable.sol";
import "./Ownable.sol";
contract Bank is Ownable,Destroyable{

… there is no need to make any further modifications to your Bank contract. That way your Bank contract will operate as before, with users being able to make deposits, withdrawals and transfers, but with the owner also being able to call the inherited close() function if they need to destroy the contract. If you perform some transactions before the owner destroys the contract, you will notice that when selfdestruct() is triggered any ether remaining in the contract (the contract address balance) is automatically transferred to the owner’s external address and added to its ether balance (visible in the Account field).

So, the owner doesn’t need to call a separate function in Bank in order to call close() internally…

@chen

function withdraw(uint amount) public {
     
      owner.transfer(amount);
      balances[owner] += amount;
      close();
  }

… and as well as destroying the contract, triggering selfdestruct() automatically transfers any ether remaining in the contract to its payable address argument (owner).

@chen

selfdestruct(owner);

You don’t need to try to code this separately.

Let me know if this is still unclear.

[jon_m]

Hi @chen,

You have coded your inheritance structure correctly We can also streamline the inheritance structure by having Bank explicitly inherit Destroyable only, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.

In addition to the other comments I’ve made in my other post, you also need to restrict who can destroy the contract. By passing owner to selfdestruct() you are ensuring that, if the contract is destroyed, any remaining funds will only be transferred to the owner (who we are assuming will be a trustworthy custodian of users’ funds in the interm period, until they can either be transferred to a new contract, or refunded). However, currently, anybody can call close() and trigger selfdestruct() in the first place. Even though the funds are essentially safe, allowing anyone to trigger the contract-destruction and fund-transfer process is obviously highly volatile!

If you’re not sure how to implement this additional restriction, have a look at some of the other students’ solutions posted here in this discussion topic. Then, when you’ve made all of the necessary modifications, post your revised solution and I’ll take a look

[jon_m]

Nice solution @blockchain.PE

We can also streamline the inheritance structure by having Bank explicitly inherit Destroyable only, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.

[chen]

Will the Bank contract be destroyed automatically without calling destroy function ?

[jon_m]

Hi @chen,

By “destroy” function are you referring to your close() function or selfdestruct()?
You don’t have a destroy() function in your code.

[chen]

I’m referring to your solution.

[jon_m]

This solution?
https://github.com/filipmartinsson/solidity-0.7.5/blob/main/inheritance-assignment/Destroyable.sol

[chen]

yes.
right…

[jon_m]

@chen

Will the Bank contract be destroyed automatically without calling destroy function ?

No …
When you deploy Bank, you will notice that the destroy() function is available to call externally from Remix (along with all of the other functions in Bank). This is because it has public visibility. If we gave it internal visibility, then it would still be available in Bank, but could only be called internally from another function within the deployed Bank contract.

When destroy() is called (hopefully restricted to the owner) this then automatically triggers selfdestruct() … which automatically transfers the remaining ether in the contract to its payable address argument, and destroys the contract.

[jon_m]

I’ve just edited my reply to make it clearer

[Aiden_Calhoun]

Ownable

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

Destroyable

pragma solidity 0.7.5;

import "./Ownable.sol";

contract Destroyable is Ownable {

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

Bank

pragma solidity 0.7.5;


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



contract bank is Ownable, Destroyable{

[jon_m]

Nice solution @Aiden_Calhoun

We can also streamline the inheritance structure by having Bank explicitly inherit Destroyable only, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.

---

@Aiden_Calhoun

address payable receiver = msg.sender;
selfdestruct(receiver);

Here, a concise alternative is to call selfdestruct() with msg.sender directly, instead of using the additional receiver variable.

selfdestruct(msg.sender);

Have a look at this post for further details about the use of a separate receiver variable in the model solution.

Keep up the great work!