When I say works, I mean it compiles and deploys; and then I can test the selfdestruct functionality by performing the following transactions âŚ
The owner calls deposit() with say 5 ether â the ownerâs external account balance reduces by 5 ether, and their individual Bank account balance increases by 5 ether.
The owner calls destroy() â the contract is destroyed and the remaining contract balance of 5 ether is transferred to the ownerâs address (their external account balance increases by 5 ether).
Were you amending the code for the inheritance as I have? If so, then what exactly isnât working when you try it?
Oddly, I havenât made any changes within the code, but it will compile and deploy the Bank contract now just fine (with the multi-level inheritance). I originally amended the code for inheritance as you have, but it would not compile for some strange reason. But itâs working now and Iâll take that as a win!
Unfortunately, there are quite a few issues with your code. Iâve also noticed that you havenât posted the first 2 assignments for this 101 course (Data Location, and Events). Have you watched all of the videos for the first part of this course, and practised with the code until you are comfortable with all of the concepts and syntax? Iâve also seen that youâve jumped ahead to the 201 course, but I strongly recommend that you go back over this course and complete it, before moving on to 201, because you need to be comfortable with all of the basics first.
Iâve seen that youâve corrected most of the issues @dan-i mentioned in his feedback on your Transfer Assignment but donât forget thatâŚ
Youâve increased the individual account holderâs balance in the deposit function, but you havenât decreased it in the withdraw function.â msg.sender.transfer(amount) transfers ether from the contract address balance to the callerâs external wallet address, but it doesnât adjust their individual user balance in the mapping. These balances perform an internal accounting role, and record each userâs share of the contractâs total ether balance. If you donât reduce a userâs individual balance for each amount withdrawn, they can continue to make repeat withdrawals up to the total amount of ether held in the contract. In other words, not only can they withdraw their own funds, but everyone elseâs too!
Youâve also modified the withdraw function with the onlyOwner modifier, but this means that only the contract owner will be able to withdraw funds, whereas the contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so it only seems fair that all users should be able to withdraw their funds as well, donât you think?
Coding for inheritance
Your first import statement is correct, but your second is missing something. Can you see what it is?
If you want a single derived contract to inherit two parent contracts, you canât declare the same derived contract twice. You need to use the following syntaxâŚ
contract Bank is Ownable, Destroyable { ... }
You also need to make sure you are using exactly the same contract names used in the parent contracts, including consistent use of upper/lower case letters, otherwise youâll get a compiler error:
To avoid this kind of error, itâs best to stick to the convention of starting all contract names with a capital letter.
If Destroyable inherits Ownable, then the owner state variable and the constructor in Ownable will be available in Destroyable, meaning that you donât need to include them in both contracts. Being able to avoid code duplication is one of the reasons for using inheritance.
Other code in Ownable
This modifer in Ownable has 3 syntax errors â can you see what they are and correct them? Here are 3 hints:
Your modifierâs name cannot contain any spaces.
Check your opening and closing brackets.
You are missing the line of code that means ânow execute the functionâ.
selfdestruct() functionality in Destroyable
When youâve corrected your code so that Bank successfully inherits Destroyable, calling the close() function will destroy the Bank contract, and the remaining contract balance will automatically be transferred to the ownerâs external address However, currently, anybody can call close() and trigger selfdestruct() in the first place. Even though the funds are essentially safe â we are assuming the owner will be a trustworthy custodian of usersâ funds in the interm period, until they can either be transferred to a new contract or refunded â allowing anyone to trigger the contract-destruction and fund-transfer process obviously creates a highly volatile environment! So you need add some code to restrict access to this function to the contract owner.
As well as destroying the contract, triggering selfdestruct() also automatically transfers any ether remaining in the contract to its payable address argument (owner). So you donât need to try to code this separately. Iâm not sure if this is what you were trying to do here âŚ
⌠but, in any case, all this function could potentially do is allow the caller to view their individual balance, which duplicates what the getBalance() function in Bank already does. And anyway, this function wonât compile in Destroyable because the balance mapping is declared in Bank, not Destroyable, soâ return balance[msg.sender]â throws an error.
Have a go at making these modifications, and Iâll take a look at it again for you. But let me know if anything is unclear, or if you have any questions
Overall, your coding of the inheritance structure and the selfdestruct functionality is good
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.
Here, a concise alternative is to call selfdestruct() with msg.sender directly, instead of using the additional recipient variable.
selfdestruct(msg.sender);
Have a look at this post for further details about the use of the additional local variable in the model solution.
Access to your deposit() function is now restricted by the onlyOwner modifier, which wasnât included in the function header before. This should be removed, otherwise no one other than the contract owner will be able to deposit ether into the Bank contract â unless this is what you intend, and then for the owner to distribute this ether to other users via the transfer() function ⌠?
Donât forget to address both of these additional points I mentioned about your Bank contract. Itâs especially important to include the additional statement in your withdraw function to adjust the userâs individual balance, otherwise your contract will fail to prevent users from withdrawing more ether than they are entitled to.
How is the below? Contract bank code wasnât really changed, just the headers to import
pragma solidity 0.7.5;
contract Ownable {
address payable public owner;
// When placed in a function this will run first
modifier onlyOwner {
require(msg.sender == owner);
_; // run the function that called this
}
constructor(){
owner = msg.sender;
}
}
pragma solidity 0.7.5;
import "./artifacts/Ownable.sol";
contract Destroyable is Ownable {
function close() public onlyOwner{
selfdestruct(owner);
}
}
pragma solidity 0.7.5;
import "./Destroyable.sol";
contract Bank is Destroyable {
Below each of the changes I suggested, Iâve copied the relevant part of your code and shown you how to modify it. Iâve commented out code which would need to be removed, and written comments to show where Iâve added code âŚ
pragma solidity 0.7.5;
// import "./Ownable.sol";
import "./Destroyable.sol";
interface GovernmentInterface {
function addTransaction(address _from, address _to, uint _amount) external;
}
contract Bank is /*Ownable, */Destroyable { ... }
function Destroy() public onlyOwner {
// address payable recipient = msg.sender;
selfdestruct(/*recipient*/msg.sender); // recipient REPLACED with msg.sender
}
function deposit() public payable /*onlyOwner */returns(uint) {
balance[msg.sender] += msg.value;
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; // ADDED
msg.sender.transfer(amount);
return balance[msg.sender];
}
In order to reduce security risk, the statements should be in the following order within the function body:
Check inputs (require statements)
Effects (update the contract state)
External interactions
Itâs important to modify the contract state for the reduction in the individual userâs balanceâŚ
balance[msg.sender] -= amount;
before actually transferring the funds out of the contract to the external addressâŚ
msg.sender.transfer(amount);
⌠just in case there is an attack after the transfer, but before the state is modified to reflect this operation. Youâll learn about the type of attack this prevents, and how it does it, in the courses which follow this one.
function transfer(address recipient, uint amount) public {
require(balance[msg.sender] >= amount);
require(msg.sender != recipient);
uint balanceBeforeSending = balance[msg.sender];
_transfer(msg.sender, recipient, amount);
governmentInstance.addTransaction(msg.sender, recipient, amount);
assert(balance[msg.sender] == balanceBeforeSending - amount);
emit transferDone(msg.sender, recipient, amount); // MOVED TO HERE
// Note that you need to change two of the arguments
}
function _transfer(address from, address to, uint amount) private {
balance[from] -= amount;
balance[to] += amount;
// emit transferDone(from, to, amount);
}
I hope thatâs clearer, but just let me know if you have any further questions
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.
address payable owner;
constructor() {
owner = msg.sender;
}
modifier onlyOwner {
require(msg.sender == owner, "You're not the owner of the contract.");
_; // run the function
}
}
// Destroyable.sol file
import â./Ownable.solâ;
contract Destroyable is Ownable {
function close() public onlyOwner {
selfdestruct(owner);
}
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.
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.
No you donât need to. 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 will still work in exactly the same way if Bank explicitly inherits both Destroyable and Ownable, and if both destroyable.sol and ownable.sol are imported, but itâs unnecessary.
There are a few issues here, but they are easily fixed:
(1) The following two lines of code in Destroyable.sol contain misspelt keywords. These generate red compiler errors, but maybe these are just copy-and-paste errors and arenât in your code in Remix âŚ
(2) Bank canât be deployed because of the following compiling issue caused by Destroyable.sol âŚ
Imported files must have a compiler declaration that is compatible i.e. both files must be able to be compiled using the same version of Solidity.
Your Ownable.sol can only be compiled using v0.7.5. It cannot be imported into your Destroyable.sol because your Destroyable.sol requires a compiler version of v0.7.6 or above âŚ
Your Destroyable.sol also cannot be imported into your Bank.sol for the opposite reason: your Bank.sol can only be compiled using v0.7.5.
Remix should have generated red compiler errors for this. These errors wonât have highlighted a specific line of code, and so the error messages canât be viewed by hovering over the red indicator highlighting the line number. However, they will be displayed in the Solidity Compiler panel â the Solidity Compiler icon indicates this with the number of error messages highlighted in red.
(3) Once the above issues are fixed, and your code compiles successfuly, youâll be able to deploy your Bank contract. However, youâll notice that the destroy() function is not available for the owner to call. Your selfdestruct functionality is coded correctly, but itâs not inherited by Bank. Can you correct this? When you have, you will be able to deploy Bank, perform some transactions, and then get the contract owner to destroy the Bank contract (by calling the destroy function from Remix). Destroying the contract will also transfer any remaining ether balance to the ownerâs external address.
Here, a concise alternative is to call selfdestruct() with msg.sender directly, instead of using the additional receiver variable.
selfdestruct(msg.sender);
Have a look at this post for further details about the use of the additional local variable in the model solution.
Let me know if anything is unclear, or if you have any questions about how to correct your code for these issues