appendSupplyChanges(...) question/suggestion

[gundas]

Question

// The public provider owners supply are not calculated in the provider supply, but we need set the last supply ID's https://github.com/CORIONplatform/solidity/blob/provider-recode/provider.sol#L1294

If I have 5000 CORION and I create (become an owner of) a public Provider, I would expect that I would get emission reward for the 2000 CORION ((5000-3000=2000). The current implementation does not pay emission rewards to an owner of a public provider no matter what is the balance.

Suggestion The appendSupplyChanges(...) function handles the following events:

• open private provider
• close public/private provider
• join provider
• part provider
• transfer between users (the user could also an owner of public or private Provider)

The function is very complex and difficult to assess if it is correct in all of the use cases. Maybe it would be possible to separate this function into multiple functions to handle more specific cases - e.g.:

• open provider
• close provider
• join provider
• part provider
• handle transfers - the major complication here if there is an owner of a provider involved. For me it is still not clear what happens if a user creates a provider and transfers all of his balance to another account. I think the user's balance should be reduced by 3000 or 8000 CORION when he creates a provider. He could get it back then provider is closed.

[iFA88]

With public provider the owner don't get any release for her/his founds. The appendsupplyChanges function handles the following:

• joinToProvider
• partFromProvider
• closeProvider
• transferFrom
• transferTo

I think this is all case event what we can "compress" into this function. If you open a provider then you must join to too in the providerDB.

If an user creates a provider, he can not transfer tokens that he/his would below the provider open limit. That means if i have 10K token and create a provider, I can not transfer more than 7K token. The transaction would be throw.

[gundas]

If an user creates a provider, he can not transfer tokens that he/his would below the provider open limit. That means if i have 10K token and create a provider, I can not transfer more than 7K token. The transaction would be throw.

This is currently not implemented yet or am I missing something?

[iFA88]

This cutie lines does that :) : https://github.com/CORIONplatform/solidity/blob/provider-recode/provider.sol#L1367-L1378

[gundas]

But only for a private provider, because of this check: https://github.com/CORIONplatform/solidity/blob/provider-recode/provider.sol#L1351

[iFA88]

You have right! I need fix this!

[gundas]

Could we replace the whole https://github.com/CORIONplatform/solidity/blob/provider-recode/provider.sol#L1355-L1375 with:

_clientSupply = getTokenBalance(client);

?

[iFA88]

This seems okay:

        // Client supply changes
        _clientSupply = getTokenBalance(client);
        if ( supplyChangeType != supplyChangeType_e.closeProvider && ( client != _owner || ( client == _owner && _priv ) ) ) {
            _setClientSupply(client, _clientSupply);
        }
        // check owner balance for the provider limits
        if ( supplyChangeType == supplyChangeType_e.transferFrom && client == _owner ) {
            require( ( _priv && _clientSupply >= minFundsForPrivate ) ||( ( ! _priv ) && _clientSupply >= minFundsForPublic ));
        }

[gundas]

What do you think of separating the shelling supply calculation. For example (this is not a test code):

function appendSupplyChanges(address client, supplyChangeType_e supplyChangeType, uint256 amount) internal {
    // egyenleg valtozott es be kell allitani ezt mindenkinel!
    var providerUID = _getClientProviderUID(client);
    if ( providerUID == 0 || _getProviderClosed(providerUID) > 0) { return; }

    var _providerSupply = _getProviderSupply(providerUID);
    var _priv = _getProviderPriv(providerUID);

    if (supplyChangeType == supplyChangeType_e.closeProvider) {
        appendSupplyChanges_shellingSupply(_providerSupply, _providerSupply, _priv, false);
    } else {
        var _owner = _getProviderOwner(providerUID);
        uint256 _clientSupply = getTokenBalance(client);

        // check owner balance for the provider limits
        if ( supplyChangeType == supplyChangeType_e.transferFrom && client == _owner ) {
            require( ( _priv && _clientSupply >= minFundsForPrivate ) ||( ( ! _priv ) && _clientSupply >= minFundsForPublic ));
        }

        // The public provider owners supply are not calculated in the provider supply, but we need set the last supply ID's
        if ( _owner != client || ( _owner == client && _priv )) {

            if (supplyChangeType == supplyChangeType_e.joinToProvider || supplyChangeType == supplyChangeType_e.transferTo) {
                appendSupplyChanges_shellingSupply(providerUID, _providerSupply, amount, _priv, true);
                _providerSupply = safeAdd(_providerSupply, amount);
            } else if (supplyChangeType == supplyChangeType_e.partFromProvider || supplyChangeType == supplyChangeType_e.transferFrom) {
                appendSupplyChanges_shellingSupply(providerUID, _providerSupply, amount, _priv, false);
                _providerSupply = safeSub(_providerSupply, amount);
            }
            _setProviderSupply(providerUID, _providerSupply);
            // maybe we should always set the client supply (also for the owner of public provider?)
            _setClientSupply(client, _clientSupply);
        }
    }
}

