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

Refactor assert revert helper to encapsulate promises #628

Conversation

facuspagnuolo
Copy link
Contributor

@facuspagnuolo facuspagnuolo commented Dec 26, 2017

No description provided.

@facuspagnuolo facuspagnuolo changed the title Refactor assert revert helper to encapsulate promises [WIP] Refactor assert revert helper to encapsulate promises Dec 26, 2017
@facuspagnuolo facuspagnuolo changed the title [WIP] Refactor assert revert helper to encapsulate promises Refactor assert revert helper to encapsulate promises Dec 26, 2017
@facuspagnuolo facuspagnuolo added tests and removed bug labels Dec 26, 2017
federicobond
federicobond previously approved these changes Dec 26, 2017
Copy link
Contributor

@federicobond federicobond left a comment

Choose a reason for hiding this comment

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

Change to assertRevert looks fine. I have not reviewed the ERC721 contract.

assert.fail('Expected revert not received');
} catch (error) {
const revertFound = error.message.search('revert') >= 0;
console.log('Revert found: ', revertFound);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this console.log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@facuspagnuolo facuspagnuolo force-pushed the refactor/assert_revert_to_encapsulate_promises branch 2 times, most recently from 98b8dcb to 8c02b9a Compare December 26, 2017 21:22
@facuspagnuolo facuspagnuolo force-pushed the refactor/assert_revert_to_encapsulate_promises branch from 8c02b9a to 55c59f8 Compare December 26, 2017 21:41
@facuspagnuolo
Copy link
Contributor Author

facuspagnuolo commented Dec 26, 2017

@federicobond I rebased this PR from master so it can be merged before #627. Actually #627 now depends on this PR. Please review

@federicobond
Copy link
Contributor

Please remove the .node-xmlhttprequest-sync-19998 file and move the .idea gitignore rule to your own .git/info/exclude.

Everything else looks good! 🎄

@facuspagnuolo facuspagnuolo force-pushed the refactor/assert_revert_to_encapsulate_promises branch from 55c59f8 to 4173800 Compare December 27, 2017 19:58
@facuspagnuolo
Copy link
Contributor Author

done @federicobond :)

Copy link
Contributor

@federicobond federicobond left a comment

Choose a reason for hiding this comment

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

LGTM

@federicobond federicobond merged commit 323d1fa into OpenZeppelin:master Dec 28, 2017
@janezkranjc
Copy link

will not the catch also catch the assert.fail('...')?

facuspagnuolo added a commit to facuspagnuolo/zeppelin-solidity that referenced this pull request Jan 28, 2018
ProphetDaniel pushed a commit to classicdelta/Smart-Contracts that referenced this pull request Mar 9, 2018
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.

3 participants