Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make contracts 4.24 compatible #951

Merged
merged 2 commits into from Jun 13, 2018
Merged

Make contracts 4.24 compatible #951

merged 2 commits into from Jun 13, 2018

Conversation

artiebits
Copy link
Contributor

πŸš€ Description

  • πŸ“˜ I've reviewed the OpenZeppelin Contributor Guidelines
  • βœ… I've added tests where applicable to test my new functionality.
  • πŸ“– I've made sure that my contracts are well-documented.
  • 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

Truffle updated solc to 0.4.24 as well.

Warnings fixed:

  • keccak256 only accepts a single "bytes" argument.
  • Using contract member "balance" inherited from the address type is deprecated, use "address(contract).balance" instead.

Copy link
Contributor

@come-maiz come-maiz left a comment

Choose a reason for hiding this comment

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

Awesome @artiebits, thanks a lot. We usually wait around a month after solc is released to update, so this will be perfect for our next release.
However, because we were slow to review this now it has some conflicts. Can you please rebase?
Also, please update the truffle version.

package.json Outdated
@@ -58,7 +58,7 @@
"ganache-cli": "6.1.0",
"solidity-coverage": "^0.5.0",
"solium": "^1.1.7",
"truffle": "^4.1.8",
"truffle": "^4.1.9",
Copy link
Contributor

Choose a reason for hiding this comment

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

@artiebits
Copy link
Contributor Author

@ElOpio np, rebased and updated

@come-maiz
Copy link
Contributor

Sorry @artiebits, once more.
@shrugs @frangio can we land this next, so there are no more conflicts on the way?

@shrugs
Copy link
Contributor

shrugs commented Jun 12, 2018

oops we did it again πŸ˜“

I'll personally avoid merging any more PRs until this one is merged. sorry for the extra work, @artiebits

@axic
Copy link
Contributor

axic commented Jun 12, 2018

Any chance merging this and #1002? It would simplify our life at the Solidity repo a lot :)

@shrugs
Copy link
Contributor

shrugs commented Jun 12, 2018

@axic will do!

@shrugs
Copy link
Contributor

shrugs commented Jun 12, 2018

Also it'd be cool if these sort of upgrades were done automatically by a bot in the future, since it's a pretty easily automatable (but human-checkable) operation

@axic
Copy link
Contributor

axic commented Jun 12, 2018

Btw, people with write access to this repo should be able (unless specifically disabled by the owner) to push to @artiebits PRs.

@artiebits
Copy link
Contributor Author

no worries, updated

@frangio
Copy link
Contributor

frangio commented Jun 13, 2018

it'd be cool if these sort of upgrades were done automatically by a bot in the future

This should be possible using Solium's fix mode!

shrugs
shrugs previously approved these changes Jun 13, 2018
Copy link
Contributor

@shrugs shrugs left a comment

Choose a reason for hiding this comment

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

a quick search for 0.4.23 in the project shows no missing pragmas, so we're good to go

@shrugs shrugs dismissed their stale review June 13, 2018 18:16

missing abi.encodePacked

Copy link
Contributor

@shrugs shrugs left a comment

Choose a reason for hiding this comment

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

LGTM

@shrugs shrugs merged commit 5daaf60 into OpenZeppelin:master Jun 13, 2018
@frangio frangio mentioned this pull request Jul 23, 2018
2 tasks
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.

None yet

5 participants