Some Minor fixes #328
Some Minor fixes #328
Conversation
|
is there a requirement to go solc |
|
@pdobacz I think we should stick with latest version before we start the audit. I don't mind to upgrade that a bit later though. Would this impact your side? |
| }); | ||
| }); | ||
|
|
||
| describe('with boundary size of numbers', () => { |
There was a problem hiding this comment.
@thec00n here is the new tests for boundary
There was a problem hiding this comment.
I did not appreciate https://github.com/omisego/plasma-contracts/blob/11b3bae4926dabc491dfbce511207b2f2798ba0f/plasma_framework/contracts/src/exits/utils/BondSize.sol#L75 in the beginning but I think it's actually the best solution in this case to ensure that the check does not overflow and to avoid a scenario where we get stuck with a new bond size.
we'd probably like to upgrade to the same version, which impacts the builder/deployer images for (or we could just yolo and use different versions for now, but if sth funny happens we'll have one more thing to debug). If we can upgrade a bit later, I'd appreciate that. Maybe after the branch-entanglement and deployment changes are behind us? But if time comes and you need to, pls upgrade and let us know, thx 🙏 |
256d913 to
bec1900
Compare
|
|
||
| if (_self.updatedBondSize != 0 && now > _self.effectiveUpdateTime) { | ||
| _self.previousBondSize = _self.updatedBondSize; | ||
| if (self.updatedBondSize != 0 && now >= self.effectiveUpdateTime) { |
There was a problem hiding this comment.
This now >= self.effectiveUpdateTime part was a bug! It was now > self.effectiveUpdateTime but in bondSize(self) it is using now < self.effectiveUpdateTime. As a result, when now == self.effectiveUpdateTime it would fail to fall into this if block.
| this.lowerBoundDivisor, | ||
| this.upperBoundMultiplier, | ||
| ); | ||
| describe('in general...', () => { |
There was a problem hiding this comment.
lol, my bad. re-word it to with normal cases
| expect(bondSize).to.be.bignumber.equal(secondNewBondSize); | ||
| describe('with boundary size of numbers', () => { | ||
| it('should able to update to max number of uint128 without having overflow issue', async () => { | ||
| const maxSizeOfUint256 = (new BN(2)).pow(new BN(128)).sub(new BN(1)); // 2^128 - 1 |
Remove this comment: "// solhint-disable-next-line reason-string" as the issue is solved. solhint bug: protofire/solhint#157
1. refactor the code a bit to remove underscore prefix for var 2. add tests to show the boundary number for BondSize see: https://git.io/JecTv
It uses "now < self.effectiveUpdateTime" to choose the updatedBondSize or not. But it is using "now > self.effectiveUpdateTime" when updating. As a result, when now == self.effectiveUpdateTime, it becomes a hole that belongs to no one.
…rityTimestamp All other places would make the update take effect right on the timestamp instead of timestamp + 1. This change makes it to be consistent with others.
Note
SafeMathas it is really meaning less since it only protectsuint256but we are usinguint128. Instead, I add tests on that we support edge case of upgrade to max num ofuint128.now == effectiveUpdateTimethe update would be broken. Found this while trying to add tests for coverage : )depositVerifierbe updated right atnewDepositVerifierMaturityTimestampupgrade to solidity 0.5.12see this comment: Some Minor fixes #328 (comment)make compiler happy when optimizer is off. Somehow it failed to compile when the optimizer is tuned off with the change on BondSize. (Whenever there is aReverting this as it end up increase deployment gas quite a lot, and we don't really have security issue as the params are fixed in PaymentExitGame contract now.requireinsidebuildParamsit fails to compile....). Found this because the coverage tool forces optimizer to be off. Seems like it cannot be called in a constructor. So I change the constructor forRoutersto becomeinitfunction instead.