Transfer Assignment

jon_m · January 7, 2022, 6:36pm

You have added all of the lines of code needed to solve the problem with the withdraw function and your statements are also in the correct order within the function body to reduce 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…

before actually transferring the funds out of the contract to the external address…

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) Have you also restricted access to the deposit() function and the transfer() function to the contract owner with the onlyOwner modifier? If you have, then also adding the onlyOwner modifier to the withdraw() function header makes sense, as there will be no other users with individual balances to withdraw. However, this would be a very limited Bank contract if there can only ever be one user, the contract owner, and would also mean we wouldn’t need the balance mapping to store multiple user balances.

Instead, the aim of our contract is to simulate a bank with bank account holders, with multiple addresses being able to deposit funds and make internal transfers between themselves. This means that we don’t want to restrict access to any of our functions to the contract owner, including the withdraw() function, because while the contract is still deployed it only seems fair that all users should be able to withdraw their funds

(2) Your withdraw event and corresponding emit statement are both coded correctly. The emit statement is in the correct position in the withdraw() function body and will log the withdrawal amount when the withdraw() function is successfully executed. Is the reason why the event is only logging the withdrawal amount, and not the withdrawer’s address, because only the contract owner can make withdrawals? If, for the reasons I’ve mentioned above, we allow multiple users to withdraw funds, you will want your withdraw event to also log the withdrawer’s address, in the same way that our deposit event logs both the amount deposited and the depositor’s address.

(3)

This line of code withdraws the wei amount from the contract’s ether balance and transfers it to the caller’s (msg.sender's) external address balance (showing in the Account field in the Deploy & Run Transactions panel in Remix). It’s important to understand that the caller’s (msg.sender's) individual balance stored in the mapping records and tracks their individual share of the total contract address balance (effectively, their entitlement to withdraw ether from the contract balance). The individual user’s balance in the mapping is adjusted to reflect changes in their share of the total contract balance, but isn’t actually where the ether is transferred out of the contract from.

Also, don’t forget to post your solution to the Events Assignment here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.

Let me know if anything is unclear, or if you have any questions about any of these points

jon_m · January 7, 2022, 7:23pm

This is an excellent solution @r0bz078

However, I couldn’t help noticing that it’s exactly the same (down to all of the identifiers used) as this student’s solution. Is this where you gained your inspiration? It’s a very good idea, and I very much encourage checking your own solutions and getting additional help and ideas from other students’ solutions, and the feedback they’ve received, posted here in the forum. However, you’ll benefit more from the course if you post your own solution, perhaps enhanced, or corrected, based on other solutions you’ve seen, even you think someone else’s is better. That way, you’ll receive valuable feedback about your own code.

I apologise, if it really is just a coincidence that both of these excellent and creative solutions are exactly the same, and you’ve both come up with the same ideas independently! Do let me know if you have.

Anyway, here’s a link to the feedback I gave the other student, because it also applies to the solution you’ve posted.

Also, don’t forget to post your solution to the Events Assignment here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.

Just let me know if you have any questions

firequo · January 9, 2022, 11:33pm

function withdraw(uint amount) public payable returns (uint){

require(amount <= balance[msg.sender]);

balance[msg.sender] -= amount;

msg.sender.transfer(amount);

return balance[msg.sender];

}

thanasornsawan · January 10, 2022, 9:50am

@jon_m thank you for your comment

I just know that .transfer(amount) help prevent re-entrancy attack from your comment.I will study more in the “smart contract security” course as you mention.
I didn’t restrict access to the deposit() function and the transfer() function to be only owner.
In the first time, I understand that anyone can transfer or deposit to our address but if withdraw should be only owner.That’s why I put onlyOwner at withdraw() function.

I just want to make function that look like normal wallet that have only one address per one owner.Because now I use address A deploy contract but I also can deposit to any accounts and withdraw from any accounts if that account has balance more than zero.

