Solidity Payable Functions - Discussion

[Spartak]

contract HelloWorld{

//data locations:
//storage
//memory
//stack

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


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


address public owner;

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

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

 // address is the key in teh mapping
mapping(address => Person) private  people; 
address[] private creators;



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); 
    //This create a person
    Person memory newPerson;
    newPerson.name = name;
    newPerson.age = age;
    newPerson.height = height;
    newPerson.balance = msg.value;
    
    if(age >= 65){
        newPerson.senior = true;
    }
    
    else{
        newPerson.senior = false;
    }
    
    insertPerson(newPerson);
    creators.push(msg.sender);
    //people[msg.sender] == newPerson;
    assert(
        keccak256(
            abi.encodePacked(
                people[msg.sender].name, 
                people[msg.sender].age, 
                people[msg.sender].height, 
                people[msg.sender].senior
                )
                )  
                //hash of the person that is in the people mapping from assert
                == 
                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, uint balance){
    address creator = msg.sender;
    return (people[creator].name, people[creator].age, people[creator].height, people[creator].senior, people[creator].balance);
}



function deletePerson(address creator) public onlyOwner {
    
    string memory name = people[creator].name;
    bool senior = people[creator].senior;
    // morao dodat ova dva gore da sejva locally ime i senior, zbog brisanja na liniji ispod
    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];
}


}

[gabba]

@jon_m

if assert() were to fail, then wouldn’t all the remaining gas up to the gas limit be consumed anyway,

I completely forgot that it was the case, you are right all the gas will be consume, regarding the security issue as the balance variable is not reused in the contract, i m not sure it ll change something regarding the security, because at the end the transaction will be reverted if the user is not added.

[alexglebov]

