Add validation in Governor on ERC-721 or ERC-1155 received by clauBv23 · Pull Request #4314 · OpenZeppelin/openzeppelin-contracts (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation22 Commits15 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Fixes #4307

Add validation to onERC1155Received, onERC1155BatchReceived, and onERC721Received hooks in Governor to check it is the executor.
Add test to the validations.

Note: This PR should be refactored after merging #4261 and #4284

PR Checklist

Tests
Documentation
Changeset entry (run npx changeset add)

Amxx mentioned this pull request

Jun 5, 2023

3 tasks

Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

If possible, the expectRevert should not be unspecified.

@Amxx good catch! But what do you mean whit if possible, is there any specific reason for not specifying it? In this case for example what is the reason for not doing it?

it("doesn't accept ether transfers", async function () {

await expectRevert.unspecified(web3.eth.sendTransaction({ from: owner, to: this.mock.address, value: 1 }));

});

it("doesn't accept ether transfers", async function () {
await expectRevert.unspecified(web3.eth.sendTransaction({ from: owner, to: this.mock.address, value: 1 }));
});

If there is no revert string, or if the revert is a panic, then unspecified might be the way.
Also, maybe some of our current unspecified should be specified.

oh perfect makes sense @Amxx thanks!

Choose a reason for hiding this comment

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

This looks good @clauBv23! Thanks. I'm adding a few suggestions and also requesting review from others.

Thanks!

Amxx previously approved these changes Jun 7, 2023

Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

LGTM

@frangio @ernestognw I merged master into this and realised the following:

ERC1155 acceptance hook was written in a way that bubble error string, but not custom error. The result is that the Governor custom error (GovernorDisabledDeposit) is replaced by the default one (ERC1155InvalidReceiver)

Do we want to change the ERC1155 hook to use the syntax from ERC721 (which bubble the custom errors)?

Do we want to change the ERC1155 hook to use the syntax from ERC721 (which bubble the custom errors)?

I'd say yes.

Co-authored-by: Claudia Barcelo claudiabarcelovaldes40@gmail.com

This was referenced

Sep 10, 2024

This was referenced

Sep 12, 2024

This was referenced

Nov 8, 2024

This was referenced

Nov 9, 2024