Thank you for help correct me that I understand wrong to put onlyOwner modifier on the withdraw function.

About withdraw event, I totally understand wrong after read your commend and try check debug “address from” and “address to” again in console.
When use msg.sender.transfer(amount); ,it is withdraw from any address that we call it and transfer amount to the balance of contract.

for example: In remix I use address ‘0x5B38Da6a701c568545dCfcB03FcB875f56beddC4’ to deploy contract and get the contract address ‘0x7EF2e0048f5bAeDe046f6BF797943daF4ED8CB47’ and then I try withdraw from address ‘0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2’.It should be refund back to the address ‘0x5B38…dC4’ but actually when withdraw from any address it will always refund money to the contract address ‘0x7EF…B47’.
I understand concept of msg.sender wrong.That’s why I put only amount because we know msg.sender is this person but really thank you for your comment.I will put more detail about withdraw from which address in the log too.

event withdrawSuccess (address indexed withdrawFrom, uint money);

function withdraw(uint amount) public returns (uint){

       //deposit 1 ether and withdraw 10 wei

        require(balance[msg.sender] >= amount, "Your balance is insufficient!!");

        balance[msg.sender] =  balance[msg.sender] - amount;

        msg.sender.transfer(amount); //withdraw from sender in wei

        //msg.sender can be anyone that we call to withdraw money.

        //the amount of money will always return to the balnce of contact address

        emit withdrawSuccess(msg.sender, amount);

        return balance[msg.sender];

    }

jon_m · January 11, 2022, 12:53am

Hi @thanasornsawan,

Ok … so, your modified withdraw() function without the onlyOwner modifier is now much better This is because any address can now call the withdraw() function and be the msg.sender, and withdraw any ether from the contract to which they are entitled. The ether an address is entitled to withdraw will be their individual balance recorded in the mapping (mapped to their address), which they will have if they have either (i) called the deposit() function and transferred ether into the contract, or (ii) been the recipient of an internal transfer made by another address calling the transfer() function.

Your withdraw event declaration, and corresponding emit statement, have also been correctly updated and will now log the withdrawer’s address as well as the withdrawal amount

Below are a few corrections to some of your comments. It may be that you do understand correctly, but have just used the wrong words in a few places. Apologies, if this is the case, but I thought it best to just make sure …

This line of code deducts the wei amount FROM the contract address’s ether balance and transfers and adds it TO the msg.sender's external address balance (the address showing in the Account field in the Deploy & Run Transactions panel in Remix).
The withdrawal amount, which is deducted from the contract address’s ether balance, is also deducted from msg.sender's individual balance stored in the mapping. This is to correctly adjust their individual share of the total contract address balance for the amount they’ve withdrawn.

As you know, the individual balances stored in the mapping can be retrieved by calling getBalance(). You can retrieve the total amount of ether stored in the contract (the contract address balance) by adding the following function to your contract …

function getContractBalance() public view returns(uint) {
    return address(this).balance;
}

After the withdraw() function is called, if you call getContractBalance() you will see that the contract ether balance has reduced by the withdrawal amount. The address which called the withdraw() function will have the same withdrawal amount added to their external address balance (showing in the Account field in the Deploy & Run Transactions panel in Remix).

As a result of what I’ve just explained, have a look at the following changes (in capital letters) that I’ve made to a couple of the comments you’ve added to your code …

thanasornsawan:

msg.sender.transfer(amount); //withdraw from CONTRACT ADDRESS in wei

//msg.sender can be anyone that we call to withdraw ETH/WEI.

/* the amount of ETH/WEI will always return to the MSG.SENDER'S EXTERNAL 
   ADDRESS BALANCE (SHOWING IN THE ACCOUNT FIELD IN THE REMIX PANEL).*/

Now, in terms of your example …

… here, I understand that address 0xAb8…cb2 already has a positive individual balance in the mapping, and is now calling the withdraw() function to withdraw an amount less than or equal to their individual balance in the mapping.