I create a variable and updated it:
uint private currentBalance;
function createPerson(string memory name, uint age, uint height) public payable {
require(age < 150, “Age has to be lower then 150”);
require(msg.value >= 1 wei); // ether, wei
currentBalance += msg.value;

[prassie]

//add the below in createPerson() function
balance += msg.value;

//also return the balance in getPerson() function

[gabba]

Please @alexglebov @prassie use this button t display your code correctly,
Thank you

[jon_m]

@gabba

regarding the security issue as the balance variable is not reused in the contract, i m not sure it ll change something regarding the security, because at the end the transaction will be reverted if the user is not added.

But it’s still of vital importance that the correct current balance is stored in the state variable each time the createPerson function is called, otherwise any error/bug would have a knock-on effect on all future balances. That doesn’t seem to me to be any less critical than emit being incorrectly called. Yet in the code we have emit positioned after assert, but the balance-update code before…

Is there an important security reason for this difference in treatment/positioning that I can’t see, or is it just more down to personal coding style?

[letscrypto]

@filip Does it make sense to keep track of the balance via our own variable? Couldn’t the contract also receive money through a standard ETH transaction, i.e. someone sending it money without calling one of the functions? In which case our variable would show the wrong balance.

By googling I found that address(this).balance should contain the balance of the smart contract, do you recommend using this?

EDIT: I see, it looks like the contract address is not payable by default? At any rate, the following fails:

    function sendToSelf() public payable {
        uint money = msg.value;
        address contractAddress = address(this);
        address payable payableAddress = payable(contractAddress);
        payableAddress.transfer(money);
    }

with the message:

transact to MemoryAndStorage.sendToSelf errored: VM error: revert.
revert The transaction has been reverted to the initial state.
Note: The called function should be payable if you send value and the value you send should be less than your current balance. Debug the transaction to get more information.

[Brandon]

I added this getter to retrieve the contract’s balance.

function getBalance() public view returns(uint){
        uint contractBalance = address(this).balance;
       return contractBalance;
   }

[jon_m]

Nice, @Brandon!

And if we’re adding your getBalance function to the same HelloWorld contract with the state variable uint public balance; 
… we can now change this variable’s visibility from public to private, because otherwise we’ll have 2 balance getters. The other functions in the contract that use this balance variable, will still work if the visibility is set to private.

[jon_m]

Hi @letscrypto,

Sorry for the delay getting back to you on this one.

@letscrypto

Does it make sense to keep track of the balance via our own variable?

Just bear in mind that the smart contract functionality we are implementing in this initial course is mainly for demonstrating what we can achieve with Solidity. Certain elements may not reflect what would be the most practical solution when building smart contracts for permanent deployment on the blockchain.

@letscrypto

By googling I found that address(this).balance should contain the balance of the smart contract, do you recommend using this?

This is a valid alternative to the method demonstrated in this part of the course to track the balance. I’m tagging @gabba to see whether he can add anything in terms of which of the two methods is more widely used, or recommended in certain situations.

@letscrypto

Couldn’t the contract also receive money through a standard ETH transaction, i.e. someone sending it money without calling one of the functions? In which case our variable would show the wrong balance.

Good question! Let’s see what @gabba has to say about this.

You are right that when the contract address is accessed via address(this) it is non-payable. I’ve had a look at your code, and apart from the transaction error message you are getting, it also throws a compiler error before deployment. This is because you need to use   address(uint160(nonPayableAddress));   to convert a non-payable address into a payable one. So you code should look like this:

function sendToSelf() public payable {
        uint money = msg.value;
        address contractAddress = address(this);
        address payable payableAddress = address(uint160(contractAddress));
        payableAddress.transfer(money);
    }

This doesn’t throw a compiling error, and, personally, I would also be tempted to make the code block more succinct, as follows:

        address payable payableAddress = address(uint160(address(this)));
        payableAddress.transfer(msg.value);

Although you could argue that yours is easier to follow and understand, because it’s broken down into more steps.

However, when my revised function is called the transaction still fails, and to be honest I haven’t worked out why yet. But I thought I’d still reply with some answers to half of your questions. Let’s see if @gabba can enlighten us with the other half!

[gabba]

Hi @letscrypto @jon_m

You can still send funds to a non payable contract by destroying a suicide contract containing eth and setting the address of the contract you are targeting.

@letscrypto

By googling I found that address(this).balance should contain the balance of the smart contract, do you recommend using this?

I ll say that it depends your use case, it will save gas using the contract balance, but using your own variable is better to keep track of funds really use by your contract.

Let’s take an example:

the balance of the contract is 0
your balance variable is 0
User A use your contract to play a game and send 1 ETH. (using a function play)
the balance of the contract is 1
your balance variable is 1
An external smart contract send 1 ETH to your non payable contract
the balance of the contract is 2
your balance variable is 1

Now let’s say you have a function which check the balance of your contract to execute an action.
There is 1 ETH which is not associate with any user because it didn’t go through you play function and the contract which sent it has been destroyed.
It could leads to a bug as you will (in some scenario) substract 2 ETH from an other variable let’s say a bank variable which only have 1 ETH tracked and it ll lead to a buffer underflow.

I think it’s better to use your balance in this case because you know that every wei are tracked inside your contract. But in other case you can use the contract balance instead.

Is payable fallback function not always invoked?

[Brandon]

Hi @jon_m,

Agreed, a getter felt right for such a critical variable.

[jon_m]

@gabba, @letscrypto

@jon_m

However, when my revised function is called the transaction still fails, and to be honest I haven’t worked out why yet.

I couldn’t get this to work, and I still haven’t worked out why
Any thoughts @gabba?
Here’s the function:

function sendToSelf() public payable {
   address payable payableAddress = address(uint160(address(this)));
   payableAddress.transfer(msg.value);
}

[gabba]

@jon_m

@jon_m

function sendToSelf() public payable {
   address payable payableAddress = address(uint160(address(this)));
   payableAddress.transfer(msg.value);
}

You need to add a payable fallback function, but why are you trying to transfert ETH to the same contract ? Are you trying to broke “THE ETHEREUM”

    function() external payable {}

[letscrypto]

It is just for understanding

[Nile]

I just added another uint in our struct Person, and called to it in getPerson, which worked, I think. But for some reason, when I put in the value of ether, the output when I pressed getPerson (after deploying) a whole bunch of zeros were added to the number of ether I put in the wallet

[Nile]

Ah, all explained in the next video with a better solution. My way saved to the struct, which wasn’t necessary in this case.

[jon_m]

Hi Nile,

@Nile

I just added another uint in our struct Person, and called to it in getPerson

I assume here you’re talking about how to store the balance and then retrieve it. Is that right? It also sounds like you did later realise that we are storing and retrieving the contract balance, rather than individual user balances, and that’s why we store it in a separate state variable and don’t create an additional property for it in the User Struct.

@Nile

But for some reason, when I put in the value of ether, the output when I pressed getPerson (after deploying) a whole bunch of zeros were added to the number of ether I put in the wallet

Did you work out that the number with all the zeros is because it’s in gwei (and not ether)?

Just let us know if anything is still unclear, or if you have any questions about this section.

[pabnan]

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
}
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 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];
}

}

[jdbarrancs]

     function getBalance()public view returns(uint){
         return (msg.sender.balance);
    }