Add burn feature

[hskang9]

I tried to add burn feature for ERC721A contract. Currently address(0) is occupied for ownership inclusion, so I used address(1) for marking burned address.

[AndreiD]

lgtm

[hskang9]

@chiru-labs Is there a concern for this change?

[santiq]

is there a reason for this parameter not being used?

[hskang9]

I thought this was needed for receiver, but feel free to remove if it is not needed.

[hskang9]

I want to ask for _safeMint function as well

[MrMcGoats]

I think it should be kept; makes it easier for other implementations to pass/use arbitrary data, should they want to

[hskang9]

I agree

[santiq]

yup, I agree too, thanks for the clarification!

[cygaar]

Hey, thanks for the PR. We will have a more built out version of the repo soon after which we'll need you to write some tests for this. Stay tuned

[MrMcGoats]

Was just looking at this again, and it looks like _safeBurn(uint256) just calls itself

[hskang9]

Was just looking at this again, and it looks like _safeBurn(uint256) just calls itself

no @MrMcGoats, as you can see from @AndreiD, it has another function to cover implementation like _safeMint

[harry830622]

You should check tokenId+1 and update it accordingly just like transfer or the tokens after tokenId will be burnt as well.
Also using 0x0000...dead for burn address would be more intuitive IMO.

[harry830622]

In addition, most of the view functions need to be updated to consider the burnt tokens.

[hskang9]

You should check tokenId+1 and update it accordingly just like transfer or the tokens after tokenId will be burnt as well.
Also using 0x0000...dead for burn address would be more intuitive IMO.

This is true. I will add that

[hskang9]

In addition, most of the view functions need to be updated to consider the burnt tokens.

I am not sure if there is a view function to check burns in usual ERC721 though. For the backward compatibility, I will pass on this and let frontend handle.

[MrMcGoats]

Was just looking at this again, and it looks like _safeBurn(uint256) just calls itself

no @MrMcGoats, as you can see from @AndreiD, it has another function to cover implementation like _safeMint

@hskang9
I'm not seeing any other function defined as _safeBurn(uint256) apart from the one on line 331. However, if that exists, I don't see the point in wrapping it with a function that has the exact same name and parameters.

As far as I can tell, this is not done with _safeMint(address,uint256). The only implementation of that function that I can find (line 350) calls _safeMint(address,uint256,bytes)

[hskang9]

My bad, after looking at the code again, I missed adding "" for bytes argument for _safeBurn impl func, so I changed the code. Thank you for finding this.

[ahbanavi]

I think we should close this, cause #61 is more likely to be merged soon.

[cygaar]

Thanks for the contribution, closing this since we've merged in burn functionality