Hi Jon,
I do hugely appreciate your answer!
Now have a look at this post which explains an important security modification that should be made to the order of the statements within your withdraw function body.
Now that you mentioned it and after reading your post I took a moment to think about it and it makes a lot of sense.
Your withdraw event and corresponding emit statement are both well coded, and the emit statement will log relevant information when the function has executed successfully. I would just add, though, that in general, emit statements are probably better placed after assert statements, rather than before.
Same here, I now have a much more clear statements order in mind.
Note that this statement is returning / outputting the user’s updated balance from the function, not updating it. You have already correctly identified the statement which updates the user’s individual balance …
My apologies, it was actually a typo, I meant to write “updateD”
Question: in other object-oriented languages (Python, C++, etc.) I have the habit of including (almost) always a return statement at the end of every function/method in order to maintain a solid I/O flow structure and for future utility, regardless if I already need it: is that a bad practice in solidity? For example, does it make the contract less clean or more expensive to deploy/run?
Did you mean to include the addBalance() function as well as the deposit() function, or did you leave this in by mistake? If the owner can increase their own individual balance by any amount they wish, without depositing an equivalent amount of ether into the contract, then this effectively means that they can withdraw other users’ funds! I’m sure you agree that this wouldn’t inspire much user confidence in the contract
Forgot to remove it from the code (I started from a recycled a piece of code I wrote while following the lessons, sorry about that).
Here is the amended code with the corrections we discussed:
pragma solidity ^0.8.7;
contract Bank{
// state variables
mapping (address=>uint) balance;
address owner;
// constructor
constructor(){
owner = msg.sender;
}
// modifiers
modifier onlyOwner {
require(msg.sender == owner);
_;
}
// events
event depositDone(uint indexed amount, address indexed depositedTo); //deposit
event withdrawDone(uint amount, address withdrawnFrom); //withdraw
// functions
function deposit() public payable 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(amount<=balance[msg.sender], "The amount requested to withdraw exceeds the available balance");
uint balanceBefore = balance[msg.sender]; //store original balance for later safety checks
balance[msg.sender]-=amount; //update balance, BEFORE actually transfering ETH in order to avoid attacks
payable(msg.sender).transfer(amount); //transfer ether
assert(balanceBefore - amount == getBalance()); // ensure that the actual balance corresponds to the operation performed
emit withdrawDone(amount, msg.sender); //withdraw event logging
return balance[msg.sender]; //updateD balance;
}
function getBalance() public view returns (uint){
return balance[msg.sender];
}
}
Many thanks for taking time to carefully review my code and answering to all of my questions!