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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Chore: Use 0 instead of uint256.max as flag to signal minting new nft. Add totalSupply to CoverNFT #655

Closed
wants to merge 0 commits into from
Closed

Chore: Use 0 instead of uint256.max as flag to signal minting new nft. Add totalSupply to CoverNFT #655

wants to merge 0 commits into from

Conversation

0xdewy
Copy link
Contributor

@0xdewy 0xdewy commented Jan 24, 2023 •
edited

Context

Closes #619

Changes proposed in this pull request

This PR replaces the flag to mint a new NFT from uint256.max to 0 for CoverNFT and StakingNFT contracts.
TokenId 0 no longer exists, except as a reference for the staking pool manager
Added totalSupply( ) to CoverNFT
Also allocationID now also uses 0 to signal a new ID

Test plan

A test was added to make sure totalSupply() is working for Cover/Staking NFTs

Checklist

  • Rebased the base branch
  • Attached corresponding Github issue
  • Prefixed the name with the type of change (i.e. feat, chore, test)
  • Performed a self-review of my own code
  • Followed the style guidelines of this project
  • Made corresponding changes to the documentation
  • Didn't generate new warnings
  • Didn't generate failures on existing tests
  • Added tests that prove my fix is effective or that my feature works

Review

When reviewing a PR, please indicate intention in comments using the following emojis:

  • 馃嵃 = Nice to have but not essential.
  • 馃挕 = Suggestion or a comment based on personal opinion
  • 馃敤 = I believe this should be changed.
  • 馃 = I don鈥檛 understand something, do you mind giving me more context?
  • 馃殌 = Feedback

@shark0der shark0der changed the base branch from nexus-v2 to feature/simplify-pool-assets January 25, 2023 10:02
@roxdanila roxdanila linked an issue Jan 25, 2023 that may be closed by this pull request
Make first NFT token id 1 instead of zero (StakingNFT and CoverNFT) and add totalSupply() #619
Closed
@shark0der shark0der force-pushed the feature/simplify-pool-assets branch 4 times, most recently from 778a313 to 92fe1aa Compare January 26, 2023 12:31
Base automatically changed from feature/simplify-pool-assets to nexus-v2 January 26, 2023 12:46
@0xdewy 0xdewy force-pushed the chore/nft-id-start-at-1 branch 2 times, most recently from 59cb957 to b683df2 Compare January 26, 2023 22:05
@0xdewy 0xdewy marked this pull request as ready for review January 26, 2023 23:35
@0xdewy 0xdewy changed the title Chore: Make NFT id start at 1. Use 0 as flag for new NFT. Chore: Use 0 instead of uint256.max as flag to signal minting new nft. Add totalSupply to CoverNFT Jan 26, 2023
@shark0der shark0der force-pushed the chore/nft-id-start-at-1 branch 2 times, most recently from ad85dc0 to 59a3edf Compare January 31, 2023 14:54
@0xdewy 0xdewy requested a review from shark0der January 31, 2023 18:48
@0xdewy 0xdewy closed this Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roxdanila roxdanila Awaiting requested review from roxdanila

@patitonar patitonar Awaiting requested review from patitonar

@shark0der shark0der Awaiting requested review from shark0der

Assignees
No one assigned
Labels
in-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make first NFT token id 1 instead of zero (StakingNFT and CoverNFT) and add totalSupply()
2 participants
@0xdewy @shark0der