Your solution is almost there… but, as a result of a compiler error in Ownable, your Bank contract cannot be deployed. Your inheritance structure is well coded, and once you correct the error, everything should work as it should.
Your modifier is missing _;
This will have generated a compiler error with an error message indicating exactly what the problem is.
A couple of other comments …
(1) We can make the inheritance struture 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.
(2) In your Destruction() function, a concise alternative to using the additional receiver variable is to just call selfdestruct() with msg.sender directly…
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.
(3) You have defined the owner address state variable in Ownable as payable. But this is unnecessary, because you don’t use owner anywhere in your code where it needs to reference a payable address. It would need to be marked payable, however, if you used owner instead of msg.sender (or instead of receiver) to call selfdestruct() with the contract owner’s address, because selfdestruct() takes a single payable address argument…
selfdestruct(owner);
However, because you would only be using owner to reference a payable address once, you could just explicity convert owner to a payable address directly within the selfdestruct() function call, leaving the owner state variable itself defined as a non-payable address…
selfdestruct(payable(owner));
Let me know if anything is unclear, or if you have any questions
I find that private and internal functions can’t be payable (but I don’t know why) so I set the close() function with the visibility public.
// SPDX-License-Identifier: MIT
pragma solidity 0.7.5;
import "./ownable.sol";
contract Destroyable is Ownable {
function close() public onlyOnwer {
selfdestruct(owner);
}
}
bank.sol
// SPDX-License-Identifier: MIT
pragma solidity 0.7.5;
import "./destroyable.sol";
contract Bank is Destroyable {
// state variables and functions code of the Bank contract goes here
}
This is a very well-presented solution which meets all of the assignment objectives
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.
It’s because functions should only be marked payable when they need to receive ether and add it to the contract balance. This will only happen when ether is sent to the contract address from an external address e.g. from Remix, a Web3 client (such as the front-end interface of a dapp), or from an external (i.e. non-derived) contract.
private functions can only be called from within the same contract. A contract is deployed at a single Ethereum address and so only has one contract ether balance. The net effect of transferring ether from a contract balance to the same contract balance would be zero, and therefore pointless.
A parent contract’s internal functions can only be called from within the same parent contract or from within its derived contract(s). A derived contract’s compiled bytecode includes the code it inherits from its parent contract(s), which includes any internal functions. This combined bytecode is then deployed as a single contract, at a single Ethereum address, with a single ether balance. Therefore, any ether received by an internal function, could only have been sent from the same contract address balance as the one now receiving it; and so, as with private functions, this would be pointless, because the net effect would also be zero.
This is correct… and the close() function shouldn’t be marked payable anyway, because it doesn’t need to receive ether. It only needs to send any ether remaining in the contract when it’s destroyed to the contract owner’s external address.
The close() function should have either public or external visibility, so that it can be inherited and called externally by the contract owner’s external address if and when they want to destroy the contract. If it is marked private, then it won’t be inherited. If it is marked internal, then it will be inherited, but only available to call from within the deployed contract. This would require adding an additional public or external function which could then be called externally by the contract owner, and which would contain a function call to close(). But this would defeat one of the main reasons for using inheritance in the first place, namely, to re-use code and avoid code duplication.
Just let me know if anything is still unclear, or if you have any questions
Your solution is almost there… but, you should find that your Bank contract cannot be deployed as a result of a compiler error in Destroyable. Once you correct this error (see below) everything should work as it should.
Your inheritance structure is well coded, but we can 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.
You have chosen to use the more up-to-date Solidity v.0.8, which is only to be encouraged However, you need to pay close attention to any errors that are generated by the compiler where you wouldn’t expect this based on the syntax taught in this course (Solidity v0.7). Although, in actual fact, the only difference in syntax between these two versions which affects the code covered in this course concerns msg.sender.
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. That’s why the model solution for this assignment (based on Solidity v0.7 syntax) allows msg.sender to be assigned to a payable address variable (receiver) without having to explicitly convert it to a payable address …
However, from Solidity v0.8, msg.sender is non-payable by default, which means having to explicity convert it to a payable address when necessary. And this is why you need to make the following modification to your code in order for it to compile …
Once you’ve corrected this, it would be better to also see your complete Bank contract to check that everything there is also coded correctly, because using Solidity v0.8 requires the same modification to be made in one specific place in this contract as well. The compiler should indicate where this is with a red error marker, and the error message should confirm that it is the same type of error as the one in your Destroyable contract.
Some additional comments …
In your destroy() function, a concise alternative to using the additional receiver variable is to just call selfdestruct() with msg.sender directly…
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.
If you’d like to fully complete this course, don’t forget to also post your solution to the Events Assignment here, and your answers to the Inheritance Reading Assignment here. The Events Assignment is from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course. The Inheritance Reading Assignment is at the beginning of the Inheritance & External Contracts section of the course, and in the article you will learn, amongst other things, about the multi-level inheritance structure I’ve mentioned above, as well as other types of inheritance structures.
pragma solidity 0.7.5;
contract Ownable {
address public owner;
//modifier use as small function that will run first before main function
//use case of modifier is when many functions need to use the same logic
modifier onlyOwner {
//check the person who can add balance is only owner do
require(msg.sender == owner, "only the owner can add money/withdraw");
_; //run the function
}
constructor() {
//msg.sender is the address who deploy contract
owner = msg.sender;
}
}
Destroy file
pragma solidity 0.7.5;
import "./ownable.sol";
contract SelfDestruct is Ownable {
function destroy() public onlyOwner{
selfdestruct (owner);
}
}
Your solution is nearly there… but your Bank contract cannot be deployed as a result of a couple of compiler errors: one in SelfDestruct, and another in either BankMap or SelfDestruct. Once you correct these errors (see below) everything should work as it should.
Your inheritance structure is mostly well-coded, but we can make it more streamlined by having BankMap explicitly inherit SelfDestruct only, because it will inherit Ownable implicitly via SelfDestruct. This will give you a multi-level inheritance structure.
Compiler errors
(1) BankMap / SelfDestruct
Your Destruct.sol file imports a file called ownable.sol, but your Bank.sol file imports a file called Ownable.sol. You need to make sure that the name of the actual file containing your Ownable contract is spelt correctly and consistently (in terms of whether it starts with a capital or lower case letter) in both import statements. However, if, as I’ve suggested above, BankMap only explicitly inherits SelfDestruct, you can just remove import "./Ownable.sol"; from Bank.sol completely.
(2) destroy() function in SelfDestruct
If you read the compiler error you should be getting for this line of code, you will see that owner needs to be explicity converted to a payable address. This is because owner is defined in your Ownable contract as a non-payable address …
… but the method selfdestruct requires a payable address argument, so that when it’s called, it will automatically transfer the ether balance remaining in the contract to the address it is called with (in your case, owner).
You can easily fix this is several different ways. Which would you choose? If you’re not sure, have a look at some of the other students’ solutions, and the feedback and comments they’ve received, posted here in this discussion topic. You will find a lot of useful information here.
I would also change your error message in the onlyOwner modifier’s require() statement …
You should no longer be restricting access to any of the BankMap contract’s functions (for the reasons I explained in my feedback for your Transfer Assignment) and so this error message needs to either refer to the restriction this modifier applies to the destroy() function in SelfDestruct, or it needs to be more generic so that it makes sense in all of its possible use cases.
Let me know if you have any questions about any of these points
Hey, so I am not sure how to automatically send crypto that another person uploaded to the contract…however, my self-destruct contract stops the contract for other users that are not the admin (account that created the contract). Can you help me understand what would allow the contract to automatically send all of the crypto to the address as stated in the instructions? I am stuck on this part but everything else seems to work:
Ownable.sol
// SPDX-License-Identifier: None
pragma solidity 0.7.5;
// way to inhert from contract ownable for owner functionality
contract Ownable {
// mapping(address => uint) public balances;
// by adding public to state variable (visibility level), automatically creates a function that you can get this variable
address public owner;
// modifier of the owner
modifier onlyOwner {
require(msg.sender == owner);
// run the function
_;
}
// sets the owner as owner of contract (msg.sender)
constructor() {
owner = msg.sender;
}
}
Destroyable.sol
// SPDX-License-Identifier: None
pragma solidity 0.7.5;
import "./Ownable.sol";
contract Destroyable is Ownable {
// address payable private owner;
uint256 number;
// constructor() {
// owner = msg.sender;
// }
function store(uint256 num) public {
number = num;
}
function retrieve() public view returns (uint256){
return number;
}
function close() public onlyOwner {
selfdestruct(msg.sender);
}
}
Bank.sol
// SPDX-License-Identifier: None
pragma solidity 0.7.5;
import "./Ownable.sol";
import "./Destroyable.sol";
contract Bank is Ownable {
mapping(address => uint) balance;
// define event
// indexed = by eth nodes, can use params to search/query for events in the past
event depositDone(uint amount, address indexed depositedTo);
event balanceTransfered(uint amount, address indexed transferedFrom, address transferredTo);
event balanceWithdrawn(address indexed withdrawnFrom);
// for anyone to deposit
// payable - allows function to receive money when people call it
function deposit() public payable returns (uint) {
// if you deposit 1 ether, it should be attributed to account
// don't need to worry about memory or storage, because saved to the blockchain
// balance mapping keeps track of internal transfers/sending
balance[msg.sender] += msg.value; // to internally track who deposited money
// add event
emit depositDone(msg.value, msg.sender);
return balance[msg.sender];
}
// function for folks to withdraw eth to their address
function withdraw(uint amount) public onlyOwner returns (uint) {
// msg.sender is an address
// you can define an address payable sender and name whatever name you want to
// address payable toSend = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4;
// Transfer has revert abilities...if fails, will revert non-gas eth
// transfer has built in error handling
// MUST check balance of user to ensure can't overdraw/withdraw other people's money
// toSend.transfer(amount);
require(balance[msg.sender] >= amount); // prevents withdraw of others' money
balance[msg.sender] -= amount; // remove amount from balance before transfer occurs
msg.sender.transfer(amount); // run the transfer function
emit balanceWithdrawn(msg.sender);
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);
// add event
emit balanceTransfered(amount, msg.sender, recipient);
assert(balance[msg.sender] == previousSenderBalance - amount);
}
function _transfer(address from, address to, uint amount) private {
balance[from] -= amount;
balance[to] += amount;
}
function close() onlyOwner public {
}
}
@jon_m thank you for help fix my understand.I just understand that “method selfdestruct requires a payable address argument” and I didn’t check the file name when import well.
Your solution is nearly there… but the Bank contract cannot be destroyed by the contract owner. The destroy() function is inherited by Bank, but when you deploy your Bank contract, you will see that there is no button available for the owner to call the destroy() function externally and trigger selfdestruct(). This is because you’ve marked the destroy() function with internal visibility, which makes it available to call from within the derived contract (Bank), but it isn’t available to be called externally. So, it needs to have either public or external visibility.
Once you correct this, you will be able call destroy() with the contract owner’s address; and because everything else is well coded, the contract will be destroyed, and the ether remaining in the contract (the contract address balance) will be successfully transferred to the owner’s external address.
We can also make the inheritance structure 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.
Let me know if anything is unclear, or if you have any questions.
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.
It would be good to see how you correct this, so that your code compiles and selfdestruct (i) destroys the contract and transfers the ether balance remaining in the contract to the address it is called with (in our case, owner).
Is it needed to put the version number into every file, or just the base contract file? The compiler asked for the version number to be added.
Since im using compiler 0.8.11 I needed to put payable in the ownable contract adress and constructor. Is this right? I could not make it work until I added there.
The problem you have is your inheritance structure. The contract we want to deploy, and then destroy, is Bank. So Bank needs to inherit Destroyable, whereas yours only inherits Ownable…
Just importing Destroyable.sol isn’t enough.
Your Destroyable contract correctly inherits Ownable, which it needs to do in order to have the onlyOwner modifier available to use to restrict access to the close() function to the contract owner (we only want the contract owner to be able to destroy the Bank contract). Because Destroyable already inherits Ownable, Bank only needs to explicity inherit Destroyable, because it will inherit Ownable implicity via Destroyable. This will give you a more streamlined multi-level inheritance structure.
At the moment, your additional close() function in Bank isn’t doing anything…
When you deploy Bank, the owner can call close() but this won’t trigger selfdestruct() in the close() function in Destroyable, and so the Bank contract won’t be destroyed, and nor will the Bank contract’s remaining ether balance be transferred to the contract owner’s external address. Once Bank inherits Destroyable, because close() in Destroyable has public visibility it will be inherited and will also be available to be called externally by the contract owner. You will also need to remove the duplicate close() function in Bank as this isn’t needed and will also cause a compiler error because it has the same function name. Despite removing this duplicate close() function, close() will still appear as a function-call button in the Remix panel, because this will now be the close() function inherited from Destroyable, which is the close() function you want the owner to be able to call.
An important reason for using inheritance is to reduce code duplication. Bank is the contract that the owner would need to destroy, therefore by inheriting Destroyable, Bank does not need to contain a duplicate of the close() function.
These are the main things that you need to correct in order to get your Bank contract working as it should, and to meet the assignment objectives. I will also send you another reply with some other points for you to consider, which I’ve noticed in other areas of your code.
Let me know if anything is still unclear, if you have any further questions, or if you need any more help in getting your Bank contract deployed and working as it should.
You will get the error not found Destroyable.sol if the actual name you’ve given to the file containing your Destroyable contract is spelt differently (including capital letters) to how you’ve written it in the import statement. For example, the following will result in the same error message you’re getting…
File name: destroyable.sol
import "./Destroyable.sol";
// Bank contract
Once you’ve resolved that error, you should get another one for this line of code in Destroyable …
This is because the selfdestruct method takes a payable address argument, so that when it’s called, it will automatically transfer the ether balance remaining in the contract to the address it is called with (in your case, owner).
You can easily fix this is several different ways. Which would you choose? If you’re not sure, have a look at some of the other students’ solutions, and the feedback and comments they’ve received, posted here in this discussion topic. You will find a lot of useful information here.
After modifying your code for these points, if you’re still having difficulties getting your code to compile or work as it should, then post your full code for each file, including everything above the contract header, just in case there is an issue with any of the pragma statements.
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.
Yes you should add the pragma statement to every file, just as you have done in your posted solution.
The selfdestruct method requires a payable address argument. As you’ve referenced the owner state variable as the argument, this needs to be explicity converted to a payable address type, because originally it was defined as non-payable …
// original code: state variable defined as a non-payable address type
address owner
//or
address public owner
You can do this in 2 different ways…
(1) If we only need the contract owner’s address to be payable for the purposes of executing selfdestruct, we can leave the owner state variable in Ownable as a non-payable address type, and explicitly convert the address to payable locally, within the function where we actually need to use it as payable. We can even do this directly within the selfdestruct() function call itself, as follows:
function close() public onlyOwner {
selfdestruct(payable(owner));
}
This is the syntax you need whether you use Solidity v.0.7 or v0.8
(2) Or you can do what you’ve done, and define the owner address state variable as a payable address type by adding the payable keyword …
address payable owner;
//or
address payable public owner
Again, this is the syntax you need whether you use Solidity v.0.7 or v0.8. The difference with v0.8 is that you need to make the additional change to msg.sender in the constructor. This is because the constructor assigns msg.sender (the address that deploys the contract) to the owner state variable.
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. This means that, with v0.7, in the Ownable contract, msg.sender can be assigned to a payable address variable (owner) without having to explicitly convert it to a payable address.
However, from Solidity v0.8, msg.sender is non-payable by default, which means having to explicity convert it to a payable address when necessary. And this is why in your Ownable contract, msg.sender has to be explicity converted to a payable address using payable() in order to be able to assign it to the payable address state variable owner within the constructor.
And this is also the reason why in your withdraw() function, in Bank, msg.sender needs to be explicity converted to a payable address using payable() for the transfer method to be called on it, because the syntax requires that the transfer method can only be called on a payable address …
<address payable>.transfer(uint amount);
FEEDBACK ON YOUR WITHDRAW FUNCTION (Transfer Assignment)
(1) 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?
(2) The require statement and the return statement you’ve added to your withdraw function are both correct But you are missing one very important line of code…
If you deploy your contract, make a deposit, and then call your withdraw function, you will notice that the caller’s address receives the requested withdrawal amount, but their balance in the mapping is not reduced (call getBalance to check this). I’m sure you can see that this is a very serious bug because, even though there is a limit on each separate withdrawal, this limit is never reduced to reflect each withdrawal. This means that a user can withdraw more than their individual balance (the share of the total contract balance to which they are entitled).
payable(msg.sender).transfer(amount) transfers ether from the contract address balance to the caller’s external wallet address, but it doesn’t adjust the individual user balances in the mapping. These balances perform an internal accounting role, and record each user’s share of the contract’s total ether balance (effectively, each user’s entitlement to withdraw ether from the contract). 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.
Once you’ve had a go at correcting your code for this, have a look at this post which explains the importance of the order of the statements within your withdraw function body.
And don’t forget to post your solution to the Events Assignment here. This assignment is from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.
As always, just let me know if anything is unclear, or if you have any further questions
However, you need to pay close attention to any errors that are generated by the compiler where you wouldn’t expect this based on the syntax taught in this course (Solidity v0.7). Although, in actual fact, the only difference in syntax between these two versions which affects the code covered in this course concerns 