function appendSupplyChanges_shellingSupply(uint256 providerSupply, uint256 amount, bool priv, bool add) internal{
    var _schellingSupply = _getSchellingRoundSupply();
    DEBUG(103, providerSupply);
    DEBUG(103, _schellingSupply);
    rightForInterest_e rightForInterest;
    if ( add ) {
        rightForInterest = checkForInterest(providerSupply, safeAdd(providerSupply, amount), priv);
        DEBUG(106, uint256(rightForInterest));

    } else {
        rightForInterest = checkForInterest(providerSupply, safeSub(providerSupply, amount), priv);
        DEBUG(107, uint256(rightForInterest));
    }
    if ( rightForInterest == rightForInterest_e.yes_yes ) {
        if ( add ) {
            _schellingSupply = safeAdd(_schellingSupply, amount);
            DEBUG(108, _schellingSupply);
        } else {
            _schellingSupply = safeSub(_schellingSupply, amount);
            DEBUG(109, _schellingSupply);
        }
    } else if ( rightForInterest == rightForInterest_e.yes_no ) {
        _schellingSupply = safeSub(_schellingSupply, providerSupply);
        DEBUG(110, _schellingSupply);
    } else if ( rightForInterest == rightForInterest_e.no_yes ) {
        _schellingSupply = safeAdd(_schellingSupply, safeAdd(providerSupply, amount));
        DEBUG(111, _schellingSupply);
    } else if ( rightForInterest == rightForInterest_e.no_no ) {
        // nope
        DEBUG(112, 0);
    }

    _setSchellingRoundSupply(_schellingSupply);
    DEBUG(116, _schellingSupply);
}

[iFA88]

Why should we do that? I have cut out now the debug things. To deploy the contract I need 4.56 M gas. This is really dangerous that we can not put that in one block :/

[gundas]

It is possible to get rid of the entire rightForInterest_e enum and simplify the logic of the schelling round supply calculation :

function appendSupplyChanges(address client, supplyChangeType_e supplyChangeType, uint256 amount) internal {
    // egyenleg valtozott es be kell allitani ezt mindenkinel!
    uint256 _clientSupply;
    var providerUID = _getClientProviderUID(client);
    if ( providerUID == 0 || _getProviderClosed(providerUID) > 0) { return; }
    var _priv = _getProviderPriv(providerUID);
    var _owner = _getProviderOwner(providerUID);
    // The public provider owners supply are not calculated in the provider supply, but we need set the last supply ID's
    if ( _owner != client || ( _owner == client && _priv ) || supplyChangeType == supplyChangeType_e.closeProvider ) {
        var _providerSupply = _getProviderSupply(providerUID);
        uint256 _providerSupplyNew = 0;  // in case of closeProvider
        if ( supplyChangeType == supplyChangeType_e.joinToProvider || supplyChangeType == supplyChangeType_e.transferTo ) {
            _providerSupplyNew = safeAdd(_providerSupply, amount);
        } else if ( supplyChangeType == supplyChangeType_e.transferFrom ){
            _providerSupplyNew = safeSub(_providerSupply, amount);
        } 
        // set provider supply - the previous code used to set provider supply only if it is not closed. 
        _setProviderSupply(providerUID, _providerSupplyNew);

        // adjust the schelling supply - could be moved to a new function
        var _schellingSupply = _getSchellingRoundSupply();
        // check if the provider used to get interest - if so, remove it from the schelling supply
        if (checkForInterest(_providerSupply, _priv)) {
            _schellingSupply = safeSub(_schellingSupply, _providerSupply);
        }
        // check if the provider should get interest now
        if (checkForInterest(_providerSupplyNew, _priv)) {
            _schellingSupply = safeAdd(_schellingSupply, _providerSupplyNew);
        }
        _setSchellingRoundSupply(_schellingSupply);
    }
    // Client supply changes
    _clientSupply = getTokenBalance(client);
    if ( supplyChangeType != supplyChangeType_e.closeProvider && ( client != _owner || ( client == _owner && _priv ) ) ) {
        _setClientSupply(client, _clientSupply);
    }
    // check owner balance for the provider limits
    if ( supplyChangeType == supplyChangeType_e.transferFrom && client == _owner ) {
        require( ( _priv && _clientSupply >= minFundsForPrivate ) ||( ( ! _priv ) && _clientSupply >= minFundsForPublic ));
    }
}

