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

getApproved require the given token ID exist #1256

Merged
merged 4 commits into from
Sep 6, 2018
Merged

getApproved require the given token ID exist #1256

merged 4 commits into from
Sep 6, 2018

Conversation

lamengao
Copy link
Contributor

🚀 Description

In the ERC-721 standard the getApproved comment mentioned "Throws if _tokenId is not a valid NFT".

    /// @notice Get the approved address for a single NFT
    /// @dev Throws if `_tokenId` is not a valid NFT.
    /// @param _tokenId The NFT to find the approved address for
    /// @return The approved address for this NFT, or the zero address if there is none
    function getApproved(uint256 _tokenId) external view returns (address);

This PR make getApproved conforms to ERC721 standard in better terms.

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

@nventuro nventuro added bug kind:improvement contracts Smart contract code. breaking change Changes that break backwards compatibility of the public API. labels Aug 30, 2018
@nventuro nventuro added this to the v2.0 milestone Aug 30, 2018
@nventuro nventuro self-assigned this Aug 30, 2018
@nventuro nventuro requested a review from shrugs August 30, 2018 10:56
nventuro
nventuro reviewed Aug 30, 2018
View reviewed changes
Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

Great catch @lamengao, thanks!

test/token/ERC721/ERC721MintBurn.behavior.js Outdated
it('clears the approval', async function () {
(await this.token.getApproved(tokenId)).should.be.equal(ZERO_ADDRESS);
context('when the token ID does not exist', function () {
it('reverts', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the current phrasing a bit confusing: maybe we could merge the context and it blocks, and change the description to something like 'causes getApproved to revert'?

We also may want to add a test block for getApproved, explicitly checking for this, though that could be considered part of #1148.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nventuro I made a quick attempt at clarifying the descriptions. And added a comment on #1148 to not forget about the getApproved tests.

@come-maiz come-maiz mentioned this pull request Sep 6, 2018
Closed
@nventuro nventuro merged commit a7ee54e into OpenZeppelin:master Sep 6, 2018
@lamengao lamengao deleted the erc721-getApproved branch September 9, 2018 05:19
This was referenced Apr 15, 2019
Closed
Closed
@dependabot-preview dependabot-preview bot mentioned this pull request May 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-maiz come-maiz come-maiz left review comments

@nventuro nventuro nventuro approved these changes

@shrugs shrugs Awaiting requested review from shrugs

Assignees

@nventuro nventuro

Labels
breaking change Changes that break backwards compatibility of the public API. bug contracts Smart contract code.
Projects
None yet
Milestone
v2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@lamengao @come-maiz @nventuro