Solidity Payable Functions - Discussion

[camarosan]

Hello friends. i understood very well the explanation, but I have two questions
in my program i can check the balance on this way

i saw through the course that is not necessary assign 0 to any variable, solidity assign this for me. In solidity the variables always initialized in 0 by default ? I suppose my solution cost more because i assign expicity 0 to the variable.

the second is in the modifier I experimented a little and saw a powerfull tool because you can put the balance there and works. This is correct( i saw on internet modifiers with only require)? can you write more code in it to simplify my functions ?

thanks for your support

[Crypto_Craven]

Hi @filip

I’ve added getBalanceOfContract as a function and made the owner address payable to transfer the funds. Then added included the payment as part of the assertion and added the amount the user paid to the struct.

P.S I made height a string because we British don’t understand centimetres

Thanks!

pragma solidity 0.5.12;

contract HelloWorld {

mapping(address => Person) people;
address payable owner; //<-- made owner payable

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

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

address[] private creators;

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

modifier onlyOwner(){
    require(msg.sender == owner, "Caller needs to be owner");
    _;
    // continue execution
}

function createPerson(string memory name, uint age, string memory height) public payable {

    require(age <= 150, "Age needs to be below 150");
    require(msg.value >= 1 ether, "message value needs to be greate than 1 eth");

    Person memory newPerson;
    newPerson.id = creators.length;
    newPerson.name = name;
    newPerson.age = age;
    newPerson.height = height;
    newPerson.senior = isSenior(age);
    newPerson.walletAddress = msg.sender;
   
    // added amout payed to person struct
    newPerson.ethPayed = msg.value;
    
    insertPerson(newPerson);
    // added transfer of funds
    owner.transfer(msg.value);
    

    assert(
        keccak256(
            abi.encodePacked(
                people[msg.sender].name,
                people[msg.sender].age,
                people[msg.sender].height,
                people[msg.sender].senior,
                people[msg.sender].ethPayed //<-- added funds to assertion
            )
        )
        == keccak256(
        abi.encodePacked(
            newPerson.name,
            newPerson.age,
            newPerson.height,
            newPerson.senior,
            newPerson.ethPayed))
    );

    emit personCreated(newPerson.name, newPerson.senior, newPerson.ethPayed);
}

function insertPerson(Person memory newPerson) private {
    creators.push(msg.sender);
    people[msg.sender] = newPerson;
}

function getPerson() public view returns (string memory name, uint age, string memory height, bool isSenior){
    return (people[msg.sender].name, people[msg.sender].age, people[msg.sender].height, people[msg.sender].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 isSenior(uint age) private pure returns (bool senior){
    if (age > 65) {
        return true;
    } else {
        return false;
    }
}

function getCreator(uint index) public view onlyOwner returns (address creator)  {
    return creators[index];
}

function getContractBalance() public view returns (uint256) {
    //added method to retrieve contract balance
    return owner.balance; 
}

}

[Crypto_Craven]

Changed it to the way you did it after watching the next lecture

pragma solidity 0.5.12;

contract HelloWorld {

mapping(address => Person) people;
address public owner;
uint balance;

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

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

address[] private creators;

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

modifier costs(uint cost){
    require(msg.value >= cost, "message value needs to be meet cost");
    balance += msg.value;
    _;
}

modifier onlyOwner(){
    require(msg.sender == owner, "Caller needs to be owner");
    _;
}

function createPerson(string memory name, uint age, string memory height) public payable costs(1 ether) {

    require(age <= 150, "Age needs to be below 150");
    
    Person memory newPerson;
    newPerson.id = creators.length;
    newPerson.name = name;
    newPerson.age = age;
    newPerson.height = height;
    newPerson.senior = isSenior(age);
    newPerson.walletAddress = msg.sender;
    newPerson.ethPayed = msg.value;
    
    insertPerson(newPerson);

    assert(
        keccak256(
            abi.encodePacked(
                people[msg.sender].name,
                people[msg.sender].age,
                people[msg.sender].height,
                people[msg.sender].senior,
                people[msg.sender].ethPayed //<-- added funds to assertion
            )
        )
        == keccak256(
        abi.encodePacked(
            newPerson.name,
            newPerson.age,
            newPerson.height,
            newPerson.senior,
            newPerson.ethPayed))
    );

    emit personCreated(newPerson.name, newPerson.senior, newPerson.ethPayed);
}

function insertPerson(Person memory newPerson) private {
    creators.push(msg.sender);
    people[msg.sender] = newPerson;
}

function getPerson() public view returns (string memory name, uint age, string memory height, bool isSenior){
    return (people[msg.sender].name, people[msg.sender].age, people[msg.sender].height, people[msg.sender].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 isSenior(uint age) private pure returns (bool senior){
    if (age > 65) {
        return true;
    } else {
        return false;
    }
}

function getCreator(uint index) public view onlyOwner returns (address creator)  {
    return creators[index];
}

function getContractBalance() public view returns (uint) {
    return balance; 
}

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

}

[RikkeHrab]

My solution to getBalance assignment:

pragma solidity 0.5.12;

contract HelloWorld{

//Struct:

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

address public 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 can help restrict what type of users you have:
    require(age > 18, "Age need to be above 18");
    
    //Check if payment is sufficient:
    require(msg.value >= 1 ether, "Insufficient funds");
    
    //First way:
    //people.push(Person(people.length, name, age, height));
    
    //How to get an address:
    
    //second way:
    Person memory newPerson;
    newPerson.name = name;
    newPerson.age = age;
    newPerson.height = height;
    if (age > 65) {
        newPerson.senior = true;
    }
    else {
        newPerson.senior = false;
    }
    **newPerson.balance = msg.value;**
    
    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
                        )
                      )
                    );

}

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);
}
//require can help make sure only owners and creators of a contract and address can delete or make changes:
function deletePerson(address creator) public {
    require(msg.sender == owner);
    delete people[creator];
    assert(people[creator].age == 0);

}
//Require can help restrict access to a contract to the owner:
function getCreator(uint index) public view returns(address) {
require(msg.sender == owner);
return creators[index];
}
//Get balance:
function getBalance() public view returns(uint balance) {
** address creator = msg.sender;**
** return (people[creator].balance);**
}
}

