Inheritance Assignment

[jon_m]

Hi @Aiden_Calhoun,

@Aiden_Calhoun

… for some reason, the Ownable contract was not being inherited through destroyable. … my only solution that was working was to have Bank inherit both separately.

That’s strange, and I can’t think why that would have happened. If you post your complete code for all 3 contracts, I’ll run it in Remix and find out what’s happened.

[jon_m]

Nice solution @kylecullen

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.

@kylecullen

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

The owner doesn’t call selfdestruct() directly. It’s placed within another function, which you’ve called close(), and it’s that function which is inherited by Bank and available to call externally in Remix when Bank is deployed: it will appear as an orange button, along with all of Bank’s other functions. When close() is called, this will automatically trigger selfdestruct() within its function body.

Due to the onlyOwner restriction, only the contract owner’s address can call close(), so you need to make sure that the Account field (at the top of the Deploy & Run Transactions panel) is set to the owner’s address.

Just let me know if anything is still unclear, or if you have any further questions.

[Aiden_Calhoun]

Sure, here it is.
Bank

pragma solidity 0.7.5;

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

interface governmentInterface{
    function addTransaction(address _from, address _to, uint _amount) external payable;
}

contract bank is Ownable, Destroyable{
    
    governmentInterface governmentInstance = governmentInterface(0xE5363f1b12023d5A402D551362746f684B5c6532);
    
   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 returns (uint){
        require(balance[msg.sender] >= amount);
        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 not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
        
        governmentInstance.addTransaction{value: 1 ether}(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;
    }
    
}

Destroyable

pragma solidity 0.7.5;

import "./Ownable.sol";

contract Destroyable is Ownable {

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

Ownable

pragma solidity 0.7.5;

contract Ownable {
    address public owners;
    uint lmit;
    
    modifier onlyOwner {
        require(msg.sender == owners);
        _; 
        
    }
    
    constructor(){
        owners = msg.sender;
    }
}

[jon_m]

Hey Aiden,

It still works for me when I remove…

import "./Ownable.sol";

and change the contract header to …

contract bank is Destroyable {

When I say works, I mean it compiles and deploys; and then I can test the selfdestruct functionality by performing the following transactions …

• The owner calls deposit() with say 5 ether — the owner’s external account balance reduces by 5 ether, and their individual Bank account balance increases by 5 ether.
• The owner calls destroy() — the contract is destroyed and the remaining contract balance of 5 ether is transferred to the owner’s address (their external account balance increases by 5 ether).

Were you amending the code for the inheritance as I have? If so, then what exactly isn’t working when you try it?

[Aiden_Calhoun]

Oddly, I haven’t made any changes within the code, but it will compile and deploy the Bank contract now just fine (with the multi-level inheritance). I originally amended the code for inheritance as you have, but it would not compile for some strange reason. But it’s working now and I’ll take that as a win!

[jon_m]

Hi @Duende,

It’s good to see you back after a while

Unfortunately, there are quite a few issues with your code. I’ve also noticed that you haven’t posted the first 2 assignments for this 101 course (Data Location, and Events). Have you watched all of the videos for the first part of this course, and practised with the code until you are comfortable with all of the concepts and syntax? I’ve also seen that you’ve jumped ahead to the 201 course, but I strongly recommend that you go back over this course and complete it, before moving on to 201, because you need to be comfortable with all of the basics first.

I’ve seen that you’ve corrected most of the issues @dan-i mentioned in his feedback on your
Transfer Assignment but don’t forget that…

@dan-i

You should also increase and decrease balance[msg.sender] in your deposit and withdraw functions.

You’ve increased the individual account holder’s balance in the deposit function, but you haven’t decreased it in the withdraw function.  msg.sender.transfer(amount) transfers ether from the contract address balance to the caller’s external wallet address, but it doesn’t adjust their individual user balance in the mapping. These balances perform an internal accounting role, and record each user’s share of the contract’s total ether balance. If you don’t reduce a user’s individual balance for each amount withdrawn, they can continue to make repeat withdrawals up to the total amount of ether held in the contract. In other words, not only can they withdraw their own funds, but everyone else’s too!

You’ve also modified the withdraw function with the onlyOwner modifier, but this means that only the contract owner will be able to withdraw funds, whereas the contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so it only seems fair that all users should be able to withdraw their funds as well, don’t you think?

---

Coding for inheritance

@Duende

pragma solidity 0.7.5;

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

contract Bank is ownable {

}
contract Bank is destroyable {
  … …
}

Your first import statement is correct, but your second is missing something. Can you see what it is?

If you want a single derived contract to inherit two parent contracts, you can’t declare the same derived contract twice. You need to use the following syntax…

contract Bank is Ownable, Destroyable { ... }

You also need to make sure you are using exactly the same contract names used in the parent contracts, including consistent use of upper/lower case letters, otherwise you’ll get a compiler error:

@Duende

pragma solidity 0.7.5;

contract Ownable { ... }

To avoid this kind of error, it’s best to stick to the convention of starting all contract names with a capital letter.

If Destroyable inherits Ownable, then the owner state variable and the constructor in Ownable will be available in Destroyable, meaning that you don’t need to include them in both contracts. Being able to avoid code duplication is one of the reasons for using inheritance.

---

Other code in Ownable

@Duende

modifier only owner }     
    require(msg.sender == owner);
        
}

This modifer in Ownable has 3 syntax errors — can you see what they are and correct them? Here are 3 hints:

1. Your modifier’s name cannot contain any spaces.
2. Check your opening and closing brackets.
3. You are missing the line of code that means “now execute the function”.

---

selfdestruct() functionality in Destroyable

@Duende

function close() public { 
  selfdestruct(owner); 
}

When you’ve corrected your code so that Bank successfully inherits Destroyable, calling the close() function will destroy the Bank contract, and the remaining contract balance will automatically be transferred to the owner’s external address However, currently, anybody can call close() and trigger selfdestruct() in the first place. Even though the funds are essentially safe — we are assuming the owner will be a trustworthy custodian of users’ funds in the interm period, until they can either be transferred to a new contract or refunded — allowing anyone to trigger the contract-destruction and fund-transfer process obviously creates a highly volatile environment! So you need add some code to restrict access to this function to the contract owner.

As well as destroying the contract, triggering selfdestruct() also automatically transfers any ether remaining in the contract to its payable address argument (owner). So you don’t need to try to code this separately. I’m not sure if this is what you were trying to do here …

@Duende

function retrieve() public view returns (uint amount){
  return balance [msg.sender] ;
}

… but, in any case, all this function could potentially do is allow the caller to view their individual balance, which duplicates what the getBalance() function in Bank already does. And anyway, this function won’t compile in Destroyable because the balance mapping is declared in Bank, not Destroyable, so  return balance[msg.sender]  throws an error.

Have a go at making these modifications, and I’ll take a look at it again for you. But let me know if anything is unclear, or if you have any questions

[david.maluenda]

Adding a forgotten assignment, It has a little Extra on it, meaning it has a little more code to it then the lecture requires.

Bank

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(0xaE036c65C649172b43ef7156b009c6221B596B8b);


mapping(address => uint)balance;


event depositDone(uint amount, address indexed depositTo);
event transferDone(address from, address to, uint amount);


function deposit() public payable onlyOwner 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);
    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);
    require(msg.sender != recipient);
    
    uint balanceBeforeSending = balance[msg.sender];
    
    _transfer(msg.sender, recipient, amount);
    
    governmentInstance.addTransaction(msg.sender, recipient, amount);
    
    assert(balance[msg.sender] == balanceBeforeSending - amount);
}

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

}

Ownable
pragma solidity 0.7.5;

contract Ownable {

address internal owner;



   modifier onlyOwner() {
    require(msg.sender == owner);
    _;
}


constructor() {
    owner = msg.sender;
}

}

Destroyable

pragma solidity 0.7.5;

import “./Ownable.sol”;

contract Destroyable is Ownable {

function Destroy() public onlyOwner {
    address payable recipient = msg.sender;
    selfdestruct(recipient);
} 

}

[EduardoDaSilva]

Good catch!
Thank you

[jon_m]

Hi @david.maluenda,

Overall, your coding of the inheritance structure and the selfdestruct functionality is good

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.

@david.maluenda

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

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

selfdestruct(msg.sender);

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

---

Access to your deposit() function is now restricted by the onlyOwner modifier, which wasn’t included in the function header before. This should be removed, otherwise no one other than the contract owner will be able to deposit ether into the Bank contract — unless this is what you intend, and then for the owner to distribute this ether to other users via the transfer() function … ?

Don’t forget to address both of these additional points I mentioned about your Bank contract. It’s especially important to include the additional statement in your withdraw function to adjust the user’s individual balance, otherwise your contract will fail to prevent users from withdrawing more ether than they are entitled to.

Just let me know if you have any questions.

[david.maluenda]

Hi Jon,

Could you please show me in the code where and how you would do the changes?
It would give a more visual understanding.

