Solidity Payable Functions - Discussion

Ethereum Smart Contract Programming 101
#122

Hi @Ouzo69,

First of all, apologies for the long delay in providing you with some feedback on this. Unfortunately, your post was missed :pray:

Yes, thatā€™s exactly right :+1:

This isnā€™t needed because you are transferring the contract balance to msg.sender (which can only be the owner due to the onlyOwner modifier): msg.sender is always payable by default (unlike address variable definitions, which are unpayable by default). However, you would need to make owner a payable address if you transferred to owner, instead of msg.sender:

function withdrawAll () public onlyOwner {
    owner.transfer(getContractBalance());   // owner  instead of  msg.sender
    assert(getContractBalance() == 0);
}

I think Iā€™ve already commented before on how itā€™s very clever how you have made the withdrawAll function more concise by using a function call to getContractBalance to retrieve the balance to transfer. You could also replace the function call withā€‚ address(this).balance :

function withdrawAll () public onlyOwner {
    msg.sender.transfer(address(this).balance);
    assert(address(this).balance == 0);
}

It would be interesting to see if there was much difference in the gas cost between these two alternativesā€¦

This is an excellent use of assert to check for an important invariant. This very much improves your contract security :+1:

2 Likes
#123

Hi @KryptoS,

Apologies for the long delay in providing you with some feedback on this. Unfortunately, your post was missed :pray:

Yes, but as the variable has been defined as having the unsigned integer data type, there is no need to assign 0 to it from the outset, as it will still return 0 if accessed before any payments are assigned to it.

uint private contractBalance;   // this is enough

In fact, itā€™s better and more secure to add the payment to contractBalance at the beginning of the function body (immediately after any require functions), and definitely not after the assert function. If assert is triggered, then revert is called, anyway, reversing any initial increase to the contract balance, and also any corresponding deduction from the callerā€™s account balance.

:ok_hand:

2 Likes
#124

Hello @jon_m !

Thank you for your very helpful comments about my post. Appreciate it!

When I wrote the ā€œmissedā€ post I had to choose between two different forums. So maybe my post had been missed, since I made the wrong choice posting it into a forum you didnā€™t expect it to be.

However, you finally found and answered it! Thanks!

Best regards

Ouzo69

1 Like
#125

No, you posted it in the right discussion topic @Ouzo69. It was our slip-up :pray: but we got there in the end :sweat_smile: once again, apologies.

2 Likes
#126

No problem!

:slight_smile:

1 Like
#127

Thanks @jon_m for your review and great feedback. :slightly_smiling_face:

1 Like
#128 
pragma solidity 0.5.12;
pragma experimental ABIEncoderV2; 
contract HelloWorld {

    
    struct Person {
        string name;
        uint age;
        uint height;
        bool senior;
        
    }
    
    event personCreated(string name, bool senior);
    event personDeleted(string name, bool senior, address deletedBy);
    
     address public owner;
     uint public balance;
     
     modifier onlyOwner(){
         require(msg.sender == owner);
         _;
     }
            constructor () public {
                owner = msg.sender;
            }
            
        mapping(address => Person) private people;
        address[] private creators;
        
        function createPerson(string memory name, uint age, uint height) public payable {
            require(age< 150, "Age too high");
            require(msg.value >= 1 ether);
            balance = balance + msg.value;
           Person memory newPerson;
           newPerson.name = name;
           newPerson.age = age;
           newPerson.height = height;
           if( age >= 65) {
               newPerson.senior = true;

           }
           else {
               newPerson.senior = false;
        }
        insertPerson(newPerson);
        creators.push(msg.sender);
        assert(
            keccak256(
                abi.encodePacked(
                    people[msg.sender].name, 
                    people[msg.sender].age, 
                    people[msg.sender].height, 
                    people[msg.sender].senior
                    )
                ) 
                == 
                keccak256(
                    abi.encodePacked(
                        newPerson.name, 
                        newPerson.age, 
                        newPerson.height, 
                        newPerson.senior
                        )
                    )
                );
                emit personCreated(newPerson.name, newPerson.senior);
             
            
        }             
        
        function insertPerson(Person memory newPerson) private {
           address creator = msg.sender;
           people[creator] = newPerson;
        }
 
    function getPerson() public view returns(string memory name, uint age, uint height, bool senior ){
        address creator = msg.sender;
        return (people[creator].name, people[creator].age, people[creator].height, people[creator].senior);
        
    }
    function deletePerson(address creator) public onlyOwner{
        string memory name = people[creator].name;
        bool senior = people[creator].senior;
        
        delete people[creator];
        assert(people[creator].age == 0);
        emit personDeleted(name, senior, msg.sender);
    }
    function getCreator(uint index) public view onlyOwner returns(address){
        return creators[index];
    }
}`Preformatted text`
1 Like
#129

