Inheritance Assignment

Hi @JP-C,

The code you’ve posted should work and meet the main assignment objectives, but can you also post the top few lines of code in your bank.sol file (up to the contract header) so we can see how it inherits the functionality it needs? Bank is the contract we want to deploy, and ultimately destroy, so even if you haven’t made any changes to the body of the contract, you still need to demonstrate how you’ve coded the inheritance relationship.

A couple of observations …

(1)

In order to transfer the remaining Bank contract balance to the contract owner’s external address, you don’t need to include this line of code in your close() function in Destroyable. It will achieve the desired result, and from a technical point of view it is well coded * (except, see the note below). However, as well as destroying the Bank contract, selfdestruct will also automatically transfer the remaining contract balance to the payable address argument it is called with (in our case, the contract owner’s address). This is all part of the pre-defined selfdestruct functionality. So, if you remove this additional line of code from your close() function, you will see that calling this function still produces exactly the same results.

* To call the totalBalance() function as the transfer method’s argument, you don’t need the this keyword. You can just use a standard function call:   msg.sender.transfer(totalBalance());

(2)  Keeping the totalBalance() function, to retrieve the total Bank contract address balance at any time, is still a good idea. It will successfully return the correct contract balance, but the contract address doesn’t need to be converted to a payable address, because it’s not receiving any ether. The function is only referencing the address’s balance property, to read and return it. So you can remove payable()  as follows …

function totalBalance() public view returns(uint) {
    return address(this).balance;
}

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

Excellent!  :muscle:

No, I’m still using 0.7.5. did corrections as per your recommendation.
But now I have run into other issue, where I cannot transfer between accounts. Could you kindly advise what could be wrong?


Will post my full code:

// SPDX-License-Identifier: MIT

pragma solidity 0.7.5;

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

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

contract Bank is Ownable, Destroyable {

    GovernmentInterface governmentInstance = GovernmentInterface(0x93f8dddd876c7dBE3323723500e83E202A7C96CC);

    mapping(address => uint) balance;

    event depositDone(uint indexed amount, address indexed depositedTo);
    event balanceTransfer(address sender, address recipient, uint indexed amount);

    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, "Balance not sufficient");
    msg.sender.transfer(amount);
    balance[msg.sender] -= 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, "Dont 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);

    emit balanceTransfer(msg.sender, recipient, amount);
    }   
    function _transfer(address from, address to, uint amount) private {
    balance[from] -= amount;
    balance[to] += amount;
    }
}
// SPDX-License-Identifier: MIT
pragma solidity 0.7.5;
import "./ownable.sol";

contract Destroyable is Ownable {
 
 function close() public onlyOwner { 
  selfdestruct(owner); 
 }
}
// SPDX-License-Identifier: MIT
pragma solidity 0.7.5;

contract Ownable {
    address payable public owner;

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

    constructor(){
    owner = msg.sender;
    }
}
// SPDX-License-Identifier: MIT
pragma solidity 0.7.5;

contract Government {

    struct Transaction {
        address from;
        address to;
        uint amount;
        uint txId;
    }

    Transaction [] transactionLog;

    function addTransaction(address _from, address _to, uint _amount) external payable {
        transactionLog.push(Transaction(_from, _to, _amount, transactionLog.length));
        }
    function getTransaction(uint _index) public view returns(address, address, uint) {
        return(transactionLog[_index].from, transactionLog[_index].to, transactionLog[_index].amount);
    }
    function getBalanceGov() public view returns (uint){
    return address(this).balance;
    }
}

1 Like

Thanks for the feedback, Jon. I made the changes you suggested to destroyable.sol:

pragma solidity 0.7.5;

import "./ownable.sol";

contract Destroyable is Ownable {

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

Plus, here you are the code for Bank:

pragma solidity 0.7.5;

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

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

contract Bank is Ownable, Destroyable {

    govermentInterface govermentInstance = govermentInterface(0xc4753C8802178e524cdB766D7E47cFc566e34443);

    //state variable
    mapping(address => uint) private  myBalance;
        
    event depositDone(uint amount, address indexed sender);
    event balanceTransfered(uint amount, address indexed sender, address indexed recipient);

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

    function withdraw(uint _amount) public onlyOwner returns(uint){
        require(myBalance[msg.sender] >= _amount, "Not enough balance!");
        
        msg.sender.transfer(_amount);
        myBalance[msg.sender] -= _amount;

        return (myBalance[msg.sender]);
    }

    function getBalance() public view returns (uint){
        return myBalance[msg.sender];
    }

    function transfer(address _recipient, uint _amount) public {
        require(myBalance[msg.sender] > _amount, "Balance insuficient!");
        require(msg.sender != _recipient, "Don't transfer balance to yourself!");

        uint prevSenderBalance = myBalance[msg.sender];

        _transfer(msg.sender, _recipient, _amount);

        govermentInstance.addTransaction(msg.sender, _recipient, _amount);

        assert(myBalance[msg.sender] == prevSenderBalance - _amount);
    }

    function _transfer(address _sender, address _recipient, uint _amount) private {
        myBalance[_sender] -= _amount;
        myBalance[_recipient] += _amount;
        emit balanceTransfered(_amount, _sender, _recipient);
    }

    function totalBalance() public view returns(uint) {
        return (address(this)).balance;
    }  
}```
1 Like

Hi @JP-C,

Your inheritance structure is well coded :ok_hand:
We can also make it more streamlined 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 your close() function, a concise alternative to using the additional receiver variable is to just call selfdestruct() with msg.sender directly (which is what you originally had) …

selfdestruct(msg.sender);

You are using Solidity v0.7.5, and prior to v0.8 msg.sender is already a payable address by default, and so doesn’t need to be explicitly converted whenever the syntax requires it to reference a payable address e.g. as the payable address argument in the selfdestruct() function call, or as the payable address the transfer modifier is called on. However, from Solidity v.0.8 msg.sender is non-payable by default, which would mean having to explicity convert msg.sender to a payable address when necessary e.g.

selfdestruct(payable(msg.sender));

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


FEEDBACK ON YOUR WITHDRAW FUNCTION (from the Transfer Assignment)

Your withdraw() function contains all of the necessary lines of code :ok_hand:

Now have a look at this post which explains an important security modification that should be made to the order of two of your statements.

You’ve modified the withdraw function with the onlyOwner modifier, but this means that only the contract owner will be able to withdraw funds while the Bank contract is deployed and operating normally. The contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so, while the contract is still deployed, it only seems fair that all users should be able to withdraw their funds as well, don’t you think? :sweat_smile: I think during the course we were adding and removing onlyOwner from various functions to demonstrate different scenarios, and I think it might have ended up being left in the withdraw() function in the solution code for this assignment by mistake!


One final observation …

You’re adding extra parentheses where you don’t need them:

withdraw() function

The values returned in return statements only need to be placed in parentheses if more than one is being returned, when they are also separated by commas.

totalBalance() function

The extra set of parentheses can be removed now, because the address isn’t being converted to payable …

return address(this).balance;

Just let me know if you have any questions :slight_smile:

Hey Arturs,

There’s nothing in your code that would be causing the revert error. This post will probably be the answer. The error you’re getting is the one described in point 2. You’ll see that the error message is the same one you’re getting.

A couple of other comments …

This is your corrected withdraw() function from the Transfer assignment

Make sure the statements in the withdraw() function in your latest version of Bank are in the same order: you’ve changed them back again, which is more of a security risk.

You’ve also now added the onlyOwner modifier to the withdraw() function header. But this means that only the contract owner will be able to withdraw funds while the Bank contract is deployed and operating normally. The contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so, while the contract is still deployed, it only seems fair that all users should be able to withdraw their funds as well, don’t you think? :sweat_smile: I think during the course we were adding and removing onlyOwner from various functions to demonstrate different scenarios, and I think it might have ended up being left in the withdraw() function in the solution code for this assignment by mistake!

The additional function you’ve added to Government to retrieve the contract address balance is needed in order to check that the ether has been successfully transferred between the contracts. If you add a similar function to the Bank contract, you’ll find that it’s also really helpful for testing, because you can also check the Bank contract’s total balance at any point in time.

Let me know if you have any questions, or if you still experience any difficulties with the external function call to Government.

withdraw() function, Bank contract

  • You’ve added the onlyOwner modifier to the withdraw() function header. But this means that only the contract owner will be able to withdraw funds while the Bank contract is deployed and operating normally. The contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so, while the contract is still deployed, it only seems fair that all users should be able to withdraw their funds as well, don’t you think? :sweat_smile: I think during the course we were adding and removing onlyOwner from various functions to demonstrate different scenarios, and I think it might have ended up being left in the withdraw() function in the solution code for this assignment by mistake!

  • The balanceWithdrawn event you’ve added, and which is emitted when the withdraw() function is successfully executed, is a good idea. However, as well as the address of the withdrawer, it also needs to log the withdrawal amount to be of any real value. You can add this to the event declaration as an additional parameter.

Just let me know if you have any questions :slight_smile:

Thank you Jonathan!

Yes, you post solved my mystery :slight_smile:

Also thank you for the tips regarding onlyOwner and adding contract balance function to Bank! - Done.

1 Like

Ownable

pragma solidity 0.7.5;

import './Destroyable';

contract Ownable is Destroyable {
    address owner;

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

    constructor {
        owner = msg.sender;
    }
}

Destroyable

pragma solidity 0.7.5;

import 'Ownable';

contract Destroyable is Ownable {

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

Hi again @Bitborn,

I’ve noticed that you never got a reply to this other post of yours last August! I went on leave just after I replied to your original post so I didn’t pick up your reply back, and it seems nobody else did either. Apologies for that! Anyway, as you seem to be working on this particular part of the 101 course again, and we are discussing some other points you’ve raised as well, I thought you might still find a response helpful to tie up any loose ends :sweat_smile:

Yes, exactly. Have a look at this post for further details about why the additional local variable receiver was used in the model solution, even though it’s not necessary.

And, by the way, msg.sender is the value assigned to the local payable address variable receiver, and not the other way around.

Yes …

  • Whichever address you deployed the contract with, this will be assigned by the constructor to the owner address state variable.
  • Because only the owner address can call your destruction() function, msg.sender in this function can only be the owner address.
  • So, the msg.sender passed as the argument to selfdestruct can only be the owner address.
  • As well as destroying the contract, selfdestruct will also automatically transfer the remaining contract balance to the payable address argument it is called with (in our case, the owner address). This is all part of the pre-defined selfdestruct functionality.
  • The remaining contract balance is transferred to the contract owner’s external address balance. You will see this reflected as an increase in the contract owner’s address balance in the Account field near the top of the Deploy & Run Transactions panel in Remix. If the address that deployed your contract (the contract owner’s address) isn’t showing in the Account field, you need to select it from the dropdown.

The contract balance, and each of the users’ individual balances recorded in the mapping (each retrieved by the getBalance function), will be zero, because the contract has been destroyed. If the owner address also had a share of the total amount of ether deposited in the contract (reflected as a balance in the mapping) then, as with all users, if the owner address calls getBalance(), this too will return a balance of zero. The contract owner’s share of the total contract balance will be zero, as it will be for all users. Instead of allowing all of the funds held in the contract to disappear, the total is transferred OUT OF the contract to the contract owner’s EXTERNAL address balance.

You need to be very clear about which balances you are looking at:

  • External address balances (think of them as wallet addresses) in the Account field.
  • Individual user balances, which record each user’s share of the contract’s total ether balance. These are like internal accounting balances, which track each user’s changing entitlement to withdraw ether from the contract. A specific user’s balance is retrieved by that user address calling the getBalance() function.
  • The total contract balance: the total amount of ether held by the address the contract is deployed at. This balance can be retrieved by adding the following function to your contract. You will probably find it very useful for testing purposes (any address can call it) …
function getContractBalance() public view returns(uint) {
    return address(this).balance;
}

Let me know if anything is still unclear about this assignment, or if you have any further questions :muscle:

Hi @diehlca17,

The code within the body of each contract (Ownable and Destroyable) is correct, but your inheritance structure is incorrectly coded. This means that your Bank contract won’t compile.

If you check the compiler errors, you will see that your files can’t be found. This is because their .sol file-type is missing in the import statements.

Once you’ve added those, you will then get a compiler error for your Ownable contract header. This is because you can’t have two contracts inheriting each other. There must be a parent/child relationship. Think carefully about which of these two contracts needs to inherit functionality from the other. This will establish an appropriate parent/child relationship, which you then need to reflect in the code.

When you’ve corrected this, as well as Ownable.sol and Destroyable.sol, you also need to post the top few lines of code in your Bank.sol file (up to and including the contract header). This is so we can see how it inherits the functionality it needs. Bank is the contract we want to deploy, and ultimately destroy, so even if you haven’t made any changes to the body of the contract, you still need to demonstrate how you’ve coded the inheritance relationship.

Just let me know if anything is unclear, or if you have any questions about how to correct your code :slight_smile:

@jon_m hey mate would you mind reviewing my code? I was also wondering what the difference would be in my event if both my value and addresses were indexed. Thank you

/ SPDX-License-Identifier: MIT

pragma solidity 0.7.5;
import “./Destroyable.sol”;
contract etherBank is Destroyable{

//Deposit, Withdraw, Transfer, and getBalance
//keeping in mind the security

mapping(address => uint)balance;
event depositedTo(uint amount, address indexed account);



function getBalance() public view returns(uint){

    return balance[msg.sender];

}

function deposit() public payable returns(uint){

    balance[msg.sender] += msg.value;

    emit depositedTo(msg.value, msg.sender);

    return balance[msg.sender];

}

function withdraw(uint value)public returns(uint){

    require(balance[msg.sender] >= value, "Not sufficient funds for withdrawal");

    uint previousBalance = balance[msg.sender];

    balance[msg.sender]-= value;

    msg.sender.transfer(value);

    assert(previousBalance-value == balance[msg.sender]);

    return balance[msg.sender];



}

function transfer(address to, uint value)public {

    require(msg.sender != to, "Can't send money to yourself m8");

    require(balance[msg.sender] >= value, "Not sufficient funds m8");

    _transfer(to, msg.sender, value);

}

function _transfer(address to, address from, uint value)private{

    balance[to]+= value;

    balance[from]-= value;



}

}

1 Like

// SPDX-License-Identifier: MIT

pragma solidity 0.7.5;

import “./Ownable.sol”;

contract Destroyable is Ownable{

function close()public onlyOwner{

   

    selfdestruct(msg.sender);    

}                                  

}

1 Like

// SPDX-License-Identifier: MIT

pragma solidity 0.7.5;

contract Ownable {

address public owner;

modifier onlyOwner{

    require(msg.sender == owner, "you're not the owner");

    _;

}

  constructor(){

    owner = msg.sender;

}

}

1 Like

An excellent set of contracts @Achilles_Armendariz :muscle:

… and welcome to the forum! I hope you’ve been enjoying this course.

Inheritance Assignment

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

Transfer Assignment

As this is the first code you’ve posted for this course, I’ll also give you some feedback on your withdraw() function in the etherBank contract, which was the task for the earlier Transfer Assignment…

You have added all of the lines of code needed to solve the problem with the withdraw() function, and the additional two lines you’ve added for the assert() statement are also well coded. In addition, your statements are in the correct order within the function body to reduce security risk:

  • Check inputs (require statements)
  • Effects (update the contract state)
  • External Interactions (e.g. transfer method)

Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…

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

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent it, in the courses which follow this one. But it’s great you’re already getting into good habits in terms of smart contract security :+1:


In terms of your question about indexing event parameters

Marking event parameters as indexed allows their names to be used as search parameters to filter logged event data and search for specific events in the frontend/UI using a library such as web3.js. However, each event parameter that is indexed (up to a maximum of 3 per event declaration) increases the gas cost, so they should only be indexed when this is actually necessary.

Therefore, deciding whether or not to index event parameters will depend on whether we will need to retrieve historical event data and, if so, whether we require the returned dataset to have been filtered according to specific event parameters other than the event name.

For example …

  • If we need to retrieve the historical event data just for the deposits made by a specific address, then we need to index the address parameter corresponding to the depositor’s address.

  • If we don’t need to retrieve the historical event data just for deposits of a specific amount, then indexing the amount parameter won’t be necessary.

  • If we need to retrieve the historical event data just for the deposits of a specific amount made by a specific address, then we need to index both of these parameters.

Here’s a link to a helpful discussion about indexing, which I think explains quite well how this works in practice:

https://ethereum.stackexchange.com/questions/8658/what-does-the-indexed-keyword-do/8659

You are actually missing a transfer event in your etherBank contract. This was the task for the Events Assignment — to add code to the contract, so that each successful transfer logs an event with appropriate data. I think you may have missed it: it’s the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course. I would encourage you to do this assignment because it will be good practice coding events, and it will also give you an opportunity to think about which event parameters to index, based on the information I’ve given you in this post :slight_smile:

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

Hello! This is the Ownable contract:

pragma solidity 0.7.5;

contract Ownable {

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

This is the Destroyable contract:

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

contract Destroyable  is Ownable {

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

And this is the beginning of the Bank contract:

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

contract Bank is Destroyable {

It’s working for me…

1 Like

awesome, thanks for the feedback! Also that explanation for events really helped me out

1 Like

Nice solution @Oprea :ok_hand:

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.

This line of code works, but because we’re using Solidity v0.7 in this course, msg.sender is already a payable address by default, and so doesn’t need to be explicitly converted whenever the syntax requires it to reference a payable address (such as here, as the payable address argument passed to selfdestruct ).

// Solidity v0.7
selfdestruct(msg.sender);   // compiles

However, from Solidity v0.8, msg.sender is non-payable by default, which means having to do what you’ve done and explicity convert it to a payable address when necessary …

// Solidity v0.8
selfdestruct(payable(msg.sender));   // compiles
selfdestruct(msg.sender);            // throws a compiler error

Let me know if you have any questions.

1 Like

Ownable.sol

pragma solidity 0.7.5;

contract Ownable {
    address public owner;

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

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

Destroyable.sol

pragma solidity 0.7.5;

import "./Ownable.sol";

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

Bank.sol

pragma solidity 0.7.5;

import "./Destroyable.sol";

contract Bank is Ownable, Destroyable{
    

I left out a chunk of the main bank file because I figured it just needed the inheritance part but if you need more I can post it! I also had a queston about naming convention. Is it wise to name the function as I did? Is there a problem with having the function named so closely to the built-in selfdestruct()?

1 Like

Great solution @0xsundown :ok_hand:

No that’s fine :+1:

But you can also remove Ownable from the Bank contract header. Destroyable already inherits Ownable, and so even if Bank only inherits Destroyable explicitly, it will still inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure…

contract A { ... }
contract B is A { ... }
contract C is B { ... }

I don’t know if it was unintentional, but your code already demonstrates that Bank.sol doesn’t need to import Ownable.sol. If Bank needed to explicity inherit Ownable — and include it in the Bank contract header as you have — then it would also need to import Ownable.sol.

Great question!

As you’ve probably already discovered, the compiler throws an error if you try to name the “wrapping” function selfdestruct because this is already the name of the in-built selfdestruct method. You are right, and wise, to consider whether using an almost identical name is a good idea, because in some situations this could definitely cause confusion. In terms of the smart contract code executing successfuly and securely, the degree of difference between the two names is irrelevant. But we do need to consider whether such a close similarity could lead to confusion when reading or developing the contract.

I actually don’t think it’s a problem here, because when I read your code, it’s obvious which is the main “wrapping” function (the function called by the contract owner from the external service), and which is the in-built selfdestruct method provided by Solidity. As with the in-built transfer method, selfdestruct can only be implemented within a function body, and so this further makes the distinction clear.

As the whole purpose of the “wrapping” function is essentially to call selfdestruct and destroy the contract, then I actually think the fact that you’ve chosen very similar names highlights the connection between them in a positive and helpful way :+1:

Just let me know if you have any further questions :slight_smile:

2 Likes