[Kraken]

My showBalance and withdrawAll functions. I didnt create a balance variable for the contract. Maybe in another case it could be useful, but isnt it safer to use the real balance ( address(this).balance ) then to rely on perfect code and no reversion attacks etc. with an extra variable like balance in the contract?

function showBalance()public view returns (uint){
uint balance = address(this).balance;
return balance;

}

Blockquote

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

[jon_m]

Hi @camarosan,

Sorry this reply is 2 weeks after you asked the original question, but I’ve noticed that you didn’t get a reply and your questions are interesting ones… so… hopefully, better late than never…

@camarosan

i saw through the course that is not necessary assign 0 to any variable, solidity assign this for me. In solidity the variables always initialized in 0 by default ?

In Solidity (unlike JavaScript) we define the data type when declaring a variable. This means that if we want to initialise a variable with its data type’s zero-equivalent value, then we can leave it unassigned, and Solidity will assign the zero-equivalent value to it by default:

uint private variable;    // variable = 0
int private variable;     // variable = 0
bool private variable;    // variable = false
string private variable;  // variable = ""

Here is a link to a useful discussion about this:
https://ethereum.stackexchange.com/questions/84270/why-solidity-doesnt-use-null-and-undefined

@camarosan

I suppose my solution cost more because i assign expicity 0 to the variable.

Yes, it will consume a small amount of additional gas because of this.

@camarosan

This is correct( i saw on internet modifiers with only require)? can you write more code in it to simplify my functions ?

Yes, you can add more code and more operations to a modifier in addition to a require statement, but I would say that there is a reason why you shouldn’t do that in this particular case:

