My first Smart Contract project!

Hello Crypto Community!

-After taking the course “Ethereum Smart Contract Programming 101” and hearing Filip on the end of the course saying that the best way to learn is actually to build your own project, I decided to give it a try and make my first project in Solidity.

  1. Why I built it and what it is designed to do?

-The idea is very simple. Contract is designed from the trustee as incentive for the child to have better grades in school in order to get a “prize”.

  1. Who the different users are?

-The users accessing the contract are :

  • The trustee ( the guardian of the child and the contract creator).
  • Child ( this is the user that trustee is registering and it can be as many children as trustee inputs).
  • Professor ( this is the user that is being registered by the trustee and for a specific child).
  1. How specific parts of the code work ? (I included this in the comments in the contract itself).

  2. What problems I had while building it, how I resolved them, and what new coding techniques/solutions I’ve learned:

  • First contract that I created had few lines and I was thinking that this is so easy … I didn’t understand at first why people are saying that coding is so difficult as it seemed easy for me, but only after reaching to @jon_m I understood that my code is interesting as an idea but very far from being a secure Smart Contract and high quality code.

  • Only after I listened and implemented some of the suggestions to develop this idea further it started to look like a more better designed and more structured code and Smart Contract.

  • I used the knowledge from the course that we learned and implemented a lot of the same knowledge in this code. I used to spend hours trying to access different parts of my code and trying to make them work, but still any time that I really couldn’t find a way out I asked for help and once again BIG THANK YOU to @jon_m for helping me out. Withouth your help I am pretty sure this code would not be developed nearly as much.

-I had some issues with implementing “Grades” instance in the “Child” struct
-And how to access some parts in the structs using specific addresses as the mapping keys. But with a bit of help I got to understand it and that we can create and design this ourselves.

  1. What features I changed, and decided to do differently, while building it, and why?
  • The only things I did differently from the beginning of the contract are :
    -I took out Professor struct and mapping and implemented all of the data in the Child struct to keep the code cleaner as it was unnecessary to have so many structs and mapping due to being able to access everything in the way it is now in the code.
    -Instead of professor registering himself I limited this to trustee only modifier
    -And instead of having to call selfDestruct() function to withdraw some funds I decided to implement withdrawFunds() function for the trustee to be able to withdraw any funds in the contract that are currently available.
    (it is really difficult to remember all of the things from the beginning of the contract that I have decided to go differently with, so I just mentioned the ones that I know for sure, but I hope that you will forgive me :stuck_out_tongue: ) .
  1. How I would like to develop it further?
  • I would like to implement some features with time locks, and removing allocateFunds() function so that in this way when specific time or date passes (the end of the school year) the funds are being distributed automatically and not having to be done from the trustee. In this way I think the contract would gain more decentralization.
  • And I would like to build a front-end in Javascript for this to create a Dapp (in this way in the contract in the lockInTheFunds() function it could be set that the amount is supposed to be in Ether or any other amount).

All comments, ideas and suggestions for improvement and other functionality of the contract are more than welcome, please… Feel free to express your thoughts and leave a feedback as everything is about all of us working together and collaborating with one another! :slight_smile:

One more time a huge THANK YOU to @jon_m , withouth whose help I would not be able to bring this contract nearly as far as we have.

P.S (only a small issue the contract has is if the same trustee calls 2 times in a row lockInTheFunds() function , contract balance gets updated, but funds for wihdrawall don’t. The funds would have to be withdrawn using selfDestruct() function). Once again, any comments, ideas and suggestions for improvements are more than welcome. :slight_smile:

This is the first peace of the code and a separate contract (Ownable) .

pragma solidity 0.5.12;

contract Ownable {
    
    address public trustee;
    
    uint public fundsAvailable;
    
    constructor () public {
        trustee = msg.sender;
    }
    
    
     modifier trusteeOnly {
        require(trustee == msg.sender);
        _;
    }
  
    
}

and the second peace.

import "./Ownable.sol";

pragma solidity 0.5.12;

contract Assignment is Ownable{
    
    struct Child {
        string name; //(this is the child's name)
        uint basic; //(basic funds locked up for each child)
        uint bonus; //(bonus locked up for each child)
        string professorName; //(professor's name)
        uint professorBonus; //(bonus locked up for the teacher to be paid on assignment of child’s grades)
        address payable professor; //(teacher responsible for giving each child their grades)
        uint requiredGrade; //(grade required for each child to receive their basic payment)
        uint bonusGrade; //  (grade required for each child to receive their bonus payment)
        Grade grades; //(created instance of the Grade struct added to Child struct so that it can be accessed through children mapping)
    }
    
    
    struct Grade{ 
        uint math;
        uint english;
        uint history;
        uint geography;
        uint physics;
        uint total;
    }
    
    uint public contractBalance;                    //(keeps track of the total funds held in the contract)
    
    mapping (address => Child) internal children;   //(mapping which will for a certain address give us the data we are looking for)
    
      function registerChild(string memory name, address childsAddress) public  trusteeOnly {
        Child memory newChild;
        
        newChild.name = name;
        children[childsAddress] = newChild;
        
         assert(
           keccak256(
               abi.encodePacked(                        //(registerChild function is adding a child to our mapping using childsAddress
                   children[childsAddress].name         // as a parameter that I have set as the key to point to our specific child)
                   ))
                   ==
                   keccak256(
                       abi.encodePacked(
                           newChild.name
                           )
                           )
                           );
        
    }
    
    function lockInTheFunds(uint basicAmount, uint bonusAmount, uint professorBonusAmount,uint requiredGrade, uint bonusGrade, address childsAddress) public trusteeOnly payable {
        require(msg.value == basicAmount + bonusAmount + professorBonusAmount, "The value must be equal as the total of basicAmount + bonusAmount + professorBonusAmount");
            contractBalance += msg.value;
                children[childsAddress].basic = basicAmount;
                children[childsAddress].bonus = bonusAmount;
                children[childsAddress].professorBonus = professorBonusAmount;      //(lockInTheFunds function allows the trustee to pay funds into the contract and allocate specific amounts to the child’s basic 
                children[childsAddress].requiredGrade = requiredGrade;              //  and bonus payments, and the teacher’s bonus payment.
                children[childsAddress].bonusGrade = bonusGrade;                    // These amounts are then locked in until the child receives their grades.)
    }
    
    function registerProfessor(string memory professorName, address payable professorAddress, address childsAddress) public trusteeOnly {
        
        children[childsAddress].professorName = professorName;              // (registerProfessor function is assigning professorName and professorAddress
        children[childsAddress].professor = professorAddress;               // values to a specific childsAddress in the children mapping)
        
    }
    
    
    function insertGrades(address childsAddress, uint math, uint english, uint history,uint geography, uint physics) public  {
        require(children[childsAddress].professor == msg.sender);
        
    Grade memory newGrades;
    
    newGrades.math = math;
    newGrades.english = english;                                            //(insertGrades function can be only called by the professor that is assigned to 
    newGrades.history = history;                                            // a specific Child and this is going to determine weather or not the Child
    newGrades.geography = geography;                                        // is going to be rewarded or not and with how much value)
    newGrades.physics = physics;
    newGrades.total = newGrades.math + newGrades.english + newGrades.history + newGrades.geography + newGrades.physics;
    
    children[childsAddress].grades = newGrades;                             //(in this way we assign a child’s grades to the grades property mapped to their address in the children mapping.)
    }
    
    function allocateFunds(address payable childsAddress) public trusteeOnly payable{
        Child storage child = children[childsAddress];
        
        if (child.grades.total >= child.bonusGrade) {
            childsAddress.transfer(child.basic);
            childsAddress.transfer(child.bonus);
            child.professor.transfer(child.professorBonus);             //( allocateFunds function can only be called by the trustee, and it will automatically
                contractBalance -= child.basic;                             // do the following if/else statements to check for a set of instructions given by the 
                contractBalance -= child.bonus;                             // trustee and distribute the funds accordingly.
                contractBalance -= child.professorBonus;                    // in this way I have decided to incentivise the professor for doing
                    child.basic = 0;                                            //  his part of the job, by giving him a bonus)
                    child.bonus = 0;
                    child.professorBonus = 0;
            
        }
        
        else if (child.grades.total >= child.requiredGrade) {
                 childsAddress.transfer(child.basic);
                 child.professor.transfer(child.professorBonus);
                    contractBalance -= child.basic;
                    contractBalance -= child.professorBonus;
                    fundsAvailable += child.bonus;
                        child.basic = 0;
                        child.bonus = 0;
                        child.professorBonus = 0;
            
        }
        
        else {
            child.professor.transfer(child.professorBonus);
                contractBalance -= child.professorBonus;
                fundsAvailable += child.basic;
                fundsAvailable += child.bonus;
                    child.basic = 0;
                    child.bonus = 0;
                    child.professorBonus = 0;
        }
   
    }
    
    function getChild(address childsAddress) public view returns(string memory childName, uint basic, uint bonus, address payable professorAddress, string memory professorName, uint requiredGrade, uint bonusGrade) {
        return(children[childsAddress].name,
               children[childsAddress].basic,
               children[childsAddress].bonus,
               children[childsAddress].professor,          //(getChild function can be accessed by anyone who has childsAddress, by calling the function
               children[childsAddress].professorName,      // he or she can see all the data listed in this function)
               children[childsAddress].requiredGrade,
               children[childsAddress].bonusGrade);
    }
    
    function getProfessor(address childsAddress) public view returns(uint professorBonus, string memory name){  
        return(children[childsAddress].professorBonus,          
               children[childsAddress].professorName);                     //(same as the getChild function, by entering childsAddress we can get the data of the
    }                                                               // professor that was assigned to that Child)
    
    function getGrades(address childsAddress) public view returns(uint math,uint english, uint history,uint geography, uint physics, uint total){
        return(children[childsAddress].grades.math,
               children[childsAddress].grades.english,
               children[childsAddress].grades.history,                     //(by entering childsAddress in this getGrades function we will get all of the grades 
               children[childsAddress].grades.geography,                   // for this Child)
               children[childsAddress].grades.physics,
               children[childsAddress].grades.total
        );
    }
    
    
    function withdrawFunds(uint amount) public trusteeOnly returns(uint) {
    require(amount <= fundsAvailable, "The amount needs to be less than or equal to funds available to transfer (fundsAvailable)");
        fundsAvailable -= amount;
        contractBalance -= amount;                                      // (this withdrawFunds function is for the trustee to be able to withdraw any surplus
        msg.sender.transfer(amount);                                    // funds that might be available and not (or no longer) locked in for any child or professor)
        return fundsAvailable;
    }
    
    
     function destroy() public trusteeOnly{
    
        selfdestruct(msg.sender);                                   // (And here we have destroy function which in the end trustee can call to selfdestruct the contract
    }                                                               // and transfer any remaining funds inside of the contract to his address)
    
    
    
    
    
    
    
}

Please, let me know what you think! :slight_smile:

4 Likes

Hey @JanjoHR1

Well done!

Few suggestions for you:

  • When you do send funds from your contract to an address, always follow the pattern called Checks -> Effects -> Interactions. This pattern has become famous after the DAO hack which you probably heard about. More info here.
    Let’s take this as example:
if (child.grades.total >= child.bonusGrade) {
            childsAddress.transfer(child.basic);
            childsAddress.transfer(child.bonus);
            child.professor.transfer(child.professorBonus);             //( allocateFunds function can only be called by the trustee, and it will automatically
                contractBalance -= child.basic;                             // do the following if/else statements to check for a set of instructions given by the 
                contractBalance -= child.bonus;                             // trustee and distribute the funds accordingly.
                contractBalance -= child.professorBonus;                    // in this way I have decided to incentivise the professor for doing
                    child.basic = 0;                                            //  his part of the job, by giving him a bonus)
                    child.bonus = 0;
                    child.professorBonus = 0;
            
        }

You should always decrease the balance in the contract before performing a transfer operation.
If you have the Solidity Smart Contract Security course in the Academy I suggest you to do it :slight_smile:
You did that correctly in your function withdrawFunds().

  • uint public fundsAvailable; should not be into the Ownable contract.
    Because the Ownable contract handles the owner functionalities, it should only contain those with no extra code/ data.

Regarding your issue with the withdraw, would you please explain which steps I have to take to reproduce it?

Really really well done here, and congrats for your 1st project.

Happy learning,
Dani

2 Likes

Thank you for this recommendation, I will do this in the real contract, here I will leave it as it is for the other people to see what went wrong :stuck_out_tongue:

I will change this also accordingly… Thank you for the advices :wink:

You need to do the following :

  1. Register child
  2. Register professor
  3. Set amounts and lock in the funds ( here you will see that contractBalance is getting updated but availableFunds don’t because it is locked in for this addresses)
  4. If you repeat the same thing with different or same amount calling lockInTheFunds one more time, contractBalance will increase by that amount, but the addresses amounts won’t increase, they will just be replaced with the new amounts. And availableFunds won’t be updated with the last amount that was paid in the 3rd step. This only happens if you don’t call after the step 3 allocateFunds function, but instead you decide to lockInTheFunds one more time…

Thank you so so much! :slight_smile:

1 Like

Hey @JanjoHR1

If I do understand correctly, the function withdrawFunds() can be used by the trustee to withdraw funds that are not “locked” for child or professor.

If that is correct, I think you are missing this line in your function lockInTheFunds() >> fundsAvailable = fundsAvailable + (msg.value - (basicAmount + bonusAmount + professorBonusAmount));

In this way, you are adding to fundsAvailable the amount of ether that are not locked for both the child and the professor.
You will be able to add funds back to fundsAvailable if the child or the professor do not receive their bonuses (you are doing that in allocateFunds() in your else statements).

I have locked 1 ether and gave 100,100,100 as bonuses and I have now:

Schermata 2020-09-19 alle 18.02.30

I am now going to lock another ether with again 100,100,100 as bonuses:

Schermata 2020-09-19 alle 18.05.50

If I now withdraw all the amount shown in fundsAvailable I have:

Schermata 2020-09-19 alle 18.08.35

I do apologise if I misinterpreted your contract, let me know :slight_smile:

Cheers,
Dani

2 Likes

Hey @JanjoHR1,

Firstly, really well done! :smiley: You’ve done a great job of presenting your project. I want to congratulate you again for all the hard work you’ve put into this over the past few weeks :muscle:

Yes, this is a great recommendation from @dan-i. If you remember from our discussions, you had originally got the right idea and reduced the balances before the transfer, but the problem was that you hadn’t saved the amounts to local variables so that you could still transfer them — when you performed the transfer afterwards you were transferring zero balances.

I think your idea was to replicate the following method from Filip’s video:

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

Here, the balance can be set to 0 before the transfer, because we have saved the balance’s value to a separate local variable (toTransfer), so we still have the value available to transfer after setting the balance to 0. In order to replicate this in your contract you would have to add the additional step of assigning each of the values that are going to be transferred (child.bonus and/or child.basic and/or child.professorBonus) to local variables before setting them to zero.

As you say, you can leave the code you originally posted as it is so that others can follow the issue through the discussion. What I would do now is just post the function after you’ve modified it for these improved security suggestions.

2 Likes

I think both @JanjoHR1 and me :grin: can learn from discussing this point further with you @dan-i . Does this also mean that you wouldn’t put the contractBalance state variable in Ownable, for the same reason?

fundsAvailable records the surplus funds in the contract — those funds within the total contractBalance which are not (or no longer) specifically locked in for potential payment to either the child or teacher on completion of the arrangement. Whereas the funds locked-in (assigned to bonus, basic and professorBonus) can potentially be paid to the child or teacher, funds assigned to fundsAvailable can only be withdrawn by the trustee. So would you not consider this related to “owner functionality” as well as the contractBalance?

1 Like

Dani’s suggestion will sort the issue with the surplus fundsAvailable not increasing. You also mentioned another issue with this:

This is because you are overwriting the existing basic, bonus and teacher’s bonus amounts (locked in when you first called the function) with the amounts input for the new call. If you want to allow this same function to be called multiple times you will also need to change the = assignment operators in the following lines to += so that the amounts are added to, instead of substituting, the exisiting ones.

You could leave the lines…

… as they are, because if you change the grade thresholds, then you will want these to be replaced and not increased.

Just one further point to make about this: why would the trustee want to pay in more than is allocated to basic, bonus and teacher’s bonus, anyway? If this isn’t necessary, then you could change the require statement to…

require(
    msg.value == basicAmount + bonusAmount + professorBonusAmount,
    "Amount paid in must equal the total of the 3 separate Amounts"
);

… and then you won’t need to add the additional line of code that Dani has suggested. However, that’s entirely your decision :slight_smile:

1 Like

Yes @dan-i the solution that you have provided is exactly what I was looking for… Thank you :smiley:

Yes I will change this now… This has been such a great road to take, and I am so happy with what this has finally become. Ofcourse there is more things always to strive and improve further but for now i am just happy and satisfied… Thank you @jon_m for all of you energy and time and patience that you have spent helping me learn and helping me with testing and with everything else.

The following lines and functions have been modified in the code since this topic has been opened:

  1. One state variable have been transferred from one contract to another
uint public availableFunds; // (transferred from Ownable contract to Assignment contract)
  1. In allocateFunds if/else statements have been changed and new uints have been added
    function allocateFunds(address payable childsAddress) public trusteeOnly payable{
        Child storage child = children[childsAddress];
        uint childsBasic = child.basic;
        uint childsBonus = child.bonus;
        uint profBonus = child.professorBonus;
        
        if (child.grades.total >= child.bonusGrade) {
            contractBalance -= child.basic;                             
            contractBalance -= child.bonus;                             
            contractBalance -= child.professorBonus;                                         
                child.basic = 0;                                       
                child.bonus = 0;                                        
                child.professorBonus = 0;
                    childsAddress.transfer(childsBasic);
                    childsAddress.transfer(childsBonus);
                    child.professor.transfer(profBonus);             
        }
        
        else if (child.grades.total >= child.requiredGrade) {
            contractBalance -= child.basic;
            contractBalance -= child.professorBonus;
            fundsAvailable += child.bonus;
                child.basic = 0;
                child.bonus = 0;
                child.professorBonus = 0;
                    childsAddress.transfer(childsBasic);
                    child.professor.transfer(profBonus);
        }
        
        else {
            fundsAvailable += child.basic;
            fundsAvailable += child.bonus;
            contractBalance -= child.professorBonus;
                child.basic = 0;
                child.bonus = 0;
                child.professorBonus = 0;
                    child.professor.transfer(profBonus);
        }
    }
  1. In lockInTheFunds function the following modifications have been made :
//before 
children[childsAddress].basic = basicAmount;
children[childsAddress].bonus = bonusAmount;
children[childsAddress].professorBonus = professorBonusAmount;
//after
children[childsAddress].basic += basicAmount;
children[childsAddress].bonus += bonusAmount;     //(instead of it being = now it is +=)
children[childsAddress].professorBonus += professorBonusAmount;      
//And the error message has been changed from : 
"The value needs to be more than the total of basicAmount + bonusAmount + professorBonusAmount"
// to now being : 
"The value needs to be equal to total of basicAmount + bonusAmount + professorBonusAmount"

Hahahaah fair point… This is also now replaced. Some of the solutions are right here in front of our nose but we just can’t see them…

This is true… I just changed to this because it is way shorter, but both @jon_m you and @dan-i are correct and I just want to thank you for showing both kind of solutions to me :slight_smile:
I am so thankful to both of you guys for helping me out now, this is so exciting! Every day you learn a bit more :smiley:
I think that I have covered here all of the missing parts in the code and that it should work great now :stuck_out_tongue: But still a lot of place for improvement!

1 Like

Sweet! :smiley: You should be very proud of yourself :muscle:

A project is never “finished”, and a developer’s work is never “done” :wink:

So, just to confirm your final decision with the lockInThe Funds function:

  • It can be called more than once, and so the funds allocated to each of the different amounts (child’s basic and bonus, and prof’s bonus) can be increased, or the required grade thresholds, can be changed by the trustee;
  • But the funds paid in must always be equal to the sum of these 3 separate amounts:
    require(
        msg.value == basicAmount + bonusAmount + professorBonusAmount,
        "Error message"
    );
  • In this way, surplus funds available for withdrawal by the trustee can only be generated at the end of each arrangement if the child’s grades are below either of the required thresholds.

Have I understood correctly?

2 Likes

Correct :white_check_mark: :slight_smile:

Yes you did… This is very well explained and for me makes also the most sence :smiley:

1 Like