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.

Sign up for GitHub

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

Jump to bottom

Fix/improve revert reason #1727 #2018

Merged
merged 24 commits into from
Jan 23, 2020
Merged

Fix/improve revert reason #1727 #2018

merged 24 commits into from
Jan 23, 2020

Conversation

alant
Copy link
Contributor

@alant alant commented Dec 8, 2019 •
edited by frangio
Loading

Fixes #1727

frangio
frangio reviewed Dec 9, 2019
View reviewed changes
contracts/token/ERC721/ERC721.sol Outdated Show resolved Hide resolved
contracts/token/ERC721/ERC721.sol Show resolved Hide resolved
contracts/token/ERC721/ERC721.sol Outdated Show resolved Hide resolved
@frangio
Copy link
Contributor

frangio commented Dec 9, 2019

Thanks @alant! I left a few comments.

alant and others added 8 commits December 9, 2019 18:02
@alant @frangio
Update contracts/token/ERC721/ERC721.sol
027822d 
Co-Authored-By: Francisco Giordano <frangio.1@gmail.com>
@alant
same revert wording per @frangio's review suggestion
07dcbd3
@alant
Merge branch 'fix/improve-revert-reason-#1727' of https://github.com/…
1755395 
…alant/openzeppelin-solidity into fix/improve-revert-reason-#1727
@alant
per @frangio's feedback, changing the inline assembly to accomplish: …
be4d51d 
…we want to ignore the first 4 bytes of content, so we should read the length and decrease it by 4, then take the memory location and add 4 to it, then store the new length at the new memory location, then that is the new byte array that we want.
@alant
change revert msg assembly per PR comment by @frangio
f0a90f6
@alant
unify revert msg in test code
ab1fb60
@alant
Merge branch 'master' into fix/improve-revert-reason-#1727
59cba42
@alant
fix some failed tests, wording change
cd436a6
frangio
frangio reviewed Dec 12, 2019
View reviewed changes
test/token/ERC721/ERC721.behavior.js Outdated Show resolved Hide resolved
contracts/token/ERC721/ERC721.sol Outdated Show resolved Hide resolved
contracts/token/ERC721/ERC721.sol Outdated Show resolved Hide resolved
contracts/token/ERC721/ERC721.sol Outdated Show resolved Hide resolved
alant and others added 9 commits December 12, 2019 17:53
@frangio
Copy link
Contributor

frangio commented Jan 14, 2020

In f279bec I've fixed the bit of assembly code that was wrong.

With this fix things should be finally working correctly now, and you should be able to change the expectRevert.unspecified that you added into the proper expectRevert(..., 'ERC721ReceiverRevertsMock: Transaction rejected by receiver'). This ensures that the revert reason is being forwarded correctly.

Sorry you had to endure so much back and forth on this!

@frangio
Copy link
Contributor

frangio commented Jan 14, 2020

Also, I've noticed ERC721ReceiverRevertsMock and ERC721ReceiverMock are exactly the same. Can you remove one of them?

@alant
Copy link
Contributor Author

alant commented Jan 15, 2020

Also, I've noticed ERC721ReceiverRevertsMock and ERC721ReceiverMock are exactly the same. Can you remove one of them?

Done.

@alant alant closed this Jan 15, 2020
@alant alant reopened this Jan 15, 2020
@alant
Copy link
Contributor Author

alant commented Jan 15, 2020

In f279bec I've fixed the bit of assembly code that was wrong.

With this fix things should be finally working correctly now, and you should be able to change the expectRevert.unspecified that you added into the proper expectRevert(..., 'ERC721ReceiverRevertsMock: Transaction rejected by receiver'). This ensures that the revert reason is being forwarded correctly.

Sorry you had to endure so much back and forth on this!

No worries. I was not able to change the test to expectRevert(..., 'ERC721ReceiverMock: 'ERC721ReceiverMock: reverting')and have them pass. Can you take another look and let me know please? Thanks.

@frangio
Copy link
Contributor

frangio commented Jan 15, 2020

expectRevert(..., 'ERC721ReceiverMock: 'ERC721ReceiverMock: reverting') is not correct. The expected revert reason for the tests which use ERC721ReceiverRevertsMock is 'ERC721ReceiverRevertsMock: Transaction rejected by receiver'. Remember the point of this feature is to forward the original revert reason!

@frangio frangio mentioned this pull request Jan 20, 2020
Merged
2 tasks
@nventuro
Copy link
Contributor

Thank you for your contribution @alanarvelo!

@nventuro nventuro merged commit 7d7cbca into OpenZeppelin:master Jan 23, 2020
KaiRo-at pushed a commit to KaiRo-at/openzeppelin-contracts that referenced this pull request Mar 16, 2020
@alant @frangio
@nventuro @KaiRo-at
Fix/improve revert reason OpenZeppelin#1727 (OpenZeppelin#2018)
f1b55d6 
* adding mock contacts, test code

* adding changes to ERC721.sol per @frangio's comments on original PR OpenZeppelin#1943

* fix solhint warnings

* Update contracts/token/ERC721/ERC721.sol

Co-Authored-By: Francisco Giordano <frangio.1@gmail.com>

* same revert wording per @frangio's review suggestion

* per @frangio's feedback, changing the inline assembly to accomplish: we want to ignore the first 4 bytes of content, so we should read the length and decrease it by 4, then take the memory location and add 4 to it, then store the new length at the new memory location, then that is the new byte array that we want.

* change revert msg assembly per PR comment by @frangio

* unify revert msg in test code

* fix some failed tests, wording change

* Update contracts/token/ERC721/ERC721.sol

Co-Authored-By: Francisco Giordano <frangio.1@gmail.com>

* Update contracts/token/ERC721/ERC721.sol

Co-Authored-By: Francisco Giordano <frangio.1@gmail.com>

* fix test case, revert without reason

* fix 'ERC721ReceiverRevertsMock: Transaction rejected by receiver'

* style change per review by @frangio

* fix revert reason forwarding

* remove duplicate contracts/mocks/ERC721ReceiverRevertsMock.sol per review OpenZeppelin#2018

* Add changelog entry

* Fix tests

* Make tests more clear

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frangio frangio frangio left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve revert reason when assuming interface compliance
3 participants
@alant @frangio @nventuro