function checkForInterest(uint256 supply, bool priv) internal returns (bool) {
    return ( ! priv && supply > 0) || ( priv && interestMinFunds <= supply );
}

[iFA88]

• A provider is private and has 50k funds. The Schelling round supply is now 50K.

• C Client joins to A provider with 20K funds.

  _providerSupplyNew = safeAdd(_providerSupply, amount);

  providerSupplyNew would be 70K

  if (checkForInterest(_providerSupplyNew, _priv)) {
  _schellingSupply = safeAdd(_schellingSupply, _providerSupplyNew);
  }

  schellingSupply would be 120K what is wrong!

[gundas]

50K is subtracted (see lines above) before 70K is added, so result will be 70K.

[iFA88]

Right! Nice work! I will run a test on it.

[iFA88]

Sadly the test are failed.. Non private non isForRent provider with 2 clients. Provider supply is 1040039837276 ION One of the clients leaves the provider and the supply need to be 1032508042095 ION but its zero!

<116> Open type 3 custom provider with Foundation: Passed
<117> Join to Foundation provider with Pista: Passed
<118> Wait 1 schelling round delay: Passed
<119> New schelling round with release: Passed
<120> Get reward from Foundation provider with Pista: Passed
<121> Get reward from Foundation provider with Pista (should be failed): Passed
<122> Get reward from Foundation provider with Foundation: Passed
<123> Get reward from Foundation provider with Foundation (should be failed): Passed
<124> Get reward from Foundation provider with iFA (should be failed): Passed
<125> Set Foundation provider rate to 60%: Passed
<126> Wait 1 schelling round delay: Passed
<127> New schelling round with release: Passed
<128> Set Foundation provider rate to 70%: Passed
<129> Get reward from Foundation provider with iFA: Passed
<130> Get reward from Foundation provider with iFA (should be failed): Passed
<131> Get reward from Foundation provider with Pista: Passed
<132> Get reward from Foundation provider with Pista (should be failed): Passed
<133> Get reward from Foundation provider with iFA (should be failed): Passed
<134> Get reward from Foundation provider with Foundation (should be failed): Passed
<135> Join to Foundation provider with iFA: Passed
<136> Get reward from Foundation provider with iFA (should be failed): Passed
<137> Wait 1 schelling round delay: Passed
<138> New schelling round with release: Passed
<139> Get reward from Foundation provider with iFA: Passed
<140> Get reward from Foundation provider with Foundation (should be failed): Passed
<141> Get reward from Foundation provider with iFA (should be failed): Passed
<142> Get reward from Foundation provider with Pista: Passed
<143> Get reward from Foundation provider with Pista (should be failed): Passed
<144> Wait 1 schelling round delay: Passed
<145> New schelling round with release: Passed
<146> Part from Foundation provider with iFA (should be failed): Passed
<147> Part from Foundation provider with Pista (should be failed): Passed
<148> Get reward from Foundation provider with Pista: Passed
<149> Get reward from Foundation provider with Pista (should be failed): Passed
<150> Get reward from Foundation provider with iFA: Passed
<151> Get reward from Foundation provider with Foundation (should be failed): Passed
<152> Part from Foundation provider with Pista: Check provider # 4.. Supply.. FAILED

[iFA88]

Maybe this missing

} else if ( supplyChangeType == supplyChangeType_e.transferFrom || supplyChangeType == supplyChangeType_e.partFromProvider ) {

[gundas]

Yes, I think you are right.

One more subtle changes in the behavior is when provider is closed (but that is another case):

// set provider supply - the previous code used to set provider supply only if it is not closed. 
_setProviderSupply(providerUID, _providerSupplyNew);

[iFA88]

I have used this for that:

if ( supplyChangeType != supplyChangeType_e.closeProvider ) { 
    _setProviderSupply(providerUID, _providerSupplyNew);
}

[iFA88]

Yep, test passed :) Good job!

[gundas]

I would propose another simplification - fully get rid of supplyChangeType_e. If we separate close provider action from appendSupplyChanges(..) function, then there is no difference between supplyChangeType_e.joinToProvider and supplyChangeType_e.transferTo. The entire supplyChangeType_e can be handled by boolean flag (add/remove funds):

function closeProvider() readyModule external {
    /*
        Closing and inactivate the provider.
        It is only possible to close that active provider which is owned by the sender itself after calling the whole share of the emission.
        Whom were connected to the provider those clients will have to disconnect after they’ve called their share of emission which was not called before.
    */
    var providerUID = _getClientProviderUID(msg.sender);
    require( providerUID > 0 );
    require( _getProviderOwner(providerUID) == msg.sender );
    require( _isClientPaidUp(msg.sender) );
    var _providerSupply = _getProviderSupply(providerUID);
    var _priv = _getProviderPriv(providerUID);
    appendSupplyChanges_schellingSupply (_providerSupply, 0, bool priv);

    _closeProvider(msg.sender);
    EProviderClose(providerUID);
}

function appendSupplyChanges(address client, bool add, uint256 amount) internal {
    uint256 _clientSupply;
    var providerUID = _getClientProviderUID(client);
    if ( providerUID == 0 || _getProviderClosed(providerUID) > 0) { return; }
    var _priv = _getProviderPriv(providerUID);
    var _owner = _getProviderOwner(providerUID);
    // The public provider owners supply are not calculated in the provider supply, but we need set the last supply ID's
    if ( _owner != client || ( _owner == client && _priv )) {
        var _providerSupply = _getProviderSupply(providerUID);

        uint256 _providerSupplyNew;
        if ( add ) {
            _providerSupplyNew = safeAdd(_providerSupply, amount);
        } else {
            _providerSupplyNew = safeSub(_providerSupply, amount);
        }
        _setProviderSupply(providerUID, _providerSupplyNew);

        appendSupplyChanges_schellingSupply(_providerSupply, _providerSupplyNew, _priv);
    }
    // Client supply changes
    _clientSupply = getTokenBalance(client);
    if ( client != _owner || ( client == _owner && _priv ) )  {
        _setClientSupply(client, _clientSupply);
    }
    // check owner balance for the provider limits
    if ( !add && client == _owner ) {
        require( ( _priv && _clientSupply >= minFundsForPrivate ) ||( ( ! _priv ) && _clientSupply >= minFundsForPublic ));
    }
}

function appendSupplyChanges_schellingSupply (uint256 providerSupply, uint256 providerSupplyNew, bool priv) internal {
    var _schellingSupply = _getSchellingRoundSupply();
    // check if the provider used to get interest - if so, remove it from the schelling supply
    if (checkForInterest(providerSupply, priv)) {
        _schellingSupply = safeSub(_schellingSupply, providerSupply);
    }
    // check if the provider should get interest now
    if (checkForInterest(providerSupplyNew, priv)) {
        _schellingSupply = safeAdd(_schellingSupply, providerSupplyNew);
    }
    _setSchellingRoundSupply(_schellingSupply);
}

[iFA88]

Sadly I can not handle openProvider without the enumeration. I got every time stack to deep, and I can not remove more variable:

    struct newProvider_s {
        uint256 balance;
        uint256 newUID;
        uint256 zero;
    }
    function openProvider(bool priv, string name, string website, uint256 country, string info, uint8 rate, bool isForRent, address admin) readyModule external {
        newProvider_s memory _newProvider;
        _newProvider.balance = getTokenBalance(msg.sender);
        checkCorrectRate(priv, rate);
        require( admin != msg.sender );
        require( ( ! isForRent ) || ( isForRent && admin != 0x00) );
        openProviderCheckFounds(_newProvider.balance, priv);
        // require( ( priv && ( _newProvider.balance >= minFundsForPrivate )) || ( ! priv && ( _newProvider.balance >= minFundsForPublic )) );
        _newProvider.newUID = _openProvider(priv, name, website, country, info, rate, isForRent, admin);
        _joinToProvider(_newProvider.newUID, msg.sender);
        appendSupplyChanges_schellingSupply(_newProvider.zero, _newProvider.balance, priv);
            //appendSupplyChanges(msg.sender, supplyChangeType_e.joinToProvider, _newProvider.balance);
        EProviderOpen(_newProvider.newUID);
    }
    function openProviderCheckFounds(uint256 bal, bool priv) {
        require( _getClientProviderUID(msg.sender) == 0x00 );
        require( ( priv && ( bal >= minFundsForPrivate )) || ( ! priv && ( bal >= minFundsForPublic )) );
    }

[gundas]

Hi, Not sure about stack too deep, but my intention was to change openProvider from:

if ( priv ) {
     appendSupplyChanges(msg.sender, supplyChangeType_e.joinToProvider, _newProvider.balance);
}

to

if ( priv ) {
     appendSupplyChanges(msg.sender, true, _newProvider.balance);
}

Similarly, partProvider(...) would call:

appendSupplyChanges(msg.sender, false, _supply);

and transferEvent(...):

appendSupplyChanges(from, false, value);
appendSupplyChanges(to, true, value);

[iFA88]

Alright, test passed! Nice work!