checkReward(...) - owner, admin, isForRent

[gundas]

I think the checkReward(...) internal function code is currently too complex - I am really concerned that it will be very difficult to find all the possible bugs/exploits.

One suggestion to simplify it could be:

• make the internal checkReward(...) function to handle only client reward and owner reward cases
• the internal checkReward(...) would separately return owner's reward for the capital and for the connected users, so that in case of IsForRent=true the admin/owner share can be calculated afterwards.

The external checkReward(...) function would take care of calculating the admin/owner reward:

• in case admin calls the external checkReward(...) , it would execute the internal checkReward(...) with owner = true parameter. And distribute the owners reward accordingly (handle the case IsForRent=true).
  • in case an admin is also a client, it would simply execute the internal checkReward(...) once again with a parameter owner=false (or client=true or some other mechanism).

[iFA88]

We need calculate 3 things, with one shared object (structure variable):

• Client reward
• Owner reward
• Revenue from clients

I can not put this into different functions because they are uses the same checkReward_s structure and writes there. It makes no sense that I copy 3 different functions the completely same code what now are in one function.

[gundas]

I was not suggesting 3 different functions - I was suggesting moving some logic out of the internal checkReward(...) function since it is way too complex. I think the admin share calculation could be done outside of this function - the internal checkReward(...) function only needs to know if it is the owner or the client calling it. The external one can take care of handling the admin call.

[iFA88]

We should skip that for the future. I will be happy if everything works and we can put that in one block! I think I can not recode that and put different functions only for see the code better and should not bigger bytecode as now.

[gundas]

I think it is easier to get the code working when it is less complex and the methods are shorter. Another option could be to drop the admin and/or IsForRent features. Or to have only one kind of providers. That would reduce the complexity and the bytecode :)

[gundas]

Hello! I was trying to simplify the checkReward(...) function since it was making my head spin. I've done quite some refactoring, so I am not sure how many new bugs I've introduced :( Please, have a look - I think this version is more readable since the functions are shorter (but most probably it could be simplified further).

The main idea is that checkReward(...) function returns either client reward or provider related reward (which is based on owner user). The getProviderReward(...) internal function does the splitting between owner and admin in case the provider is rented. The getRewardInternal(...) function is handy since it can be used from both external getReward(...) and checkReward(...) functions.

struct checkReward_s {
    uint256 roundID;
    uint256 roundTo;
    uint256 providerSupply;
    uint256 clientSupply;
    uint256 schellingReward;
    uint256 schellingSupply;
    uint8   rate;
    bool    priv;
    uint256 closed;
    uint256 clientPaidUpTo;
    bool getInterest;
    // reward for a client of a provider
    uint256 clientReward;
    //reward for an owner of a private provider
    uint256 ownerReward;
    //revenue from clients
    uint256 providerReward;
}

function getReward(address beneficiary, uint256 providerUID, uint256 roundLimit) readyModule external {
    /*
        Polling the share from the token emission token emission for clients and for providers.

        It is optionaly possible to give an address of a beneficiary for whom we can transfer the accumulated amount. In case we don’t enter any address then the amount will be transfered to the caller’s address.
        As the interest should be checked at each schelling round in order to get the share from that so to avoid the overflow of the gas the number of the check rounds should be limited.
        It is possible to enter optionaly the number of the check rounds.  If it is 0 then it is automatic.

        @beneficiary        Address of the beneficiary
        @limit              Quota of the check rounds.
        @providerUID        Unique ID of the provider
        @reward             Accumulated amount from the previous rounds.
    */
    var _roundLimit = roundLimit;
    var _beneficiary = beneficiary;
    if ( roundLimit == 0 ) { _roundLimit = gasProtectMaxRounds; }
    if ( _beneficiary == 0x00 ) { _beneficiary = msg.sender; }

    var senderStatus = _getSenderStatus(providerUID);
    if ( senderStatus == senderStatus_e.none ) {
        return;
    }

    var (_senderReward, _ownerReward, _adminReward, _round) = getRewardInternal(senderStatus,providerUID, _roundLimit );

    if (_senderReward > 0) {
        require( moduleHandler(moduleHandlerAddress).transfer(address(this), _beneficiary, _senderReward, false ) );
    }

    if (_ownerReward > 0) {
        address _owner = _getProviderOwner(providerUID);
        require( moduleHandler(moduleHandlerAddress).transfer(address(this), _owner, _ownerReward, false ) );
    }
    if (_adminReward > 0) {
        address _admin = _getProviderAdmin(providerUID);
        require( moduleHandler(moduleHandlerAddress).transfer(address(this), _admin, _adminReward, false ) );
    }
}

function getRewardInternal (senderStatus_e senderStatus, uint256 providerUID, uint256 roundLimit) internal
    returns (uint256 senderReward, uint256 ownerReward, uint256 adminReward, uint256 round)
{
    // check client reward
    uint256 clientReward;
    if (senderStatus == senderStatus_e.client || senderStatus == senderStatus_e.adminAndClient) {
        checkReward_s memory _clientData;
        (_clientData, round) = checkReward(msg.sender, true, providerUID, roundLimit);
        clientReward = _clientData.clientReward;
    }

    // check owner/admin reward.
    if (senderStatus == senderStatus_e.adminAndClient || senderStatus == senderStatus_e.admin || senderStatus == senderStatus_e.owner) {
        bool _isForRent = _getProviderIsForRent(providerUID);
        (ownerReward, adminReward, round) = getProviderReward(providerUID, _isForRent, roundLimit);
        // now decide what goes to senderReward
        if (senderStatus == senderStatus_e.owner) {
            senderReward = ownerReward;
            ownerReward = 0;
        } else { // admin
            senderReward = adminReward;
            adminReward = 0;
            // if not for rent, admin also can take owner's reward
            if (!_isForRent) {
                senderReward = safeAdd(senderReward, ownerReward);
                ownerReward = 0;
            } 
        }
    }

    // add the clientReward 
    senderReward = safeAdd(senderReward, clientReward);

    return (senderReward, ownerReward, adminReward, round);
}

function getProviderReward (uint256 providerUID, bool isForRent, uint256 roundLimit) internal 
    returns (uint256 ownerReward, uint256 adminReward, uint256 round) {
    /* 
        Calculates owner or admin reward for the given provider. 
        If provider IsForRent, then the providerReward is split between the admin and owner. Otherwise adminReward will be 0.

    */

    address _owner = _getProviderOwner(providerUID);
    var (_ownerData, _ownerRound) = checkReward(_owner, false, providerUID, roundLimit);

    // split the providerReward to admin and owner depending if it is for rent or not
    if (_ownerData.providerReward > 0) {
        if (isForRent) {
            ownerReward = safeMul(_ownerData.providerReward, rentRate) / 100;
            adminReward = safeSub(_ownerData.providerReward, ownerReward);
        } else {
            // full provider reward
            ownerReward = _ownerData.providerReward;
        }
    }
    safeAdd(ownerReward, _ownerData.ownerReward);

    return (ownerReward, adminReward, _ownerData.roundID);
}

function checkReward(address client, bool isClient, uint256 providerUID, uint256 roundLimit) internal returns(checkReward_s data, uint256 round) {
    /*
        The function can be executed in two modes:
         - when isClient=true, it returns the provider clientReward
         - when isClient=false, it returns the provider's ownerReward (in case the provider is private) and providerReward (revenue from clients)
    */

    data.priv = _getProviderPriv(providerUID);

    // Get paidUps and set the first schelling round ID
    data.clientPaidUpTo = _getClientPaidUpTo(client);
    data.roundID = data.clientPaidUpTo;
    data.roundTo = _getCurrentSchellingRound();
    data.closed = _getProviderClosed(providerUID);
    if ( data.closed > 0 ) {
        data.roundTo = data.closed;
    }

    // load last rate
    data.rate = _getClientLastPaidRate(client);

    // For loop START
    for ( data.roundID ; data.roundID<data.roundTo ; data.roundID++ ) {
        if ( roundLimit > 0 && round == roundLimit ) { break; }
        round = safeAdd(round, 1);
        // Get provider Rate
        data.rate = _getProviderRateHistory(providerUID, data.roundID, data.rate);
        // Get schelling reward and supply for the current checking round
        (data.schellingReward, data.schellingSupply) = _getSchellingRoundDetails(data.roundID);
        // Get provider supply for the current checking round
        data.providerSupply = _getProviderSupply(providerUID, data.roundID, data.providerSupply);
        // Get client/owner supply for this checking round
        if ( data.clientPaidUpTo > 0 ) {
            data.clientSupply = _getClientSupply(client, data.roundID, data.clientSupply);
        }

        // Check, that the Provider has right for getting interest for the current checking round
        data.getInterest = checkForInterest(data.providerSupply, data.priv);

        // Checking if there is any reward
        if (data.getInterest && data.schellingReward > 0 && data.schellingSupply > 0) {
            if (isClient) {
                // calculate client  reward
                if ( data.rate > 0 ) {
                    data.clientReward = safeAdd(data.clientReward, safeMul(safeMul(data.schellingReward, data.clientSupply) / data.schellingSupply, data.rate) / 100);
                }
            } else { // admin/owner
                if ( data.priv ) {
                    // Check for schelling reward for the owner
                    uint256 ownerReward = safeMul(data.schellingReward, data.clientSupply) / data.schellingSupply;
                    data.ownerReward = safeAdd(data.ownerReward, ownerReward);
                }
                // revenue from clients
                // rate (we can not mul with zero)
                if ( data.rate < 100 ) {
                    // in case of private provider, adjust the providerSupply
                    uint256 providerSupply = data.providerSupply;
                    if ( data.priv ) {
                        providerSupply = safeSub(providerSupply, data.clientSupply);
                    } 
                    if ( data.providerSupply > 0 ) {
                        data.providerReward = safeMul(safeMul(data.schellingReward, providerSupply) / data.schellingSupply, safeSub(100, data.rate)) / 100;
                    }
                }
            }
        }
        data.clientPaidUpTo = safeAdd(data.roundID, 1);
    }
    // For loop END

    // Set last paidUpTo, rate and supply
    if ( !isClient || ( isClient && data.priv ) ) {
        _setClientSupply(client, data.clientPaidUpTo, data.clientSupply);
    }
    _setClientPaidUpTo(client, data.clientPaidUpTo);
    _setClientLastPaidRate(client, data.rate);

    //save last provider supply
    _setProviderSupply(providerUID, data.roundID, data.providerSupply);
}

function checkReward(uint256 providerUID, uint256 roundLimit) public constant returns (uint256 senderReward, uint256 adminReward, uint256 ownerReward, uint256 round) {
    /*
        Polling the share from the token emission token emission for clients and for providers.

        It is optionaly possible to give an address of a beneficiary for whom we can transfer the accumulated amount. In case we don’t enter any address then the amount will be transfered to the caller’s address.
        As the interest should be checked at each schelling round in order to get the share from that so to avoid the overflow of the gas the number of the check rounds should be limited.
        It is possible to enter optionaly the number of the check rounds.  If it is 0 then it is automatic.

        @providerUID        Unique ID of the provider
        @roundLimit         Quota of the check rounds.
        @reward             Accumulated amount from the previous rounds.
    */
    var _roundLimit = roundLimit;
    if ( _roundLimit == 0 ) { _roundLimit = _roundLimit-1; } // willfully

    var senderStatus = _getSenderStatus(providerUID);
    if ( senderStatus != senderStatus_e.none ) {
        (senderReward, ownerReward, adminReward, round) = getRewardInternal(senderStatus,providerUID, _roundLimit );
    }

    return (senderReward, adminReward, ownerReward, round);
}

[iFA88]

Sadly it fails the test. The checkReward is really complex. We have now a working version. You doing great work with the "compressing", but please check for bugs because after 6 days this contract would be deployed to the blockchain. We can replace this at any time in the future :)

[gundas]

Which are the tests you are running? I've almost managed to get the tesztelo.html working with testrpc, but some transactions always fail.

[iFA88]

I have updated my repos. You need this special applications for test: https://github.com/iFA88/eth-testrpc https://github.com/iFA88/pyethereum I have now update the tester too. And I run "New contract complex test" with Firefox developer edition.

[gundas]

Hi, thanks! I've installed the packages and I am able to run the tests. However, they always fail at:

<29> Second provider deploy: Wait for miners.. FAILED

The provider[1] contract address is returned, but the check for the code size fails. Maybe you have any idea what could be wrong?

[iFA88]

Yes. It is possible:

• Not enough gas for deploy the contract
• During deploy the constructor function throws