We can also streamline the inheritance by having Bank just inherit Destroyable, because it will then indirectly inherit Ownable via Destroyable. This will give you a multi-level inheritance structure.
I couldnât help noticing that you have a couple of issues in your Bank contract. I know this isnât part of this assignment, so Iâm wondering if this is just a copy-and-paste issue, and if the contract youâve been deploying and testing doesnât have these errorsâŠ
function deposit() public payable{
balance[msg.sender] = msg.value; // operator should be +=
emit depositDone(msg.value, msg.sender);
}
// When we deposit ether, we want to add it to our balance, not replace it!
/* This function doesn't need to be payable, because it's sending
not receiving ether */
function withdraw(uint amount) public payable onlyOwner{ //remove payable
require(balance[msg.sender] >= amount);
msg.sender.transfer(amount);
//balance[msg.sender] -= amount; //include this before transfer()
}
Thatâs a very good question, and shows youâre really thinking through whatâs happening while doing your testing.
We would only actually use selfdestruct() in limited circumstances. And you have quite rightly pointed out one of its disadvantages. The point of selfdestruct() is to prevent or reduce the impact of a much worse scenario: an attacker finding a way to exploit an unforeseen vulnerability in the contract code and stealing all the funds. Having the funds go to an âownerâ entity obviously relies on a certain degree of trust, but this is obviously preferable than losing everything to a malicious attacker, and we are much more likely to get a refund (or at least a partial one)! In our simple example, a single owner address triggers selfdestruct() and the transfer of the remaining funds to their own address. In reality, it might need the authorisation of more than one addresss, and the transfer of funds could be to a different contract address instead of a wallet address. However, my understanding is that, in practice, use of selfdestruct() is not encouraged, and other types of security mechanisms are built into smart contracts instead, such as the implementation of proxy contracts. You will learn more about the practicalities of this if you go on to take the Smart Contract Security course.
If you do some of your own research about selfdestruct() you will find that it is by no means a perfect security solution, and comes with specific weaknesses and disadvantages. However, its relatively straightforward and easy-to-understand functionality makes it ideal to use in an introductory example of how inheritance works. And to be honest, weâve used it more as a way to demonstrate how inheritance works and is coded in Solidity, rather than to teach you selfdestruct() for its own particular merits.
I hope that goes some way towards answering your question
Noted. Your answers are very detailed and clear.
Something else I have noticed.
If the transfer function is onlyOwner then if an address other than the contract owner deposits funds into the bank contracts it wonât be able to withdraw. And since transfer only concerns msg.sender I think there is no issue if we make the withdraw function just public (not onlyOwner).
I have tested it and it works fine but I donât know if this the best solution.
What do you think?
Yes, youâre absolutely right. I think during the course we were adding and removing onlyOwner from various functions to demonstrate various different scenarios. Somehow it ended up being left in the withdraw() function. We would only want to restrict the withdrawal of funds to the owner, whilst allowing any address to deposit funds into the contract, in scenarios such as: donating funds to a project; users depositing funds within a dapp where, once deposited, are restricted to being spent on transactions related to that dapp only; or where there are other mechanisms for withdrawal provided by functions other than withdraw(). The different permutations are endless.
So, here, there isnât really a âbestâ solution: it just depends on what scenario you want to cater for. You will get so much more out of these assignments by doing exactly what youâre doing â experimenting with the code, and modifying and testing it with a view to improving it, and adapting it to different scenarios and use cases that are meaningful to you.
⊠also, just having looked at the code again, by using onlyOwner at least somewhere within Bank (even if the practicality of this is rather forced) allows us to demonstrate that the functionality of the onlyOwner modifier (defined in Ownable) is also available in Destroyable and Bank by inheritance. That might be why it was left there, although I totally agree with you that it doesnât make a lot of practical sense in the overall context of this particular contract.
Hi @dani88,
It looks like you have your contracts mixed up hereâŠ
This function should be in Destroyable, because this is the functionality that will be used to destroy the contract that inherits it. Destroyable needs to inherit Ownable, in order for it to have access to the owner state variable and the onlyOwner modifier. So, if Destroyable inherits Ownable, it also needs to import the .sol file which contains Ownable (not helloworld.sol).
close() now doesnât return a value (which is correct), but youâve forgotten to also removeâreturns()âfrom the function header. And as I mentioned before:
Without seeing your Ownable contract, I canât see if you have defined the owner address as payable. If you havenât, another option is to convert owner to a payable address when passing it to selfdestruct(). An even simpler way to do this, is to call selfdestruct() with msg.sender instead of owner, because in the version of Solidity youâre using, msg.sender is payable by default. Even if you use msg.sender, selfdestruct can still only be initiated by the contract owner, because access to the close() function is restricted by the onlyOwner modifier (inherited from Ownable).
Then, as I mentioned beforeâŠ
Because of the inheritance structure, it will be easier for us to check your code if you post all 3 contracts. Did you try to attach a screen shot in your last post, as well as the code? All that shows is:
Anyway, instead of screen shots, please post your actual code, because this makes it much easier for us to check it.
I took a screenshot of the error message. I thought it might help. Thanks for taking the time to explain this.
I feel that I am getting closer to the answer. I appreciate your guidance. âŠthanks for not just giving me the answers. I have worked hard on this, and itâs been good for me.
So, I realized that if âOwnableâ was the owner, then âDestoryableâ must be the victim!!, so I changed the name⊠Also, I put an empty function in âDestroyableâ, it flagged it, so I deleted it.
Solidity is flagging "contract HelloWorld is Ownable, Destroyable {. line 5
says that definition of base has to precede definition of derived contract contract HelloWorld is Ownable, Destroyable {
Hereâs the code that I have:
pragma solidity 0.5.12;
contract Ownable{
address public owner;
modifier onlyOwner(){
require(msg.sender == owner);
_; //Continue execution
}
constructor() public{
owner = msg.sender;
address payable owner;
}
}
`import "./Ownable.sol";
import "./Destroyable.sol";
pragma solidity 0.5.12;
contract HelloWorld is Ownable, Destroyable {
struct Person {
uint id;
string name;
uint age;
uint height;
bool senior;
}
event personCreated(string name, bool senior);
event personDeleted(string name, bool senior, address deletedBy);
uint public balance;
modifier costs (uint cost){
require(msg.value >= cost);
_;
}
mapping (address => Person) private people;
address[] private creators;
function createPerson(string memory name, uint age, uint height, bool senior) public payable costs(1 ether) {
require(age < 150, "Age needs to be below 150");
balance += msg.value;
//This creates a person
Person memory newPerson;
newPerson.name = name;
newPerson.age = age;
newPerson.height = height;
if(age >= 65){
newPerson.senior = true;
}
else{
newPerson.senior = false;
}
insertPerson(newPerson);
creators.push(msg.sender);
assert(
keccak256(
abi.encodePacked(
people[msg.sender].name,
people[msg.sender].age,
people[msg.sender].height,
people[msg.sender].senior
)
)
==
keccak256(
abi.encodePacked(
newPerson.name,
newPerson.age,
newPerson.height,
newPerson.senior
)
)
);
emit personCreated(newPerson.name, newPerson.senior);
}
function insertPerson(Person memory newPerson) private {
address creator = msg.sender;
people[creator] = newPerson;
// ln 78 creator.send(); does not work. one is address the other is address payable This forces the creator of the contract to think
//of whether or not the creator needs to receive money. It shouldn't in the example written.
//so, address creator = msg.sender; Problem is converting a payable to a non-payable address.
//to return = address payable test = address(uint160(creator));
}
function getPerson() public view returns(string memory name, uint age, uint height, bool senior){
address creator = msg.sender;
return (people[creator].name, people[creator].age, people[creator].height, people[creator].senior);
}
function deletePerson(address creator) public onlyOwner {
string memory name = people[creator].name;
bool senior = people[creator].senior;
delete people[creator];
assert(people[creator].age == 0);
emit personDeleted(name, senior, owner);
}
function getCreator(uint index) public view onlyOwner returns(address){
return creators[index];
}
function withdrawAll() public onlyOwner returns(uint){
uint toTransfer = balance;
balance = 0;
msg.sender.transfer(balance);
return toTransfer;
}
function close() public payable onlyOwner{
selfdestruct(owner);
}
}
`import "./helloWorld.sol";
pragma solidity 0.5.12;
contract Destroyable{
}`````
Yes⊠youâll learn more this way. Iâm glad youâre finding this approach helpful
Line 5 defines which base (parent) contracts (Ownable and Destroyable) are inherited by the derived (child) contract (HelloWorld). You have correctly imported Destroyable.sol into helloWorld.sol, but you have also imported helloWorld.sol into DestroyableâŠ
Only the base/parent contract should be imported into the derived/child contract, because it is the parent contractâs functionality that we want to have available in the child contract, and not vice versa.
When you remove the lineâ import "./helloWorld.sol";â from Destroyable.sol the error will disappear.
But what is the point of HelloWorld inheriting Destroyable, if Destroyable is empty? None whatsoever. This is why I said in my last post that the close() functionâŠ
At the moment, you have the close() function in HelloWorld. You need to move it to Destroyable. You could leave it in HelloWorld and just not inherit Destroyable. But the whole point of this assignment is to demonstrate and practise inheritance. It is also good practice to keep separate functionality in separate modules (here, our separate modules are separate files).
You will then get an error for your selfdestruct() function call (now in Destroyable):
If you look at the error message, you should be able to see that the problem is that owner still isnât a payable address. Making the close() function payable (as you have) doesnât convert the owner address to a payable address â it only enables the close() function to receive ether, which it doesnât need to do. Itâs the owner address that needs to receive ether, so you need to either:
1)âConvert owner to a payable address in close() either (i) using a local variable e.g.
Important noteâ This conversion of a non-payable to a payable address is now easier in Solidity v0.6 and above, where you can just useâ payable(nonpayableAddress)â The more up-to-date syntax is taught in the new Ethereum Smart Contract Programming 101 course where we use v0.7.5
or
2)âAs I mentioned beforeâŠ
or
3)âDefine the owner address as payable within Ownable. This would be done in the state variable:
address payable public owner;
⊠and not in the constructor:
Implementing just one of the above 3 methods will remove the remaining compiler error, and also the addtional warnings (flagged in orange) in Ownable. You will then just be left with one orange warning in HelloWorld for this line:
See if you can work out from the warning message what it is you need to remove. Can you also work out why can you remove it?
Thanks so much for your patience with me. I appreciate it:
(2 ether) should read () or (msg.value). as per your instructions earlierâŠsorry I missed that one.
almost there!!
I am getting an error message on helloWorld
contract HelloWorld is Ownable, Destroyable {
This error is affecting all functions that are restricted to owner. When I delete âDestroyableâ, the error message goes away, but the assignment isnât complete. It is written just as it was in the video, and I am not sure what I did wrong.
I have auto compile checked, so remix compiles what I change.
This was correct before⊠but now youâve removed itâŠ
The problem was thisâŠ
In summary:
If an inherited contract (one that you have included in the contract header after is ) is in a separate file, then you MUST IMPORT THE FILE WHERE THE INHERITED CONTRACT IS LOCATED, AS WELL AS declaring it in the contract header. You havenât done this inâhelloWorld.solâor inâDestroyable.sol
After youâve got your inheritance fixed, you can then start to address the issue with owner still not being a payable address, by following my earlier email from here:
No ⊠those instructions were related toâ1 etherâbeing an invalid function parameter in your original leaseAgreement contract, which I suggested you changed to HelloWorld.
Usingâ1 etherâas the argument in the modifierâcosts(1 ether)â is correct and the warning you will get doesnât refer to that. When you reach this stage of the compiling progress, read the actual error message you get and it will tell you what you need to remove. But can you work out why you need to remove it?
And just one other thing⊠if you send ether to a function, you donât need to explicity state msg.value as a parameter. Solidity will automatically make msg.value available for you to reference within the function body (in the same way that it does with msg.sender).
import './Ownable.sol';
import './selfdestruct.sol';
contract Bank is Ownable, SelfDesctuct{
// all previous code...
function destroyContract() private{
close();
}
}
I will reuse the code from the class to build my destroyable class.
pragma solidity 0.7.5;
//Legacy code from the Inheritance Class. We should made owner address "payable", as selfdestruct requires a payable address
contract Ownable
{
address payable public owner;
modifier onlyOwner
{
require(msg.sender == owner);
_; // run the function
}
constructor()
{
owner = msg.sender;
}
}
//We will create a derived contract that will inherit behavior from the Ownable contract
contract Destroyable is Ownable
{
//Our close function will enforce contract ownership using the onlyOwner modifier
function close() public onlyOwner
{
//then we finally destroy the contract, since the proper validation was done by the modifier
selfdestruct(owner);
}
}
Thank you for this feedback.
So first I have modified the pragma statements to: pragma solidity 0.8.0; in all files. I think it is good to use a newer compiler, also pinning a specific version of it. It is also clear with what compiler this is supposed to work.
Then whenever I need a payable sender address I use: payable(msg.sender).
I saw that you wanted me to make âownerâ payable. I honestly thought this code had filled that requirement, so I didnât address it. I will use another method to make it payable.
I will take that course when I finish this one. I figure that I may need the information in this contract for some reason, and besides, the repetition will do me good!!
If I comment out âonlyOwnerâ. the error goes away, so that would be my guess. I have no idea how I can do that and restrict the functions. BTW, If I do comment out those three functionsâ âonlyOwnerâ feature, it leaves only one error itâs âcontract HelloWorld is Ownable, Destyroyable {â
The ownerâs contract is below:
When I compile I am little confused to whether the transfer functionality is working as I seem to be getting a revert error. This may just be me not fully understanding how to work with the remix platform or it may be in the code but I canât seem to figure it out. Other then that it compiles and I am able to use the self-destruct function within the Bank contract.