It is better practice, and almost certainly safer from a risk management perspective, to perform all of the require checks before making changes to a contract’s state (here, increasing the balance). Even though your modifier increases the balance by msg.value before checking that age < 150, it is true that if the age check then fails, all of the operations performed previously in the function call (including the balance increase) will still be reverted. However, the more operations (and especially changes to state) that we allow to be performed before a potential revert can be triggered, the more we are increasing the risk of introducing vulnerabilites into our code. In addition, even though all operations would be reverted, the gas already consumed by each operation isn’t refunded.

You should also bear in mind that the whole point of using a modifier is to avoid code duplication across multiple functions. The more functionality you add to a modifier, the less reusable it becomes. In this particular case, I think it is clear that whenever calling a function requires a payment, then this amount would also need to be added to the overall contract balance, but, generally speaking, you should certainly think carefully about whether you are restricting your modifier’s usability by adding additional code to it. We should also remember that our code is generally clearer and more secure, the more modular it is, and so it’s good practice to try to allocate code for different tasks to separate functions and modifiers. However, again, there is certainly a case to be made here that increasing the balance by the payment amount is part of the same task as checking that the amount is sufficient.

I hope this goes some way towards answering your questions. Do let us know, though, if you are still unsure about anything. Keep up the great work!

[siratoure95]

My solution to homework question:
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;

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

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

mapping (address => Person) private people;
mapping(address => uint) public balances;
address[] private creators;
//To have financial trnascation you must put the key word payable
//payable: allow a function to recieve money
//
function createPerson(string memory name, uint age, uint height) public payable{
require(age < 150, "Age needs to be below 150");
//check to see if people send enough money 
//check if payment is >= 1
//msg.value: allows function to obtain
// the amount of money sent to a function 
//1 = small amount of money called wei
//wei is the smallest unit in eth 
//To get 1 ETH u need to use the folllowing: 1 ether
require(msg.value >= 1 ether);
//save the current balance
getContract(balances[msg.sender]);
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 getContract(uint newBalance) public {
    
    balances[msg.sender] = newBalance;
}        

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

}

[jon_m]

Hi @siratoure95,

I think you’ve got confused here.

The idea is to store, track, and be able to get, the current contract balance, not individual user balances. The payment in ether required when a user calls createPerson to create a new record, is paid to the contract address and not to the user’s address. So, your mapping…

@siratoure95

mapping(address => uint) public balances;

…doesn’t store anything. This means that the function call…

@siratoure95

getContract(balances[msg.sender]);

…which you’ve added to createPerson, is called with an argument value of zero, because…

balances[msg.sender]   // => 0

To call getContract with the payment value, you’d need to call it with msg.value …

getContract(msg.value);

But even then, your getContract function would still be assigning this “balance” to the user who paid it (in your mapping), and not adding it to a variable recording the contract balance.

If you do decide to allocate the payment using a separate function, then this should be marked with private visibility (not public) because it should only be called from within the same contract and not externally as well.

Have another go, bearing in mind what I’ve explained above. You will also find several alternative solutions posted by students in this discussion thread, and further discussion and feedback on these.

Do let us know if you have any questions, so we can help you

[camarosan]

thank you very much @jon_n.
I understand everything

[Ciaran]

Deliberately skipped the later part of the contract which hasn’t been changed.

pragma solidity 0.5.12;

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);
    
    //viewable contract balance (NEW)
    uint public balance;
    address public owner;

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

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

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

    //create person
    function createPerson(string memory name, uint age, uint height) public payable {
        //check enough money added to contract
        require(msg.value >= 1 ether);
        //increase contract balance by value sent to contract (NEW)
        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);
    }

[HardlyCodeMan]

I thought I’d just use another event rather than taking up more lines and memory with another function and variables with the assumption that by doing this it would cost the user a greater transaction gas fee for createPerson()

contract HelloWorld {
…
event paymentReceived(uint balance);
…

function createPerson ...
    ...
    emit paymentReceived(address(this).balance);
}
...

}

[bulletfingers]

I added two lines of code to the function, one checks to make sure the person requesting to take money out has at least that amount of balance.

Second, I subtracted whatever was taken out from the person’s balance.

function withdraw(uint amount) public returns (uint) {
    require(amount <= balance[msg.sender], "Be gone peasant!");
    msg.sender.transfer(amount);
    balance[msg.sender] -= amount;
}

