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

Add ERC721ALowCap + tests #114

Merged
merged 24 commits into from
Apr 4, 2022
Merged

Add ERC721ALowCap + tests #114

merged 24 commits into from
Apr 4, 2022

Conversation

Austinhs
Copy link
Contributor

Ended up creating a function "tokensOfOwner" as its something Divine Anarchy uses a lot within its dApp experience. Figured I would create a pull request incase it was something you guys wanted to add to the main repo :)

@Vectorized
Copy link
Collaborator

See #115 on tokensOfOwner function.

@Austinhs Austinhs changed the title Add tokensOfOwner function + tests + Append Divine Apples to projects.md Add ERC721ALowCap + tests + Append Divine Apples to projects.md Feb 17, 2022
@Vectorized
Copy link
Collaborator

Vectorized commented Feb 18, 2022

I suggest putting the projects.md update into another PR, so that the maintainers can merge that first.

@Austinhs Austinhs changed the title Add ERC721ALowCap + tests + Append Divine Apples to projects.md Add ERC721ALowCap + tests Feb 18, 2022
@Austinhs
Copy link
Contributor Author

@Vectorized in #115 you compressed the function by a bit

    /**
     * @dev Returns the tokenIds of the address. O(totalSupply) in complexity.
     */
    function tokensOfOwner(address owner) public view returns (uint256[] memory) {
        uint256 holdingAmount = balanceOf(owner);
        uint256 currSupply    = _currentIndex;
        uint256 tokenIdsIdx;
        address currOwnershipAddr;

        uint256[] memory list = new uint256[](holdingAmount);

        unchecked {
            for (uint256 i; i < currSupply; i++) {
                TokenOwnership memory ownership = _ownerships[i];

                if (ownership.burned) {
                    continue;
                }

                // Find out who owns this sequence
                if (ownership.addr != address(0)) {
                    currOwnershipAddr = ownership.addr;
                }

                // Append tokens the last found owner owns in the sequence
                if (currOwnershipAddr == owner) {
                    list[tokenIdsIdx++] = i;
                }

                // All tokens have been found, we don't need to keep searching
                if(tokenIdsIdx == holdingAmount) {
                    break;
                }
            }
        }

        return list;
    }

Specifically you removed this:


                // All tokens have been found, we don't need to keep searching
                if(tokenIdsIdx == holdingAmount) {
                    break;
                }

Am I wrong in assuming this loop is going to continue? Or is it because this is a view it does not matter and its better to save the space in the contract to lower deploy cost. (I think things like comments, spacing etc will get removed with the optimizer but I could be wrong)

@Vectorized
Copy link
Collaborator

Vectorized commented Feb 19, 2022

@Austinhs You can add the break if you want.

I simply copied and pasted from my own code. Didn't code the break part it as its just a view function
(just let Infura brute force all the way for free lol). Don't look too much into it. ;)

I am waiting for the maintainers and others to drop their opinions.

Copy link
Contributor

@ahbanavi ahbanavi left a comment

Choose a reason for hiding this comment

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

Some style notes.
Also, add some tests for burned tokens too, just like transfer.
And it would be better if you add tests with ERC721AOwnersExplicit too.

*/
function tokensOfOwner(address owner) public view returns (uint256[] memory) {
uint256 holdingAmount = balanceOf(owner);
uint256 currSupply = _currentIndex;
Copy link
Contributor

@ahbanavi ahbanavi Feb 19, 2022

Choose a reason for hiding this comment

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

Remove extra white spaces before the assignment =. docs


import '../extensions/ERC721ALowCap.sol';

contract ERC721ALowCapMock is ERC721ALowCap {
Copy link
Contributor

@ahbanavi ahbanavi Feb 19, 2022

Choose a reason for hiding this comment

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

Filename should be the same as contract name, try changing the filename to ERC721ALowCapMock.sol. docs

await this.token.deployed();
});

context('with minted tokens', 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.

Use 2 spaces for indentation in this context instead of 4.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be able to run npm run lint to autofix all formatting issues!

@Austinhs
Copy link
Contributor Author

Fixed the linting errors. Thanks for the npm run lint suggestion @johnnyshankman definitely saved a few mins of my time <3.

@ahbanavi , you mentioned adding tests with ERC721AOwnersExplicit did you just want it to be included in the mock or did you have something else in mind?

Sorry for the holdup on this branch, its been hectic month or so and just recently getting back to work on contract stuff again so I figured I'd finish this up as well as I knew it was still open.

@Vectorized
Copy link
Collaborator

I'll need to check with the team if they are ok with this extension.

We may consider a different name if we decide to go ahead with it.

@ahbanavi
Copy link
Contributor

@Austinhs glad to have you back
A lot has changed since you opened this PR.
We should change the base extension to work with _startTokenId and use createTestSuite in tests.
I work on this on my fork and open a PR to your branch; tests for ERC721AOwnersExplicit can wait.

Copy link
Contributor

@ahbanavi ahbanavi left a comment

Choose a reason for hiding this comment

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

IMO it's good to go
@Vectorized

@Vectorized
Copy link
Collaborator

Lemme sit on it for a while, probably will merge this weekend if team agrees.

@Vectorized Vectorized mentioned this pull request Apr 2, 2022
@Vectorized
Copy link
Collaborator

I've made a new PR that is based on this.

This (#114) will be merged first if #224 is approved.

@Vectorized Vectorized merged commit 76076c5 into chiru-labs:main Apr 4, 2022
@Austinhs Austinhs deleted the tokensOfOwner branch April 5, 2022 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants