My First Contract : TimeClock service delivery / labor hire contract

Hi All,

I've developed my first Ethereum smart contract and am looking for any feedback or suggestions. Its a service delivery contract that tries to solve the problems of:
- How as a freelancer do I know I'm going to get paid?
- How as a contractor do I know I'm going to get work done properly without losing all my money?

There are many other use cases as well as these.

I'm looking for help with a code review, security review or suggestions.

The source code is here: https://github.com/dmozzy/TimeClock and is fully opensourced under the MIT licence.

Comments

  • o0ragman0oo0ragman0o Posts: 1,251Member, Moderator mod
    Hi @danmosc. Nice tidy looking contract. I haven't time to look through it deeply ATM but notice there's are a couple of Solidty features such as accessor functions, modifiers and globals you could make use of. I don't know if your aware of them and have simply chosen not to code in that style. There's not real difference. But if you're not aware, here's my two cents...

    Accessor functions take the place of getters and generated for public state variable and even complex state variables such as arrays mappings and structs. They save come coding though I like to include but the function signatures in my source just to make it obvious how an accessor is called.

    uint public myState;

    // function myState() public constant returns (uint);

    mapping (address => mapping ( uint => address)) public myComplexState;

    // function myComplexState(address a, uint b) public constant returns (address);
    Modifiers are a way of abstracting validation predicates which can make the intent of the state mutation or purpose of the function itself more readable.

    uint public sumNum;
    address public owner;

    function add(uint num) public return (uint) {
    if (msg.sender != owner) throw; //
    We can abstract the `owner` validation check into its own modifier making the function itself more intuitive

    uint public sumNum;
    address public owner;

    modifier isOwner() {
    if (msg.sender != owner) throw;
    _
    }

    function add(uint num) public isOwner return (uint) {

    sumNum += num;
    return sumNum;
    }
    Sensible naming of modifiers can also make the function definition more intuitive. Say you use a mutex for reentry protection of state mutating functions (I highly recommend) You can chain the modifiers to almost read as a sentance...

    modifier isOwner() {
    if (msg.sender != owner) throw;
    _
    }

    modifier canEnter() {
    if (mutex) throw; // Checks if contract has already been entered.
    _
    }

    function protectedFunc() public onlyOwner canEnter {
    // some stuff
    }
    There's also a number of handy global units in Solidity. For the expected time frames in the contract, it might be a little more readable to use the global time units such as `hours` or `days` rather than variables in seconds. Though for purity sake I'd probably tend toward base units too and just let the front end handle conversions.

    I'll have a closer look tomorrow.
  • danmoscdanmosc Posts: 3Member
    Many thanks @o0ragman0o. Wasn't expecting such helpful feedback so quickly!

    Re: Modifiers, I left these out because I haven't seen this in other programming languages and I didn't fully understand them. They don't seem to tricky so will give them a go. Agree that it makes the contract more readable.

    I had a few problems with accessors around arrays and mappings. Not sure if this was a truffle issue or I just didn't fully understand it. In the end I went for 'getContractee (uint number)' to get the data from the Contractee array as a workaround. I can give this another go now that I know it should work. I have used accessors around the other state variables (e.g. contractorAddress).

    Am interested in the mutex concept. I must be missing something though as I'm not sure if you set this as a state variable (and unset it at the end of the function?). Can you point me to an example? I did significant work trying to avoid the reentrant issues by separating state changes and the sending of ether. Hopefully I got them all.

    Again thanks for looking over this, much appreciated.

  • o0ragman0oo0ragman0o Posts: 1,251Member, Moderator mod
    edited November 2016
    @danmosc This is the secure patterning that I use. It consists of mutex reentry protection but as this prevents a contract from calling it's own public functions, I abstract state mutation logic into internal functions and just use the publics for entry validation (using modifiers) and parameter processing. This pattern also makes deriving reentry protected contracts much easier as you don't need to rewrite the state mutation logic when overloading public functions.

    contract Protected {

    bool mutex;
    uint count; // example state variable
    The contract can be protected by using this modifier on functions containing external call/send.

    modifier protected() {

    if (mutex) throw; // Checks if contract has already been entered.
    mutex = true;
    _
    delete mutex; // sets to false and refunds some gas
    return;
    }
    This modifier can be added to all other non constant public functions to prevent reentry

    modifier canEnter() {
    if (mutex) throw; // Checks if contract has already been entered.
    _
    }
    No return is used in the `protected` function because the modifier itself handles the return.
    This means that return values need to be set in return parameters, e.g. 'success_' (note that as a personal convention, I use leading or trailing underscores to differentiate function and return parameters)

    function safeSend(address _to, uint _amount) public protected returns (bool success_) {
    if (!_to.send(_amount)) throw;
    success_ = true;
    }
    All other publics work normally and only need the `canEnter` modifier.

    function countMe() public canEnter returns (uint) {
    // some messy state stuff which can only be entered if mutex is false

    count += 1;
    return count; // returns work normally with return type ans return parameters
    }
    Constant public functions don't need protection as they don't mutate state

    function double() public constant return (uint) {
    return 2 * count;
    }
    Protected publics can no longer call each other as if the `external` keyword was used rather than `public` so it makes more sense to just use `external` on protected publics for another point of unambiguity. Then we can imply that `external` is likely to mutate state while `public` is also likely to be `constant` (but don't count on it!)
    This function will cause a throw when it tries to call already protected `count()`

    function countMeToo() external canEnter returns (uint) {

    countMe(); // cant reenter
    countMe();
    return count;
    }
    So a better pattern it to move state maintenance into internal function and use public/external functions for entry validation logic and parameter preparation.

    function countMeInternal() internal returns (uint) {
    count += 1;
    return count;
    }
    Both these protected publics can now mutate state in different ways.
      
    function countMe() external canEnter returns (uint) {
    return countMeInternal();
    }

    function countMeToo() external canEnter returns (uint) {
    countMeInternal()
    countMeInternal()
    return count;
    }
    Overloading inherited functions with additional entry validation or parameter possessing is also easily achieved without messing with the parent state logic.

    contract Derived is Protected {

    address owner; // presume there is a constructor to set this.
    modifier onlyOwner() {
    if (sender != owner) throw;
    }
    We can now overload the base contract's public functions with additional entry validation without having to rewrite state mutation logic

    function countMe() external onlyOwner canEnter returns (uint) {
    return countMeInternal();
    }
  • danmoscdanmosc Posts: 3Member
    Thanks again @o0ragman0o, much appreciated.

    Let me digest and update the contract with your improvements. I can now see how the modifiers can prevent the re-entrant issues. Is also much simpler than what I was trying to do.
Sign In or Register to comment.