Transfer Assignment

[jon_m]

Hi @vale_dinap,

You’re very welcome, and I’m glad you found my feedback helpful

You’ve made a good job of amending your contract

@vale_dinap

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?

This is a very good question, and also an important one to consider when coding Ethereum smart contracts in Solidity, because usually additional code means more low level operations and therefore higher transaction costs in terms of gas consumption. Obviously security, utility and functionality should not be compromised just to save gas; however, generally speaking, transaction costs will be lower if we only add the code that is necessary, and aim to code as efficiently and concisely as possible. Avoiding redundant code can also help to reduce the risk of bugs.

It’s perfectly acceptable for Solidity functions not to return any values; and if they don’t need to, then it’s better practice that they don’t, for the reasons I’ve just outlined above. The possible future utility of a piece of code also isn’t really a factor with smart contracts, because once they are deployed they cannot be modified. So, any code that is redundant when the contract is deployed is likely to remain redundant.

Our Bank contract, as it is, doesn’t actually need the deposit or withdraw functions to return the updated balances, and so the return statements in the function bodies, and  returns(uint)  in the function headers, can be removed. We already have the getBalance function which can be called separately to consult the individual account holder’s updated balance after any transaction (a deposit, withdrawal or transfer); and getBalance is a view function, which doesn’t generate a transaction, and so doesn’t consume any gas.

You can get an indication of the reduction in gas costs by deploying your Bank contract in Remix and then making a deposit and a withdrawal (i) with the code in both functions that returns the updated balances, and (ii) after removing this code from both functions. After each transaction (contract deployment, deposit, withdrawal) you can see the associated gas cost within the transaction receipt generated in the Remix console.

I say that it’s an indication of the gas cost, because I’m aware that these gas costs in Remix may not be entirely accurate. You’ll notice that Remix generates two gas cost figures: transaction cost and execution cost, and I still haven’t entirely got to the bottom of what the difference is, and whether there is any double counting or other aspects that need to be taken into consideration. These figues used to be different amounts, but I’ve now noticed they are the same! It’s also possible to click on “Debug” next to a transaction receipt, which will take you to the Debugger in the panel on the left. Here you can click through each of the separate low-level operations performed in that transaction and see the gas consumed for each one. I’ve used that before to identify the gas consumption for each individual, low-level operation performed for a specific chunk of code. By adding those up, I’ve tried to arrive at the total gas consumed for alternative chunks of code, in order to compare how much gas they use. But, as I’m sure you can imagine, this gets quite time consuming!

A more straight forward way to arrive at the gas consumption is to deploy the contract using Truffle and Ganache, and then perform various transactions using the Truffle console. You will learn how to use these tools in the 201 course, which follows this one.

Anyway, for now, the gas cost figures in Remix will at least prove to you that returning the updated balance increases the gas cost of all 3 transactions: contract deployment, deposit and withdrawal. The gas cost of deployment will be a one-off cost, whereas the gas cost associated with each function will be incurred each time they are executed.

Just let me know if you have any further questions.

[mareng91]

function withdraw(uint amount) public returns (uint) {
//msg.sender is an address
require(balance [msg.sender] == amount, “Insufficent funds”);
msg.sender.transfer(amount);
}

I used the require function to make sure the adress held enough funds for the withdraw.

[mareng91]

function withdraw(uint amount) public returns (uint) {
//msg.sender is an address
require(balance [msg.sender] >= amount, “Insufficent funds”);
msg.sender.transfer(amount);
}

I changed the == to >= so the balance can be more than amount.
not so good if it has to be equal to

[jon_m]

An excellent solution @CryptoByNight

You have added both of the essential 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:

1. check inputs (require statements)
2. effects (update the contract state)
3. external interactions

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);

… to prevent a malicious fallback function (in a receiving contract) making successful re-entrant calls to the withdraw() function, thereby allowing it to execute multiple withdrawals before its individual account balance is modified in the contract state to accurately reflect these operations (which would otherwise enable the initial require statement to fail and trigger revert).

@CryptoByNight

I thought the .transfer function was a potential security risk.

It’s true that the latest advice is not to use the address members transfer() and send() , but for a different reason. The fixed stipend of 2300 gas, which they forward to a calling contract’s fallback function, used to be enough gas for the fallback function to execute successfully, but not enough gas to allow a re-entrant call (thereby reducing the risk of re-entrancy attacks). But since the Istanbul hard fork changed the gas charges for certain operations, this fixed gas stipend is no longer guaranteed to be sufficient.

Instead of transfer() you can use the address member call() to perform the transfer of ether from the contract address balance to the external address …

(bool success, ) = msg.sender.call{value: amount}("");
require(success, "Transfer failed");

Using call() would normally pose a greater risk of re-entrancy attacks than using transfer() or send() , because it will forward all of the remaining gas (up to the gas limit set by the caller) to the fallback function. However, implementing the Checks-Effects-Interactions Pattern will guard against this, as I’ve described above.

You can read more about these issues in the following article:
https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

@CryptoByNight

Are safer ways for practical smart contract deployment explained in future sections?

You’ll learn more about re-entrancy and other types of attacks, and some of coding techniques and smart-contract best practices we can implement to guard against them and reduce security risk, in the courses which follow this one: Ethereum Smart Contract Programming 201, and the Ethereum Smart Contract Security course.

Just let me know if you have any further questions.

[jon_m]

Hi @mareng91,

The require statement you’ve added is correct, but your withdraw function is missing an essential line of code …

If you deploy your contract, make a deposit, and then call your withdraw function as it is, you will notice that the caller’s external address receives the requested withdrawal amount, but their balance in the mapping is not reduced! This means that they can make repeat withdrawals — each one no greater than their initial balance, which isn’t being reduced — up to the total amount of ether held in the contract (the sum of all of the individual account holders’ balances). So, I’m sure you can see why this is a very serious bug!

msg.sender.transfer(amount) transfers ether from the contract address balance to the caller’s external 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. 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.

---

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 a yellow/orange warning about this.

---

Once you’ve made the above modifications to your code, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

---

Please, format your code before posting it. This will make it clearer and easier to read. It will also make it easier to copy and paste if we need to deploy and test it.
Follow the instructions in this FAQ: How to post code in the forum

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

[unwise_guy]

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

[mareng91]

Thanks for the feedback.
i’ve changed the withdraw function.

I haven’t worked with coding before so i appreciate all your inputs

[mareng91]

    function withdraw(uint amount) public returns (uint) {
  
  require(balance [msg.sender] >= amount, "Insufficent funds");
  balance[msg.sender] -= amount;
  msg.sender.transfer(amount);
  return amount;
}`Preformatted text`

[jon_m]

An excellent solution @unwise_guy

You have added both of the essential 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:

1. check inputs (require statements)
2. effects (update the contract state)
3. external interactions

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);

… just in case there is an attack after the transfer, but before the state is modified to reflect this operation. You’ll learn about the type of attack this prevents, and how it does 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

Keep up the great coding!

[jon_m]

That’s looking good now @mareng91

I’m glad you found the feedback helpful

Just one final comment …

@mareng91

return amount;

This return statement is correct, and your function does now return a uint value, but do you think returning the same value as the one input into the function is useful? Normally, the output is the result of the computation performed by the function. The result of depositing ether (the input amount) into the contract is the user’s new balance. A user’s individual balance is tracked and stored in the mapping. Can you work out how to reference the user’s updated balance from the mapping, and return that instead? If you need some help, you can always have a look at some of the other students’ solutions posted here in this discussion topic.

Just let me know if you have any questions about this, or any other matter related to the course or the assignments. I know it can be a lot to absorb if you are new to coding
By the way, have you already done the JavaScript Programming course? If you are new to coding, then you should definitely do that course before this one, because it also covers the fundamentals of programming in general. This Solidity course assumes that you are already comfortable with those fundamentals, and builds upon that.

[mareng91]

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

  require(balance [msg.sender] >= amount, "Insufficent funds");
  balance[msg.sender] -= amount;
  msg.sender.transfer(amount);
  return balance[msg.sender];
}

[mareng91]

I do this on the evenings and construction work on the days + family time with 2 kids.
not an excuse But i don’t get so many hours a week, for now …
I have done the javascript course, but i realise that i might have to backtrack at some point , because i forgot some of it

I really appreciate your feedback , it makes me more avare of the implications of the code i write

[jon_m]

You’re very welcome Marcus!