Much appreciated and thanks for your input.

[Shadowstep2003]

How is the below? Contract bank code wasn’t really changed, just the headers to import

pragma solidity 0.7.5;

contract Ownable {
    
    address payable public owner; 
     
     // When placed in a function this will run first
    modifier onlyOwner {
     require(msg.sender == owner);
     _; // run the function that called this
    }
    
    constructor(){
     owner = msg.sender;
    }
     
}

pragma solidity 0.7.5;

import "./artifacts/Ownable.sol";

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

pragma solidity 0.7.5;

import "./Destroyable.sol";

contract Bank is Destroyable {

[jon_m]

Hi @david.maluenda,

Below each of the changes I suggested, I’ve copied the relevant part of your code and shown you how to modify it. I’ve commented out code which would need to be removed, and written comments to show where I’ve added code …

@jon_m

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.

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 { ... }

---

@jon_m

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

function Destroy() public onlyOwner {
    // address payable recipient = msg.sender;
    selfdestruct(/*recipient*/msg.sender); // recipient REPLACED with msg.sender
}

---

@jon_m

Access to your deposit() function is now restricted by the onlyOwner modifier, which wasn’t included in the function header before. This should be removed, otherwise no one other than the contract owner will be able to deposit ether into the Bank contract — unless this is what you intend, and then for the owner to distribute this ether to other users via the transfer() function … ?

 function deposit() public payable /*onlyOwner */returns(uint) {
    balance[msg.sender] += msg.value;
    emit depositDone(msg.value, msg.sender);
    return balance[msg.sender];
}

---

@jon_m

It’s especially important to include the additional statement in your withdraw function to adjust the user’s individual balance, otherwise your contract will fail to prevent users from withdrawing more ether than they are entitled to.

function withdraw(uint amount) public returns(uint) {
    require(balance[msg.sender] >= amount);
    balance[msg.sender] -= amount;  // ADDED
    msg.sender.transfer(amount);
    return balance[msg.sender];
}

In order to reduce security risk, the statements should be in the following order within the function body:

1. Check inputs (require statements)
2. Effects (update the contract state)
3. External interactions

It’s important to modify the contract state for the reduction in the individual user’s balance…

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract to the external address…

msg.sender.transfer(amount);

… just in case there is an attack after the transfer, but before the state is modified to reflect this operation. You’ll learn about the type of attack this prevents, and how it does it, in the courses which follow this one.

---

@jon_m

• It’s better to emit the event at the very end of the function which completes the transaction i.e. transfer() not _transfer().
  _transfer() is what we call a helper function .
  Another important factor here is that if you emit an event in a helper function, you are restricting your ability to reuse that helper function. It will be more reuseable if we can call it from another function when we may not want to emit the same event — either a different one, or no event at all. That way we can reuse the helper function whenever we just want its functionality (i.e. the operations, or computation, that it performs).
• In general an emit statement is probably better placed after an assert statement.

function transfer(address recipient, uint amount) public {
    require(balance[msg.sender] >= amount);
    require(msg.sender != recipient);
    
    uint balanceBeforeSending = balance[msg.sender];
    _transfer(msg.sender, recipient, amount);
    
    governmentInstance.addTransaction(msg.sender, recipient, amount);
    
    assert(balance[msg.sender] == balanceBeforeSending - amount);
    emit transferDone(msg.sender, recipient, amount);  // MOVED TO HERE
    // Note that you need to change two of the arguments
}

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

I hope that’s clearer, but just let me know if you have any further questions

[jon_m]

Another great solution @Shadowstep2003

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.

Keep up the great coding!

[alp257]

// Ownable.sol file
contract Ownable {

address payable owner;

constructor() {
    owner = msg.sender;
}

modifier onlyOwner {
    require(msg.sender == owner, "You're not the owner of the contract.");
    _; // run the function
}

}

// Destroyable.sol file

import “./Ownable.sol”;

contract Destroyable is Ownable {

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

}

// Bank.sol file

pragma solidity 0.7.5;

import “./Destroyable.sol”;

contract Bank is Destroyable {
…
}

[CryptoByNight]

pragma solidity 0.7.5;

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


pragma solidity 0.7.5;

import "./Ownable.sol";

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


pragma solidity 0.7.5;

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

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

[jon_m]

Great solution @alp257

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.

Keep up the great coding!

[jon_m]

Nice solution @CryptoByNight

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.

Just let me know if you have any questions.

[Jules]

[Jules]

do I need to import both ownable and destroyable into my Bank contract?

[CryptoByNight]

Yes good call. That makes sense.