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

Replacing custom interface with stock openzeppelin IERC721.sol #5376

Merged
merged 11 commits into from
Dec 9, 2019

Conversation

nfurfaro
Copy link
Contributor

Description

As per the title.

  • changed IPublicLock.sol to abstract prevent compilation errors
  • modified a few functions' visibility from external to public to prevent compilation errors
  • added 2 getter functions (ownerOf and totalSupply) and changed existing to ownerOf and _totalSupply tp prevent duplicate declaration error.
  • added one extra check in function ownerOf based on the openzeppelin implementation of the same function.

Issues

Fixes #
Refs #

Checklist:

  • 1 PR, 1 purpose: my Pull Request applies to a single purpose
    • This PR only contains configuration changes (package.json, etc.)
    • This PR only contains code changes (if configuration changes are required, do a separate PR first, then re-base)
  • My code follows the style guidelines of this project, including naming conventions
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • If my code adds or changes components, I have written corresponding stories with Storybook
  • I have performed a self-review of my own code
  • If my code involves visual changes, I am adding applicable screenshots to this thread

@HardlyDifficult
Copy link
Contributor

Tests are really not happy right now ;)

@nfurfaro
Copy link
Contributor Author

Huh. maybe something going on with solium... indentation looks fine to me. Will see if I can make it happy again.

@HardlyDifficult
Copy link
Contributor

Huh. maybe something going on with solium... indentation looks fine to me. Will see if I can make it happy again.

Note that both the tests AND lint are failing.

@nfurfaro
Copy link
Contributor Author

yeah, will look into this. The tests passed locally, will see if I can repro and sort it out.

@nfurfaro nfurfaro force-pushed the n44o-replace-interface-IERC721 branch from 2e02d3c to 2ed3a32 Compare November 28, 2019 00:26
@HardlyDifficult
Copy link
Contributor

This can be closed, right? Was replaced by another PR - re-open if I missed something.

@nfurfaro
Copy link
Contributor Author

nfurfaro commented Dec 5, 2019

Reopening as I have this working locally now.

@nfurfaro nfurfaro reopened this Dec 5, 2019
@HardlyDifficult
Copy link
Contributor

Sorry about that, at a glance I thought #5383 replaced this.

Let me know once it's ready for review

@nfurfaro nfurfaro force-pushed the n44o-replace-interface-IERC721 branch from 2ed3a32 to 4ae4a53 Compare December 6, 2019 17:18
@nfurfaro
Copy link
Contributor Author

nfurfaro commented Dec 6, 2019

@NickCuso When you have time could you give this a look over? thx

@nfurfaro
Copy link
Contributor Author

nfurfaro commented Dec 6, 2019

Ha! well, that didn't work.

@HardlyDifficult
Copy link
Contributor

heh yea didn't mean to complicate things :) i don't feel strongly either way if you want to revert to what you had

This reverts commit a649c3d.
@nfurfaro
Copy link
Contributor Author

nfurfaro commented Dec 7, 2019

I will revert the last commit which switched from internal to private. I think the rest should be good.

@nfurfaro
Copy link
Contributor Author

nfurfaro commented Dec 7, 2019

ok, reverted to my earlier PR.

@nfurfaro
Copy link
Contributor Author

nfurfaro commented Dec 9, 2019

@NickCuso If you're ok with the approach, I'd like to merge this. We can certainly look at further tweaks later, but right now the small changes discussed above break so many tests, I'm not sure the value is there in pursuing them atm.

@nfurfaro nfurfaro merged commit e19963a into master Dec 9, 2019
@nfurfaro nfurfaro deleted the n44o-replace-interface-IERC721 branch December 9, 2019 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants