search

UptownResearch / pyTokenSpike

intial work on pyTokens
0 stars 0 forks source link

Use DecimalMath.sol - Fixes #5 #19

Closed alcueca closed 4 years ago

alcueca commented 4 years ago

Major refactoring to use DecimalMath

When using a.muld(b, long) we are implying that b is long, and the result is in the same format as a When using a.divd(b, long) we are assuming that b is long, and the result is in the same format as a

Have a look, the tests still pass and to me it seems that the values are unchanged, but I can't really know until we get a comprehensive test suite.

I broke the tests in pyTokenFactory, not sure what happened there :thinking:

aniemerg commented 4 years ago

It looks like deploying a single pyToken contract from the Factory costs 4million+ gas? Can that be right?

alcueca commented 4 years ago

We know we can deploy pyToken contracts from pyToken.js, so it can't be right.

I don't think I changed anything in pyTokenFactory.sol, but I can compare between branches to be sure. Did I mess up with the imports when changing openzeppelin-contracts to @openzeppelin/contracts?

aniemerg commented 4 years ago

Well, recall that pyTokenFactory.createPyToken deploys 2 pyToken contracts. If you comment out one of the deploys, it works. I think it was probably already very close to the 8 million limit and something you did put it over the line.

aniemerg commented 4 years ago

Gas of a single pyToken deploy: 4978011

alcueca commented 4 years ago

The pyToken contract shouldn't be that heavy, and should be lighter now that I've taken out the math functions. What was the gas usage before?

alcueca commented 4 years ago

It was definitely a gas usage problem. Calls to libraries are more expensive when converted to bytecode than calls to internal functions, and there are a lot of calls in pyToken.sol.

I commented out the calls to console.log (let's replace them by tests) and used a constant instead of calls to long.unit(). Let's keep an eye on gas costs going forward.