Skip to content

Conversation

barakman
Copy link
Contributor

@barakman barakman commented Aug 7, 2018

"Luckily", since owner is not used in this code, the problem remains submerged.

Fixes #

🚀 Description

"Luckily", since owner is not used in this code, the problem remains submerged.

Make sure that owner is indeed the real owner.

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

"Luckily", since `owner` is not used in this code, the problem remains submerged.
@nventuro nventuro self-assigned this Aug 7, 2018
@nventuro
Copy link
Contributor

nventuro commented Aug 7, 2018

Wow, looks like I missed this one in #1137. I'll double check the other test files to verify this doesn't come up anywhere else.

@nventuro
Copy link
Contributor

nventuro commented Aug 7, 2018

@barakman thanks for spotting this! However, your fix is against the spirit of #1137 and what we intend for the OZ test code style. The account list should start with _ to prevent using accounts[0], and we should be explicit about owner (e.g. claimable = await Claimable.new({ from: owner });, etc.). Could you make those changes?

Thanks again!

@nventuro nventuro added bug kind:improvement tests Test suite and helpers. labels Aug 7, 2018
@nventuro nventuro added this to the v2.0 milestone Aug 7, 2018
@barakman barakman closed this Aug 7, 2018
@barakman
Copy link
Contributor Author

barakman commented Aug 7, 2018

@nventuro:
Yes, I have realized this after submitting the change.
I have resubmitted the correct change at #1161.
Thanks

@nventuro
Copy link
Contributor

nventuro commented Aug 7, 2018

Great! For future reference though, there is no need to create a new PR: you can commit on top of the old branch, and the PR will be updated accordingly :) This also applies to code reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug tests Test suite and helpers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants