Solidity Payable Functions - Discussion

Taha · July 28, 2020, 7:24am

Hi @Maddash79
Thanks for reaching out!
Yes, it is correct

KingArt · July 29, 2020, 1:28am

Hi Filip,

Below is my create person function. It looks correct to me and it compiles just fine, however when I deploy in remix the tx fails and says “VM error, the execution might have thrown” thoughts on what I could be doing wrong?

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;
       
       Person memory newPerson;
       newPerson.name = name;
       newPerson.age = age;
       newPerson.height = height;
       
   
       if(age >= 65){
           newPerson.senior = true;
       }
       
     else{
           newPerson.senior = false;
       }

Taha · July 29, 2020, 9:37pm

@KingArt
Thanks for reaching out!
Can you post the full solidity code here?

Also, let me you which environment and network you are using?

KingArt · July 30, 2020, 1:01am

Thanks for reaching out Taha, I think I fixed it, it is working although I’m not sure what I did to fix it lol

tfrancisco · August 4, 2020, 9:07pm

I included this

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

to get the balance of the contract. It works. Each person costs 100 wei and I added two.

Taha · August 5, 2020, 12:05am

@tfrancisco
Very good. Keep up the good work

ajphminer · August 9, 2020, 4:19pm

My solution is:

1.Create a private state variable named contractBalance:

uint private contractBalance;

Initiate this in the constructor:

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

add msg.value to contractBalance every time createPerson is called

contractBalance += msg.Preformatted textvalue;

create a function to query the balance that only the owner can call

 function getBalance() public view onlyOwner returns(uint balance){
        return contractBalance;
    }

Dan_O_Kane · August 10, 2020, 7:09pm

@filip

I added a public variable total to save the balance.
in the createPerson function I’ve added

total += msg.value;

and I added an event so I could see it in action

event balance(uint balance);
address public owner;
uint public total;

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);
  total += msg.value; 
  emit balance(total);

nielvosloo · August 10, 2020, 9:38pm

My solution is as follows:

1. Create structure to hold contract owner information

struct Owner {
     address _address;
     uint256 _balance;
}

2. Change constructor to set the contract balance to zero

constructor() public {
     owner._address = msg.sender;
     //owner._balance = msg.sender.balance;
     owner._balance = 0;
}

3. Changed the createPerson function to update the owner balance with the sender value

require(msg.value >= 1 ether);
owner._balance += msg.value;

4. Added function to get the owner balance in ether

function getBalance() public view _onlyOwner returns(uint) {
     return owner._balance / 1 ether;
}

*Changed the constructor to set the balance to zero after realizing that the ether added to the contract is different to the contract owner.

Taha · August 11, 2020, 9:42am

@Dan_O_Kane
Amazing work!

Kristhofer_Drost1 · August 12, 2020, 5:04pm

I honestly don’t know what to add.

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");
      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, owner);
   }
   function getCreator(uint index) public view onlyOwner returns(address){
       return creators[index];
   }

I’ll say maybe adding uint balance; newPerson. balance=msg.value; and people[creator].balance;

If I’m wrong, please, I would love to recieve some help.

Taha · August 12, 2020, 11:41pm

@Kristhofer_Drost1
The contract looks fine to me

elterremoto · August 17, 2020, 10:57pm

added:
struct Person {

    string name;
    uint age;
    uint height;
    bool isSenior;
    uint balance; //added
    
}

… also on the createPerson:
Person memory newPerson;
newPerson.name=name;
newPerson.age=age;
newPerson.height = height;
newPerson.balance = msg.value;

hannezzon · August 22, 2020, 1:01pm

Possible I did not get the assignment - still I learnt from the exercise:

// Keep separate balance earned on all created persons activity
uint256 public createdPersonBalance = 0;

function createPerson(string memory name, uint age, uint height) public payable {
    require(age < 150, "Age must be less that 150");
    // check payment is >= 1 ether 
    require(msg.value >= 1 ether);

// …all code …
// update the created-balance
createdPersonBalance =createdPersonBalance + msg.value;
emit personCreated(newPerson.name, newPerson.senior);
}

// Charge for setting a message
function setMessage(string memory newMessage) public payable {
require(msg.value >= 1 ether);
message = newMessage;
}

// Report the contract balance and the specific created person balance
function getBalanceReport() public view returns(uint256 total, uint256 createdBalance){
return(address(this).balance,createdPersonBalance);
}

hannezzon · August 22, 2020, 2:07pm

