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
Yes, if you wanted to, you could call the file something totally different to the name of the base contract it contains. Then, for a derived contract to inherit that base contract, the exact file name is what goes in the import statement, and the exact contract name is what goes in the derived contract header (after the is keyword).
The changes you’ve made to Ownable look good. Are you using Solidity v0.8 in all 3 contracts? I assume you must be because you have explicity converted msg.sender to a payable address in the constructor:
You only need to do that if you’re using version v0.8. If you’re using v0.7 (like you were for previous assignments) then you don’t need to add payable() here, because prior to v0.8, msg.sender is already payable by default.
If you are using v0.8, then you should also have modified the following line in your withdraw() function in Bank…
// <payable address>.transfer(uint amount)
// The transfer method must be called on a payable address
// Solidity v0.7
msg.sender.transfer(amount); // msg.sender payable by default
/* where msg.sender needs to reference a payable address (such as here)
it DOESN'T need to be explicity converted */
// Solidity v0.8
payable(msg.sender).transfer(amount); // msg.sender non-payable by default
/* where msg.sender needs to reference a payable address (such as here)
it DOES need to be explicity converted */
contract Ownable {
// we make this payable so we can receive/send ether, includes additional
// members "transfer" and "send".
address payable owner;
modifier onlyOwner {
require(msg.sender == owner);
_; // means run the function
}
constructor(){
// initialize owner here and set to deployed address owner
owner = payable(msg.sender);
}
}
import "./Ownable.sol";
contract Destroyable is Ownable {
function destroy() public onlyOwner {
selfdestruct(owner);
}
}
pragma solidity 0.7.5;
// first few lines of Bank only
import "./Destroyable.sol";
contract Bank is Destroyable {
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.
Even though you didn’t need to post your full EtherBank contract for this assignment, it would be good to see your final version after you’ve corrected the withdraw() function. Have you managed to deploy a fully working EtherBank contract, perform some deposits, internal transfers and withdrawals, and finally have the contract owner destroy the contract, transferring all of the remaining ether in the contract address balance to the owner’s external address?
Let me know if you have any questions about anything
Hi Jon. Thanks for following up and pushing me to go further . Please find below my etherbank contract. All operations went good.
pragma solidity 0.7.5;
import "./Destroyable.sol";
import "./Ownable.sol";
contract EtherBank is Ownable,Destroyable{
//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);
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;
}
}
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.
Are you using Solidity v0.7.5 in all 3 contracts? If you are then you don’t need to explicity convert msg.sender to a payable address in the constructor in Ownable …
You only need to do that if you’re using version v0.8. If you’re using v0.7, then you don’t need to add payable() here, because prior to v0.8, msg.sender is already payable by default.
If you use v0.8, then you should also modify the following line in your withdraw() function in Bank…
// <payable address>.transfer(uint amount)
// The transfer method must be called on a payable address
// Solidity v0.7
msg.sender.transfer(amount); // msg.sender payable by default
/* where msg.sender needs to reference a payable address (such as here)
it DOESN'T need to be explicity converted */
// Solidity v0.8
payable(msg.sender).transfer(amount); // msg.sender non-payable by default
/* where msg.sender needs to reference a payable address (such as here)
it DOES need to be explicity converted */
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
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?
Your inheritance structure is well coded
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
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? 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 …
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? 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.
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? 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.
