Solidity Payable Functions - Discussion

Ethereum Smart Contract Programming 101
#21

Hi.
I was ambiguous with the previous comment, forgive me. So in this link:

In the last block (from line 88) I discovered now two error in the code.

This is the original GitHub code:

 function withdrawAll() public onlyOwner returns(uint) {
       toTransfer = balance;
       balance = 0;
       msg.sender.transfer(balance);
       return toTransfer;
   }

Bug fix:

  • line88: uint toTransfer = balance; → It was missing the uint command.
  • line90: msg.sender.transfer(toTransfer); → Like you mentioned before, this is how it is supposed to be

The code is failing in the original case, if you try to withdraw all your funds from the contract. That is gonna wipe itself and that is not gonna return for the contract owner. In the previous line we defined balance = 0;, after there is no funds to send to the owner.

Please correct me, if I am wrong!
Byko

2 Likes
#22

Hi.
I totally agree you are correct. :+1:
You could post a pull request if you want the honor?

Ivo

1 Like
#23

Hi.
I done with that. I never used GitHub before. I am really rookie with coding, but it was really nice to learn something new again. After some youtube tutorials, I figured it out how that works.
Byko

2 Likes
#24

Nice. Learn something new every day!
Ivo

#25

I will continue for the rest of my life. :smiley:
It is fun. It is my hobby.

1 Like
#27

@ivga80

Hey!
I’d spotted the same errors :wink:

By the way, these same errors are also in lines 99 and 101 in the send-and-transfer.sol code in Filip’s GitHub repository — this is the file I used to check my code from the video tutorial, rather than the one you have mentioned.

I’ve also spotted a redundant line of code in payment-assignment-solution.sol in Filip’s GitHub repository:

Line 39 …

require(msg.value >= 1 ether);

… should be removed, as this condition is already accounted for in the modifier costs (lines 25-28), which is called in the function header in line 37: costs(1 ether).
This was deleted in the video.

I’ve just looked, and this same redundant line of code also appears at line 28 in the file you mentioned with the other errors: https://github.com/filipmartinsson/solidity-0.5.12-course/blob/master/External-Contracts/HelloWorld.sol

I haven’t got a GitHub account yet, so I hope it’s not too cheeky to ask if you would mind posting a pull request for these as well? :wink:

Hope that helps with some more housekeeping! :slight_smile:

3 Likes
#28

Hey! I will do it today. I keep practicing at least. :wink:

2 Likes
#29

If you have a smart contract on ethereum and you want people to pay to use it, there’s no stopping anybody else from just omitting the payment parameters and redeploying the contract and everyone else using it for free?

1 Like
#30

@ivga80 @filip @gabba @pedromndias

Here are a few questions I have from this section about payable functions…

Question 1

This question is about the position of…

balance += msg.value;

…in the createPerson function.

In the assignment, why is it ok to add msg.value to the contract’s balance at the beginning of the function body, immediately after require() ? … instead of putting this code at the very end, like we did with emit ?
If emit is placed before assert() the event still shouldn’t be emitted if assert() fails, because revert is called and cancels execution of all of the function body’s prior code. Despite this, however, in the lesson we learnt that it is still safer to place emit at the very end of the function body, as we only want to emit the event if the new person is definitely created. So, surely this same security logic should apply to updating the balance — only doing it at the very end of the function body when we can be sure execution has been successful, and the ether definitely charged and credited to the contract…

Question 2

The assignment solution defined the state variable balance as public :

uint public balance;

In reality, is it more likely that the contract owner would want the contract balance to be stored in a private state variable, with a separate getter function (restricted to the contract owner) to access the balance? e.g.

address public owner;
uint private balance;       // balance defined as private

constructor() public {
   owner = msg.sender;
}

modifier onlyOwner() {
   require(msg.sender == owner, "Operation restricted to the contract owner");
   _;
}

/* public getter function wrapped around private balance variable,
   but access restricted to contract owner */
function getBalance() public view onlyOwner returns(uint) {
   return balance;
}

I wanted to add the name balance to the uint returned from the getBalance function (just like we did previously with age and height — but when I add this to the code, as follows …

    returns(uint balance)

… it throws the following warning (and I don’t understand what the problem is):

Warning: This declaration shadows an existing declaration. ...
... The shadowed declaration is here:
uint private balance; // Use with getBalance function restricted to onlyOwner

Question 3

I tried to make the getPerson function payable  but it throws an error because it seems that view is incompatible with payable — does this mean that viewing a contract’s data must always be free of charge?

Question 4

Why did we include a return statement in the withdrawAll function, when, as far as I can see, we aren’t using the returned value (the total balance withdrawn) anywhere? Is it because it gives us the option of using this value somewhere else in the contract? If so, what functionality could we add that would utilise it? I’ve tried to experiment with this, but without any results…

1 Like
#31

Solution assignment getBalance.
I’ve added a new getBalance function at the end of the code, that return “msg.sender.balance”.

pragma solidity 0.5.12;

contract payableFunctions{
    
    struct Car{
        string model;
        uint year;
    }
    //ALWAYS TO DO: DEFINE OWNER ADRESS
    address public owner;
    modifier onlyOwner(){
        require (msg.sender == owner);
        _;
    }
    constructor() public{
        owner = msg.sender;
    }
    
    mapping (address => Car) private cars;
    //WE CREATE AN ARRAY OF ADDRESS, FOR KNOWING WHO HAVE CREATED THE CARS
    address [] private creators;
    
    //FUNCTION CREATE A CAR
    function createCar(string memory model, uint year) public payable{
        require(year>1900, "It's impossible to have an old car like this!");
        require(msg.value>= 1 ether);
        Car memory newCar;
        newCar.model = model;
        newCar.year = year;
        
        insertCar(newCar);
        creators.push(msg.sender);
    }
    function insertCar(Car memory newCar) private{
        address creator = msg.sender;
        cars[creator] = newCar;
    }
    function getCar() public view returns(string memory model, uint year){
        address creator = msg.sender;
        return(cars[creator].model, cars[creator].year);
    }
    function getBalance()public view returns(uint){
        return (msg.sender.balance);
    }
}
1 Like
#32

You guys are amazing! @jon_m @Byko. I’ve corrected all the files now, I think… :wink:

2 Likes
#33

Question 1

Totally agree , and if it fails less gas will be consume during the transaction.
Do not hesitate to open an issue in @filip repository

Question 2

As there is no setter in this contract for the balance it’s not really mandatory to type the balance as private. Private just mean you can’t modify the variable from outside. For a public variable the compiler is creating automatically a getter that’s why public vars increases the size of a contract when you deploy it.

Everything is visible on the blockchain so it will just takes you more effort to know the variable value but you will find it doing a bit of forensic . Private doesn’t mean secret in the blockchain world.

The problem is you are using a variable name already used in your contract, you can call it _balance will not throw any warning.
Previously in the getPerson function we don’t have this problem because name , age ,height and senior are not variables declared in the contract but variable of a structure so the compiler doesn’t find any conflicting variables name.

Question 3

A function can be only one of the followings type payable/view/pure
A function using view will only be able to read from the storage , reading is free so it should be gas free.
A function using payable is able to receive eth.
You can totally do that:

    function getBalance() public payable onlyOwner returns(uint _balance) {
        require(msg.value == 0.5 ether);
       return balance;
    }

But as i told you is pointless as you can read any data in the blockchain.
If you want to have fun i recommend you this exercise:

https://ethernaut.openzeppelin.com/level/0x76b9fade124191ff5642ba1731a8279b30ebe644

You will see that we can read the value of the variable bytes32[3] private data;

Question 4

The reason here could be to display it in the frontend, don’t forget that most of those smart contracts are for educational purpose, they are not ultra optimized but it ll be interesting in the future course. To go a bit deeper :wink:

2 Likes
#34

@Byko

Glad to help! :slightly_smiling_face:

With this same issue, I’ve also noticed that the same errors have been copied into the code samples for the section on Inheritance, specifically:

I also noticed that in lecture 2 in the Inheritance section, the code sample link is to the Inheritance-Assignment-Solution folder. This should be to the Inheritance folder instead, as the solution shouldn’t be viewed until lesson 4, after the assignment has been done.

Hope that’s helpful too :slightly_smiling_face:

1 Like
#35

Really helpful comments, thanks! I’ll definitely check out The Ethernaut too.

Will we learn how to display outputs from Solidity smart contracts in the frontend using HTML/JavaScript in the next course? I guess that’s what we’re talking about here…

#36

I took the Javascript Programming for Blockchain Developers course after Ethereum Smart Contract Programming 101 and I found very useful.
In the beginning is mostly frontend, if you are new to programming like, me. I recommend to take that course sometime.
Also helps a lot with the basic mindset, how to design a contract.

1 Like
#37

Hi,

Yeh, I took that course before this Solidity one — opposite way to you :smile: I’ve been learning JavaScript for about a year now, and you are right that it definitely helps with the Solidity “mindset”, as so many concepts are the same or very similar.

With JavaScript I already know how to request data with AJAX requests and display it in an HTML frontend. However, I’m not sure how I’d do that with a Solidity smart contract backend… that’s what I was referring to in my post… I’m sure all will be revealed in the next Solidity course :smiley:

1 Like
#38

We usually recommend taking the Javascript Programming for Blockchain Developers course as the first course.:wink:
Ivo

3 Likes
#39

Just a thought… if assert() were to fail, then wouldn’t all the remaining gas up to the gas limit be consumed anyway, meaning that it won’t ever actually make any difference to the gas cost whether we add msg.value to the contract’s balance before or after assert() , but only makes a difference in terms of the function’s security?

1 Like
#40

I was too excited to take the solidity course. That is why I started with that :smiley:

2 Likes
#41 
uint public balance;

function createPerson(string memory name, uint age, uint height) public payable costs(1 ether){
      require(age < 150, "Age needs to be below 150");
      balance += msg.value;
}