setClientSupply() and setProviderSupply() - change supply for the current schelling round

[gundas]

There was a recent code change in providerDB - it is not possible anymore to change the supply for a Client/Customer for the current schelling round:

        if ( ( schellingRound == currentSchellingRound && ( ! providers[providerUID].supply[schellingRound].valid )  ) ||
            schellingRound != currentSchellingRound ) {
            providers[providerUID].supply[schellingRound].amount = amount;
            providers[providerUID].supply[schellingRound].valid = true;
        }

I think this is problematic - multiple transfers can occur, users can join and part providers within the timespan of a single schelling round. Their (and providers') supply should be updated accordingly.

I think that it should be the opposite - previous schelling rounds' supply should be not changeable.

[iFA88]

The transaction execute runs on an single line/thread. There is no way to do something what the next or the previous transaction would be change and gives an non deterministic return.

https://github.com/CORIONplatform/solidity/blob/provider-recode/provider.sol#L153-L159 With this function you can set the client supply for the current Schelling round.

[iFA88]

If we use your example ( in big lines ): In the same block:

• First transaction: Client joins to the 2# provider with 1M COR. Schelling supply and provider supply will be increased with 1M, and the client supply would be changed to 1M.
• Second transaction: Clients parts from the 2#. Schelling supply and provider supply would be decreased with 1M, and the client structure would be reset.

[gundas]

https://github.com/CORIONplatform/solidity/blob/provider-recode/provider.sol#L153-L159 With this function you can set the client supply for the current Schelling round.

I've missed that other function and thought that there is no way to change the supply once it is set. My bad.

[gundas]

I am still a little bit puzzled. The checkReward(...) internal method at the very end sets the Client supply and the Provider supply. For Provider Depending if data.roundID==data.currentSchellingRound the supply is either updated or not. For Client - a similar story.

Later, the external checkReward(...) function executes transfers to distribute rewards, thus the Client/Provider supplies are updated again.

Since there is no compound reward, it seems strange to even have a possibility to update the Client/Provider supplies for the previous schelling rounds (that would be the case in internal checkReward(...) function if the rounds are limited and data.roundID does not run-up to the current schelling round).

I think all changes to Client/Provider supply should happen only in appendSupplyChanges(...) and only for the current schelling round. The checkReward(..) function should not update neither Client nor Provider supplies.

[gundas]

OK, I kind of understand the logic - client supply is set in internal checkReward(...) so that the next call to it can simply retrieve the supply without iterating through history. In my opinion this is really complex and fragile. I really think that the code should be refactored into small and easy to understand functions and only then it should be optimized for the cost/bytecode size etc.

[iFA88]

Yes you see that correct. We need "save" the supply for the next reward call. If the paidUpTo is the current Schelling round we don't edit that, because we don't want to overwrite his/her current supply for an old. This will appendSupplyChanges do, changing the supply for an valid with reward.

I really think that the code should be refactored into small and easy to understand functions and only then it should be optimized for the cost/bytecode size etc.

I'm really curious how can anyone do that. In the first step I'm happy that it works as excepted. I think no one contract is so complex in any Ethereum system like this.

[gundas]

I'm really curious how can anyone do that. In the first step I'm happy that it works as excepted. I think no one contract is so complex in any Ethereum system like this.

Ethereum smart contracts are only trusted because their code can be checked. If the code is very difficult to read the users will be less keen on trusting the contract. I had really hard time understanding the provider contract - I think it is too complex to be evaluated if it works correctly in all (creative) border cases and if it can withstand some creative abuse.

I think:

• the methods should be short and have a clear (preferably single) meaning https://refactoring.guru/smells/long-method.
• conditional statements should not be nested too deep (.e.g. https://blog.codinghorror.com/flattening-arrow-code/
• if the above is fixed there should be less remaining (not checked) assumptions that the data is in some particular state

In my opinion, if methods are short and clear, it is much easier to check the correctness of the code.

It is not for me to decide, but I would create the simplest Provider contract first - no admin feature, no Private feature etc. Get it working clean/refactor the code so it is clear and easy to understand. Optimize it for blockchain etc. Then I would start considering adding extra features like admin and Private provider without complicating the existing code so the conditional statements and methods functionality are kept to the minimum.

I understand this would take some weeks or maybe a month to develop/refactor, but the result would be the code which is much easier to read and understand.

[iFA88]

I fully understand your logic. The bad with 'updating' the provider contract, that we need deploy a new database for the new functions. With the new DB every user would lost their data, the providers need campaign again whit his/her provider and need build a new client 'area'.. this should not happen!

We have the full provider with every functions, we should use that and not drop the features out. Like an car, I buy a Porsche and remove the turbo from it :)