No … it shouldn’t be transferred to the address that deployed the contract (0x5B3…dC4) or to the smart contract address (0x7EF…B47). It should be transferred out of the smart contract (deducted from 0x7EF…B47) and added to the external account balance of 0xAb8…cb2, the address that called the withdraw() function (the msg.sender).

I think you might find the following two posts helpful …

This post lays out step-by-step instructions of how to properly check that all of your functions are working correctly, and it details what you should see happening and where, if it is.
This post contains some useful information about what specific operations each of the different functions (deposit, withdraw and transfer) are performing, and how the different balances are affected (or not affected) by each.

Anyway, I hope this is helpful. Just let me know if anything is unclear, or if you have any questions

jon_m · January 11, 2022, 2:47am

Hi @firequo,

You have added all of the lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body to reduce 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…

balance[msg.sender] -= amount;

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

However, we should not mark the withdraw function as payable, because it doesn’t need to receive ether from an external address in order to update the contract balance. If we make it payable, then, if the caller sends an ether value to the withdraw function by mistake (as well as calling it with a withdrawal amount) then this ether would be added to the contract balance and potentially lost to the user: this is because the withdraw function does not make an equivalent accounting adjustment to the mapping for such a deposit (only for withdrawals). However, the function is much securer if we leave it as non-payable, because then, if the caller makes the same mistake, the function will revert, and the ether won’t be sent.

A function only needs to be marked payable when it receives ether from an external address to add to the contract address balance. It might help to think of payable as the code that enables msg.value to be added to the contract address balance, without us having to add any further code for this. In the deposit function, this happens automatically because we have marked the function as payable. The line balance[msg.sender] += msg.value; then adds the same value to the individual user’s balance in the mapping, in order to keep track of their share of the total funds held in the contract.

In contrast, the withdraw function is deducting ether from the contract balance and sending it to an external address. It does this using the address member transfer (which is like a method called on a payable address)…

<address payable>.transfer(uint amount);

And don’t forget to post your solution to the Events Assignment here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.

Let me know if you have any questions

c1cag1b3 · January 11, 2022, 3:26pm

Hi Jon!

Thank you so much for your assistance!
Here are the changes I made:

function withdraw(uint amount) public returns (uint){

    require(balance[msg.sender] >= amount, "Balance not sufficient");

    balance[msg.sender] -= amount;

    msg.sender.transfer(amount);

    return balance[msg.sender];

}

function getBalance() public view returns (uint) {

        return balance[msg.sender];

}

function transfer(address recipient, uint amount) public {

    require(balance[msg.sender] >= amount, "Balance not sufficient");

    require(msg.sender != recipient, "Dont transfer money to yourself");

    uint previousSenderBalance = balance[msg.sender];

    _transfer(msg.sender, recipient, amount);

   

    assert(balance[msg.sender] == previousSenderBalance - amount);

    emit balanceTransfer(msg.sender, recipient, amount);

    }  

    function _transfer(address from, address to, uint amount) private {

    balance[from] -= amount;

    balance[to] += amount;

    }

}

Hope I got everything correct

jon_m · January 11, 2022, 6:45pm

Yes … that’s all correct now

fedev · January 12, 2022, 5:12am

Hey! This is my solution.
If the msg sender is trying to withdraw more than what he has in the balance, he will still be able to withdraw 100% of what the bank owes him. It raises just if he has an empty balance.

pragma solidity 0.7.5;

contract SimpleBank {

  mapping(address => uint) balances;
  address owner;

  constructor(){
    owner = msg.sender;
  }

  modifier onlyOwner {
    require(msg.sender == owner, "Owner only");
    _;
  }

  /***
  *  Events
  */

  event balanceChanged(
    address indexed caller, // can be queried later
    address indexed target, // can be queried later
    uint amount,
    bool increment // true == add, false == remove
  );
 
  event depositReceived(
    address indexed sender,
    uint amount
  );

  event withdrawalProcessed(
    address indexed recipient,
    uint amount
  );


  /**
  *  Getters
  */

  function getSenderBalance() public view
    returns(uint){
    return balances[msg.sender];
  }

  function getBalance(address addr) public view
    returns(uint){
    return balances[addr];
  }


  /**
  *  Single balance modifiers
  */

  function _removeFromBalance(address recipient, uint amount) private
    returns(uint){
    if  (amount > balances[recipient]){
      uint amt = balances[recipient];
      balances[recipient] = 0;
      emit balanceChanged(msg.sender, recipient, amt, false);
      return amt;
    } else {
      balances[recipient] -= amount;
      emit balanceChanged(msg.sender, recipient, amount, false);
      return amount;
    }
  }

  function _addToBalance(address recipient, uint amount) private
    returns(uint){
    balances[recipient] += amount;
    emit balanceChanged(msg.sender, recipient, amount, true);
    return amount;
  }

  function deposit() public payable
    returns(uint){
    emit depositReceived(msg.sender, msg.value);
    uint deposited = _addToBalance(msg.sender, msg.value);
    return balances[msg.sender];
  }

  function withdraw(uint amount) public
    returns(uint){
    require(amount > 0, "Amount too small");
    uint senderPreBalance = balances[msg.sender];
    require(senderPreBalance > 0, "Balance empty");
    amount = _removeFromBalance(msg.sender, amount);
    msg.sender.transfer(amount);
    emit withdrawalProcessed(msg.sender, amount);
    return balances[msg.sender];
  }


  /**
  * From - to
  */

  function _transfer(address from, address to, uint amount) private
  {
    balances[from] -= amount;
    emit balanceChanged(msg.sender, from, amount, false);
    balances[to] += amount;
    emit balanceChanged(msg.sender, to, amount, true);
  }

  function transfer(address recipient, uint amount) public
  {
    require(amount > 0, "No 0 amount transfer");
    require(msg.sender != recipient, "No self transfer");
    require(balances[msg.sender] >= amount, "Insufficient balance");

    uint preSenderBalance = balances[msg.sender];
    uint preRecipientBalance = balances[recipient];

    _transfer(msg.sender, recipient, amount);

    assert(balances[msg.sender] == preSenderBalance - amount);
    assert(balances[recipient] == preRecipientBalance + amount);
  }
}

JayM700 · January 13, 2022, 6:29am

    event withdrawComplete(uint amount, address indexed withdrawnFrom);

    function withdraw(uint amount) public returns (uint) {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        
        uint previousSenderBalance = balance[msg.sender];

        balance[msg.sender] -= amount;
        
        msg.sender.transfer(amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);

        emit withdrawComplete(amount, msg.sender);

        return balance[msg.sender];
    }

casadcode · January 12, 2022, 6:50pm

Seems to work through testing, anyone have any suggestions on where to insert emit for event logging THANKS

Cosmin_Fechet · January 13, 2022, 1:27am

function withdraw(uint amount) public returns (uint){
        require(amount >= balance[msg.sender]);
        msg.sender.transfer(amount);
        uint previousBalance = balance[msg.sender];
        assert(balance[msg.sender] == previousBalance - amount);
    }

jon_m · January 13, 2022, 4:39am

Nice solution @JayM700

You have added all of the additional lines of code needed to solve the problem with the withdraw function.

Your withdraw event and corresponding emit statement are also both well coded. The emit statement is in the correct position in the withdraw() function body and will log appropriate data when the withdraw() function is successfully executed.

The additional two lines you’ve included for the assert() statement are also well coded.

Now have a look at this post which explains an important security modification that should be made to the order of two of the statements within your withdraw function body.

Just let me know if you have any questions

jon_m · January 13, 2022, 5:22am

Hi @casadcode,

The require statement and the return statement you’ve added to your withdraw function are both correct, and the additional two lines you’ve included for the assert() statement are also well coded

But you are missing one very important line of code …

If you deploy your contract, make a deposit, and then call your withdraw function, you will notice that the caller’s address receives the requested withdrawal amount, but their balance in the mapping is not reduced (call getBalance to check this). I’m sure you can see that this is a very serious bug because, even though there is a limit on each separate withdrawal, this limit is never reduced to reflect each withdrawal. This means that a user can withdraw more than their own individual balance (the share of the total contract balance to which they are entitled), and effectively steal other users’ funds.

msg.sender.transfer(amount) transfers ether from the contract address balance to the caller’s external wallet address, but it doesn’t adjust the individual user balances in the mapping. These balances perform an internal accounting role, and record each user’s share of the contract’s total ether balance (effectively, each user’s entitlement to withdraw ether from the contract). So, just as we increase a user’s individual balance when they deposit ether in the contract, we need to do the opposite whenever they make a withdrawal.

Once you’ve had a go at correcting your code for this, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

After completing this assignment, have a really good think about why your testing didn’t pick up on the missing line of important code discussed above. Think about how you can make your testing more thorough, so that it identifies this kind of issue

An emit statement should be placed at the end of a function, after the operations associated with the event itself (which it is logging data for) has 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.

Don’t forget to do the Events Assignment and post your solution here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course, and it will help you to practise coding event declarations, emit statements, and where to position them.

And finally, a friendly reminder not to post screen shots, because we can’t copy and paste your code in order to execute and test it, if we need to. Instead, follow the instructions in this FAQ: How to post code in the forum

Let me know if you have any questions about any of these points

jon_m · January 13, 2022, 6:41am

Hi @Cosmin_Fechet,

I think you need to slow down a bit, and make sure you test your code to see if it actually works and does what it should

There are lots of issues with this solution. I’ll point you in the right direction, and then see if you can correct it yourself

(1) The condition in your require() statement does the opposite of what it should. We want to make sure a user can’t withdraw more than their individual balance, not force them to only withdraw more than their share!

(2) Once you’ve corrected the require() statement, you will still find that every time you call the withdraw() function, assert() fails and triggers revert. This is because the condition in your assert() statement can never be true. Can you see why?

Once, you can see the problem, comment out these last two lines, and continue with point 3. We’ll come back and correct these two lines of code associated with assert() at the end, because you need to make another important modification first.

(3) Your withdraw function is missing an essential line of code …

If you now deploy your contract (with just the two statements you should currently have in your withdraw function body), make a deposit, and then call your withdraw() function, you will notice that the caller’s address receives the requested withdrawal amount, but their balance in the mapping is not reduced (call getBalance to check this). I’m sure you can see that this is a very serious bug because, even though there is a limit on each separate withdrawal, this limit is never reduced to reflect each withdrawal. This means that a user can withdraw more than their own individual balance (the share of the total contract balance to which they are entitled), and effectively steal other users’ funds.

msg.sender.transfer(amount) transfers ether from the contract address balance to the caller’s external wallet address, but it doesn’t adjust the individual user balances in the mapping. These balances perform an internal accounting role, and record each user’s share of the contract’s total ether balance (effectively, each user’s entitlement to withdraw ether from the contract). So, just as we increase a user’s individual balance when they deposit ether in the contract, we need to do the opposite whenever they make a withdrawal.

(4) Notice that the withdraw function header also includes returns(uint) . This is not mandatory to include, and the function can still operate effectively without returning a value. But as it’s been included in the function header, you should also include a return statement in the function body. The compiler will have given you an orange warning about this.

(5) Now let’s return to the two lines of code I asked you to comment out. Your local variable previousBalance was saving the user’s balance after the transfer, not before. This didn’t make any difference because you hadn’t included the essential line of code (mentioned above) needed to reduce this balance for the withdrawal amount. However, after adding this essential line of code (in point 3) you’ll now need to reposition this local variable declaration and assignment, so that the assert() statement checks an invariance that should always be true, instead of a condition that can never be true

(6) Once you’ve made all of the above changes and tested your withdraw() function to ensure it is working as it should, have a look at this post which explains the importance of the order of two of the statements that you should now have in your withdraw() function body. Make sure you have these statements in the right order to guard against re-entrancy attacks and keep your smart contract secure.

Let me know if anything is unclear, if you have any questions, or if you need any help in correcting your withdraw() function

jon_m · January 15, 2022, 3:37am

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!

fedev · January 15, 2022, 8:54am

Hi @jon_m what a in-depth review, thank you so much, I really appreciate!
Regarding your comments:

Yes you’re right, it might be confusing and has definitely its drawbacks (smart contracts which depends upon on that must absolutely take this behaviour in consideration). But this can protect greedy and lazy users (as I am) from bad frontends. Have you ever had to replay (and sadly… rePAY) a transaction just because the UI of the dapp wasn’t rounding properly? That sucks and does really hurt when fees are as high as they’re now on ethereum

No excuses, you’re 100% right. The private helpers should have been used in transfer too.

Yeah I saw it, but as I said I’m kind of lazy and I just let that… “just in case I need it for some reason…”
Obviously not for-prod behaviour

You’re right, I had better understood this (and coded accordingly) in the MultiSig final project in which I’ve always put assertions in the helpers and emitted signals at the end of the public function.
I still have to get used to signals, as I still think them more as log functions, when I should threat them as costly signals for other processes. And this require clear vision/planning of what I would expect to integrate/need from the SC itself. It’s a good exercise.

Yes I definetly have to get used to think about gas!

Your comments are really helpful and I would be super happy if you could review the Multi Sig final project with the same spirit: I’m trying to learn as much as I can!

Have a nice day

jon_m · January 18, 2022, 12:41am

Hey Federico,

Glad you found the comments helpful. What’s good is that, I can see from your reply, it got you thinking and reflecting further on things that you’d already come across yourself. Personally, I’ve always found this reflection, re-working and re-factoring stage really profitable, especially if you’ve already invested so much effort into the original solution, as you obviously do.

I will gladly take a look at your Multisig project and provide you with some feedback. I will try to do that sometime over the next few days

Point 1

Good point However, when you build a dapp, I do think the best solution for this would be to handle any rounding issues in the front-end before the actual withdraw() or transfer() function is called. The user’s current balance could be retrieved first by calling the getter (no gas cost for calling a getter), and then performing or alerting the user to any roundings or necessary adjustments to the actual amount they have requested to withdraw or transfer. Once these issues have been addressed (based on current information retrieved from the smart contract), the user can then have an option to accept or reject the adjusted/rounded amount (generated in the front-end). If they accept, then only at this point would the function in the smart contract, which sends the transaction to the network, automatically be called. The user experience would be that of performing just one single operation/transaction, but with a few additional steps in the UI if the original amount they request needs rounding or adjusting. The smart contract function would still contain the necessary require statements as further validation checks, but overall this approach should address the important issue you’ve raised, whilst also keeping gas costs down.

fedev · January 17, 2022, 4:37pm

Hey! You’re definitely right. Also while attending the 201 course and coming across the EIP20 proposal I’ve found that both txs with amount > balance should throw AND txs with amount == 0 should not throw and emit standard txs.
I will obviously stick to the standards even if the second clause looks a bit strange/waste of gas but maybe there was a reason at that time to ask complaiance to not throw on transfer 0 amounts.

diehlca17 · January 21, 2022, 3:23am

This problem can be easily fixed by setting a requirement that the balance is larger than the amount we are withdrawing. It returns the balance after the transfer.

   function withdraw(uint amount) public returns (uint) {
        require(balance[msg.sender] >= amount, "cannot withdraw more than your balance");
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }