Hi @markzachary,
… and welcome to the forum! … I hope you’ve been enjoying the course!
The solution you’ve posted inherits 2 other contracts, so can we see those as well, so that we can fully understand how it works and deploy and test it if necessary? Unless we can see the parent contracts, it’s also difficult to make comments or provide you with feedback with any degree of certainty.
Having said that, I assume that ethBank4 must be what we’ve been calling Ownable, and ethBank2 what we’ve been calling Bank, right?
Have you tried testing your contract? … If you have, and assuming I’m right in my assumptions about the parent contracts, you should have found a few problems that need solving…
(1) The contract owner will never be able to destroy the contract, because the line of code with the selfdestruct
method is unreachable. On compiling your code you should get an orange warning that notifies you about this. It’s unreachable, because a function will finish executing after a return statement.
Have you completed all of the other course assignments? If you haven’t, I would strongly suggest that you do, and that you post your solutions, because that will help you practice and become aware of these sorts of concepts. It will also give you a chance to get some useful feedback about your progress before moving on to more advanced topics.
(2) During testing, after several different transactions have been performed — involving the deposit, withdraw and transfer functions, and resulting in several users holding shares of the total contract address balance — you will notice that when the contract owner calls the destroy() function, they only receive their share of the remaining contract balance, and not all of the remaining funds. This is because the withdraw() function (which I assume is inherited) is being called with a withdrawal amount equivalent to the contract owner’s individual balance in the mapping, and not the total contract address balance.
In fact, you don’t even need this line of code …
As well as destroying the contract, selfdestruct
also automatically transfers 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, and is why there is no need to include an extra line of code to perform this transfer of funds.
(3) Before making any amendments for the above issues, the return statement is currently returning the contract owner’s total external address balance (showing in the Account field near the top of the Deploy & Run Transactions panel in Remix) after the withdrawal of their individual share of the total Ether held in the contract, but before the close
transaction’s gas cost has also been deducted.
However, after making the necessary modifications for (1) and (2) above, even if this return statement is corrected to return the total remaining contract address balance, it should still be removed, because …
-
If it’s placed before selfdestruct
, it will prevent selfdestruct
from executing (as already discussed above);
-
If it’s placed after selfdestruct
, the return statement itself cannot execute, because once selfdestruct
has been executed, the contract will have been destroyed and its code removed from the blockchain, and so the return statement will actually no longer exist, anyway.
I would also suggest that the contract we were calling Bank should be the most derived contract in your inheritance structure, the contract that we actually deploy, and not Destroyable. Once you’ve corrected your close() function for the above issues, even though your exisiting inheritance structure will now produce the desired result when Destroyable is deployed, one important reason for using inheritance is to create re-usable modules of code. And by having Destroyable inherit Bank, we are considerably reducing the re-usability of Destroyable, because instead of other contracts just being able to implement its “off-the-shelf” contract destruction functionality, together with the contract ownership functionality which Destroyable also inherits from Ownable, these other contracts will also have to inherit all the additional “baggage” from Bank, most of which they probably won’t need.
So, see if you can also come up with an alternative inheritance structure, which increases overall modularity and re-usability.
Let me know if anything is unclear, if you need any further help adapting your solution, or if you have any questions