Your modified return statement is now much more useful

@mareng91

I do this on the evenings and construction work on the days + family time with 2 kids.
not an excuse But i don’t get so many hours a week, for now …

That’s actually a pretty understandable excuse! … yeh, that definitely makes learning to program smart contracts doubly challenging. Just remember, doing a little but often will be more productive than trying to do too much but infrequently.

That’s good that you’ve done the JavaScript course, and so you’ll definitely be OK to continue with this course. When you’re a beginner, lots of repetition and practice is important and not trying to cover too much too quickly. Going back over the JavaScript course will definitely be helpful. Experimenting with the code from lectures and assignments yourself can also be a very productive learning method. You can try coding as much yourself from memory as possible, and then start reworking, adapting and personalising the same syntax and patterns so that you end up with your own variations on the same theme. When you’re going back over the code, and experimenting with it, really try to actively think about what it’s doing. Also, have a good read of some of the other posts in these forum discussions (for additional information, clarification, confirmation and inspiration), and then ask questions if anything is still unclear.

[zlr1984]

require(balance [msg.sender] >= amount, “not enough funds”);
msg.sender.transfer (amount);

[jon_m]

Hi @zlr1984,

The require statement you’ve added is correct, but your withdraw function is missing an essential line of code …

If you deploy your contract, make a deposit, and then call your withdraw function as it is, you will notice that the caller’s external address receives the requested withdrawal amount, but their balance in the mapping is not reduced! This means that they can make repeat withdrawals — each one no greater than their initial balance, which isn’t being reduced — up to the total amount of ether held in the contract (the sum of all of the individual account holders’ balances). So, I’m sure you can see why this is a very serious bug!

msg.sender.transfer(amount)   transfers ether from the contract address balance to the caller’s external 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. 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.

---

Notice that the original withdraw function header for this assignment contains  returns(uint) . Did you remove it from your solution? It’s not mandatory to include, and the function can still operate effectively without returning a value. But if you’ve included it in the function header, you should also include a return statement in the function body. The compiler would give you a yellow/orange warning about this.

Once you’ve made the above modifications to your code, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

Please, format your code before posting it. This will make it clearer and easier to read. It will also make it easier to copy and paste if we need to deploy and test it.
Follow the instructions in this FAQ: How to post code in the forum

And post your full withdraw function, so we can see whether it is returning a value or not.

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

[zlr1984]

Thanks. I will check that.

[josejalapeno]

function withdraw(uint amount) public returns(uint){
require(balance[msg.sender] >= amount, “Not Enough Funds”);
msg.sender.transfer(amount);

[jon_m]

Hi @josejalapeno

The require statement you’ve added is correct, but your withdraw function is missing an essential line of code …

If you deploy your contract, make a deposit, and then call your withdraw function as it is, you will notice that the caller’s external address receives the requested withdrawal amount, but their balance in the mapping is not reduced! This means that they can make repeat withdrawals — each one no greater than their initial balance, which isn’t being reduced — up to the total amount of ether held in the contract (the sum of all of the individual account holders’ balances). So, I’m sure you can see why this is a very serious bug!

msg.sender.transfer(amount) transfers ether from the contract address balance to the caller’s external 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. 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.

---

Notice that the withdraw function header contains returns(uint). It’s not mandatory to include, and the function can still operate effectively without returning a value. But if you’ve included it in the function header, you should also include a return statement in the function body. The compiler will have given you a yellow/orange warning about this.

Once you’ve made the above modifications to your code, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

And don’t forget to format your code before posting it. This will make it clearer, easier to read, and will also help you to spot any syntax errors before posting — for example, you’ve missed a closing curly bracket at the end of your withdraw function. Formatting your code also makes it easier to copy and paste if we need to deploy and test it.
Follow the instructions in this FAQ: How to post code in the forum

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

[josejalapeno]

I saw your answer, its really difficult until right now for me to fully understand everything, I still don’t have any ideas what kind of code should I be putting.

[Lennart_Lucas]

Add a require and reduce balance:

function withdraw(uint amount) public returns (uint) {
   require(balance[msg.sender] >= amount, "Not enough funds!");
   msg.sender.transfer(amount);
   balance[msg.sender] -= amount;
   return balance[msg.sender];
}