Skip to content

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
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!

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.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Changes that break backwards compatibility of the public API. bug contracts Smart contract code.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants