From what you’ve said in your post it sounds like lots of things have now clicked into place
I understand what you mean here. I like to think of the mapping balances as accounting entries, and the additions and deductions we make to these balances in the various functions as accounting adjustments. These balances are “tied” to funds, because they reflect each individual user’s share of the total contract balance, effectively their entitlement to transfer ether out of the contract to an external address. If it wasn’t for these individual balances, we would just have a pool of funds in the contract without any way to record or control the allocation of ownership of these funds to individual external addresses, and how this changes over time.
Your attempt to implement 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.
Unfortunately, your Destroyable and Bank contracts don’t compile for various reasons, and so your Bank contract can’t be deployed…
(1) For the current version of your code, you should be getting compiler errors for both of your import statements…
This is because each file path should have a forward slash instead of a back slash.
(2) Once you’ve corrected the import statements, you should now be getting a compiler error for this line …
The error here is an upper/lower case inconsistency with one of the letters in your modifier name. Let the compiler, and the error messages it generates, help you find these types of spelling-related errors.
(3) The next compiler errors that are generated are for the following two lines of code:
In the terminate() funtion in Destroyable…
In the withdraw() function in Bank…
Prior to Solidity 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 (such as in the above two lines of code. However, you are using Solidity v0.8.10, and from v.0.8 msg.sender is non-payable by default, and so this means that in the above two lines of code you need to explicity convert msg.sender to a payable address using the syntax payable() …
// In the terminate() function in Destroyable
// selfdestruct() must be called with a payable address argument
address payable receiver = payable(msg.sender);
selfdestruct(receiver);
// In the withdraw() function in Bank
// the transfer method must be called on a payable address
payable(msg.sender).transfer(amount);
In the terminate() function, a concise alternative to using the additional receiver variable is to call selfdestruct() with msg.sender directly, and convert it to a payable address within the function call itself…
selfdestruct(payable(msg.sender));
Have a look at this post for further details about the use of the additional local variable in the model solution.
(4) The next compiler error is generated for the function call within the DestroyThis() function in Bank…
Again, the error message will make it clear what the problem is. You are trying to call a function in the Destroyable contract that doesn’t exist!
The inherited terminate() function has public visibility and so it is available to call within Bank. This means that you can just call it as follows …
function DestroyThis() public onlyOwner {
terminate();
}
However, one of the key reasons for using inheritance is to avoid code duplication. The terminate() function has public visibility and so, when Bank is deployed, terminate() is also available to be called directly by the contract owner’s external address whenever they want to destroy the contract, instead of having to call it via the additional DestroyThis() function you’ve added to Bank. Therefore, you can remove the whole of the DestroyThis() function from Bank.
By the way I think you missed out the Events Assignment, because you are missing a Transfer event in your Bank contract, and a corresponding emit statement in its transfer() function.
Let me know if anything is unclear, if you have any questions, or if you need any help correcting your contracts, deploying Bank, and getting it to do what it should
Hey guys hoping someone can help me with a couple questions i have regarding the Inheritance Assignment with the 3 contracts… Bank.sol, Destroyable.sol. and Ownable. sol. With bank.sol I noticed I could not withdraw or transfer money in Remix without adding “payable” to those functions. Is it correct that i would have to add “payable” in the header of the withdraw function and transfer functions in order to withdraw and transfer money?
One other question as welll… I noticed the “withdraw” function in bank.sol does not adjust the balance after the withdrawal. Shouldn’t there be a line to do this so we can accurately reflect the balance, like balance[msg.sender] -= amount. Something like this?
No… neither of these functions receive ether, so they don’t need to be marked payable. A function should only be marked payable when it needs to receive ether from an external address to add to the contract address balance e.g. the deposit() function. It might help to think of payable in a function header as the code that enables msg.value to be added to the contract address balance, without us having to add any further code for this. In the deposit() function, this happens automatically because we have marked the function as payable . The line …
balance[msg.sender] += msg.value;
… then adds the same value to the individual user’s balance in the mapping, in order to keep track of their share of the total funds held in the contract.
However, we should not mark the withdraw() function as payable , because it doesn’t need to receive any ether from an external address and add it to the contract balance. If we make it payable, then, if the caller sends an ether value to the withdraw() function by mistake (as well as calling it with a withdrawal amount) then this ether would be added to the contract balance and potentially lost to the user: this is because the withdraw() function does not make an equivalent accounting adjustment to the mapping for such a deposit (only for withdrawals). However, the function is much securer if we leave it as non-payable, because then, if the caller makes the same mistake, the function will revert, and the ether won’t be sent.
Effectively, the withdraw() function does the opposite of what the deposit() function does: it deducts ether from the contract balance and sends it to an external address. It does this using the address membertransfer (which is like a method called on a payable address)…
<address payable>.transfer(uint amount);
The separate transfer() function (not the special transfer method in the withdraw function) shouldn’t be marked payable either, for the same reasons I’ve already outlined for the withdraw() function. In fact, calling the transfer() function doesn’t involve any ether entering or leaving the Bank contract at all. Instead, it performs an internal transfer of funds (effectively, a reallocation of funds) between two individual users of the Bank contract. The net effect to the contract balance is zero, and the only change is to the individual user balances in the mapping, in order to adjust the amount each of the parties involved in the internal transfer is entitled to withdraw from the contract (their share of the total pooled funds) i.e. the recipient’s entitlement (balance) is increased by the same amount the sender’s entitlement is reduced.
Yes, you are right that we need this line of code. I’ve checked the solution code for the Inheritance Assignment in GitHub, and this line of code is missing in the withdraw() function in the Bank contract. This is an error. Well spotted!
msg.sender.transfer(amount) transfers ether from the contract address balance to the caller’s external address, but it doesn’t adjust the individual user balances in the mapping. As I’ve explained above, these balances perform an internal accounting role, and record each user’s share of the contract’s total ether balance. So, just as we increase a user’s individual balance when they deposit ether in the contract, we need to do the opposite whenever they make a withdrawal.
If you still have difficulties depositing, withdrawing and internally transferring funds with your deployed Bank contract, you may find this post helpful. It lays out step-by-step instructions of how to properly check that all of your functions are working correctly, and it details what you should see happening and where, if it is.
You can check the Bank contract’s ether balance if you add the following function to your Bank contract (if you don’t already have it) …
function getContractBalance() public view returns(uint) {
return address(this).balance;
}
Let me know if anything is still unclear, or if you have any further questions
(1) Your full code from the 2 files containing your Ownable and Destroyable contracts;
(2) Only include your full code from the file containing your Bank contract if you have made modifications within the contract itself. Otherwise, just include the first few lines of code up to and including the contract header.
Although I inherited from Ownable as well as Destroyable, it would not be necessary as Destroyable alreads inherits from Ownable. I did it in order to have it explicit in this assignment.
// Ownable contract
// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.7.5;
contract Ownable{
address payable owner;
modifier onlyOwner {
require(msg.sender == owner);
_; //run function
}
constructor(){
owner = msg.sender;
}
}
// Destroyable contract
// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.7.5;
import "./ownable.sol";
contract Destroyable is Ownable {
function selfDestroy() public {
selfdestruct(owner);
}
}
// Bank contract
// The first few lines of code up to and including the contract header.
// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.7.5;import "./ownable.sol";
import "./destroyable.sol";
contract Bank is Ownable, Destroyable {
Your solution is almost there… Everything compiles, your inheritance structure works, the contract can be destroyed by calling the selfDestroy() function in Destroyable, and on destruction any ether still remaining in the contract will be transferred to the contract owner’s external address You just have one problem to resolve…
At the moment anyone can call selfDestroy() and destroy the contract. The funds are safe, because only the contract owner can receive them. But the fact that any address can actually trigger selfdestruct is obviously not ideal
The owner address argument, which selfdestruct() is called with, determines which external address the remaining contract balance will be transferred to. It doesn’t place any restriction on which address can trigger selfdestruct() and destroy the contract.
What functionality, inherited from Ownable, can you implement in Destroyable, to ensure that only the contract owner can call selfDestroy() and trigger selfdestruct ?
Correct
Bank only needs to explicitly inherit Destroyable, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.
Great video on inheritance! Great to visually see what this means. Am I correct in understanding the following structure?
Base Contract (parent): HelloWorld.sol (with the bank contract)
Derived Contract (child): Ownable.sol (with the Ownable contract)
Also, am I correct in making a comparison between Base/Derived Contract as parent-child components in React? For example, you have a parent item that passes down props to the child within it for example? Is this a similar-enough comparison for example purposes?
Yes and No, the Base contract will have access to Devired Contracts, but it will not pass any props, because its an inheritance, Base contract will be able to access all functionalities of Derived contract, but this does not mean that props or arguments can be sent to the Derived Contract.
Its more like a extra module for the Base contract than a parent-child components.
I can’t comment on any comparison with React, but I can confirm the following about Solidity …
Child/derived contracts are not “within” parent/base contracts. If anything, it’s the opposite (but only figuratively speaking)… When a derived contract is compiled, all of the functionality that it inherits from a parent contract is automatically included, and the result is a single set of bytecode, which is then deployed as a single smart contract at a single Ethereum address on the Ethereum blockchain.
The functionality that a derived contract inherits from its base contract(s) is the following:
All functions, except for those with private visibility (although only public and internal functions will be accessible to call from within the derived contract itself).
State variables with public or internal visibility.
All modifiers.
All events.
Constructors
Inherited state variables will include any public or internal mappings or arrays. If these store struct instances, then the properties (or attributes, as they can also be called) of these struct instances will be available to be referenced using dot notation within the derived contract.
You’ve got this the wrong way round…
Ownable is the base/parent contract, which is inherited by the derived/child contract Bank. In fact, in the Inheritance Assignment we should ideally end up with the following multi-level inheritance structure:
// Ownable.sol
contract Ownable {...} // Base/parent contract of Destroyable
// Destroyable.sol
import "./Ownable.sol"
contract Destroyable is Ownable {...} /* Derived/child contract of Ownable
AND base/parent contract of Bank */
// HelloWorld.sol
import "./Destroyable.sol"
contract Bank is Destroyable {...} // Derived/child contract of Destroyable
And just to be clear in terms of what @thecil is explaining here …
With inheritance in Solidity, it’s the derived contract(s) that can access (and have available) certain functionality in the base contract. For example, the derived contract can reference a public or internal state variable, or call a public or internal function, in the base contract; but the base contract can’t reference any of the state variables, or call any of the functions, in the derived contract.
Let me know if anything is still unclear, or if you have any further questions
What is the base contract?
Also known as parent contract, it’s the contract that is inherited or derived into a base contract either through single inheritance or multiple inheritance.
Which functions are available for derived contracts?
Any of the public, internal functions of the base contracts that are inherited.
What is hierarchical inheritance?
It’s when a single base contract is derived in multiple contracts.
No … the base (or parent) contract is inherited by the child (or derived) contract. We can also say that the child contract is derived from the base (or parent) contract.
… through any type of inheritance relationship: single, multi-level, hierarchical or multiple inheritance.
An excellent, and very well-presented solution @fedev
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.
Let me know if you have any questions about this point.
Hello. Could someone help me, please? I have a issue when I try to get the information from “getTransaction” function in Government contract. It says “call to Government.getTransaction errored: VM error: invalid opcode.”
I am adding below the two contracts. Maybe is something I did wrong and I can’t seem to notice what.
pragma solidity 0.7.5;
import "./Destroyable.sol";
import "./Ownable.sol";
interface GovernmentInterface{
function addTransaction(address _from, address _to, uint _amount) external;
}
contract EtherBank is Ownable,Destroyable{
GovernmentInterface GovernmentInstance = GovernmentInterface(0x9ecEA68DE55F316B702f27eE389D10C2EE0dde84);
//storage - means permanent storage of data (persistant data)
//memory - temporare data storage (is stored temporarely as long as the function executes)
//calldata - similar to memory, but READ-ONLY
mapping (address => uint)balance;
event depositDone(uint amount, address depositedTo);
event balanceTransfered(uint amount, address fromAddress, address toAddress);
function deposit() public payable returns (uint){
balance[msg.sender] += msg.value; //this line is not necessarly because the balance mapping has nothing to do with the value of the contract, it just keeps track of our internal balances (which address deposited which money)
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);
require(msg.sender != recipient);
uint previousSenderBalance = balance[msg.sender];
_transfer(msg.sender, recipient, amount);
GovernmentInstance.addTransaction(msg.sender, recipient, amount);
assert(balance[msg.sender] == previousSenderBalance - amount);
emit balanceTransfered(amount, msg.sender, recipient);
}
function _transfer(address from, address to, uint amount) private{
balance[from] -= amount;
balance[to] += amount;
}
}
Whenever you redeploy Government, then it will have a different contract address, and so you also need to change the Government address assigned to your governmentInstance in this line of your code in Bank:
You need to change this address before you deploy Bank, so that it’s the same as the current address that your Government contract is deployed at. If you forget to replace the previous address of your external contract for the new one, then when you call Bank’s transfer() function either of the following can happen …
(1) The transaction completes successfully, but the addTransaction() function isn’t called on the Government contract that you currently have deployed and showing in the Deploy & Run Transactions panel. So, when you then call getTransaction() on your Government contract, you get the error message that you’ve been getting:
call to Government.getTransaction errored: VM error: invalid opcode.
This is because the transfer data from the transaction wasn’t passed to the addTransaction() function that you are expecting, and so wasn’t added to the transactionlog array that you are now trying to retrieve data from. You get this error message because there is no data at the array index you are trying to look up.
OR
(2) The transaction reverts, because the external function call fails, and you get the following default error message for revert, which doesn’t actually explain what the error is:
transact to Bank.transfer errored: VM error: revert.
The transaction has been reverted to the initial state.
Note: The called function should be payable if you send value
and the value you send should be less than your current balance.
Debug the transaction to get more information.
Forgetting to update the Government contract address is easily done, and catches most of us out the first time we make this mistake
I hope that resolves your issue, but let me know if you still experience any problems.
You just have one problem to resolve…