[bulletfingers]

Is there any reason that in your solution on Github that you have subtracted from the balance first (I subtracted second)?

    balance[msg.sender] -= amount;
    msg.sender.transfer(amount);

[abuga]

@bulletfingers

I dont think that is a problem to transfer first then subtract, however it is good practice to subtract first then send.

Happy learning
Abel S

[jon_m]

@bulletfingers

Is there any reason that in your solution on Github that you have subtracted from the balance first (I subtracted second)?

    balance[msg.sender] -= amount;
    msg.sender.transfer(amount);

Check this post out. It’s to prevent something called a re-entrancy attack which you will learn about in a later course.

[bulletfingers]

Thanks Jon, I had some suspicion about this, but couldn’t remember what the concept was called or if it applied in smart contract world.

In a centralized world, I can imagine that a server might shut off or fail at an exact second where you sent money but didn’t reduce the balance, then someone might be able to double spend. This is probably different than a re-entrancy attack, but an example I thought of that made me suspicious initially.

Excited to learn more about that in the future security course.

[jon_m]

Hey @bulletfingers,

You are certainly thinking along the right lines In a re-entrancy attack, external code hijacks your contract during execution making it behave differently to how it should. A transfer of funds to an external contract could trigger something known as a fallback function in the external contract, which, if malicious, could call the withdraw function in our contract multiple times until the contract’s ether balance is reduced to zero. If our code reducing the caller’s balance for the amount transferred is placed after this “attack loop”, then the require statement placed at the beginning of the function body will never throw, because the caller’s recorded balance in the mapping isn’t reduced each time the function is re-entered. We can prevent this type of attack by adjusting the internal state of our contract for the effect of the transfer before the actual external transfer takes place: the require statement would then throw whenever the caller’s fund balance is insufficient to meet the amount requested during one the attack iterations.

[D0tmask]

Hi everyone,

So my function looks like:

function withdraw(uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient!");
        msg.sender.transfer(amount); //build-in error handeling
        balance[msg.sender] -= amount;
        emit withdrawComplete(msg.sender, amount);
    }

Also @jon_m I have a question. How this function suppose to look like in compiler version 0.8.0? If I use the same function in that version the following error popup:

TypeError: “send” and “transfer” are only available for objects of type “address payable”, not “address”.

I tried to use “payable” in different places but I can’t figure that out.

Thanks.

[jon_m]

Hey @D0tmask,

You have added all of the additional lines of code needed to solve the problem with the withdraw function

Now have a look at this post which explains an important security modification that should be made to the order of the statements within your withdraw function body.

The original assignment code includes returns(uint) in the withdraw function header. This is not mandatory to include, and the function can still operate effectively without returning a value. But if it was included, you may just want to think about the return statement you would also need to add to the function body.

@D0tmask

How this function suppose to look like in compiler version 0.8.0? If I use the same function in that version the following error popup:

TypeError: “send” and “transfer” are only available for objects of type “address payable”, not “address”.

I tried to use “ payable ” in different places but I can’t figure that out.

That’s great that you are already experimenting with using the very latest version 0.8.0 Here is a link to the relevant page in the Solidity v0.8.0 documentation, where you can find out about the main changes that have been introduced with this version, and how to code for them.
https://docs.soliditylang.org/en/v0.8.0/080-breaking-changes.html
The answer to your specific question can be found in the 9th bullet point in this section:
https://docs.soliditylang.org/en/v0.8.0/080-breaking-changes.html#new-restrictions
You will see that the issue is that msg.sender is no longer a payable address by default, and so when using it with transfer() or send() you need to convert it to a payable address, which can be done as follows:

payable(msg.sender).transfer(amountToTransfer);

Let us know if you have any further questions, but, hopefully, now that I’ve pointed you in the right direction in terms of the relevant documentation, you’ll be able to find the answers to these sorts of questions more easily yourself from now on

[D0tmask]

Hi @jon_m,

The code is working fine now. Thanks for your help and references

Cheers.