Skip to content

Conversation

@jijordre
Copy link

@jijordre jijordre commented May 19, 2019

This PR adds dependency on solc@^0.5.0 a.o. by introducing breaking changes such as address payable (see breaking changes).

An additional branch feature/truffle exists that on top of this transforms the project into one based on Truffle.

@willitscale
Copy link
Owner

Thanks for putting on the pull request, but I've got 2 concerns; the first is the indentation of the function definitions doesn't align with any coding standard I'm familiar to and the second is the change to the split method as this had a pretty hefty gas cost before hand. What's the benefit of the additional while loop?

@jijordre
Copy link
Author

The indentation is one that comes out of the box using IntelliJ. I suspect you refer in particular to indentation in Strings.sol, right. I'll update this right away.

The benefit of the additional while loop is to work around the lack of support for .push() on dynamic memory arrays. If you could suggest an alternate approach I'd be happy to implement accordingly.

I see that in the alternate PR #6 a different approach has been taken by providing the split array as storage parameter. Not sure that I like this approach, at least not as the only solution. But possibly these two split implementation could complement one another.

@jijordre jijordre force-pushed the feature/solc-latest branch from 500cb00 to a82e770 Compare May 20, 2019 08:34
@willitscale
Copy link
Owner

Do you know the gas cost difference with your change e.g. before/after by percentage?

@jijordre
Copy link
Author

jijordre commented May 23, 2019

I did some benchmarking with a few different configurations. I used function testSplit() in StringsTests.sol in the mentioned branch feature/truffle as test logic.

The first, "new" configuration is identical to the one in feature/truffle with the exception of deployment being done to Ganache v2.0.1. The configuration is summarized by Truffle:

> ./node_modules/.bin/truffle version
Truffle v5.0.19 (core: 5.0.19)
Solidity v0.5.0 (solc-js)
Node v10.13.0
Web3.js v1.0.0-beta.37

In the second, "old" configuration I downgraded Truffle and hence also solc:

> ./node_modules/.bin/truffle version
Truffle v4.1.15 (core: 4.1.15)
Solidity v0.4.25 (solc-js)

And naturally in this configuration I used your master Strings.sol.

The result to my surprise shows a decrease of gas cost, from 153246 in the "old" configuration to 63371 in the "new" configuration, i.e. down more than 58%. I reckon that calling push() on dynamic arrays in Solidity is a fairly gas costly operation and outweighs the additional loop to calculate the number of split components and subsequent instantiation of fixed size array.

Final test I just completed was to stick to your return of storage array. I.e. in my implementation I return memory array. Thus the only change from your implementation is to add memory data location to the input parameters: function split(string memory _base, string memory _value).
The gas cost for this alternate split() call is now slightly up from master to 154221, i.e. an increase of less than a percent.

To me this suggests going with the fixed size array implementation.

@willitscale willitscale merged commit 000a42d into willitscale:master Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants