Skip to content

Update open zeppelin and oraclize - #398

Merged
adamdossa merged 17 commits into
dev-3.0.0from
update-openZeppelin-and-oraclize
Nov 14, 2018
Merged

Update open zeppelin and oraclize#398
adamdossa merged 17 commits into
dev-3.0.0from
update-openZeppelin-and-oraclize

Conversation

@kostind

Copy link
Copy Markdown
Contributor

Please check if the PR fulfills these requirements

  • The commit message follows our Submission guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

Update OpenZeppelin and Oraclize

What is the current behavior?

(You can also link to an open issue here)

What is the new behavior?

(Define and describe any new functionality. Clarify if this is a feature change)

Does this PR introduce a breaking change?

Run "npm install" to update version of openzeppelin-solidity.

Any Other information:

@kostind

kostind commented Nov 8, 2018

Copy link
Copy Markdown
ContributorAuthor

New version of Ownable contract emits OwnershipTransferred event in a constructor.
I had to replace tx.logs[1].args._ticker on tx.logs[2].args._ticker, tx.logs[1].args._securityTokenAddress on tx.logs[2].args._securityTokenAddress, etc

Comment threadcontracts/modules/STO/USDTieredSTO.sol
Comment threadcontracts/tokens/SecurityToken.sol Outdated
Comment threadcontracts/tokens/SecurityToken.sol Outdated
Comment threadcontracts/tokens/SecurityToken.sol Outdated
Comment threadcontracts/tokens/SecurityToken.sol Outdated
Comment threadcontracts/tokens/SecurityToken.sol Outdated
@satyamakgec

Copy link
Copy Markdown
Contributor

@kostind could you please remove the conflicts so I can approve the PR.

Comment threadcontracts/tokens/SecurityToken.sol Outdated
maxsam4
maxsam4 previously approved these changes Nov 9, 2018
@maxsam4

Copy link
Copy Markdown
Contributor

Hmm, PR Travis build is broken.

@kostind

Copy link
Copy Markdown
ContributorAuthor

@kostind could you please remove the conflicts so I can approve the PR.

@satyamakgec done

@kostind

kostind commented Nov 9, 2018

Copy link
Copy Markdown
ContributorAuthor

Hmm, PR Travis build is broken.

@maxsam4 npm run wintest -> 935 passing (5m)
Which command does Travis build use?

Confilcts in package.json were resolved recently.
Could it be the case of failure?

@maxsam4

Copy link
Copy Markdown
Contributor

@kostind The push commit is building fine even in Travis as well. Pull PR merges the files and runs coverage.
npm run wincov will be the nearest command to it.

I resolved the package.json conflicts and it could be that something went wrong there. unlikely though.

@kostind

kostind commented Nov 9, 2018

Copy link
Copy Markdown
ContributorAuthor

@maxsam4 I tried to resolve conflicts as well, but you won.
I ran npm run wincov locally.

@kostind

Copy link
Copy Markdown
ContributorAuthor

I've got the same result locally for npm run wincov

@maxsam4

Copy link
Copy Markdown
Contributor

I've got the same result locally for npm run wincov

Just confirming, From the same result, You mean it timed out as well?
Interesting. I'll have to take a deeper look.

@kostind

Copy link
Copy Markdown
ContributorAuthor

@maxsam4 Yes, build hangs on the same step.

@kostind

kostind commented Nov 9, 2018

Copy link
Copy Markdown
ContributorAuthor

truffle test test/b_capped_sto.js -> all tests passed

@maxsam4
maxsam4 dismissed their stale reviewNovember 9, 2018 13:11

need to fix coverage

@maxsam4

maxsam4 commented Nov 12, 2018

Copy link
Copy Markdown
Contributor

@satyamakgec Solidity-coverage is now fixed, please review this again.

adamdossa
adamdossa previously requested changes Nov 14, 2018
_adjustTotalSupplyCheckpoints();
balances[_from] = balances[_from].sub(_value);
totalSupply_ = totalSupply_.sub(_value);
_burn(_from, _value);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maxsam4@satyamakgec@kostind Should this be super._burn instead? Or otherwise we should maybe rename our _burn function so we don't clash with the OZ namespace.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(bit surprised this actually works as currently written)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was confused by this when I first saw it as well. It works because our _burn() takes an extra parameter. Therefore, it's function overloading and not overriding that's happening here.

Comment threadcontracts/tokens/SecurityToken.sol Outdated
}

function _burnFrom(address _from, uint256 _value, bytes _data) internal returns(bool) {
require(_value <= balanceOf(_from), "Value too high");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already checked in the _burn call (from _burnFrom)

@adamdossa
adamdossa dismissed their stale reviewNovember 14, 2018 16:46

I put the changes in

@adamdossa
adamdossa merged commit e4dc5f3 into dev-3.0.0Nov 14, 2018
@maxsam4
maxsam4 deleted the update-openZeppelin-and-oraclize branch April 9, 2019 07:07
Sign up for freeto 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.

4 participants

@kostind@satyamakgec@maxsam4@adamdossa