So, I can deposit Eth into my Bank.sol contract. I can withdraw with no errors, but when I transfer, I get an error. To transfer, I have gone to the top![Screen Shot 2021-05-14 at 4.18.21 PM|517x499]
(upload://lruqF1rkA9FzOz0GE4RKeD2crV3.png)
From what I have gathered, the first address is the Bank.sol contract. The others are available to transfer money to. So, I have copied some of the others, and tried to send money to them. After I hit copy, I moved the first back to the field, so that the window shows only the first address, but I paste another into the address bar as recipient. and I am getting an error. The code looks identical to me, what am I missing?
The error message says that the call function should be payable. I tried payable between “amount)” and “public”, it didn’t work.
Thank you so much for your reply! I will address your tips.
Also, I checked the solution code and I noticed that in Destroyable contract, there is a line of code stating the msg.sender as payable. Is this 100% necessary? I’m not sure why is it so.
import "./Ownable.sol";
pragma solidity 0.7.5;
contract Destroyable is Ownable {
function destroy() public onlyOwner {
address payable receiver = msg.sender;
selfdestruct(receiver);
}
}
Oh dear, that does sound frustrating! I think I understand the problem you are having. If you enter the course from the My Courses page (which I think is the default page when you log in to the Academy) then the algorithm will automatically take you to what it thinks you should be watching next. But if you want to start from the beginning, there are 2 ways to override this:
Navigate to the All Courses page (from the menu at the top of the page) and find the course. Then, when you enter it, it should take you to the course contents page, where you can click on the very first lesson and start from there;
Enter the course as normal from My Courses, and on the lesson page it takes you to, go to the “index path” (like a file path) just above the video, and navigate to the course contents page from there.
You can also use either the course contents page, or the “index path” on each page, to navigate to any lesson in the course you want to watch again (again, overriding the automatic selection algorithm). I think this is what was meant by…
If all, or some of the lessons in the new course have already been marked as completed, then in each individual lesson you can click on the blue button below the video that says Complete. It should then turn back to white and say Mark As Complete. If you want to start the course from the beginning and work your way through from lesson to lesson, then you could go into all the lessons it says you’ve completed and reset this button — that way the “Continue where you left off” algorithm should work again as it should for someone starting the course from the very beginning, and so you should then be able to just continue with the course from the default My Courses page (probably what you usually use).
So… we can’t do that for you… but you should be able to
Let me know if that makes sense, and if it works for you
For this Inheritance Assignment, the code you’ve posted looks good, Donnie
I’ve deleted all of your code relating to the Government contract and the external function call, because that comes in the next section, so you need to post any code related to that here. I don’t want to over-complicate this discussion post with code students won’t have seen yet when they do this assignment
In terms of the code in the close() function body…
The method you have used in your code is correct: define the owner address as payable within Ownable…
address payable owner;
… and call selfdestruct() with owner . This still works in v0.7 using the same syntax.
It’s only in v0.8 where msg.sender changes from being payable to non-payable by default. So, you can use selfdestruct(msg.sender); for v.0.7 as well.
So, instead of having to explicitly convert owner to a payable address using…
address(uint160(nonpayableAddress));
…we can now perform this conversion using the simpler…
payable(nonpayableAddress);
So, you need to modify your 2 options (comments in your close() function) for this change.
Hopefully you can go back and complete this course from the beginning. If you do, then, when you come to do the Transfer Assignment you will notice that you have an important line of code missing in your withdraw() function. If you test your code as it is at the moment, you should be able to already see what the serious bug is…
This could well be an error thrown because you haven’t previously deployed your Government contract and/or haven’t replaced the Government address in your Government instance definition (in Bank) with the new one. You should be deploying your Government contract before deploying Bank. Then, if you redeploy Government at any stage, it will have a different contract address, which needs to be updated in Bank, otherwise the external contract call to addTransaction() will fail and cause the whole transfer() function to revert.
If this is what is causing the error, the problem is that the error message you get is just a default one, and so that’s why it isn’t helpful in pinpointing the error. If it’s a default message then the error may not have anything to do with the function not being marked payable. The withdraw() function does not need to receive ether (it only sends it) so it doesn’t need to be marked payable. This is why adding payable to the function header didn’t solve the problem.
Ideally, I think you should go back and complete the earlier part of the course, because if you haven’t seen the lectures where the transfer function is introduced and explained, it will be difficult to follow and fully understand the additional changes we make with the external function call (within the transfer function) to the Government contract.
Also, I think you are missing some key principles from the earlier lessons and assignments:
No, all of the addresses in the dropdown are simulating external wallet addresses for different users. The one you deploy the contract with (by default the top one) will become the Bank contract owner — as a result of our constructor and owner state variable.
The contract address is the one next to the deployed contract where you can open up all of the function calls you are able to make from Remix. You only need to copy and paste the Government contract address whenever you need to replace the one in the contract instance definition (explained above).
The Bank contract address automatically receives ether when the deposit() function is called. That’s why we make it a payable function.
<address payable>.transfer(uint256 amount) automatically transfers ether from the contract address, to whichever external wallet address (from our dropdown) calls the withdraw() function.
The transfer() function that you are talking about (and which is throwing an error and reverting) involves no ether being transferred between an external wallet address and the contract address. When transfer() is executed the total amount of ether held in the contract remains the same — it is only an internal Bank transfer between two bank account holders, reflecting the change in each of their shares in the total funds.
However, we do identify each account holder’s internal Bank balance (stored in the mapping) by their external wallet address, so doing the following is correct…
… except that, after copying the recipient’s address, you can then select any address that has a bank account balance > 0, because, as I have just mentioned, you aren’t transferring from the contract address, but simply making an internal transfer between 2 account holders. When you’ve got this transfer() function working, you will see the change in the sender’s and recipient’s bank account balance by calling the getBalance() function with their respective addresses. But, you won’t see a change in the ether balance in their external wallets, because this will only change when a deposit or withdrawal is made to/from the contract address.
… but try depositing amounts from several addresses, and then get one of those addresses to withdraw what it deposited. If the contract works as it should, if that same address then tries to withdraw another amount from the contract, the transaction should revert … but what actually happens with your code? …Why?.. the answer will tell you what this missing line is that I keep mentioning…
Correct
Explicitly inheriting both Destroyable and Ownable, is not wrong *… but as you have correctly observed 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.
*but then it must be…
Bank is Ownable, Destroyable {...}
// not ...Destroyable, Ownable...
Your destruct function header throws a compiler error. Can you resolve it?
Apart from that, your Destructible contract is looking good, but it relies on you having made an additional change in Ownable. So can we see that contract as well?
We also need to see what modifications you’ve made to the start of your Bank.sol file, and Bank contract header, for the inheritance, because that’s the contract we are actually deploying, using, and then destroying, and which, therefore, needs to inherit both the contract ownership and contract destruction functionality. Once completed successfully, you should be able to deploy Bank, perform some transactions, then destroy the Bank contract, transferring any remaining ether balance to the caller of the selfdestruct function (here, the contract owner).
The use of the additional local variable receiver is not actually necessary. It was done like this to break down what is happening into more easier-to-understand steps (although this can obviously confuse some students more than it helps!) However, it does highlight that the address passed to selfdestruct() needs to be a payable address. Have a look at this post where you can read more about the use of this receiver variable. It is only marked payable because if we assign a payable address (e.g. msg.sender before v0.8) to a variable and don’t define the variable as payable, then it will be assigned and stored as a non-payable address.
I hope that makes sense — just let me know if you have any more questions
Here’s the complete example. I made it possible to call selfdestruct from outside the contract although Filip wanted it to only be available from derived contracts as I didn’t want to create one more function in the Bank contract in order to call self destruct.
pragma solidity 0.7.5;
contract Ownable {
address payable owner;
constructor() {
owner = msg.sender;
}
modifier onlyOwner {
require(msg.sender == owner);
_;
}
}
contract Destructible is Ownable {
function destruct() public onlyOwner {
selfdestruct(owner);
}
}
contract Bank is Destructible {
mapping(address => uint) balances;
event depositDone(address indexed depositedFrom, uint amount);
event withdrawalDone(address indexed withdrawnTo, uint amount);
function deposit() public payable returns (uint) {
balances[msg.sender] += msg.value;
emit depositDone(msg.sender, msg.value);
return balances[msg.sender];
}
function withdraw(uint amount) public returns (uint) {
require(balances[msg.sender] >= amount, "Not enough funds for withdrawal!");
uint originalAmount = balances[msg.sender];
msg.sender.transfer(amount);
balances[msg.sender] -= amount;
assert(balances[msg.sender] + amount == originalAmount);
emit withdrawalDone(msg.sender, amount);
return balances[msg.sender];
}
function getBalance() public view returns (uint) {
return balances[msg.sender];
}
}
pragma solidity 0.7.5;
contract Ownable{
address payable owner; /* declaring the owner variable as a payable address type so it can receive funds*/
modifier onlyOwner {
require(msg.sender == owner);
_;
}
constructor(){
owner = msg.sender;
}
}
Destroyable contract:
pragma solidity 0.7.5;
import "./Ownable.sol";
contract Destroyable is Ownable{
function destroy() public onlyOwner {
selfdestruct(owner); // sends funds to owner's address, contract's data is cleared freeing up space in the Ethereum blockchain
}
}
Bank Contract
pragma solidity 0.7.5;
import "./Destroyable.sol";
contract Bank is Destroyable{.....}
Giving your destruct() function public visibility is exactly what is required. This means that it will be inherited by Bank, and can be called from an external service. In order for the contract owner to be able to call destruct() and trigger selfdestruct() it needs to be available externally, because the contract owner (distinct from the contract itself) has an external address. If we gave destruct() internal visibility, then it could only be called from within the Bank contract, and as you say, that would require an additional public function, which we don’t want, as that would result in code duplication — one of key things inheritance should help us avoid.
Unlike state variables, there is no default visibility with functions, and so we must define this in all function headers. State variables, on the other hand have internal visibility by default (when no visibility is explicitly defined).
Well done for making sure your owner address is payable , so that selfdestruct() is called with a payable address and, therefore, any remaining ether in the contract is paid/transferred to the owner when selfdestruct() is triggered.
Your implementation of a multi-level inheritance structure is also the best approach, because it is more streamlined. Bank only needs to explicitly inherit Destroyable, because it will implicitly inherit Ownable via Destroyable.
Best practice would usually be to place each contract in a separate file and import each base contract into the file of its derived contract using an import statement. This keeps our code as modular as possible.
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.
You’re making great progress! Just let us know if you have any questions
Your comments are also accurate, and show that you have a good understanding of some of the finer points. And your implementation of a multi-level inheritance structure is also the best approach, because it is more streamlined: Bank only needs to explicitly inherit Destroyable, because it will implicitly inherit Ownable via Destroyable.
