Inheritance Assignment

[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!

[EduardoDaSilva]

pragma solidity 0.7.5;

import "./Destroyable.sol";

contract Bank is Destroyable {
    

pragma solidity 0.7.5;

import "./Ownable.sol";

contract Destroyable is Ownable {

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

pragma solidity 0.7.5;

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

[jon_m]

Another good solution @EduardoDaSilva

Your implementation of a multi-level inheritance structure is the best approach, because it is more streamlined. Bank only needs to explicitly inherit Destroyable, because it will implicitly inherit Ownable via Destroyable.

---

@EduardoDaSilva

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

[Duende]

pragma solidity 0.7.5;

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

contract Bank is ownable {

}
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);
    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, "Balance no sufficient");
    require (msg.sender != recipient, "Dont transfer money to yourshelf");
    
    uint previussendbalance=balance[msg.sender];
    
    _transfer(msg.sender,recipient, amount);
    
    assert (balance[msg.sender] == previussendbalance - amount);
}
    


function _transfer ( address from, address to, uint amount) private {
    balance [from]-=amount;
    balance[to]+=amount;
    
    
}

}

type or paste code here
```pragma solidity 0.7.5;

contract destroyable {
 address payable private owner;
 uint amount;
 
 constructor() {
  owner = msg.sender;
 }
 
 function retrieve() public view returns (uint amount){
  return balance [msg.sender] ;
 }
 
 function close() public { 
  selfdestruct(owner); 
 }
}`Preformatted text`

pragma solidity 0.7.5;

contract Ownable {
    address public owner;
    
    modifier only owner }
        
        require(msg.sender == owner);
        
    }
    
    constructor () {
        
        owner= msg.sender;
    }
}`Preformatted text`

[kylecullen]

Question? How do you call the destroy function after the bank contract is deployed?

pragma solidity 0.7.5;

import "./Ownable.sol";

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

pragma solidity 0.7.5;

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

contract Bank is Ownable, Destroyable{

[Aiden_Calhoun]

Taking note @jon_m !
I originally tried to have Bank explicitly inherit Destroyable, but for some reason, the Ownable contract was not being inherited through destroyable. I’m not sure if it was an issue with my code or a problem with the Remix IDE, but my only solution that was working was to have Bank inherit both separately.

Thank you!