PatrickAlphaC / hardhat-nft-fcc

100 stars 139 forks source link

fixed mintNft function typo #41

Closed RohanNero closed 1 year ago

RohanNero commented 2 years ago

Changing the function to increment the s_tokenCounter after the CreatedNFT() event is emitted so that the event is emitted with the correct tokenId. Another solution would be to ignore this PR and just change the emit to this:

emit CreatedNFT(s_tokenCounter - 1, highValue);
RoboCrypter commented 2 years ago

@RohanNero : This PR you have made, did you check it by running some tests, If not then you should!

RohanNero commented 2 years ago

@RohanNero : This PR you have made, did you check it by running some tests, If not then you should!

@ABossOfMyself

Yes, I have a separate copy of this repo that is filled with tests. I didn't submit those because I figured Patrick would prefer it if the repo stays similar to how it was in the video.

Here are some relevant unit tests:

it("should _safeMint NFT to the correct address", async function () {
            const startingValue = await dynamicSvg.balanceOf(deployer.address)
            await dynamicSvg.mintNft(HIGH_VALUE)
            const endingValue = await dynamicSvg.balanceOf(deployer.address)
            assert.equal(
                startingValue.add(1).toString(),
                endingValue.toString()
            )
        })
        it("should _safeMint NFT with correct tokenId", async function () {
            const ownerAddr = await dynamicSvg.ownerOf(0)
            //console.log(`Owner address: ${ownerAddr.toString()}`)
            assert.equal(ownerAddr, deployer.address)
        })
        // tokenId emitted is 1 because I already minted once in the beforeEach()
        it("should emit the CreatedNFT event correctly", async function () {
            const expectedValue = HIGH_VALUE + 1000000000
            await expect(dynamicSvg.mintNft(expectedValue))
                .to.emit(dynamicSvg, "CreatedNFT")
                .withArgs(1, expectedValue)
        })
RohanNero commented 2 years ago

Accidently closed PR by changing my forked branch's name.

PatrickAlphaC commented 1 year ago

To keep this similar to the video though, I'm going to close it. I do appreciate the PR tho!

RohanNero commented 1 year ago

Looking back on this PR, I understand not wanting to change s_tokenCounter = s_tokenCounter + 1; to s_tokenCounter ++; to keep it as similar to the video as possible. However, doesn't the CreatedNFT event emit the wrong tokenId since the s_tokenCounter value is incremented before the event?

I might make a new PR and add -1 to the s_tokenCounter value inside of the emit.