JoinColony / colonyNetwork

Colony Network smart contracts
https://colony.io/
GNU General Public License v3.0
438 stars 106 forks source link

Make use of external functions #569

Open elenadimitrova opened 5 years ago

elenadimitrova commented 5 years ago

External functions can be used to save gas when large arrays of data are being received. This is especially true for the reputation mining process where such large input arrays are accepted. An experimental branch was pushed that started testing this which shows 1,000 gas savings in confirmJustificationRootHash (down from 139140 to 138377).

Note that to enable multi-dimensional array to be accepted in an external function, the ABIEncoderV2 has to be enabled. See documentation note here

Currently we use dynamic size input array of bytes in some external candidate reputation functions which blocks their switch to external, however this will potentially be unblocked when #393 is done.

area commented 5 years ago

While doing #393 with an eye on unblocking this issue, I refactored to a point where in principle, we should have been able switch to external on respondToChallenge. However, doing so gave us the dreaded 'stack too deep' error. Elena found this, and it seems logical to assume the same issue applies to bytes32[] which... we have an awful lot of in that function call. The only solution at this juncture would seem to be compressing all of our bytes32[]s in to one giant bytes32[], but we've opted to not do that for intelligibility purposes right now. We're going to be the only entity reputation mining at the start, so shouldn't need to use this function so we can hold off such drastic refactoring action for now.

(Note that we also need to remove the lines

u[U_DECAY_TRANSITION] = 0;
    u[U_GLOBAL_CHILD_UPDATE] = 0;
    u[U_NEW_REPUTATION] = 0;

as calldata is read only. Two of these are fairly clean to the remove, but the third is less so and should warrant more consideration when this is being done).

dev1644 commented 5 years ago

Does this issue require some work or is it fixed?

elenadimitrova commented 5 years ago

As per @area 's comment above it's iceboxed for now.

dev1644 commented 5 years ago

Are you guys waiting for the ABIEncoderv2 to be released for this issue?

elenadimitrova commented 5 years ago

No, we have a "stack too deep" issue, like I said, read the last comment for details.