Inheritance Assignment

Alex_13 · November 13, 2021, 11:40am

As always, thanks a lot for your input Jon

First of all, to answer some of your remarks:

I mixed up the numbers of the version => of course I am using v0.7.5
Your comments concerning the multi-level inheritance are well understood and I adjusted my code accordingly
I also understood your comments regarding the variable declaration (for the variable owner)
I realized what my mistake was with the modification of my close() function and I adjusted it accordingly by inserting the onlyOwner modifier.

My adjusted solution:

File bankOwnable.sol

pragma solidity 0.7.5;

import "./bankOwnableDestruction.sol";

contract Bank is Destroyable{
...
}

File ownable.sol

pragma solidity 0.7.5;

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

File bankOwnableDestruction.sol

pragma solidity 0.7.5;

import "./ownable.sol";

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

I have another question concerning the meaning of msg.sender:

The mistake with the onlyOwner modifier in my Destroyable contract is actually the result of a misconception about the meaning of msg.sender
From the tutorial of this academy and also from a written documentation (See “Block and Transaction Properties”), I assumed that msg.sender is equal to the current address that interacts with the contract, or in other words the address of the entity that calls the contract and its functions etc. I think this statement is correct, isn’t it?
My misunderstanding was that I was mislead by the functionality of the Ownable conctract in the way that I assumed each time another address interacts with the contract, the constructor changes the value of the variable owner to the address of the party currently interacting with the contract. In the moment of writing the Ownable contract, I just missed the important fact that the constructor is executed only once when the contract is deployed. Since I do not change the variable owner somewhere else, it remains equal to the address of the party that deploys the contract.

To sum it up, are the following statements correct?
1.) msg.sender equals the address of the party/entity that currently interacts with the conctract (applies to both, the party that initially deployed the contract and all other parties that interact with the contract after its deployment)
2.) Using the Ownable contract, as shown above, is a best practice to store the address of the party that deployed the contract and can then be used for different purposes (in this case to selfdestruct the contract).

A final point:
I totally agree with the safety aspect related to selfdestruct - in case a smart contract has vulnerable code that is being exploited by an attacker, the designated contract owner can withdraw all remaining funds from the contract and mitigate the financial damage.
However, having one address with the power to withdraw all funds from a contract represents a strong concentration of power. I assume it is common practice that initiating the function to trigger selfdestruct requires the signature of multiple parties, but still the power remains in the hand of e.g. a few members of the developer team. My question actually is, is this a big/hot topic in the crypto space (the good old story about security vs. decentralization) and what is the general perception about this issue (feel free to post your own opinion if you want )

_Vincent · November 14, 2021, 9:40pm

Hey, it’s my turn

First, a “mistake” in the assignment I think : “Then, you should be able to deploy the Bank contract, perform some transactions and, finally, destroy the contract, transferring any remaining ether held in the contract to the contract owner.”

In fact, in our example, there is “owner = msg.sender” in Ownable class. That means, any remaining ether in balance[msg.sender] will be always equals to balance of the owner cause this is the same address.

So, my code below don’t work, because the transfer method try to transfer remaining ETH from and to the same balance. I think we have to rewrite lots of logic in order to perform a scenario where the msg.sender and owner will be different in order to check if the transfer of remainin ETH will work. I will see in the assignement solution what it will.

The good side of this “mistake”, if it is, I know use the debugger with Remix.

DESTROYABLE.SOL

pragma solidity 0.7.5;

import "./Ownable.sol";


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

OWNABLE.SOL

pragma solidity 0.7.5;

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

BANK.SOL

pragma solidity 0.7.5;

import "./Destroyable.sol";


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

    
    //EVENTS
    event balanceAdded(uint amount, address indexed depositedTo);
    event depositDone(uint value, address indexed sender);
    event closureDone(uint value, address indexed sender);
    
    //CLOSURE
    function closeWalletBasic() public
    {
        uint senderBalance = getSenderBalance();
        
        if(senderBalance > 0)
        {
            transfer(owner, senderBalance);   
            
            assert(balance[msg.sender] == 0);
        }
        
        close();
        emit closureDone(balance[msg.sender], msg.sender);
    }

_Vincent · November 14, 2021, 10:43pm

Hum… after checked the assignment solution :

I don’t see any code to cover the original following point (in bold) :
Then, you should be able to deploy the Bank contract, perform some transactions and, finally, destroy the contract, transferring any remaining ether held in the contract to the contract owner
What means this line of code ?
govermentInstance.addTransaction(msg.sender, recipient, amount); (I,ve got my answer for that : this line comes from the next lesson course (“External Contracts”) but it appears before in code solution here…)

Thanks for your feedback,
Vincent.

jon_m · November 15, 2021, 3:32am

Hi @Alex_13,

That’s a nicely modified solution

Correct … As you know, interacting with a smart contract requires calling a function. If a function takes parameters, it is called with arguments. The address which calls a function is like an additional default argument i.e. this value is automatically fed into the function for us, and Solidity allows us to reference this calling address with msg.sender. The important thing to understand here, is that, like other parameter names, msg.sender has local, function scope i.e. it only references the calling address within the function it is used in, and only temporarily, until the function has finished executing. Each time a function is called with a different address, msg.sender will always reference whatever that address is, whilst that specific function’s code is executing. However, whenever we assign msg.sender to a state variable (including storage arrays and mappings), the temporary value that msg.sender represents within a function, is then stored persistently until it is next modified, either by the same function or a different one.

Correct … Think of the constructor as a function which is only executed on deployment of the contract. As with any other function, the calling address (in this case the deployer’s address) can be referenced within the constructor with msg.sender. Again, msg.sender only has local scope, and only temporarily represents the deployer’s address on deployment; but, as the constructor is only executed once, msg.sender within a constructor can only ever represent a single address. As you have said, the fact that our owner state variable continues to hold the deployer’s address (and never changes), is because, once assigned by the constructor, there are no other functions which enable owner to be changed to a different address. We could easily enable this to happen, though, by adding the following function to Ownable, for example…

function changeOwner(address newOwner) public onlyOwner {
    owner = newOwner;
}

Notice that only the current owner can call this function, meaning that only the current owner can effectively authorise a change to a new owner.

There are many possible variations here, including setting up an array of owner addresses. Obviously, the code then starts to become more complicated, but this would address the important issue you raised in terms of the potential security risk of only having one owner address.

So, yes … this statement is correct in the context of the additional comments I’ve made above.

Yes … it enables certain transactions to be restricted to a certain authorised address (or addresses). Additional addresses, or a different address to the deployer’s, can also be set on deployment by adding an address parameter (or parameters) to the constructor, which are input as arguments on deployment.

This is a perfectly valid concern and important consideration. I’ve already mentioned above how multiple authorised addresses can be set. selfdestruct() itself can only take a single payable address argument, but you could add some conditional logic to the close() function which requires it to be called separately by each of the authorised addresses before selfdestruct() is triggered.

Yes, you are absolutely right that this is a trade-off issue. As a user/investor, you would have to decide whether the inclusion of functionality which enabled a group of authorised addresses to quickly and effectively “save” the funds in the event of an attack, serious bug or critical failure — or which prevented this from happening — whether, on balance, this decreased your overall risk exposure, and outweighed any compromises you were having to make in terms of your views on what is an acceptable level of decentralisation.

When you deployed and tested your solution, you may have noticed that, after selfdestruct() has been executed, function calls are still successfull and don’t throw errors! We can be sure that the contract has been destroyed, because the getters now return zero values. However, a clear disadvantage with using selfdestruct is that, after the contract has been destroyed, a payable function can still be called, by mistake, with an ether value and, instead of reverting, the ether will be lost. You can test this yourself by calling the deposit() function with an ether value after you have checked that the contract has been destroyed. Therefore, if a smart contract deployed on the mainnet is ever destroyed with selfdestruct, all users need to be notified and informed that they must not send any more funds to that contract address.

The potential repercussions of selfdestruct need to be fully understood before choosing to implement it; but for the purposes of this introductory course, it provides a simple and straightforward way to practise implementing inheritance, which is our main objective in this assignment. More effective alternatives do exist, such as pausable contracts. These involve more advanced techniques and are therefore out of the scope of this introductory course, but you can learn about pausable contracts and other smart-contract emergency and security features, such as proxy contracts, in the Smart Contract Security course.

I hope this goes some way towards answering your very pertinent, and well-thought out questions. Just let me know if you have any more

B_S · November 15, 2021, 6:00am

a perusing of that entire types page hinted at a lot of answers i was missing! i still probably needed your explicit help and that of stack exchange, but reviewing that afterward has been helpful too. thank you

jon_m · November 15, 2021, 9:26am

Hi @_Vincent,

Your Ownable and Destroyable contracts are perfect, and your inheritance structure is also very well coded

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.

The problem is with the Bank contract…

Firstly, unless you post your full Bank contract, including all of the functions for performing deposits, withdrawals and transfers, then I won’t be able to see if there are also some issues with that code which are contributing to the overall problem. You’ve moved on to this assignment before I’ve been able to confirm that you have a fully working Bank contract from the earlier Transfer assignment. You should be starting off this assignment with that same Bank contract, but I assume you’ve understood that, and have just posted the new code you’ve added.

I think you may not have understood that there is a difference between…

the contract owner’s address stored in the owner state variable (this is the address that originally deploys the contract); and
the actual contract address (the address which the contract is deployed at).

These are two completely different things, and so what you say here …

… isn’t a mistake because, as well as destroying the contract, calling selfdestruct automatically transfers the total remaining contract balance (not the contract owner’s individual share of the total contract balance) to the external address it is called with (the payable address argument).

You can check the Bank contract’s ether balance if you add the following function to your Bank contract…

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

The contract balance will be the total amount of ether held in the contract for ALL users. So, if all of your functions are coded correctly, it should always be equal to the sum of all of the user’s individual account balances stored in the mapping. Including the above getter just means you don’t have to remember how much ether has been deposited and withdrawn by different users before the contract is destroyed.

When an external address calls the deposit() function with an ether value, you will see the ether balance for that external address decrease by that amount (displayed in the Account field in the Deploy & Run Transactions panel). You can think of this as the user’s wallet balance decreasing by the amount they deposit in the smart contract. All ether deposited in the contract by any external address gets added to the same contract balance. At the same time, the mapping keeps track of each individual user’s balance, which is effectively each user’s share of the total contract balance.

Whenever a withdrawal is made, the exact opposite happens: the contract balance reduces by the withdrawal amount, the user’s external address balance (the address which calls the withdraw function) increases by that same amount, and the user’s individual balance in the mapping is also adjusted.

Calling the separate transfer() function (not the special transfer method in the withdraw function) does not change the contract balance or any external address balance. The transfer() function performs an internal transfer between two individual users within the Bank contract. As no funds are leaving or entering the contract, the only change is to the balances in the mapping for the two individual users involved in the internal transfer.

You don’t need to add your closeWalletBasic() function to the Bank contract, or any of the code you have included within its function body. The close() function in your Destroyable contract is inherited by Bank, and so will be available to call by the contract owner address when Bank is deployed. The close() function contains all of the functionality needed for the contract owner (and only the contract owner) to destroy the contract and transfer the remaining contract balance (which may include some of their own funds) to their own external address.

There are more comments I could make about the code in your closeWalletBasic() function, but I think once you understand the other points I’ve made above, it will become clear why none of this code is actually needed.

Hopefully, this helps you to understand how to complete this assignment and to successfully do the following

jon_m · November 15, 2021, 9:07am

I’ve just made a lot of changes to my feedback for this assignment, which I posted earlier. So, if you’ve already read it before you see this message, then please read the updated version, which is hopefully simplified

jon_m · November 15, 2021, 9:38am

Yes, you are right… that line of code shouldn’t be in the solution for this assignment.

_Vincent · November 15, 2021, 12:15pm

Hi @jon_m,
thanks you so much for your time and your detailed explanation !

You’re right : for I, I don’t understood what you explain 'til now ! :).
This will be great to have an additional video course in ETH 101, detailing that. I think it’s mandatory for understanding contracts with a global view.

So, if I understand well : in order to test one part of this assignment, we have to deploy with an address but call the destruct with another.

Just a little hard to know which standard function do exactly. For instance, with standard Transfer()" function, we have to update the balance of the contract manually, but with standard Destruct() function, the remaining called address balance is automatically transferred to the contract balance. We have to practice for that.

@jon_m, a new update : one important part of your explanation (about address(this).balance) is explained AFTER the assignment, in “Value Calls” video course…bad timing

Many thanks.

jon_m · November 15, 2021, 9:32pm

Hi @_Vincent,

No … whichever address you choose to deploy the contract (the address showing in the Account field, or the one you select from the dropdown) will be assigned by the constructor to the owner state variable…

Even though you are only deploying Bank, this assignment happens because the Ownable contract is inherited. You can check this immediately after you’ve deployed Bank by calling the getter owner which is automatically created because you’ve given this state variable public visibility. This owner address never changes, and because we have restricted the close() function with the inherited modifier onlyOwner, only the address stored in the owner state variable (the contract deployer) can call this function and destroy the contract. When the close() function is called, selfdestruct is automatically triggered, and the remaining balance in the contract (address(this).balance) is automatically transferred out of the contract (effectively, “rescued”) and added to the external address balance passed to selfdestruct as a payable address …

// syntax
selfdestruct(address payable recipient)

// your code
selfdestruct(owner)  // sends remaining contract balance to owner address

You do still seem confused here …
The transfer() function (which takes two arguments, recipient and amount) does not affect the contract balance. This function simply performs an internal accounting adjustment between two individual users. Their external address balances, and the total contract balance do not change. All that changes is the share of the total contract balance each user is entitled to (recorded as the individual user balances stored in the mapping).

The contract balance only changes when ether either enters or leaves the contract, for example, when the deposit function, withdraw function or selfdestruct is called. The contract balance is always modified automatically as a result of other operations which we code e.g.

(1) The transfer method we have added to the withdraw() function…

// syntax
<address payable>.transfer(uint amount);

// our code
msg.sender.transfer(amount);

… deducts the value of the amount parameter (input into the withdraw function in wei) from the contract balance, and increases the external address balance of msg.sender (whichever address has called the withdraw function). As I’ve mentioned before, we also need to add an additional line of code to adjust the individual user’s balance for this transaction in the mapping.

(2) Because the deposit() function has been marked payable, whatever ether value this function is called with is deducted from the caller’s external address balance, and automatically added to the contract balance. As with the withdraw function, we also need to add an additional line of code to adjust the individual user’s balance for this transaction in the mapping.

(3) You seem to have got this the wrong way round… As I’ve mentioned above, when the predefined selfdestruct method is triggered, the remaining contract balance — which you can check with address(this).balance — is automatically transferred to the external address placed between the parentheses. So, just before the contract is destroyed and removed from the blockchain, the external address balance (in our case, the contract owner’s) automatically increases by the remaining contract balance, and the contract balance is automatically reduced to zero.

I really encourage you to spend some time practising and experimenting with different combinations of transactions, and then finally destroying the contract, and observing how the different balances change with each transaction…

The external address balances, in the Account field dropdown
The contract address balance: address(this).balance
The individual user balances, stored in the mapping and retrieved by calling getBalance() with the address you want to see the balance for.

And when you have a finished Bank contract, it would be good to see the full code posted here

jon_m · November 15, 2021, 9:46pm

Thanks as well @_Vincent for your comments about how we can improve the course by adding some additional explanations and clarifications at certain points. This is very helpful feedback

As you can see, we are already aware of areas which can cause confusion for students, and are ready to provide the necessary explanations and support when needed, but what we need to do now is add some of this additional information to the actual course material, so that these things are clearer to students at the most appropriate time, which is essentially what you are saying.

Thanks for enthusiastic engagement — it’s very much appreciated

_Vincent · November 15, 2021, 9:50pm

Many thanks for your time again @jon_m, gonna playing hard on these parts before going further.

jon_m · November 15, 2021, 9:52pm

Now is a perfect time to do that — some experimentation and consolidation Then you’ll be ready to blast off into 201

11lll · November 16, 2021, 8:32pm

Are there any downsides of using the following approach compared to the solution on GitHub?

pragma solidity 0.8.7;

import "./Ownable.sol";

contract selfDestruct is Ownable {
 address payable private owner;

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

jon_m · November 17, 2021, 5:02pm

Hi @11lll,

This code is correct, and a perfectly valid alternative solution, but only if you make the owner state variable in Ownable payable.

This line doesn’t make sense, because the owner state variable is already inherited from Ownable, and you can’t use the same identifier for two state variables.

Once you’ve corrected this, it would be better to see all 3 of your contracts to check that everything is coded correctly. If you are using Solidity v0.8 instead of v0.7, then you should have also made a couple of additional changes (in Ownable and Bank) to those I’ve mentioned above.

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

Alex_13 · November 17, 2021, 9:26pm

Thank you very much for the comprehensive explanations

As always, very detailed but easily comprehensible answers!

moise · November 19, 2021, 4:16am

Hi, here is my solution to Inheritance assignment

contract Ownable{
    address owner;
    constructor(){
        owner = msg.sender;
    }
    
    modifier onlyOwner(){
        require(msg.sender == owner,"You must be the owner of the contract to remove it");
        _;
    }
    
    // disable the self transfert
    modifier denySelfTransfert(address _sender, address _recipient){
        require(_sender != _recipient,"cannot send funds to yourself");
        _;
    }
}


contract Destroyable{
    function close(address payable _owner) internal {
        selfdestruct(_owner);
    }
}


pragma solidity ^0.8.7;
import "./Ownable.sol";
import "./Destroyable.sol";

contract Bank is Ownable,Destroyable{
    mapping(address => uint) balance;
    event depositDone(address sender,uint _amount);
    event withdrawDone(address indexed withdrawer, uint indexed _amount);
    event TranserDone(address sender, address recipient,uint amount);

    function deposit()public payable {
        balance[msg.sender] += msg.value;
        //logging desposits 
        emit depositDone(msg.sender,msg.value);
    }
    
    function withdraw(uint _amount) public  returns(uint){
        //Check the address that is calling this function have enough money in his balance
        require(balance[msg.sender] >= _amount,"No enough funds");
        balance[msg.sender] -= _amount;
        payable(msg.sender).transfer(_amount);
        //logging withdraw 
        emit withdrawDone(msg.sender,_amount);
        return balance[msg.sender];
    }

    function transfert (address _recipient,uint _amount) public{
        _transfert(msg.sender,_recipient,_amount);
    }
    
    function _transfert(address _sender,address _recipient,uint _amount) private denySelfTransfert(_sender,_recipient){
        require(balance[_sender] >= _amount);
        balance[_sender] -= _amount;
        balance[_recipient] += _amount;
        emit TranserDone(_sender,_recipient,_amount);
    }
    function getBalance()public view returns(uint){
        return balance[msg.sender];
    }

    function remove () public onlyOwner {
        close(payable(owner));
    }
}

jon_m · February 9, 2022, 2:33am

Hi @moise,

Your solution works, you’ve correctly coded a multiple inheritance structure, and you have met all of the main assignment objectives

Some observations and comments …

(1) If you change the visibility of the close() function in Destroyable to public, it will still be inherited, and it will also be available for the contract owner to call directly, instead of having to call it via the additional remove() function you’ve added to Bank. You can then remove the remove() function, as long as you also do the following:

Have Destroyable inherit Ownable (instead of Bank). This will give you a multi-level inheritance structure, and will make the onlyOwner modifier and the owner state variable available in Destroyable.
Add the onlyOwner modifier to close() instead of remove().
Remove the payable address parameter from close() and change the argument passed to selfdestruct so that it still references the contract owner’s address and is also payable. This can be done in 3 different ways. Which would you choose?

(2) Because the denySelfTransfer modifier doesn’t (i) provide functionality associated with contract ownership, and (ii) depend on any of the other code in Ownable, it would be better defined in Bank instead of Ownable. This will keep the separation of our code into 3 different contracts based on a clear and logical division of functionality: contract ownership related, contract destruction related, and Bank contract specific.

(3) There isn’t any point in having two separate functions, transfer() and _transfer(), if all of the checks and computations are performed in _transfer(), and all transfer() does is call _transfer(). You may as well remove transfer(), and just call _transfer() directly. This will keep your code clearer and more concise. Code for a single transaction is split between more than one function where there are clear distinctions between different sub-groups of operations, and placing these sub-groups in separate functions will result in better organisation, clearer presentation, and improved modularity.

(4) It is not good practice to omit the pragma statement from any Solidity file, because we should always define the Solidity compiler version (or range of versions) which our code is written to be compatible with.
Because the Bank contract inherits functionality from Destroyable and Ownable, even if the pragma statements are omitted from either or even both of their files, Remix will still attempt to compile Bank as a single contract together with the code it inherits. Bank will compile and deploy successfully as long as the code in both of the inherited contracts is compatible with the Solidity version declared in the pragma statement at the top of the file containing Bank. However, the compiler still issues orange/yellow warnings, highlighting that it’s best practice to include pragma statements in each file.
In the absence of a pragma statement, it seems that Remix will always try to find a suitable compiler version, although this doesn’t seem to be 100% reliable.
So, in conslusion, always include a pragma statement at the top of every .sol file.

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

kasunkv · November 21, 2021, 4:57pm

Here is my solution.

Ownable.sol

pragma solidity 0.8.9;

contract Ownable {
    address owner;
    
    event OwnershipSet(address indexed owner);
    
    modifier onlyOwner {
        require(msg.sender == owner, "You are not the owner");
        _;
    }
    
    constructor() {
        owner = msg.sender;
        emit OwnershipSet(owner);
    }
}

Destroyable.sol

pragma solidity 0.8.9;

import "./Ownable.sol";

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

Bank.sol

pragma solidity 0.8.9;

import "./Destroyable.sol";

contract Bank is Destroyable {
    mapping(address => uint) balance;
    
    event DepositDone(uint amount, address indexed depositedTo);
    event TransferDone(uint amount, uint balance, address indexed transferedTo);
    event WithdrawDone(uint amount, uint balance, address indexed withdrawnTo);
    
    function deposit() public payable returns (uint) {
        balance[msg.sender] += msg.value;
        
        emit DepositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function transfer(address _recipient, uint _amount) public {
        require(balance[msg.sender] >= _amount, "Insufficient balance");
        require(msg.sender != _recipient, "Don't send money to yourself.");
        
        balance[msg.sender] -= _amount;
        balance[_recipient] += _amount;
        
        payable(_recipient).transfer(_amount);
        
        emit TransferDone(_amount, balance[msg.sender], _recipient);
    }
    
    function withdraw(uint _amount) public {
        require(balance[msg.sender] >= _amount, "Insufficient balance");
        
        balance[msg.sender] -= _amount;
        
        payable(msg.sender).transfer(_amount);
        
        emit WithdrawDone(_amount, balance[msg.sender], msg.sender);
    }
    
    function getBalance() public view returns (uint) {
        return balance[msg.sender];
    }
    
    function getContractBalance() public view onlyOwner returns (uint) {
        return address(this).balance;
    }
}

giomiri · November 22, 2021, 6:49pm

Destroyable.sol

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


contract Destroyable is Ownable {
    
    function close() public onlyOwner {
    owner.transfer(address(this).balance);
    assert(address(this).balance == 0);
    selfdestruct(owner);
        
    }
}```

Ownable.sol

pragma solidity 0.7.5;

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

contract Bank is Ownable, Destroyable {