My takeaway from this lesson is the importance with using patterns on how to order and check the transaction with state changes. Note to self

        // Send is good for custom error handling
        if (msg.sender.send(toTransfer)){
            return toTransfer;   
        } else {
            createdPersonBalance = createdBal;
            return 0;
        }


        // transfer is good to use for reverting state on error
        msg.sender.transfer(toTransfer);

@filip
I used the address (as spotted in the docs) to get the contract total balance as opposed keeping the total in a state variable.
uint256 toTransfer = address(this).balance;
Is this a newer version or is there maybe a security consideration with using address(this) ?

jon_m · August 25, 2020, 10:08pm

Hi @hannezzon,

This is excellent!

You’ve actually made the assignment harder by adding an additional payable function setMessage. Without this addition, the total contract balance would always be the same as the cumulative balance generated from creating new people with the createPerson function. In this simpler scenario, using a state variable (uint public balance;) to store and record the contract balance by adding the payments made each time a new person is created (with balance = balance + msg.value; or balance += msg.value) … this would return the same balance figure as using address(this).balance i.e. you would only need to use one or the other. In fact, by marking the state variable public, a getter will be created automatically, and so with the first alternative there would be no need for a separate getBalance function. However, the separate function would be needed for the second alternative, because you don’t need to store and record the cumulative balance in a state variable if you use address(this).balance, and so a getter wouldn’t be automatically created.

However, as you have more than one payable function, the cumulative balance stored in your state variable…

… will only include ETH charged for calling the createPerson function, and so you are right to also include a separate getBalanceReport function that returns…

… as this is the total contract balance and will include ETH charged for calling both payable functions.

However, as I’ve mentioned above, because you have marked your state variable createdPersonBalance with public visibility, a separate getter is automatically generated for this balance, and so there is no need to include it in the values returned from your getBalanceReport function. If, on the other hand, you set the visibility of your createdPersonBalance state variable to private, you will also need to return this balance from the getBalanceReport function.

Unfortunately, your code as currently posted will throw a compiler error, as you haven’t declared message as a variable. If you do this, then you should be able to successfully deploy your contract, and the ETH charged for calling the setMessage function will also be added to the total contract balance together with the ETH charged for calling the createPerson function.

Your getBalanceReport function also generates a warning in the compiler:

Warning: This declaration shadows an existing declaration.

This is easily rectified by modifying the name given to the second returns() value in the function header. Even just adding an underscore (uint256 _createdPersonBalance) would resolve this.

Keep on learning!.. you’re making great progress!

jon_m · August 25, 2020, 11:37pm

Hi again @hannezzon,

They are both just alternatives that can be used. However, depending on your particular use case, one may be more appropriate than the other. I think you have demonstrated this particularly well in your assignment solution:

address(this).balance has the advantage of not requiring a state variable to be declared to store and record the changing balance. However, it will only ever return the total contract balance.
storing and recording a balance in a state variable has the advantage of being able to store and record different types of balances (depending on the code you use to update/reassign its value) and not just the total balance of the whole contract.

josephg · August 29, 2020, 4:03am

I added following lines into the code

mapping(address => uint256) public balances;

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);
                            // Check if payment is larger than 1
        balances[owner] += msg.value;

And finally function to call balance of contract

function getBalance() public returns(uint256) {
        return balances[owner];
    }

jon_m · August 29, 2020, 9:01pm

Hi @josephg,

Your solution works
However, it’s an overcomplicated way to do something which should require a lot simpler code.
You’ve used a mapping, but then you only ever store one value in the mapping, and that’s the owner’s balance. This does mean that your mapping keeps a cumulative record of the contract balance, but we can just as easily do this with a single state variable:

uint256 public balance;  // instead of the mapping
balance += msg.value;   // in the createPerson function

In addition, mapping the balance to the owner’s address is actually misleading, because the owner’s address is not the same as the contract address; and until the owner withdraws funds from the contract, their account won’t actually have those funds.

As you have marked your mapping as public (or if we use the simplified code and mark the balance state variable as public as well) a getter will automatically be created and so there is no need for the getBalance function — this only creates duplication. However, if you mark the mapping or state variable as private, then you will need the getBalance function, which you should also mark with view:

uint256 private balance;
// balance increases each time a new person is created in the createPerson function
function getBalance() public view returns(uint256) {
   return balance;
}

Well done for being extremely creative though!

Christopher_Duff · August 31, 2020, 1:44pm

keep getting this error even though I change the value of the contract

“creation of HelloWorld 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.”