Skip to content

Conversation

@chase1745
Copy link

@chase1745 chase1745 commented Jul 12, 2018

🚀 Description

Solidity v050 will no longer allow default data locations for function parameters (and return parameters) (argotorg/solidity#4014), so all such instances have been made explicit memory locations.

  • 📘 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).

@chase1745 chase1745 force-pushed the use-explicit-data-locations branch from cfaef6b to c4a473e Compare July 17, 2018 04:04
@nventuro
Copy link
Contributor

Awesome, thanks @chase1745! I want to leave this one open until argotorg/solidity#4014 is merged, so that we can test against the solidity nightly build.

@nventuro nventuro added the on hold Put on hold for some reason that must be specified in a comment. label Jul 20, 2018
@nventuro nventuro self-assigned this Jul 20, 2018
@nventuro nventuro added kind:improvement contracts Smart contract code. labels Jul 21, 2018
@chase1745
Copy link
Author

argotorg/solidity#4518 is now the PR that contains the breaking changes, just for reference.

@nventuro
Copy link
Contributor

nventuro commented Aug 8, 2018

Now tracking argotorg/solidity#4738.

@nventuro
Copy link
Contributor

Woot @chase1745 looks like argotorg/solidity#4738 was finally merged! We should be able to test against nightly builds now - could you fix the merge conflicts so that we can move along with this once the build passes? Thanks!

@nventuro nventuro removed the on hold Put on hold for some reason that must be specified in a comment. label Aug 15, 2018
@chase1745 chase1745 force-pushed the use-explicit-data-locations branch from c4a473e to ea8ce5f Compare August 17, 2018 04:00
@chase1745
Copy link
Author

@nventuro Should be good to go👍

@chase1745
Copy link
Author

It's failing the CI build because calldata was not a keyword in previous versions, but it now is required, so when it runs against the latest build with v050 it should pass.

@nventuro
Copy link
Contributor

nventuro commented Aug 17, 2018

That's inconvenient - it doesn't exist in previous versions and is now required?

Given that, is there a way we can have OZ compile in both 0.4.24 and 0.5.0 (without having to use the experimental pragma)?

@leonardoalt
Copy link

@nventuro @chase1745
In that case (where calldata is now mandatory for v0.5.0) I don't think you can compile the same code with the different versions. There are probably other similar cases, as 0.5.0 brings many changes.

@nventuro
Copy link
Contributor

I feared this may be the case. I don't want to drop our testing using the nightly build though - I'll try to come up with a way to handle this (e.g. a script that patches our source code) during the next couple days. Until then, this should be on hold, so we can keep things tidy and on the same PR.

@nventuro nventuro added the on hold Put on hold for some reason that must be specified in a comment. label Aug 21, 2018
@nventuro nventuro removed the on hold Put on hold for some reason that must be specified in a comment. label Oct 2, 2018
@nventuro nventuro changed the base branch from master to solc-nightly October 2, 2018 12:55
@nventuro
Copy link
Contributor

nventuro commented Oct 2, 2018

I've updated the PR to target the solc-nightly branch, though with the recent 2.0 renaming it may be easier to simply do these changes again from scratch.

@axic
Copy link
Contributor

axic commented Oct 2, 2018

Most of this should be backwards compatible and could be merged on master. What is not backwards compatible is the calldata keyword as that was added newly.

@nventuro
Copy link
Contributor

Fixed via e5b94c1

@nventuro nventuro closed this Jan 14, 2019
@chase1745 chase1745 deleted the use-explicit-data-locations branch January 15, 2019 05:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contracts Smart contract code.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants