Hi Federico,
Apologies for the delay in giving you some feedback on this assignment.
Your solution is excellent, and I really like how youâve adapted and been creative with the code, experimenting with your own variations 
Youâve solved the problem with the withdraw function (adding your own adaptation â see my comments below), and the following operations are performed in the correct order within the control flow, which reduces security risk âŠ
- Check inputs (require statements)
- Effects (update the contract state)
- External Interactions (e.g.
transfer method)
Just as you have done, itâs important to modify the contract state for the reduction in the individual userâs balanceâŠ
// if there is sufficient balance to withdraw the whole amount requested
balances[recipient] -= amount;
// if the requested amount is greater than the user's individual balance
balances[recipient] = 0;
before actually transferring the funds out of the contract to the external addressâŠ
msg.sender.transfer(amount);
This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the userâs individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. Youâll learn more about this type of attack, and how the above order of operations helps to prevent it, in the courses which follow this one. But itâs great youâre already getting into good habits in terms of smart contract security 
Some additional comments âŠ
(1)âStill allowing a withdrawal to go ahead if the amount requested by a user is greater than their individual balance, but reducing the amount actually withdrawn to the total balance the user has available, is an interesting variation. Youâve implemented this very well, and the require() statements that youâve included instead are appropriate.
However, do you think that this would be appropriate in practice? When someone requests the withdrawal of a specific amount, donât you think they would rather have the transaction stopped, and be notified that they donât have sufficient funds, rather than finding out afterwards that the amount theyâve received is less than expected? The transaction can be reverted and the error handled in the front-end user interface in such a way that the user is fully aware of the circumstances, and is given the opportunity to repeat the withdrawal but for a lower amount.
(2)âAdding the helper functions _addToBalance() and _removeFromBalance() is a nice implementation of modularity: breaking down a computation and separating out specific chunks of code into their own specialised functions, where they can be reused elsewhere. However, youâve missed the opportunity to re-use both of these functions in the transfer() function to perform the necessary adjustments to the individual balances of the sender and recipient. Instead of adding another helper function (_transfer), you can call both of the helper functions you already have âŠ
//_transfer(msg.sender, recipient, amount);
amount = _removeFromBalance(msg.sender, amount);
_addToBalance(recipient, amount);
As the _removeFromBalance() function would no longer only be used for withdrawals, you would also need to think of a less specific name for the address parameter than recipient, so that it makes sense for all use cases. However, your if/else logic could still be applied to internal transfers initiated by the transfer() function, allowing them to still go ahead if the transfer amount requested is greater than the callerâs individual balance. Just like with withdrawals, in these cases the amount actually transferred could also be reduced to the total balance available to the caller.
(3)âIn your deposit() function, you donât need to assign the value returned from the _addToBalance function call to a local variable, because this value isnât used and will just be lost anyway when the function finishes executing.
You should have got an orange compiler warning for this, notifying you of an unused local variable.
The _addToBalance() helper function doesnât need to return any value, and the function call can be made as followsâŠ
function _addToBalance(address recipient, uint amount) private {
balances[recipient] += amount;
}
function deposit() public payable returns(uint) {
_addToBalance(msg.sender, msg.value);
emit depositReceived(msg.sender, msg.value);
return balances[msg.sender];
}
On the other hand, your _removeFromBalance() helper function does need to return a value, because the amount may need to be changed to the callerâs maximum balance available in the if-statement. Therefore, itâs also correct to assign the return value to the amount parameter in the main function that calls this helper âŠ
Notice in point 2, above, that this line of code could be used in both the withdraw() and the transfer() function.
(4)âAll 3 event declarations and their corresponding emit statements are well coded, and the emit statements log appropriate data when their respective functions are successfully executed.
Where possible, an emit statement should be placed at the end of a function, after all of the operations associated with the event itself have occurred, but before a return statement if there is one. Generally speaking, itâs probably also better to place an emit statement after an assert statement if there is one.
So, bearing this in mind, your emit statement in the withdraw() function is in the correct position, but you should reconsider where you place the one in your deposit() function.
Do you think itâs necessary to emit 2 events for each deposit and each withdrawal? Personally, I think itâs an unnecessary duplication of data, and that you could safely remove the balanceChanged event and its corresponding emit statements in the helper functions. You already have sufficient data logged by the depositReceived and the withdrawalProcessed events. We know that the amount associated with a depositReceived event will be an increase to the senderâs individual balance, and that associated with a withdrawalProcessed event will be a reduction in the recipientâs individual balance, without needing the Boolean values to be logged in a separate balanceChanged event.
Removing the balanceChanged event and adding a separate event only for the transfer() function, means that youâll only need one event emitted at the end of this function as well (which is the most appropriate place for an event, anyway). At the same time, the transfer() function will still be able to make use of the two helper functions to adjust the individual user balances accordingly.
(5)âIf you need a getter to return a userâs individual balance to anyone, then you no longer need the getter which only returns the callerâs own balance, because the other getter can now be used for both. Remember that the more code a contract contains, the higher the gas cost will be to deploy it. So, we canât afford to be wasteful by including redundant code.
I hope you find these comments helpful. Let me know if you have any questions, and please do let me know if you disagree with any of my suggestions.
Youâre making great progress with Solidity, Federico 
Keep it up!
Seems to work through testing, anyone have any suggestions on where to insert emit for event logging THANKS