Inheritance Assignment

[jahh]

Please let me know if this right thank you

[jon_m]

That’s all looking good now, Andreas

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

[yestome]

Yes, fully appreciate this is the most effective way to approach inheritance structures. -
and thanks for confirming my code!

[jon_m]

Hi @jahh,

You should be getting compiler errors for the following lines of code:

@jahh

function close() onlyOwner{
selfDestruct(owner);
}

If you read the error messages, they will tell you how to resolve them. Here is some additional information about the changes you need to make…

1. You need to declare each function’s visibility.

2. The Solidity language comes with selfdestruct() predefined and built-in. It must be written all in lower case letters, or the compiler will not recognise it as valid code.

3. When the selfdestruct() function is called it should automatically transfer the ether balance remaining in the contract to the address it is called with. For this to happen, the address (in your case, owner ) needs to be a payable address, but yours is currently non-payable. If you don’t know, or can’t find out how to convert owner to a payable address (there are alternatives), then have a look at other students’ solutions, and the feedback and suggestions they’ve received, posted in this discussion topic.

Although it isn’t incorrect to have all of your code in one contract (Bank), the aim of this assignment is to practise inheritance So now try moving (i) the selfdestruct() functionality to a contract (Destroyable) in a separate file, and (ii) the contract ownership functionality to a contract (Ownable) in another separate file, so you have 3 contracts/files in total. Then add the necessary code so that Bank inherits the selfdestruct() and Ownable functionalities, and both are available when we just deploy Bank i.e. you 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.

A couple of other observations:

• Your withdraw() function is missing
• Although it will still work without, it would be clearer to give the first unit parameter in your Deposit event declaration an identifier (a name).

Try to modify your code for these points. Just let me know if anything is unclear, or if you have any questions

[tom88norman]

Ownable contract

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

Destroyable contract

import "./ownable.sol";

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

Top of Bank contract…

pragma solidity 0.7.5;

import "./ownable.sol";
import "./destroyable.sol";

contract Bank is Ownable, Destroyable {

As always, any feedback is most welcome!

[Rafael_Albuquerque]

Is it better to use payable(msg.sender) in the owner contract?

contract Ownable {
    address payable public owner;
    
    constructor() {
        owner = payable(msg.sender);
    }
    
    modifier onlyOwner() {
        require(msg.sender == owner, "Only contract owner can do this");
        _;
    }
}

Solidity 0.8.1 need to explicit owner as payable.

pragma solidity 0.8.1;

import "./Ownable.sol";

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

[jon_m]

An excellent solution @tom88norman

Well done for realising that you needed to make the owner address payable , so that selfdestruct() is called with a payable address and, therefore, any remaining ether in the contract is paid/transferred to the owner when selfdestruct() is triggered.

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.

Don’t forget that you need to declare the solidity compiler version at the top of every file with:
pragma solidity ...;  (you may have already included this in each of your own files, but I thought I’d mention it just in case).

You’re making great progress!

[jon_m]

Nice solution @Rafael_Albuquerque

You have done a good job of using the appropriate syntax for Solidity v.0.8.1

Well done for realising that you needed to make the owner address payable , so that selfdestruct() is called with a payable address and, therefore, any remaining ether in the contract is paid/transferred to the owner when selfdestruct() is triggered.

@Rafael_Albuquerque

Is it better to use payable(msg.sender) in the owner contract?

Do you mean in the Destroyable contract, as follows?

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

…meaning that you can leave owner as a non-payable address (in Ownable) as follows?

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

If you define an address state variable as payable, then it will be payable whenever it is referenced without having to explicitly convert it each time. This is why in your posted solution you are able to call selfdestruct() as follows:

selfdestruct(owner);

However, if your address state variable only needs to be payable in specific situations, it is better practice (and more secure) to store it in the contract state as a non-payable address, and then convert it locally whenever it needs to be payable e.g.

selfdestruct(payable(owner));

… or use a suitable alternative such as   payable(msg.sender)

Can we also 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).

Let me know if anything is unclear, if I have misunderstood your question, or if you have any further questions

[tom88norman]

Awesome, thanks. Yeah it took a bit of error message deciphering to figure out I needed to make the owner address payable for this to work but got there in the end. Thanks again for the reply!

[vili]

function close() public onlyOwner { //onlyOwner is custom modifier
selfdestruct(msg.sender); // owner is the owners address
}

The code I created, I have it on my bank contract but you can create a new contract I guess, works the same

[jon_m]

Hi @vili,

@vili

The code I created, I have it on my bank contract but you can create a new contract I guess, works the same

Yes, but the aim of this assignment is to practise inheritance. So now try moving (i) the selfdestruct() functionality to a separate contract (Destroyable) in a separate file, and (ii) the contract ownership functionality to a separate contract (Ownable) in another separate file (if you haven’t already done so). You should have 3 separate contracts/files in total. Then add the necessary code so that Bank inherits the selfdestruct() and Ownable functionalities, and both are available when we just deploy Bank i.e. you 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.

@vili

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

This is correct, but once you’ve set up your inheritance structure, can we see how you have implemented this close() function, and what code you’ve added for the inheritance, by posting your Ownable and Destroyable contracts, and the start of your Bank.sol file, and Bank contract header?

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

[decrypter]

Ok, it was not clear that you wanted the derived contracts as well; the assignment only asked for the Destroyable contract. I’ll get back to you.
Regarding the internal visibility: the intention was to prevent anyone from directly calling
Destroyable.close() if it is overloaded in a derived contract. Now, I did some research and it seems like solidity, unlike other languages, does not allow this anyway. If a function is overloaded there is no way to explicitly call the parent contract implementation from the outside (like remix), correct?

[jon_m]

Hi @decrypter

@decrypter

Now, I did some research and it seems like solidity, unlike other languages, does not allow this anyway.

Like other statically-typed programming languages, Solidity does allow overloading. Although, from your post, I’m not entirely sure we are using the term to refer to the same thing. Have a look at this discussion about overloading (polymorphism) in Solidity, and let me know if this is what you are referring to.

I’m not clear about what exactly you mean by…

@decrypter

Regarding the internal visibility: the intention was to prevent anyone from directly calling
Destroyable.close() if it is overloaded in a derived contract.

It is the derived contract, not the parent contract, that we are deploying. When the derived contract is deployed, close() will be available via inheritance. It’s the onlyOwner modifier that is already preventing anyone from calling close() and destroying the derived contract. And for the contract owner to call close(), it needs to have public visibility (if it had external visibility, then it wouldn’t be inherited).
However, if you post your derived contract with your specific implementation, then I will be able to see from your code what it is you intend, and can comment more meaningfully.

@decrypter

If a function is overloaded there is no way to explicitly call the parent contract implementation from the outside (like remix), correct?

Here, it seems like you are using “overloaded” to mean “inherited”. Overloading in Solidity has nothing to do with where a function can be called from — that’s determined by the visibility. To be clear, if we only deploy Bank, then close() will only be available to be called on the Bank contract. Even though Destroyable’s functionality is inherited, it is not our intention to also deploy the Destroyable contract. But even if we were to deploy both Bank and Destroyable, if close() was called on Destroyable, instead of on Bank, then Destroyable would be destroyed, not Bank. But it makes no sense to deploy Destroyable, because its purpose is to separate out certain core functionality, and to allow this same core functionality to be implemented in contracts that inherit it, and not for it to be used separately for its own sake.

Anyway, to avoid confusion and talking at cross purposes, I think the best thing is to see both contracts for your original solution

[yestome]

Hello Jon,

I’m not quite sure you intended to send this to me as I’m not decrypter …?

Kind regards -
yestome / Andreas

[1984]

@jon_m thx a lot for your detailed comments and corrections. One can feel that you do that with love

[Deini-Atakan]

pragma solidity 0.7.5;

contract Ownable{
    
    address payable owner;
    
    constructor(){
        owner = msg.sender;
    }
    
    modifier ownerOnly{
        require(msg.sender == owner, "You aren't the owner");
        _;
    }
}

This is going to check the owner and deal with everything about being owner.

pragma solidity 0.7.5;

import "./Ownable.sol";

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

And this is for making the self destruct only possible by the owner.

pragma solidity 0.7.5;

import "./Destroyable.sol";


contract Bank is Destroyable{
    
    mapping(address => uint) balance;

    event depositSuccessful(uint amount, address indexed _from);
    event withdrawSuccessful(uint amount, address indexed _to);
    
    event fundsTransfered(uint amount, address indexed _from, address indexed _to);
    
    
    function deposit() public payable returns(uint){
        balance[msg.sender] += msg.value;
        emit depositSuccessful(msg.value,msg.sender);//sending out log that deposit is done
        return balance[msg.sender];
    }
    
    function withdraw(uint _amount)public returns(uint){
        require(balance[msg.sender] >= _amount, "Not enough funds");
        balance[msg.sender] -= _amount;
        msg.sender.transfer(_amount);
        emit withdrawSuccessful(_amount,msg.sender);
        return balance[msg.sender];
    }
    
    function getBalance() public view returns(uint){
        return balance[msg.sender];
    }
    
    function transfer(uint _amount,address _to) public{
        require(balance[msg.sender] >= _amount, "Not enough balance");
        require(msg.sender != _to, "Can't send funds to yourself?");
        _transfer(msg.sender,_to,_amount);
        emit fundsTransfered(_amount, msg.sender, _to);
    }
    
    function _transfer(address _from,address _to, uint _amount) private{
        balance[_from] -= _amount;
        balance[_to] += _amount;
    }
        
    
    
}

And this is the latest code after all is done. I hope it’s right

[Kippie]

Assuming that a contract “Ownable” contains the logic defining a specific owner

pragma solidity 0.7.5;

import "./Ownable.sol"

contract Destroyable is Ownable{

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

[jon_m]

Hey Andreas,

In my reply to decrypter, I linked to our Polymorphism discussion, as it was relevant. I think you may have got a notification as well, because it is your discussion thread (which you started with your question) that I linked to. I’ve checked my reply to decrypter and it’s definitely sent to him and not you, so I think you just got notified.

Thanks for the message though — strange things can happen sometimes

[jon_m]

@1984

One can feel that you do that with love

Of course

[yestome]

Jon -
relevant yes it is - thanks for the notification !