Solidity Payable Functions - Discussion

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


}
1 Like

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.

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;

1 Like

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

//also return the balance in getPerson() function

1 Like

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

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ā€¦ :thinking:

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?

@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.

1 Like

I added this getter to retrieve the contractā€™s balance.

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

Nice, @Brandon! :ok_hand:

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.

1 Like

Hi @letscrypto,

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

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.

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.

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! :grin:

3 Likes

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.

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.

3 Likes

Hi @jon_m,

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

1 Like

@gabba, @letscrypto

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

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

@jon_m

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ā€ :rofl:

    function() external payable {}
1 Like

It is just for understanding :slight_smile:

1 Like

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

1 Like

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

1 Like

Hi Nile,

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.

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.

1 Like

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

}

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