Too much messing around at the end of the video. Really Confusing and not very helpful!
Not expecting a reply from Filip.
DOODESVILLE

1 Like
#130

Hey @doodesville, sorry to hear that :frowning:

Could you please describe me a little bit what you do not like? (send me a private message if you want to keep it private).

Carlos Z.

#131

I made a view function at the end of the contract. Itā€™s purpose is to get the current balance of the user by .balance. I hope this is sufficient.

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

1 Like
#132

First I got a copy of @Filip 's code
I add a variable image
I add the amount payed to the variable image

I get the balance in wei. (as expected, it is the base unit)

1 Like
#133

Hi @filip !

General Question about send function example from the video:

You stated, if the send function returns false, execution continues and resets balance to 0, which is bad because it would lead to a ā€œloss in fundsā€. However, it would not be registered in the blockchain, so the funds would technically still belong to the address owner, no ?

Thanks!

1 Like
#134

Even the send functions returns false, the execution continues, that mean that funds are going to be lost because the function executeā€¦so basically it will be registered in the blockchain has false (in value returned) but the execution was approved.

If you have any more questions, please let us know so we can help you! :slight_smile:

Carlos Z.

#135

Hello Filip,

I did the codes as you taught us, and by withdrawal, I got nothing in it. Is there something I am missing, or ommited?

pragma solidity 0.5.12;

contract HelloWorld{

struct Person {
  uint id;
  string name;
  uint age;
  uint height;
  bool senior;
}

event personCreated(string name, bool senior);
event personDeleted(string name, bool senior, address deletedBy);

address public owner;
uint public balance;


modifier onlyOwner(){
    require(msg.sender == owner);
    _; //Continue execution
}

modifier costs(uint cost){
    require(msg.value >= cost);
    _;
}

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

mapping (address => Person) private people;
address[] private creators;

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

    //This creates a person
    Person memory newPerson;
    newPerson.name = name;
    newPerson.age = age;
    newPerson.height = height;

    if(age >= 65){
       newPerson.senior = true;
   }
   else{
       newPerson.senior = false;
   }

    insertPerson(newPerson);
    creators.push(msg.sender);

    assert(
        keccak256(
            abi.encodePacked(
                people[msg.sender].name,
                people[msg.sender].age,
                people[msg.sender].height,
                people[msg.sender].senior
            )
        )
        ==
        keccak256(
            abi.encodePacked(
                newPerson.name,
                newPerson.age,
                newPerson.height,
                newPerson.senior
            )
        )
    );
    emit personCreated(newPerson.name, newPerson.senior);
}
function insertPerson(Person memory newPerson) private {
    address creator = msg.sender;
    people[creator] = newPerson;
}
function getPerson() public view returns(string memory name, uint age, uint height, bool senior){
    address creator = msg.sender;
    return (people[creator].name, people[creator].age, people[creator].height, people[creator].senior);
}
function deletePerson(address creator) public onlyOwner {
  string memory name = people[creator].name;
  bool senior = people[creator].senior;

   delete people[creator];
   assert(people[creator].age == 0);
   emit personDeleted(name, senior, owner);

}
function getCreator(uint index) public view onlyOwner returns(address){
return creators[index];
}
function withdrawAll() public onlyOwner returns(uint) {
uint toTransfer = balance;
balance = 0;
msg.sender.transfer(toTransfer);
return toTransfer;
}

}

1 Like
#136

Hey @Ako1, hope you are good.

I have tested your code, it works great for me, your withdraw functions works good, I created a person with another account (address), I successfully add 1 ether to the contract through creating that person.

Then I just use the balance function to verify that the contract have some balance on it, 1 ether (in wei), then use the withdraw function with the account that deploy the contract (which is the owner) and it successfully withdraw the ether to the account.

Carlos Z

1 Like
#137

I dunno if i got it wrong, but to return the smart contract balance i did:

function getCtBalance() public view returns (uint256) {
return address(this).balance;
}

1 Like
#138

@filip is it bad in terms of security to do it like this??

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

1 Like
#139

Yep, because you are not updating the contract balance to zero, meaning this could exploit the contract balance.

If you have any more questions, please let us know so we can help you! :slight_smile:

Carlos Z.

#140

To be able to check contractā€™s balance I added a state variable:
uint public balance;

And then in the function:
function createPerson(string memory name, uint age, uint height) public payable {
require(age <= 150, ā€œAge needs to be below 150ā€);
require(msg.value >= 1 ether);
balance = address(this).balance;

1 Like
#141

Getting contract balance:

function balanceETH() public view returns(uint256){
        return address(this).balance / 1000000000000000000; //
        
    } //A function that returns the balance of the smart contract in ETH. Probably more practical to return in wei in the vast majority of cases and do the required conversions in the frontend of a dapp.
1 Like